On Tue, May 26, 2026 at 07:53:53AM +0000, MOESSBAUER, Felix wrote:
> On Fri, 2026-05-08 at 10:56 +0100, Simon Horman wrote:
> > + David Ertman, Sasha Neftin, Heiner Kallweit
> >
> > On Tue, May 05, 2026 at 12:11:34PM +0200, Felix Moessbauer wrote:
> > > The PM runtime_idle API expects to get an indication if the device can
> > > be powered down. Instead of returning the appropriate state, we
> > > currently directly power down the device (if not active) and return
> > > that the device is busy.
> > >
> > > We change this by making the function side-effect free and just return
> > > the state.
> > >
> > > Fixes: 749ab2cd12704 ("igb: add basic runtime PM support")
> > > Signed-off-by: Felix Moessbauer <[email protected]>
> > > ---
> > > drivers/net/ethernet/intel/igb/igb_main.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/net/ethernet/intel/igb/igb_main.c
> > > b/drivers/net/ethernet/intel/igb/igb_main.c
> > > index ce91dda00ec0e..e8ab0b506a104 100644
> > > --- a/drivers/net/ethernet/intel/igb/igb_main.c
> > > +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> > > @@ -9652,7 +9652,7 @@ static int igb_runtime_idle(struct device *dev)
> > > struct igb_adapter *adapter = netdev_priv(netdev);
> > >
> > > if (!igb_has_link(adapter))
> > > - pm_schedule_suspend(dev, MSEC_PER_SEC * 5);
> > > + return 0;
> > >
> > > return -EBUSY;
> > > }
> >
> > Hi Felix,
> >
> > I am not sure this is the right approach, are you seeing a behavioural
> > problem?
>
> I don't see a behavioral issue, but a clear API violation. That's why I
> also added a fixes tag.
>
> >
> >
> > This pattern seems to also be present in at least e1000e, igc and r8169.
> >
> > The igb and e1000e implementations seem to have co-evolved [1][2],
> > possibly in conjunction with OOT versions of each driver.
> > The igc implementation came later, perhaps copying e1000e or igb [3].
>
> I'm fine with also changing the other drivers, however I'm not able to
> test these (except for e1000e).
>
> >
> > The git log for r8169 seems to provide a justification for why that
> > driver users this approach [4].
> >
> > - Let the idle notification check whether we can suspend and let it
> > schedule the suspend. This way we don't need to have calls to
> > pm_schedule_suspend in different places.
>
> To me it still is a bit vague, why it was implemented like that. By
> implementing it as done prior to my change, we have pm_schedule_suspend
> in multiple places (along with a hard-coded delay of 5 secs).
>
> >
> > While the current e1000e implementation seems to address some reliability
> > issues [1], although it's not entirely clear to me how that relates to the
> > issue at hand.
>
> I don't think that this relates, but I also found the igb_runtime_idle
> inconsistency while debugging why setting the mac addr on an igb nic
> without cable attached (no carrier) fails on some systems.
>
> IOW: What I propose here is a drive-by fix based on me reading the code
> and the PM API.
Thanks,
I understand your position wrt API violation. But given the history of this
code, both in igb and other Intel drivers, I think we need some insight
from people familiar with this code regarding what is going on here.
>
> Best regards,
> Felix
>
> >
> > Fix issues with:
> > RuntimePM causing the device to repeatedly flip between suspend and
> > resume
> > with the interface administratively downed.
> > Having RuntimePM enabled interfering with the functionality of Energy
> > Efficient Ethernet.
> >
> > Added checks to disallow functions that should not be executed if the
> > device is currently runtime suspended
> >
> > Make runtime_idle callback to use same deterministic behavior as the igb
> > driver.
> >
> > [1] 63eb48f151b5 ("e1000e Refactor of Runtime Power Management")
> > Fri Feb 14 07:16:46 2014 +0000
> > [2] 749ab2cd1270 ("igb: add basic runtime PM support")
> > Wed Jan 4 20:23:37 2012 +0000
> > [3] 9513d2a5dc7f ("igc: Add legacy power management support")
> > Thu Nov 14 09:54:46 2019 +020
> > [4] a92a08499b1f ("r8169: improve runtime pm in general and suspend unused
> > ports")
> > Mon Jan 8 21:39:13 2018 +0100
