Re: [net-next,v15,10/13] net: mdio: realtek-rtl9300: Open up C22 and C45 space in parallel

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 4 · Low: 0

- [Medium] otto_emdio_probe_one() now installs both accessor pairs
  (bus->read/write and bus->read_c45/write_c45) on every SMI bus, but…
- [Medium] priv->smi_bus_is_c45[] is set per physical SMI bus whenever
  any DT child of that bus declares compatible = "ethernet-phy-…
- [Medium] Installing bus->read_c45/bus->write_c45 on every SMI bus
  (including the C22-polling ones) makes PHY-driver code paths live that…
- [Medium] The driver's C22 path is Realtek-specific:
  otto_emdio_9300_read_c22()/otto_emdio_9300_write_c22() always encode…

--

net: mdio: realtek-rtl9300: Open up C22 and C45 space in parallel

The patch stops selecting the mii_bus accessors based on the per-bus
smi_bus_is_c45[] flag and instead registers both the C22 and the C45
read/write callbacks on every SMI bus, matching what the downstream
driver does.

Link: https://github.com/openwrt/openwrt/blob/main/target/linux/realtek/files-6.18/drivers/net/phy/realtek/realtek_multiport.c#L349
Link: https://github.com/openwrt/openwrt/blob/main/target/linux/realtek/files-6.18/drivers/net/mdio/mdio-realtek-otto.c#L1072

> diff --git a/drivers/net/mdio/mdio-realtek-rtl9300.c b/drivers/net/mdio/mdio-realtek-rtl9300.c
> index ae7ba86e90b9a..b90972c1dc6a2 100644
> --- a/drivers/net/mdio/mdio-realtek-rtl9300.c
> +++ b/drivers/net/mdio/mdio-realtek-rtl9300.c
> @@ -688,13 +688,10 @@ static int otto_emdio_probe_one(struct device *dev, struct otto_emdio_priv *priv
>  		return -ENOMEM;
>  
>  	bus->name = "Realtek Switch MDIO Bus";
> -	if (priv->smi_bus_is_c45[mdio_bus]) {
> -		bus->read_c45 = otto_emdio_read_c45;
> -		bus->write_c45 = otto_emdio_write_c45;
> -	} else {
> -		bus->read = otto_emdio_read_c22;
> -		bus->write = otto_emdio_write_c22;
> -	}
> +	bus->read_c45 = otto_emdio_read_c45;
> +	bus->write_c45 = otto_emdio_write_c45;
> +	bus->read = otto_emdio_read_c22;
> +	bus->write = otto_emdio_write_c22;

[Severity: Medium]
The commit message says that "when polling is set to C22 most of the C45
space can not be accessed" and that only "EEE register access is allowed
independently from the polling mode".  Should the opening up be limited to
that documented exception rather than to the whole opposite address space?

The hardware frame format and the PPU scan mode are still derived per bus
from priv->smi_bus_is_c45[] in otto_emdio_9300_setup_controller():

	if (priv->smi_bus_is_c45[i])
		glb_ctrl_val |= RTL9300_GLB_CTRL_INTF_SEL(i);

and in otto_emdio_9310_setup_controller():

	err = regmap_assign_bits(priv->regmap, RTL9310_SMI_GLB_CTRL1,
				 RTL9310_SMI_GLB_FMT_SEL_C45(i),
				 priv->smi_bus_is_c45[i]);

so the restriction described in the changelog is unchanged.  Before this
patch an access into the non-configured space was refused by mdio_bus.c
because the callback was NULL:

drivers/net/phy/mdio_bus.c:__mdiobus_c45_read() {
	if (bus->read_c45)
		retval = bus->read_c45(bus, addr, devad, regnum);
	else
		retval = -EOPNOTSUPP;
	...
}

Now the command is issued to the controller instead.  otto_emdio_run_cmd()
only reports the controller FAIL bit:

	return cmdstate & info->cmd_fail ? -ENXIO : 0;

Can a command that the controller accepts but cannot perform correctly in
the current mode return success here, so that otto_emdio_read_c45() hands
the data register contents to phylib as a valid PHY register value?  And can
a wrongly executed write leave the PHY in a state the driver has no record
of?

The port is also not excluded from the polling engine around a transfer -
otto_emdio_set_port_polling() is only toggled from the attach/detach
notifiers, and priv->lock only serializes the CPU side command writes.

Note the driver keeps the opposite policy elsewhere.  The C22 handlers still
refuse MMD access with:

	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;
	}

