Re: [net-next PATCH v3 6/8] net: dsa: realtek: rtl8365mb: add port_bridge_{join,leave}

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: dsa: realtek: rtl8365mb: add port_bridge_{join,leave}

This commit implements hardware offloading of bridge functionality for the
rtl8365mb switch by configuring per-port isolation registers and Extended
Filtering IDs (EFIDs) for Independent VLAN Learning (IVL).

> diff --git a/drivers/net/dsa/realtek/rtl83xx.c b/drivers/net/dsa/realtek/rtl83xx.c
> index 2b9bd4462714b..3ab91cd82743a 100644
> --- a/drivers/net/dsa/realtek/rtl83xx.c
> +++ b/drivers/net/dsa/realtek/rtl83xx.c
[ ... ]
> @@ -325,6 +325,120 @@ void rtl83xx_reset_deassert(struct realtek_priv *priv)
>  	gpiod_set_value(priv->reset, false);
>  }
>  
> +/**
> + * rtl83xx_port_bridge_join() - join a port to a bridge
[ ... ]
> +int rtl83xx_port_bridge_join(struct dsa_switch *ds, int port,
> +			     struct dsa_bridge bridge,
> +			     bool *tx_forward_offload,
> +			     struct netlink_ext_ack *extack)
> +{
> +	struct realtek_priv *priv = ds->priv;
> +	struct dsa_port *dp;
> +	u32 mask = 0;
> +	int ret;
> +
> +	if (!priv->ops->port_add_isolation)
> +		return -EOPNOTSUPP;
> +
> +	dev_dbg(priv->dev, "bridge %d join port %d\n", bridge.num, port);
> +
> +	/* Add this port to the isolation group of every other port
> +	 * offloading this bridge.
> +	 */
> +	dsa_switch_for_each_user_port(dp, ds) {
> +		/* Handle this port after */
> +		if (dp->index == port)
> +			continue;
> +
> +		/* Skip ports that are not in this bridge */
> +		if (!dsa_port_offloads_bridge(dp, &bridge))
> +			continue;
> +
> +		ret = priv->ops->port_add_isolation(priv, dp->index, BIT(port));
> +		if (ret)
> +			return ret;

What happens if this port_add_isolation() fails midway through the loop,
or if any subsequent operations fail?

It looks like the function returns immediately without rolling back the
isolation masks of the ports that were already successfully modified.

This would leave the hardware partially configured, allowing the modified
bridge ports to forward traffic to the standalone port in hardware, while
the DSA core aborted the join in software, leading to a network isolation
regression.

Should there be an error unwinding path here that reverts the isolation
masks for the previously configured ports using port_remove_isolation()?

> +
> +		mask |= BIT(dp->index);
> +	}
> +
> +	/* Add those ports to the isolation group of this port */
> +	ret = priv->ops->port_add_isolation(priv, port, mask);
> +	if (ret)
> +		return ret;
> +
> +	/* Use the bridge number as the EFID for this port */
> +	if (priv->ops->port_set_efid) {
> +		ret = priv->ops->port_set_efid(priv, port, bridge.num);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}