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

On Thu, 2026-07-30 at 16:37 +0000, [email protected] wrote:
> 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 <[email protected]>
> 
> 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.

This code will be reworked or removed in next version.

> 
> [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.
> 

This code will be reworked or removed in next version.

> > +}
> 
> [ ... ]
> 
> > 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.
> 

This will be reworked based on other review comments. I'll check afterward if
the problem still exists.

> [ ... ]
> 
> > +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.
> 

No, this pattern is used in other files and proposed in devres.c:
 \* Usage example:
 \*
 \*      base = devm_of_iomap(&pdev->dev, node, 0, NULL);
 \*      if (IS_ERR(base))
 \*              return PTR_ERR(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.
> 

Yes, ds->phys_mii_mask is initialized in dsa.c dsa_switch_setup():

/\* Initialize ds->phys_mii_mask before registering the user MDIO bus  
 \* driver and before ops->setup() has run, since the switch drivers and  
 \* the user MDIO bus driver rely on these values for probing PHY  
 \* devices or not  
 \*/  
ds->phys_mii_mask |= dsa_user_ports(ds);

> [ ... ]
> 
> > +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?
> 
ds->phys_mii_mask is valid here, see the comment above.

> [ ... ]
> 
> > +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.
> 

Good point, I will fix that.

> [ ... ]
> 
> > +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?
> 
I will also fix that in the next version.