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

[Vladimir Oltean <olteanv@gmail.com>]

On Sun, Mar 07, 2021 at 10:51:18AM +0100, Tobias Waldekranz wrote:
> 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.

Ok, I'm sending it as-is. Thanks again!