Re: [PATCH net-next v24 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 6 potential issue(s) to consider:
- [High] net: dsa: an8855: Unconditional sleep in FDB dump loop causes severe latency
- [High] net: dsa: an8855: Missing port matrix hardware synchronization in port_enable
- [Medium] net: dsa: an8855: Failure to enable TX LPI mode when timer is non-zero
- [Medium] net: dsa: an8855: FDB dump incorrectly reports static entries as dynamic
- [Medium] net: dsa: an8855: Incorrect jumbo frame size encoding breaks MTU configuration
- [Low] net: dsa: an8855: Duplication of standard MAC statistics in ethtool -S
--

commit 162cbb2c5557815ac251de80602fb068741ed063
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/mediatek/an8855.c b/drivers/net/dsa/mediatek/an8855.c
> index 0000000000000..b4b02b7cb6eb8 100644
> --- /dev/null
> +++ b/drivers/net/dsa/mediatek/an8855.c
> @@ -0,0 +1,1741 @@
[ ... ]
> +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: Low]
Is it necessary to include these standard statistics in the private ethtool 
strings? 

The networking core guidelines recommend avoiding duplication in ethtool -S 
for counters that already have a standard uAPI (e.g., exposed via 
rtnl_link_stats64 or get_eth_mac_stats).

[ ... ]
> +		/* 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);

[Severity: High]
Does this loop introduce too much latency? 

Since the loop processes up to 2048 records and sleeps unconditionally for 
1000-1500us on every valid bank entry while holding priv->reg_mutex, a full 
FDB dump could block all ATU/VLAN and port configurations for more than 2 
seconds. 

Can this be refactored to poll hardware readiness instead of relying on a 
hardcoded sleep?

[ ... ]
> +static int an8855_port_enable(struct dsa_switch *ds, int port,
> +			      struct phy_device *phy)
> +{
> +	struct an8855_priv *priv = ds->priv;
> +	int ret;
> +
> +	mutex_lock(&priv->reg_mutex);
> +	priv->ports[port].enable = true;
> +	ret = regmap_set_bits(priv->regmap, AN8855_PMCR_P(port),
> +			      AN8855_PMCR_TX_EN | AN8855_PMCR_RX_EN);
> +	mutex_unlock(&priv->reg_mutex);
> +
> +	return ret;
> +}

[Severity: High]
Can a port added to a bridge while administratively down become permanently 
isolated when brought up? 

When a port is disabled, mt7530_update_port_member() defers writing the 
updated pm matrix to the hardware. Here in an8855_port_enable(), the port 
is enabled but the synchronized priv->ports[port].pm matrix is not written 
to AN8855_PORTMATRIX, which leaves the port disconnected.

[ ... ]
> +static int an8855_phylink_mac_enable_tx_lpi(struct phylink_config *config,
> +					    u32 timer, bool tx_clock_stop)
> +{
[ ... ]
> +	if (timer) {
[ ... ]
> +		ret = regmap_update_bits(priv->regmap, AN8855_CKGCR,
> +					 AN8855_LPI_TXIDLE_THD_MASK, val);
> +		mutex_unlock(&priv->reg_mutex);
> +		if (ret) {
> +			dev_err(dp->ds->dev, "failed to set global LPI timer\n");
> +			return ret;
> +		}
> +	} else {
> +		ret = regmap_set_bits(priv->regmap, AN8855_PMEEECR_P(port),
> +				      AN8855_LPI_MODE_EN);
> +		if (ret) {
> +			dev_err(dp->ds->dev, "failed to enable LPI for port %d\n",
> +				port);
> +			return ret;
> +		}
> +	}
> +
> +	mutex_lock(&priv->reg_mutex);
> +	ret = regmap_set_bits(priv->regmap, AN8855_PMCR_P(port),
> +			      AN8855_PMCR_FORCE_EEE1G |
> +			      AN8855_PMCR_FORCE_EEE100);
> +	mutex_unlock(&priv->reg_mutex);

[Severity: Medium]
Will TX LPI mode fail to enable if a custom idle timer is passed? 

When timer != 0, the branch updates the global threshold in AN8855_CKGCR 
but completely skips setting the AN8855_LPI_MODE_EN bit for the port via 
AN8855_PMEEECR_P. 

Since the disable path clears this bit, failing to set it here means the 
power-saving mode won't actually be enabled.

> diff --git a/drivers/net/dsa/mediatek/an8855.h b/drivers/net/dsa/mediatek/an8855.h
> index 0000000000000..85003e5684509 100644
> --- /dev/null
> +++ b/drivers/net/dsa/mediatek/an8855.h
> @@ -0,0 +1,755 @@
[ ... ]
> +#define   AN8855_MAX_RX_JUMBO_12K	FIELD_PREP_CONST(AN8855_MAX_RX_JUMBO, 0xa)
> +#define   AN8855_MAX_RX_JUMBO_15K	FIELD_PREP_CONST(AN8855_MAX_RX_JUMBO, 0xb)
> +#define   AN8855_MAX_RX_JUMBO_16K	FIELD_PREP_CONST(AN8855_MAX_RX_JUMBO, 0xc)

[Severity: Medium]
Is it possible that jumbo frame MTU configuration will break for frames 
>= 12K? 

The shared MT7530 library computes the jumbo size encoding using a linear 
division: DIV_ROUND_UP(length, 1024). For a 15K frame, this produces 0xF. 

However, the AN8855 hardware uses a custom non-linear encoding where 15K 
is 0xB. Passing 0xF will program an invalid or undefined value to the 
hardware, causing reception of jumbo frames to fail.

> diff --git a/drivers/net/dsa/mediatek/mt7530-lib.c b/drivers/net/dsa/mediatek/mt7530-lib.c
> index 098dd45d3ee9f..4325768033325 100644
> --- a/drivers/net/dsa/mediatek/mt7530-lib.c
> +++ b/drivers/net/dsa/mediatek/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_ATRD_ENT_STATUS]) {
> +		regmap_field_read(priv->fields[MT7530_ATRD_ENT_STATUS], &val);
> +		fdb->noarp = val == STATIC_ENT;
> +	}
> +	if (priv->fields[AN8855_ATRD_ARP]) {
> +		regmap_field_read(priv->fields[AN8855_ATRD_ARP], &val);
> +		fdb->noarp = !!val;
> +	}
>  }

[Severity: Medium]
Will this incorrectly report static entries as dynamic during an FDB dump?

The read path evaluates fdb->noarp from the AN8855_ATRD_ARP field. However, 
when writing a static entry, the type is set via AN8855_ATWD_TYPE (0 for 
static) and no ARP field is manipulated. 

Because this reads an unrelated ARP field instead of the TYPE field, 
an8855_port_fdb_dump() will falsely compute and report the static flag to 
the bridge.

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