Re: [PATCH net-next v19 10/10] net: dsa: Add Airoha AN8855 5-Port Gigabit DSA Switch driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider:
- [High] Unprotected access to `dev->phydev` and inverted logic in `an8855_phylink_mac_enable_tx_lpi`.
- [High] FDB flush operations run without the required register mutex, racing with concurrent FDB interactions.
- [High] Incorrect maths logic in `an8855_set_ageing_time` leads to register field overflow and heavily misconfigured switch aging time.
- [High] Failure to propagate the error code in `an8855_port_fdb_dump()` leads to silent dump truncations and spurious infinite looping.
- [High] Missing and incorrectly mapped `regmap_field` entries lead to uninitialized pointers, causing a kernel crash when configuring port mirroring or reading statistics.
- [High] Ignoring the return value of `regmap_read()` leads to using uninitialized stack variables.
- [High] Torn updates on the PMCR register due to bypassed regmap locking.
- [Medium] TX flow control enablement incorrectly checks `rx_pause` instead of `tx_pause`.
- [Medium] The driver duplicates standard UAPI networking statistics into driver-private `ethtool -S` strings.
- [Medium] Writing an undocumented, invalid Jumbo Frame register value for MTUs larger than 12K.
--

--- Patch [10]: [PATCH net-next v19 10/10] net: dsa: Add Airoha AN8855 5-Port Gigabit DSA Switch driver ---
commit 95a1bba0a8e59d58da6f98ffbe49372121797adc
Author: Christian Marangi <ansuelsmth@gmail.com>

net: dsa: Add Airoha AN8855 5-Port Gigabit DSA Switch driver

Add Airoha AN8855 5-Port Gigabit DSA switch. Switch can support
10M, 100M, 1Gb, 2.5G and 5G Ethernet Speed but 5G is currently error out
as it's not currently supported as requires additional configuration for
the PCS.

