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

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Missing pinctrl state rollback in the non-WoL error path of fec_resume().

Pre-existing issues:
- [High] Unbalanced regulator_disable() in the error path of fec_resume() when Wake-on-LAN is enabled.
- [High] The return value of pm_runtime_force_resume() is ignored, leading to a potential kernel panic.
- [High] The device is left in a permanently broken state if pm_runtime_force_suspend() fails in fec_suspend().
--

commit 59a1dced7a285c5b42fbbe817f6f458b3076e059
Author: Tapio Reijonen <tapio.reijonen@vaisala.com>
Subject: net: fec: fix pinctrl default state restore order on resume

This patch fixes the pinctrl default state restore order on resume for the
non-WoL path. It ensures the default pinctrl state is restored before
enabling clocks, which fixes PHY reset issues for PHYs using
PHY_RST_AFTER_CLK_EN.

Fixes: de40ed31b3c5 ("net: fec: add Wake-on-LAN support")

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

[Severity: High]
This is a pre-existing issue, but looking at fec_suspend(), the driver
performs destructive teardown operations (stopping the PHY, detaching the
net device, disabling hardware clocks) before calling
pm_runtime_force_suspend().

If pm_runtime_force_suspend() fails, fec_suspend() immediately returns the
error. Because the suspend callback returned an error, the PM core will
abort the system suspend but will not call the resume callback for this
specific device.

Does this leave the network interface in a half-suspended, detached state
with no rollback, rendering it permanently unusable until it is manually
reset or unbound?

> 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 the return value of
pm_runtime_force_resume() is ignored here. 

If clock enablement fails internally, the driver continues executing and
calls fec_restart(), which directly writes to hardware registers.

Can this trigger a synchronous external abort and a fatal kernel panic on
ARM/i.MX platforms when writing to unclocked peripheral registers?

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

[ ... ]

>  		} 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, we jump to failed_clk and return an
error without restoring the pinctrl state back to the sleep state. 

Could this leave the pins in the active/default state while the device
fails to resume, potentially causing power leaks or pin contention?

[ ... ]

>  failed_clk:
>  	if (fep->reg_phy)
>  		regulator_disable(fep->reg_phy);
>  	return ret;
>  }

[Severity: High]
This is a pre-existing issue, but when Wake-on-LAN is enabled, the initial
regulator_enable() is intentionally skipped at the start of fec_resume()
because the regulator was kept on during suspend.

If a subsequent operation like fec_enet_clk_enable() fails and jumps to
failed_clk, regulator_disable() is called unconditionally.

Could this unbalanced disable drop the regulator use count without a
matching enable in the WoL path? This might eventually cause a negative
use count warning or unexpectedly power down other devices sharing the
regulator.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260526-b4-fec-resume-pinctrl-order-v1-1-f2f926325424@vaisala.com?part=1