Re: [PATCH net-next v2 2/2] net: dsa: mxl862xx: implement bridge offloading

[Daniel Golle <daniel@makrotopia.org>]

On Tue, Mar 10, 2026 at 05:53:40PM +0200, Vladimir Oltean wrote:
> Hi Daniel,
> 
> On Tue, Mar 10, 2026 at 12:40:29AM +0000, Daniel Golle wrote:
> >  * drop manually resetting port learning state on bridge<->standalone
> >    transitions, DSA framework takes care of that
> (...)
> > +	/* Revert leaving port to its single-port bridge */
> > +	if (!join) {
> > +		dp = dsa_to_port(ds, port);
> > +
> > +		bitmap_zero(priv->ports[port].portmap, MXL862XX_MAX_BRIDGE_PORTS);
> > +		__set_bit(dp->cpu_dp->index, priv->ports[port].portmap);
> > +		priv->ports[port].flood_block = 0;
> > +		priv->ports[port].learning = false;
> 
> So is this needed or not? Change log says "drop" but code says "keep".
> 
> The core does:
> dsa_port_bridge_leave()
> -> dsa_port_switchdev_unsync_attrs()
>    -> dsa_port_clear_brport_flags()
>       -> dsa_port_bridge_flags() // BR_LEARNING in mask and not in val

Sorry, I just forgot to remove it there. It should not be needed, but
I'll rebuild and test without it to be sure.

> 
> > +		ret = mxl862xx_set_bridge_port(ds, port);
> > +		if (err)
> > +			ret = err;
> > +
> > +		mxl862xx_port_fast_age(ds, port);
> > +	}
> (...)
> >  * manually mxl862xx_port_fast_age() in mxl862xx_port_stp_state_set()
> >    to avoid FDB poisoning due to race condition
> (...)
> > +	/* Revert leaving port to its single-port bridge */
> > +	if (!join) {
> > +		dp = dsa_to_port(ds, port);
> > +
> > +		bitmap_zero(priv->ports[port].portmap, MXL862XX_MAX_BRIDGE_PORTS);
> > +		__set_bit(dp->cpu_dp->index, priv->ports[port].portmap);
> > +		priv->ports[port].flood_block = 0;
> > +		priv->ports[port].learning = false;
> > +		ret = mxl862xx_set_bridge_port(ds, port);
> > +		if (err)
> > +			ret = err;
> > +
> > +		mxl862xx_port_fast_age(ds, port);
> > +	}
> 
> I only requested this to be done on mxl862xx_port_stp_state_set(), as a
> consequence to your workaround, not on mxl862xx_port_bridge_leave() ->
> mxl862xx_update_bridge().

I'll remove that then, it was already present in v1.

> 
> The framework actually has logic to fast age the FDB. A standalone port
> is in BR_STATE_FORWARDING, and a leaving/joining bridge port goes
> through BR_STATE_DISABLED - del_nbp() -> br_stp_disable_port().
> So we have a guaranteed STP transition based on which this hook runs:
> 
> dsa_port_switchdev_unsync_attrs():
> 	/* Port left the bridge, put in BR_STATE_DISABLED by the bridge layer,
> 	 * so allow it to be in BR_STATE_FORWARDING to be kept functional
> 	 */
> 	dsa_port_set_state_now(dp, BR_STATE_FORWARDING, true);
> ->
> 		/* Fast age FDB entries or flush appropriate forwarding database
> 		 * for the given port, if we are moving it from Learning or
> 		 * Forwarding state, to Disabled or Blocking or Listening state.
> 		 * Ports that were standalone before the STP state change don't
> 		 * need to fast age the FDB, since address learning is off in
> 		 * standalone mode.
> 		 */
> 
> 		if ((dp->stp_state == BR_STATE_LEARNING ||
> 		     dp->stp_state == BR_STATE_FORWARDING) &&
> 		    (state == BR_STATE_DISABLED ||
> 		     state == BR_STATE_BLOCKING ||
> 		     state == BR_STATE_LISTENING))
> 			dsa_port_fast_age(dp);
> 
> so I think fast aging is unnecessary here.
> 
> Your workaround is different, DSA doesn't know that
> dsa_port_set_state(BR_STATE_LEARNING) with dp->learning == false
> actually temporarily enables learning. It assumes it doesn't, so it
> doesn't call dsa_port_fast_age(). That's why you have to do it.

