Re: [PATCH 4/4] net: dsa: soce: Add initial driver support for MRS switches
From: Vasilij Strassheim <hidden>
Date: 2026-08-03 16:44:59
Also in:
sashiko-reviews
On Thu, 2026-07-30 at 16:37 +0000, sashiko-bot@kernel.org wrote:
quoted hunk ↗ jump to hunk
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 [off-list ref] 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.quoted
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[ ... ]quoted
+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.
quoted
+}[ ... ]quoted
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[ ... ]quoted
+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.
[ ... ]quoted
+static int soce_sw_probe(struct mdio_device *mdiodev) +{[ ... ]quoted
+ 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);
[ ... ]quoted
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[ ... ]quoted
+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);
[ ... ]quoted
+int soce_user_mii_bus_register(struct dsa_switch *ds) +{[ ... ]quoted
+ 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.
[ ... ]quoted
+int soce_mdio_20_03_write(struct dsa_switch *ds, int portnumber, int regnum, + u16 val) +{[ ... ]quoted
+ /* 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.
[ ... ]quoted
+static int soce_mdio_23_02_read_c22(struct dsa_switch *ds, int portnumber, + int regnum) +{[ ... ]quoted
+ 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.