Re: [PATCH net-next v2 5/9] net: dsa: lan9645x: add bridge support

[Jens Emil Schulz Ostergaard]

On Sun, 2026-03-29 at 12:56 -0700, Jakub Kicinski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> net: dsa: lan9645x: add bridge support
> 
> This commit adds support for hardware offloading of the bridge in the
> lan9645x DSA driver. It enables a single bridge device to be used with
> the switch.
> 
> > diff --git a/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> > index a0908cbf89c7..599e589c4ec3 100644
> > --- a/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> > +++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> [ ... ]
> > @@ -145,6 +155,8 @@ static int lan9645x_setup(struct dsa_switch *ds)
> >               return err;
> >       }
> > 
> > +     mutex_init(&lan9645x->fwd_domain_lock);
> > +
> >       /* Link Aggregation Mode: NETDEV_LAG_HASH_L2 */
> >       lan_wr(ANA_AGGR_CFG_AC_SMAC_ENA |
> >              ANA_AGGR_CFG_AC_DMAC_ENA,
> [ ... ]
> > @@ -240,6 +252,11 @@ static int lan9645x_setup(struct dsa_switch *ds)
> > 
> >       lan9645x_port_set_tail_drop_wm(lan9645x);
> > 
> > +     lan9645x->owq = alloc_ordered_workqueue("%s-owq", 0,
> > +                                             dev_name(lan9645x->dev));
> > +     if (!lan9645x->owq)
> > +             return -ENOMEM;
> > +
> 
> Is it possible that the hardware remains initialized if the call to
> alloc_ordered_workqueue fails? Because the setup callback returns an error,
> the DSA core will skip the teardown callback. This would bypass
> lan9645x_npi_port_deinit, potentially leaving the conduit netdev in a
> corrupted state with hardware tagging enabled.

I do not think it is common practice to undo switch register writes in the
error path here. We reset the switch registers in .setup. If we exit
here there is nothing to teardown.

> 
> Also in lan9645x_setup, the initialization loop for PGID_ENTRIES iterates
> through index 88:
> 
> for (int i = 0; i < PGID_ENTRIES; ++i)
>         lan_wr(ANA_PGID_CFG_OBEY_VLAN_SET(1),
>                lan9645x, ANA_PGID_CFG(i));
> 
> Could this leave the CPU port's source configuration in an undefined state?
> The CPU port is port 9, which has its source analysis register at PGID_SRC
> (80) + 9 = 89, but the loop terminates before reaching it.
> 

No as I mentioned in another mail this OBEY_VLAN does not make sense for
pgid 89.

> > @@ -258,6 +275,271 @@ 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)
> > +{
> > +     u32 age_secs = max(1, msecs / MSEC_PER_SEC / 2);
> > +     struct lan9645x *lan9645x = ds->priv;
> > +
> > +     /* Entry is must suffer two aging scans before it is removed, so it is
> > +      * aged after 2*AGE_PERIOD, and the unit is in seconds.
> > +      * An age period of 0 disables automatic aging.
> > +      */
> > +     lan_rmw(ANA_AUTOAGE_AGE_PERIOD_SET(age_secs),
> > +             ANA_AUTOAGE_AGE_PERIOD,
> > +             lan9645x, ANA_AUTOAGE);
> > +     return 0;
> > +}
> 
> Does this function properly disable automatic aging when requested? When the
> Linux bridge passes msecs = 0 to disable aging, the max macro evaluates to 1.
> Since writing 1 configures an aggressive 2-second aging period, it appears
> this might not fulfill the user's intent.
> 

Yes that is right. I will change it to:


	lan_rmw(ANA_AUTOAGE_AGE_PERIOD_SET(msecs ? age_secs : 0),
		ANA_AUTOAGE_AGE_PERIOD,
		lan9645x, ANA_AUTOAGE);


