diff mbox

[dpdk-dev,v2] net/failsafe: fix tx sub device deactivating

Message ID 1502893168-61001-1-git-send-email-matan@mellanox.com (mailing list archive)
State Accepted, archived
Delegated to: Ferruh Yigit
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Matan Azrad Aug. 16, 2017, 2:19 p.m. UTC 
  The corrupted code couldn't recognize that all sub devices
were not ready for tx traffic when failsafe PMD was trying
to switch device because of an unreachable condition using.

Hence, the current tx sub device variable was not updated
correctly.

The fix removed the unreachable branch and added new one
in the right place respecting the original intent.

Fixes: ebea83f899d8 ("net/failsafe: add plug-in support")
Fixes: 598fb8aec6f6 ("net/failsafe: support device removal")
Cc: stable@dpdk.org

Signed-off-by: Matan Azrad <matan@mellanox.com>
Acked-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 drivers/net/failsafe/failsafe_private.h | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

Comments

Gaëtan Rivet Aug. 16, 2017, 2:39 p.m. UTC | #1 
On Wed, Aug 16, 2017 at 05:19:28PM +0300, Matan Azrad wrote:
> The corrupted code couldn't recognize that all sub devices
> were not ready for tx traffic when failsafe PMD was trying
> to switch device because of an unreachable condition using.
> 
> Hence, the current tx sub device variable was not updated
> correctly.
> 
> The fix removed the unreachable branch and added new one
> in the right place respecting the original intent.
> 
> Fixes: ebea83f899d8 ("net/failsafe: add plug-in support")
> Fixes: 598fb8aec6f6 ("net/failsafe: support device removal")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Matan Azrad <matan@mellanox.com>
> Acked-by: Gaetan Rivet <gaetan.rivet@6wind.com>
> ---
>  drivers/net/failsafe/failsafe_private.h | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/failsafe/failsafe_private.h b/drivers/net/failsafe/failsafe_private.h
> index 0361cf4..ef646db 100644
> --- a/drivers/net/failsafe/failsafe_private.h
> +++ b/drivers/net/failsafe/failsafe_private.h
> @@ -334,7 +334,7 @@ fs_switch_dev(struct rte_eth_dev *dev,
>  	} else if ((txd && txd->state < req_state) ||
>  		   txd == NULL ||
>  		   txd == banned) {
> -		struct sub_device *sdev;
> +		struct sub_device *sdev = NULL;

Good catch, actually this makes me think that the FOREACH_SUBDEV_STATE
macro is not following the usual tailq API (which sets the iterator to
NULL upon terminating). This can throw-off the unsuspecting writer.

I will fix this soon™. In the meantime, sdev should be initialized to
NULL.

>  		uint8_t i;
>  
>  		/* Using acceptable device */
> @@ -346,9 +346,10 @@ fs_switch_dev(struct rte_eth_dev *dev,
>  			PRIV(dev)->subs_tx = i;
>  			break;
>  		}
> -	} else if (txd && txd->state < req_state) {
> -		DEBUG("No device ready, deactivating tx_dev");
> -		PRIV(dev)->subs_tx = PRIV(dev)->subs_tail;
> +		if (i >= PRIV(dev)->subs_tail || sdev == NULL) {
> +			DEBUG("No device ready, deactivating tx_dev");
> +			PRIV(dev)->subs_tx = PRIV(dev)->subs_tail;
> +		}
>  	} else {
>  		return;
>  	}
> -- 
> 2.7.4
>
Matan Azrad Aug. 16, 2017, 2:53 p.m. UTC | #2 
Hi

> -----Original Message-----

> From: Gaëtan Rivet [mailto:gaetan.rivet@6wind.com]

> Sent: Wednesday, August 16, 2017 5:39 PM

> To: Matan Azrad <matan@mellanox.com>

> Cc: dev@dpdk.org; stable@dpdk.org

> Subject: Re: [PATCH v2] net/failsafe: fix tx sub device deactivating

> 

> On Wed, Aug 16, 2017 at 05:19:28PM +0300, Matan Azrad wrote:

> > The corrupted code couldn't recognize that all sub devices were not

> > ready for tx traffic when failsafe PMD was trying to switch device

> > because of an unreachable condition using.

> >

> > Hence, the current tx sub device variable was not updated correctly.

> >

> > The fix removed the unreachable branch and added new one in the right

> > place respecting the original intent.

> >

