Re: [PATCH v4 net-next 07/11] net: switchdev: remove lag_mod_cb from switchdev_handle_fdb_event_to_device

[Florian Fainelli <f.fainelli@gmail.com>]

On 2/21/2022 1:23 PM, Vladimir Oltean wrote:
> When the switchdev_handle_fdb_event_to_device() event replication helper
> was created, my original thought was that FDB events on LAG interfaces
> should most likely be special-cased, not just replicated towards all
> switchdev ports beneath that LAG. So this replication helper currently
> does not recurse through switchdev lower interfaces of LAG bridge ports,
> but rather calls the lag_mod_cb() if that was provided.
> 
> No switchdev driver uses this helper for FDB events on LAG interfaces
> yet, so that was an assumption which was yet to be tested. It is
> certainly usable for that purpose, as my RFC series shows:
> 
> https://patchwork.kernel.org/project/netdevbpf/cover/20220210125201.2859463-1-vladimir.oltean@nxp.com/
> 
> however this approach is slightly convoluted because:
> 
> - the switchdev driver gets a "dev" that isn't its own net device, but
>    rather the LAG net device. It must call switchdev_lower_dev_find(dev)
>    in order to get a handle of any of its own net devices (the ones that
>    pass check_cb).
> 
> - in order for FDB entries on LAG ports to be correctly refcounted per
>    the number of switchdev ports beneath that LAG, we haven't escaped the
>    need to iterate through the LAG's lower interfaces. Except that is now
>    the responsibility of the switchdev driver, because the replication
>    helper just stopped half-way.
> 
> So, even though yes, FDB events on LAG bridge ports must be
> special-cased, in the end it's simpler to let switchdev_handle_fdb_*
> just iterate through the LAG port's switchdev lowers, and let the
> switchdev driver figure out that those physical ports are under a LAG.
> 
> The switchdev_handle_fdb_event_to_device() helper takes a
> "foreign_dev_check" callback so it can figure out whether @dev can
> autonomously forward to @foreign_dev. DSA fills this method properly:
> if the LAG is offloaded by another port in the same tree as @dev, then
> it isn't foreign. If it is a software LAG, it is foreign - forwarding
> happens in software.
> 
> Whether an interface is foreign or not decides whether the replication
> helper will go through the LAG's switchdev lowers or not. Since the
> lan966x doesn't properly fill this out, FDB events on software LAG
> uppers will get called. By changing lan966x_foreign_dev_check(), we can
> suppress them.
> 
> Whereas DSA will now start receiving FDB events for its offloaded LAG
> uppers, so we need to return -EOPNOTSUPP, since we currently don't do
> the right thing for them.
> 
> Cc: Horatiu Vultur <horatiu.vultur@microchip.com>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian