Re: [net-next PATCH v3 2/8] net: dsa: realtek: rtl8365mb: use dsa helpers for port iteration

[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: use dsa helpers for port iteration

This commit refactors rtl8365mb port iteration to use the
dsa_switch_for_each_*() helpers, adds a blocking setup phase for all
ports up front, and extends CPU-port isolation to include all user
ports.

> diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c
> index edbc16345d0d..7ddb82fcd026 100644
> --- a/drivers/net/dsa/realtek/rtl8365mb.c
> +++ b/drivers/net/dsa/realtek/rtl8365mb.c

[ ... ]

> @@ -1943,11 +1946,11 @@ static int rtl8365mb_reset_chip(struct realtek_priv *priv)
>  static int rtl8365mb_setup(struct dsa_switch *ds)
>  {
>  	struct realtek_priv *priv = ds->priv;
> +	struct dsa_port *cpu_dp, *dp;
>  	struct rtl8365mb_cpu *cpu;
> -	struct dsa_port *cpu_dp;
>  	struct rtl8365mb *mb;
> +	u32 userports_mask;
>  	int ret;
> -	int i;
>  
>  	mb = priv->chip_data;
>  	cpu = &mb->cpu;

[ ... ]

> +	/* Start with all ports blocked, including unused ports */
> +	dsa_switch_for_each_port(dp, ds) {
> +		struct rtl8365mb_port *p = &mb->ports[dp->index];
> +
> +		/* Set the initial STP state of all ports to DISABLED, otherwise
> +		 * ports will still forward frames to the CPU despite being
> +		 * administratively down by default.
> +		 */
> +		rtl8365mb_port_stp_state_set(ds, dp->index, BR_STATE_DISABLED);
> +
> +		/* Start with all port completely isolated */
> +		ret = rtl8365mb_port_set_isolation(priv, dp->index, 0);
> +		if (ret)
> +			goto out_teardown_irq;
> +
> +		/* Disable learning */
> +		ret = rtl8365mb_port_set_learning(priv, dp->index, false);
> +		if (ret)
> +			goto out_teardown_irq;
> +
> +		/* Set up per-port private data */
> +		p->priv = priv;
> +		p->index = dp->index;
> +	}
> +
> +	userports_mask = dsa_user_ports(ds);
> +
>  	/* Configure CPU tagging */
>  	dsa_switch_for_each_cpu_port(cpu_dp, ds) {
> -		cpu->mask |= BIT(cpu_dp->index);
> -
> +		/* Use the first CPU port as trap_port */
>  		if (cpu->trap_port == RTL8365MB_MAX_NUM_PORTS)
>  			cpu->trap_port = cpu_dp->index;
> +
> +		/* Forward to all user ports */
> +		ret = rtl8365mb_port_set_isolation(priv, cpu_dp->index,
> +						   userports_mask);
> +		if (ret)
> +			goto out_teardown_irq;
>  	}
> +
> +	cpu->mask = dsa_cpu_ports(ds);
>  	cpu->enable = cpu->mask > 0;
>  	ret = rtl8365mb_cpu_config(priv);
>  	if (ret)
>  		goto out_teardown_irq;
>  
> -	/* Configure ports */
> -	for (i = 0; i < priv->num_ports; i++) {
> -		struct rtl8365mb_port *p = &mb->ports[i];
> -
> -		if (dsa_is_unused_port(ds, i))
> -			continue;
> -
> +	/* Configure user ports */
> +	dsa_switch_for_each_user_port(dp, ds) {
>  		/* Forward only to the CPU */
> -		ret = rtl8365mb_port_set_isolation(priv, i, cpu->mask);
> +		ret = rtl8365mb_port_set_isolation(priv, dp->index, cpu->mask);
>  		if (ret)
>  			goto out_teardown_irq;

[Medium]
Does this refactor change behaviour for DSA-type ports (DSA_PORT_TYPE_DSA)?

Before the patch, the configuration loop iterated every non-unused port
and applied cpu->mask as the isolation mask, which included DSA-type
ports:

	for (i = 0; i < priv->num_ports; i++) {
		...
		if (dsa_is_unused_port(ds, i))
			continue;
		/* Forward only to the CPU */
		ret = rtl8365mb_port_set_isolation(priv, i, cpu->mask);
		...
	}

After the patch, the first pass sets isolation to 0 for every port, and
the subsequent passes only match CPU ports and user ports:

	#define dsa_switch_for_each_cpu_port(_dp, _ds) \
		dsa_switch_for_each_port((_dp), (_ds)) \
			if (dsa_port_is_cpu((_dp)))

	#define dsa_switch_for_each_user_port(_dp, _ds) \
		dsa_switch_for_each_port((_dp), (_ds)) \
			if (dsa_port_is_user((_dp)))

So a DSA-type port keeps isolation=0 from the initial pass and is never
reconfigured.

In addition, the masks derived from dsa_user_ports() and dsa_cpu_ports()
exclude DSA-type ports, so CPU and user ports will not include DSA link
ports in their isolation masks either, which looks like it would block
forwarding across cascaded switches in both directions.

The commit message describes the behaviour change only as "CPU isolation
now includes all user ports"; is the loss of DSA-port configuration
intentional, and if so should it be called out in the commit message?
-- 
pw-bot: cr