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

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

[Raslan Darawsheh]

fs_bus_uninit is always returning 0 no matter what was the status
of each sub device bus_uninit value.

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;
 
 	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);
+			ret = (ret ? ret : sdev_ret);
 			continue;
 		}
 		sdev->state = DEV_PROBED - 1;
 	}
-	return 0;
+	return ret;
 }
 
 int
-- 
2.7.4

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

[Gaëtan Rivet]

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

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

[Raslan Darawsheh]

fs_bus_uninit is always returning 0 no matter what was the status
of each sub device bus_uninit value.

Fixes: a46f8d58 ("net/failsafe: add fail-safe PMD")

Signed-off-by: Raslan Darawsheh <rasland@mellanox.com>
---
 drivers/net/failsafe/failsafe_eal.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/net/failsafe/failsafe_eal.c b/drivers/net/failsafe/failsafe_eal.c
index c8f4318..f6e44ca 100644
--- a/drivers/net/failsafe/failsafe_eal.c
+++ b/drivers/net/failsafe/failsafe_eal.c
@@ -90,19 +90,21 @@ fs_bus_uninit(struct rte_eth_dev *dev)
 {
 	struct sub_device *sdev = NULL;
 	uint8_t i;
-	int ret;
+	int sdev_ret;
+	int ret = 0;
 
 	FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_PROBED) {
-		ret = rte_eal_hotplug_remove(sdev->bus->name,
-					     sdev->dev->name);
-		if (ret) {
-			ERROR("Failed to remove requested device %s",
-			      sdev->dev->name);
+		sdev_ret = rte_eal_hotplug_remove(sdev->bus->name,
+							sdev->dev->name);
+		if (sdev_ret) {
+			ERROR("Failed to remove requested device %s"
+			      "(errno: %d)", sdev->dev->name, sdev_ret);
+			ret = -1;
 			continue;
 		}
 		sdev->state = DEV_PROBED - 1;
 	}
-	return 0;
+	return ret;
 }
 
 int
@@ -111,8 +113,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;
 }
-- 
2.7.4

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

[Gaëtan Rivet]

On Tue, Aug 29, 2017 at 11:55:45AM +0300, Raslan Darawsheh wrote:
> fs_bus_uninit is always returning 0 no matter what was the status
> of each sub device bus_uninit value.
> 
> Fixes: a46f8d58 ("net/failsafe: add fail-safe PMD")
> 
> Signed-off-by: Raslan Darawsheh <rasland@mellanox.com>
> ---
>  drivers/net/failsafe/failsafe_eal.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/failsafe/failsafe_eal.c b/drivers/net/failsafe/failsafe_eal.c
> index c8f4318..f6e44ca 100644
> --- a/drivers/net/failsafe/failsafe_eal.c
> +++ b/drivers/net/failsafe/failsafe_eal.c
> @@ -90,19 +90,21 @@ fs_bus_uninit(struct rte_eth_dev *dev)
>  {
>  	struct sub_device *sdev = NULL;
>  	uint8_t i;
> -	int ret;
> +	int sdev_ret;
> +	int ret = 0;
>  
>  	FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_PROBED) {
> -		ret = rte_eal_hotplug_remove(sdev->bus->name,
> -					     sdev->dev->name);
> -		if (ret) {
> -			ERROR("Failed to remove requested device %s",
> -			      sdev->dev->name);
> +		sdev_ret = rte_eal_hotplug_remove(sdev->bus->name,
> +							sdev->dev->name);
> +		if (sdev_ret) {
> +			ERROR("Failed to remove requested device %s"
> +			      "(errno: %d)", sdev->dev->name, sdev_ret);

The error string should be on one line to help grepping the issue.
The code displayed is not errno, it should not be named as such.


+			ERROR("Failed to remove requested device %s (%d)",
+			      sdev->dev->name, sdev_ret);

> +			ret = -1;
>  			continue;
>  		}
>  		sdev->state = DEV_PROBED - 1;
>  	}
> -	return 0;
> +	return ret;
>  }
>  
>  int
> @@ -111,8 +113,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;
>  }
> -- 
> 2.7.4
> 

