Re: [PATCH net-next v3 5/8] net: mdio: Add SoC-e SWIP MDIO controller driver

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 2 · Low: 0

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

- [Medium] soce_mdio_iomap() (drivers/net/mdio/mdio-soce.c:48) returns
  whatever devm_ioremap() returns.
- [Medium] The external MDIO bus selector (bits 26:16) lives in the same
  32-bit control register as OPSTATUS (bit 0), but the idle check does…

--

net: mdio: Add SoC-e SWIP MDIO controller driver

This adds a driver for the MDIO controller in SoC-e Ethernet switch IP
cores. It supports Clause 22 and Clause 45 accesses. The transaction
control register also holds an external MDIO bus selector in bits 26:16,
which a generic MMIO MDIO mux manages and which the driver preserves when
it starts a transaction.

> diff --git a/drivers/net/mdio/mdio-soce.c b/drivers/net/mdio/mdio-soce.c
> new file mode 100644
> index 0000000000000..59c0b3da483eb
> --- /dev/null
> +++ b/drivers/net/mdio/mdio-soce.c
> @@ -0,0 +1,239 @@

[ ... ]

> +static void __iomem *soce_mdio_iomap(struct device *dev, int index)
> +{
> +	struct resource res;
> +	int ret;
> +
> +	ret = of_address_to_resource(dev->of_node, index, &res);
> +	if (ret)
> +		return IOMEM_ERR_PTR(ret);
> +
> +	return devm_ioremap(dev, res.start, resource_size(&res));

[Severity: Medium]
Can a failed mapping get past the checks in soce_mdio_probe()?

devm_ioremap() returns NULL on failure, not an ERR_PTR. This happens both
when the devres allocation fails and when ioremap() fails:

lib/devres.c:__devm_ioremap() {
	void __iomem **ptr, *addr = NULL;
	...
	if (!ptr)
		return NULL;
	...
	} else
		devres_free(ptr);

	return addr;
}

soce_mdio_probe() checks both results only with IS_ERR():

	priv->data = soce_mdio_iomap(dev, SOCE_MDIO_DATA_IOMAP_IDX);
	if (IS_ERR(priv->data))
		return PTR_ERR(priv->data);

	priv->ctrl = soce_mdio_iomap(dev, SOCE_MDIO_CTRL_IOMAP_IDX);
	if (IS_ERR(priv->ctrl))
		return PTR_ERR(priv->ctrl);

IS_ERR(NULL) is false, so probe would continue into
devm_of_mdiobus_register() with a NULL base.

The first bus access would then dereference NULL. That access comes either
from a PHY child during registration or through the mux:

mdio_mux_read()
  soce_mdio_read()
    soce_mdio_wait_for_idle()
      readl_poll_timeout(NULL, ...)

Could soce_mdio_iomap() convert a NULL result into IOMEM_ERR_PTR(-ENOMEM),
or could probe use IS_ERR_OR_NULL()? A helper that also requests the region
doesn't fit here, because the parent switch driver already requests the
whole range.

> +}
> +
> +static int soce_mdio_wait_for_idle(struct soce_mdio *priv)
> +{
> +	void __iomem *ctrl = priv->ctrl;
> +	u32 val;
> +
> +	return readl_poll_timeout(ctrl, val,
> +		!(val & SOCE_MDIO_CTRL_OPSTATUS), 10,
> +		SOCE_MDIO_TIMEOUT_US);
> +}
> +
> +static void soce_mdio_start(struct soce_mdio *priv, u32 command)
> +{
> +	void __iomem *ctrl = priv->ctrl;
> +
> +	/* Keep the currently selected MDIO bus while updating op bits. */
> +	command |= readl(ctrl) & SOCE_MDIO_CTRL_BUS_MASK;
> +	writel(command, ctrl);
> +}
> +
> +static int soce_mdio_read(struct mii_bus *bus, int phy_addr, int regnum)
> +{
> +	struct soce_mdio *priv = bus->priv;
> +	void __iomem *data = priv->data;
> +	u32 command;
> +	int ret;
> +
> +	ret = soce_mdio_wait_for_idle(priv);
> +	if (ret)
> +		return ret;

[Severity: Medium]
Does this idle check also protect the bus selector bits in the same
register?

The commit message says:

    Return a timeout instead of waiting indefinitely, and allow a later
    access to retry if the hardware becomes idle again.

So when a completion wait returns -ETIMEDOUT, the engine may still be busy.
Nothing aborts or resets it before mdio_lock is released, and there is no
mii_bus->reset callback.

If the next access goes through the mux to a different child bus,
mdio_mux_read() calls switch_fn before it calls the parent's read callback:

drivers/net/mdio/mdio-mux.c:mdio_mux_read() {
	...
	r = pb->switch_fn(pb->current_child, cb->bus_number, pb->switch_data);
	...
	r = pb->mii_bus->read(pb->mii_bus, phy_id, regnum);
	...
}

mdio_mux_mmioreg_switch_fn() then rewrites the selector halfword without
checking OPSTATUS:

			x = ioread16(p);
			y = (x & ~s->mask) | desired_child;
			if (x != y) {
				iowrite16((x & ~s->mask) | desired_child, p);

This means soce_mdio_wait_for_idle() runs only after bits 26:16 have
already changed.

If the hardware doesn't latch the selector when a transaction starts, could
the rest of the timed-out frame be sent to a different external MDIO bus?
The original PHY would get a truncated frame and the newly selected bus
would get a fragment from the middle of it. Neither the mux's current_child
nor the return code would show that this happened.

soce_mdio_write() and the c45 variants have the same ordering. Does this IP
latch the selector per transaction, or does the idle check also need to
cover the selector writes?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260923-devel-vstrassheim-soce-dsa-ml-v3-0-ddebafcb9ba7%40linutronix.de