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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] net: dsa: NULL deref in mt7530_lib_fdb_read() due to mismatched field check
- [Medium] net: dsa: ethtool -S stat duplication for standard uAPI counters
- [Medium] net: dsa: stale tx_lpi_timer_ports values corrupt global LPI timer calculation
--

commit 6c2fcaf5228ec0a75518542784a8b035277e4900
Author: Christian Marangi <ansuelsmth@gmail.com>
Subject: net: dsa: Add Airoha AN8855 5-Port Gigabit DSA Switch driver

This introduces the driver for the Airoha AN8855 5-Port Gigabit DSA switch.
It supports multiple Ethernet speeds up to 2.5G and includes necessary
adjustments to the mt7530-lib to support Airoha structures.

> diff --git a/drivers/net/dsa/an8855.c b/drivers/net/dsa/an8855.c
> index 0000000000000..428d7dd85dd80
> --- /dev/null
> +++ b/drivers/net/dsa/an8855.c
[ ... ]
> +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"),

[Severity: Medium]
Are these standard networking statistics meant to be duplicated as
driver-private ethtool -S strings here? Standard counters already have a
defined uAPI, and duplicating them in private strings exposes redundant
data to userspace tools.

[ ... ]
> +static void an8855_get_strings(struct dsa_switch *ds, int port,
> +			       u32 stringset, uint8_t *data)
> +{
> +	int i;
> +
> +	if (stringset != ETH_SS_STATS)
> +		return;
> +
> +	for (i = 0; i < ARRAY_SIZE(an8855_mib); i++)
> +		ethtool_puts(&data, an8855_mib[i].name);
> +}

[Severity: Medium]
Since the statistics mapped in an8855_mib[] overlap with standard uAPI
counters, doesn't exposing them here violate kernel networking guidelines
for ethtool -S statistics?

[ ... ]
> +static void an8855_phylink_mac_disable_tx_lpi(struct phylink_config *config)
> +{
> +	struct dsa_port *dp = dsa_phylink_to_port(config);
> +	struct an8855_priv *priv = dp->ds->priv;
> +	int port = dp->index;
> +	int ret;
> +
> +	mutex_lock(&priv->reg_mutex);
> +	ret = regmap_clear_bits(priv->regmap, AN8855_PMCR_P(port),
> +				AN8855_PMCR_FORCE_EEE1G |
> +				AN8855_PMCR_FORCE_EEE100);
> +	mutex_unlock(&priv->reg_mutex);

[Severity: Medium]
When EEE is disabled on a port here, shouldn't we also clear the software
state for the timer (priv->tx_lpi_timer_ports[port] = 0)? 

[ ... ]
> +static int an8855_phylink_mac_enable_tx_lpi(struct phylink_config *config,
> +					    u32 timer, bool tx_clock_stop)
> +{
[ ... ]
> +	if (timer) {
> +		int i;
> +
> +		/* Save requested timer and search the highest one */
> +		mutex_lock(&priv->reg_mutex);
> +		priv->tx_lpi_timer_ports[dp->index] = timer;
> +		for (i = 0; i < AN8855_NUM_PORTS; i++) {
> +			if (!priv->ports[i].enable)
> +				continue;
> +
> +			if (i == dp->index)
> +				continue;
> +
> +			if (timer < priv->tx_lpi_timer_ports[i])
> +				timer = priv->tx_lpi_timer_ports[i];
> +		}

[Severity: Medium]
Because an8855_phylink_mac_disable_tx_lpi() doesn't clear the port's software
timer state, couldn't this loop evaluate stale timer values from ports where
EEE was previously disabled? Could this corrupt the global LPI timer
calculation?

> diff --git a/drivers/net/dsa/mt7530-lib.c b/drivers/net/dsa/mt7530-lib.c
> index 098dd45d3ee9f..cf474faf8ed6a 100644
> --- a/drivers/net/dsa/mt7530-lib.c
> +++ b/drivers/net/dsa/mt7530-lib.c
[ ... ]
> @@ -483,8 +497,14 @@ void mt7530_lib_fdb_read(struct mt7530_lib_priv *priv, struct mt7530_fdb *fdb)
>  	fdb->mac[1] = val;
>  	regmap_field_read(priv->fields[MT7530_ATRD_MAC_BYTE_0], &val);
>  	fdb->mac[0] = val;
> -	regmap_field_read(priv->fields[MT7530_ATRD_ENT_STATUS], &val);
> -	fdb->noarp = val == STATIC_ENT;
> +	if (priv->fields[MT7530_ATWD_ENT_STATUS]) {
> +		regmap_field_read(priv->fields[MT7530_ATRD_ENT_STATUS], &val);

[Severity: High]
Can this cause a NULL pointer dereference? The condition checks if the ATWD 
(write) field exists, but then unconditionally dereferences the ATRD (read) 
field. If a driver populates the ATWD field but omits the ATRD field, 
wouldn't regmap_field_read() crash?

> +		fdb->noarp = val == STATIC_ENT;
> +	}
> +	if (priv->fields[AN8855_ATRD_ARP]) {
> +		regmap_field_read(priv->fields[AN8855_ATRD_ARP], &val);
> +		fdb->noarp = !!val;
> +	}
>  }
>  EXPORT_SYMBOL_GPL(mt7530_lib_fdb_read);

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