Messages in this thread | | | Date | Tue, 03 Nov 2015 17:11:51 -0500 | From | Jarod Wilson <> | Subject | Re: [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
| |