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

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

On Mon, 2026-10-12 at 11:24 +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 2 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 2 (1) · 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] When the general-purpose PGIDs run out, L2 multicast groups
>   fall back to PGID_MC.
> - [Medium] Setting ds->ageing_time_min/max makes a bridge join fail late,
>   and the failed join leaves DSA's dp->vlan_filtering out of step with…
> 
> Pre-existing issues:
> - [Medium] This problem wasn't introduced by this patch, but the new
>   lan9645x_set_ageing_time() is affected by it.
> 
> --
> 
> net: dsa: lan9645x: add bridge support
> 
> This adds hardware bridge offload to the lan9645x DSA driver for a single
> bridge. It adds ageing time configuration with min/max limits enforced by
> the DSA core, plus bridge port flags and STP state handling. Host flooding
> is done by adding the CPU port module to the global flood masks.
> 
> > diff --git a/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> > index a0f48f3ad9aff..cefe643fce182 100644
> > --- a/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> > +++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> 
> [ ... ]
> 
> > @@ -306,6 +302,19 @@ static int lan9645x_setup(struct dsa_switch *ds)
> >       ds->fdb_isolation = true;
> >       ds->max_num_bridges = 1;
> > 
> > +     /* ANA_AUTOAGE_AGE_PERIOD is a seconds-based field and entries are
> > +      * aged after 2 * AGE_PERIOD, giving (2 * FIELD_MAX) seconds of
> > +      * maximum aging and 2 seconds of minimum aging.
> > +      *
> > +      * An AGE_PERIOD of 0 stops the analyzer from aging at all, which is
> > +      * the opposite of what a bridge ageing_time of 0 asks for. The bridge
> > +      * expires every dynamic entry immediately in that case and floods, so
> > +      * reject it rather than keep learned addresses forever.
> > +      */
> > +     ds->ageing_time_min = 2U * MSEC_PER_SEC;
> > +     ds->ageing_time_max = 2U * MSEC_PER_SEC *
> > +                           FIELD_MAX(ANA_AUTOAGE_AGE_PERIOD);
> 
> [Severity: Medium]
> Can a bridge join that fails on these limits leave the port VLAN-unaware
> in hardware after the next successful join?
> 
> With ageing_time_min set, dsa_switch_ageing_time() returns -ERANGE for any
> bridge ageing_time below 2s, including 0. For example:
> 
>   ip link add br0 type bridge vlan_filtering 1 ageing_time 0
> 
> dsa_port_switchdev_sync_attrs() applies the ageing time last. By then,
> dsa_port_vlan_filtering(dp, br_vlan_enabled(br)) has already called
> lan9645x_port_vlan_filtering(true) and set dp->vlan_filtering = true.
> 
> The rollback in dsa_port_bridge_join() then does:
> 
> net/dsa/port.c:dsa_port_bridge_join() {
>     ...
> out_rollback_unoffload:
>         switchdev_bridge_port_unoffload(...);
>         dsa_flush_workqueue();
> out_rollback_unbridge:
>         dsa_broadcast(DSA_NOTIFIER_BRIDGE_LEAVE, &info);
> out_rollback:
>         dsa_port_bridge_destroy(dp, br);
>     ...
> }
> 
> This path never calls dsa_port_switchdev_unsync_attrs() or
> dsa_port_reset_vlan_filtering(), so dp->vlan_filtering stays true.
> 
> Meanwhile the BRIDGE_LEAVE notifier reaches lan9645x_port_bridge_leave().
> That function calls lan9645x_vlan_set_hostmode(), which sets
> p->vlan_aware = false.
> 
> Now suppose the ageing time is fixed and the port is added to a
> vlan_filtering=1 bridge again. dsa_port_vlan_filtering() returns early:
> 
>         if (dsa_port_is_vlan_filtering(dp) == vlan_filtering)
>                 return 0;
> 
> So .port_vlan_filtering is never called. Neither
> lan9645x_port_bridge_join() nor lan9645x_vlan_clear_hostmode() restores
> p->vlan_aware.
> 
> Would the port then stay at VLAN_AWARE_ENA=0 in ANA_VLAN_CFG while it is
> in a VLAN-aware bridge? That would give wrong ingress VLAN classification
> and filtering.
> 
> The incomplete rollback is in the DSA core. This patch makes it reachable
> through the new ageing limits, and the new bridge_leave creates the
> mismatch on the driver side. The issue still seems to be there at the end
> of the series.
> 

Right. As discussed for v14, the missing
dsa_port_switchdev_unsync_attrs() in the dsa_port_bridge_join() rollback
is a DSA core issue.

But I think I can make sure the driver is not affected by it. The next
version keeps p->vlan_aware equal to what DSA last passed to
port_vlan_filtering(), so bridge_leave no longer clears it, and derives
the hardware VLAN awareness from the port being bridged and
p->vlan_aware. A standalone port is VLAN-unaware regardless, and a later
join to a VLAN-aware bridge is configured correctly even when DSA skips
the port_vlan_filtering() call.


