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

[PATCH v3] 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,
making resume the proper mirror of suspend. The call is made
unconditionally: fec_suspend() only switches to 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 pinctrl_pm_select_default_state() is a no-op.

Fixes: de40ed31b3c5 ("net: fec: add Wake-on-LAN support")
Signed-off-by: Tapio Reijonen <tapio.reijonen@vaisala.com>
---
Changes in v3:
- Move pinctrl_pm_select_default_state() before fec_enet_clk_enable()
  unconditionally instead of duplicating the clk-enable call in both
  WoL branches; safe because fec_suspend() leaves the pins in the
  default state on the WoL path (per Wei Fang's review).
- Link to v2: https://lore.kernel.org/r/20260527-b4-fec-resume-pinctrl-order-v2-1-293096f48703@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 | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index f89aa94ce0202d5f28f37362ce70e0943aa14025..6ebde65d7f1b8728a6c5d28f75cc0b0c4d983969 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -5594,6 +5594,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();
@@ -5610,8 +5611,6 @@ static int fec_resume(struct device *dev)
 			val &= ~(FEC_ECR_MAGICEN | FEC_ECR_SLEEP);
 			writel(val, fep->hwp + FEC_ECNTRL);
 			fep->wol_flag &= ~FEC_WOL_FLAG_SLEEP_ON;
-		} else {
-			pinctrl_pm_select_default_state(&fep->pdev->dev);
 		}
 		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>

[PATCH v3] 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,
making resume the proper mirror of suspend. The call is made
unconditionally: fec_suspend() only switches to 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 pinctrl_pm_select_default_state() is a no-op.

Fixes: de40ed31b3c5 ("net: fec: add Wake-on-LAN support")
Signed-off-by: Tapio Reijonen <tapio.reijonen@vaisala.com>
---
Changes in v3:
- Move pinctrl_pm_select_default_state() before fec_enet_clk_enable()
  unconditionally instead of duplicating the clk-enable call in both
  WoL branches; safe because fec_suspend() leaves the pins in the
  default state on the WoL path (per Wei Fang's review).
- Link to v2: https://lore.kernel.org/r/20260527-b4-fec-resume-pinctrl-order-v2-1-293096f48703@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 | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index f89aa94ce0202d5f28f37362ce70e0943aa14025..6ebde65d7f1b8728a6c5d28f75cc0b0c4d983969 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -5594,6 +5594,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();
@@ -5610,8 +5611,6 @@ static int fec_resume(struct device *dev)
 			val &= ~(FEC_ECR_MAGICEN | FEC_ECR_SLEEP);
 			writel(val, fep->hwp + FEC_ECNTRL);
 			fep->wol_flag &= ~FEC_WOL_FLAG_SLEEP_ON;
-		} else {
-			pinctrl_pm_select_default_state(&fep->pdev->dev);
 		}
 		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 v3] net: fec: fix pinctrl default state restore order on resume

[Wei Fang]

>  drivers/net/ethernet/freescale/fec_main.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec_main.c
> b/drivers/net/ethernet/freescale/fec_main.c
> index
> f89aa94ce0202d5f28f37362ce70e0943aa14025..6ebde65d7f1b8728a6c5d28f7
> 5cc0b0c4d983969 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -5594,6 +5594,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();
> @@ -5610,8 +5611,6 @@ static int fec_resume(struct device *dev)
>  			val &= ~(FEC_ECR_MAGICEN | FEC_ECR_SLEEP);
>  			writel(val, fep->hwp + FEC_ECNTRL);
>  			fep->wol_flag &= ~FEC_WOL_FLAG_SLEEP_ON;
> -		} else {
> -			pinctrl_pm_select_default_state(&fep->pdev->dev);
>  		}
>  		fec_restart(ndev);
>  		netif_tx_lock_bh(ndev);
> 

It looks good to me, thanks for fixing this issue.

Reviewed-by: Wei Fang <wei.fang@nxp.com>

By the way, it seems you posted two identical patches, which might
cause confusion.

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

[Tapio Reijonen]

Hi Wei,

