Re: [PATCH net-next v20 09/10] net: dsa: mt7530: generalize and move common function to lib module

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [High] Unsafe conversion of atomic command register triggers into multiple sequential Read-Modify-Write (RMW) operations.
- [High] Hardware staging registers for FDB and VLAN data will retain stale or garbage bits due to the switch from full-register overwrites to field-based RMWs.
- [High] `mt7530_port_enable()` incorrectly applies `PCR_MATRIX_MASK` to an unshifted port matrix value, isolating the port and dropping all traffic.
- [High] Missing `reg_fields` initialization for `ID_EN7528` leads to a NULL pointer dereference during port mirroring.
- [High] mt7530_lib_port_bridge_flags() disables MAC address learning when it is supposed to be enabled, and vice versa.
- [High] mt7530_lib_fdb_read() swaps the last two bytes of the MAC address when reading FDB entries.
- [High] Unchecked regmap_field_read() failures lead to the use of uninitialized stack variables, potentially leaking stack memory to userspace or corrupting hardware state.
- [Medium] `mt7530_lib_mib_reset()` inadvertently disables the hardware MIB counters for octets and breaks the flush sequence by writing 0 instead of 1.
--

--- Patch [9]: [PATCH net-next v20 09/10] net: dsa: mt7530: generalize and move common function to lib module ---

commit 302309065c4dc5c4e0f17f164c9e39732602b582
Author: Christian Marangi [off-list ref]

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

This commit moves common functions for the MT7530 switch into a separate
library module to prepare for Airoha support. It refactors register
accesses to use the regmap reg_field API instead of raw read/writes. A
new library struct is introduced to house the shared dependencies and
register fields.

[ ... ]

