Re: [PATCH 4/4] net: dsa: soce: Add initial driver support for MRS switches

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Incorrect error check for `devm_of_iomap()` leads to potential NULL pointer dereference on mapping failure.
- [High] The `ds->phys_mii_mask` is never initialized, causing the driver's custom MDIO bus to silently ignore all PHY reads and writes.
- [High] Concurrent MDIO accesses via `slave_mii_bus` and `user_mii_bus` race on shared hardware registers, causing state corruption.
- [Medium] Missing bitmasks on input parameters in MDIO hardware register construction allows out-of-bounds values to corrupt adjacent register fields.
- [Medium] The driver unconditionally issues a flush command to the hardware FDB controller even if the previous command is still being processed.
- [Medium] Asynchronous, switch-wide FDB flush is performed instead of a synchronous, per-port flush.
--

--- Patch [4]: [PATCH 4/4] net: dsa: soce: Add initial driver support for MRS switches ---
commit 4ad10a20f72f216349a16415f424608c5d3e5c86
Author: Vasilij Strassheim <v.strassheim@linutronix.de>

net: dsa: soce: Add initial driver support for MRS switches

This commit adds initial support for SoC-e switches based on FPGA IP cores.
It provides MDIO access helpers for supported IP register layouts and common
STP/FDB handling.

> diff --git a/drivers/net/dsa/soce/soce_common.c b/drivers/net/dsa/soce/soce_common.c
> index 0000000000000..f7f345a690e46
> --- /dev/null
> +++ b/drivers/net/dsa/soce/soce_common.c

[ ... ]

> +static void soce_fast_age(struct dsa_switch *ds, int port,
> +			  const struct soce_layout *layout)
> +{
> +	struct soce_priv *priv = ds->priv;
> +	struct soce_dsa_local *local;
> +	void __iomem *p_mes_ctrl;
> +	u32 val;
> +
> +	local = &priv->local;
> +	if (!layout || !local->base_addr)
> +		return;
> +
> +	p_mes_ctrl = local->base_addr + layout->fdb_ctrl;
> +	if (readl_poll_timeout(p_mes_ctrl, val,
> +			       !(val & SOCE_FDB_CLEAR_UNDER_PROCESS),
> +			       SOCE_FAST_AGE_POLL_US, SOCE_FAST_AGE_TIMEOUT_US))
> +		dev_warn(ds->dev, "SoC-e fast_age timeout on port %d\n", port);
> +
> +	writel(SOCE_FDB_CLEAR_FLUSH_MSTID, p_mes_ctrl);

[Severity: Medium]
Does this code continue to issue the flush command even if the hardware is
still processing the previous one? If readl_poll_timeout() fails, it prints
a warning but unconditionally executes the writel() below it.

[Severity: Medium]
Will this flush the entire MSTID globally rather than just the specified port?
The command ignores the port argument. Additionally, it appears to return
immediately without waiting for the clear operation to complete.

> +}

[ ... ]

> diff --git a/drivers/net/dsa/soce/soce_dsa_core.c b/drivers/net/dsa/soce/soce_dsa_core.c
> index 0000000000000..b88d00c56d718
> --- /dev/null
> +++ b/drivers/net/dsa/soce/soce_dsa_core.c

[ ... ]

> +static int soce_phy_read(struct dsa_switch *ds, int port, int regnum)
> +{
> +	struct soce_priv *priv = ds->priv;
> +	struct soce_dsa_local *local;
> +
> +	local = &priv->local;
> +	if (!local->mdio_ops || !local->mdio_ops->phy_read)
> +		return 0xffff;
> +
> +	return local->mdio_ops->phy_read(ds, port, regnum);
> +}

[Severity: High]
Can concurrent MDIO accesses race on the shared hardware registers here?
The DSA core implicitly registers the slave_mii_bus, and the driver explicitly
registers a user_mii_bus. Both buses have their own independent mdio_lock
mutex, but they share the same underlying MMIO routines.

[ ... ]

> +static int soce_sw_probe(struct mdio_device *mdiodev)
> +{
[ ... ]
> +	local->base_addr = devm_of_iomap(dev, switch_node, 0, NULL);
> +	if (IS_ERR(local->base_addr)) {
> +		dev_err(dev, "failed to map switch register space for %pOF\n",
> +			switch_node);
> +		of_node_put(switch_node);
> +		return PTR_ERR(local->base_addr);
> +	}

[Severity: High]
Does devm_of_iomap() return NULL on failure rather than an error pointer?
Checking IS_ERR() here will evaluate to false if it returns NULL, leading to a
potential NULL pointer dereference later when accessing the MMIO base.

[ ... ]

> diff --git a/drivers/net/dsa/soce/soce_mdio.c b/drivers/net/dsa/soce/soce_mdio.c
> index 0000000000000..241fc373de8b8
> --- /dev/null
> +++ b/drivers/net/dsa/soce/soce_mdio.c

[ ... ]

> +static int soce_user_phy_read(struct mii_bus *bus, int addr, int reg)
> +{
> +	struct dsa_switch *ds = bus->priv;
> +
> +	if (ds->phys_mii_mask & BIT(addr))
> +		return ds->ops->phy_read(ds, addr, reg);
> +
> +	return 0xffff;
> +}

[Severity: High]
Is ds->phys_mii_mask ever initialized? It appears to default to 0 when
allocated in soce_sw_probe(). If it is always 0, this check will fail and
all user PHY reads will silently return 0xffff.

[ ... ]

> +int soce_user_mii_bus_register(struct dsa_switch *ds)
> +{
[ ... ]
> +	ds->user_mii_bus->parent = ds->dev;
> +	ds->user_mii_bus->phy_mask = ~ds->phys_mii_mask;

[Severity: High]
Since ds->phys_mii_mask defaults to 0, will setting phy_mask to the inverted
value instruct the MDIO core to ignore all PHYs and prevent probing entirely?

[ ... ]

> +int soce_mdio_20_03_write(struct dsa_switch *ds, int portnumber, int regnum,
> +			  u16 val)
> +{
[ ... ]
> +	/* Then write control */
> +	regvalue = (mdio->phy_addr << SOCE_MDIO_20_03_CTRL_PHY_ADDR_SHIFT) |
> +		   (regnum << SOCE_MDIO_20_03_CTRL_REG_ADDR_SHIFT) |
> +		   SOCE_MDIO_20_03_CTRL_OP_WRITE |
> +		   SOCE_MDIO_20_03_CTRL_START |
> +		   mdio->mdio_output;
> +	writel(regvalue, ctrl);

[Severity: Medium]
Should the phy_addr and regnum inputs be bounded or masked before shifting?
If an oversized regnum is passed in via userspace ioctls, or a malformed DT
provides a large soce,phy-addr, the shifted bits could overflow into adjacent
control register fields.

[ ... ]

> +static int soce_mdio_23_02_read_c22(struct dsa_switch *ds, int portnumber,
> +				    int regnum)
> +{
[ ... ]
> +	regvalue = (regnum << SOCE_MDIO_23_02_CTRL_REGADDRDEVTYPE_OFFSET) +
> +		   (mdio->phy_addr << SOCE_MDIO_23_02_CTRL_PHYADDR_OFFSET);
> +	writel(regvalue, params);

[Severity: Medium]
Similarly here, can the addition of unmasked values risk arithmetic overflow
into adjacent bit ranges if the inputs are out-of-bounds?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260729-devel-vstrassheim-soce-dsa-ml-v1-0-be569dae1b20@linutronix.de?part=4