Re: [PATCH net-next v25 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 7 potential issue(s) to consider:
- [High] net: dsa: an8855: Port matrix is unconditionally zeroed out during port_enable
- [Medium] net: dsa: an8855: Unchecked regmap_read in an8855_phylink_mac_link_up causes silent register corruption
- [Medium] net: dsa: an8855: Uninitialized stack variable read in an8855_port_fdb_dump()
- [Medium] net: dsa: an8855: an8855_phylink_mac_enable_tx_lpi() leaves stale timer values on immediate LPI
- [Medium] net: dsa: an8855: missing mtu_enforcement_ingress flag allows MTU violation
- [Low] net: dsa: an8855: Driver duplicates standard UAPI statistics as private ethtool strings
- [Low] net: dsa: an8855: an8855_port_enable() leaves priv->ports[port].enable true on error
--

commit d3cae945049e5b42ed716ab1ac6797e64ffa036c
Author: Christian Marangi <ansuelsmth@gmail.com>

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

This patch adds support for the Airoha AN8855 5-Port Gigabit DSA switch,
including phylink MAC operations and standard switch functions. It supports
multiple port speeds, though 5G speed is currently disabled pending PCS
configuration support.

> diff --git a/drivers/net/dsa/mediatek/an8855.c b/drivers/net/dsa/mediatek/an8855.c
> --- /dev/null
> +++ b/drivers/net/dsa/mediatek/an8855.c
> @@ -0,0 +1,1765 @@
[ ... ]
> +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 expose these standard hardware counters through
driver-private ethtool strings? The driver already implements the standard
.get_eth_mac_stats interface. Exposing values like TxDrop and TxCrcErr
via ethtool -S duplicates standard UAPI statistics, which goes against the
Networking Drivers guidelines for standard statistics.

[ ... ]
> +static int an8855_port_fdb_dump(struct dsa_switch *ds, int port,
> +				dsa_fdb_dump_cb_t *cb, void *data)
> +{
> +	struct an8855_priv *priv = ds->priv;
> +	int banks, count = 0;
> +	u32 rsp;
> +	int ret;
> +	int i;
> +
> +	mutex_lock(&priv->reg_mutex);
> +
> +	/* Load search port */
> +	ret = regmap_write(priv->regmap, AN8855_ATWD2,
> +			   FIELD_PREP(AN8855_ATWD2_PORT, BIT(port)));
> +	if (ret)
> +		goto exit;
> +	ret = mt7530_lib_fdb_cmd(&priv->lib_priv, MT7530_FDB_START,
> +				 AN8855_FDB_MAT_MAC_PORT, &rsp);
> +	if (ret < 0)
> +		goto exit;
> +
> +	do {
> +		/* From response get the number of banks to read, exit if 0 */
> +		banks = FIELD_GET(AN8855_ATC_HIT, rsp);

[Severity: Medium]
Could this read an uninitialized stack value? If mt7530_lib_fdb_cmd()
succeeds but the final regmap_field_read(..., rsp) inside it fails, the
function returns 0 without populating rsp. This would leave rsp
uninitialized here, leading to unpredictable loop behavior when parsing FDB
entries.

[ ... ]
> +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 = 0;
> +
> +	mutex_lock(&priv->reg_mutex);
> +
> +	regmap_read(priv->regmap, AN8855_PMCR_P(port), &reg);

[Severity: Medium]
What happens if this regmap_read() fails? The return value is not checked,
so reg would remain 0. The function then sets specific bits (speed, duplex,
TX_EN, RX_EN) and writes it back, which could silently overwrite and
corrupt other configuration bits (like MAC mode or backoff settings) that
were configured in an8855_phylink_mac_config().

[ ... ]
> +static int an8855_port_enable(struct dsa_switch *ds, int port,
> +			      struct phy_device *phy)
> +{
> +	struct dsa_port *dp = dsa_to_port(ds, port);
> +	struct an8855_priv *priv = ds->priv;
> +	int ret;
> +
> +	mutex_lock(&priv->reg_mutex);
> +
> +	/* Allow the user port gets connected to the cpu port and also
> +	 * restore the port matrix if the port is the member of a certain
> +	 * bridge.
> +	 */
> +	if (dsa_port_is_user(dp)) {
> +		struct dsa_port *cpu_dp = dp->cpu_dp;
> +
> +		priv->ports[port].pm |= BIT(cpu_dp->index);
> +	}
> +	priv->ports[port].enable = true;
> +	ret = regmap_fields_write(priv->lib_priv.fields[MT7530_PCR_MATRIX],
> +				  port, 0);

[Severity: High]
Should this be passing priv->ports[port].pm instead of 0? Writing the
literal 0 disables the port's forwarding matrix in hardware, effectively
isolating the port and dropping all forwarded traffic, rather than
applying the port matrix computed just above.

[Severity: Low]
Also, if this regmap_fields_write() fails, the function jumps to the exit
label and returns an error without reverting priv->ports[port].enable back
to false. Can this desynchronize the software state from the hardware state
and cause issues in loops that check priv->ports[i].enable (such as the LPI
timer calculation)?

> +	if (ret)
> +		goto exit;
> +
> +	ret = regmap_set_bits(priv->regmap, AN8855_PMCR_P(port),
> +			      AN8855_PMCR_TX_EN | AN8855_PMCR_RX_EN);
> +
> +exit:
> +	mutex_unlock(&priv->reg_mutex);
> +
> +	return ret;
> +}
[ ... ]
> +static int an8855_setup(struct dsa_switch *ds)
> +{
[ ... ]
> +	/* Set min a max ageing value supported */
> +	ds->ageing_time_min = AN8855_L2_AGING_MS_CONSTANT;
> +	ds->ageing_time_max = FIELD_MAX(AN8855_AGE_CNT) *
> +			      FIELD_MAX(AN8855_AGE_UNIT) *
> +			      AN8855_L2_AGING_MS_CONSTANT;
> +
> +	/* User reported problem with WiFi roaming and
> +	 * ethernet port. Enabling assisted learning fix
> +	 * the issue.
> +	 */
> +	ds->assisted_learning_on_cpu_port = true;
> +
> +	return 0;
> +}

[Severity: Medium]
Does this switch require the mtu_enforcement_ingress flag to be set here?
Because mt7530_lib_port_change_mtu() configures the MTU limit globally
based on the CPU port's requested MTU, user ports configured with smaller
MTUs might accept oversized frames. Setting ds->mtu_enforcement_ingress =
true would let the DSA core normalize bridged port MTUs.

[ ... ]
> +static int an8855_phylink_mac_enable_tx_lpi(struct phylink_config *config,
> +					    u32 timer, bool tx_clock_stop)
> +{
[ ... ]
> +		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);

[Severity: Medium]
If timer == 0 is requested to enter LPI immediately, the software array
priv->tx_lpi_timer_ports[port] is not updated to 0. If the port previously
had a non-zero timer, won't this leave a stale value in the array? Because
the global LPI timer is derived from the maximum of all enabled ports, a
stale non-zero value here could artificially inflate the global threshold.

> +		if (ret) {
> +			dev_err(dp->ds->dev, "failed to enable LPI for port %d\n",
> +				port);
> +			return ret;
> +		}
> +	}
[ ... ]

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