|SUCCESS| [3/3] 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/163177

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-04-17

Commit: Unknown (from patch series)
Author: Stephen Hemminger <stephen at networkplumber.org>
Subject: [PATCH 1/3] net/bonding: restore dedicated queue state on mode set error

This patch fixes error handling for enable/disable dedicated queues
functions. If bond_ethdev_mode_set() fails, the original state is
restored before returning the error.

> diff --git a/drivers/net/bonding/rte_eth_bond_8023ad.c b/drivers/net/bonding/rte_eth_bond_8023ad.c
> index ba88f6d261..a74b0059ac 100644
> --- a/drivers/net/bonding/rte_eth_bond_8023ad.c
> +++ b/drivers/net/bonding/rte_eth_bond_8023ad.c
> @@ -1727,7 +1727,7 @@ RTE_EXPORT_SYMBOL(rte_eth_bond_8023ad_dedicated_queues_enable)
>  int
>  rte_eth_bond_8023ad_dedicated_queues_enable(uint16_t port)
>  {
> -	int retval = 0;
> +	int ret;
>  	struct rte_eth_dev *dev;
>  	struct bond_dev_private *internals;
>  
> @@ -1744,17 +1744,20 @@ rte_eth_bond_8023ad_dedicated_queues_enable(uint16_t port)
>  	if (dev->data->dev_started)
>  		return -1;
>  
> +	uint8_t old = internals->mode4.dedicated_queues.enabled;

Does this violate C90 by declaring variables mid-function? The DPDK
coding style requires declarations at the start of a block.

>  	internals->mode4.dedicated_queues.enabled = 1;
> +	ret = bond_ethdev_mode_set(dev, internals->mode);
> +	if (ret != 0)
> +		internals->mode4.dedicated_queues.enabled = old;
>  
> -	bond_ethdev_mode_set(dev, internals->mode);
> -	return retval;
> +	return ret;
>  }
>  
>  RTE_EXPORT_SYMBOL(rte_eth_bond_8023ad_dedicated_queues_disable)
>  int
>  rte_eth_bond_8023ad_dedicated_queues_disable(uint16_t port)
>  {
> -	int retval = 0;
> +	int ret;
>  	struct rte_eth_dev *dev;
>  	struct bond_dev_private *internals;
>  
> @@ -1768,9 +1771,11 @@ rte_eth_bond_8023ad_dedicated_queues_disable(uint16_t port)
>  	if (dev->data->dev_started)
>  		return -1;
>  
> +	uint8_t old = internals->mode4.dedicated_queues.enabled;

Same concern here regarding variable declaration placement.

[ ... ]

Commit: Unknown (from patch series)
Author: Stephen Hemminger <stephen at networkplumber.org>
Subject: [PATCH 2/3] net/bonding: prevent crash on Rx/Tx from secondary process

This patch prevents NULL pointer crashes when secondary processes attempt
Rx/Tx on bonding devices by installing blackhole stub functions instead.

> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
> index 96725071da..6a42257d2b 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(NOTICE, "receive not supported in secondary");
> +		once = false;
> +	}
> +	return 0;
> +}

Does this code have a race condition? If two threads from the secondary
process call bond_ethdev_rx_secondary() concurrently, can both threads
see once==true and both emit the log message?

> +
> +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(NOTICE, "transmit not supported in secondary");
> +		once = false;
> +	}
> +	rte_pktmbuf_free_bulk(bufs, nb_pkts);
> +	return nb_pkts;
> +}

Same potential race condition here with the static once flag.

[ ... ]

Commit: Unknown (from patch series)
Author: Stephen Hemminger <stephen at networkplumber.org>
Subject: [PATCH 3/3] net/bonding: remove redundant function names from log

This patch removes manual function name prefixes from log messages since
RTE_BOND_LOG already includes them.

No concerns found in this patch.