|SUCCESS| [v2, 4/4] net/bonding: remove redundant function names from log

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/164555

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-05-29

git sha: Unknown - patch series from patchwork
Author: Stephen Hemminger <stephen at networkplumber.org>
Subject: [PATCH v2 1/4] net/bonding: make 8023ad enable function void

This patch series refactors the bonding driver to simplify error handling
and improve logging. The first patch changes bond_mode_8023ad_enable()
from returning int to void since it never fails.

> diff --git a/drivers/net/bonding/eth_bond_8023ad_private.h b/drivers/net/bonding/eth_bond_8023ad_private.h
> index ab7d15f81a..bd7a5848de 100644
> --- a/drivers/net/bonding/eth_bond_8023ad_private.h
> +++ b/drivers/net/bonding/eth_bond_8023ad_private.h
> @@ -209,25 +209,10 @@ bond_mode_8023ad_setup(struct rte_eth_dev *dev,
>   * @internal
>   *
>   * Enables 802.1AX mode and all active members on bonding interface.
> - *
> - * @param dev Bonding interface
> - * @return
> - *  0 on success, negative value otherwise.
>   */

[ ... ]

> diff --git a/drivers/net/bonding/rte_eth_bond_8023ad.c b/drivers/net/bonding/rte_eth_bond_8023ad.c
> index ba88f6d261..eba713e381 100644
> --- a/drivers/net/bonding/rte_eth_bond_8023ad.c
> +++ b/drivers/net/bonding/rte_eth_bond_8023ad.c
> @@ -1287,7 +1287,7 @@ bond_mode_8023ad_setup(struct rte_eth_dev *dev,
>  		bond_mode_8023ad_start(dev);
>  }
>  
> -int
> +void
>  bond_mode_8023ad_enable(struct rte_eth_dev *bond_dev)
>  {
>  	struct bond_dev_private *internals = bond_dev->data->dev_private;
> @@ -1296,8 +1296,6 @@ bond_mode_8023ad_enable(struct rte_eth_dev *bond_dev)
>  	for (i = 0; i < internals->active_member_count; i++)
>  		bond_mode_8023ad_activate_member(bond_dev,
>  				internals->active_members[i]);
> -
> -	return 0;
>  }

Does bond_mode_8023ad_activate_member() itself always succeed? If that
function can fail, should this code check the return value and handle
errors? If those failures matter, changing bond_mode_8023ad_enable() to
void discards error information.

> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
> index 96725071da..7fcb3ec7d7 100644
> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
> @@ -1619,8 +1619,7 @@ bond_ethdev_mode_set(struct rte_eth_dev *eth_dev, uint8_t mode)
>  		eth_dev->rx_pkt_burst = bond_ethdev_rx_burst;
>  		break;
>  	case BONDING_MODE_8023AD:
> -		if (bond_mode_8023ad_enable(eth_dev) != 0)
> -			return -1;
> +		bond_mode_8023ad_enable(eth_dev);

[ ... ]

> @@ -1641,13 +1640,13 @@ bond_ethdev_mode_set(struct rte_eth_dev *eth_dev, uint8_t mode)
>  		eth_dev->rx_pkt_burst = bond_ethdev_rx_burst_active_backup;
>  		break;
>  	case BONDING_MODE_ALB:
> -		if (bond_mode_alb_enable(eth_dev) != 0)
> -			return -1;
> +		bond_mode_alb_enable(eth_dev);
>  
>  		eth_dev->tx_pkt_burst = bond_ethdev_tx_burst_alb;
>  		eth_dev->rx_pkt_burst = bond_ethdev_rx_burst_alb;
>  		break;
>  	default:
> +		RTE_BOND_LOG(ERR, "Invalid mode %#x", mode);

In the BONDING_MODE_ALB case, was bond_mode_alb_enable() changed to
void in a prior commit, or is that being changed in this patch without
updating the header file? The patch only shows changes to the 8023ad
code.

---

git sha: Unknown - patch series from patchwork
Author: Stephen Hemminger <stephen at networkplumber.org>
Subject: [PATCH v2 2/4] net/bonding: check mode before setting dedicated queues

This patch adds validation to prevent enabling/disabling dedicated
queues when not in 802.3ad mode.

> diff --git a/drivers/net/bonding/rte_eth_bond_8023ad.c b/drivers/net/bonding/rte_eth_bond_8023ad.c
> index eba713e381..d1f30229d0 100644
> --- a/drivers/net/bonding/rte_eth_bond_8023ad.c
> +++ b/drivers/net/bonding/rte_eth_bond_8023ad.c
> @@ -1725,7 +1725,6 @@ RTE_EXPORT_SYMBOL(rte_eth_bond_8023ad_dedicated_queues_enable)
>  int
>  rte_eth_bond_8023ad_dedicated_queues_enable(uint16_t port)
>  {
> -	int retval = 0;
>  	struct rte_eth_dev *dev;
>  	struct bond_dev_private *internals;
>  
> @@ -1742,17 +1741,19 @@ rte_eth_bond_8023ad_dedicated_queues_enable(uint16_t port)
>  	if (dev->data->dev_started)
>  		return -1;
>  
> +	if (internals->mode != BONDING_MODE_8023AD)
> +		return -1;
> +
>  	internals->mode4.dedicated_queues.enabled = 1;
>  
>  	bond_ethdev_mode_set(dev, internals->mode);
> -	return retval;
> +	return 0;
>  }

This validation is good. Does bond_ethdev_mode_set() have failure paths
that should be checked? The original code had 'retval' but never
assigned anything except 0.

> @@ -1766,9 +1767,12 @@ rte_eth_bond_8023ad_dedicated_queues_disable(uint16_t port)
>  	if (dev->data->dev_started)
>  		return -1;
>  
> +	if (internals->mode != BONDING_MODE_8023AD)
> +		return -1;
> +
>  	internals->mode4.dedicated_queues.enabled = 0;
>  
>  	bond_ethdev_mode_set(dev, internals->mode);
>  
> -	return retval;
> +	return 0;
>  }

