diff mbox series

[net,v4,02/15] net: stmmac: reuse the MDIO reset GPIO on resume

Message ID 20260926-submit-stmmac-reset-fixes-v1-v4-2-ec1c0250b3c9@gmail.com (mailing list archive)
State New
Headers
Series net: stmmac: preserve datapath state across MTU and resume failures |

Commit Message

James Hilliard Sept. 26, 2026, 3:48 p.m. UTC 
MDIO bus registration acquires the optional snps,reset GPIO through
devres. A later bus reset must reuse that descriptor: requesting the
same exclusive GPIO again returns -EBUSY while devres still owns it.
Cache the successfully acquired descriptor for subsequent resets.
Propagate acquisition errors without caching an error pointer.

Fixes: 7c86f20d15b7 ("net: stmmac: use GPIO descriptors in stmmac_mdio_reset")
Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac.h      |  1 +
 drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c | 18 ++++++++++++------
 2 files changed, 13 insertions(+), 6 deletions(-)

Comments

netdev-bot+sashiko@kernel.org Sept. 30, 2026, 4:51 a.m. UTC | #1 
Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

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

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).

- [High] Once the cached descriptor is used, stmmac_mdio_reset() really
  does pulse the snps,reset PHY reset line on every system resume…
- [Low] The commit message explains the mechanism (the second exclusive
  request of snps,reset returns -EBUSY) but not what users see.

--

net: stmmac: reuse the MDIO reset GPIO on resume

MDIO bus registration acquires the snps,reset GPIO. This commit caches
that descriptor in priv->mdio_reset_gpio, so later calls to
stmmac_mdio_reset(), such as the one on resume, reuse it. Without the
cache, those calls request the exclusive line again and fail with
-EBUSY.

> MDIO bus registration acquires the optional snps,reset GPIO through
> devres. A later bus reset must reuse that descriptor: requesting the
> same exclusive GPIO again returns -EBUSY while devres still owns it.

[Severity: Low]

This isn't a bug, but could the commit message describe the effect that
users see?

stmmac_resume() ignores the return value of stmmac_mdio_reset(). Before
this patch, on OF boards with a snps,reset GPIO, every resume without
MAC WoL silently skipped two things: the PHY reset pulse and the
STE101P dummy MDC write:

    writel(0, priv->ioaddr + mii_address);

Nothing was logged when this happened.

The message also doesn't say that this change turns the PHY hard reset
on resume back on. That reset has not run since 7c86f20d15b7. Without a
symptom or behaviour change in the message, stable backporters
following the Fixes: tag have nothing to match against bug reports.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
> index afe98ff5bdcb0..07d5eab05996b 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
> @@ -386,14 +386,20 @@ int stmmac_mdio_reset(struct mii_bus *bus)
>  
>  #ifdef CONFIG_OF
>  	if (priv->device->of_node) {
> -		struct gpio_desc *reset_gpio;
> +		struct gpio_desc *reset_gpio = priv->mdio_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);
> +		/* The bus reset also runs on resume. Devres retains the GPIO
> +		 * until unbind, so do not request an already owned line again.
> +		 */
> +		if (!reset_gpio) {
> +			reset_gpio = devm_gpiod_get_optional(priv->device,
> +							     "snps,reset",
> +							     GPIOD_OUT_LOW);
> +			if (IS_ERR(reset_gpio))
> +				return PTR_ERR(reset_gpio);
> +			priv->mdio_reset_gpio = reset_gpio;
> +		}

[Severity: High]

With the descriptor cached, stmmac_mdio_reset() now really pulses the
snps,reset line on every system resume where MAC WoL is off. Does
anything re-initialise the PHY after that?

Before this patch, the second devm_gpiod_get_optional() on the
exclusive line failed in gpiod_request_commit() with -EBUSY. The
resume path ignores that error:

stmmac_resume()
    } else {
        pinctrl_pm_select_default_state(priv->device);
        /* reset the phy so that it's ready */
        if (priv->mii)
            stmmac_mdio_reset(priv->mii);
    }

So no PHY hard reset has run on resume since 7c86f20d15b7. Before that
commit the reset did run, but mdio_bus_phy_resume()->phy_init_hw()
followed it.

Today stmmac_phylink_setup() sets config->mac_managed_pm = true, and
phylink copies that into phydev->mac_managed_pm. As a result,
mdio_bus_phy_resume() returns early:

mdio_bus_phy_resume()
    if (phydev->mac_managed_pm)
        return 0;
    ...
    ret = phy_init_hw(phydev);

The MAC-managed path only runs phylink_prepare_resume()->phy_resume()
and phylink_resume()->phylink_start()->phy_start()->__phy_resume().
Neither of these replays config_init() or config_intr().

Wouldn't that leave the PHY at its power-on or strap defaults while
phylib believes it is still configured? The config_init() settings
would be lost: RGMII delays or skews, clock-out, LEDs, vendor quirks,
and the autonomous EEE disable.

For an interrupt-driven PHY, the reset also clears the interrupt
enable register. phy_start() restarts aneg, sees link down and moves
to PHY_NOLINK, and it does not poll after that. Could link-up then
never be reported, so the interface stays down until the admin runs a
down/up cycle?

PHY-based WoL seems to be affected too. phylink_ethtool_set_wol()
removes the modes the PHY handles from the MAC mask:

    wolopts &= ~w.wolopts;

With PHY WoL, priv->wolopts is therefore 0, so resume takes the reset
branch. Won't the reset pulse erase the PHY's WoL enables, so the next
suspend is not armed for wake?

Resume now also blocks for the snps,reset-delays-us msleep() calls.
When those delays are 0, the DMA reset in stmmac_hw_setup() needs the
PHY RX clock, and it runs right after reset deassert.

This also looks unresolved at the end of the series. After "net:
stmmac: track datapath and power ownership across failed reopening",
stmmac_resume_hw() still calls stmmac_mdio_reset(). stmmac_resume()
still only calls phylink_prepare_resume() and phylink_resume() after
it, and no phy_init_hw() call was added in stmmac or phylink.

This patch carries a Fixes: tag, so it will reach stable. Should the
resume path either re-initialise the PHY after the reset, or skip the
reset pulse on resume?

[ ... ]
diff mbox series

Patch

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
index 7582fca63741..c7c876408d85 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
@@ -287,6 +287,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..07d5eab05996 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
@@ -386,14 +386,20 @@  int stmmac_mdio_reset(struct mii_bus *bus)
 
 #ifdef CONFIG_OF
 	if (priv->device->of_node) {
-		struct gpio_desc *reset_gpio;
+		struct gpio_desc *reset_gpio = priv->mdio_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);
+		/* The bus reset also runs on resume. Devres retains the GPIO
+		 * until unbind, so do not request an already owned line again.
+		 */
+		if (!reset_gpio) {
+			reset_gpio = devm_gpiod_get_optional(priv->device,
+							     "snps,reset",
+							     GPIOD_OUT_LOW);
+			if (IS_ERR(reset_gpio))
+				return PTR_ERR(reset_gpio);
+			priv->mdio_reset_gpio = reset_gpio;
+		}
 
 		device_property_read_u32_array(priv->device,
 					       "snps,reset-delays-us",