On Fri, Apr 17, 2026 at 09:51:35AM -0700, Stephen Hemminger wrote:
> The calls to enable and disable dedicated queues are missing
> proper error handling. If setting bonding mode fails,
> restore original state and propagate the error return value.
>
> Fixes: 112891cd27e5 ("net/bonding: add dedicated HW queues for LACP control")
> Cc: [email protected]
>
> Signed-off-by: Stephen Hemminger <[email protected]>
> ---
> drivers/net/bonding/rte_eth_bond_8023ad.c | 19 ++++++++++++-------
> 1 file changed, 12 insertions(+), 7 deletions(-)
>
> 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;
Any particular reason for the variable rename? Not that it's a problem, but
it does expand the diff.
> 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;
Should you add a check that old != 1, and just return ok if so?
> internals->mode4.dedicated_queues.enabled = 1;
> + ret = bond_ethdev_mode_set(dev, internals->mode);
> + if (ret != 0)
> + internals->mode4.dedicated_queues.enabled = old;
>
Looking through the code, for 8023ad mode, I don't see any way of failure,
so this failure handling seems pointless. The function
bond_mode_8023ad_enable() loops through all the devices in the bond but
never checks for error for any of them, so always returns zero.
What is missing here is a check that the internals->mode is actually set to
8023ad.
Similar feedback applies to the disable function below.
/Bruce
