Re: [PATCH net-next v12 05/11] net: mdio: realtek-rtl9300: Configure hardware polling during probing

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

New issues:
- [High] notify_phy_attach aborts phy_attach_direct() for PHYs unmapped to switch ports

Pre-existing issues:
- [High] Hardware polling corrupts software C45-over-C22 MMD access sequences at runtime
--

--- Patch [5]: [PATCH net-next v12 05/11] net: mdio: realtek-rtl9300: Configure hardware polling during probing ---
commit b348a1eaa3ac89042c8633ae18e01b3be6294f9d
Author: Markus Stockhausen <[email protected]>

net: mdio: realtek-rtl9300: Configure hardware polling during probing

During PHY probing and configuration complex configuration sequences
might be issued and firmware might be loaded. Hardware polling can
interfere badly with that. This patch disables polling for all ports
when the MDIO driver starts, reenables it after the PHY has been
attached, and disables it just before the PHY is detached.

Link: https://github.com/openwrt/openwrt/blob/main/target/linux/realtek/files-6.18/drivers/net/mdio/mdio-realtek-otto.c#L818
Link: https://lore.kernel.org/netdev/680696024a8648535ce6dee771fe4de67802e0e8.1769053496.git.daniel@makrotopia.org/

> diff --git a/drivers/net/mdio/mdio-realtek-rtl9300.c b/drivers/net/mdio/mdio-realtek-rtl9300.c
> index 1873928b362dd..210e570c09732 100644
> --- a/drivers/net/mdio/mdio-realtek-rtl9300.c
> +++ b/drivers/net/mdio/mdio-realtek-rtl9300.c

[ ... ]

> @@ -582,6 +591,33 @@ static int otto_emdio_9310_setup_controller(struct otto_emdio_priv *priv)
>  	return 0;
>  }
>  
> +static int otto_emdio_notify_phy_attach(struct phy_device *phydev)
> +{
> +	struct otto_emdio_priv *priv = otto_emdio_bus_to_priv(phydev->mdio.bus);
> +	int port = otto_emdio_phy_to_port(phydev->mdio.bus, phydev->mdio.addr);
> +
> +	if (port < 0)
> +		return port;

[Severity: High]
Are PHYs unmapped to switch ports handled correctly here?

By returning a negative error code (e.g., -ENOENT) instead of 0 here, it
looks like it will cause the generic phylib attachment sequence to abort.
A MAC driver calling phy_connect() for an unmapped PHY on the bus will
hit this when otto_emdio_phy_to_port() returns -ENOENT, leading
phy_attach_direct() to fatally abort the attachment and fail to bring up
the network interface.

Should this return 0 instead to allow non-switch PHYs to attach properly?

> +
> +	return otto_emdio_set_port_polling(priv, port, true);
> +}

[ ... ]

> @@ -611,6 +647,9 @@ static int otto_emdio_probe_one(struct device *dev, struct otto_emdio_priv *priv
>  		bus->write = otto_emdio_write_c22;

[Severity: High]
This is a pre-existing issue, but can the hardware polling engine corrupt
software C45-over-C22 MMD access sequences at runtime?

Since this patch re-enables hardware polling for a C22 PHY in
notify_phy_attach(), the unpaused hardware polling engine can interleave
with the 4-step software C45-over-C22 sequence used when phylib falls back
to standard C22 writes. This could overwrite the MMD address/data registers
(13 and 14).

Would it be safer to temporarily disable the hardware polling engine across
multi-register C45-over-C22 transactions in otto_emdio_write_c22() and
otto_emdio_read_c22() to prevent race conditions?

>  	}
>  	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://sashiko.dev/#/patchset/[email protected]?part=5