and the file header comment states:

 * - Access to MDIO_AN_EEE_ADV and MDIO_AN_EEE_LPABLE works via C45 over C22.
 * - It is unclear if these sequences change for different PHYs.

Should the header comment and the MMD guard be updated together with this
change, or should the new access be narrowed to the EEE registers?

[Severity: Medium]
Does this change PHY detection and binding on device trees that mix C22 and
C45 PHYs on one SMI bus?

otto_emdio_map_ports() sets the flag for the whole bus as soon as a single
child declares Clause 45:

	if (of_device_is_compatible(phy_dn, "ethernet-phy-ieee802.3-c45"))
		priv->smi_bus_is_c45[bus] = true;

and setup_controller() then programs the frame format for the entire
interface from that flag.  Previously a PHY of the opposite protocol on such
a bus was unreachable, so get_phy_c22_id()/get_phy_c45_ids() returned
-EOPNOTSUPP and of_mdiobus_register() failed with a clear error.  With both
callback pairs installed the ID read is now issued on an interface programmed
for the other framing, and since otto_emdio_run_cmd() only checks the FAIL
bit, whatever is in the data register becomes the PHY ID used for driver
matching.

The C45 fallback in get_phy_device() also becomes reachable on C22 polling
buses:

drivers/net/phy/phy_device.c:get_phy_device() {
	if (!is_c45 && phy_id == 0 && bus->read_c45) {
		r = get_phy_c45_ids(bus, addr, &c45_ids);
		if (!r)
			return phy_device_create(bus, addr, phy_id,
						 true, &c45_ids);
	}
	...
}

Can this create a bogus C45 phydev where registration previously returned
-ENODEV?

Once such a mismatched PHY is attached, otto_emdio_notify_phy_attach()
enables hardware polling for the port without comparing the individual PHY
protocol against the bus polling mode:

	if (!priv->smi_bus_is_c45[chan->mdio_bus] &&
	    !phy_id_compare_vendor(phydev->phy_id, RTL_VENDOR_ID)) {
		phydev_err(phydev, "Only Realtek PHYs allowed on C22 bus\n");
		return -EOPNOTSUPP;
	}

	return otto_emdio_set_port_polling(priv, port, true);

Would the PPU then send wrong-format polling transactions for that port,
leaving MAC_LINK_STS unpopulated as described in the file header comment?

[Severity: Medium]
Can enabling read_c45/write_c45 on C22 polling buses route MMD accesses to
the wrong PHY on the same bus?

The Realtek PHY driver reaches the bus C45 accessors through helpers that
pass the literal address 0 rather than phydev->mdio.addr:

drivers/net/phy/realtek/realtek_main.c:
static int rtlgen_read_vend2(struct phy_device *phydev, int regnum)
{
	return __mdiobus_c45_read(phydev->mdio.bus, 0, MDIO_MMD_VEND2, regnum);
}

static int rtlgen_write_vend2(struct phy_device *phydev, int regnum, u16 val)
{
	return __mdiobus_c45_write(phydev->mdio.bus, 0, MDIO_MMD_VEND2, regnum,
				   val);
}

