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

On Sun, Mar 07, 2021 at 10:51:18AM +0100, Tobias Waldekranz wrote:

On Sun, Mar 07, 2021 at 02:58, Vladimir Oltean [off-list ref] wrote:

quoted

On Sat, Mar 06, 2021 at 07:17:09PM +0100, Tobias Waldekranz wrote:

quoted

On Sat, Mar 06, 2021 at 16:04, Vladimir Oltean [off-list ref] wrote:

quoted

On Sat, Mar 06, 2021 at 04:00:33PM +0200, Vladimir Oltean wrote:

quoted

Hi Tobias,

On Sat, Mar 06, 2021 at 01:24:55AM +0100, Tobias Waldekranz wrote:

quoted

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!