Otherwise,
Acked-by: Gaetan Rivet <gaetan.rivet@6wind.com>

-- 
Gaëtan Rivet
6WIND

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

[Raslan Darawsheh]

fs_bus_uninit is always returning 0 no matter what was the status
of each sub device bus_uninit value.

Fixes: a46f8d58 ("net/failsafe: add fail-safe PMD")

Signed-off-by: Raslan Darawsheh <rasland@mellanox.com>
---
 drivers/net/failsafe/failsafe_eal.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/net/failsafe/failsafe_eal.c b/drivers/net/failsafe/failsafe_eal.c
index c8f4318..aeb87a0 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 sdev_ret;
+	int ret = 0;
 
 	FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_PROBED) {
-		ret = rte_eal_hotplug_remove(sdev->bus->name,
-					     sdev->dev->name);
-		if (ret) {
-			ERROR("Failed to remove requested device %s",
-			      sdev->dev->name);
+		sdev_ret = rte_eal_hotplug_remove(sdev->bus->name,
+							sdev->dev->name);
+		if (sdev_ret) {
+			ERROR("Failed to remove requested device %s (err: %d)",
+			      sdev->dev->name, sdev_ret);
 			continue;
 		}
 		sdev->state = DEV_PROBED - 1;
 	}
-	return 0;
+	return ret;
 }
 
 int
@@ -111,8 +112,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;
 }
-- 
2.7.4

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

[Gaëtan Rivet]

On Tue, Aug 29, 2017 at 12:08:08PM +0300, Raslan Darawsheh wrote:
> fs_bus_uninit is always returning 0 no matter what was the status
> of each sub device bus_uninit value.
> 
> Fixes: a46f8d58 ("net/failsafe: add fail-safe PMD")
> 

No need to send another version, but for next time: you could have
included my ack at this point :).

> Signed-off-by: Raslan Darawsheh <rasland@mellanox.com>
Acked-by: Gaetan Rivet <gaetan.rivet@6wind.com>

Thanks,

> ---
>  drivers/net/failsafe/failsafe_eal.c | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/failsafe/failsafe_eal.c b/drivers/net/failsafe/failsafe_eal.c
> index c8f4318..aeb87a0 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 sdev_ret;
> +	int ret = 0;
>  
>  	FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_PROBED) {
> -		ret = rte_eal_hotplug_remove(sdev->bus->name,
> -					     sdev->dev->name);
> -		if (ret) {
> -			ERROR("Failed to remove requested device %s",
> -			      sdev->dev->name);
> +		sdev_ret = rte_eal_hotplug_remove(sdev->bus->name,
> +							sdev->dev->name);
> +		if (sdev_ret) {
> +			ERROR("Failed to remove requested device %s (err: %d)",
> +			      sdev->dev->name, sdev_ret);
>  			continue;
>  		}
>  		sdev->state = DEV_PROBED - 1;
>  	}
> -	return 0;
> +	return ret;
>  }
>  
>  int
> @@ -111,8 +112,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;
>  }
> -- 
> 2.7.4
> 

-- 
Gaëtan Rivet
6WIND

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

[Ferruh Yigit]

On 8/29/2017 10:28 AM, Gaëtan Rivet wrote:
> On Tue, Aug 29, 2017 at 12:08:08PM +0300, Raslan Darawsheh wrote:
>> fs_bus_uninit is always returning 0 no matter what was the status
>> of each sub device bus_uninit value.
>>
>> Fixes: a46f8d58 ("net/failsafe: add fail-safe PMD")
Cc: stable@dpdk.org
>>
>> Signed-off-by: Raslan Darawsheh <rasland@mellanox.com>
> Acked-by: Gaetan Rivet <gaetan.rivet@6wind.com>

Applied to dpdk-next-net/master, thanks.