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

[Christian Marangi <ansuelsmth@gmail.com>]

On Sat, Oct 10, 2026 at 04:11:43PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] net: severe MDIO bus performance regression via sequential regmap_field accesses
> - [Medium] net: dsa: mt7530: stale hardware bits preserved during VLAN setup due to RMW
> --
> 
> commit 9f9905cb3bdc569b54ef5ded23eb827e6ba6ffc2
> Author: Christian Marangi <ansuelsmth@gmail.com>
> 
> net: dsa: mt7530: generalize and move common function to lib module
> 
> This commit creates a mt7530 library module to share common functions in
> preparation for Airoha switch support. The functions are refactored to use
> the reg_field regmap API for register access while aiming to preserve the
> same logic. Minimum modifications are made to header files to permit reuse
> of common macros and enums.
> 
> > 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
> 
> [ ... ]
> 
> > +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);
> 
> [Severity: High]
> Does this sequence of regmap_field_write() calls cause a severe performance
> regression on the MDIO bus?
> 
> Since the MDIO bus is inherently slow, taking around 20 to 30 microseconds
> per transaction, replacing a bulk 32-bit regmap_write() with individual
> regmap_field_write() operations expands a single register update into
> multiple Read-Modify-Write cycles. 
> 
> It looks like this single FDB entry write will now execute up to 15 RMW
> operations instead of a single bulk update.
>

I think it's acceptable for the good generalization done here and also it's
not exactly an hot path updating the FDB entry.
 
> > +	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);
> 
> [ ... ]
> 
> > +void mt7530_lib_fdb_read(struct mt7530_lib_priv *priv, struct mt7530_fdb *fdb)
> > +{
> > +	u32 val = 0;
> > +
> > +	regmap_field_read(priv->fields[MT7530_ATRD_CVID], &val);
> > +	fdb->vid = val;
> > +	regmap_field_read(priv->fields[MT7530_ATRD_AGE_TIMER], &val);
> > +	fdb->aging = val;
> 
> [Severity: High]
> Similarly, does this sequence expand a single 3-register access into 10
> separate MDIO read transactions?
> 
> If a full FDB dump of 2048 entries is requested, or continuous stat polling
> occurs, this could multiply the bus traffic and cause severe latency and
> MDIO bus congestion.
> 
> > +	regmap_field_read(priv->fields[MT7530_ATRD_PORT_MAP], &val);
> > +	fdb->port_mask = val;
> > +	regmap_field_read(priv->fields[MT7530_ATRD_MAC_BYTE_5], &val);
> > +	fdb->mac[5] = val;
> 
> [ ... ]
> 

Same reason, it's acceptable.

> > +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: Medium]
> Can this sequence of regmap_field_write() calls preserve stale hardware bits
> and leak them into new VLAN entries?
> 
> By using individual regmap_field_write() operations, this performs
> Read-Modify-Write cycles on the previously populated hardware register via
> RD_VID. The old implementation used a full 32-bit regmap_write() which
> safely cleared unspecified bits.
> 
> It appears that any leftover hardware bits in MT7530_VAWD1, such as EG_CON
> or PORT_STAG from prior usages or bootloader states, will be preserved.
> 
> Could this lead to corrupted egress tagging behaviors or STAG attributes on
> standard VLANs?
> 

From what I can see the vlan entry gets read first for existance. This zero
all these values addressing this concern.

> > +
> > +	/* Decide whether adding tag or not for those outgoing packets from the
> > +	 * port inside the VLAN.
> > +	 * CPU port is always taken as a tagged port for serving more than one
> 
> [ ... ]
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20261009161102.38961-1-ansuelsmth@gmail.com?part=9

-- 
	Ansuel