Re: [PATCH net-next 2/5] net: pcs: pcs-xpcs-regmap: support XPCS memory-mapped MDIO bus via regmap

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] net: pcs: MDIO bus ID collision in devm_xpcs_regmap_register()
--

commit f4103deca1c1d0d48e4d0e095369247a1ba3779b
Author: Daniel Thompson <daniel@riscstar.com>

net: pcs: pcs-xpcs-regmap: support XPCS memory-mapped MDIO bus via regmap

This commit introduces devm_xpcs_regmap_register() and indirect MDIO CSR
access helpers so drivers can register a DesignWare XPCS instance backed by a
regmap.

> diff --git a/drivers/net/pcs/pcs-xpcs-regmap.c b/drivers/net/pcs/pcs-xpcs-regmap.c
> new file mode 100644
> index 0000000000000..f3f3aaaf0b75a
> --- /dev/null
> +++ b/drivers/net/pcs/pcs-xpcs-regmap.c
> @@ -0,0 +1,169 @@

[ ... ]

> +struct dw_xpcs *devm_xpcs_regmap_register(struct device *dev,
> +					  struct regmap *regmap)
> +{
> +	static atomic_t id = ATOMIC_INIT(-1);
> +	struct dw_xpcs *xpcs;
> +	struct mii_bus *bus;
> +	int ret;
> +
> +	bus = devm_mdiobus_alloc_size(dev, 0);
> +	if (!bus)
> +		return ERR_PTR(-ENOMEM);
> +
> +	bus->name = "DW XPCS MCI/APB3";
> +	bus->read = xpcs_regmap_read_c22;
> +	bus->write = xpcs_regmap_write_c22;
> +	bus->read_c45 = xpcs_regmap_read_c45;
> +	bus->write_c45 = xpcs_regmap_write_c45;
> +	bus->phy_mask = ~0;
> +	bus->parent = dev;
> +	bus->priv = regmap;
> +
> +	snprintf(bus->id, sizeof(bus->id), "dwxpcs-%x", atomic_inc_return(&id));

[Severity: Medium]
Can bus->id collide with an existing platform DW XPCS bus here?

In drivers/net/pcs/pcs-xpcs-plat.c, xpcs_plat_init_bus() defines its own
separate static atomic counter and uses the identical "dwxpcs-%x" format
string:

drivers/net/pcs/pcs-xpcs-plat.c:xpcs_plat_init_bus() {
	static atomic_t id = ATOMIC_INIT(-1);
	...
	snprintf(pxpcs->bus->id, MII_BUS_ID_SIZE,
		 "dwxpcs-%x", atomic_inc_return(&id));
}

If a system probes both a platform DW XPCS via xpcs_plat_probe() ->
xpcs_plat_init_bus() and a regmap DW XPCS via devm_xpcs_regmap_register()
(for example, an SoC with a platform XPCS and a PCIe Toshiba TC9564 card),
both counters start at -1 and format their first bus->id as "dwxpcs-0".

When devm_mdiobus_register() calls __mdiobus_register() -> device_register(),
device_add_class_symlinks() tries to create the class symlink under
/sys/class/mdio_bus/dwxpcs-0, which fails with -EEXIST and causes
__mdiobus_register() to fail with -EINVAL.

Should devm_xpcs_regmap_register() use a distinct prefix (such as
"dwxpcs-regmap-%x") or share a common counter or IDA with
xpcs_plat_init_bus()?

> +
> +	/* MDIO-bus here serves as just a back-end engine abstracting out
> +	 * the MDIO and MCI/APB3 IO interfaces utilized for the DW XPCS CSRs
> +	 * access.
> +	 */
> +	ret = devm_mdiobus_register(dev, bus);

[ ... ]

> +}
> +EXPORT_SYMBOL_GPL(devm_xpcs_regmap_register);

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019030129.1470307-1-elder@riscstar.com?part=2