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

[sashiko-bot@kernel.org]

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 <ansuelsmth@gmail.com>

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.

> diff --git a/drivers/net/dsa/mt7530-lib.c b/drivers/net/dsa/mt7530-lib.c
> new file mode 100644
> index 0000000000000..01ed941efc892
> --- /dev/null
> +++ b/drivers/net/dsa/mt7530-lib.c
> @@ -0,0 +1,1106 @@
[ ... ]
> +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.

> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index d01772cfe3a2d..f313a7a7bfef1 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
[ ... ]
> @@ -24,10 +24,159 @@
>  #include <net/pkt_cls.h>
>  
>  #include "mt7530.h"
> +#include "mt7530-lib.h"
>  
>  #define MT7530_STATS_POLL_INTERVAL	(1 * HZ)
>  #define MT7530_STATS_RATE_LIMIT		(HZ / 10)
>  
> +static const struct mt7530_reg_field mt753x_fields[] = {
[ ... ]
> +	{ 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.

[ ... ]
> @@ -1373,7 +1141,7 @@ mt7530_port_enable(struct dsa_switch *ds, int port,
>  	if (dsa_port_is_user(dp)) {
>  		struct dsa_port *cpu_dp = dp->cpu_dp;
>  
> -		priv->ports[port].pm |= PCR_MATRIX(BIT(cpu_dp->index));
> +		priv->ports[port].pm |= BIT(cpu_dp->index);
>  	}
>  	priv->ports[port].enable = true;
>  	mt7530_rmw(priv, MT7530_PCR_P(port), PCR_MATRIX_MASK,
>  		   priv->ports[port].pm);

[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