Re: [PATCH net-next v5 4/7] net: dsa: motorcomm: Isolate all ports during chip setup

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] In yt921x_chip_reset() (drivers/net/dsa/motorcomm/chip.c), the
  new port-isolation loop does not run right after the hardware reset,…

--

net: dsa: motorcomm: Isolate all ports during chip setup

This commit writes all-ones to YT921X_PORTn_ISOLATION for every port in
yt921x_chip_reset(), so that all ports are isolated after the hardware
reset. It prepares for a later isolation refactoring. The commit message
notes that traffic can still leak before and during the reset.

> diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c
> index 7686995b803f3..fbf3a0cba49b3 100644
> --- a/drivers/net/dsa/motorcomm/chip.c
> +++ b/drivers/net/dsa/motorcomm/chip.c

[ ... ]

> @@ -2257,6 +2258,17 @@ static int yt921x_chip_reset(struct yt921x_priv *priv)
>  		return -EINVAL;
>  	}
>  
> +	/* Block all traffic as soon as we reset the hardware since it now acts
> +	 * as a dumb switch.
> +	 */
> +	for (int port = 0; port < YT921X_PORT_NUM; port++) {
> +		ctrl = ~(u32)0;
> +		res = yt921x_reg_write(priv, YT921X_PORTn_ISOLATION(port),
> +				       ctrl);
> +		if (res)
> +			return res;
> +	}
> +

[Severity: Low]
The commit message says ports are isolated "right after the hardware
reset". Does the placement of this loop match that?

In yt921x_chip_reset() the loop comes after the CPU tag TPID check, and
that check can return early once the reset has already completed:

    res = yt921x_reg_write(priv, YT921X_RST, YT921X_RST_HW);
    ...
    res = yt921x_reg_wait(priv, YT921X_RST, ~0, &val);
    if (res)
        return res;

    res = yt921x_reg_read(priv, YT921X_CPU_TAG_TPID, &val);
    if (res)
        return res;
    eth_p_tag = FIELD_GET(YT921X_CPU_TAG_TPID_TPID_M, val);
    if (eth_p_tag != ETH_P_YT921X) {
        ...
        return -EINVAL;
    }

If the TPID read fails, or the TPID does not match ETH_P_YT921X, the
function returns before any isolation register is written. The chip has
already been reset at that point, so it forwards between all ports.

yt921x_dsa_setup() then returns the error:

    mutex_lock(&priv->reg_lock);
    res = yt921x_chip_reset(priv);
    mutex_unlock(&priv->reg_lock);

    if (res)
        return res;

Probe fails and the hardware is not touched again. The switch keeps
bridging every front port.

yt921x_chip_detect() has already checked the chip ID, mode and extmode
before the reset. Would it be safe to move this loop to just after
yt921x_reg_wait() and before the TPID check? That would also match the
commit message.

The code is the same at the end of the series, in "net: dsa: motorcomm:
Support BR_FLOOD and BR_MCAST_FLOOD offloading", where the TPID check
still runs before the isolation loop.

>  	return 0;
>  }

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261108214450.1662362-1-mmyangfl%40gmail.com