---

git sha: Unknown - patch series from patchwork
Author: Stephen Hemminger <stephen at networkplumber.org>
Subject: [PATCH v2 3/4] net/bonding: prevent crash on Rx/Tx from secondary process

This patch installs blackhole burst functions in secondary processes
to prevent NULL pointer dereferences.

> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
> index 7fcb3ec7d7..8a9c329fb8 100644
> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
> @@ -56,6 +56,33 @@ get_vlan_offset(struct rte_ether_hdr *eth_hdr, uint16_t *proto)
>  	return vlan_offset;
>  }
>  
> +static uint16_t
> +bond_ethdev_rx_secondary(void *queue __rte_unused,
> +			 struct rte_mbuf **bufs __rte_unused, uint16_t nb_pkts __rte_unused)
> +{
> +	static bool once = true;
> +
> +	if (once) {
> +		/* once per process is enough of a notice */
> +		RTE_BOND_LOG(ERR, "receive not supported in secondary");
> +		once = false;
> +	}
> +	return 0;
> +}
> +
> +static uint16_t
> +bond_ethdev_tx_secondary(void *queue __rte_unused, struct rte_mbuf **bufs, uint16_t nb_pkts)
> +{
> +	static bool once = true;
> +
> +	if (once) {
> +		RTE_BOND_LOG(ERR, "transmit not supported in secondary");
> +		once = false;
> +	}
> +	rte_pktmbuf_free_bulk(bufs, nb_pkts);
> +	return nb_pkts;
> +}

Is the 'once' variable thread-safe here? If multiple threads call these
functions simultaneously, could they both see once==true and both log,
or could there be a data race on the 'once' variable itself? The coding
style guide suggests avoiding static storage where thread safety matters.

[ ... ]

> @@ -3798,9 +3830,18 @@ bond_probe(struct rte_vdev_device *dev)
>  			RTE_BOND_LOG(ERR, "Failed to probe %s", name);
>  			return -1;
>  		}
> -		/* TODO: request info from primary to set up Rx and Tx */
> +
>  		eth_dev->dev_ops = &default_dev_ops;
>  		eth_dev->device = &dev->device;
> +
> +		/*
> +		 * Propagation of bond mode would require adding
> +		 * MP client/server support and lots of error handling.
> +		 *
> +		 * For now just install a black hole.
> +		 */
> +		eth_dev->tx_pkt_burst = bond_ethdev_tx_secondary;
> +		eth_dev->rx_pkt_burst = bond_ethdev_rx_secondary;
>  		rte_eth_dev_probing_finish(eth_dev);
>  		return 0;
>  	}

---

git sha: Unknown - patch series from patchwork
Author: Stephen Hemminger <stephen at networkplumber.org>
Subject: [PATCH v2 4/4] net/bonding: remove redundant function names from log

This patch removes redundant function names from log messages since
RTE_BOND_LOG already includes them.

> diff --git a/drivers/net/bonding/rte_eth_bond_api.c b/drivers/net/bonding/rte_eth_bond_api.c
> index 9e5df67c18..d9b6f1c417 100644
> --- a/drivers/net/bonding/rte_eth_bond_api.c
> +++ b/drivers/net/bonding/rte_eth_bond_api.c
> @@ -485,8 +485,8 @@ __eth_bond_member_add_lock_free(uint16_t bonding_port_id, uint16_t member_port_i
>  	ret = rte_eth_dev_info_get(member_port_id, &dev_info);
>  	if (ret != 0) {
>  		RTE_BOND_LOG(ERR,
> -			"%s: Error during getting device (port %u) info: %s",
> -			__func__, member_port_id, strerror(-ret));
> +			"Error getting device (port %u) info: %s",
> +			member_port_id, strerror(-ret));

[ ... ]