|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: ba88f6d261 (patch 1/3)
Author: Stephen Hemminger <stephen at networkplumber.org>
Subject: net/bonding: restore dedicated queue state on mode set error

The patch improves error handling in the dedicated queue enable/disable
functions by restoring the original state when bond_ethdev_mode_set()
fails.

> 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;

[ ... ]

> @@ -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 code violate C89 by mixing declaration and statements? The
coding style guide requires C89 compatibility. The 'uint8_t old'
declaration appears after the 'if (dev->data->dev_started)' statement.

>  	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;
>  }

[ ... ]

> @@ -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;

Does this code violate C89 by mixing declaration and statements? Same
issue as above - 'uint8_t old' appears after the conditional return.

>  	internals->mode4.dedicated_queues.enabled = 0;
> +	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;
>  }

---

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

This patch installs blackhole burst functions in secondary processes to
prevent NULL pointer crashes when calling rx_burst or tx_burst.

> 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;

Does this code have a thread-safety issue? Multiple threads could
simultaneously enter bond_ethdev_rx_secondary() and all see 'once' as
true, causing multiple log messages instead of the intended single
notice. Should this use rte_thread_once or atomic operations for proper
multi-thread synchronization?

> +
> +	if (once) {
> +		/* once per process is enough of a notice */
> +		RTE_BOND_LOG(NOTICE, "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;

Does this code have a thread-safety issue? Same concern as
bond_ethdev_rx_secondary() - multiple threads could race on the 'once'
flag.

> +
> +	if (once) {
> +		RTE_BOND_LOG(NOTICE, "transmit not supported in secondary");
> +		once = false;
> +	}
> +	rte_pktmbuf_free_bulk(bufs, nb_pkts);
> +	return nb_pkts;
> +}

[ ... ]

> @@ -1599,6 +1626,11 @@ bond_ethdev_mode_set(struct rte_eth_dev *eth_dev, uint8_t mode)
>  {
>  	struct bond_dev_private *internals;
>  
> +	if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
> +		RTE_BOND_LOG(ERR, "Setting mode in secondary not allowed");
> +		return -1;
> +	}

[ ... ]