+void mt7530_lib_mib_reset(struct mt7530_lib_priv *priv)
+{
+	regmap_field_write(priv->fields[MT7530_CCR_MIB_ENABLE], 0);
+	regmap_field_write(priv->fields[MT7530_CCR_RX_OCT_CNT_GOOD], 0);
+	regmap_field_write(priv->fields[MT7530_CCR_RX_OCT_CNT_BAD], 0);
+	regmap_field_write(priv->fields[MT7530_CCR_TX_OCT_CNT_GOOD], 0);
+	regmap_field_write(priv->fields[MT7530_CCR_TX_OCT_CNT_BAD], 0);

[Severity: Medium]
Does this correctly clear the counters? The original code wrote a 1 to flush
and clear the counters. Writing 0 to a write-1-to-clear field might fail to
reset them.

[ ... ]

+int mt7530_lib_fdb_cmd(struct mt7530_lib_priv *priv, enum mt7530_fdb_cmd cmd,
+		       u32 mat, u32 *rsp)
+{
+	u32 val;
+	int ret;
+
+	ret = regmap_field_write(priv->fields[MT7530_ATC_MAT], mat);
+	if (ret)
+		return ret;
+
+	ret = regmap_field_write(priv->fields[MT7530_ATC_CMD], cmd);
+	if (ret)
+		return ret;
+
+	ret = regmap_field_write(priv->fields[MT7530_ATC_BUSY], 1);

[Severity: High]
Can splitting the FDB command execution into multiple read-modify-writes
corrupt the MAC table operations? The previous implementation constructed the
32-bit value and wrote it atomically, preventing intermediate hardware states.

[ ... ]

+static void mt7530_lib_fdb_write(struct mt7530_lib_priv *priv, u16 vid,
+				 u8 port_mask, const u8 *mac,
+				 u8 aging, u8 type)
+{
+	regmap_field_write(priv->fields[MT7530_ATWD_CVID], vid);
+	regmap_field_write(priv->fields[MT7530_ATWD_IVL], 1);

[Severity: High]
Could this approach leak uninitialized or stale data into the FDB table?
Writing fields individually performs a read-modify-write, preserving undefined
bits from prior operations, whereas the old code explicitly cleared
unspecified bits by writing fully initialized arrays directly.

[ ... ]

+void mt7530_lib_fdb_read(struct mt7530_lib_priv *priv, struct mt7530_fdb *fdb)
+{
+	u32 val;
+
+	regmap_field_read(priv->fields[MT7530_ATRD_CVID], &val);
+	fdb->vid = val;

[Severity: High]
If regmap_field_read fails, could we end up assigning uninitialized stack
memory from val to the fdb fields? This might leak kernel stack data to
userspace during an FDB dump.

[ ... ]

+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;
+	u32 val;
+
+	/* 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);

[Severity: High]
Is it possible for priv->fields to lack an initialization here? The
ID_EN7528 entry in mt753x_table appears to be missing its reg_fields
setup, which could lead to a NULL pointer dereference.

[Severity: High]
Could a failure in regmap_field_read leave val and monitor_port
uninitialized? Using these uninitialized values in the subsequent condition
might cause unpredictable branch behavior.

[ ... ]

+int mt7530_lib_port_bridge_flags(struct mt7530_lib_priv *priv, int port,
+				 struct switchdev_brport_flags flags,
+				 struct netlink_ext_ack *extack)
+{
+	struct dsa_switch *ds = priv->ds;
+
+	if (flags.mask & BR_LEARNING)
+		regmap_fields_write(priv->fields[MT7530_PSC_SA_DIR],
+				    port, !!(flags.val & BR_LEARNING));

[Severity: High]
Does this invert the learning logic? The hardware bit SA_DIS disables
learning, so passing the boolean value of BR_LEARNING directly might
disable learning when it is supposed to be enabled, and vice versa.

[ ... ]

+static int mt7530_vlan_cmd(struct mt7530_lib_priv *priv, enum mt7530_vlan_cmd cmd,
+			   u16 vid)
+{
+	u32 val;
+	int ret;
+
+	ret = regmap_field_write(priv->fields[MT7530_VTCR_FUNC], cmd);
+	if (ret)
+		return ret;
+
+	ret = regmap_field_write(priv->fields[MT7530_VTCR_VID], vid);

[Severity: High]
Will breaking this command register access into multiple read-modify-write
operations cause issues with the hardware state machine? The original code
wrote the entire VTCR register in a single atomic operation.

[ ... ]

+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);
+
+	/* Validate the entry with independent learning, create egress tag per
+	 * VLAN and joining the port as one of the port members.
+	 */
+	regmap_field_write(priv->fields[MT7530_VAWD_IVL_MAC], 1);

[Severity: High]
Might this leave stale or garbage bits in the VAWD1 register? The original
logic performed a full 32-bit register overwrite, explicitly clearing
unspecified bits like EG_CON. Using sequential regmap_field_write operations
performs read-modify-writes, which retains any previous unmapped bits.

[ ... ]

[ ... ]

+	{ MT7530_ATRD_ENT_STATUS, REG_FIELD(MT7530_ATRD, 2, 3), },
+	{ MT7530_ATRD_MAC_BYTE_5, REG_FIELD(MT7530_TSRA2, 24, 31), },
+	{ MT7530_ATRD_MAC_BYTE_4, REG_FIELD(MT7530_TSRA2, 16, 23), },

[Severity: High]
Are the bit ranges for MAC_BYTE_4 and MAC_BYTE_5 swapped here? The original
logic mapped MAC_BYTE_4 to bits 24-31 and MAC_BYTE_5 to bits 16-23 in the
TSRA2 register, causing corrupted MAC addresses on FDB read.

[ ... ]

[Severity: High]
Does this effectively isolate the port by dropping all traffic? The bitmask
in pm is now constructed without the PCR_MATRIX() shift, but it is passed
directly into mt7530_rmw() where PCR_MATRIX_MASK is applied. Since the
unshifted bits fall outside the mask, this will write zero to the matrix
field.

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