Re: [PATCH] net/failsafe: fix failsafe bus uninit return value

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

Hi Raslan,

On Tue, Aug 29, 2017 at 10:51:29AM +0300, Raslan Darawsheh wrote:
> fs_bus_uninit is always returning 0 no matter what was the status
> of each sub device bus_uninit value.

An error on fs_bus_uninit should not stop failsafe_eal_uninit from
finishing its operation.

So for the sake of internal consistency for this scope, it might be
acceptable to yield an error.

However, if you do so, you then need to ignore the error within
failsafe_eal_uninit, so that the fail-safe device state is also properly
set.

something like:

diff --git a/drivers/net/failsafe/failsafe_eal.c b/drivers/net/failsafe/failsafe_eal.c
index c8f4318..d144719 100644
--- a/drivers/net/failsafe/failsafe_eal.c
+++ b/drivers/net/failsafe/failsafe_eal.c
@@ -111,8 +111,6 @@ failsafe_eal_uninit(struct rte_eth_dev *dev)
        int ret;

        ret = fs_bus_uninit(dev);
-       if (ret)
-               return ret;
        PRIV(dev)->state = DEV_PROBED - 1;
-       return 0;
+       return ret;
}

> 
> Will now return the first sub device fail value in case it fails.
> 
> Fixes: a46f8d58 ("net/failsafe: add fail-safe PMD")
> 
> Signed-off-by: Raslan Darawsheh <rasland@mellanox.com>
> ---
>  drivers/net/failsafe/failsafe_eal.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/failsafe/failsafe_eal.c b/drivers/net/failsafe/failsafe_eal.c
> index c8f4318..4295347 100644
> --- a/drivers/net/failsafe/failsafe_eal.c
> +++ b/drivers/net/failsafe/failsafe_eal.c
> @@ -90,19 +90,20 @@ fs_bus_uninit(struct rte_eth_dev *dev)
>  {
>  	struct sub_device *sdev = NULL;
>  	uint8_t i;
> -	int ret;
> +	int ret = 0, sdev_ret;

No second declaration on the same line if one is initialized please.

>  
>  	FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_PROBED) {
> -		ret = rte_eal_hotplug_remove(sdev->bus->name,
> +		sdev_ret = rte_eal_hotplug_remove(sdev->bus->name,
>  					     sdev->dev->name);
> -		if (ret) {
> +		if (sdev_ret) {
>  			ERROR("Failed to remove requested device %s",
>  			      sdev->dev->name);

I think printing the error code might be useful, at least it would not
be completely lost.

> +			ret = (ret ? ret : sdev_ret);

ret is meaningless anyway, as it might represent any sub_device removal
error. I'd set it to -1 if sdev_ret is displayed to avoid any
unnecessary confusion.

>  			continue;
>  		}
>  		sdev->state = DEV_PROBED - 1;
>  	}
> -	return 0;
> +	return ret;
>  }
>  
>  int
> -- 
> 2.7.4
> 

Best regards,
-- 
Gaëtan Rivet
6WIND