lkml.org 
[lkml]   [2015]   [Nov]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH net-next] net/core: ensure features get disabled on new lower devs
    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.

    >> +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?

    --
    Jarod Wilson
    jarod@redhat.com




    \
     
     \ /
      Last update: 2015-11-03 23:41    [W:3.323 / U:0.168 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site