On 5/29/26 11:37, Wei Fang wrote:
>>   drivers/net/ethernet/freescale/fec_main.c | 3 +--
>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/freescale/fec_main.c
>> b/drivers/net/ethernet/freescale/fec_main.c
>> index
>> f89aa94ce0202d5f28f37362ce70e0943aa14025..6ebde65d7f1b8728a6c5d28f7
>> 5cc0b0c4d983969 100644
>> --- a/drivers/net/ethernet/freescale/fec_main.c
>> +++ b/drivers/net/ethernet/freescale/fec_main.c
>> @@ -5594,6 +5594,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();
>> @@ -5610,8 +5611,6 @@ static int fec_resume(struct device *dev)
>>   			val &= ~(FEC_ECR_MAGICEN | FEC_ECR_SLEEP);
>>   			writel(val, fep->hwp + FEC_ECNTRL);
>>   			fep->wol_flag &= ~FEC_WOL_FLAG_SLEEP_ON;
>> -		} else {
>> -			pinctrl_pm_select_default_state(&fep->pdev->dev);
>>   		}
>>   		fec_restart(ndev);
>>   		netif_tx_lock_bh(ndev);
>>
> 
> It looks good to me, thanks for fixing this issue.
> 
> Reviewed-by: Wei Fang <wei.fang@nxp.com>
> 
> By the way, it seems you posted two identical patches, which might
> cause confusion.
> 
Yes, that was my mistake -- sorry for the noise.
I aborted the first 'b4 sned' with Ctrl-C, but it looks like the message 
had already been handed off to the SMTP server by that point, so it went 
out anyway.
I'll be more careful next time.

Thanks for the review!

Best regards,
Tapio Reijonen

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

[sashiko-bot]

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

Pre-existing issues:
- [High] The return value of `pm_runtime_force_resume()` is ignored in `fec_resume()`, potentially leading to a synchronous external abort.
- [High] Unbalanced `regulator_disable()` in the `fec_resume()` error path when Wake-on-LAN (WoL) is enabled.
- [High] Race condition in `fec_drv_remove()` with `tx_timeout_work` leading to a Use-After-Free.
- [High] The error path in `fec_resume()` fails to revert the effects of `pm_runtime_force_resume()`, leaking clocks and PM state.
--

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

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

This commit changes the order in fec_resume() so that the default pinctrl
state is restored before enabling the clocks. This fixes an issue where PHYs
were not receiving a hardware reset pulse after clock enable because the
GPIO pin was still in sleep state.

> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index f89aa94ce0202..6ebde65d7f1b8 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -5594,6 +5594,7 @@ 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 the return value of
pm_runtime_force_resume() is ignored here. If this call fails (for example,
due to clock enablement failure), the bus clocks will remain disabled.

When fec_restart() is subsequently called, it directly accesses memory-mapped
hardware registers. Can accessing these registers without the bus clocks
enabled result in a synchronous external abort and a kernel panic?

> [ ... ]

While reviewing this code, I also noticed a few other pre-existing issues
related to the error path in fec_resume():

