Re: [PATCH net-next v3 4/8] 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:
- [Medium] Data race due to unprotected lockless read of `priv->phy_poll`.
--

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

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

This commit synchronizes kernel and hardware polling for Realtek Otto switches
by disabling polling for all ports on start and managing it per-PHY during
attach and detach. This prevents hardware polling from interfering with
complex configuration sequences or firmware loading.

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

[ ... ]

> @@ -576,6 +588,47 @@ 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);
> +	int ret;
> +
> +	if (port < 0)
> +		return port;
> +
> +	if (test_bit(port, priv->phy_poll))
> +		return 0;

[Severity: Medium]
Could this lockless read of priv->phy_poll result in a data race?

While this test_bit() is lockless, the updates in the guarded sections below
use the non-atomic __set_bit() and __clear_bit(). If multiple PHYs on the
same MDIO bus are attached or detached concurrently, the plain
read-modify-write from __set_bit() or __clear_bit() on the same word could
race with this read, causing KCSAN warnings and undefined behavior.

Should this test_bit() check be moved inside the scoped_guard() block, which
safely supports early returns?

> +
> +	scoped_guard(mutex, &priv->lock) {
> +		ret = otto_emdio_set_port_polling(priv, port, true);
> +		if (!ret)
> +			__set_bit(port, priv->phy_poll);
> +	}
> +
> +	return ret;
> +}
> +
> +static void otto_emdio_notify_phy_detach(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);
> +	struct mii_bus *bus = phydev->mdio.bus;
> +	int ret;
> +
> +	if (port < 0)
> +		return;
> +
> +	scoped_guard(mutex, &priv->lock) {
> +		ret = otto_emdio_set_port_polling(priv, port, false);
> +		if (!ret)
> +			__clear_bit(port, priv->phy_poll);
> +	}
> +	
> +	if (ret)
> +		dev_err(bus->parent, "failed to disable polling for port %d\n", port);
> +}

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