[PATCH v2] net: fec: fix pinctrl default state restore order on resume

[PATCH v2] net: fec: fix pinctrl default state restore order on resume

[Tapio Reijonen]

In fec_resume(), fec_enet_clk_enable() is called before
pinctrl_pm_select_default_state() in the non-WoL path, inverting the
ordering used in fec_suspend() which correctly switches to the sleep
pinctrl state before disabling clocks.

For PHYs with the PHY_RST_AFTER_CLK_EN flag (e.g. TI DP83848 or
SMSC LAN87xx), fec_enet_clk_enable() triggers a hardware reset pulse
via the phy-reset GPIO. With the GPIO pin still in sleep pinctrl state
at that point, the GPIO write has no physical effect and the PHY never
receives the required reset after clock enable, leading to unreliable
link establishment after system resume.

Fix by restoring the default pinctrl state before enabling clocks in the
non-WoL resume path, making resume the proper mirror of suspend.

Fixes: de40ed31b3c5 ("net: fec: add Wake-on-LAN support")
Signed-off-by: Tapio Reijonen <tapio.reijonen@vaisala.com>
---
Changes in v2:
- Remove a stray blank line between the Fixes: tag and Signed-off-by:
  in the commit message; no functional change (per Wei Fang's review).
- Link to v1: https://lore.kernel.org/r/20260526-b4-fec-resume-pinctrl-order-v1-1-f2f926325424@vaisala.com
---
 drivers/net/ethernet/freescale/fec_main.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index f89aa94ce0202d5f28f37362ce70e0943aa14025..723af4c057d7aeacb1e90301d95da52b79264400 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -5594,12 +5594,12 @@ static int fec_resume(struct device *dev)
 		if (fep->rpm_active)
 			pm_runtime_force_resume(dev);
 
-		ret = fec_enet_clk_enable(ndev, true);
-		if (ret) {
-			rtnl_unlock();
-			goto failed_clk;
-		}
 		if (fep->wol_flag & FEC_WOL_FLAG_ENABLE) {
+			ret = fec_enet_clk_enable(ndev, true);
+			if (ret) {
+				rtnl_unlock();
+				goto failed_clk;
+			}
 			fec_enet_stop_mode(fep, false);
 			if (fep->wake_irq) {
 				disable_irq_wake(fep->wake_irq);
@@ -5612,6 +5612,11 @@ static int fec_resume(struct device *dev)
 			fep->wol_flag &= ~FEC_WOL_FLAG_SLEEP_ON;
 		} else {
 			pinctrl_pm_select_default_state(&fep->pdev->dev);
+			ret = fec_enet_clk_enable(ndev, true);
+			if (ret) {
+				rtnl_unlock();
+				goto failed_clk;
+			}
 		}
 		fec_restart(ndev);
 		netif_tx_lock_bh(ndev);

---
base-commit: 79bd2dded182b1d458b18e62684b7f82ffc682e5
change-id: 20260526-b4-fec-resume-pinctrl-order-fde0cff2bbff

Best regards,
-- 
Tapio Reijonen <tapio.reijonen@vaisala.com>

RE: [PATCH v2] net: fec: fix pinctrl default state restore order on resume

[Wei Fang]

Please wait at least 24 hours before sending a revised version. Give reviewers
enough time to provide feedback.
See https://elixir.bootlin.com/linux/v7.1-rc5/source/Documentation/process/maintainer-netdev.rst#L15

> @@ -5594,12 +5594,12 @@ static int fec_resume(struct device *dev)
>                 if (fep->rpm_active)
>                         pm_runtime_force_resume(dev);
> 
> -               ret = fec_enet_clk_enable(ndev, true);
> -               if (ret) {
> -                       rtnl_unlock();
> -                       goto failed_clk;
> -               }
>                 if (fep->wol_flag & FEC_WOL_FLAG_ENABLE) {
> +                       ret = fec_enet_clk_enable(ndev, true);
> +                       if (ret) {
> +                               rtnl_unlock();
> +                               goto failed_clk;
> +                       }
>                         fec_enet_stop_mode(fep, false);
>                         if (fep->wake_irq) {
>                                 disable_irq_wake(fep->wake_irq);
> @@ -5612,6 +5612,11 @@ static int fec_resume(struct device *dev)
>                         fep->wol_flag &= ~FEC_WOL_FLAG_SLEEP_ON;
>                 } else {
> 
> pinctrl_pm_select_default_state(&fep->pdev->dev);
> +                       ret = fec_enet_clk_enable(ndev, true);
> +                       if (ret) {
> +                               rtnl_unlock();
> +                               goto failed_clk;
> +                       }
>                 }

I think fec_enet_clk_enable() can be moved before fec_restart(), and there is
no need to call fec_enet_clk_enable() separately in the if...else... branches.

Or move pinctrl_pm_select_default_state() before fec_enet_clk_enable(),
as shown below:

--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -5644,6 +5644,7 @@ static int fec_resume(struct device *dev)
                if (fep->rpm_active)
                        pm_runtime_force_resume(dev);

+               pinctrl_pm_select_default_state(&fep->pdev->dev);
                ret = fec_enet_clk_enable(ndev, true);
                if (ret) {
                        rtnl_unlock();

>                 fec_restart(ndev);
>                 netif_tx_lock_bh(ndev);

Re: [PATCH v2] net: fec: fix pinctrl default state restore order on resume

[Tapio Reijonen]

Hi,

On 5/27/26 10:47, Wei Fang wrote:
> Please wait at least 24 hours before sending a revised version. Give reviewers
> enough time to provide feedback.
> See https://elixir.bootlin.com/linux/v7.1-rc5/source/Documentation/process/maintainer-netdev.rst#L15
> 
>> @@ -5594,12 +5594,12 @@ static int fec_resume(struct device *dev)
>>                  if (fep->rpm_active)
>>                          pm_runtime_force_resume(dev);
>>
>> -               ret = fec_enet_clk_enable(ndev, true);
>> -               if (ret) {
>> -                       rtnl_unlock();
>> -                       goto failed_clk;
>> -               }
>>                  if (fep->wol_flag & FEC_WOL_FLAG_ENABLE) {
>> +                       ret = fec_enet_clk_enable(ndev, true);
>> +                       if (ret) {
>> +                               rtnl_unlock();
>> +                               goto failed_clk;
>> +                       }
>>                          fec_enet_stop_mode(fep, false);
>>                          if (fep->wake_irq) {
>>                                  disable_irq_wake(fep->wake_irq);
>> @@ -5612,6 +5612,11 @@ static int fec_resume(struct device *dev)
>>                          fep->wol_flag &= ~FEC_WOL_FLAG_SLEEP_ON;
>>                  } else {
>>
>> pinctrl_pm_select_default_state(&fep->pdev->dev);
>> +                       ret = fec_enet_clk_enable(ndev, true);
>> +                       if (ret) {
>> +                               rtnl_unlock();
>> +                               goto failed_clk;
>> +                       }
>>                  }
> 
> I think fec_enet_clk_enable() can be moved before fec_restart(), and there is
> no need to call fec_enet_clk_enable() separately in the if...else... branches.
> 
> Or move pinctrl_pm_select_default_state() before fec_enet_clk_enable(),
> as shown below:
> 
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -5644,6 +5644,7 @@ static int fec_resume(struct device *dev)
>                  if (fep->rpm_active)
>                          pm_runtime_force_resume(dev);
> 
> +               pinctrl_pm_select_default_state(&fep->pdev->dev);
>                  ret = fec_enet_clk_enable(ndev, true);
>                  if (ret) {
>                          rtnl_unlock();
> 
>>                  fec_restart(ndev);
>>                  netif_tx_lock_bh(ndev);
> 
Agreed, the second form is cleaner -- it keeps the fix to a one-line 
move of pinctrl_pm_select_default_state() and drops the branch 
duplication I had in v2.


Calling it unconditionally is safe for the WoL path as well: 
fec_suspend() only selects the sleep pinctrl state on the non-WoL path 
and leaves the pins in the default state when WoL is enabled, so on a 
WoL resume the device is already in the default state and the call is a 
no-op.


I'll send v3 with this shape.


Many thanks for the review,
Tapio Reijonen

Re: [PATCH v2] net: fec: fix pinctrl default state restore order on resume

[sashiko-bot]

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

New issues:
- [Medium] The pinctrl state is not reverted to the sleep state if `fec_enet_clk_enable()` fails during the non-WoL resume path.

Pre-existing issues:
- [High] Unbalanced `regulator_disable()` in the error path of `fec_resume()` when Wake-on-LAN (WoL) is enabled.
- [High] The return value of `pm_runtime_force_resume()` is ignored, which can lead to system crashes if clock enablement fails.
--

commit 31651e9d0db1a567cb015eadbc480a45cac209ed
Author: Tapio Reijonen <tapio.reijonen@vaisala.com>

net: fec: fix pinctrl default state restore order on resume

In fec_resume(), fec_enet_clk_enable() is called before
pinctrl_pm_select_default_state() in the non-WoL path, inverting the
ordering used in fec_suspend() which correctly switches to the sleep
pinctrl state before disabling clocks.

Fixes: de40ed31b3c5 ("net: fec: add Wake-on-LAN support")
Signed-off-by: Tapio Reijonen <tapio.reijonen@vaisala.com>

> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index f89aa94ce0202..723af4c057d7a 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -5594,12 +5594,12 @@ static int fec_resume(struct device *dev)
>  		if (fep->rpm_active)
>  			pm_runtime_force_resume(dev);

[Severity: High]
This is a pre-existing issue, but should the return value of
pm_runtime_force_resume() be checked here?

If it fails, it returns a negative error code, but the result is ignored
and execution proceeds to fec_restart(). On i.MX platforms, accessing
memory-mapped registers without their clocks enabled causes a precise or
imprecise external abort, leading to a kernel panic.

>  
> -		ret = fec_enet_clk_enable(ndev, true);
> -		if (ret) {
> -			rtnl_unlock();
> -			goto failed_clk;
> -		}
>  		if (fep->wol_flag & FEC_WOL_FLAG_ENABLE) {
> +			ret = fec_enet_clk_enable(ndev, true);
> +			if (ret) {
> +				rtnl_unlock();
> +				goto failed_clk;

[Severity: High]
This isn't a bug introduced by this patch, but will this error path cause
an unbalanced regulator_disable() if WoL is enabled?

At the start of fec_resume(), regulator_enable() is conditionally executed
only when WoL is NOT enabled:

	if (fep->reg_phy && !(fep->wol_flag & FEC_WOL_FLAG_ENABLE)) {
		ret = regulator_enable(fep->reg_phy);

However, if fec_enet_clk_enable() fails here, we jump to failed_clk which
unconditionally disables the regulator:

failed_clk:
	if (fep->reg_phy)
		regulator_disable(fep->reg_phy);

Since we are in the WoL enabled path, the regulator was never enabled. This
would incorrectly decrement the use count and trigger a WARN_ON in the
regulator core, potentially powering off a shared regulator prematurely.

> +			}
>  			fec_enet_stop_mode(fep, false);
>  			if (fep->wake_irq) {

[ ... ]

> @@ -5612,6 +5612,11 @@ static int fec_resume(struct device *dev)
>  			fep->wol_flag &= ~FEC_WOL_FLAG_SLEEP_ON;
>  		} else {
>  			pinctrl_pm_select_default_state(&fep->pdev->dev);
> +			ret = fec_enet_clk_enable(ndev, true);
> +			if (ret) {
> +				rtnl_unlock();
> +				goto failed_clk;

[Severity: Medium]
If fec_enet_clk_enable() fails here, should the pinctrl state be reverted
back to the sleep state?

The code jumps to failed_clk without reverting the state, leaving the pins
in the active state even though the device resume is aborted.

> +			}
>  		}
>  		fec_restart(ndev);
>  		netif_tx_lock_bh(ndev);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260527-b4-fec-resume-pinctrl-order-v2-1-293096f48703@vaisala.com?part=1