[PATCH V2 1/4] net/bonding: fix non-active slaves aren't stopped

[Ferruh Yigit]

On 3/24/2022 3:00 AM, Min Hu (Connor) wrote:
> From: Huisong Li <lihuisong at huawei.com>
> 
> When stopping a bonded port, all slaves should be deactivated. But only

s/deactivated/stopped/ ?

> active slaves are stopped. So fix it and do "deactivae_slave()" for active

s/deactivae_slave()/deactivate_slave()/

> slaves.

Hi Connor,

When a bonding port is closed, is it clear if all slave ports or active 
slave ports should be stopped?

> 
> Fixes: 0911d4ec0183 ("net/bonding: fix crash when stopping mode 4 port")
> Cc: stable at dpdk.org
> 
> Signed-off-by: Huisong Li <lihuisong at huawei.com>
> Signed-off-by: Min Hu (Connor) <humin29 at huawei.com>
> ---
>   drivers/net/bonding/rte_eth_bond_pmd.c | 20 +++++++++++---------
>   1 file changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
> index b305b6a35b..469dc71170 100644
> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
> @@ -2118,18 +2118,20 @@ bond_ethdev_stop(struct rte_eth_dev *eth_dev)
>   	internals->link_status_polling_enabled = 0;
>   	for (i = 0; i < internals->slave_count; i++) {
>   		uint16_t slave_id = internals->slaves[i].port_id;
> +
> +		internals->slaves[i].last_link_status = 0;
> +		ret = rte_eth_dev_stop(slave_id);
> +		if (ret != 0) {
> +			RTE_BOND_LOG(ERR, "Failed to stop device on port %u",
> +				     slave_id);
> +			return ret;

Should it return here or try to stop all ports?
What about to record the return status, but keep continue to stop all 
ports. And return error if any of the stop failed?

> +		}
> +
> +		/* active slaves need to deactivate. */

" active slaves need to be deactivated. " ?

>   		if (find_slave_by_id(internals->active_slaves,
>   				internals->active_slave_count, slave_id) !=
> -						internals->active_slave_count) {
> -			internals->slaves[i].last_link_status = 0;
> -			ret = rte_eth_dev_stop(slave_id);
> -			if (ret != 0) {
> -				RTE_BOND_LOG(ERR, "Failed to stop device on port %u",
> -					     slave_id);
> -				return ret;
> -			}
> +				internals->active_slave_count)

I think original indentation for this line is better.

>   			deactivate_slave(eth_dev, slave_id);
> -		}
>   	}
>   
>   	return 0;