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

[Paolo Abeni <pabeni@redhat.com>]

On 3/27/26 2:01 AM, Daniel Golle wrote:
> +static int mxl862xx_set_bridge_port(struct dsa_switch *ds, int port)
> +{
> +	struct mxl862xx_bridge_port_config br_port_cfg = {};
> +	struct dsa_port *dp = dsa_to_port(ds, port);
> +	struct mxl862xx_priv *priv = ds->priv;
> +	struct mxl862xx_port *p = &priv->ports[port];
> +	u16 bridge_id = dp->bridge ?
> +		priv->bridges[dp->bridge->num] : p->fid;
> +	bool enable;
> +	int i, idx;
> +
> +	br_port_cfg.bridge_port_id = cpu_to_le16(port);
> +	br_port_cfg.bridge_id = cpu_to_le16(bridge_id);
> +	br_port_cfg.mask = cpu_to_le32(MXL862XX_BRIDGE_PORT_CONFIG_MASK_BRIDGE_ID |
> +				       MXL862XX_BRIDGE_PORT_CONFIG_MASK_BRIDGE_PORT_MAP |
> +				       MXL862XX_BRIDGE_PORT_CONFIG_MASK_MC_SRC_MAC_LEARNING |
> +				       MXL862XX_BRIDGE_PORT_CONFIG_MASK_EGRESS_SUB_METER);

AI reviews say:

Does this silently switch the hardware from Independent VLAN Learning
(IVL) to Shared VLAN Learning (SVL)? The mask no longer includes
MXL862XX_BRIDGE_PORT_CONFIG_MASK_VLAN_BASED_MAC_LEARNING, and the
vlan_src_mac_vid_enable and vlan_dst_mac_vid_enable fields are not set.
For a VLAN-aware bridge, this appears to break VLAN isolation
boundaries, as a MAC address learned on one VLAN could affect forwarding
decisions on other VLANs.

[ ... ]
> +static int mxl862xx_add_single_port_bridge(struct dsa_switch *ds, int port)
> +{
> +	struct dsa_port *dp = dsa_to_port(ds, port);
> +	struct mxl862xx_priv *priv = ds->priv;
> +	int ret;
> +
> +	ret = mxl862xx_allocate_bridge(priv, &priv->ports[port].fid);
> +	if (ret) {
> +		dev_err(ds->dev, "failed to allocate a bridge for port %d\n", port);
> +		return ret;
> +	}
> +
> +	priv->ports[port].learning = false;
> +	bitmap_zero(priv->ports[port].portmap, MXL862XX_MAX_BRIDGE_PORTS);
> +	__set_bit(dp->cpu_dp->index, priv->ports[port].portmap);
> +
> +	ret = mxl862xx_set_bridge_port(ds, port);
> +	if (ret)
> +		return ret;

AI review says:

If mxl862xx_set_bridge_port() fails, does this leak the newly allocated
hardware bridge? It looks like there is no call to mxl862xx_free_bridge()
on this error path.

[...]
> +static int mxl862xx_port_bridge_join(struct dsa_switch *ds, int port,
> +				     const struct dsa_bridge bridge,
> +				     bool *tx_fwd_offload,
> +				     struct netlink_ext_ack *extack)
>  {
> -	struct mxl862xx_bridge_port_config br_port_cfg = {};
> -	struct dsa_port *dp = dsa_to_port(ds, port);
> -	struct mxl862xx_bridge_alloc br_alloc = {};
> +	struct mxl862xx_priv *priv = ds->priv;
> +	u16 fw_id;
>  	int ret;
>  
> -	ret = MXL862XX_API_READ(ds->priv, MXL862XX_BRIDGE_ALLOC, br_alloc);
> -	if (ret) {
> -		dev_err(ds->dev, "failed to allocate a bridge for port %d\n", port);
> -		return ret;
> +	if (!priv->bridges[bridge.num]) {
> +		ret = mxl862xx_allocate_bridge(priv, &fw_id);
> +		if (ret)
> +			return ret;
> +
> +		priv->bridges[bridge.num] = fw_id;
> +
> +		/* Free bridge here on error, DSA rollback won't. */
> +		ret = mxl862xx_sync_bridge_members(ds, &bridge);
> +		if (ret) {
> +			mxl862xx_free_bridge(ds, &bridge);
> +			return ret;

AI review says:

If mxl862xx_sync_bridge_members() encounters an error partway through
updating the ports in its loop, are the already-updated ports left pointing
to a bridge ID that is then freed here?
It seems the software portmaps and hardware states of the preceding ports
are not rolled back, which might leave them corrupted and permanently
referencing a freed bridge.



> +		}
> +
> +		return 0;
>  	}
>  
> -	br_port_cfg.bridge_id = br_alloc.bridge_id;
> -	br_port_cfg.bridge_port_id = cpu_to_le16(port);
> -	br_port_cfg.mask = cpu_to_le32(MXL862XX_BRIDGE_PORT_CONFIG_MASK_BRIDGE_ID |
> -				       MXL862XX_BRIDGE_PORT_CONFIG_MASK_BRIDGE_PORT_MAP |
> -				       MXL862XX_BRIDGE_PORT_CONFIG_MASK_MC_SRC_MAC_LEARNING |
> -				       MXL862XX_BRIDGE_PORT_CONFIG_MASK_VLAN_BASED_MAC_LEARNING);
> -	br_port_cfg.src_mac_learning_disable = true;
> -	br_port_cfg.vlan_src_mac_vid_enable = false;
> -	br_port_cfg.vlan_dst_mac_vid_enable = false;
> -	/* As this function is only called for user ports it is safe to rely on
> -	 * cpu_dp being valid
> -	 */
> -	br_port_cfg.bridge_port_map[0] = cpu_to_le16(BIT(dp->cpu_dp->index));
> +	return mxl862xx_sync_bridge_members(ds, &bridge);
> +}
> +
> +static void mxl862xx_port_bridge_leave(struct dsa_switch *ds, int port,
> +				       const struct dsa_bridge bridge)
> +{
> +	struct dsa_port *dp = dsa_to_port(ds, port);
> +	struct mxl862xx_priv *priv = ds->priv;
> +	struct mxl862xx_port *p = &priv->ports[port];
> +	int err;
> +
> +	err = mxl862xx_sync_bridge_members(ds, &bridge);
> +	if (err)
> +		dev_err(ds->dev,
> +			"failed to sync bridge members after port %d left: %pe\n",
> +			port, ERR_PTR(err));

AI review says:

Is the leaving port still present in dp->bridge when this function is
called?
The DSA core typically clears dp->bridge after the leave callback returns.
If so, mxl862xx_sync_bridge_members() would still iterate over the
leaving port and incorrectly add it to the hardware portmaps of all
remaining bridge members.

>  
> -	return MXL862XX_API_WRITE(ds->priv, MXL862XX_BRIDGEPORT_CONFIGSET, br_port_cfg);
> +	/* Revert leaving port, omitted by the sync above, to its
> +	 * single-port bridge
> +	 */
> +	bitmap_zero(p->portmap, MXL862XX_MAX_BRIDGE_PORTS);
> +	__set_bit(dp->cpu_dp->index, p->portmap);
> +	p->flood_block = 0;
> +	err = mxl862xx_set_bridge_port(ds, port);

AI review says:

Could there be two issues during this revert?
First, if dp->bridge is still set as noted above, mxl862xx_set_bridge_port()
will evaluate dp->bridge to true and use the shared bridge's hardware FID
instead of the port's standalone FID, keeping the port assigned to the
shared bridge.
Second, if learning was enabled while the port was in the bridge,
p->learning
remains true here. Should p->learning be set to false when the port reverts
to a standalone port to prevent inappropriate MAC learning?