Re: [PATCH net-next] switchdev: fix BUG when port driver doesn't support set attr op
Thu, Jun 11, 2015 at 05:03:21PM CEST, sfel...@gmail.com wrote: >On Wed, Jun 10, 2015 at 11:16 PM, Jiri Pirko wrote: >> Thu, Jun 11, 2015 at 12:00:47AM CEST, sfel...@gmail.com wrote: >>>On Wed, Jun 10, 2015 at 2:47 PM, Scott Feldman wrote: On Wed, Jun 10, 2015 at 2:25 PM, David Ahern wrote: > On 6/10/15 2:56 PM, sfel...@gmail.com wrote: >> >> From: Scott Feldman >> >> Fix a BUG() where CONFIG_NET_SWITCHDEV is set but the driver for a >> bridged >> port does not support switchdec_port_attr_set op. Don't BUG() if >> -EOPNOTSUPP is returned. >> >> Signed-off-by: Scott Feldman >> Reported-by: Brenden Blanco >> --- >> net/switchdev/switchdev.c |2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c >> index e008057..99bced4 100644 >> --- a/net/switchdev/switchdev.c >> +++ b/net/switchdev/switchdev.c >> @@ -103,7 +103,7 @@ static void switchdev_port_attr_set_work(struct >> work_struct *work) >> >> rtnl_lock(); >> err = switchdev_port_attr_set(asw->dev, &asw->attr); >> - BUG_ON(err); >> + BUG_ON(err && err != -EOPNOTSUPP); >> rtnl_unlock(); >> >> dev_put(asw->dev); >> > > Should that be WARN_ON instead of BUG_ON? I think I had it as WARN when we were working on the initial patches, but we changed it to BUG_ON because we should only get an error here if the driver screwed something up between PREPARE phase and COMMIT phase, so it should be considered a driver bug which needs fixing. >>> >>>Actually, ignore what I said above. I was confusing this BUG_ON with >>>the one in switchdev_port_attr_set(). Perhaps this BUG_ON() you're >>>commenting on should be WARN(). A driver could return an err in >>>PREPARE phase and that shouldn't be a BUG_ON situation; seems WARN >>>would be better. It the case where the driver returns an err in >>>COMMIT phase but didn't return an err in PREPARE phase we want to >>>BUG_ON(). Maybe that case doesn't justify BUG_ON either, based on the >>>link you posted. >>> >>>Jiri, IIRC, you suggested the BUG_ON(). Does it still sound right >>>based on the point David is raising? >> >> Hmm, looking at code of switchdev_port_attr_set. In case that fails in >> prepare state (which can easily happen for example due to -ENOMEM) this >> BUG_ON is hit as well. That is not right. I think we should change it >> just to warning. Also I think that prink (or a flavour) is more suitable >> here than WARN. > >Thanks, I'll change it to netdev_err. > >> Btw, why switchdev_port_obj_add has WARN and not BUG ? > >Not sure. We should be consistent. WARN seems better for both >obj_add/attr_set than BUG, given the link David Ahern posted. Do you >agree? Okay. Sounds reasonable. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net-next] switchdev: fix BUG when port driver doesn't support set attr op
On Wed, Jun 10, 2015 at 11:16 PM, Jiri Pirko wrote: > Thu, Jun 11, 2015 at 12:00:47AM CEST, sfel...@gmail.com wrote: >>On Wed, Jun 10, 2015 at 2:47 PM, Scott Feldman wrote: >>> On Wed, Jun 10, 2015 at 2:25 PM, David Ahern wrote: On 6/10/15 2:56 PM, sfel...@gmail.com wrote: > > From: Scott Feldman > > Fix a BUG() where CONFIG_NET_SWITCHDEV is set but the driver for a bridged > port does not support switchdec_port_attr_set op. Don't BUG() if > -EOPNOTSUPP is returned. > > Signed-off-by: Scott Feldman > Reported-by: Brenden Blanco > --- > net/switchdev/switchdev.c |2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c > index e008057..99bced4 100644 > --- a/net/switchdev/switchdev.c > +++ b/net/switchdev/switchdev.c > @@ -103,7 +103,7 @@ static void switchdev_port_attr_set_work(struct > work_struct *work) > > rtnl_lock(); > err = switchdev_port_attr_set(asw->dev, &asw->attr); > - BUG_ON(err); > + BUG_ON(err && err != -EOPNOTSUPP); > rtnl_unlock(); > > dev_put(asw->dev); > Should that be WARN_ON instead of BUG_ON? >>> >>> I think I had it as WARN when we were working on the initial patches, >>> but we changed it to BUG_ON because we should only get an error here >>> if the driver screwed something up between PREPARE phase and COMMIT >>> phase, so it should be considered a driver bug which needs fixing. >> >>Actually, ignore what I said above. I was confusing this BUG_ON with >>the one in switchdev_port_attr_set(). Perhaps this BUG_ON() you're >>commenting on should be WARN(). A driver could return an err in >>PREPARE phase and that shouldn't be a BUG_ON situation; seems WARN >>would be better. It the case where the driver returns an err in >>COMMIT phase but didn't return an err in PREPARE phase we want to >>BUG_ON(). Maybe that case doesn't justify BUG_ON either, based on the >>link you posted. >> >>Jiri, IIRC, you suggested the BUG_ON(). Does it still sound right >>based on the point David is raising? > > Hmm, looking at code of switchdev_port_attr_set. In case that fails in > prepare state (which can easily happen for example due to -ENOMEM) this > BUG_ON is hit as well. That is not right. I think we should change it > just to warning. Also I think that prink (or a flavour) is more suitable > here than WARN. Thanks, I'll change it to netdev_err. > Btw, why switchdev_port_obj_add has WARN and not BUG ? Not sure. We should be consistent. WARN seems better for both obj_add/attr_set than BUG, given the link David Ahern posted. Do you agree? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net-next] switchdev: fix BUG when port driver doesn't support set attr op
From: sfel...@gmail.com Date: Wed, 10 Jun 2015 13:56:02 -0700 > From: Scott Feldman > > Fix a BUG() where CONFIG_NET_SWITCHDEV is set but the driver for a bridged > port does not support switchdec_port_attr_set op. Don't BUG() if > -EOPNOTSUPP is returned. > > Signed-off-by: Scott Feldman > Reported-by: Brenden Blanco > --- > net/switchdev/switchdev.c |2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c > index e008057..99bced4 100644 > --- a/net/switchdev/switchdev.c > +++ b/net/switchdev/switchdev.c > @@ -103,7 +103,7 @@ static void switchdev_port_attr_set_work(struct > work_struct *work) > > rtnl_lock(); > err = switchdev_port_attr_set(asw->dev, &asw->attr); > - BUG_ON(err); > + BUG_ON(err && err != -EOPNOTSUPP); > rtnl_unlock(); > > dev_put(asw->dev); I agree with other feedback, in that this function can return other "normal" errors like -ENOMEM and that's not absolutely not BUG_ON() material. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net-next] switchdev: fix BUG when port driver doesn't support set attr op
Thu, Jun 11, 2015 at 12:00:47AM CEST, sfel...@gmail.com wrote: >On Wed, Jun 10, 2015 at 2:47 PM, Scott Feldman wrote: >> On Wed, Jun 10, 2015 at 2:25 PM, David Ahern wrote: >>> On 6/10/15 2:56 PM, sfel...@gmail.com wrote: From: Scott Feldman Fix a BUG() where CONFIG_NET_SWITCHDEV is set but the driver for a bridged port does not support switchdec_port_attr_set op. Don't BUG() if -EOPNOTSUPP is returned. Signed-off-by: Scott Feldman Reported-by: Brenden Blanco --- net/switchdev/switchdev.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c index e008057..99bced4 100644 --- a/net/switchdev/switchdev.c +++ b/net/switchdev/switchdev.c @@ -103,7 +103,7 @@ static void switchdev_port_attr_set_work(struct work_struct *work) rtnl_lock(); err = switchdev_port_attr_set(asw->dev, &asw->attr); - BUG_ON(err); + BUG_ON(err && err != -EOPNOTSUPP); rtnl_unlock(); dev_put(asw->dev); >>> >>> Should that be WARN_ON instead of BUG_ON? >> >> I think I had it as WARN when we were working on the initial patches, >> but we changed it to BUG_ON because we should only get an error here >> if the driver screwed something up between PREPARE phase and COMMIT >> phase, so it should be considered a driver bug which needs fixing. > >Actually, ignore what I said above. I was confusing this BUG_ON with >the one in switchdev_port_attr_set(). Perhaps this BUG_ON() you're >commenting on should be WARN(). A driver could return an err in >PREPARE phase and that shouldn't be a BUG_ON situation; seems WARN >would be better. It the case where the driver returns an err in >COMMIT phase but didn't return an err in PREPARE phase we want to >BUG_ON(). Maybe that case doesn't justify BUG_ON either, based on the >link you posted. > >Jiri, IIRC, you suggested the BUG_ON(). Does it still sound right >based on the point David is raising? Hmm, looking at code of switchdev_port_attr_set. In case that fails in prepare state (which can easily happen for example due to -ENOMEM) this BUG_ON is hit as well. That is not right. I think we should change it just to warning. Also I think that prink (or a flavour) is more suitable here than WARN. Btw, why switchdev_port_obj_add has WARN and not BUG ? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net-next] switchdev: fix BUG when port driver doesn't support set attr op
On Wed, Jun 10, 2015 at 2:47 PM, Scott Feldman wrote: > On Wed, Jun 10, 2015 at 2:25 PM, David Ahern wrote: >> On 6/10/15 2:56 PM, sfel...@gmail.com wrote: >>> >>> From: Scott Feldman >>> >>> Fix a BUG() where CONFIG_NET_SWITCHDEV is set but the driver for a bridged >>> port does not support switchdec_port_attr_set op. Don't BUG() if >>> -EOPNOTSUPP is returned. >>> >>> Signed-off-by: Scott Feldman >>> Reported-by: Brenden Blanco >>> --- >>> net/switchdev/switchdev.c |2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c >>> index e008057..99bced4 100644 >>> --- a/net/switchdev/switchdev.c >>> +++ b/net/switchdev/switchdev.c >>> @@ -103,7 +103,7 @@ static void switchdev_port_attr_set_work(struct >>> work_struct *work) >>> >>> rtnl_lock(); >>> err = switchdev_port_attr_set(asw->dev, &asw->attr); >>> - BUG_ON(err); >>> + BUG_ON(err && err != -EOPNOTSUPP); >>> rtnl_unlock(); >>> >>> dev_put(asw->dev); >>> >> >> Should that be WARN_ON instead of BUG_ON? > > I think I had it as WARN when we were working on the initial patches, > but we changed it to BUG_ON because we should only get an error here > if the driver screwed something up between PREPARE phase and COMMIT > phase, so it should be considered a driver bug which needs fixing. Actually, ignore what I said above. I was confusing this BUG_ON with the one in switchdev_port_attr_set(). Perhaps this BUG_ON() you're commenting on should be WARN(). A driver could return an err in PREPARE phase and that shouldn't be a BUG_ON situation; seems WARN would be better. It the case where the driver returns an err in COMMIT phase but didn't return an err in PREPARE phase we want to BUG_ON(). Maybe that case doesn't justify BUG_ON either, based on the link you posted. Jiri, IIRC, you suggested the BUG_ON(). Does it still sound right based on the point David is raising? -scott -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net-next] switchdev: fix BUG when port driver doesn't support set attr op
On 6/10/15 3:47 PM, Scott Feldman wrote: Should that be WARN_ON instead of BUG_ON? I think I had it as WARN when we were working on the initial patches, but we changed it to BUG_ON because we should only get an error here if the driver screwed something up between PREPARE phase and COMMIT phase, so it should be considered a driver bug which needs fixing. Linus rants from time to time about the prolific use of BUG_ON. e.g., https://lkml.org/lkml/2015/4/28/528 'BUG_ON() is for things where our internal data structures are so corrupted that we don't know what to do, and there's no way to continue. Not for "I want to sprinkle these things around and this should not happen".' David -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net-next] switchdev: fix BUG when port driver doesn't support set attr op
On Wed, Jun 10, 2015 at 2:25 PM, David Ahern wrote: > On 6/10/15 2:56 PM, sfel...@gmail.com wrote: >> >> From: Scott Feldman >> >> Fix a BUG() where CONFIG_NET_SWITCHDEV is set but the driver for a bridged >> port does not support switchdec_port_attr_set op. Don't BUG() if >> -EOPNOTSUPP is returned. >> >> Signed-off-by: Scott Feldman >> Reported-by: Brenden Blanco >> --- >> net/switchdev/switchdev.c |2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c >> index e008057..99bced4 100644 >> --- a/net/switchdev/switchdev.c >> +++ b/net/switchdev/switchdev.c >> @@ -103,7 +103,7 @@ static void switchdev_port_attr_set_work(struct >> work_struct *work) >> >> rtnl_lock(); >> err = switchdev_port_attr_set(asw->dev, &asw->attr); >> - BUG_ON(err); >> + BUG_ON(err && err != -EOPNOTSUPP); >> rtnl_unlock(); >> >> dev_put(asw->dev); >> > > Should that be WARN_ON instead of BUG_ON? I think I had it as WARN when we were working on the initial patches, but we changed it to BUG_ON because we should only get an error here if the driver screwed something up between PREPARE phase and COMMIT phase, so it should be considered a driver bug which needs fixing. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net-next] switchdev: fix BUG when port driver doesn't support set attr op
On 6/10/15 2:56 PM, sfel...@gmail.com wrote: From: Scott Feldman Fix a BUG() where CONFIG_NET_SWITCHDEV is set but the driver for a bridged port does not support switchdec_port_attr_set op. Don't BUG() if -EOPNOTSUPP is returned. Signed-off-by: Scott Feldman Reported-by: Brenden Blanco --- net/switchdev/switchdev.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c index e008057..99bced4 100644 --- a/net/switchdev/switchdev.c +++ b/net/switchdev/switchdev.c @@ -103,7 +103,7 @@ static void switchdev_port_attr_set_work(struct work_struct *work) rtnl_lock(); err = switchdev_port_attr_set(asw->dev, &asw->attr); - BUG_ON(err); + BUG_ON(err && err != -EOPNOTSUPP); rtnl_unlock(); dev_put(asw->dev); Should that be WARN_ON instead of BUG_ON? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net-next] switchdev: fix BUG when port driver doesn't support set attr op
On Wed, Jun 10, 2015 at 01:56:02PM -0700, sfel...@gmail.com wrote: > From: Scott Feldman > > Fix a BUG() where CONFIG_NET_SWITCHDEV is set but the driver for a bridged > port does not support switchdec_port_attr_set op. Don't BUG() if > -EOPNOTSUPP is returned. > > Signed-off-by: Scott Feldman > Reported-by: Brenden Blanco Looks good since -EOPNOTSUPP seems to be the safe failure case when switchdev is enabled or not. Acked-by: Andy Gospodarek -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH net-next] switchdev: fix BUG when port driver doesn't support set attr op
From: Scott Feldman Fix a BUG() where CONFIG_NET_SWITCHDEV is set but the driver for a bridged port does not support switchdec_port_attr_set op. Don't BUG() if -EOPNOTSUPP is returned. Signed-off-by: Scott Feldman Reported-by: Brenden Blanco --- net/switchdev/switchdev.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c index e008057..99bced4 100644 --- a/net/switchdev/switchdev.c +++ b/net/switchdev/switchdev.c @@ -103,7 +103,7 @@ static void switchdev_port_attr_set_work(struct work_struct *work) rtnl_lock(); err = switchdev_port_attr_set(asw->dev, &asw->attr); - BUG_ON(err); + BUG_ON(err && err != -EOPNOTSUPP); rtnl_unlock(); dev_put(asw->dev); -- 1.7.10.4 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
