Re: [PATCH net-next v15 07/13] net: mdio: realtek-rtl9300: Configure hardware polling during probing

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Pre-existing bug in `phy_attach_direct`: the error path for an already-attached PHY inadvertently tears down the existing attachment.
--

commit b9cbeb0822e562618f953089bee5ff136bf2e472
Author: Markus Stockhausen <markus.stockhausen@gmx.de>

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

This commit synchronizes hardware polling with the kernel's MDIO driver by
disabling polling at driver start and just before PHY detach, and re-enabling
it after PHY attach. It introduces regmap bit access to handle these polling
state transitions and gracefully handles deferred probing of PHYs.

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 7de8adec243ba..a925640290cb0 100644
> --- a/drivers/net/mdio/mdio-realtek-rtl9300.c
> +++ b/drivers/net/mdio/mdio-realtek-rtl9300.c

[ ... ]

> @@ -625,6 +664,9 @@ static int otto_emdio_probe_one(struct device *dev, struct otto_emdio_priv *priv
>  		bus->write = otto_emdio_write_c22;
>  	}
>  	bus->parent = dev;
> +	bus->notify_phy_attach = otto_emdio_notify_phy_attach;
> +	bus->notify_phy_detach = otto_emdio_notify_phy_detach;
> +

[Severity: High]
This is a pre-existing issue, but while looking at how phy_attach_direct()
calls notify_phy_attach(), I noticed a bug in the error handling for
already-attached PHYs.

Does this error path inadvertently tear down an existing valid attachment?

If a second MAC driver attempts to attach to a PHY that is already attached to
a different interface, phy_attach_direct() catches the error and jumps to the
error label:

drivers/net/phy/phy_device.c:phy_attach_direct() {
    ...
	if (phydev->attached_dev) {
		dev_err(&dev->dev, "PHY already attached\n");
		err = -EBUSY;
		goto error;
	}
    ...
error:
	/* phy_detach_internal() does all of the cleanup below */
	phy_detach_internal(phydev, false);
	return err;
}

This unconditional cleanup calls phy_detach_internal(), which nullifies the
existing pointers:

drivers/net/phy/phy_device.c:phy_detach_internal() {
    ...
		phydev->attached_dev->phydev = NULL;
		phydev->attached_dev = NULL;
		phy_link_topo_del_phy(dev, phydev);
    ...
}

Will this cause a NULL pointer dereference or use-after-free in the original
MAC driver when it next tries to access the PHY, as its phydev pointer has been
unexpectedly cleared?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831143439.2404484-1-markus.stockhausen@gmx.de?part=7