Those helpers were written for the r8169 internal PHY bus (commit
da681ed73fb98 "net: phy: realtek: improve mmd register access for internal
PHY's").  rtlgen_read_mmd()/rtlgen_write_mmd() use them precisely for the
EEE registers this patch wants to open up:

	if (devnum == MDIO_MMD_VEND2)
		ret = rtlgen_read_vend2(phydev, regnum);
	else if (devnum == MDIO_MMD_PCS && regnum == MDIO_PCS_EEE_ABLE)
		ret = rtlgen_read_vend2(phydev, RTL_MDIO_PCS_EEE_ABLE);
	else if (devnum == MDIO_MMD_AN && regnum == MDIO_AN_EEE_ADV)
		ret = rtlgen_read_vend2(phydev, RTL_MDIO_AN_EEE_ADV);

and they are installed as .read_mmd/.write_mmd on the "Generic FE-GE Realtek
PHY" entry matched by rtlgen_match_phy_device() for RTL_GENERIC_PHYID, a
Realtek vendor ID this MDIO driver explicitly admits on C22 buses.

Before this patch __mdiobus_c45_read() returned -EOPNOTSUPP on such a bus.
Now the literal 0 is resolved against the shared switch bus:

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

So phy_ethtool_set_eee() -> genphy_c45_write_eee_adv() -> phy_write_mmd()
for port N would rewrite the EEE advertisement of whichever port is mapped
to SMI address 0, and genphy_c45_read_eee_abilities() would populate port
N's supported_eee from that port.  If no port on the bus uses address 0,
otto_emdio_phy_to_port() returns -ENOENT instead of -EOPNOTSUPP, which
callers that treat -EOPNOTSUPP as "feature absent" do not expect:

drivers/net/phy/realtek/realtek_main.c:rtl822x_read_mmd() {
	...
	if (ret != -EOPNOTSUPP)
		return ret;
	...
}

The hardcoded address 0 predates this series, but is this patch what makes
it reachable on C22 buses?

[Severity: Medium]
Registering the C22 accessors on C45 buses also makes them reachable for
non-Realtek PHYs.  Should the vendor gating follow the PHY rather than the
bus?

The C22 path is Realtek specific.  otto_emdio_9300_read_c22() and
otto_emdio_9300_write_c22() always encode the page fields:

	.c22_data	= FIELD_PREP(RTL9300_PHY_CTRL_REG_ADDR, regnum) |
			  FIELD_PREP(RTL9300_PHY_CTRL_PARK_PAGE, 0x1f) |
			  FIELD_PREP(RTL9300_PHY_CTRL_MAIN_PAGE, priv->page[port]),

so per the file header comment the controller emits a page select write to
PHY register 31 before every C22 transaction, while
otto_emdio_read_c22()/otto_emdio_write_c22() emulate register 31 in
software.

The only guard against doing that to a PHY without a Realtek page register
is bus scoped, in otto_emdio_notify_phy_attach():

	if (!priv->smi_bus_is_c45[chan->mdio_bus] &&
	    !phy_id_compare_vendor(phydev->phy_id, RTL_VENDOR_ID)) {
		phydev_err(phydev, "Only Realtek PHYs allowed on C22 bus\n");
		return -EOPNOTSUPP;
	}

On a C45 bus that check is skipped, and a non-Realtek 10G PHY is legitimately
allowed there.  Previously bus->read/bus->write were NULL on such a bus, so
no C22 access could reach that PHY.  Now a PHY driver's phy_read()/
phy_write(), or userspace via ioctl(SIOCGMIIREG)/ioctl(SIOCSMIIREG), takes
the non-C45 branch:

drivers/net/phy/phy.c:phy_mii_ioctl() {
	case SIOCGMIIREG:
		if (mdio_phy_id_is_c45(mii_data->phy_id)) {
			...
			ret = mdiobus_c45_read(phydev->mdio.bus, prtad, devad,
	...
}

Can this write the vendor specific register 31 of a foreign PHY, and can a
read of register 31 return driver bookkeeping instead of the device's actual
register content, leaving software and hardware state diverged?

Extending the vendor check to the C22 accessors on every bus, or omitting the
page fields for non-Realtek devices, would keep the gating with the PHY.

>  	bus->parent = dev;
>  	bus->notify_phy_attach = otto_emdio_notify_phy_attach;
>  	bus->notify_phy_detach = otto_emdio_notify_phy_detach;

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