Re: [PATCH net-next v23 8/9] net: dsa: mt7530: generalize and move common function to lib module

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 3 · Low: 3 (4)

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] A name collision between a macro and an enum constant puts the
  mirror-enable regmap field at the wrong index.
- [Medium] mt7530_read_port_stats64() no longer fills
  rtnl_link_stats64.multicast.
- [Medium] mt753x_table[ID_EN7528] now uses `.reg_fields =
  mt7531_fields`, so on EN7528 mirroring goes to MT7531_CFC (MIRROR_EN…
- [Medium] mt7530_lib_port_mdb_add(), mt7530_lib_port_mdb_del() and
  mt7530_hw_vlan_del() declare `u32 val;` without initializing it.
- [Low] mt7530_lib_fdb_cmd() and mt7530_vlan_cmd() ignore failures of
  their final status reads.
- [Low] Two comments were not updated to match the new code.
- [Low] Splitting whole-register accesses into regmap_field accesses
  multiplies register traffic for FDB and VLAN operations.

Pre-existing issues:
- [Low] This problem wasn't introduced by this patch, but
  mt7530_hw_vlan_update() ignores the results of the RD_VID command, the…
- [Low] This problem wasn't introduced by this patch, but
  mt7530_lib_port_mdb_add()/mt7530_lib_port_mdb_del() handle any failure…
- [Low] This problem wasn't introduced by this patch, but
  mt7530_lib_fdb_write() returns void and ignores every register write.
- [Low] This problem wasn't introduced by this patch, but
  mt7530_port_fdb_dump() returns 0 unconditionally.

--

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

This commit adds a mt7530 library module to prepare for Airoha DSA switch
support, and moves the common MT7530 functions into it under a mt7530_lib
prefix. The functions now use the regmap reg_field API. There is one
common field table, plus per-chip mirror fields taken from the match table.
The commit message says no behaviour change is expected.

> diff --git a/drivers/net/dsa/mt7530-lib.c b/drivers/net/dsa/mt7530-lib.c
> new file mode 100644
> index 0000000000000..098dd45d3ee9f
> --- /dev/null
> +++ b/drivers/net/dsa/mt7530-lib.c

[ ... ]

> +int mt7530_lib_fdb_cmd(struct mt7530_lib_priv *priv, enum mt7530_fdb_cmd cmd,
> +		       u32 mat, u32 *rsp)
> +{

[ ... ]

> +	ret = regmap_field_read_poll_timeout(priv->fields[MT7530_ATC_BUSY],
> +					     val, !val, 20, 20000);
> +	if (ret < 0) {
> +		dev_err(priv->dev, "reset timeout\n");
> +		return ret;
> +	}
> +
> +	regmap_field_read(priv->fields[MT7530_ATC_INVALID], &val);
> +	if (cmd == MT7530_FDB_READ && val)
> +		return -EINVAL;
> +
> +	if (rsp)
> +		regmap_field_read(priv->fields[__MT7530_ATC], rsp);
> +
> +	return 0;
> +}

[Severity: Low]
The old mt7530_fdb_cmd() returned an error if the final regmap_read() of
MT7530_ATC failed. Both regmap_field_read() calls here ignore the return
value.

After a successful busy poll val is 0, and a failed read leaves it at 0.
That means an invalid entry would be reported as success.

A failed read of rsp leaves the previous command's ATC_SRCH_HIT and
ATC_SRCH_END bits in place. Could mt7530_port_fdb_dump() then process stale
search state and report duplicate entries (bounded by cnt)?

mt7530_vlan_cmd() does the same thing with MT7530_VTCR_INVALID. The last
patch in the series wraps these reads in NULL checks, but it still doesn't
check their return values.

Should these reads check their return values as the old code did?

> +
> +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_MAC_BYTE_0], mac[0]);
> +}

[Severity: Low]
This isn't a bug introduced by this patch, but mt7530_lib_fdb_write()
returns void and ignores every register write, just like the old
mt7530_fdb_write().

mt7530_lib_port_fdb_add() and mt7530_lib_port_fdb_del() then issue
MT7530_FDB_WRITE even if writing the ATA/ATWD staging registers failed.

Could that write or delete an entry keyed on a partly programmed MAC/VID?

