Re: [PATCH net v2] net: stmmac: request the MDIO reset GPIO only once

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 2

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] Guard mismatch between where the MDIO reset GPIO is acquired
  and where it is used: stmmac_mdio_register() acquires the descriptor…
- [Low] The new devm_gpiod_get_optional() call is placed before the
  driver learns that the DT MDIO node is disabled, and is not…
- [Low] The new devm_gpiod_get_optional() in stmmac_mdio_register()
  (stmmac_mdio.c:604-609) is not wrapped in `#if…

--

net: stmmac: request the MDIO reset GPIO only once

This moves the "snps,reset" GPIO lookup out of stmmac_mdio_reset() and
into stmmac_mdio_register(), caching the descriptor in
struct stmmac_priv::mdio_reset_gpio.  stmmac_mdio_reset() now only
pulses the cached line, so repeated calls no longer fail with -EBUSY
from a second devm_gpiod_get_optional().

A few questions about the new placement of the request.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
> index afe98ff5bdcb..346f93f86abe 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
> @@ -386,15 +386,8 @@ int stmmac_mdio_reset(struct mii_bus *bus)
>  
>  #ifdef CONFIG_OF
>  	if (priv->device->of_node) {
> -		struct gpio_desc *reset_gpio;
>  		u32 delays[3] = { 0, 0, 0 };
>  
> -		reset_gpio = devm_gpiod_get_optional(priv->device,
> -						     "snps,reset",
> -						     GPIOD_OUT_LOW);
> -		if (IS_ERR(reset_gpio))
> -			return PTR_ERR(reset_gpio);
> -
>  		device_property_read_u32_array(priv->device,
>  					       "snps,reset-delays-us",
>  					       delays, ARRAY_SIZE(delays));
> @@ -402,11 +395,11 @@ int stmmac_mdio_reset(struct mii_bus *bus)
>  		if (delays[0])
>  			msleep(DIV_ROUND_UP(delays[0], 1000));
>  
> -		gpiod_set_value_cansleep(reset_gpio, 1);
> +		gpiod_set_value_cansleep(priv->mdio_reset_gpio, 1);
>  		if (delays[1])
>  			msleep(DIV_ROUND_UP(delays[1], 1000));
>  
> -		gpiod_set_value_cansleep(reset_gpio, 0);
> +		gpiod_set_value_cansleep(priv->mdio_reset_gpio, 0);
>  		if (delays[2])
>  			msleep(DIV_ROUND_UP(delays[2], 1000));
>  	}

[Severity: Medium]
Is the pulse site still gated on the same condition as the new
acquisition site?

Here in stmmac_mdio_reset() the only guard is CONFIG_OF plus
priv->device->of_node, while the request added in stmmac_mdio_register()
is additionally gated on mdio_bus_data->needs_reset.  For a DT device
where needs_reset is false, priv->mdio_reset_gpio stays NULL, so both
gpiod_set_value_cansleep() calls are no-ops, the delays are still slept,
and the function returns 0.

The caller that the commit message identifies as the victim does not test
needs_reset either:

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:stmmac_resume() {
	...
		/* reset the phy so that it's ready */
		if (priv->mii)
			stmmac_mdio_reset(priv->mii);
	...
}

needs_reset == false together with an of_node is produced in-tree, for
example:

drivers/net/ethernet/stmicro/stmmac/dwmac-dwc-qos-eth.c:tegra_eqos_probe() {
	/* MDIO bus was already reset just above */
	plat_dat->mdio_bus_data->needs_reset = false;
}

drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c:loongson_dwmac_dt_config() {
	plat->mdio_node = of_get_child_by_name(np, "mdio");
	if (plat->mdio_node) {
		dev_info(&pdev->dev, "Found MDIO subnode\n");
		plat->mdio_bus_data->needs_reset = true;
	}
}

In the loongson case needs_reset stays false when there is no "mdio"
subnode, while the device still has an of_node.

On such a configuration, before this patch the first stmmac_mdio_reset()
call did request the line and drive the pulse; afterwards the pulse never
reaches the wire.  Would gating the acquisition on exactly the same
condition as the use site (or not gating it on needs_reset at all) avoid
that divergence?

Related, the commit message says:

    "The request is still gated on mdio_bus_data->needs_reset, which is
    the condition that installs the reset callback, and on the device
    using DT, as the reset itself is."

Is that accurate for the stmmac_resume() path, which calls
stmmac_mdio_reset() regardless of needs_reset?

For what it is worth, no in-tree DT was found that combines
needs_reset == false with "snps,reset-gpio" (all in-tree users of that
property go through stmmac_platform.c, which sets needs_reset = true), so
the lost pulse depends on a firmware or vendor DT, such as the
firmware-provided Loongson one.

> @@ -608,6 +601,14 @@ int stmmac_mdio_register(struct net_device *ndev)
>  	if (!mdio_bus_data)
>  		return 0;
>  
> +	if (mdio_bus_data->needs_reset && dev_of_node(priv->device)) {
> +		priv->mdio_reset_gpio =
> +			devm_gpiod_get_optional(priv->device, "snps,reset",
> +						GPIOD_OUT_LOW);
> +		if (IS_ERR(priv->mdio_reset_gpio))
> +			return PTR_ERR(priv->mdio_reset_gpio);
> +	}
> +

[Severity: Low]
Can this request run for an MDIO bus that is disabled in DT and will
never be registered?

stmmac_of_get_mdio() does not filter unavailable nodes in either branch:

drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c:stmmac_of_get_mdio() {
	if (of_match_node(need_mdio_ids, np)) {
		mdio_node = of_get_child_by_name(np, "mdio");
	} else {
		for_each_child_of_node(np, mdio_node) {
			if (of_device_is_compatible(mdio_node,
						    "snps,dwmac-mdio"))
				break;
	...
}

so a disabled child still reaches stmmac_mdio_setup(), which sets
plat->mdio_bus_data->needs_reset = true.  The MDIO core then rejects it
before the reset callback can ever be installed:

drivers/net/mdio/of_mdio.c:__of_mdiobus_register() {
	/* Do not continue if the node is disabled */
	if (!of_device_is_available(np))
		return -ENODEV;
}

and stmmac_mdio_register() turns that into success:

	err = of_mdiobus_register(new_bus, mdio_node);
	if (err == -ENODEV) {
		err = 0;
		dev_info(dev, "MDIO bus is disabled\n");
		goto bus_register_fail;

Before this patch the lookup only happened from the reset callback, so
nothing was requested in that configuration.  With the request moved
here, three things change for such a board:

- a lookup error (-EPROBE_DEFER while the gpiochip is not yet present,
  -EBUSY from another consumer, or anything else) is returned from
  stmmac_mdio_register() and aborts or defers the whole MAC probe, for a
  reset that can never run
- on success the devm descriptor holds the line for the device lifetime
  with no user at all, since probe succeeds and devres cleanup does not
  run
- GPIOD_OUT_LOW drives the reset signal to its inactive level for a bus
  DT asked not to be used

Would requesting the GPIO only after a bus has actually been registered,
or skipping the request when the MDIO node is present but unavailable,
be preferable?

No in-tree DT was found that declares "snps,reset-gpio" on the MAC node
and marks its MDIO child disabled, so the probe-abort and early
de-assertion parts are not demonstrated on an in-tree board; the
reserved-but-unused line follows directly from the code.

[Severity: Low]
Should this request be inside the same conditional compilation block as
its only consumer?

stmmac_mdio_reset() wraps its whole body, including both
gpiod_set_value_cansleep(priv->mdio_reset_gpio, ...) calls, in:

	int stmmac_mdio_reset(struct mii_bus *bus)
	{
	#if IS_ENABLED(CONFIG_STMMAC_PLATFORM)

so with CONFIG_STMMAC_PLATFORM=n the function reduces to return 0, and
priv->mdio_reset_gpio has no reader, while the new request above is
compiled unconditionally.

That combination is buildable: DWMAC_LOONGSON, DWMAC_MOTORCOMM and
STMMAC_PCI sit after "endif # STMMAC_PLATFORM" in
drivers/net/ethernet/stmicro/stmmac/Kconfig, and dwmac-loongson.c
includes only stmmac.h and stmmac_libpci.h, so it links without
stmmac_platform.o while still setting needs_reset = true on a device
with a firmware-provided of_node:

drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c:loongson_dwmac_dt_config() {
	plat->mdio_bus_data->needs_reset = true;
}

In that build the "snps,reset" line is claimed for the device lifetime
while the code that would pulse it does not exist, and a transient
lookup failure aborts probe.  Before the patch acquisition and use were
always compiled together.  Putting the request under the same
IS_ENABLED(CONFIG_STMMAC_PLATFORM) guard, or dropping the guard from
stmmac_mdio_reset(), would keep them paired.

No in-tree Loongson DT carries snps,reset-gpio, so this has no observed
effect on in-tree boards.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260920054703.1897755-1-xiaolinkui%40126.com