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

[Jakub Kicinski <kuba@kernel.org>]

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 change implements hardware bridge offload for rtl8365mb by programming
per-port isolation masks and an EFID derived from the DSA bridge number.
The common helpers rtl83xx_port_bridge_join() and rtl83xx_port_bridge_leave()
are added for reuse across realtek drivers.

> diff --git a/drivers/net/dsa/realtek/rtl83xx.c b/drivers/net/dsa/realtek/rtl83xx.c
> index 2b9bd4462714..3ab91cd82743 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
> + * @ds: DSA switch instance
> + * @port: port index
> + * @bridge: bridge being joined
> + * @tx_forward_offload: if the switch can offload TX forwarding
> + * @extack: netlink extended ack for reporting errors
> + *
> + * This function handles joining a port to a bridge. It updates the port
> + * isolation masks and EFID.
> + *
> + * Context: Can sleep.
> + * Return: 0 on success, negative value for failure.
> + */
> +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;

[Medium]
If port_add_isolation() succeeds for one or more peer ports and then fails
for a subsequent peer, this returns without undoing the earlier updates.

Looking at dsa_port_bridge_join() in net/dsa/port.c, the failure path goes
to out_rollback which calls dsa_port_bridge_destroy() but does not dispatch
DSA_NOTIFIER_BRIDGE_LEAVE, so rtl83xx_port_bridge_leave() is not invoked to
revert the partial state.

Can peer ports end up retaining BIT(port) in their forwarding mask even
though port never successfully joined the bridge, leaving frames from
bridge members forwardable to a non-member?

> +
> +		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;

[Medium]
Similarly, if this call fails after peers have already been updated, the
peers keep BIT(port) set without any rollback. Should this path walk the
peers again and clear BIT(port) from the ones already modified before
returning the error?

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

[Medium]
If port_set_efid() fails here, both the peer ports' isolation masks and
this port's isolation mask have already been updated to include the bridge
membership, but the function returns an error as if the join did not
happen. Should the isolation mask updates be reverted (and a plain EFID 0
restored where needed) before returning?

> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_NS_GPL(rtl83xx_port_bridge_join, "REALTEK_DSA");
> +

[ ... ]