> [ ... ]
> > +static void __lan9645x_port_set_host_flood(struct lan9645x *lan9645x, int port,
> > +                                        bool uc, bool mc)
> > +{
> > +     bool mc_ena, uc_ena;
> > +
> > +     lockdep_assert_held(&lan9645x->fwd_domain_lock);
> > +
> > +     /* We want promiscuous and all_multi to affect standalone ports, for
> > +      * debug and test purposes.
> > +      *
> > +      * However, the linux bridge is incredibly eager to put bridged ports in
> > +      * promiscuous mode.
> > +
> > +      * This is unfortunate since lan9645x flood masks are global and not per
> > +      * ingress port. When some port triggers unknown uc/mc to the CPU, the
> > +      * traffic from any port is forwarded to the CPU.
> > +      *
> > +      * If the host CPU is weak, this can cause tremendous stress. Therefore,
> > +      * we compromise by ignoring this host flood request for bridged ports.
> > +      */
> > +     if (lan9645x_port_is_bridged(lan9645x_to_port(lan9645x, port)))
> > +             return;
> 
> Will ignoring the host flood request for bridged ports break IPv6 Neighbor
> Discovery? IPv6 ND relies on multicast, and the PGID_MC and PGID_MCIPV6 masks
> do not include the CPU port by default. By ignoring the bridge's request to
> enable multicast flooding, the CPU port may fail to receive ICMPv6 ND packets.
> 

No that is handled by other mechanisms. There are explicit handles to make sure
such traffic reaches the cpu.

> [ ... ]
> > +/* Called in atomic context */
> > +static void lan9645x_port_set_host_flood(struct dsa_switch *ds, int port,
> > +                                      bool uc, bool mc)
> > +{
> > +     struct lan9645x *lan9645x = ds->priv;
> > +     struct lan9645x_host_flood_work *w;
> > +
> > +     w = kzalloc_obj(*w, GFP_ATOMIC);
> 
> Is kzalloc_obj an available kernel function? Using it might cause a build
> error, as the standard API would be kzalloc(sizeof(*w), GFP_ATOMIC).
> 
> > +     if (!w)
> > +             return;
> 
> Are there consequences to returning silently when the allocation fails?
> If this allocation fails under memory pressure, the network stack will still
> assume the configuration was successfully applied, leaving the hardware state
> out of sync.
> 

But we can not return any error in the dsa op to the network stack, so I think
this is our only option.

> [ ... ]
> > +static int lan9645x_port_bridge_join(struct dsa_switch *ds, int port,
> > +                                  struct dsa_bridge bridge,
> > +                                  bool *tx_fwd_offload,
> > +                                  struct netlink_ext_ack *extack)
> > +{
> [ ... ]
> > +     /* The bridge puts ports in IFF_ALLMULTI before calling
> > +      * port_bridge_join, so clean up before the port is marked as bridged.
> > +      */
> > +     __lan9645x_port_set_host_flood(lan9645x, port, false, false);
> > +     lan9645x->bridge_mask |= BIT(p->chip_port);
> > +
> > +     mutex_unlock(&lan9645x->fwd_domain_lock);
> > +
> > +     /* Later: stp_state_set updates forwarding */
> > +
> > +     return 0;
> > +}
> 
> Could clearing the host flood masks here lead to lost hardware state when the
> port leaves the bridge? If a user previously enabled IFF_PROMISC or
> IFF_ALLMULTI on the standalone port, the software netdev flags remain set.
> When lan9645x_port_bridge_leave is called, it does not restore the hardware's
> promiscuous or multicast state to match the unchanged software flags.
> 

Yes this is a bug. I will split flood requests and register writes, and make sure
the requested state is restored on bridge leave.

> > +static void lan9645x_port_bridge_stp_state_set(struct dsa_switch *ds, int port,
> > +                                            u8 state)
> > +{
> > +     struct lan9645x *lan9645x;
> > +     struct lan9645x_port *p;
> > +     bool learn_ena;
> > +
> > +     lan9645x = ds->priv;
> > +     p = lan9645x_to_port(lan9645x, port);
> > +
> > +     mutex_lock(&lan9645x->fwd_domain_lock);
> > +
> > +     p->stp_state = state;
> > +
> > +     if (state == BR_STATE_FORWARDING)
> > +             lan9645x->bridge_fwd_mask |= BIT(p->chip_port);
> > +     else
> > +             lan9645x->bridge_fwd_mask &= ~BIT(p->chip_port);
> > +
> > +     learn_ena = (state == BR_STATE_LEARNING ||
> > +                  state == BR_STATE_FORWARDING) && p->learn_ena;
> > +
> > +     lan_rmw(ANA_PORT_CFG_LEARN_ENA_SET(learn_ena),
> > +             ANA_PORT_CFG_LEARN_ENA, lan9645x,
> > +             ANA_PORT_CFG(p->chip_port));
> > +
> > +     lan9645x_update_fwd_mask(lan9645x);
> > +     mutex_unlock(&lan9645x->fwd_domain_lock);
> > +}
> 
> Are ingress data frames correctly dropped for ports in non-forwarding STP
> states? When the port is removed from bridge_fwd_mask,
> lan9645x_update_fwd_mask calculates the source PGID mask as
> bridge_mask & bridge_fwd_mask & ~BIT(dp->index).
> 
> This evaluates to the set of all other forwarding ports in the bridge. Since
> ingress forwarding is not explicitly disabled on the port (for example, by
> setting ANA_PORT_CFG_PORTID_VAL to NONE), data frames received on a blocking
> or learning port can enter the analyzer and be forwarded.
> 

Yes this is a bug, I need to use a zero mask for the src pgid when a port is
in non-forwarding STP state.

> > +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);
> > +
> > +     /* Last port leaving clears bridge dev */
> > +     if (!lan9645x->bridge_mask)
> > +             lan9645x->bridge = NULL;
> > +
> > +     lan9645x_update_fwd_mask(lan9645x);
> > +
> > +     mutex_unlock(&lan9645x->fwd_domain_lock);
> > +}
> 
> Should p->learn_ena be reset when a port leaves the bridge?
> 
> During port_bridge_flags, p->learn_ena is set to true. When leaving the
> bridge, this flag remains true. The DSA core will transition the leaving
> port to BR_STATE_FORWARDING, which calls lan9645x_port_bridge_stp_state_set
> and leaves hardware learning enabled. This can pollute the shared FDB with
> MAC addresses from standalone ports, leading to silent packet drops if
> bridged ports attempt to forward traffic to them.


No I believe DSA core takes care of this with dsa_port_clear_brport_flags.