Re: [PATCH net-next v2 4/8] net: mdio: realtek-rtl9300: Configure hardware polling during probing

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The `init_polling` callback signature is missing a context pointer, preventing it from accessing hardware registers.
- [Medium] Calling `tune_polling` and `init_polling` while holding `priv->lock` risks a recursive mutex deadlock.
--

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

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. E.g. a hardware polling MMD c45 over c22
request might break an ongoing firmware loading sequence.

[1] https://github.com/openwrt/openwrt/blob/main/target/linux/realtek/files-6.18/drivers/net/mdio/mdio-realtek-otto.c#L818
[2] 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 616edcde15d95..a8e9a497a0dc5 100644
> --- a/drivers/net/mdio/mdio-realtek-rtl9300.c
> +++ b/drivers/net/mdio/mdio-realtek-rtl9300.c

[ ... ]

> @@ -210,6 +213,9 @@ struct otto_emdio_info {
>  	u8 num_buses;
>  	u8 num_ports;
>  	u16 num_pages;
> +	u32 poll_ctrl;
> +	int (*init_polling)(int port);

[Severity: Medium]
Will future implementations of this callback need access to the driver's
private data?

The init_polling() callback only takes an integer port parameter, meaning any
implementation won't have access to the driver's private data context (priv)
or regmap. Will this prevent it from actually interacting with the hardware
registers?

> +	int (*tune_polling)(struct phy_device *phydev);
>  	int (*setup_controller)(struct otto_emdio_priv *priv);
>  	int (*read_c22)(struct mii_bus *bus, int port, int regnum, u32 *value);
>  	int (*read_c45)(struct mii_bus *bus, int port, int dev_addr, int regnum, u32 *value);

[ ... ]

> @@ -588,6 +602,49 @@ 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;
> +
> +	scoped_guard(mutex, &priv->lock) {
> +		if (priv->info->tune_polling) {
> +			ret = priv->info->tune_polling(phydev);

[Severity: Medium]
Does invoking tune_polling() while holding priv->lock risk a recursive mutex
deadlock?

In otto_emdio_notify_phy_attach(), priv->lock is acquired via scoped_guard.
If a future implementation of tune_polling() calls standard PHY APIs like
phy_read(), the call would eventually route down to otto_emdio_read_c22(),
which attempts to blindly acquire priv->lock again, potentially deadlocking
the thread.

> +			if (ret)
> +				return ret;
> +		}
> +
> +		ret = otto_emdio_set_port_polling(priv, port, true);
> +		if (!ret)
> +			__set_bit(port, priv->phy_poll);
> +	}
> +
> +	return ret;
> +}

[ ... ]

> @@ -733,6 +793,27 @@ static int otto_emdio_map_ports(struct device *dev)
>  	return err;
>  }
>  
> +static int otto_emdio_init_polling(struct otto_emdio_priv *priv)
> +{
> +	int err;
> +
> +	scoped_guard(mutex, &priv->lock) {
> +		for (int port = 0; port < priv->info->num_ports; port++) {
> +			err = otto_emdio_set_port_polling(priv, port, false);
> +			if (err)
> +				return err;
> +
> +			if (priv->info->init_polling) {
> +				err = priv->info->init_polling(port);

[Severity: Medium]
Does invoking init_polling() while holding priv->lock also risk a recursive
mutex deadlock, similar to tune_polling() above?

> +				if (err)
> +					return err;
> +			}
> +		}
> +	}
> +
> +	return 0;
> +}

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