Re: [dpdk-dev] [PATCH 2/7] net/failsafe: check code of allmulticast mode switch

["Gaëtan Rivet" <gaetan.rivet@6wind.com>]

On Mon, Sep 09, 2019 at 01:13:04PM +0100, Andrew Rybchenko wrote:
> From: Ivan Ilchenko <Ivan.Ilchenko@oktetlabs.ru>
> 
> rte_eth_allmulticast_enable()/rte_eth_allmulticast_disable() return
> value was changed from void to int, so this patch modify usage
> of these functions across net/failsafe according to new return type.
> 
> Try to keep all-multicast mode consistent across all active
> devices in the case of failure.
> 
> Signed-off-by: Ivan Ilchenko <Ivan.Ilchenko@oktetlabs.ru>
> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>

A small nit below, but otherwise
Acked-by: Gaetan Rivet <gaetan.rivet@6wind.com>

> ---
>  drivers/net/failsafe/failsafe_ether.c |  8 +++--
>  drivers/net/failsafe/failsafe_ops.c   | 44 ++++++++++++++++++++++++---
>  2 files changed, 46 insertions(+), 6 deletions(-)
> 
[...]
> diff --git a/drivers/net/failsafe/failsafe_ops.c b/drivers/net/failsafe/failsafe_ops.c
> index fee783ad07..b90fa8676c 100644
> --- a/drivers/net/failsafe/failsafe_ops.c
> +++ b/drivers/net/failsafe/failsafe_ops.c
> @@ -723,10 +723,28 @@ fs_allmulticast_enable(struct rte_eth_dev *dev)
>  {
>  	struct sub_device *sdev;
>  	uint8_t i;
> +	int ret = 0;
>  
>  	fs_lock(dev, 0);
> -	FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE)
> -		rte_eth_allmulticast_enable(PORT_ID(sdev));
> +	FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) {
> +		ret = rte_eth_allmulticast_enable(PORT_ID(sdev));
> +		ret = fs_err(sdev, ret);
> +		if (ret != 0) {
> +			ERROR("All-multicast mode enable failed for subdevice %d",
> +				PORT_ID(sdev));
> +			break;
> +		}
> +	}
> +	if (ret != 0) {
> +		/* Rollback in the case of failure */
> +		FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) {
> +			ret = rte_eth_allmulticast_disable(PORT_ID(sdev));
> +			ret = fs_err(sdev, ret);
> +			if (ret != 0)
> +				ERROR("All-multicast mode disable to rollback failed for subdevice %d",

I'd formulate the error as such:

"All-multicast mode disable during rollback failed for subdevice %d"

That may not be the best either, but the current format seems a little
off? I could be wrong.

> +					PORT_ID(sdev));
> +		}
> +	}
>  	fs_unlock(dev, 0);
>  }
>  
> @@ -735,10 +753,28 @@ fs_allmulticast_disable(struct rte_eth_dev *dev)
>  {
>  	struct sub_device *sdev;
>  	uint8_t i;
> +	int ret = 0;
>  
>  	fs_lock(dev, 0);
> -	FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE)
> -		rte_eth_allmulticast_disable(PORT_ID(sdev));
> +	FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) {
> +		ret = rte_eth_allmulticast_disable(PORT_ID(sdev));
> +		ret = fs_err(sdev, ret);
> +		if (ret != 0) {
> +			ERROR("All-multicast mode disable failed for subdevice %d",
> +				PORT_ID(sdev));
> +			break;
> +		}
> +	}
> +	if (ret != 0) {
> +		/* Rollback in the case of failure */
> +		FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) {
> +			ret = rte_eth_allmulticast_enable(PORT_ID(sdev));
> +			ret = fs_err(sdev, ret);
> +			if (ret != 0)
> +				ERROR("All-multicast mode enable to rollback failed for subdevice %d",

Same as above.

> +					PORT_ID(sdev));
> +		}
> +	}
>  	fs_unlock(dev, 0);
>  }
>  
> -- 
> 2.17.1
> 

-- 
Gaëtan Rivet
6WIND