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