| Message ID | 2285569.iZASKD2KPV@rjwysocki.net |
|---|---|
| State | New |
| Headers | show |
| Series | cpuidle: Do not return from cpuidle_play_dead() on callback failures | expand |
Hello Rafael, On Fri, Nov 15, 2024 at 10:00:25PM +0100, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > After a previous change, cpuidle_play_dead(), which is the only caller > of idle state :enter_dead() callbacks, ignores their return values, so > they may as well be void. > > Suggested-by: Peter Zijlstra <peterz@infradead.org> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > > v1 -> v2: New patch > > Interestingly enough, the only user of :enter_dead() idle state callbacks > in the current mainline turns out to be ACPI idle. For that matter, the only user of cpuidle_play_dead() is the native_play_dead(). Was that always the case? Some of the other architectures select the deepest available idle state at boot time, and enter that state when a CPU is offlined. In any case I am ok with this. Reviewed-by: Gautham R. Shenoy <gautham.shenoy@amd.com> -- Thanks and Regards gautham. > > --- > drivers/acpi/processor_idle.c | 7 ++----- > include/linux/cpuidle.h | 2 +- > 2 files changed, 3 insertions(+), 6 deletions(-) > > Index: linux-pm/include/linux/cpuidle.h > =================================================================== > --- linux-pm.orig/include/linux/cpuidle.h > +++ linux-pm/include/linux/cpuidle.h > @@ -61,7 +61,7 @@ struct cpuidle_state { > struct cpuidle_driver *drv, > int index); > > - int (*enter_dead) (struct cpuidle_device *dev, int index); > + void (*enter_dead) (struct cpuidle_device *dev, int index); > > /* > * CPUs execute ->enter_s2idle with the local tick or entire timekeeping > Index: linux-pm/drivers/acpi/processor_idle.c > =================================================================== > --- linux-pm.orig/drivers/acpi/processor_idle.c > +++ linux-pm/drivers/acpi/processor_idle.c > @@ -578,7 +578,7 @@ static void __cpuidle acpi_idle_do_entry > * @dev: the target CPU > * @index: the index of suggested state > */ > -static int acpi_idle_play_dead(struct cpuidle_device *dev, int index) > +static void acpi_idle_play_dead(struct cpuidle_device *dev, int index) > { > struct acpi_processor_cx *cx = per_cpu(acpi_cstate[index], dev->cpu); > > @@ -591,11 +591,8 @@ static int acpi_idle_play_dead(struct cp > else if (cx->entry_method == ACPI_CSTATE_SYSTEMIO) { > io_idle(cx->address); > } else > - return -ENODEV; > + return; > } > - > - /* Never reached */ > - return 0; > } > > static __always_inline bool acpi_idle_fallback_to_c1(struct acpi_processor *pr) > > >
On 11/19/2024 09:09, Gautham R. Shenoy wrote: > Hello Rafael, > > On Fri, Nov 15, 2024 at 10:00:25PM +0100, Rafael J. Wysocki wrote: >> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >> >> After a previous change, cpuidle_play_dead(), which is the only caller >> of idle state :enter_dead() callbacks, ignores their return values, so >> they may as well be void. >> >> Suggested-by: Peter Zijlstra <peterz@infradead.org> >> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > >> --- >> >> v1 -> v2: New patch >> >> Interestingly enough, the only user of :enter_dead() idle state callbacks >> in the current mainline turns out to be ACPI idle. > > For that matter, the only user of cpuidle_play_dead() is the > native_play_dead(). Was that always the case? > > Some of the other architectures select the deepest available idle > state at boot time, and enter that state when a CPU is offlined. > > In any case I am ok with this. > > Reviewed-by: Gautham R. Shenoy <gautham.shenoy@amd.com> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com> > -- > Thanks and Regards > gautham. >> >> --- >> drivers/acpi/processor_idle.c | 7 ++----- >> include/linux/cpuidle.h | 2 +- >> 2 files changed, 3 insertions(+), 6 deletions(-) >> >> Index: linux-pm/include/linux/cpuidle.h >> =================================================================== >> --- linux-pm.orig/include/linux/cpuidle.h >> +++ linux-pm/include/linux/cpuidle.h >> @@ -61,7 +61,7 @@ struct cpuidle_state { >> struct cpuidle_driver *drv, >> int index); >> >> - int (*enter_dead) (struct cpuidle_device *dev, int index); >> + void (*enter_dead) (struct cpuidle_device *dev, int index); >> >> /* >> * CPUs execute ->enter_s2idle with the local tick or entire timekeeping >> Index: linux-pm/drivers/acpi/processor_idle.c >> =================================================================== >> --- linux-pm.orig/drivers/acpi/processor_idle.c >> +++ linux-pm/drivers/acpi/processor_idle.c >> @@ -578,7 +578,7 @@ static void __cpuidle acpi_idle_do_entry >> * @dev: the target CPU >> * @index: the index of suggested state >> */ >> -static int acpi_idle_play_dead(struct cpuidle_device *dev, int index) >> +static void acpi_idle_play_dead(struct cpuidle_device *dev, int index) >> { >> struct acpi_processor_cx *cx = per_cpu(acpi_cstate[index], dev->cpu); >> >> @@ -591,11 +591,8 @@ static int acpi_idle_play_dead(struct cp >> else if (cx->entry_method == ACPI_CSTATE_SYSTEMIO) { >> io_idle(cx->address); >> } else >> - return -ENODEV; >> + return; >> } >> - >> - /* Never reached */ >> - return 0; >> } >> >> static __always_inline bool acpi_idle_fallback_to_c1(struct acpi_processor *pr) >> >> >>
Hi Gautham, On Tue, Nov 19, 2024 at 4:09 PM Gautham R. Shenoy <gautham.shenoy@amd.com> wrote: > > Hello Rafael, > > On Fri, Nov 15, 2024 at 10:00:25PM +0100, Rafael J. Wysocki wrote: > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > After a previous change, cpuidle_play_dead(), which is the only caller > > of idle state :enter_dead() callbacks, ignores their return values, so > > they may as well be void. > > > > Suggested-by: Peter Zijlstra <peterz@infradead.org> > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > --- > > > > v1 -> v2: New patch > > > > Interestingly enough, the only user of :enter_dead() idle state callbacks > > in the current mainline turns out to be ACPI idle. > > For that matter, the only user of cpuidle_play_dead() is the > native_play_dead(). Was that always the case? At least as long ago as of 4.20. > Some of the other architectures select the deepest available idle > state at boot time, and enter that state when a CPU is offlined. That's fine as long as the state to use is already known at that time. > In any case I am ok with this. > > Reviewed-by: Gautham R. Shenoy <gautham.shenoy@amd.com> > -- > Thanks and Regards > gautham. > > > > --- > > drivers/acpi/processor_idle.c | 7 ++----- > > include/linux/cpuidle.h | 2 +- > > 2 files changed, 3 insertions(+), 6 deletions(-) > > > > Index: linux-pm/include/linux/cpuidle.h > > =================================================================== > > --- linux-pm.orig/include/linux/cpuidle.h > > +++ linux-pm/include/linux/cpuidle.h > > @@ -61,7 +61,7 @@ struct cpuidle_state { > > struct cpuidle_driver *drv, > > int index); > > > > - int (*enter_dead) (struct cpuidle_device *dev, int index); > > + void (*enter_dead) (struct cpuidle_device *dev, int index); > > > > /* > > * CPUs execute ->enter_s2idle with the local tick or entire timekeeping > > Index: linux-pm/drivers/acpi/processor_idle.c > > =================================================================== > > --- linux-pm.orig/drivers/acpi/processor_idle.c > > +++ linux-pm/drivers/acpi/processor_idle.c > > @@ -578,7 +578,7 @@ static void __cpuidle acpi_idle_do_entry > > * @dev: the target CPU > > * @index: the index of suggested state > > */ > > -static int acpi_idle_play_dead(struct cpuidle_device *dev, int index) > > +static void acpi_idle_play_dead(struct cpuidle_device *dev, int index) > > { > > struct acpi_processor_cx *cx = per_cpu(acpi_cstate[index], dev->cpu); > > > > @@ -591,11 +591,8 @@ static int acpi_idle_play_dead(struct cp > > else if (cx->entry_method == ACPI_CSTATE_SYSTEMIO) { > > io_idle(cx->address); > > } else > > - return -ENODEV; > > + return; > > } > > - > > - /* Never reached */ > > - return 0; > > } > > > > static __always_inline bool acpi_idle_fallback_to_c1(struct acpi_processor *pr) > > > > > > >
Index: linux-pm/include/linux/cpuidle.h =================================================================== --- linux-pm.orig/include/linux/cpuidle.h +++ linux-pm/include/linux/cpuidle.h @@ -61,7 +61,7 @@ struct cpuidle_state { struct cpuidle_driver *drv, int index); - int (*enter_dead) (struct cpuidle_device *dev, int index); + void (*enter_dead) (struct cpuidle_device *dev, int index); /* * CPUs execute ->enter_s2idle with the local tick or entire timekeeping Index: linux-pm/drivers/acpi/processor_idle.c =================================================================== --- linux-pm.orig/drivers/acpi/processor_idle.c +++ linux-pm/drivers/acpi/processor_idle.c @@ -578,7 +578,7 @@ static void __cpuidle acpi_idle_do_entry * @dev: the target CPU * @index: the index of suggested state */ -static int acpi_idle_play_dead(struct cpuidle_device *dev, int index) +static void acpi_idle_play_dead(struct cpuidle_device *dev, int index) { struct acpi_processor_cx *cx = per_cpu(acpi_cstate[index], dev->cpu); @@ -591,11 +591,8 @@ static int acpi_idle_play_dead(struct cp else if (cx->entry_method == ACPI_CSTATE_SYSTEMIO) { io_idle(cx->address); } else - return -ENODEV; + return; } - - /* Never reached */ - return 0; } static __always_inline bool acpi_idle_fallback_to_c1(struct acpi_processor *pr)