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

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:

quoted

 * drop manually resetting port learning state on bridge<->standalone
   transitions, DSA framework takes care of that

(...)

quoted

+	/* 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.

quoted

+		ret = mxl862xx_set_bridge_port(ds, port);
+		if (err)
+			ret = err;
+
+		mxl862xx_port_fast_age(ds, port);
+	}

(...)

quoted

 * manually mxl862xx_port_fast_age() in mxl862xx_port_stp_state_set()
   to avoid FDB poisoning due to race condition

(...)

quoted

+	/* 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);
}