> > Fixes: ebea83f899d8 ("net/failsafe: add plug-in support")

> > Fixes: 598fb8aec6f6 ("net/failsafe: support device removal")

> > Cc: stable@dpdk.org

> >

> > Signed-off-by: Matan Azrad <matan@mellanox.com>

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

> > ---

> >  drivers/net/failsafe/failsafe_private.h | 9 +++++----

> >  1 file changed, 5 insertions(+), 4 deletions(-)

> >

> > diff --git a/drivers/net/failsafe/failsafe_private.h

> > b/drivers/net/failsafe/failsafe_private.h

> > index 0361cf4..ef646db 100644

> > --- a/drivers/net/failsafe/failsafe_private.h

> > +++ b/drivers/net/failsafe/failsafe_private.h

> > @@ -334,7 +334,7 @@ fs_switch_dev(struct rte_eth_dev *dev,

> >  	} else if ((txd && txd->state < req_state) ||

> >  		   txd == NULL ||

> >  		   txd == banned) {

> > -		struct sub_device *sdev;

> > +		struct sub_device *sdev = NULL;

> 

> Good catch, actually this makes me think that the FOREACH_SUBDEV_STATE

> macro is not following the usual tailq API (which sets the iterator to NULL

> upon terminating). This can throw-off the unsuspecting writer.


I think you right.
Else, you should go all over this macro using and initiate sdev to NULL.

> 

> I will fix this soon™. In the meantime, sdev should be initialized to NULL.


Great 

> 

> >  		uint8_t i;

> >

> >  		/* Using acceptable device */

> > @@ -346,9 +346,10 @@ fs_switch_dev(struct rte_eth_dev *dev,

> >  			PRIV(dev)->subs_tx = i;

> >  			break;

> >  		}

> > -	} else if (txd && txd->state < req_state) {

> > -		DEBUG("No device ready, deactivating tx_dev");

> > -		PRIV(dev)->subs_tx = PRIV(dev)->subs_tail;

> > +		if (i >= PRIV(dev)->subs_tail || sdev == NULL) {

> > +			DEBUG("No device ready, deactivating tx_dev");

> > +			PRIV(dev)->subs_tx = PRIV(dev)->subs_tail;

> > +		}

> >  	} else {

> >  		return;

> >  	}

> > --

> > 2.7.4

> >

> 

> --

> Gaëtan Rivet

> 6WIND
Ferruh Yigit Aug. 21, 2017, 12:56 p.m. UTC | #3 
On 8/16/2017 3:19 PM, Matan Azrad wrote:
> The corrupted code couldn't recognize that all sub devices
> were not ready for tx traffic when failsafe PMD was trying
> to switch device because of an unreachable condition using.
> 
> Hence, the current tx sub device variable was not updated
> correctly.
> 
> The fix removed the unreachable branch and added new one
> in the right place respecting the original intent.
> 
> Fixes: ebea83f899d8 ("net/failsafe: add plug-in support")
> Fixes: 598fb8aec6f6 ("net/failsafe: support device removal")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Matan Azrad <matan@mellanox.com>
> Acked-by: Gaetan Rivet <gaetan.rivet@6wind.com>

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

Patch

diff --git a/drivers/net/failsafe/failsafe_private.h b/drivers/net/failsafe/failsafe_private.h
index 0361cf4..ef646db 100644
--- a/drivers/net/failsafe/failsafe_private.h
+++ b/drivers/net/failsafe/failsafe_private.h
@@ -334,7 +334,7 @@  fs_switch_dev(struct rte_eth_dev *dev,
 	} else if ((txd && txd->state < req_state) ||
 		   txd == NULL ||
 		   txd == banned) {
-		struct sub_device *sdev;
+		struct sub_device *sdev = NULL;
 		uint8_t i;
 
 		/* Using acceptable device */
@@ -346,9 +346,10 @@  fs_switch_dev(struct rte_eth_dev *dev,
 			PRIV(dev)->subs_tx = i;
 			break;
 		}
-	} else if (txd && txd->state < req_state) {
-		DEBUG("No device ready, deactivating tx_dev");
-		PRIV(dev)->subs_tx = PRIV(dev)->subs_tail;
+		if (i >= PRIV(dev)->subs_tail || sdev == NULL) {
+			DEBUG("No device ready, deactivating tx_dev");
+			PRIV(dev)->subs_tx = PRIV(dev)->subs_tail;
+		}
 	} else {
 		return;
 	}