Re: [net-next,v15,03/13] net: mdio: realtek-rtl9300: deny C45 over C22 access

[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: 1 · Low: 1

- [Medium] The new guard is keyed purely on the register number, so it
  rejects every C22 access to registers 13/14, not only the C45-over-C22…
- [Low] The patch fixes a defect that exists in released kernels — the
  commit message itself states that C45-over-C22 access "is accepted by…

--

net: mdio: realtek-rtl9300: deny C45 over C22 access

This adds a register number check to the C22 read and write accessors of
the RTL9300 MDIO driver so that accesses to MII_MMD_CTRL and MII_MMD_DATA
return -EOPNOTSUPP and emit a one-time warning. The stated reason is that
hardware polling breaks C45 over C22 PHY access.

No Link: tags are present in the commit header.

> net: mdio: realtek-rtl9300: deny C45 over C22 access
>
>     Hardware polling breaks C45 over C22 PHY access. Currently this
>     is accepted by the driver but it silently fails with undefined
>     results.

[Severity: Low]
The changelog describes behaviour that "is accepted by the driver but it
silently fails with undefined results", which reads like a defect present in
released kernels, but there is no Fixes: tag and no note about stable
routing or about this being deliberately net-next only.

The broken accessors are the pre-existing otto_emdio_read_c22() and
otto_emdio_write_c22() installed as bus->read/bus->write in
otto_emdio_probe_one(), which came in with commit 24e31e4747698 ("net:
mdio: Add RTL9300 MDIO driver"). Another patch of this same series ("net:
mdio: realtek-rtl9300: suppress sysfs bind/unbind attributes") already
carries exactly that tag.

Should this one carry a Fixes: tag as well, so backporters can tell which
kernels are affected and whether the new -EOPNOTSUPP behaviour is intended
for stable?

> diff --git a/drivers/net/mdio/mdio-realtek-rtl9300.c b/drivers/net/mdio/mdio-realtek-rtl9300.c
> index 73ac5fdcd2674..69565a87f99db 100644
> --- a/drivers/net/mdio/mdio-realtek-rtl9300.c
> +++ b/drivers/net/mdio/mdio-realtek-rtl9300.c

[ ... ]

> @@ -453,6 +454,12 @@ static int otto_emdio_read_c22(struct mii_bus *bus, int phy_id, int regnum)
>  	int ret, port;
>  	u32 value;
>  
> +	if (regnum == MII_MMD_CTRL || regnum == MII_MMD_DATA) {
> +		dev_warn_once(&bus->dev,
> +			      "C45 over C22 read access broken due to polling\n");
> +		return -EOPNOTSUPP;
> +	}
> +
>  	port = otto_emdio_phy_to_port(bus, phy_id);
>  	if (port < 0)
>  		return port;

[ ... ]

> @@ -463,11 +470,18 @@ static int otto_emdio_read_c22(struct mii_bus *bus, int phy_id, int regnum)
>  	struct otto_emdio_priv *priv = otto_emdio_bus_to_priv(bus);
>  	int ret, port;
>  
> +	if (regnum == MII_MMD_CTRL || regnum == MII_MMD_DATA) {
> +		dev_warn_once(&bus->dev,
> +			      "C45 over C22 write access broken due to polling\n");
> +		return -EOPNOTSUPP;
> +	}
> +

[Severity: Medium]
Does this filter reject more than the C45 over C22 indirection?

The mii_bus ->read/->write ABI in include/linux/phy.h only passes
(addr, regnum), so otto_emdio_read_c22() and otto_emdio_write_c22() have no
way to tell an MMD indirection sequence from a plain access to C22 register
13/14. The check keys purely on the register number, so every access to
0x0d/0x0e is denied.

Two Clause 22 Realtek PHYs use register 14 as an ordinary vendor register,
and both entries in realtek_drvs explicitly declare no MMD support:

drivers/net/phy/realtek/realtek_main.c:
	static int rtl8211b_suspend(struct phy_device *phydev)
	{
		phy_write(phydev, MII_MMD_DATA, BIT(9));

		return genphy_suspend(phydev);
	}

	static int rtl8211b_resume(struct phy_device *phydev)
	{
		phy_write(phydev, MII_MMD_DATA, 0);

		return genphy_resume(phydev);
	}

	PHY_ID_MATCH_EXACT(0x001cc910),
	.name		= "RTL8211 Gigabit Ethernet",
	.config_aneg	= rtl8211_config_aneg,
	.read_mmd	= &genphy_read_mmd_unsupported,
	.write_mmd	= &genphy_write_mmd_unsupported,

and rtl8211_config_aneg() does the same for its forced-100Mbit quirk:

		phy_write(phydev, 0x17, 0x2138);
		phy_write(phydev, 0x0e, 0x0260);

Both callers discard the phy_write() return value. On a bus that is not
classified as native C45, otto_emdio_probe_one() installs these accessors:

	} else {
		bus->read = otto_emdio_read_c22;
		bus->write = otto_emdio_write_c22;
	}

so rtl8211b_suspend() -> phy_write() -> mdiobus_write() ->
otto_emdio_write_c22() now returns -EOPNOTSUPP before touching hardware,
the RTL8211B power-down bit is never programmed, suspend still reports
success through genphy_suspend(), and the bus prints "C45 over C22 write
access broken due to polling" even though no MMD access took place.

The C22 bus filter added later in the series by "net: mdio:
realtek-rtl9300: Add page tracking" only checks
phy_id_compare_vendor(phydev->phy_id, RTL_VENDOR_ID), which RTL8211
(0x001cc910) and RTL8211B (0x001cc912) both match, so these PHYs are not
excluded.

Does this also break direct register access to 0x0d/0x0e from userspace via
SIOCGMIIREG/SIOCSMIIREG, which reaches phy_mii_ioctl() ->
mdiobus_read()/mdiobus_write() with no MMD emulation involved?

Would it work to scope the rejection to the actual C22 MMD emulation path,
or to express it as a bus level "no MMD over C22" capability that phylib can
consult, instead of filtering two register numbers in the bus accessors?

>  	port = otto_emdio_phy_to_port(bus, phy_id);
>  	if (port < 0)
>  		return port;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831143439.2404484-1-markus.stockhausen%40gmx.de