> +
> +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_ENT_STATUS], &val);
> +	fdb->noarp = val == STATIC_ENT;
> +}

[Severity: Low]
This isn't a bug, but splitting whole-register accesses into regmap fields
adds a lot of register traffic on the MDIO path, where the regmap has no
cache. Each field write is a read plus a write. Each 32-bit access over
MDIO is a page-select write plus two 16-bit transfers.

The register access counts before and after this patch:

- mt7530_lib_fdb_read(): 10 register reads instead of 3.
- mt7530_lib_fdb_write(): 3 clears plus 12 field read-modify-writes,
  instead of 3 writes.
- mt7530_lib_fdb_cmd(): 3 read-modify-writes, the poll and 2 more reads,
  instead of 1 write, the poll and 1 read.

mt7530_port_fdb_dump() repeats FDB_NEXT plus mt7530_lib_fdb_read() for up
to MT7530_NUM_FDB_RECORDS entries. It does this while holding reg_mutex and
the MDIO bus lock, with about 3x the old bus traffic.

Would it make sense to keep whole-register accesses for the ATA, ATWD,
ATRD and ATC registers?

[ ... ]

> +int mt7530_lib_port_mdb_add(struct mt7530_lib_priv *priv, int port,
> +			    const struct switchdev_obj_port_mdb *mdb,
> +			    struct dsa_db db)
> +{
> +	const u8 *addr = mdb->addr;
> +	u16 vid = mdb->vid;
> +	u8 port_mask = 0;
> +	u32 val;
> +	int ret;
> +
> +	mutex_lock(priv->reg_mutex);
> +
> +	mt7530_lib_fdb_write(priv, vid, 0, addr, 0, STATIC_EMP);
> +	if (!mt7530_lib_fdb_cmd(priv, MT7530_FDB_READ,
> +				ATC_MAT_MACTAB, NULL)) {
> +		regmap_field_read(priv->fields[MT7530_ATRD_PORT_MAP],
> +				  &val);
> +		port_mask = val;
> +	}

[Severity: Medium]
Can val be used uninitialized here? When regmap_read() fails,
regmap_field_read() returns early without writing *val.

The old code called regmap_read() directly on the caller's variable.
mt7530_regmap_read() in mt7530-mdio.c sets *val = 0 first, with the comment
"Callers do not check for errors, keep the value deterministic".
regmap_field_read() reads into its own local reg_val, so that zero no
longer reaches this caller.

On an MDIO read failure, stack garbage would become port_mask and be
written as a STATIC_ENT multicast port map.

mt7530_lib_port_mdb_del() has the same pattern. mt7530_hw_vlan_del() tests
an uninitialized val:

    regmap_field_read(priv->fields[MT7530_VAWD_VLAN_VALID], &val);
    if (!val) {

Should these val variables start at 0, or should the return values be
checked?

> +
> +	port_mask |= BIT(port);
> +	mt7530_lib_fdb_write(priv, vid, port_mask, addr, -1, STATIC_ENT);
> +	ret = mt7530_lib_fdb_cmd(priv, MT7530_FDB_WRITE,
> +				 ATC_MAT_MACTAB, NULL);

[Severity: Low]
This is a pre-existing issue, but any failure of MT7530_FDB_READ (I/O
error or timeout) is handled the same way as a missing entry. In that case
mt7530_lib_port_mdb_add() writes a mask containing only BIT(port), and
mt7530_lib_port_mdb_del() writes STATIC_EMP with a zero mask.

After a failed lookup, can this drop the multicast membership of the other
ports?

[ ... ]

> +static void mt7530_hw_vlan_update(struct mt7530_lib_priv *priv, u16 vid,
> +				  struct mt7530_hw_vlan_entry *entry,
> +				  mt7530_vlan_op vlan_op)
> +{
> +	u32 val = 0;
> +
> +	/* Fetch entry */
> +	mt7530_vlan_cmd(priv, MT7530_VTCR_RD_VID, vid);
> +
> +	regmap_field_read(priv->fields[MT7530_VAWD_PORT_MEM], &val);
> +	entry->old_members = val;
> +
> +	/* Manipulate entry */
> +	vlan_op(priv, entry);
> +
> +	/* Flush result to hardware */
> +	mt7530_vlan_cmd(priv, MT7530_VTCR_WR_VID, vid);
> +}

[Severity: Low]
This isn't a bug introduced by this patch, but mt7530_hw_vlan_update()
ignores the results of the RD_VID command, the PORT_MEM read and the final
WR_VID command. The old code did the same.

vlan_op() can run on stale VAWD data. mt7530_lib_port_vlan_add() and
mt7530_lib_port_vlan_del() then update the PVID state and return 0, even if
the VLAN table write failed.

Should the errors be passed back to the callers?

[ ... ]

> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 50e80585d5585..83cfbd8ee081f 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -24,10 +24,163 @@
>  #include <net/pkt_cls.h>
>  
>  #include "mt7530.h"
> +#include "mt7530-lib.h"

[ ... ]

> +static const struct mt7530_reg_field mt7530_fields[] = {
> +	{ MT7530_MIRROR_EN, REG_FIELD(MT753X_MFC, 3, 3) },
> +	{ MT7530_MIRROR_PORT, REG_FIELD(MT753X_MFC, 0, 2) },
> +};
> +
> +static const struct mt7530_reg_field mt7531_fields[] = {
> +	{ MT7530_MIRROR_EN, REG_FIELD(MT7531_CFC, 19, 19) },
> +	{ MT7530_MIRROR_PORT, REG_FIELD(MT7531_CFC, 16, 18) },
> +};

[Severity: High]
Does MT7530_MIRROR_EN expand to the enum index here? mt7530.h includes
mt7530-lib.h, which declares:

    enum mt7530_regmap_field {
    	MT7530_MIRROR_EN,
    	...

After that include, mt7530.h still keeps the old register bit define:

    #define  MT7530_MIRROR_EN		BIT(3)

So in mt7530.c both tables get id = BIT(3) = 8, which is
MT7530_CCR_TX_OCT_CNT_GOOD in the enum. mt7530-lib.c only includes
mt7530-lib.h, so in the library MT7530_MIRROR_EN is 0.

mt7530_setup_lib_priv() fills in mt753x_fields first and the per-chip
table second. That overwrites fields[8] with the mirror enable field and
leaves fields[0] NULL, since priv comes from devm_kzalloc().

A tc matchall mirred rule then reaches:

mt753x_port_mirror_add()
  mt7530_lib_port_mirror_add()
    regmap_field_read(priv->fields[MT7530_MIRROR_EN], &val)

with a NULL field, and regmap_field_read() dereferences field->regmap.
mt7530_lib_port_mirror_del() also writes through the same NULL field.

mt7530_lib_mib_reset() is called from mt7530_setup() and
mt7531_setup_common(), and does:

    regmap_field_write(priv->fields[MT7530_CCR_TX_OCT_CNT_GOOD], 1);

This would now set the global mirror enable bit (MFC bit 3 or CFC bit 19)
on every setup, and MIB CCR bit 5 would never be set. The old code wrote
CCR_MIB_ACTIVATE, which included TX_OCT_CNT_GOOD.

mt7530.h and mt7530.c aren't changed later in the series, so this seems to
still be present at the end of it. Should the old MT7530_MIRROR_EN define
be removed or renamed?

[ ... ]

> @@ -35,15 +188,15 @@ static struct mt753x_pcs *pcs_to_mt753x_pcs(struct phylink_pcs *pcs)
>  
>  /* String, offset, and register size in bytes if different from 4 bytes */
>  static const struct mt7530_mib_desc mt7530_mib[] = {

[ ... ]

> +	MIB_DESC(MT7530_MIB_TX_DROP, -1, "TxDrop"),

[Severity: Low]
This isn't a bug, but this comment no longer matches the descriptor. The
descriptor now holds regmap field IDs (field_low, field_high, with -1
meaning no high word), not an offset and a size.

The same goes for this comment in mt7530.h:

    /* Each define is an offset of MT7530_PORT_MIB_COUNTER */

Each MT7530_PORT_MIB_* define now includes the MT7530_MIB_COUNTER base and
is used as an absolute port 0 address.

[ ... ]

> @@ -861,38 +833,40 @@ static void mt7530_read_port_stats64(struct mt7530_priv *priv, int port,
>  	 * To simulate a global frame counter, read Unicast and addition Multicast
>  	 * and Broadcast later
>  	 */
> -	mt7530_read_port_stats(priv, port, MT7530_PORT_MIB_RX_UNICAST, 1,
> -			       &storage->rx_packets);
> -	mt7530_read_port_stats(priv, port, MT7530_PORT_MIB_RX_MULTICAST, 1,
> -			       &storage->multicast);
> -	storage->rx_packets += storage->multicast;
> -	mt7530_read_port_stats(priv, port, MT7530_PORT_MIB_RX_BROADCAST, 1,
> -			       &data);
> +	mt7530_lib_read_port_stats(lib_priv->fields[MT7530_MIB_RX_UNICAST],
> +				   NULL, port, &storage->rx_packets);
> +	mt7530_lib_read_port_stats(lib_priv->fields[MT7530_MIB_RX_MULTICAST],
> +				   NULL, port, &data);
> +	storage->rx_packets += data;

[Severity: Medium]
The old code read the RX multicast counter into storage->multicast. The new
code reads it into the scratch variable data and only adds it to
rx_packets. Nothing else in mt7530*.c writes the multicast field now.

mt7530_stats_refresh() starts from a zeroed struct rtnl_link_stats64.
Won't ndo_get_stats64 then always report multicast as 0 on every user port?
The commit message says no behaviour change is expected.

[ ... ]

> @@ -1785,7 +1250,8 @@ mt7530_port_fdb_dump(struct dsa_switch *ds, int port,
>  		}
>  	} while (--cnt &&
>  		 !(rsp & ATC_SRCH_END) &&
> -		 !mt7530_fdb_cmd(priv, MT7530_FDB_NEXT, &rsp));
> +		 !mt7530_lib_fdb_cmd(&priv->lib_priv, MT7530_FDB_NEXT,
> +				     ATC_MAT_MACTAB, &rsp));
>  err:
>  	mutex_unlock(&priv->reg_mutex);
>  

[Severity: Low]
This is a pre-existing issue, but mt7530_port_fdb_dump() still returns 0
unconditionally after this label. A failed FDB_START jumps to err, and a
failed FDB_NEXT ends the loop without keeping its error.

Should the error be returned to dsa_port_fdb_dump(), so an incomplete dump
isn't reported as success?

[ ... ]

> @@ -3509,6 +2738,8 @@ const struct mt753x_info mt753x_table[] = {
>  	},
>  	[ID_AN7583] = {
>  		.id = ID_AN7583,
> +		.reg_fields = mt7531_fields,
> +		.num_reg_fields = ARRAY_SIZE(mt7531_fields),
>  		.pcs_ops = &mt7530_pcs_ops,

[ ... ]

> @@ -3519,6 +2750,8 @@ const struct mt753x_info mt753x_table[] = {
>  	},
>  	[ID_EN7528] = {
>  		.id = ID_EN7528,
> +		.reg_fields = mt7531_fields,
> +		.num_reg_fields = ARRAY_SIZE(mt7531_fields),
>  		.pcs_ops = &mt7530_pcs_ops,

[Severity: Medium]
Is it intended that EN7528 uses mt7531_fields? The MT753X_MIRROR_REG()
macro still in mt7530.h maps only MT7531, MT7988, EN7581 and AN7583 to
MT7531_CFC. Before this patch, EN7528 mirroring used MT753X_MFC with
enable BIT(3) and port GENMASK(2, 0).

Other code that this patch doesn't touch also treats EN7528 as MFC-based.
mt753x_conduit_state_change() groups it with MT7530 and MT7621, and
mt753x_cpu_port_enable() leaves it out of the MT7531_CFC CPU_PMAP chips.

On EN7528, mt7530_lib_port_mirror_add() would now do a read-modify-write
on bits 16..19 of register 0x4 instead of bits 0..3 of MFC. It would still
return success.

AN7583 changes as well. MT753X_MIRROR_EN() did not list ID_AN7583, so its
enable bit was BIT(3) in CFC, which overlaps CPU_PMAP. It is now BIT(19).
If that was an old bug being fixed, could the commit message say so?

The MT753X_MIRROR_* macros are now unused, and they contradict the new
tables for both chips.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260925182435.21207-1-ansuelsmth%40gmail.com