drivers/net/ethernet/freescale/fec_main.c:fec_resume() {
    ...
	if (fep->reg_phy && !(fep->wol_flag & FEC_WOL_FLAG_ENABLE)) {
		ret = regulator_enable(fep->reg_phy);
        ...
failed_clk:
	if (fep->reg_phy)
		regulator_disable(fep->reg_phy);
	return ret;
}

[Severity: High]
This isn't a bug introduced by this patch, but the error path appears to
have an unbalanced regulator_disable() call when Wake-on-LAN is enabled.

At the start of fec_resume(), the PHY regulator is only enabled if WoL is
NOT enabled. If a subsequent step like fec_enet_clk_enable() fails, the
code jumps to this failed_clk label and unconditionally disables the
regulator.

Does this corrupt the regulator's use count and potentially power off the
PHY unexpectedly?

[Severity: High]
This is another pre-existing issue in this same error path. If
pm_runtime_force_resume() succeeded earlier in the function, it
transitioned the device to RPM_ACTIVE and enabled the clocks.

If fec_enet_clk_enable() then fails, we return an error without calling
pm_runtime_force_suspend() or otherwise reverting the runtime PM state.

Does this leave the device stuck in an inconsistent active PM state with
leaked AHB/IPG clocks?


I also noticed an unrelated pre-existing issue in fec_drv_remove():

drivers/net/ethernet/freescale/fec_main.c:fec_drv_remove() {
    ...
	cancel_work_sync(&fep->tx_timeout_work);
	fec_ptp_stop(pdev);
	unregister_netdev(ndev);
    ...
}

[Severity: High]
This is a pre-existing issue, but calling cancel_work_sync() before
unregister_netdev() seems to create a race condition.

If a transmit timeout occurs immediately after this call but before
unregister_netdev() finishes taking the interface down, the networking
stack's watchdog timer will call the driver's ndo_tx_timeout callback,
which re-arms the work via schedule_work().

When the scheduled work eventually executes, it will access the freed
structures. Could this result in a Use-After-Free crash?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260529-b4-fec-resume-pinctrl-order-v3-1-56bb52b80ebf@vaisala.com?part=1

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

[sashiko-bot]

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

New issues:
- [Medium] The error path for `fec_enet_clk_enable()` in `fec_resume()` lacks a rollback for `pinctrl_pm_select_default_state()`, leaving pins active on failure.

Pre-existing issues:
- [High] The return value of `pm_runtime_force_resume(dev)` is ignored in `fec_resume()`, potentially leading to a kernel panic from unclocked register access.
- [High] Missing rollback and resource cleanup when `pm_runtime_force_suspend(dev)` fails in `fec_suspend()`.
- [High] Unbalanced `regulator_disable` in the `fec_resume()` error path if Wake-on-LAN is enabled.
- [High] Incorrect teardown order of `tx_timeout_work` in `fec_drv_remove()` leads to a potential Use-After-Free.
- [High] NAPI is enabled after hardware interrupts are re-enabled in `fec_resume()`, potentially causing lost interrupts.
--

commit 7f067374b27c3332158fb55d7eb7cfaf9e7c00c3
Author: Tapio Reijonen <tapio.reijonen@vaisala.com>

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

This commit reorders the pinctrl default state selection and clock enable
in fec_resume(). By restoring the default pinctrl state before enabling
clocks, it ensures that phy reset pulses have a physical effect and fixes
unreliable link establishment after system resume.

> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index f89aa94ce0202..6ebde65d7f1b8 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -5594,6 +5594,7 @@ 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 does ignoring the return value of
pm_runtime_force_resume() potentially lead to a kernel panic?

If fec_runtime_resume() is invoked and clk_prepare_enable() fails, an error
is returned, but it is ignored here. The code then proceeds to call
fec_restart(), which accesses hardware registers without the clocks enabled.
Could this cause a synchronous external abort?

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

[Severity: Medium]
If clock enablement fails, the function jumps to failed_clk and returns
without reverting the pinctrl state via pinctrl_pm_select_sleep_state().

Does this cause a state leak where the device pins remain active
indefinitely despite the resume failing?

[Severity: High]
This is a pre-existing issue, but is the regulator disable balanced in the
fec_resume() error path if Wake-on-LAN is enabled?

The phy regulator is only enabled if Wake-on-LAN is disabled:

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

However, if fec_enet_clk_enable() fails, the failed_clk error path
unconditionally calls regulator_disable(fep->reg_phy).

If Wake-on-LAN was enabled, could this cause a regulator reference count
underflow, unexpectedly powering off other devices sharing the same
regulator?

>  			rtnl_unlock();

[ ... ]

> @@ -5610,8 +5611,6 @@ static int fec_resume(struct device *dev)
>  			val &= ~(FEC_ECR_MAGICEN | FEC_ECR_SLEEP);
>  			writel(val, fep->hwp + FEC_ECNTRL);
>  			fep->wol_flag &= ~FEC_WOL_FLAG_SLEEP_ON;
> -		} else {
> -			pinctrl_pm_select_default_state(&fep->pdev->dev);
>  		}
>  		fec_restart(ndev);

[Severity: High]
This is a pre-existing issue, but is it possible for an interrupt to be
lost here?

fec_restart() explicitly enables hardware interrupts, but napi_enable()
is called afterwards in fec_resume(). If an interrupt fires in this window:

fec_enet_interrupt()
   ...
   napi_schedule_prep()

Could it clear the hardware event but fail to schedule napi because
napi_schedule_prep() returns false, stalling packet processing?

>  		netif_tx_lock_bh(ndev);

[ ... ]

[Severity: High]
This is a pre-existing issue, but does fec_suspend() handle
pm_runtime_force_suspend() failures correctly?

If pm_runtime_force_suspend() fails, fec_suspend() unlocks rtnl_lock and
returns the error early. This skips regulator_disable() and leaves the
network interface permanently detached with napi and the phy stopped.

If the system aborts suspend, does the device remain broken?

[Severity: High]
This is a pre-existing issue, but is there a potential use-after-free
in fec_drv_remove()?

cancel_work_sync(&fep->tx_timeout_work) is called before
unregister_netdev(ndev). During unregister_netdev(), the networking core
watchdog can still fire and schedule tx_timeout_work.

Since the scheduled work item is left pending and executes after fep
has been freed, can this lead to a use-after-free?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260529-b4-fec-resume-pinctrl-order-v3-1-6eda0f592fca@vaisala.com?part=1

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

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Fri, 29 May 2026 06:18:57 +0000 you wrote:
> 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.
> 
> [...]

Here is the summary with links:
  - [v3] net: fec: fix pinctrl default state restore order on resume
    https://git.kernel.org/netdev/net/c/b455410146bf

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html