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 j...@resnulli.us 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 sfel...@gmail.com wrote: On Wed, Jun 10, 2015 at 2:25 PM, David Ahern dsah...@gmail.com wrote: On 6/10/15 2:56 PM, sfel...@gmail.com wrote: From: Scott Feldman sfel...@gmail.com 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 sfel...@gmail.com Reported-by: Brenden Blanco bbla...@plumgrid.com --- 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 sfel...@gmail.com 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 sfel...@gmail.com Reported-by: Brenden Blanco bbla...@plumgrid.com --- 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 sfel...@gmail.com wrote: On Wed, Jun 10, 2015 at 2:25 PM, David Ahern dsah...@gmail.com wrote: On 6/10/15 2:56 PM, sfel...@gmail.com wrote: From: Scott Feldman sfel...@gmail.com 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 sfel...@gmail.com Reported-by: Brenden Blanco bbla...@plumgrid.com --- 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
Thu, Jun 11, 2015 at 05:03:21PM CEST, sfel...@gmail.com wrote: On Wed, Jun 10, 2015 at 11:16 PM, Jiri Pirko j...@resnulli.us 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 sfel...@gmail.com wrote: On Wed, Jun 10, 2015 at 2:25 PM, David Ahern dsah...@gmail.com wrote: On 6/10/15 2:56 PM, sfel...@gmail.com wrote: From: Scott Feldman sfel...@gmail.com 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 sfel...@gmail.com Reported-by: Brenden Blanco bbla...@plumgrid.com --- 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 6/10/15 2:56 PM, sfel...@gmail.com wrote: From: Scott Feldman sfel...@gmail.com 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 sfel...@gmail.com Reported-by: Brenden Blanco bbla...@plumgrid.com --- 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 2:25 PM, David Ahern dsah...@gmail.com wrote: On 6/10/15 2:56 PM, sfel...@gmail.com wrote: From: Scott Feldman sfel...@gmail.com 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 sfel...@gmail.com Reported-by: Brenden Blanco bbla...@plumgrid.com --- 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 Wed, Jun 10, 2015 at 01:56:02PM -0700, sfel...@gmail.com wrote: From: Scott Feldman sfel...@gmail.com 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 sfel...@gmail.com Reported-by: Brenden Blanco bbla...@plumgrid.com Looks good since -EOPNOTSUPP seems to be the safe failure case when switchdev is enabled or not. Acked-by: Andy Gospodarek go...@cumulusnetworks.com -- 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 sfel...@gmail.com wrote: On Wed, Jun 10, 2015 at 2:25 PM, David Ahern dsah...@gmail.com wrote: On 6/10/15 2:56 PM, sfel...@gmail.com wrote: From: Scott Feldman sfel...@gmail.com 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 sfel...@gmail.com Reported-by: Brenden Blanco bbla...@plumgrid.com --- 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
