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