* Re: [PATCH net v2] net: stmmac: request the MDIO reset GPIO only once
2026-09-20 5:47 [PATCH net v2] net: stmmac: request the MDIO reset GPIO only once Linkui Xiao
@ 2026-09-20 8:07 ` Lorenzo Bianconi
2026-09-21 1:39 ` Linkui Xiao
2026-09-21 5:49 ` netdev-bot+sashiko
1 sibling, 1 reply; 4+ messages in thread
From: Lorenzo Bianconi @ 2026-09-20 8:07 UTC (permalink / raw)
To: Linkui Xiao
Cc: maxime.chevallier, andrew+netdev, davem, edumazet, kuba, pabeni,
mcoquelin.stm32, alexandre.torgue, netdev, linux-stm32,
linux-arm-kernel, linux-kernel, Linkui Xiao, stable
[-- Attachment #1: Type: text/plain, Size: 5005 bytes --]
> From: Linkui Xiao <xiaolinkui@kylinos.cn>
>
> stmmac_mdio_reset() calls devm_gpiod_get_optional() every time it runs.
> A GPIO line can only be requested once, so from the second call on
> gpiod_request_commit() returns -EBUSY. devm_gpiod_get_optional() only
> turns -ENOENT into NULL, hence the error is passed straight back and
> stmmac_mdio_reset() bails out before pulsing "snps,reset" and before
> running the STE101P MDC workaround.
>
> The first call, made by of_mdiobus_register(), succeeds, so the failure
> is only visible later on: every resume that does not use WoL goes
> through stmmac_resume() -> stmmac_mdio_reset(), and that caller ignores
> the return value, so the PHY silently stays un-reset.
>
> The descriptor used to be requested exactly once: stmmac_mdio_reset()
> resolved "snps,reset-gpio" itself and cached the GPIO number in
> stmmac_mdio_bus_data::reset_gpio, and commit ae26c1c6cb9b ("stmmac: fix
> PHY reset during resume") relies on that cache to reuse the line on
> every call. commit 7c86f20d15b7 ("net: stmmac: use GPIO descriptors in
> stmmac_mdio_reset") replaced it with a devm_gpiod_get_optional() that
> caches nothing, so the request is repeated on every call and fails from
> the second one on.
>
> Request the GPIO in stmmac_mdio_register(), at probe time, and keep the
> descriptor in struct stmmac_priv. This is where devm-gpiod is meant to be
> used: the line is acquired with the device and released with it, and any
> failure to acquire it is reported during probe instead of being ignored
> by stmmac_resume(). stmmac_mdio_reset() then only pulses the cached line.
>
> 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.
>
> Fixes: 7c86f20d15b7 ("net: stmmac: use GPIO descriptors in stmmac_mdio_reset")
> Cc: stable@vger.kernel.org
> Signed-off-by: Linkui Xiao <xiaolinkui@kylinos.cn>
> ---
> Changes in v2:
> - Do not request the GPIO from stmmac_mdio_reset(); request it once in
> stmmac_mdio_register() at probe time instead. (Maxime Chevallier)
>
> drivers/net/ethernet/stmicro/stmmac/stmmac.h | 2 ++
> .../net/ethernet/stmicro/stmmac/stmmac_mdio.c | 19 ++++++++++---------
> 2 files changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> index 7582fca63741..986fb43db45f 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> @@ -25,6 +25,7 @@
> #include <net/xdp.h>
> #include <uapi/linux/bpf.h>
>
> +struct gpio_desc;
> struct stmmac_pcs;
>
> struct stmmac_resources {
> @@ -287,6 +288,7 @@ struct stmmac_priv {
>
> unsigned int pause_time;
> struct mii_bus *mii;
> + struct gpio_desc *mdio_reset_gpio;
>
> struct stmmac_pcs *integrated_pcs;
>
> 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));
nit: what about moving even these properties to stmmac_mdio_register()? It
seems a bit odd to have half of the parsing in stmmac_mdio_register() and
half in stmmac_mdio_reset(). What do you think?
Regards,
Lorenzo
> @@ -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));
> }
> @@ -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);
> + }
> +
> stmmac_mdio_bus_config(priv);
>
> new_bus = mdiobus_alloc();
> --
> 2.25.1
>
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH net v2] net: stmmac: request the MDIO reset GPIO only once
2026-09-20 5:47 [PATCH net v2] net: stmmac: request the MDIO reset GPIO only once Linkui Xiao
2026-09-20 8:07 ` Lorenzo Bianconi
@ 2026-09-21 5:49 ` netdev-bot+sashiko
1 sibling, 0 replies; 4+ messages in thread
From: netdev-bot+sashiko @ 2026-09-21 5:49 UTC (permalink / raw)
To: xiaolinkui
Cc: maxime.chevallier, andrew+netdev, davem, edumazet, kuba, pabeni,
mcoquelin.stm32, alexandre.torgue, netdev, linux-stm32,
linux-arm-kernel, linux-kernel, xiaolinkui, stable
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
^ permalink raw reply [flat|nested] 4+ messages in thread