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

[Vladimir Oltean <olteanv@gmail.com>]

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

> +		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().

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.


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 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);
}