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

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

[Felix Moessbauer]

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;
 }
-- 
2.53.0

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

[Simon Horman]

+ 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?


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

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.

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.

    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

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

[MOESSBAUER, Felix]

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.

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

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

[Simon Horman]

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