Re: [PATCH net v2 2/2] net: dsa: mt7530: trap frames to conduit in MFC on EN7581 and AN7583

[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] The patch fixes the same missing MFC CPU_EN/CPU_PORT programming
  on two switch variants, but the Fixes: tag names only the EN7581…
- [Low] After this patch, the comments no longer match the code for
  EN7581/AN7583.

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but port
  mirroring on AN7583 writes the wrong bit.

--

net: dsa: mt7530: trap frames to conduit in MFC on EN7581 and AN7583

On EN7581 and AN7583, setting the MT7531_CFC CPU_PMAP bitmap by itself
does not get trapped control frames to the CPU port. This patch adds
ID_EN7581 and ID_AN7583 to mt753x_conduit_state_change(). MT7530_CPU_EN
and MT7530_CPU_PORT in MT753X_MFC are then programmed when the conduit
becomes operational.

> Fixes: 2b0229f67932 ("net: dsa: mt7530: Add EN7581 support")

[Severity: Low]
Should there also be a Fixes tag for the AN7583 commit?

2b0229f67932 did not add ID_AN7583. It was added by d76556db10bf41
("net: dsa: mt7530: Add AN7583 support"), and that commit added AN7583
with the same missing MFC programming.

Stable trees that have 2b0229f67932 but not d76556db10bf41 have no
ID_AN7583. The AN7583 half of the check in mt753x_conduit_state_change()
can't be applied there as written.

Perhaps something like:

Fixes: d76556db10bf ("net: dsa: mt7530: Add AN7583 support")

> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 20bb07fac0f7b..92b5e949ffab7 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -3338,7 +3338,8 @@ mt753x_conduit_state_change(struct dsa_switch *ds,
>  	 * interface is up.
>  	 */
>  	if (priv->id != ID_MT7530 && priv->id != ID_MT7621 &&
> -	    priv->id != ID_EN7528)
> +	    priv->id != ID_EN7528 &&
> +	    priv->id != ID_EN7581 && priv->id != ID_AN7583)
>  		return;

[Severity: Low]
This isn't a bug, but should the comments be updated now that this check
also covers EN7581 and AN7583?

The comment above this check still mentions only MT7530:

	/* Set the CPU port to trap frames to for MT7530. Trapped frames will be
	 * forwarded to the numerically smallest CPU port whose conduit
	 * interface is up.
	 */

The comment in mt753x_cpu_port_enable() says the CFC CPU_PMAP bitmap
chooses the trap destination. Its check also includes EN7581 and AN7583:

	/* Add the CPU port to the CPU port bitmap for MT7531 and the switch on
	 * the MT7988 SoC. Trapped frames will be forwarded to the CPU port that
	 * is affine to the inbound user port.
	 */
	if (priv->id == ID_MT7531 || priv->id == ID_MT7988 ||
	    priv->id == ID_EN7581 || priv->id == ID_AN7583)
		mt7530_set(priv, MT7531_CFC, MT7531_CPU_PMAP(BIT(port)));

The two comments now describe different forwarding rules for the same
chips. Neither one says that EN7581 and AN7583 need both registers, or
which register chooses the destination.

[Severity: Medium]
This is a pre-existing issue and was not introduced by this patch. Looking
at the AN7583 handling, MT753X_MIRROR_EN() in mt7530.h seems to be missing
ID_AN7583:

drivers/net/dsa/mt7530.h:
#define MT753X_MIRROR_EN(id)		((id == ID_MT7531 || \
					  id == ID_MT7988 || \
					  id == ID_EN7581) ? \
					 MT7531_MIRROR_EN : MT7530_MIRROR_EN)

MT753X_MIRROR_REG() and the MT753X_MIRROR_PORT_*() macros do include
ID_AN7583. On AN7583 the register is therefore MT7531_CFC, but the enable
bit is MT7530_MIRROR_EN (BIT(3)). That bit is inside MT7531_CPU_PMAP_MASK
(GENMASK(7, 0)).

Does mt753x_port_mirror_add() then add port 3 to the CPU port bitmap for
trapped frames, instead of enabling mirroring?

	val = mt7530_read(priv, MT753X_MIRROR_REG(priv->id));
	...
	val |= MT753X_MIRROR_EN(priv->id);
	...
	mt7530_write(priv, MT753X_MIRROR_REG(priv->id), val);

MT7531_MIRROR_EN (BIT(19)) is never set, so mirroring would not work on
AN7583. The -EEXIST check in mt753x_port_mirror_add() also tests the wrong
bit.

When the last mirror is removed, mt753x_port_mirror_del() does this:

	val &= ~MT753X_MIRROR_EN(priv->id);

That clears CPU_PMAP bit 3 in MT7531_CFC, not the mirror enable bit.

This seems to go back to d76556db10bf41 ("net: dsa: mt7530: Add AN7583
support"). Adding ID_AN7583 to MT753X_MIRROR_EN() would fix it.

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