[PATCH net-next] switchdev: fix BUG when port driver doesn't support set attr op

[PATCH net-next] switchdev: fix BUG when port driver doesn't support set attr op

[sfeldma]

From: Scott Feldman <sfeldma@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 <sfeldma@gmail.com>
Reported-by: Brenden Blanco <bblanco@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);
-- 
1.7.10.4

Re: [PATCH net-next] switchdev: fix BUG when port driver doesn't support set attr op

[Andy Gospodarek]

On Wed, Jun 10, 2015 at 01:56:02PM -0700, sfeldma@gmail.com wrote:
> From: Scott Feldman <sfeldma@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 <sfeldma@gmail.com>
> Reported-by: Brenden Blanco <bblanco@plumgrid.com>

Looks good since -EOPNOTSUPP seems to be the safe failure case when
switchdev is enabled or not.

Acked-by: Andy Gospodarek <gospo@cumulusnetworks.com>

Re: [PATCH net-next] switchdev: fix BUG when port driver doesn't support set attr op

[David Ahern]

On 6/10/15 2:56 PM, sfeldma@gmail.com wrote:
> From: Scott Feldman <sfeldma@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 <sfeldma@gmail.com>
> Reported-by: Brenden Blanco <bblanco@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?

Re: [PATCH net-next] switchdev: fix BUG when port driver doesn't support set attr op

[Scott Feldman]

On Wed, Jun 10, 2015 at 2:25 PM, David Ahern <dsahern@gmail.com> wrote:
> On 6/10/15 2:56 PM, sfeldma@gmail.com wrote:
>>
>> From: Scott Feldman <sfeldma@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 <sfeldma@gmail.com>
>> Reported-by: Brenden Blanco <bblanco@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.

Re: [PATCH net-next] switchdev: fix BUG when port driver doesn't support set attr op

[David Ahern]

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

Re: [PATCH net-next] switchdev: fix BUG when port driver doesn't support set attr op

[Scott Feldman]

On Wed, Jun 10, 2015 at 2:47 PM, Scott Feldman <sfeldma@gmail.com> wrote:
> On Wed, Jun 10, 2015 at 2:25 PM, David Ahern <dsahern@gmail.com> wrote:
>> On 6/10/15 2:56 PM, sfeldma@gmail.com wrote:
>>>
>>> From: Scott Feldman <sfeldma@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 <sfeldma@gmail.com>
>>> Reported-by: Brenden Blanco <bblanco@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

Re: [PATCH net-next] switchdev: fix BUG when port driver doesn't support set attr op

[Jiri Pirko]

Thu, Jun 11, 2015 at 12:00:47AM CEST, sfeldma@gmail.com wrote:
>On Wed, Jun 10, 2015 at 2:47 PM, Scott Feldman <sfeldma@gmail.com> wrote:
>> On Wed, Jun 10, 2015 at 2:25 PM, David Ahern <dsahern@gmail.com> wrote:
>>> On 6/10/15 2:56 PM, sfeldma@gmail.com wrote:
>>>>
>>>> From: Scott Feldman <sfeldma@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 <sfeldma@gmail.com>
>>>> Reported-by: Brenden Blanco <bblanco@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 ?

Re: [PATCH net-next] switchdev: fix BUG when port driver doesn't support set attr op

[Scott Feldman]

On Wed, Jun 10, 2015 at 11:16 PM, Jiri Pirko <jiri@resnulli.us> wrote:
> Thu, Jun 11, 2015 at 12:00:47AM CEST, sfeldma@gmail.com wrote:
>>On Wed, Jun 10, 2015 at 2:47 PM, Scott Feldman <sfeldma@gmail.com> wrote:
>>> On Wed, Jun 10, 2015 at 2:25 PM, David Ahern <dsahern@gmail.com> wrote:
>>>> On 6/10/15 2:56 PM, sfeldma@gmail.com wrote:
>>>>>
>>>>> From: Scott Feldman <sfeldma@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 <sfeldma@gmail.com>
>>>>> Reported-by: Brenden Blanco <bblanco@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?

Re: [PATCH net-next] switchdev: fix BUG when port driver doesn't support set attr op

[Jiri Pirko]

Thu, Jun 11, 2015 at 05:03:21PM CEST, sfeldma@gmail.com wrote:
>On Wed, Jun 10, 2015 at 11:16 PM, Jiri Pirko <jiri@resnulli.us> wrote:
>> Thu, Jun 11, 2015 at 12:00:47AM CEST, sfeldma@gmail.com wrote:
>>>On Wed, Jun 10, 2015 at 2:47 PM, Scott Feldman <sfeldma@gmail.com> wrote:
>>>> On Wed, Jun 10, 2015 at 2:25 PM, David Ahern <dsahern@gmail.com> wrote:
>>>>> On 6/10/15 2:56 PM, sfeldma@gmail.com wrote:
>>>>>>
>>>>>> From: Scott Feldman <sfeldma@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 <sfeldma@gmail.com>
>>>>>> Reported-by: Brenden Blanco <bblanco@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.

Re: [PATCH net-next] switchdev: fix BUG when port driver doesn't support set attr op

[David Miller]

From: sfeldma@gmail.com
Date: Wed, 10 Jun 2015 13:56:02 -0700

> From: Scott Feldman <sfeldma@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 <sfeldma@gmail.com>
> Reported-by: Brenden Blanco <bblanco@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.