Re: [PATCH net 1/1] igb: Return state in pm_runtime_idle instead of power-down

[Simon Horman <horms@kernel.org>]

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 <felix.moessbauer@siemens.com>
> > > ---
> > >  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