Understood. Thank you for explaining the context in detail.

> 
> 
> Did you reply to my comment from v1 to remove the "bool join" false
> sharing from mxl862xx_update_bridge()? Because you didn't, and I'm not
> sure why.

I wanted to reply to that but then forgot...
You sample code below makes it much more clear also what you meant,
and I will follow your suggestion in v3.


> 
> I meant to see:
> 
> static int mxl862xx_sync_bridge_members(struct dsa_switch *ds,
> 					struct mxl862xx_bridge *mxlbridge)
> {
> 	struct mxl862xx_priv *priv = ds->priv;
> 	int member, ret = 0;
> 
> 	/* Update all current bridge members' portmaps */
> 	for_each_set_bit(member, mxlbridge->portmap,
> 			 MXL862XX_MAX_BRIDGE_PORTS) {
> 		struct dsa_port *dp = dsa_to_port(ds, member);
> 		int err;
> 
> 		/* Build portmap: CPU port + all bridge members except self */
> 		bitmap_copy(priv->ports[member].portmap, mxlbridge->portmap,
> 			    MXL862XX_MAX_BRIDGE_PORTS);
> 		__clear_bit(member, priv->ports[member].portmap);
> 		__set_bit(dp->cpu_dp->index, priv->ports[member].portmap);
> 
> 		err = mxl862xx_set_bridge_port(ds, member);
> 		if (err)
> 			ret = err;
> 	}
> 
> 	return ret;
> }
> 
> static int mxl862xx_port_bridge_join(struct dsa_switch *ds, int port,
> 				     struct dsa_bridge bridge,
> 				     bool *tx_fwd_offload,
> 				     struct netlink_ext_ack *extack)
> {
> 	struct mxl862xx_bridge *mxlbridge;
> 
> 	mxlbridge = mxl862xx_find_bridge(ds, bridge);
> 	if (!mxlbridge) {
> 		mxlbridge = mxl862xx_allocate_bridge(ds, bridge.num);
> 		if (IS_ERR(mxlbridge))
> 			return PTR_ERR(mxlbridge);
> 	}
> 
> 	__set_bit(port, mxlbridge->portmap);
> 	priv->ports[port].bridge = mxlbridge;
> 
> 	/* The operation may fail mid way and there is no way to restore
> 	 * the driver in sync with a known FW state. So we consider FW
> 	 * I/O failure as catastrophic, no point to complicate the
> 	 * driver by restoring mxlbridge->portmap or the bridge pointer.
> 	 */
> 	return mxl862xx_sync_bridge_members(ds, mxlbridge);
> }
> 
> static void mxl862xx_port_bridge_leave(struct dsa_switch *ds, int port,
> 				       struct dsa_bridge bridge)
> {
> 	struct dsa_port *dp = dsa_to_port(ds, port);
> 	struct mxl862xx_bridge *mxlbridge;
> 	int err;
> 
> 	mxlbridge = mxl862xx_find_bridge(ds, bridge);
> 	if (!mxlbridge)
> 		return;
> 
> 	__clear_bit(port, mxlbridge->portmap);
> 	priv->ports[port].bridge = NULL;
> 
> 	err = mxl862xx_sync_bridge_members(ds, mxlbridge);
> 	if (err) {
> 		dev_err(ds->dev,
> 			"failed to sync bridge members after port %d left: %pe\n",
> 			port, ERR_PTR(err));
> 	}
> 
> 	/* Revert leaving port, omitted by the sync above, to its
> 	 * single-port bridge
> 	 */
> 	bitmap_zero(priv->ports[port].portmap, MXL862XX_MAX_BRIDGE_PORTS);
> 	__set_bit(dp->cpu_dp->index, priv->ports[port].portmap);
> 	priv->ports[port].flood_block = 0;
> 	err = mxl862xx_set_bridge_port(ds, port);
> 	if (err) {
> 		dev_err(ds->dev,
> 			"failed to update bridge port %d state: %pe\n", port,
> 			ERR_PTR(err));
> 	}
> 
> 	return MXL862XX_API_WRITE(ds->priv, MXL862XX_BRIDGEPORT_CONFIGSET, br_port_cfg);
> 	if (bitmap_empty(mxlbridge->portmap, MXL862XX_MAX_BRIDGE_PORTS))
> 		mxl862xx_free_bridge(ds, mxlbridge);
> }