Re: [PATCH net-next v21 8/9] net: dsa: mt7530: generalize and move common function to lib module

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Clearing `__MT7530_VAWD2` in `mt7530_hw_vlan_add()` destroys the egress tag configuration of all other ports in the VLAN.
- [Medium] Missing clearing of the `__MT7530_VAWD1` register before configuring VLAN 0.
- [Low] Passing an `int *` to `regmap_field_read()` triggers an incompatible pointer types warning.
- [Low] Enabling mirroring before setting the destination port creates a race condition.
--

commit df02cf8f99d1289b20bc7e77c9448ff004d35005
Author: Christian Marangi <ansuelsmth@gmail.com>

net: dsa: mt7530: generalize and move common function to lib module

This commit introduces a library module for MT7530 and moves common functions
into it. The functions are generalized to use the reg_field regmap API while
applying the same logic, and mirror functions are cleaned up.

> diff --git a/drivers/net/dsa/mt7530-lib.c b/drivers/net/dsa/mt7530-lib.c
> --- /dev/null
> +++ b/drivers/net/dsa/mt7530-lib.c

[ ... ]

> +int mt7530_lib_port_mirror_add(struct mt7530_lib_priv *priv, int port,
> +			       struct dsa_mall_mirror_tc_entry *mirror,
> +			       bool ingress, struct netlink_ext_ack *extack)
> +{
> +	int monitor_port = -1;
> +	u32 val = 0;
> +
> +	/* Check for existent entry */
> +	if ((ingress ? priv->mirror_rx : priv->mirror_tx) & BIT(port))
> +		return -EEXIST;
> +
> +	regmap_field_read(priv->fields[MT7530_MIRROR_EN], &val);
> +	regmap_field_read(priv->fields[MT7530_MIRROR_PORT], &monitor_port);

[Severity: Low]
Does this pass an incompatible pointer type?

The monitor_port variable is declared as an int, but regmap_field_read()
expects an unsigned int * as its second argument. This can trigger a compiler
warning for incompatible pointer types, which may cause builds to fail if
configured with -Werror.

> +
> +	/* MT7530 only supports one monitor port */
> +	if (val && monitor_port != mirror->to_local_port)
> +		return -EEXIST;
> +
> +	regmap_field_write(priv->fields[MT7530_MIRROR_EN], 1);
> +	regmap_field_write(priv->fields[MT7530_MIRROR_PORT],
> +			   mirror->to_local_port);

[Severity: Low]
Could this create a race condition where mirroring is enabled before the
destination port is safely updated?

Writing the enable bit before configuring the destination port creates a small
window where mirroring is active but directed to the old or default destination
port. This might cause a momentary glitch where packets are mirrored to the
wrong port.

[ ... ]

> +static void mt7530_hw_vlan_add(struct mt7530_lib_priv *priv,
> +			       struct mt7530_hw_vlan_entry *entry)
> +{
> +	struct dsa_port *dp = dsa_to_port(priv->ds, entry->port);
> +	u8 new_members;
> +	u32 val;
> +
> +	new_members = entry->old_members | BIT(entry->port);
> +
> +	/* Clear VAWD registers from previous access */
> +	regmap_field_write(priv->fields[__MT7530_VAWD1], 0);
> +	regmap_field_write(priv->fields[__MT7530_VAWD2], 0);

[Severity: High]
Can this wipe out the egress tag configuration of all other ports in the VLAN?

When adding a port to an existing VLAN, mt7530_hw_vlan_update() copies the
current VLAN entry from hardware into the VAWD1 and VAWD2 registers. By
unconditionally zeroing __MT7530_VAWD2 here, the egress tag settings of all
prior ports in the VLAN are reset to zero (MT7530_VLAN_EGRESS_UNTAG).

Adding a user port to a VLAN could thus strip the CPU port of its required
MT7530_VLAN_EGRESS_STACK tagging, breaking VLAN traffic routing.

[ ... ]

> +int mt7530_lib_setup_vlan0(struct mt7530_lib_priv *priv)
> +{
> +	regmap_field_write(priv->fields[MT7530_VAWD_IVL_MAC], 1);
> +	regmap_field_write(priv->fields[MT7530_VAWD_VTAG_EN], 0);
> +	regmap_field_write(priv->fields[MT7530_VAWD_EG_CON], 1);

[Severity: Medium]
Should this clear the __MT7530_VAWD1 register before configuring VLAN 0?

Unlike mt7530_hw_vlan_add() which clears the register first, this sequence
performs read-modify-write operations on individual fields. Any residual bits
from previous VLAN or FDB hardware commands (like the PORT_STAG bit or reserved
bits) might leak into the configuration for VLAN 0. This state leak can
misconfigure the switch for VLAN-unaware bridging traffic.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260911225414.225980-1-ansuelsmth@gmail.com?part=8