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

[Jens Emil Schulz Ostergaard]

On Mon, 2026-10-12 at 11:24 +0000, netdev-bot+sashiko@xxxxxxxxxx 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.
> 
> [ ... ]
> 
> > @@ -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.
> 
> [ ... ]
> 
> > +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.
> 
> [ ... ]
> 
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261008-dsa_lan9645x_switch_driver_base-v14-0-3d2a72661213%40microchip.com