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

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 [off-list ref]

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.

[ ... ]

+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