> diff --git a/drivers/net/dsa/an8855.c b/drivers/net/dsa/an8855.c
> new file mode 100644
> index 0000000000000..be4d78fa278fa
> --- /dev/null
> +++ b/drivers/net/dsa/an8855.c
> @@ -0,0 +1,1682 @@
[ ... ]
> +static const struct mt7530_reg_field an8855_fields[] = {
[ ... ]
> +	{ MT7530_MIB_TX_PKT_SZ_512_TO_1023, REG_FIELD_ID(AN8855_PORT_MIB_TX_PKT_SZ_512_TO_1023, 0, 32, AN8855_NUM_PORTS, 0x200) },
> +	{ AN8855_MIB_TX_PKT_SZ_1024_TO_1518, REG_FIELD_ID(AN8855_PORT_MIB_TX_PKT_SZ_1024_TO_1518, 0, 32, AN8855_NUM_PORTS, 0x200) },
> +	{ AN8855_MIB_TX_PKT_SZ_1519_TO_MAX, REG_FIELD_ID(AN8855_PORT_MIB_TX_PKT_SZ_1519_TO_MAX, 0, 32, AN8855_NUM_PORTS, 0x200) },
> +	{ MT7530_MIB_TX_BYTES_LOW, REG_FIELD_ID(AN8855_PORT_MIB_TX_BYTES_LOW, 0, 32, AN8855_NUM_PORTS, 0x200) },
> +	{ MT7530_MIB_TX_BYTES_HIGH, REG_FIELD_ID(AN8855_PORT_MIB_TX_BYTES_HIGH, 0, 32, AN8855_NUM_PORTS, 0x200) },
[ ... ]
> +	{ MT7530_MIB_RX_PKT_SZ_256_TO_511, REG_FIELD_ID(AN8855_PORT_MIB_RX_PKT_SZ_256_TO_511, 0, 32, AN8855_NUM_PORTS, 0x200) },
> +	{ MT7530_MIB_RX_PKT_SZ_512_TO_1023, REG_FIELD_ID(AN8855_PORT_MIB_RX_PKT_SZ_512_TO_1023, 0, 32, AN8855_NUM_PORTS, 0x200) },
> +	{ AN8855_MIB_TX_PKT_SZ_1024_TO_1518, REG_FIELD_ID(AN8855_PORT_MIB_RX_PKT_SZ_1024_TO_1518, 0, 32, AN8855_NUM_PORTS, 0x200) },
> +	{ AN8855_MIB_TX_PKT_SZ_1519_TO_MAX, REG_FIELD_ID(AN8855_PORT_MIB_RX_PKT_SZ_1519_TO_MAX, 0, 32, AN8855_NUM_PORTS, 0x200) },
> +	{ MT7530_MIB_RX_BYTES_LOW, REG_FIELD_ID(AN8855_PORT_MIB_RX_BYTES_LOW, 0, 32, AN8855_NUM_PORTS, 0x200) },

[Severity: High]
Are there missing or incorrectly mapped regmap_field entries here?

The array omits required mappings for MT7530_MIRROR_EN and MT7530_MIRROR_PORT.
Also, it looks like AN8855_MIB_TX_PKT_SZ_1024_TO_1518 is mapped twice, once
here to an RX register, leaving the corresponding RX slot uninitialized.

Could this result in a NULL pointer dereference when userspace queries
ethtool/RMON statistics or configures mirroring?

> +	{ MT7530_MIB_RX_BYTES_HIGH, REG_FIELD_ID(AN8855_PORT_MIB_RX_BYTES_HIGH, 0, 32, AN8855_NUM_PORTS, 0x200) },
> +	{ MT7530_MIB_RX_CTRL_DROP, REG_FIELD_ID(AN8855_PORT_MIB_RX_CTRL_DROP, 0, 32, AN8855_NUM_PORTS, 0x200) },
> +	{ MT7530_MIB_RX_INGRESS_DROP, REG_FIELD_ID(AN8855_PORT_MIB_RX_INGRESS_DROP, 0, 32, AN8855_NUM_PORTS, 0x200) },
> +	{ MT7530_MIB_RX_ARL_DROP, REG_FIELD_ID(AN8855_PORT_MIB_RX_ARL_DROP, 0, 32, AN8855_NUM_PORTS, 0x200) },
> +};
> +
> +static const struct mt7530_mib_desc an8855_mib[] = {
> +	MIB_DESC(MT7530_MIB_TX_DROP, -1, "TxDrop"),
> +	MIB_DESC(MT7530_MIB_TX_CRC_ERR, -1, "TxCrcErr"),
> +	MIB_DESC(MT7530_MIB_TX_COLLISION, -1, "TxCollision"),
> +	MIB_DESC(AN8855_MIB_TX_OVERSIZE_DROP, -1, "TxOversizeDrop"),

[Severity: Medium]
Does this array duplicate standard UAPI networking statistics into driver
private ethtool strings?

Standard practice suggests that statistics with existing standard UAPIs
(like TxDrop, TxCrcErr, etc.) should use standard interfaces instead of
being manually exposed via ethtool -S.

Could these standard counters be removed to prevent bloating the ethtool
interface?

[ ... ]
> +static void an8855_port_fast_age(struct dsa_switch *ds, int port)
> +{
> +	struct an8855_priv *priv = ds->priv;
> +	int ret;
> +
> +	/* Set to clean Dynamic entry */
> +	ret = regmap_write(priv->regmap, AN8855_ATA2, AN8855_ATA2_TYPE);
> +	if (ret) {
> +		dev_err(priv->ds->dev, "failed to update ATA2 reg: %d\n", ret);
> +		return;
> +	}
> +
> +	/* Set Port */
> +	ret = regmap_write(priv->regmap, AN8855_ATWD2,
> +			   FIELD_PREP(AN8855_ATWD2_PORT, BIT(port)));
> +	if (ret) {
> +		dev_err(priv->ds->dev, "failed to update ATWD2 reg: %d\n", ret);
> +		return;
> +	}
> +
> +	/* Flush Dynamic entry at port */
> +	ret = mt7530_lib_fdb_cmd(&priv->lib_priv, MT7530_FDB_FLUSH,
> +				 AN8855_FDB_MAT_MAC_TYPE_PORT, NULL);

[Severity: High]
Are these indirect FDB register writes vulnerable to concurrent access?

We modify shared indirect FDB registers (AN8855_ATA2 and AN8855_ATWD2) and
then trigger mt7530_lib_fdb_cmd without holding the priv->reg_mutex
synchronization lock here.

Could concurrent FDB reads or writes from other CPU threads overwrite these
indirect registers mid-flight?

> +	if (ret)
> +		dev_err(priv->ds->dev, "failed to send FDB cmd: %d\n", ret);
> +}
> +
[ ... ]
> +static int an8855_set_ageing_time(struct dsa_switch *ds, unsigned int msecs)
> +{
> +	struct an8855_priv *priv = ds->priv;
> +	u32 age_count, age_unit, val;
> +
> +	/* Convert msec in AN8855_L2_AGING_MS_CONSTANT counter */
> +	val = msecs / AN8855_L2_AGING_MS_CONSTANT;
> +	/* Derive the count unit */
> +	age_unit = val / FIELD_MAX(AN8855_AGE_UNIT);
> +	/* Get the count in unit, age_unit is always incremented by 1 internally */
> +	age_count = val / (age_unit + 1);
> +
> +	return regmap_update_bits(priv->regmap, AN8855_AAC,
> +				  AN8855_AGE_CNT | AN8855_AGE_UNIT,
> +				  FIELD_PREP(AN8855_AGE_CNT, age_count) |

[Severity: High]
Is the mathematical logic deriving age_unit and age_count correct?

By dividing val by FIELD_MAX(AN8855_AGE_UNIT), age_count can reach around
2042. However, AN8855_AGE_CNT is a 9-bit field with a maximum of 511.

Did you intend to swap the target division boundaries, dividing by
FIELD_MAX(AN8855_AGE_CNT) instead, to prevent register field overflow during
FIELD_PREP?

> +				  FIELD_PREP(AN8855_AGE_UNIT, age_unit));
> +}
> +
[ ... ]
> +static int an8855_port_fdb_dump(struct dsa_switch *ds, int port,
> +				dsa_fdb_dump_cb_t *cb, void *data)
> +{
[ ... ]
> +	do {
> +		/* From response get the number of banks to read, exit if 0 */
> +		banks = FIELD_GET(AN8855_ATC_HIT, rsp);
> +		if (!banks)
> +			break;
> +
> +		/* Each banks have 4 entry */
> +		for (i = 0; i < 4; i++) {
> +			struct mt7530_fdb _fdb = {  };
> +
> +			count++;
> +
> +			/* Check if bank is present */
> +			if (!(banks & BIT(i)))
> +				continue;
> +
> +			/* Select bank entry index */
> +			ret = regmap_write(priv->regmap, AN8855_ATRDS,
> +					   FIELD_PREP(AN8855_ATRD_SEL, i));
> +			if (ret)
> +				break;
> +			/* wait 1ms for the bank entry to be filled */
> +			usleep_range(1000, 1500);
> +			mt7530_lib_fdb_read(&priv->lib_priv, &_fdb);
> +
> +			ret = cb(_fdb.mac, _fdb.vid, _fdb.noarp, data);
> +			if (ret < 0)
> +				break;

[Severity: High]
Does this inner loop break correctly handle error propagation?

If the userspace netlink buffer fills, cb() returns a negative error code
and breaks the inner loop.

However, the outer loop fails to check this error, and calls
mt7530_lib_fdb_cmd() which overwrites ret with 0. Will this cause the
driver to spin endlessly until AN8855_NUM_FDB_RECORDS is reached and
silently return 0, truncating the FDB dump?

> +		}
> +
> +		/* Stop if reached max FDB number */
> +		if (count >= AN8855_NUM_FDB_RECORDS)
> +			break;
> +
> +		/* Read next bank */
> +		ret = mt7530_lib_fdb_cmd(&priv->lib_priv, MT7530_FDB_NEXT,
> +					 AN8855_FDB_MAT_MAC_PORT, &rsp);
> +		if (ret < 0)
> +			break;
> +	} while (true);
[ ... ]
> +static int an8855_port_change_mtu(struct dsa_switch *ds, int port,
> +				  int new_mtu)
> +{
> +	struct an8855_priv *priv = ds->priv;
> +
> +	return mt7530_lib_port_change_mtu(&priv->lib_priv, port, new_mtu);

[Severity: Medium]
Can this MT7530 library function safely handle AN8855 jumbo frame
configuration?

The mt7530_lib_port_change_mtu function calculates Jumbo codes using a
linear formula. However, for the AN8855, the jumbo mapping is non-linear
past 9K according to the header definitions (e.g., 12K is 0xa, 15K is 0xb).

If a user configures an MTU larger than 12K, could this write an
undocumented, invalid Jumbo Frame register value?

> +}
> +
[ ... ]
> +static void an8855_phylink_get_caps(struct dsa_switch *ds, int port,
> +				    struct phylink_config *config)
> +{
> +	struct an8855_priv *priv = ds->priv;
> +	u32 reg;
> +	int ret;
> +
> +	switch (port) {
[ ... ]
> +	config->mac_capabilities = MAC_ASYM_PAUSE | MAC_SYM_PAUSE |
> +				   MAC_10 | MAC_100 | MAC_1000FD | MAC_2500FD;
> +
> +	ret = regmap_read(priv->regmap, AN8855_CKGCR, &reg);
> +	if (ret)
> +		dev_err(ds->dev, "failed to read EEE LPI timer\n");
> +
> +	memcpy(config->lpi_interfaces, config->supported_interfaces,
> +	       sizeof(config->lpi_interfaces));
> +	config->lpi_capabilities = MAC_100FD | MAC_1000FD;
> +	/* Global LPI TXIDLE Threshold, default 60ms (unit 2us) */
> +	config->lpi_timer_default = FIELD_GET(AN8855_LPI_TXIDLE_THD_MASK, reg) *

[Severity: High]
Is the uninitialized stack variable reg used if the regmap_read above fails?

The code logs an error but does not return early. Could this result in
configuring the LPI timers with uninitialized stack garbage?

> +				    AN8855_TX_LPI_UNIT;
> +
> +	config->eee_enabled_default = true;
> +}
> +
[ ... ]
> +static void an8855_phylink_mac_link_up(struct phylink_config *config,
> +				       struct phy_device *phydev, unsigned int mode,
> +				       phy_interface_t interface, int speed,
> +				       int duplex, bool tx_pause, bool rx_pause)
> +{
> +	struct dsa_port *dp = dsa_phylink_to_port(config);
> +	struct an8855_priv *priv = dp->ds->priv;
> +	int port = dp->index;
> +	u32 reg;
> +
> +	regmap_read(priv->regmap, AN8855_PMCR_P(port), &reg);

[Severity: High]
What happens if this regmap_read fails?

The return value is completely ignored. If it fails, the uninitialized reg
variable will be bitwise-modified and written directly back into the MAC
control register.

> +	if (phylink_autoneg_inband(mode)) {
> +		reg &= ~AN8855_PMCR_FORCE_MODE;
> +	} else {
> +		reg |= AN8855_PMCR_FORCE_MODE | AN8855_PMCR_FORCE_LNK;
> +
> +		reg &= ~AN8855_PMCR_FORCE_SPEED;
> +		switch (speed) {
[ ... ]
> +		reg &= ~AN8855_PMCR_FORCE_FDX;
> +		if (duplex == DUPLEX_FULL)
> +			reg |= AN8855_PMCR_FORCE_FDX;
> +
> +		reg &= ~AN8855_PMCR_RX_FC_EN;
> +		if (rx_pause || dsa_port_is_cpu(dp))
> +			reg |= AN8855_PMCR_RX_FC_EN;
> +
> +		reg &= ~AN8855_PMCR_TX_FC_EN;
> +		if (rx_pause || dsa_port_is_cpu(dp))

[Severity: Medium]
Should this condition check tx_pause instead of rx_pause?

When enabling AN8855_PMCR_TX_FC_EN, the code ignores the tx_pause argument
provided by phylink, which might break TX flow control in asymmetric pause
setups.

> +			reg |= AN8855_PMCR_TX_FC_EN;
> +	}
> +
> +	reg |= AN8855_PMCR_TX_EN | AN8855_PMCR_RX_EN;
> +
> +	regmap_write(priv->regmap, AN8855_PMCR_P(port), reg);

[Severity: High]
Does this read-modify-write sequence bypass regmap locking?

Because the atomic regmap lock is dropped between the regmap_read at the
start of the function and this regmap_write, could concurrent modifications
(like an8855_phylink_mac_enable_tx_lpi using regmap_set_bits) be
irreversibly overwritten?

> +}
> +
[ ... ]
> +static int an8855_phylink_mac_enable_tx_lpi(struct phylink_config *config,
> +					    u32 timer, bool tx_clock_stop)
> +{
> +	struct dsa_port *dp, *other_dp;
> +	struct an8855_priv *priv;
> +	struct dsa_switch *ds;
> +	int port;
> +	u32 val;
> +	int ret;
> +
> +	dp = dsa_phylink_to_port(config);
> +	port = dp->index;
> +	ds = dp->ds;
> +	priv = ds->priv;
> +
> +	/* TX LPI timer is global, find the highest timer
> +	 * across all port.
> +	 * If requested timer is 0, set to enter LPI immediately
> +	 * for the single port.
> +	 */
> +	if (timer) {
> +		dsa_switch_for_each_user_port(other_dp, ds) {
> +			struct phy_device *phydev;
> +			struct net_device *dev;
> +
> +			if (other_dp == dp)
> +				continue;
> +
> +			dev = other_dp->user;
> +			phydev = dev->phydev;
> +
> +			if (timer > phydev->eee_cfg.tx_lpi_timer)

[Severity: High]
Can this lead to a NULL pointer dereference?

The code unconditionally dereferences dev and phydev without checking if
the port is bound to a network device or if a PHY is attached (e.g.,
fixed-link or disabled ports).

Additionally, does the condition timer > phydev->eee_cfg.tx_lpi_timer use
inverted logic? It replaces timer when it is strictly greater, finding the
lowest timer across ports instead of the highest as documented.

> +				timer = phydev->eee_cfg.tx_lpi_timer;
> +		}
> +
> +		timer /= AN8855_TX_LPI_UNIT;
> +		if (FIELD_FIT(AN8855_LPI_TXIDLE_THD_MASK, timer))
[ ... ]

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