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

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

This is my reply to:
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260805-dsa_lan9645x_switch_driver_base-v11-0-007ebc983a0a%40microchip.com?part=5

> --- Patch [5]: [PATCH net-next v11 5/9] net: dsa: lan9645x: add bridge support ---
> commit a6410541102bca5214b00713a7a5b063ffd47ab2
> Author: Jens Emil Schulz Østergaard <jensemil.schulzostergaard@microchip.com>
>
> net: dsa: lan9645x: add bridge support
>
> This adds bridge offload to the lan9645x DSA driver: bridge join/leave,
> STP state and learning, port flooding flags, host flood handling through
> an ordered workqueue, and an ageing time setter. A single bridge device
> is supported.
>
> A few questions about the bridge integration below.
>
> > diff --git a/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> > index b253813518c20..c145d28ffbb99 100644
> > --- a/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> > +++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
>
> [ ... ]
>
> > @@ -265,15 +269,34 @@ 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) {
> > +		err = -ENOMEM;
> > +		goto err_mutex;
> > +	}
> > +
> >  	ds->mtu_enforcement_ingress = true;
> >  	ds->assisted_learning_on_cpu_port = true;
> >  	ds->fdb_isolation = true;
>
> Discovered by: opus-5 · Missed by: fable-5, sashiko-gemini · Confirmed by: fable-5
> Should ds->max_num_bridges be set to 1 here as well, next to
> ds->fdb_isolation?
>
> In net/dsa/dsa.c, dsa_bridge_num_get() treats a zero maximum as "no FDB
> isolation" and hands back a constant bridge number:
>
> 	/* Switches without FDB isolation support don't get unique
> 	 * bridge numbering
> 	 */
> 	if (!max)
> 		return 0;
>
> and net/dsa/port.c dsa_port_bridge_create() skips the range check for the
> same reason:
>
> 	bridge->num = dsa_bridge_num_get(br, ds->max_num_bridges);
> 	if (ds->max_num_bridges && !bridge->num) {
> 		NL_SET_ERR_MSG_MOD(extack,
> 				   "Range of offloadable bridges exceeded");
>
> So the driver requests FDB isolation while the isolation key the core
> returns is always 0, and the single-bridge limit is then open coded in
> lan9645x_port_bridge_join() with -EBUSY and "Only one bridge supported".
> The other in-tree drivers that set fdb_isolation (felix, sja1105,
> vitesse-vsc73xx-core, rtl8365mb, netc) also set max_num_bridges.
>
> The driver does not consume db.bridge.num anywhere in the final series, so
> nothing is mis-keyed today; it is the mismatch between the two fields that
> looks unintended.
>

Yes, i think this is right. I will set max_num_bridges=1 and remove the open
coded single-bridge limit.

> > +	/* 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.
> > +	 */
> > +	ds->ageing_time_max = 2U * MSEC_PER_SEC *
> > +			      FIELD_MAX(ANA_AUTOAGE_AGE_PERIOD);
> > +
>
> Discovered by: opus-5 · Missed by: fable-5, sashiko-gemini · Confirmed by: fable-5
> Only the upper ageing bound is published to the core here, leaving
> ds->ageing_time_min at 0.  Should ds->ageing_time_min = 2 * MSEC_PER_SEC be
> set too?
>
> net/dsa/switch.c dsa_switch_ageing_time() only rejects out-of-range
> requests when the corresponding limit is nonzero:
>
> 	if (ds->ageing_time_min && ageing_time < ds->ageing_time_min)
> 		return -ERANGE;
>
> 	if (ds->ageing_time_max && ageing_time > ds->ageing_time_max)
> 		return -ERANGE;
>
> net/bridge/br_stp.c br_set_ageing_time() imposes no lower bound either, so
> a request below 2000 ms reaches lan9645x_set_ageing_time() and is clamped
> while success is reported:
>
> 	ip link set br0 type bridge ageing_time 100
> 	  -> lan9645x_set_ageing_time(ds, 1000)
> 	     age_secs = max(1, 1000 / 1000 / 2) = max(1, 0) = 1
> 	     ANA_AUTOAGE_AGE_PERIOD = 1, i.e. entries age after 2 s
>
> For any msecs in 1..3999 the division yields 0 or 1 and the clamp forces 1,
> so every sub-2 s request silently becomes an effective 2 s hardware
> ageing while the bridge keeps and reports the value userspace asked for.
> drivers/net/dsa/netc/netc_main.c sets ds->ageing_time_min alongside
> ds->ageing_time_max.

We intentionally do not set ds->ageing_time_min because we would like to support
disabling ageing with an explicit zero value.

We generally round down to a 2s multiple, but I decided to round up in the
(0,2s] interval. If a user requests a very fast ageing time, I think it is
better to configure it to 2s, than to disable ageing.

>
> >  	dev_info(lan9645x->dev,
> >  		 "SKU features: max_ports=%d\n",
> >  		 lan9645x->num_phys_ports - lan9645x->num_port_dis);
> >  
> >  	return 0;
> > +
> > +err_mutex:
> > +	mutex_destroy(&lan9645x->fwd_domain_lock);
> > +	lan9645x_npi_port_deinit(lan9645x, lan9645x->npi);
> > +	return err;
> >  }
>
> [ ... ]
>
> > @@ -282,6 +305,282 @@ 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 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(msecs ? age_secs : 0),
> > +		ANA_AUTOAGE_AGE_PERIOD,
> > +		lan9645x, ANA_AUTOAGE);
> > +	return 0;
> > +}
>
> [ ... ]
>
> > +static void __lan9645x_port_set_host_flood(struct lan9645x *lan9645x)
> > +{
> > +	bool mc_ena, uc_ena;
> > +	u16 unbridged;
> > +
> > +	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.
> > +	 */
> > +	unbridged = ~lan9645x->bridge_mask & GENMASK(NUM_PHYS_PORTS - 1, 0);
> > +
> > +	uc_ena = !!(lan9645x->host_flood_uc_mask & unbridged);
>
> Discovered by: fable-5, opus-5 · Missed by: sashiko-gemini
> This isn't a bug, but could the commit message mention the two user
> visible behaviours added here?
>
> The log says only "Add support for hardware offloading of the bridge. We
> support a single bridge device.", while this masking means a
> .port_set_host_flood request for a bridged port is accepted and then
> dropped:
>
> net/dsa/user.c dsa_user_change_rx_flags() -> dsa_user_manage_host_flood()
> -> net/dsa/port.c dsa_port_set_host_flood() -> lan9645x_port_set_host_flood()
> -> lan9645x_host_flood_work_fn() -> __lan9645x_port_set_host_flood()
>
> Since br_manage_promisc() puts bridge ports in promiscuous mode when the
> bridge has VLAN filtering disabled, the CPU port bit is never set in
> PGID_UC/PGID_MC/PGID_MCIPV4/PGID_MCIPV6 for a bridged port, and no error
> is returned to userspace.  lan9645x_port_bridge_join() also revokes an
> already granted request through __lan9645x_port_set_host_flood(), and
> lan9645x_port_bridge_leave() restores it.  The sibling driver for the same
> hardware family, felix_port_set_host_flood() in
> drivers/net/dsa/ocelot/felix.c, honours the request unconditionally.
>

I can mention this in the commit message. I would add that even when VLAN
filtering is enabled, the conditions for when a bridged port is not put in
promiscuous mode are narrow. In most regular situations a bridged port is
put into promiscuous mode, even with VLAN filtering.

> The second omission is .set_ageing_time, ds->ageing_time_max and the
> "msecs == 0 disables automatic ageing" semantic, which are described only
> in the comment above and in the changelog below the --- marker.
>
> [ ... ]
>
> > +static void lan9645x_port_bridge_stp_state_set(struct dsa_switch *ds, int port,
> > +					       u8 state)
> > +{
>
> [ ... ]
>
> > +	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);
> > +}
>
> [ ... ]
>
> >  static const struct dsa_switch_ops lan9645x_switch_ops = {
> >  	.get_tag_protocol		= lan9645x_get_tag_protocol,
> >  
> > @@ -295,6 +594,15 @@ static const struct dsa_switch_ops lan9645x_switch_ops = {
> >  	/* MTU  */
> >  	.port_change_mtu		= lan9645x_change_mtu,
> >  	.port_max_mtu			= lan9645x_get_max_mtu,
> > +
> > +	/* Bridge integration */
> > +	.set_ageing_time		= lan9645x_set_ageing_time,
> > +	.port_pre_bridge_flags		= lan9645x_port_pre_bridge_flags,
> > +	.port_bridge_flags		= lan9645x_port_bridge_flags,
> > +	.port_bridge_join		= lan9645x_port_bridge_join,
> > +	.port_bridge_leave		= lan9645x_port_bridge_leave,
> > +	.port_stp_state_set		= lan9645x_port_bridge_stp_state_set,
> > +	.port_set_host_flood		= lan9645x_port_set_host_flood,
> >  };
>
> Discovered by: fable-5, opus-5 · Missed by: sashiko-gemini
> STP state and learning offload are registered here without .port_fast_age,
> so when net/dsa/port.c dsa_port_set_state() calls dsa_port_fast_age() there
> is no hardware MAC table flush.  Dynamic entries learned on a port would
> stay in the ANA MAC table after the port leaves BR_STATE_FORWARDING or
> BR_STATE_LEARNING, after BR_LEARNING is cleared, and after bridge leave.
>
> This is resolved later in the same series: the follow-up patch "net: dsa:
> lan9645x: add mac table integration" adds lan9645x_port_fast_age() calling
> lan9645x_mact_flush() and registers ".port_fast_age =
> lan9645x_port_fast_age", with the per-port flush implemented in
> lan9645x_mac.c using ANA_ANAGEFIL_PID_EN/PID_VAL plus two CMD_AGE scans.
> So the gap exists only at this intermediate patch; mentioning it here is
> just for completeness.
>
> Would it be worth reordering so the flush hook lands together with the STP
> offload, or is the intermediate state acceptable?

I will reorganize the commit order so bridge support lands after vlan, mactable
and mdb.