Re: [PATCH net-next v24 10/10] net: dsa: Add Airoha AN8855 5-Port Gigabit DSA Switch driver
From: netdev-bot+sashiko
Date: Thu Oct 01 2026 - 00:47:03 EST
Thank you for your contribution! Sashiko AI review found 12 potential
issue(s) to consider.
Critical: 0 · High: 2 · Medium: 7 · Low: 3
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] In the AN8855 driver, the shared VLAN read-modify-write path
reads the VLAN write-data register (AN8855_VAWD0) back, not the read-…
- [High] an8855_port_enable() sets ports[port].enable and turns on PMCR
TX/RX, but it never writes the cached forwarding matrix…
- [Medium] mt7530_lib_port_change_mtu() writes DIV_ROUND_UP(length, 1024)
into MT7530_GMACCR_MAX_RX_JUMBO.
- [Medium] an8855_set_mac_eee() rejects any tx_lpi_timer > 0xFFF (4095
us).
- [Medium] The new an8855_mib table exposes, as private ethtool -S
strings, counters that already have standard uAPI homes, and the…
- [Medium] an8855_setup() clears the AN8855_UNIPMF (IP unknown-multicast
flood) bit for every user port, and nothing sets it again.
- [Medium] an8855_pcs_config() updates AN8855_PLL_CTRL_2 with
AN8855_RG_DA_QP_PLL_SDM_HREN_INTF, a field an8855.h defines in…
- [Medium] SGMII without in-band negotiation is always forced to 1000
Mb/s in the PCS.
- [Medium] an8855_port_fdb_dump() declares 'u32 rsp;' without
initialising it and passes it to mt7530_lib_fdb_cmd().
- [Low] NET_DSA_AN8855 can be enabled with COMPILE_TEST and
CONFIG_REGMAP=n.
- [Low] an8855_pcs_config() changes several mode-specific SerDes/PCS bits
in one direction only.
- [Low] an8855_rmon_ranges gives the top RMON histogram bucket as { 1519,
AN8855_MAX_MTU }.
--
net: dsa: Add Airoha AN8855 5-Port Gigabit DSA Switch driver
This patch adds a DSA driver for the switch inside the Airoha AN8855. It
reuses the shared mt7530-lib helpers through a per-chip regmap_field
table, and it extends the lib with several AN8855-specific fields. It
also adds a PCS for the SGMII/2500base-X CPU port and EEE support.
> diff --git a/drivers/net/dsa/mediatek/Kconfig b/drivers/net/dsa/mediatek/Kconfig
> index 7b48300dca0c5..2799e78a86517 100644
> --- a/drivers/net/dsa/mediatek/Kconfig
> +++ b/drivers/net/dsa/mediatek/Kconfig
> @@ -1,4 +1,14 @@
> # SPDX-License-Identifier: GPL-2.0-only
> +config NET_DSA_AN8855
> + tristate "Airoha AN8855 Ethernet switch support"
> + depends on MFD_AIROHA_AN8855 || COMPILE_TEST
> + depends on NET_DSA
> + select NET_DSA_MT7530_LIB
> + select NET_DSA_TAG_AIROHA
[Severity: Low]
Can this fail to build with COMPILE_TEST=y, MFD_AIROHA_AN8855=n and
CONFIG_REGMAP=n?
REGMAP is only guaranteed through the select REGMAP in
MFD_AIROHA_AN8855. Neither NET_DSA_MT7530_LIB nor NET_DSA selects it.
In include/linux/regmap.h, struct reg_field and devm_regmap_field_alloc()
only exist under CONFIG_REGMAP, and there are no stubs for them.
struct mt7530_reg_field and an8855_setup_lib_priv() use both.
Is a select REGMAP needed on NET_DSA_AN8855 or NET_DSA_MT7530_LIB?
> diff --git a/drivers/net/dsa/mediatek/an8855.c b/drivers/net/dsa/mediatek/an8855.c
> new file mode 100644
> index 0000000000000..b4b02b7cb6eb8
> --- /dev/null
> +++ b/drivers/net/dsa/mediatek/an8855.c
> @@ -0,0 +1,1741 @@
[ ... ]
> + { MT7530_VAWD_IVL_MAC, REG_FIELD(AN8855_VAWD0, 5, 5) },
> + { MT7530_VAWD_EG_CON, REG_FIELD(AN8855_VAWD0, 11, 11) },
> + { MT7530_VAWD_VTAG_EN, REG_FIELD(AN8855_VAWD0, 10, 10) },
> + { MT7530_VAWD_PORT_MEM, REG_FIELD(AN8855_VAWD0, 26, 31) },
> + { MT7530_VAWD_FID, REG_FIELD(AN8855_VAWD0, 1, 4) },
> + { MT7530_VAWD_VLAN_VALID, REG_FIELD(AN8855_VAWD0, 0, 0) },
> + { __MT7530_VAWD1, REG_FIELD(AN8855_VAWD0, 0, 31) },
> +
> + { MT7530_VAWD_ETAG, REG_FIELD(AN8855_VAWD0, 12, 23) },
> + { __MT7530_VAWD2, REG_FIELD(AN8855_VAWD1, 0, 31) },
[Severity: High]
On AN8855, do the VLAN read-modify-write paths in mt7530-lib.c read
back the correct register?
After the VTCR read command, mt7530_hw_vlan_update() reads the entry
through the VAWD fields:
mt7530_hw_vlan_update() {
...
mt7530_vlan_cmd(priv, MT7530_VTCR_RD_VID, vid);
regmap_field_read(priv->fields[MT7530_VAWD_PORT_MEM], &val);
entry->old_members = val;
...
}
mt7530_hw_vlan_del() also checks:
regmap_field_read(priv->fields[MT7530_VAWD_VLAN_VALID], &val);
if (!val) {
In this table all of those fields map to AN8855_VAWD0, which is the
write-data register. an8855.h defines a read-data register, but nothing
uses it:
/* Same register field of VAWD0 */
#define AN8855_VARD0 0x10200618
If the hardware puts the read result in VARD0, the fetched entry is
whatever was last written to VAWD0. For example, an8855_setup() ends
with mt7530_lib_setup_vlan0(), which leaves PORT_MEM set to all members
and VALID=1. The first bridge VLAN add would then read old_members as
all ports.
Could this make ports members of VIDs they were never added to, or carry
ETAG bits from one VID to another?
Could it also let mt7530_hw_vlan_del() see a stale VALID=0, after which
WR_VID writes stale data over a live VLAN?
> +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(MT7530_MIB_RX_DROP, -1, "RxDrop"),
> + MIB_DESC(MT7530_MIB_RX_FILTERING, -1, "RxFiltering"),
> + MIB_DESC(MT7530_MIB_RX_CRC_ERR, -1, "RxCrcErr"),
[Severity: Medium]
Should some of these counters go through the standard interfaces rather
than ethtool -S?
RxCrcErr matches the eth-mac FrameCheckSequenceErrors counter and
rtnl_link_stats64.rx_crc_errors. TxCollision matches collisions, and
RxDrop/TxDrop match rx_dropped/tx_dropped.
mt7530_lib_get_eth_mac_stats() never fills FrameCheckSequenceErrors.
an8855_switch_ops also has no .get_stats64, while mt7530.c implements
mt7530_get_stats64() from the same MIB.
[ ... ]
> +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;
[ ... ]
> + 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]
Can rsp be used uninitialized here?
mt7530_lib_fdb_cmd() ignores the result of its final read and returns 0
anyway:
if (rsp)
regmap_field_read(priv->fields[__MT7530_ATC], rsp);
return 0;
regmap_field_read() leaves *val untouched when it fails. If that read
fails after FDB_START, FIELD_GET() runs on an uninitialized stack value.
After FDB_NEXT it would run on a stale value.
Should rsp be initialized, or should mt7530_lib_fdb_cmd() return the read
error?
[ ... ]
> +static int an8855_port_max_mtu(struct dsa_switch *ds, int port)
> +{
> + return AN8855_MAX_MTU;
> +}
[Severity: Medium]
Does mt7530_lib_port_change_mtu() program a valid jumbo value on AN8855?
It writes the MT7530 linear encoding:
regmap_field_write(priv->fields[MT7530_GMACCR_MAX_RX_JUMBO],
DIV_ROUND_UP(length, 1024));
an8855.h documents a non-linear encoding for this field:
#define AN8855_MAX_RX_JUMBO_9K FIELD_PREP_CONST(AN8855_MAX_RX_JUMBO, 0x9)
#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)
At AN8855_MAX_MTU, length is 15338 + 14 + 4 + 4 = 15360, so 0xf gets
written, and that value is not defined.
More generally, frame lengths from 12289 to 15360 give 0xd-0xf. Lengths
from 9217 to 12288 give 0xa-0xc, which the hardware would read as
12K/15K/16K.
[ ... ]
> +static const struct ethtool_rmon_hist_range an8855_rmon_ranges[] = {
[ ... ]
> + { 1024, 1518 },
> + { 1519, AN8855_MAX_MTU },
> + {}
> +};
[Severity: Low]
Should the top bucket's upper bound be a frame length?
The other buckets are frame lengths in octets. AN8855_MAX_MTU (15338) is
an L3 MTU, while the PKT_SZ_1519_TO_MAX counter counts frames up to
15360 bytes.
[ ... ]
> +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]
Should this also write the cached port matrix to hardware?
mt7530_update_port_member() skips the hardware write for disabled ports:
if (priv->ports[port].enable)
regmap_fields_write(priv->fields[MT7530_PCR_MATRIX],
port, port_bitmap);
mt7530_port_enable() in mt7530.c then applies it when the port comes up:
regmap_update_bits(priv->regmap, MT7530_PCR_P(port), PCR_MATRIX_MASK,
PCR_MATRIX(priv->ports[port].pm));
an8855_setup() calls an8855_port_disable() for every user port, so every
port starts with enable=false.
If a port joins a bridge, or is marked isolated, while it is down, does
that matrix ever reach AN8855_PORTMATRIX_P(port)?
If not, bridged ports would not forward to each other in hardware, and an
isolated port would still reach its peers.
[ ... ]
> +static int an8855_set_mac_eee(struct dsa_switch *ds, int port,
> + struct ethtool_keee *e)
> +{
> + if (e->tx_lpi_timer > 0xFFF)
> + return -EINVAL;
> +
> + return 0;
> +}
[Severity: Medium]
Is the 0xFFF limit from MT7530 right for AN8855?
an8855_phylink_get_caps() reports the default timer from the 18-bit
AN8855_LPI_TXIDLE_THD_MASK field, in 2us units:
config->lpi_timer_default = FIELD_GET(AN8855_LPI_TXIDLE_THD_MASK, reg) *
AN8855_TX_LPI_UNIT;
That comes to about 60000us, and phylink reports it via get_eee.
ethtool set-eee does a get-modify-set. dsa_user_set_eee() therefore
passes the unchanged 60000us timer to set_mac_eee(), and the request is
aborted with -EINVAL.
Wouldn't that make "ethtool --set-eee lanX eee off" fail unless tx-timer
is also lowered to 4095 or less?
an8855_phylink_mac_enable_tx_lpi() accepts timers up to the full 18-bit
field and clamps them.
[ ... ]
> + /* Disable Unknown Multicast Forward on user ports */
> + ret = regmap_clear_bits(priv->regmap, AN8855_UNMF, BIT(dp->index));
> + if (ret)
> + return ret;
> +
> + ret = regmap_clear_bits(priv->regmap, AN8855_UNIPMF, BIT(dp->index));
> + if (ret)
> + return ret;
[Severity: Medium]
What sets a user port's AN8855_UNIPMF bit again after this?
an8855_port_pre_bridge_flags() accepts BR_MCAST_FLOOD, but
mt7530_lib_port_bridge_flags() only updates the UNMF mask:
if (flags.mask & BR_MCAST_FLOOD)
regmap_field_update_bits(priv->fields[MT7530_UNM_FFP],
With this, is unknown IP multicast still not flooded between bridged user
ports when mcast_flood is on, which is the bridge default?
[ ... ]
> + /* PLL - SDM */
> + ret = regmap_update_bits(priv->regmap, AN8855_PLL_CTRL_2,
> + AN8855_RG_DA_QP_PLL_SDM_HREN_INTF,
> + FIELD_PREP(AN8855_RG_DA_QP_PLL_SDM_HREN_INTF, 0x0));
> + if (ret)
> + return ret;
[Severity: Medium]
Should this register be AN8855_PLL_CTRL_4?
an8855.h defines the field under PLL_CTRL_4:
#define AN8855_PLL_CTRL_4 0x1022e410
#define AN8855_RG_DA_QP_PLL_SDM_HREN_INTF GENMASK(4, 3)
In PLL_CTRL_2, bits 4:2 hold AN8855_RG_DA_QP_PLL_BPA_INTF, which the
"PLL - LPF" step just set to 0x5.
Clearing bits 4:3 here changes BPA to 0x1, and SDM_HREN in PLL_CTRL_4 is
never written.
[ ... ]
> + } else {
> + ret = regmap_update_bits(priv->regmap, AN8855_RG_AN_SGMII_MODE_FORCE,
> + AN8855_RG_FORCE_CUR_SGMII_MODE |
> + AN8855_RG_FORCE_CUR_SGMII_SEL,
> + AN8855_RG_FORCE_CUR_SGMII_SEL);
> + if (ret)
> + return ret;
[Severity: Low]
Several mode-specific bits in an8855_pcs_config() only ever change in
one direction.
These are only set: FORCE_TXC_SEL, FORCE_SPD_MODE_P0,
FORCE_CUR_SGMII_SEL, P0_MII_MODE, P0_RA_AN_EN and FORCE_AN_DONE.
These are only cleared: SGMII_MODE/SGMII_AN_EN, P0_DIS_MII_MODE and
P0_MII_RA_*.
For example, FORCE_SPD_MODE_P0 is set for forced SGMII but never cleared
for in-band SGMII.
Could moving between in-band SGMII, forced SGMII and 2500base-X, or
starting from a PCS the bootloader configured, leave a mix of register
state from two modes?
[ ... ]
> + /* Force Speed with fixed-link or 2500base-x as doesn't support aneg */
> + if (interface == PHY_INTERFACE_MODE_2500BASEX ||
> + neg_mode != PHYLINK_PCS_NEG_INBAND_ENABLED) {
> + if (interface == PHY_INTERFACE_MODE_2500BASEX)
> + val = AN8855_RG_LINK_MODE_P0_SPEED_2500;
> + else
> + val = AN8855_RG_LINK_MODE_P0_SPEED_1000;
[Severity: Medium]
Does SGMII without in-band negotiation work at 10/100?
an8855_phylink_get_caps() advertises MAC_10 | MAC_100 along with SGMII
on port 5, but this code always forces the PCS to 1000.
an8855_phylink_mac_link_up() only updates PMCR, and an8855_pcs_ops has
no .pcs_link_up to program the resolved speed into the PCS.
With a 10/100 fixed-link, or an out-of-band PHY that resolves to 10/100,
would the PCS stay at 1000 while the MAC runs at the lower speed?
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260928194511.41878-1-ansuelsmth%40gmail.com