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

[sashiko-bot@kernel.org]

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