Re: [PATCH net-next v13 8/9] net: dsa: lan9645x: add bridge support

[Jens Emil Schulz Ostergaard <jensemil.schulzostergaard@microchip.com>]

On Fri, 2026-10-02 at 21:14 +0000, netdev-bot+sashiko@kernel.org wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Thank you for your contribution! Sashiko AI review found 3 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 3 · Low: 0
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [Medium] __lan9645x_port_set_host_flood() computes uc_ena/mc_ena from
>   host_flood_*_mask & ~bridge_mask, so it drops every host flood request…
> - [Medium] The MDB code falls back to the L2 multicast flood PGID
>   (PGID_MC) when it runs out of general-purpose PGIDs.
> - [Medium] The patch sets ds->ageing_time_min = 2000 ms, so enslaving a
>   lan9645x port to a bridge with ageing_time below 2 s (for example 0,…
> 
> --
> 
> net: dsa: lan9645x: add bridge support
> 
> This adds hardware bridge offload to the lan9645x DSA driver for a single
> bridge. It covers ageing time, bridge port flags, STP state and host flood.
> The CPU port module is taken out of the default UC/MC flood PGIDs. Host
> flood requests from offloaded bridged ports are recorded, but they are not
> programmed until the port leaves the bridge.
> 
> > diff --git a/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> > index cc85e18c5a3b3..e41a73c20606b 100644
> > --- a/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> > +++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> 
> [ ... ]
> 
> > @@ -324,6 +333,257 @@ static void lan9645x_port_phylink_get_caps(struct dsa_switch *ds, int port,
> 
> [ ... ]
> 
> > +static int lan9645x_port_bridge_flags(struct dsa_switch *ds, int port,
> > +                                   struct switchdev_brport_flags f,
> > +                                   struct netlink_ext_ack *extack)
> > +{
> 
> [ ... ]
> 
> > +     if (f.mask & BR_MCAST_FLOOD) {
> > +             bool ena = !!(f.val & BR_MCAST_FLOOD);
> > +
> > +             lan9645x_port_pgid_set(lan9645x, PGID_MC, port, ena);
> 
> [Severity: Medium]
> Does this work with the MDB fallback to PGID_MC?
> 
> When the general-purpose PGIDs run out, __lan9645x_mdb_add() and
> __lan9645x_mdb_del() in lan9645x_mdb.c point a group at PGID_MC:
> 
> drivers/net/dsa/microchip/lan9645x/lan9645x_mdb.c:__lan9645x_mdb_add() {
>     ...
>                 if (lan9645x_mdb_on_flood_mask(mdb_entry, type))
>                         return lan9645x_mdb_widen_dest(lan9645x, mdb_entry,
>                                                        type, NULL, PGID_MC,
>                                                        new_ports);
>     ...
> }
> 
> drivers/net/dsa/microchip/lan9645x/lan9645x_mdb.c:__lan9645x_mdb_del() {
>     ...
>         lan9645x_mdb_narrow_dest(lan9645x, mdb_entry, type, NULL, PGID_MC,
>                                  new_ports);
>     ...
> }
> 
> After the fallback, the MAC table entry forwards to whatever PGID_MC
> holds, not to the member mask that the MDB code computed. That worked
> when PGID_MC was fixed to all front ports.
> 
> With this change, "bridge link set dev swpN mcast_flood off" clears swpN
> from PGID_MC. In the software bridge, mcast_flood only affects
> unregistered multicast. Here, though, it looks like a registered member
> port with mcast_flood off would stop getting any group that has fallen
> back to PGID_MC.
> 
> One way to reach this is more than 48 distinct front port subsets among
> non-IP L2 MDB groups, for example static "bridge mdb add" entries on a
> switch with 6 or more user ports. That would use up PGID_GP_START to
> PGID_GP_END. IP multicast should not be affected because the MAC entry
> encodes the port mask. CPU membership comes from MAC_CPU_COPY.


Dismissed. This is intended. Once a group has to fall back to PGID_MC, it is
forwarded like unregistered multicast, i.e. according to the user's
mcast_flood configuration. This matches mscc/ocelot, where an mdb add
that cannot get a PGID leaves the group without a MAC table entry, so
its traffic is flooded as unknown multicast.

lan9645x cannot simply forget the entry, since host membership is carried
by MAC_CPU_COPY in the entry and not by the flood masks, so it keeps the
entry and points it at PGID_MC instead. A PGID with all ports would
forward the group to ports where mcast_flood was disabled.


> 
> > +             lan9645x_port_pgid_set(lan9645x, PGID_MCIPV4, port, ena);
> > +             lan9645x_port_pgid_set(lan9645x, PGID_MCIPV6, port, ena);
> > +     }
> 
> [ ... ]
> 
> > +static void __lan9645x_port_set_host_flood(struct lan9645x *lan9645x)
> > +{
> > +     bool mc_ena, uc_ena;
> > +     u16 unbridged;
> > +
> > +     lockdep_assert_held(&lan9645x->fwd_domain_lock);
> 
> [ ... ]
> 
> > +     unbridged = ~lan9645x->bridge_mask &
> > +                 GENMASK(lan9645x->num_phys_ports - 1, 0);
> > +
> > +     uc_ena = !!(lan9645x->host_flood_uc_mask & unbridged);
> 
> [Severity: Medium]
> lan9645x_setup() no longer puts the CPU port module in PGID_UC, PGID_MC,
> PGID_MCIPV4 or PGID_MCIPV6. That leaves the host flood bit as the only
> path for unknown unicast and unregistered multicast to reach the CPU.
> With the unbridged filter here, can the CPU ever get that traffic when
> only bridged ports ask for host flooding?
> 
> The bridge calls dev_set_allmulti(dev, 1) on every port in br_add_if().
> br_manage_promisc() sets promisc when vlan_filtering=0, and also when
> there is more than one auto port. dsa_user_manage_host_flood() passes
> the flags through unchanged:
> 
> net/dsa/user.c:dsa_user_manage_host_flood() {
>     bool mc = dev->flags & (IFF_PROMISC | IFF_ALLMULTI);
>     ...
>     bool uc = dev->flags & IFF_PROMISC;
> 
>     dsa_port_set_host_flood(dp, uc, mc);
> }
> 
> Here, however, ~bridge_mask filters those requests out.
> 
> A few cases seem to be affected:
> 
> - Take a bridge with a lan9645x port and a foreign port such as wlan,
>   veth or tap. The software bridge can't flood FDB misses or
>   unregistered multicast to the foreign port. Assisted learning only
>   covers addresses it has already learned. DSA returns -EOPNOTSUPP for
>   MDB entries whose orig_dev is a foreign port, so the hardware would
>   drop a multicast stream going to a wifi client.
> 
> - With multicast snooping disabled, br0 receives no non-link-local
>   multicast.
> 
> - Setting promisc or allmulti explicitly on br0 or on a bridged port
>   does nothing.
> 
> Documentation/networking/switchdev.rst asks for unknown unicast to be
> flooded to every port in the domain. It also asks for flooding to the CPU
> when IFF_PROMISC or IFF_ALLMULTI is set. I'd also expect the promisc and
> allmulti cases of test_bridge in
> tools/testing/selftests/net/forwarding/local_termination.sh to fail.
> 
> felix has the same global flood mask limitation, but it honours host
> flood for bridged ports. The commit message only presents this as
> protection for a weak CPU and doesn't mention these consequences.
> 

Correct. This was intentional, as described in the comment and commit
message. I was aware of cases 1 and 3, but was willing to accept that
in the name of comfortably running a weak host CPU. I was not aware
of 2 though, and I would not accept that. I was also not aware of
switchdev.rst specifically asking not to do this. Therefore, I change
v14 to honours host flood requests from bridged ports as well, as
felix does, so the CPU port module is in the unicast and multicast flood
masks while any port requests it.


> > +     lan9645x_port_pgid_set(lan9645x, PGID_UC, lan9645x->num_phys_ports,
> > +                            uc_ena);
> 
> [ ... ]
> 
> > +static void lan9645x_port_bridge_leave(struct dsa_switch *ds, int port,
> > +                                    struct dsa_bridge bridge)
> > +{
> > +     struct lan9645x *lan9645x = ds->priv;
> > +     struct lan9645x_port *p;
> > +
> > +     p = lan9645x_to_port(lan9645x, port);
> > +
> > +     mutex_lock(&lan9645x->fwd_domain_lock);
> > +
> > +     lan9645x->bridge_mask &= ~BIT(p->chip_port);
> > +
> > +     __lan9645x_port_set_host_flood(lan9645x);
> > +     lan9645x_vlan_set_hostmode(p);
> 
> [Severity: Medium]
> Can the new ds->ageing_time_min = 2U * MSEC_PER_SEC in lan9645x_setup()
> leave the driver and the DSA core disagreeing when a bridge join fails
> partway through?
> 
> Suppose a port joins a bridge whose ageing_time is below 2 s, for
> example 0. dsa_port_switchdev_sync_attrs() applies the brport flags, STP
> state and VLAN filtering first. Only after that does
> dsa_port_ageing_time() fail with -ERANGE:
> 
> dsa_port_bridge_join()
>   DSA_NOTIFIER_BRIDGE_JOIN -> lan9645x_port_bridge_join()
>   dsa_port_switchdev_sync_attrs()
>     dsa_port_inherit_brport_flags()  p->learn_ena = true, dp->learning = true
>     dsa_port_set_state()
>     dsa_port_vlan_filtering(true)    p->vlan_aware = true, dp->vlan_filtering = true
>     dsa_port_ageing_time()           -ERANGE
>   out_rollback_unoffload / out_rollback_unbridge
>     DSA_NOTIFIER_BRIDGE_LEAVE -> lan9645x_port_bridge_leave()
> 
> The rollback never calls dsa_port_switchdev_unsync_attrs(). Here,
> lan9645x_vlan_set_hostmode() sets p->vlan_aware = false while
> dp->vlan_filtering stays true. p->learn_ena stays true as well.
> 
> If the port later joins a vlan_filtering=1 bridge, dsa_port_vlan_filtering()
> returns early:
> 
>         if (dsa_port_is_vlan_filtering(dp) == vlan_filtering)
>                 return 0;
> 
> As a result, lan9645x_port_vlan_filtering() is never called. Would the
> port then run VLAN-unaware (UNAWARE_PVID, no VLAN_AWARE_ENA) inside a
> VLAN-aware bridge?
> 
> If the port stays standalone and is brought down and up again, this path
> runs:
> 
> dsa_port_enable_rt()
>   dsa_port_set_state_now(BR_STATE_FORWARDING)
>     lan9645x_port_bridge_stp_state_set()
> 
> learn_ena is computed from the stale p->learn_ena, and
> ANA_PORT_CFG_LEARN_ENA gets set on a standalone port in HOST_PVID. Is
> that intended?
> 
> The missing unsync in the DSA core rollback is older than this patch.
> However, ageing_time_min is what makes it reachable for lan9645x. On top
> of that, bridge_leave resets vlan_aware without DSA knowing.
> 

Dismissed, preexisting. As is mentioned the missing dsa_port_switchdev_unsync
attrs() in the dsa_port_bridge_join() rollback predates this patch. I think it
affects every driver for which a step of dsa_port_switchdev_sync_attrs() can fail
after VLAN filtering was applied, including all users of
ageing_time_min. Only the core can reset the cached dp->vlan_filtering and
dp->learning, so that is better fixed in DSA. In v14 I can make the driver
independent of this for the learning part though, so I can add that.

> > +     lan9645x_update_fwd_mask(lan9645x);
> > +
> > +     mutex_unlock(&lan9645x->fwd_domain_lock);
> > +}
> 
> [ ... ]
> 
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260929-dsa_lan9645x_switch_driver_base-v13-0-827c2d3617f2%40microchip.com