| Message ID | 4992010.31r3eYUQgx@rjwysocki.net (mailing list archive) |
|---|---|
| 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 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 :-)
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?
On Fri, Nov 15, 2024 at 01:46:29PM +0100, Rafael J. Wysocki wrote: > 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? Yeah, but perhaps remove the return value entirely if we're going to ignore it anyway. And then assume that any return is a failure to die. I mean, something like: if (drv->states[i].enter_dead && !drv->states[i].enter_dead(dev, i)) panic("Dead CPU walking..."); is 'fun' but not very useful.
On Fri, Nov 15, 2024 at 1:55 PM Peter Zijlstra <peterz@infradead.org> wrote: > > On Fri, Nov 15, 2024 at 01:46:29PM +0100, Rafael J. Wysocki wrote: > > 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? > > Yeah, but perhaps remove the return value entirely if we're going to > ignore it anyway. And then assume that any return is a failure to die. > > I mean, something like: > > if (drv->states[i].enter_dead && !drv->states[i].enter_dead(dev, i)) > panic("Dead CPU walking..."); > > is 'fun' but not very useful. The panic would be hard to debug if it ever triggers I'm afraid and there is the fallback to HLT in the caller. An error message could be printed here though: if (drv->states[i].enter_dead && !drv->states[i].enter_dead(dev, i)) pr_err("CPU %d: Unexpectedly undead\n", dev->cpu);
On Fri, Nov 15, 2024 at 02:25:23PM +0100, Rafael J. Wysocki wrote: > > I mean, something like: > > > > if (drv->states[i].enter_dead && !drv->states[i].enter_dead(dev, i)) > > panic("Dead CPU walking..."); > > > > is 'fun' but not very useful. > > The panic would be hard to debug if it ever triggers I'm afraid and > there is the fallback to HLT in the caller. I was being facetious, removing the return value and simply calling them all in reverse order is fine.
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; }