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

[Vladimir Oltean <olteanv@gmail.com>]

On Sat, Mar 07, 2026 at 03:31:17AM +0000, Daniel Golle wrote:
> +static int mxl862xx_update_bridge(struct dsa_switch *ds,
> +				  struct mxl862xx_bridge *mxlbridge,
> +				  int port, bool join)

There's a lot of false sharing here. If you move the "if (join)" and
"if (!join)" blocks to their respective callers, you end up with a much
smaller truly common function.

> +{
> +	struct mxl862xx_priv *priv = ds->priv;
> +	struct dsa_port *dp;
> +	int member, ret;
> +
> +	if (join) {
> +		__set_bit(port, mxlbridge->portmap);
> +		priv->ports[port].bridge = mxlbridge;
> +	} else {
> +		__clear_bit(port, mxlbridge->portmap);
> +		priv->ports[port].bridge = NULL;
> +	}
> +
> +	/* Update all current bridge members' portmaps */
> +	for_each_set_bit(member, mxlbridge->portmap,
> +			 MXL862XX_MAX_BRIDGE_PORTS) {
> +		dp = dsa_to_port(ds, member);
> +
> +		/* 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);
> +
> +		priv->ports[member].learning = true;

Actually, dsa_port_inherit_brport_flags() / dsa_port_clear_brport_flags() ensures
the BR_LEARNING flag passed through mxl862xx_port_bridge_flags() will keep the
hardware in sync with the bridge. You don't need to set this to true or to false
at runtime. Just at initial probe time, before DSA has made any call.

> +		ret = mxl862xx_set_bridge_port(ds, member);
> +		if (ret)
> +			return ret;

What about going through all ports anyway, and return error at the end
if any bridge member portmap update failed? I know this patch set is
written assuming FW I/O failure is catastrophic, but here you may be
failing for a different port than the one which is leaving the bridge.
You can at least make an attempt to disable forwarding to/from that port.

> +	}
> +
> +	/* 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 (ret)
> +			return ret;
> +
> +		mxl862xx_port_fast_age(ds, port);
> +	}
> +
> +	return 0;
> +}
> +
> +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;
>  
> -	return MXL862XX_API_WRITE(ds->priv, MXL862XX_BRIDGEPORT_CONFIGSET, br_port_cfg);
> +	mxlbridge = mxl862xx_find_bridge(ds, bridge);
> +	if (!mxlbridge) {
> +		mxlbridge = mxl862xx_allocate_bridge(ds, bridge.num);
> +		if (IS_ERR(mxlbridge))
> +			return PTR_ERR(mxlbridge);
> +	}
> +
> +	return mxl862xx_update_bridge(ds, mxlbridge, port, true);
> +}
> +
> +static void mxl862xx_port_bridge_leave(struct dsa_switch *ds, int port,
> +				       struct dsa_bridge bridge)
> +{
> +	struct mxl862xx_bridge *mxlbridge;
> +
> +	mxlbridge = mxl862xx_find_bridge(ds, bridge);
> +	if (!mxlbridge)
> +		return;
> +
> +	mxl862xx_update_bridge(ds, mxlbridge, port, false);

Don't let this fail silently.

> +
> +	if (bitmap_empty(mxlbridge->portmap, MXL862XX_MAX_BRIDGE_PORTS))
> +		mxl862xx_free_bridge(ds, mxlbridge);
>  }
>  
>  static int mxl862xx_port_setup(struct dsa_switch *ds, int port)
> @@ -366,6 +614,262 @@ static void mxl862xx_phylink_get_caps(struct dsa_switch *ds, int port,
>  		  config->supported_interfaces);
>  }
>  
> +static int mxl862xx_port_fdb_add(struct dsa_switch *ds, int port,
> +				 const unsigned char *addr, u16 vid, struct dsa_db db)
> +{
> +	struct mxl862xx_mac_table_add param = { };
> +	struct mxl862xx_priv *priv = ds->priv;
> +	struct mxl862xx_bridge *mxlbridge;
> +	u16 fid;
> +	int ret;
> +
> +	switch (db.type) {
> +	case DSA_DB_PORT:
> +		fid = priv->ports[db.dp->index].fid;
> +		break;
> +
> +	case DSA_DB_BRIDGE:
> +		mxlbridge = mxl862xx_find_bridge(ds, db.bridge);
> +		if (!mxlbridge)
> +			return -ENOENT;
> +		fid = mxlbridge->bridge_id;
> +		break;
> +
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	param.port_id = cpu_to_le32(port);
> +	param.fid = cpu_to_le16(fid);
> +	param.static_entry = true;
> +	param.tci = cpu_to_le16(vid & 0xFFF);
> +	memcpy(param.mac, addr, ETH_ALEN);

ether_addr_copy() - similar comment for port_fdb_del()

> +
> +	ret = MXL862XX_API_WRITE(priv, MXL862XX_MAC_TABLEENTRYADD, param);
> +	if (ret)
> +		dev_err(ds->dev, "failed to add FDB entry on port %d\n", port);
> +
> +	return ret;
> +}
> +
> +static int mxl862xx_port_fdb_del(struct dsa_switch *ds, int port,
> +				 const unsigned char *addr, u16 vid, struct dsa_db db)
> +{
> +	struct mxl862xx_mac_table_remove param = { };
> +	struct mxl862xx_priv *priv = ds->priv;
> +	struct mxl862xx_bridge *mxlbridge;
> +	u16 fid;
> +	int ret;
> +
> +	switch (db.type) {
> +	case DSA_DB_PORT:
> +		fid = priv->ports[db.dp->index].fid;
> +		break;
> +
> +	case DSA_DB_BRIDGE:
> +		/* Use multi-port bridge FID */
> +		mxlbridge = mxl862xx_find_bridge(ds, db.bridge);
> +		if (!mxlbridge)
> +			return -ENOENT;
> +		fid = mxlbridge->bridge_id;
> +		break;
> +
> +	default:
> +		return -EOPNOTSUPP;
> +	}

Please group this together with the FID selection from port_fdb_add()
into a single helper.

> +
> +	param.fid = cpu_to_le16(fid);
> +	param.tci = cpu_to_le16(vid & 0xFFF);
> +	memcpy(param.mac, addr, ETH_ALEN);
> +
> +	ret = MXL862XX_API_WRITE(priv, MXL862XX_MAC_TABLEENTRYREMOVE, param);
> +	if (ret)
> +		dev_err(ds->dev, "failed to remove FDB entry on port %d\n", port);
> +
> +	return ret;
> +}
> +
> +static int mxl862xx_port_fdb_dump(struct dsa_switch *ds, int port,
> +				  dsa_fdb_dump_cb_t *cb, void *data)
> +{
> +	struct mxl862xx_mac_table_read param = { };
> +	struct mxl862xx_priv *priv = ds->priv;
> +	u32 entry_port_id;
> +	int ret;
> +
> +	while (true) {
> +		ret = MXL862XX_API_READ(priv, MXL862XX_MAC_TABLEENTRYREAD, param);
> +		if (ret)
> +			return ret;
> +
> +		if (param.last)
> +			break;
> +
> +		entry_port_id = le32_to_cpu(param.port_id);
> +
> +		if (entry_port_id == port)
> +			cb(param.mac, param.tci & 0x0FFF,
> +			   param.static_entry, data);

Catch and propagate the error from the FDB dump callback. See commit
21b52fed928e ("net: dsa: sja1105: fix broken backpressure in
.port_fdb_dump").

> +
> +		memset(&param, 0, sizeof(param));
> +	}
> +
> +	return 0;
> +}
> +
> +static int mxl862xx_port_mdb_add(struct dsa_switch *ds, int port,
> +				 const struct switchdev_obj_port_mdb *mdb,
> +				 struct dsa_db db)
> +{
> +	return mxl862xx_port_fdb_add(ds, port, mdb->addr, mdb->vid, db);
> +}
> +
> +static int mxl862xx_port_mdb_del(struct dsa_switch *ds, int port,
> +				 const struct switchdev_obj_port_mdb *mdb,
> +				 struct dsa_db db)
> +{
> +	return mxl862xx_port_fdb_del(ds, port, mdb->addr, mdb->vid, db);
> +}
> +
> +static int mxl862xx_set_ageing_time(struct dsa_switch *ds, unsigned int msecs)
> +{
> +	struct mxl862xx_cfg param;
> +	int ret;
> +
> +	ret = MXL862XX_API_READ(ds->priv, MXL862XX_COMMON_CFGGET, param);
> +	if (ret) {
> +		dev_err(ds->dev, "failed to read switch config\n");
> +		return ret;
> +	}
> +
> +	param.mac_table_age_timer = cpu_to_le32(MXL862XX_AGETIMER_CUSTOM);
> +	param.age_timer = cpu_to_le32(msecs / 1000);
> +	ret = MXL862XX_API_WRITE(ds->priv, MXL862XX_COMMON_CFGSET, param);
> +	if (ret)
> +		dev_err(ds->dev, "failed to set ageing\n");
> +
> +	return ret;
> +}
> +
> +static void mxl862xx_port_stp_state_set(struct dsa_switch *ds, int port,
> +					u8 state)
> +{
> +	struct mxl862xx_stp_port_cfg param = {
> +		.port_id = cpu_to_le16(port),
> +	};
> +	struct mxl862xx_priv *priv = ds->priv;
> +	int ret;
> +
> +	switch (state) {
> +	case BR_STATE_DISABLED:
> +		param.port_state = MXL862XX_STP_PORT_STATE_DISABLE;
> +		break;
> +	case BR_STATE_BLOCKING:
> +	case BR_STATE_LISTENING:
> +		param.port_state = MXL862XX_STP_PORT_STATE_BLOCKING;
> +		break;
> +	case BR_STATE_LEARNING:
> +		param.port_state = MXL862XX_STP_PORT_STATE_LEARNING;
> +		break;
> +	case BR_STATE_FORWARDING:
> +		param.port_state = MXL862XX_STP_PORT_STATE_FORWARD;
> +		break;
> +	default:
> +		dev_err(ds->dev, "invalid STP state: %d\n", state);
> +		return;
> +	}
> +
> +	ret = MXL862XX_API_WRITE(priv, MXL862XX_STP_PORTCFGSET, param);
> +	if (ret) {
> +		dev_err(ds->dev, "failed to set STP state on port %d\n", port);
> +		return;
> +	}
> +
> +	/* The firmware may re-enable MAC learning as a side-effect of
> +	 * entering LEARNING or FORWARDING state (per 802.1D defaults).
> +	 * Re-apply the driver's intended learning and metering config
> +	 * so that standalone ports keep learning disabled.
> +	 */
> +	ret = mxl862xx_set_bridge_port(ds, port);
> +	if (ret)
> +		dev_err(ds->dev, "failed to reapply brport flags on port %d\n", port);

Do you need to also manually fast-age the port if p->learning == false? Packets
may be coming in as soon as you put the port in MXL862XX_STP_PORT_STATE_LEARNING
already, and they will end up in the FDB even if from Linux' perspective,
it had learning off all along. It is a race condition.

> +}