| Message ID | 4992010.31r3eYUQgx@rjwysocki.net |
|---|---|
| State | New |
| Headers | show |
| Series | [v1] cpuidle: Do not return from cpuidle_play_dead() on callback failures | expand |
On 11/14/2024 11:46, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > If the :enter_dead() idle state callback fails for a certain state, > there may be still a shallower state for which it will work. > > Because the only caller of cpuidle_play_dead(), native_play_dead(), > falls back to hlt_play_dead() if it returns an error, it should > better try all of the idle states for which :enter_dead() is present > before failing, so change it accordingly. > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com> Tested-by: Mario Limonciello <mario.limonciello@amd.com> # 6.12-rc7 > --- > drivers/cpuidle/cpuidle.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > Index: linux-pm/drivers/cpuidle/cpuidle.c > =================================================================== > --- linux-pm.orig/drivers/cpuidle/cpuidle.c > +++ linux-pm/drivers/cpuidle/cpuidle.c > @@ -70,9 +70,10 @@ int cpuidle_play_dead(void) > return -ENODEV; > > /* Find lowest-power state that supports long-term idle */ > - for (i = drv->state_count - 1; i >= 0; i--) > - if (drv->states[i].enter_dead) > - return drv->states[i].enter_dead(dev, i); > + for (i = drv->state_count - 1; i >= 0; i--) { > + if (drv->states[i].enter_dead && !drv->states[i].enter_dead(dev, i)) > + return 0; > + } > > return -ENODEV; > } > > >
On Fri, Nov 15, 2024 at 11:14 AM Peter Zijlstra <peterz@infradead.org> wrote: > > On Thu, Nov 14, 2024 at 06:46:20PM +0100, Rafael J. Wysocki wrote: > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > If the :enter_dead() idle state callback fails for a certain state, > > there may be still a shallower state for which it will work. > > > > Because the only caller of cpuidle_play_dead(), native_play_dead(), > > falls back to hlt_play_dead() if it returns an error, it should > > better try all of the idle states for which :enter_dead() is present > > before failing, so change it accordingly. > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > --- > > drivers/cpuidle/cpuidle.c | 7 ++++--- > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > Index: linux-pm/drivers/cpuidle/cpuidle.c > > =================================================================== > > --- linux-pm.orig/drivers/cpuidle/cpuidle.c > > +++ linux-pm/drivers/cpuidle/cpuidle.c > > @@ -70,9 +70,10 @@ int cpuidle_play_dead(void) > > return -ENODEV; > > > > /* Find lowest-power state that supports long-term idle */ > > - for (i = drv->state_count - 1; i >= 0; i--) > > - if (drv->states[i].enter_dead) > > - return drv->states[i].enter_dead(dev, i); > > + for (i = drv->state_count - 1; i >= 0; i--) { > > + if (drv->states[i].enter_dead && !drv->states[i].enter_dead(dev, i)) > > + return 0; > > + } > > Hmm, strictly speaking there is no 'success' return from play_dead(). On > success, the CPU is dead :-) Well, would you prefer something like for (i = drv->state_count - 1; i >= 0; i--) { if (drv->states[i].enter_dead) drv->states[i].enter_dead(dev, i); } and adding a comment before the final return statement that :enter_dead() only returns on failure?
Index: linux-pm/drivers/cpuidle/cpuidle.c =================================================================== --- linux-pm.orig/drivers/cpuidle/cpuidle.c +++ linux-pm/drivers/cpuidle/cpuidle.c @@ -70,9 +70,10 @@ int cpuidle_play_dead(void) return -ENODEV; /* Find lowest-power state that supports long-term idle */ - for (i = drv->state_count - 1; i >= 0; i--) - if (drv->states[i].enter_dead) - return drv->states[i].enter_dead(dev, i); + for (i = drv->state_count - 1; i >= 0; i--) { + if (drv->states[i].enter_dead && !drv->states[i].enter_dead(dev, i)) + return 0; + } return -ENODEV; }