> [ ... ]
> 
> > @@ -324,6 +333,246 @@ static void lan9645x_port_phylink_get_caps(struct dsa_switch *ds, int port,
> >       lan9645x_phylink_get_caps(ds->priv, port, config);
> >  }
> > 
> > +static int lan9645x_set_ageing_time(struct dsa_switch *ds, unsigned int msecs)
> > +{
> > +     struct lan9645x *lan9645x = ds->priv;
> > +
> > +     /* Entry must suffer two aging scans before it is removed, so it is
> > +      * aged after 2*AGE_PERIOD, and the unit is in seconds.
> > +      */
> > +     lan_rmw(ANA_AUTOAGE_AGE_PERIOD_SET(msecs / (2 * MSEC_PER_SEC)),
> > +             ANA_AUTOAGE_AGE_PERIOD,
> > +             lan9645x, ANA_AUTOAGE);
> > +
> > +     return 0;
> > +}
> 
> [Severity: Medium]
> This is a pre-existing issue in the DSA core and was not introduced by
> this patch, but it affects the new lan9645x_set_ageing_time(). Does
> raising the bridge ageing time ever reach the hardware?
> 
> dsa_port_ageing_time() sends the notifier before it updates the cached
> value:
> 
> net/dsa/port.c:dsa_port_ageing_time() {
>     ...
>         err = dsa_port_notify(dp, DSA_NOTIFIER_AGEING_TIME, &info);
>         if (err)
>                 return err;
> 
>         dp->ageing_time = ageing_time;
>     ...
> }
> 
> dsa_switch_fastest_ageing_time() then takes the minimum over all ports,
> including the old value of the requesting port:
> 
>         dsa_switch_for_each_port(dp, ds)
>                 if (dp->ageing_time && dp->ageing_time < ageing_time)
>                         ageing_time = dp->ageing_time;
> 
> So a change from 300000 ms to 600000 ms programs 300000 ms through
> lan9645x_set_ageing_time() and returns success. 600000 ms is then cached
> in dp->ageing_time, with no further hardware write.
> 
> The fix probably belongs in the DSA core rather than in this driver: the
> requester's cached value could be updated, or left out, before the
> minimum is computed.
> 

Dismissed, preexisting.

The fix belongs in the DSA core.


> [ ... ]
> 
> > +static int lan9645x_port_bridge_flags(struct dsa_switch *ds, int port,
> > +                                   struct switchdev_brport_flags f,
> > +                                   struct netlink_ext_ack *extack)
> > +{
> > +     struct lan9645x *lan9645x = ds->priv;
> > +
> 
> [ ... ]
> 
> > +     if (f.mask & BR_MCAST_FLOOD) {
> > +             bool ena = !!(f.val & BR_MCAST_FLOOD);
> > +
> > +             lan9645x_port_pgid_set(lan9645x, PGID_MC, port, ena);
> > +             lan9645x_port_pgid_set(lan9645x, PGID_MCIPV4, port, ena);
> > +             lan9645x_port_pgid_set(lan9645x, PGID_MCIPV6, port, ena);
> > +     }
> 
> [Severity: Medium]
> Does this break the PGID_MC fallback that the mdb code uses when the
> general-purpose PGIDs run out?
> 
> When a group's PGID is shared or no PGID is free, __lan9645x_mdb_del()
> points the L2 group at PGID_MC:
> 
> drivers/net/dsa/microchip/lan9645x/lan9645x_mdb.c:__lan9645x_mdb_del() {
>     ...
>         /* Shared PGID or on flood mask. Point the entry at the L2 MC flood mask
>          * instead. This way our mac table entry survives and MAC_CPU_COPY
>          * keeps host membership working.
>          */
>     ...
>         lan9645x_mdb_narrow_dest(lan9645x, mdb_entry, type, NULL, PGID_MC,
>                                  new_ports);
>     ...
> }
> 
> __lan9645x_mdb_add() also keeps a group that is already on the flood mask
> pointed at PGID_MC:
> 
>                 /* For a L2 MC group already on the flood mask, we keep it there
>                  * so that the host can still join/leave a group we had to give
>                  * up offloading. A front port that joins forwards according to
>                  * the flood masks, like the other members.
>                  */
>                 if (lan9645x_mdb_on_flood_mask(mdb_entry, type))
>                         return lan9645x_mdb_widen_dest(lan9645x, mdb_entry,
>                                                        type, NULL, PGID_MC,
>                                                        new_ports);
> 
> This fallback depended on PGID_MC holding every front port, as
> lan9645x_setup() sets it. With this change, a port configured with:
> 
>   bridge link set dev swpN mcast_flood off
> 
> is removed from PGID_MC. If that port is also a registered member of an
> L2 group in the fallback state, will it stop receiving that group's
> traffic?
> 
> The software bridge still delivers registered groups to their members
> whatever their mcast_flood setting, so hardware forwarding would no
> longer match it. Also, when such a port joins through
> __lan9645x_mdb_add(), the join reports success but the port gets no
> traffic. The issue still seems to be there at the end of the series.
> 

Dismissed, intended.

This is intended, as discussed for v14. Once a group has to fall back
to PGID_MC it is forwarded like unregistered multicast, i.e. according
to the mcast_flood configuration of the ports, including members that
join while it is there. 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 instead forward the
group to non-members where mcast_flood was disabled.

The fallback is limited to non-IP L2 groups. IPv4 and IPv6 groups
carry the port mask in the MAC table entry and never use a general
purpose PGID, and groups with the same member ports share one, so it
takes more than 48 distinct member sets among L2 groups to get here.
The mdb patch documents the fallback to the flood masks.


> [ ... ]
> 
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261008-dsa_lan9645x_switch_driver_base-v14-0-3d2a72661213%40microchip.com