Re: [PATCH net-next v25 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 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.

> +	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;

[ ... ]

> +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?

> +
> +	/* 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