Re: [PATCH net 2/2] net: dsa: Always react to global bridge attribute changes

[Tobias Waldekranz <tobias@waldekranz.com>]

On Sun, Mar 07, 2021 at 02:58, Vladimir Oltean <olteanv@gmail.com> wrote:
> On Sat, Mar 06, 2021 at 07:17:09PM +0100, Tobias Waldekranz wrote:
>> On Sat, Mar 06, 2021 at 16:04, Vladimir Oltean <olteanv@gmail.com> wrote:
>> > On Sat, Mar 06, 2021 at 04:00:33PM +0200, Vladimir Oltean wrote:
>> >> Hi Tobias,
>> >>
>> >> On Sat, Mar 06, 2021 at 01:24:55AM +0100, Tobias Waldekranz wrote:
>> >> > This is the second attempt to provide a fix for the issue described in
>> >> > 99b8202b179f, which was reverted in the previous commit.
>> >> >
>> >> > When a change is made to some global bridge attribute, such as VLAN
>> >> > filtering, accept events where orig_dev is the bridge master netdev.
>> >> >
>> >> > Separate the validation of orig_dev based on whether the attribute in
>> >> > question is global or per-port.
>> >> >
>> >> > Fixes: 5696c8aedfcc ("net: dsa: Don't offload port attributes on standalone ports")
>> >> > Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
>> >> > ---
>> >>
>> >> What do you think about this alternative?
>> >
>> > Ah, wait, this won't work when offloading objects/attributes on a LAG.
>> > Let me actually test your patch.
>> 
>> Right. But you made me realize that my v1 is also flawed, because it
>> does not guard against trying to apply attributes to non-offloaded
>> ports. ...the original issue :facepalm:
>> 
>> I have a version ready which reuses the exact predicate that you
>> previously added to dsa_port_offloads_netdev:
>> 
>> -               if (netif_is_bridge_master(attr->orig_dev))
>> +               if (dp->bridge_dev == attr->orig_dev)
>> 
>> Do you think anything else needs to be changed, or should I send that as
>> v2?
>
> Sorry, I just get a blank stare when I look at that blob of code you've
> added at the beginning of dsa_slave_port_attr_set, it might as well be
> correct but I'm not smart enough to process it and say "yes it is".
>
> What do you think about this one? At least for me it's easier to
> understand what's going on, and would leave a lot more room for further
> fixups if needed.

I like the approach of having to explicitly state the supported orig_dev
per attribute or object. I think we should go with your fix.