Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1261948

Re: [PATCH net-next] net/core: ensure features get disabled on new lower devs

Show all headers | View raw

On 11/03/2015 02:11 PM, Jarod Wilson wrote:
> Alexander Duyck wrote:
>> On 11/03/2015 12:36 PM, Jarod Wilson wrote:
>>> With moving netdev_sync_lower_features() after the .ndo_set_features
>>> calls, I neglected to verify that devices added *after* a flag had been
>>> disabled on an upper device were properly added with that flag
>>> disabled as
>>> well. This currently happens, because we exit 
>>> __netdev_update_features()
>>> when we see dev->features == features for the upper dev. We can retain
>>> the
>>> optimization of leaving without calling .ndo_set_features with a bit of
>>> tweaking and a goto here.
>>>
>>> Changing err to ret was somewhat arbitrary and makes the patch look 
>>> more
>>> involved, but seems to better fit the altered use.
> ...
>>> +     if (!ret) {
>>> +         dev->features = features;
>>> +         ret = 1;
>>> +     }
>>> +
>>
>> I would take the "ret = 1;" out of the if statement and let it stay here
>> by itself. Technically anything that traversed this path was returning 1
>> previously so we probably want to retain that behavior.
>
> Ah, that. I took a look at all the callers of 
> __netdev_update_features, and most don't even check return value, the 
> one that does (netdev_update_features) only cares if its zero or not 
> zero, so I figured it didn't really matter here, but it would indeed 
> return 2 now instead of 1, if it got that from ndo_set_features. For 
> consistency's sake, I can respin and just always set ret = 1 though.

One thought I just had would be to make it so that we assign -1 to ret 
and then jump inside the earlier features==features check rather than 
altering the ret value here.  Then we could just use a ternary value at 
the end and just do "return ret < 0 ? 0 : 1;".  That would take care of 
the return values and the features flag you called out below.

>>> +sync_lower:
>>>     /* some features must be disabled on lower devices when disabled
>>>      * on an upper device (think: bonding master or bridge)
>>>      */
>>>     netdev_for_each_lower_dev(dev, lower, iter)
>>>         netdev_sync_lower_features(dev, lower, features);
>>>
>>> -     if (!err)
>>> -         dev->features = features;
>>
>> You could just alter the if statement here to check for a non-zero ret
>> value since you should have it as either 0 or 1. It shouldn't have any
>> other values.
>>
>> That way you will have disabled the feature on the lower devices before
>> advertising that it has been disabled on the upper device.
>
> If this check is down here, the goto will trigger, setting 
> dev->features = features, but then, we got there because dev->features 
> == features already, so meh. But it would also NOT trigger in the case 
> of ndo_set_features returning 0 anymore, because we set ret = 1. Or am 
> I missing something or misunderstanding what you're suggesting here?
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH net-next] net/core: generic support for disabling netdev features down stack Jarod Wilson <jarod@redhat.com> - 2015-11-02 19:00 +0100
  Re: [PATCH net-next] net/core: generic support for disabling netdev  features down stack Alexander Duyck <alexander.duyck@gmail.com> - 2015-11-02 19:10 +0100
    Re: [PATCH net-next] net/core: generic support for disabling netdev  features down stack Jarod Wilson <jarod@redhat.com> - 2015-11-02 23:00 +0100
  [PATCH v2 net-next] net/core: generic support for disabling netdev features down stack Jarod Wilson <jarod@redhat.com> - 2015-11-03 04:00 +0100
    Re: [PATCH v2 net-next] net/core: generic support for disabling  netdev features down stack David Miller <davem@davemloft.net> - 2015-11-03 05:50 +0100
    Re: [PATCH v2 net-next] net/core: generic support for disabling  netdev features down stack Nikolay Aleksandrov <nikolay@cumulusnetworks.com> - 2015-11-03 11:10 +0100
      Re: [PATCH v2 net-next] net/core: generic support for disabling  netdev features down stack Geert Uytterhoeven <geert@linux-m68k.org> - 2015-11-03 15:00 +0100
        Re: [PATCH v2 net-next] net/core: generic support for disabling netdev  features down stack Jarod Wilson <jarod@redhat.com> - 2015-11-03 15:00 +0100
          Re: [PATCH v2 net-next] net/core: generic support for disabling  netdev features down stack Nikolay Aleksandrov <nikolay@cumulusnetworks.com> - 2015-11-03 15:10 +0100
            Re: [PATCH v2 net-next] net/core: generic support for disabling netdev  features down stack Jarod Wilson <jarod@redhat.com> - 2015-11-03 17:10 +0100
    [PATCH net-next] net/core: fix for_each_netdev_feature Jarod Wilson <jarod@redhat.com> - 2015-11-03 16:20 +0100
      Re: [PATCH net-next] net/core: fix for_each_netdev_feature Nikolay Aleksandrov <nikolay@cumulusnetworks.com> - 2015-11-03 16:50 +0100
      Re: [PATCH net-next] net/core: fix for_each_netdev_feature David Miller <davem@davemloft.net> - 2015-11-03 17:40 +0100
    [PATCH net-next] net/core: ensure features get disabled on new lower devs Jarod Wilson <jarod@redhat.com> - 2015-11-03 21:40 +0100
      Re: [PATCH net-next] net/core: ensure features get disabled on new  lower devs Alexander Duyck <alexander.duyck@gmail.com> - 2015-11-03 22:20 +0100
        Re: [PATCH net-next] net/core: ensure features get disabled on new  lower devs Jarod Wilson <jarod@redhat.com> - 2015-11-03 23:20 +0100
          Re: [PATCH net-next] net/core: ensure features get disabled on new  lower devs Alexander Duyck <alexander.duyck@gmail.com> - 2015-11-04 00:10 +0100
      Re: [PATCH net-next] net/core: ensure features get disabled on new  lower devs Nikolay Aleksandrov <nikolay@cumulusnetworks.com> - 2015-11-03 22:30 +0100
      Re: [PATCH net-next] net/core: ensure features get disabled on new  lower devs Michal Kubecek <mkubecek@suse.cz> - 2015-11-03 23:00 +0100
        Re: [PATCH net-next] net/core: ensure features get disabled on new  lower devs Jarod Wilson <jarod@redhat.com> - 2015-11-03 23:00 +0100
      [PATCH v2 net-next] net/core: ensure features get disabled on new lower devs Jarod Wilson <jarod@redhat.com> - 2015-11-04 05:10 +0100
        Re: [PATCH v2 net-next] net/core: ensure features get disabled on  new lower devs David Miller <davem@davemloft.net> - 2015-11-05 04:00 +0100
          Re: [PATCH v2 net-next] net/core: ensure features get disabled on  new lower devs Florian Fainelli <f.fainelli@gmail.com> - 2015-11-13 01:30 +0100
            Re: [PATCH v2 net-next] net/core: ensure features get disabled on  new lower devs Jiri Pirko <jiri@resnulli.us> - 2015-11-13 11:30 +0100
              Re: [PATCH v2 net-next] net/core: ensure features get disabled on new  lower devs Nikolay Aleksandrov <nikolay@cumulusnetworks.com> - 2015-11-13 12:00 +0100
                Re: [PATCH v2 net-next] net/core: ensure features get disabled on new  lower devs Laura Abbott <labbott@redhat.com> - 2015-11-13 23:40 +0100

csiph-web