Re: [PATCH net-next v24 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 3 potential issue(s) to consider:
- [High] net: dsa: mt7530: spurious command/trigger execution via sequential RMW on command registers
- [High] net: dsa: mt7530: stale hardware state leaks into VLAN table entries
- [High] net: dsa: mt7530: potential data loss and severe MDIO overhead in FDB writes
--

commit 01f4c0abd9feaaa7a4bc2c096b85e94c4360593d
Author: Christian Marangi <ansuelsmth@gmail.com>
net: dsa: mt7530: generalize and move common function to lib module

This commit moves common MT7530 switch functions into a new library module 
and converts them to use the regmap field API. The refactoring aims to 
prepare for Airoha switch support while maintaining existing logic.

> diff --git a/drivers/net/dsa/mediatek/mt7530-lib.c b/drivers/net/dsa/mediatek/mt7530-lib.c
> new file mode 100644
> index 0000000000000..098dd45d3ee9f
> --- /dev/null
> +++ b/drivers/net/dsa/mediatek/mt7530-lib.c
> @@ -0,0 +1,1128 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include <linux/device.h>
> +#include <linux/if_bridge.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +#include <net/dsa.h>
> +
> +#include "mt7530-lib.h"
> +
> +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], 1);
> +	regmap_field_write(priv->fields[MT7530_CCR_RX_OCT_CNT_BAD], 1);
> +	regmap_field_write(priv->fields[MT7530_CCR_TX_OCT_CNT_GOOD], 1);
> +	regmap_field_write(priv->fields[MT7530_CCR_TX_OCT_CNT_BAD], 1);
> +	regmap_field_write(priv->fields[MT7530_CCR_MIB_ENABLE], 1);
> +}

[Severity: High]
Could this sequence trigger multiple spurious flushes? Since 
regmap_field_write() performs a read-modify-write (RMW), each sequential 
write to this register might repeatedly write back flush trigger bits before 
the operation completes. The original implementation avoided this by writing 
the entire register atomically with a single regmap_write().

[ ... ]

> +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 these sequential RMW operations on the Address Table Command register 
lead to premature or spurious FDB command executions? 

If the BUSY bit happens to be already set (e.g., from a previous command 
timeout, which the code polls for and can occur), the very first 
regmap_field_write() might read BUSY=1, modify the MAT field, and write it 
back with BUSY=1 still set. This would trigger the hardware immediately 
with incomplete command fields. Previously, this was handled safely by 
constructing the payload and BUSY bit in a local variable and issuing a 
single atomic regmap_write().

[ ... ]

> +static void mt7530_lib_fdb_write(struct mt7530_lib_priv *priv, u16 vid,
> +				 u8 port_mask, const u8 *mac,
> +				 u8 aging, u8 type)
> +{
> +	/* Clear ATWD registers from previous access */
> +	regmap_field_write(priv->fields[__MT7530_ATWD1], 0);
> +	regmap_field_write(priv->fields[__MT7530_ATWD2], 0);
> +	regmap_field_write(priv->fields[__MT7530_ATWD3], 0);
> +
> +	regmap_field_write(priv->fields[MT7530_ATWD_CVID], vid);
> +	regmap_field_write(priv->fields[MT7530_ATWD_IVL], 1);
> +	regmap_field_write(priv->fields[MT7530_ATWD_FID], FID_BRIDGED);
> +	regmap_field_write(priv->fields[MT7530_ATWD_AGE_TIMER], aging);
> +	regmap_field_write(priv->fields[MT7530_ATWD_PORT_MAP], port_mask);
> +	/* STATIC_ENT indicate that entry is static wouldn't
> +	 * be aged out and STATIC_EMP specified as erasing an
> +	 * entry
> +	 */
> +	regmap_field_write(priv->fields[MT7530_ATWD_ENT_STATUS], type);
> +	regmap_field_write(priv->fields[MT7530_ATWD_MAC_BYTE_5], mac[5]);
> +	regmap_field_write(priv->fields[MT7530_ATWD_MAC_BYTE_4], mac[4]);
> +	regmap_field_write(priv->fields[MT7530_ATWD_MAC_BYTE_3], mac[3]);
> +	regmap_field_write(priv->fields[MT7530_ATWD_MAC_BYTE_2], mac[2]);
> +	regmap_field_write(priv->fields[MT7530_ATWD_MAC_BYTE_1], mac[1]);
> +	regmap_field_write(priv->fields[MT7530_ATWD_MAC_BYTE_0], mac[0]);
> +}

[Severity: High]
Will these sequential updates to the FDB payload registers cause data loss? 

These are write-only command payload buffers. Reading them (which 
regmap_field_write() does internally during the read-modify-write cycle) 
typically returns 0. If it returns 0, the RMW operation would overwrite and 
destroy the fields set by the preceding calls within the same register.

Additionally, since caching is disabled in the regmap configuration, this 
sequence generates 24 individual MDIO transactions per FDB update. Could this 
create severe performance overhead in the FDB hot path compared to the 
original bulk regmap_write()?

[ ... ]

> +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);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_field_write(priv->fields[MT7530_VTCR_BUSY], 1);

[Severity: High]
Similar to mt7530_lib_fdb_cmd(), could these sequential RMWs on the VLAN Table 
Command register risk spurious VLAN command executions if the BUSY bit is 
already 1 when the first field is updated?

[ ... ]

> +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);
> +	regmap_field_write(priv->fields[MT7530_VAWD_VTAG_EN], 1);
> +	regmap_field_write(priv->fields[MT7530_VAWD_PORT_MEM],
> +			   new_members);
> +	regmap_field_write(priv->fields[MT7530_VAWD_FID],
> +			   FID_BRIDGED);
> +	regmap_field_write(priv->fields[MT7530_VAWD_VLAN_VALID], 1);

[Severity: High]
Does this sequence leak uninitialized or stale bits into the VLAN table entry? 

This register (MT7530_VAWD1) is populated by the hardware after executing a 
read command (MT7530_VTCR_RD_VID). For newly initialized VLANs, the hardware 
might return garbage or leftover state.

Because the regmap_field_write() RMW sequence modifies only specific bits 
without clearing the entire register first, these unspecified bits are 
preserved and written back. The original code used a bulk regmap_write() 
to deterministically zero out unspecified fields.

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