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

[Jiri Pirko <jiri@resnulli.us>]

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 ?