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

[sashiko-bot@kernel.org]

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