[v8,01/16] ACPI: processor: Simplify initial onlining to use same path for cold and hotplug

Commit Message

Jonathan Cameron April 26, 2024, 1:51 p.m. UTC

Separate code paths, combined with a flag set in acpi_processor.c to
indicate a struct acpi_processor was for a hotplugged CPU ensured that
per CPU data was only set up the first time that a CPU was initialized.
This appears to be unnecessary as the paths can be combined by letting
the online logic also handle any CPUs online at the time of driver load.

Motivation for this change, beyond simplification, is that ARM64
virtual CPU HP uses the same code paths for hotplug and cold path in
acpi_processor.c so had no easy way to set the flag for hotplug only.
Removing this necessity will enable ARM64 vCPU HP to reuse the existing
code paths.

Leave noisy pr_info() in place but update it to not state the CPU
was hotplugged.

Acked-by: Rafael J. Wysocki <[e-mail]>
Reviewed-by: Hanjun Guo <[e-mail]>
Tested-by: Miguel Luis <[e-mail]>
Reviewed-by: Gavin Shan <[e-mail]>
Signed-off-by: Jonathan Cameron <[e-mail]>

---
v8: No change
---
 drivers/acpi/acpi_processor.c   |  1 -
 drivers/acpi/processor_driver.c | 44 ++++++++++-----------------------
 include/acpi/processor.h        |  2 +-
 3 files changed, 14 insertions(+), 33 deletions(-)

Comments

Miguel Luis April 26, 2024, 4:05 p.m. UTC | #1

> On 26 Apr 2024, at 13:51, Jonathan Cameron <[e-mail]> wrote:
> 
> Separate code paths, combined with a flag set in acpi_processor.c to
> indicate a struct acpi_processor was for a hotplugged CPU ensured that
> per CPU data was only set up the first time that a CPU was initialized.
> This appears to be unnecessary as the paths can be combined by letting
> the online logic also handle any CPUs online at the time of driver load.
> 
> Motivation for this change, beyond simplification, is that ARM64
> virtual CPU HP uses the same code paths for hotplug and cold path in
> acpi_processor.c so had no easy way to set the flag for hotplug only.
> Removing this necessity will enable ARM64 vCPU HP to reuse the existing
> code paths.
> 
> Leave noisy pr_info() in place but update it to not state the CPU
> was hotplugged.
> 
> Acked-by: Rafael J. Wysocki <[e-mail]>
> Reviewed-by: Hanjun Guo <[e-mail]>
> Tested-by: Miguel Luis <[e-mail]>
> Reviewed-by: Gavin Shan <[e-mail]>
> Signed-off-by: Jonathan Cameron <[e-mail]>
> 
> ---
> v8: No change
> ---
> drivers/acpi/acpi_processor.c   |  1 -
> drivers/acpi/processor_driver.c | 44 ++++++++++-----------------------
> include/acpi/processor.h        |  2 +-
> 3 files changed, 14 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
> index 7a0dd35d62c9..7fc924aeeed0 100644
> --- a/drivers/acpi/acpi_processor.c
> +++ b/drivers/acpi/acpi_processor.c
> @@ -216,7 +216,6 @@ static int acpi_processor_hotadd_init(struct acpi_processor *pr)
> * gets online for the first time.
> */
> pr_info("CPU%d has been hot-added\n", pr->id);
> - pr->flags.need_hotplug_init = 1;
> 
> out:
> cpus_write_unlock();
> diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
> index 67db60eda370..55782eac3ff1 100644
> --- a/drivers/acpi/processor_driver.c
> +++ b/drivers/acpi/processor_driver.c
> @@ -33,7 +33,6 @@ MODULE_AUTHOR("Paul Diefenbaugh");
> MODULE_DESCRIPTION("ACPI Processor Driver");
> MODULE_LICENSE("GPL");
> 
> -static int acpi_processor_start(struct device *dev);
> static int acpi_processor_stop(struct device *dev);
> 
> static const struct acpi_device_id processor_device_ids[] = {
> @@ -47,7 +46,6 @@ static struct device_driver acpi_processor_driver = {
> .name = "processor",
> .bus = &cpu_subsys,
> .acpi_match_table = processor_device_ids,
> - .probe = acpi_processor_start,
> .remove = acpi_processor_stop,
> };
> 
> @@ -115,12 +113,10 @@ static int acpi_soft_cpu_online(unsigned int cpu)
> * CPU got physically hotplugged and onlined for the first time:
> * Initialize missing things.
> */
> - if (pr->flags.need_hotplug_init) {
> + if (!pr->flags.previously_online) {
> int ret;
> 
> - pr_info("Will online and init hotplugged CPU: %d\n",
> - pr->id);
> - pr->flags.need_hotplug_init = 0;
> + pr_info("Will online and init CPU: %d\n", pr->id);
> ret = __acpi_processor_start(device);
> WARN(ret, "Failed to start CPU: %d\n", pr->id);
> } else {
> @@ -167,9 +163,6 @@ static int __acpi_processor_start(struct acpi_device *device)
> if (!pr)
> return -ENODEV;
> 
> - if (pr->flags.need_hotplug_init)
> - return 0;
> -
> result = acpi_cppc_processor_probe(pr);
> if (result && !IS_ENABLED(CONFIG_ACPI_CPU_FREQ_PSS))
> dev_dbg(&device->dev, "CPPC data invalid or not present\n");
> @@ -185,32 +178,21 @@ static int __acpi_processor_start(struct acpi_device *device)
> 
> status = acpi_install_notify_handler(device->handle, ACPI_DEVICE_NOTIFY,
>     acpi_processor_notify, device);
> - if (ACPI_SUCCESS(status))
> - return 0;
> + if (!ACPI_SUCCESS(status)) {
> + result = -ENODEV;
> + goto err_thermal_exit;
> + }
> + pr->flags.previously_online = 1;
> 
> - result = -ENODEV;
> - acpi_processor_thermal_exit(pr, device);
> + return 0;
> 
> +err_thermal_exit:
> + acpi_processor_thermal_exit(pr, device);
> err_power_exit:
> acpi_processor_power_exit(pr);
> return result;
> }
> 
> -static int acpi_processor_start(struct device *dev)
> -{
> - struct acpi_device *device = ACPI_COMPANION(dev);
> - int ret;
> -
> - if (!device)
> - return -ENODEV;
> -
> - /* Protect against concurrent CPU hotplug operations */
> - cpu_hotplug_disable();
> - ret = __acpi_processor_start(device);
> - cpu_hotplug_enable();
> - return ret;
> -}
> -
> static int acpi_processor_stop(struct device *dev)
> {
> struct acpi_device *device = ACPI_COMPANION(dev);
> @@ -279,9 +261,9 @@ static int __init acpi_processor_driver_init(void)
> if (result < 0)
> return result;
> 
> - result = cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN,
> -   "acpi/cpu-drv:online",
> -   acpi_soft_cpu_online, NULL);
> + result = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
> +   "acpi/cpu-drv:online",
> +   acpi_soft_cpu_online, NULL);
> if (result < 0)
> goto err;
> hp_online = result;
> diff --git a/include/acpi/processor.h b/include/acpi/processor.h
> index 3f34ebb27525..e6f6074eadbf 100644
> --- a/include/acpi/processor.h
> +++ b/include/acpi/processor.h
> @@ -217,7 +217,7 @@ struct acpi_processor_flags {
> u8 has_lpi:1;
> u8 power_setup_done:1;
> u8 bm_rld_set:1;
> - u8 need_hotplug_init:1;
> + u8 previously_online:1;

Reviewed-by: Miguel Luis <[e-mail]>

Miguel

> };
> 
> struct acpi_processor {
> -- 
> 2.39.2
>

Miguel Luis April 26, 2024, 5:21 p.m. UTC | #2

Hi Jonathan, 

> On 26 Apr 2024, at 16:05, Miguel Luis <[e-mail]> wrote:
> 
> 
> 
>> On 26 Apr 2024, at 13:51, Jonathan Cameron <[e-mail]> wrote:
>> 
>> Separate code paths, combined with a flag set in acpi_processor.c to
>> indicate a struct acpi_processor was for a hotplugged CPU ensured that
>> per CPU data was only set up the first time that a CPU was initialized.
>> This appears to be unnecessary as the paths can be combined by letting
>> the online logic also handle any CPUs online at the time of driver load.
>> 
>> Motivation for this change, beyond simplification, is that ARM64
>> virtual CPU HP uses the same code paths for hotplug and cold path in
>> acpi_processor.c so had no easy way to set the flag for hotplug only.
>> Removing this necessity will enable ARM64 vCPU HP to reuse the existing
>> code paths.
>> 
>> Leave noisy pr_info() in place but update it to not state the CPU
>> was hotplugged.

On a second thought, do we want to keep it? Can't we just assume that no 
news is good news while keeping the warn right after __acpi_processor_start ?

Miguel

>> 
>> Acked-by: Rafael J. Wysocki <[e-mail]>
>> Reviewed-by: Hanjun Guo <[e-mail]>
>> Tested-by: Miguel Luis <[e-mail]>
>> Reviewed-by: Gavin Shan <[e-mail]>
>> Signed-off-by: Jonathan Cameron <[e-mail]>
>> 
>> ---
>> v8: No change
>> ---
>> drivers/acpi/acpi_processor.c   |  1 -
>> drivers/acpi/processor_driver.c | 44 ++++++++++-----------------------
>> include/acpi/processor.h        |  2 +-
>> 3 files changed, 14 insertions(+), 33 deletions(-)
>> 
>> diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
>> index 7a0dd35d62c9..7fc924aeeed0 100644
>> --- a/drivers/acpi/acpi_processor.c
>> +++ b/drivers/acpi/acpi_processor.c
>> @@ -216,7 +216,6 @@ static int acpi_processor_hotadd_init(struct acpi_processor *pr)
>> * gets online for the first time.
>> */
>> pr_info("CPU%d has been hot-added\n", pr->id);
>> - pr->flags.need_hotplug_init = 1;
>> 
>> out:
>> cpus_write_unlock();
>> diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
>> index 67db60eda370..55782eac3ff1 100644
>> --- a/drivers/acpi/processor_driver.c
>> +++ b/drivers/acpi/processor_driver.c
>> @@ -33,7 +33,6 @@ MODULE_AUTHOR("Paul Diefenbaugh");
>> MODULE_DESCRIPTION("ACPI Processor Driver");
>> MODULE_LICENSE("GPL");
>> 
>> -static int acpi_processor_start(struct device *dev);
>> static int acpi_processor_stop(struct device *dev);
>> 
>> static const struct acpi_device_id processor_device_ids[] = {
>> @@ -47,7 +46,6 @@ static struct device_driver acpi_processor_driver = {
>> .name = "processor",
>> .bus = &cpu_subsys,
>> .acpi_match_table = processor_device_ids,
>> - .probe = acpi_processor_start,
>> .remove = acpi_processor_stop,
>> };
>> 
>> @@ -115,12 +113,10 @@ static int acpi_soft_cpu_online(unsigned int cpu)
>> * CPU got physically hotplugged and onlined for the first time:
>> * Initialize missing things.
>> */
>> - if (pr->flags.need_hotplug_init) {
>> + if (!pr->flags.previously_online) {
>> int ret;
>> 
>> - pr_info("Will online and init hotplugged CPU: %d\n",
>> - pr->id);
>> - pr->flags.need_hotplug_init = 0;
>> + pr_info("Will online and init CPU: %d\n", pr->id);
>> ret = __acpi_processor_start(device);
>> WARN(ret, "Failed to start CPU: %d\n", pr->id);
>> } else {
>> @@ -167,9 +163,6 @@ static int __acpi_processor_start(struct acpi_device *device)
>> if (!pr)
>> return -ENODEV;
>> 
>> - if (pr->flags.need_hotplug_init)
>> - return 0;
>> -
>> result = acpi_cppc_processor_probe(pr);
>> if (result && !IS_ENABLED(CONFIG_ACPI_CPU_FREQ_PSS))
>> dev_dbg(&device->dev, "CPPC data invalid or not present\n");
>> @@ -185,32 +178,21 @@ static int __acpi_processor_start(struct acpi_device *device)
>> 
>> status = acpi_install_notify_handler(device->handle, ACPI_DEVICE_NOTIFY,
>>    acpi_processor_notify, device);
>> - if (ACPI_SUCCESS(status))
>> - return 0;
>> + if (!ACPI_SUCCESS(status)) {
>> + result = -ENODEV;
>> + goto err_thermal_exit;
>> + }
>> + pr->flags.previously_online = 1;
>> 
>> - result = -ENODEV;
>> - acpi_processor_thermal_exit(pr, device);
>> + return 0;
>> 
>> +err_thermal_exit:
>> + acpi_processor_thermal_exit(pr, device);
>> err_power_exit:
>> acpi_processor_power_exit(pr);
>> return result;
>> }
>> 
>> -static int acpi_processor_start(struct device *dev)
>> -{
>> - struct acpi_device *device = ACPI_COMPANION(dev);
>> - int ret;
>> -
>> - if (!device)
>> - return -ENODEV;
>> -
>> - /* Protect against concurrent CPU hotplug operations */
>> - cpu_hotplug_disable();
>> - ret = __acpi_processor_start(device);
>> - cpu_hotplug_enable();
>> - return ret;
>> -}
>> -
>> static int acpi_processor_stop(struct device *dev)
>> {
>> struct acpi_device *device = ACPI_COMPANION(dev);
>> @@ -279,9 +261,9 @@ static int __init acpi_processor_driver_init(void)
>> if (result < 0)
>> return result;
>> 
>> - result = cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN,
>> -   "acpi/cpu-drv:online",
>> -   acpi_soft_cpu_online, NULL);
>> + result = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
>> +   "acpi/cpu-drv:online",
>> +   acpi_soft_cpu_online, NULL);
>> if (result < 0)
>> goto err;
>> hp_online = result;
>> diff --git a/include/acpi/processor.h b/include/acpi/processor.h
>> index 3f34ebb27525..e6f6074eadbf 100644
>> --- a/include/acpi/processor.h
>> +++ b/include/acpi/processor.h
>> @@ -217,7 +217,7 @@ struct acpi_processor_flags {
>> u8 has_lpi:1;
>> u8 power_setup_done:1;
>> u8 bm_rld_set:1;
>> - u8 need_hotplug_init:1;
>> + u8 previously_online:1;
> 
> Reviewed-by: Miguel Luis <[e-mail]>
> 
> Miguel
> 
>> };
>> 
>> struct acpi_processor {
>> -- 
>> 2.39.2
>> 
>

Jonathan Cameron April 26, 2024, 5:49 p.m. UTC | #3

On Fri, 26 Apr 2024 17:21:41 +0000
Miguel Luis <[e-mail]> wrote:

> Hi Jonathan, 
> 
> > On 26 Apr 2024, at 16:05, Miguel Luis <[e-mail]> wrote:
> > 
> > 
> >   
> >> On 26 Apr 2024, at 13:51, Jonathan Cameron <[e-mail]> wrote:
> >> 
> >> Separate code paths, combined with a flag set in acpi_processor.c to
> >> indicate a struct acpi_processor was for a hotplugged CPU ensured that
> >> per CPU data was only set up the first time that a CPU was initialized.
> >> This appears to be unnecessary as the paths can be combined by letting
> >> the online logic also handle any CPUs online at the time of driver load.
> >> 
> >> Motivation for this change, beyond simplification, is that ARM64
> >> virtual CPU HP uses the same code paths for hotplug and cold path in
> >> acpi_processor.c so had no easy way to set the flag for hotplug only.
> >> Removing this necessity will enable ARM64 vCPU HP to reuse the existing
> >> code paths.
> >> 
> >> Leave noisy pr_info() in place but update it to not state the CPU
> >> was hotplugged.  
> 
> On a second thought, do we want to keep it? Can't we just assume that no 
> news is good news while keeping the warn right after __acpi_processor_start ?

Good question - my inclination was to keep this in place for now as removing
it would remove a source of information people may expect on x86 hotplug.

Then maybe propose dropping it as overly noisy kernel as a follow up
patch after this series is merged.  Felt like a potential rat hole I didn't
want to go down if I could avoid it.

If any x86 experts want to shout that no one cares then I'll happily drop
the print.  We've carefully made it so that on arm64 we have no way to tell
if this is hotplug or normal cpu bring up so we can't just print it on
hotplug.

Jonathan


> 
> Miguel
> 
> >> 
> >> Acked-by: Rafael J. Wysocki <[e-mail]>
> >> Reviewed-by: Hanjun Guo <[e-mail]>
> >> Tested-by: Miguel Luis <[e-mail]>
> >> Reviewed-by: Gavin Shan <[e-mail]>
> >> Signed-off-by: Jonathan Cameron <[e-mail]>
> >> 
> >> ---
> >> v8: No change
> >> ---
> >> drivers/acpi/acpi_processor.c   |  1 -
> >> drivers/acpi/processor_driver.c | 44 ++++++++++-----------------------
> >> include/acpi/processor.h        |  2 +-
> >> 3 files changed, 14 insertions(+), 33 deletions(-)
> >> 
> >> diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
> >> index 7a0dd35d62c9..7fc924aeeed0 100644
> >> --- a/drivers/acpi/acpi_processor.c
> >> +++ b/drivers/acpi/acpi_processor.c
> >> @@ -216,7 +216,6 @@ static int acpi_processor_hotadd_init(struct acpi_processor *pr)
> >> * gets online for the first time.
> >> */
> >> pr_info("CPU%d has been hot-added\n", pr->id);
> >> - pr->flags.need_hotplug_init = 1;
> >> 
> >> out:
> >> cpus_write_unlock();
> >> diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
> >> index 67db60eda370..55782eac3ff1 100644
> >> --- a/drivers/acpi/processor_driver.c
> >> +++ b/drivers/acpi/processor_driver.c
> >> @@ -33,7 +33,6 @@ MODULE_AUTHOR("Paul Diefenbaugh");
> >> MODULE_DESCRIPTION("ACPI Processor Driver");
> >> MODULE_LICENSE("GPL");
> >> 
> >> -static int acpi_processor_start(struct device *dev);
> >> static int acpi_processor_stop(struct device *dev);
> >> 
> >> static const struct acpi_device_id processor_device_ids[] = {
> >> @@ -47,7 +46,6 @@ static struct device_driver acpi_processor_driver = {
> >> .name = "processor",
> >> .bus = &cpu_subsys,
> >> .acpi_match_table = processor_device_ids,
> >> - .probe = acpi_processor_start,
> >> .remove = acpi_processor_stop,
> >> };
> >> 
> >> @@ -115,12 +113,10 @@ static int acpi_soft_cpu_online(unsigned int cpu)
> >> * CPU got physically hotplugged and onlined for the first time:
> >> * Initialize missing things.
> >> */
> >> - if (pr->flags.need_hotplug_init) {
> >> + if (!pr->flags.previously_online) {
> >> int ret;
> >> 
> >> - pr_info("Will online and init hotplugged CPU: %d\n",
> >> - pr->id);
> >> - pr->flags.need_hotplug_init = 0;
> >> + pr_info("Will online and init CPU: %d\n", pr->id);
> >> ret = __acpi_processor_start(device);
> >> WARN(ret, "Failed to start CPU: %d\n", pr->id);
> >> } else {
> >> @@ -167,9 +163,6 @@ static int __acpi_processor_start(struct acpi_device *device)
> >> if (!pr)
> >> return -ENODEV;
> >> 
> >> - if (pr->flags.need_hotplug_init)
> >> - return 0;
> >> -
> >> result = acpi_cppc_processor_probe(pr);
> >> if (result && !IS_ENABLED(CONFIG_ACPI_CPU_FREQ_PSS))
> >> dev_dbg(&device->dev, "CPPC data invalid or not present\n");
> >> @@ -185,32 +178,21 @@ static int __acpi_processor_start(struct acpi_device *device)
> >> 
> >> status = acpi_install_notify_handler(device->handle, ACPI_DEVICE_NOTIFY,
> >>    acpi_processor_notify, device);
> >> - if (ACPI_SUCCESS(status))
> >> - return 0;
> >> + if (!ACPI_SUCCESS(status)) {
> >> + result = -ENODEV;
> >> + goto err_thermal_exit;
> >> + }
> >> + pr->flags.previously_online = 1;
> >> 
> >> - result = -ENODEV;
> >> - acpi_processor_thermal_exit(pr, device);
> >> + return 0;
> >> 
> >> +err_thermal_exit:
> >> + acpi_processor_thermal_exit(pr, device);
> >> err_power_exit:
> >> acpi_processor_power_exit(pr);
> >> return result;
> >> }
> >> 
> >> -static int acpi_processor_start(struct device *dev)
> >> -{
> >> - struct acpi_device *device = ACPI_COMPANION(dev);
> >> - int ret;
> >> -
> >> - if (!device)
> >> - return -ENODEV;
> >> -
> >> - /* Protect against concurrent CPU hotplug operations */
> >> - cpu_hotplug_disable();
> >> - ret = __acpi_processor_start(device);
> >> - cpu_hotplug_enable();
> >> - return ret;
> >> -}
> >> -
> >> static int acpi_processor_stop(struct device *dev)
> >> {
> >> struct acpi_device *device = ACPI_COMPANION(dev);
> >> @@ -279,9 +261,9 @@ static int __init acpi_processor_driver_init(void)
> >> if (result < 0)
> >> return result;
> >> 
> >> - result = cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN,
> >> -   "acpi/cpu-drv:online",
> >> -   acpi_soft_cpu_online, NULL);
> >> + result = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
> >> +   "acpi/cpu-drv:online",
> >> +   acpi_soft_cpu_online, NULL);
> >> if (result < 0)
> >> goto err;
> >> hp_online = result;
> >> diff --git a/include/acpi/processor.h b/include/acpi/processor.h
> >> index 3f34ebb27525..e6f6074eadbf 100644
> >> --- a/include/acpi/processor.h
> >> +++ b/include/acpi/processor.h
> >> @@ -217,7 +217,7 @@ struct acpi_processor_flags {
> >> u8 has_lpi:1;
> >> u8 power_setup_done:1;
> >> u8 bm_rld_set:1;
> >> - u8 need_hotplug_init:1;
> >> + u8 previously_online:1;  
> > 
> > Reviewed-by: Miguel Luis <[e-mail]>
> > 
> > Miguel
> >   
> >> };
> >> 
> >> struct acpi_processor {
> >> -- 
> >> 2.39.2
> >>   
> >   
>

Rafael J. Wysocki April 26, 2024, 5:57 p.m. UTC | #4

On Fri, Apr 26, 2024 at 7:49 PM Jonathan Cameron
<[e-mail]> wrote:
>
> On Fri, 26 Apr 2024 17:21:41 +0000
> Miguel Luis <[e-mail]> wrote:
>
> > Hi Jonathan,
> >
> > > On 26 Apr 2024, at 16:05, Miguel Luis <[e-mail]> wrote:
> > >
> > >
> > >
> > >> On 26 Apr 2024, at 13:51, Jonathan Cameron <[e-mail]> wrote:
> > >>
> > >> Separate code paths, combined with a flag set in acpi_processor.c to
> > >> indicate a struct acpi_processor was for a hotplugged CPU ensured that
> > >> per CPU data was only set up the first time that a CPU was initialized.
> > >> This appears to be unnecessary as the paths can be combined by letting
> > >> the online logic also handle any CPUs online at the time of driver load.
> > >>
> > >> Motivation for this change, beyond simplification, is that ARM64
> > >> virtual CPU HP uses the same code paths for hotplug and cold path in
> > >> acpi_processor.c so had no easy way to set the flag for hotplug only.
> > >> Removing this necessity will enable ARM64 vCPU HP to reuse the existing
> > >> code paths.
> > >>
> > >> Leave noisy pr_info() in place but update it to not state the CPU
> > >> was hotplugged.
> >
> > On a second thought, do we want to keep it? Can't we just assume that no
> > news is good news while keeping the warn right after __acpi_processor_start ?
>
> Good question - my inclination was to keep this in place for now as removing
> it would remove a source of information people may expect on x86 hotplug.
>
> Then maybe propose dropping it as overly noisy kernel as a follow up
> patch after this series is merged.  Felt like a potential rat hole I didn't
> want to go down if I could avoid it.
>
> If any x86 experts want to shout that no one cares then I'll happily drop
> the print.

You can do that I think and see if anyone complains.  I'm not really
expecting this to happen, though.

Jonathan Cameron April 26, 2024, 6:09 p.m. UTC | #5

On Fri, 26 Apr 2024 18:49:49 +0100
Jonathan Cameron <[e-mail]> wrote:

> On Fri, 26 Apr 2024 17:21:41 +0000
> Miguel Luis <[e-mail]> wrote:
> 
> > Hi Jonathan, 
> >   
> > > On 26 Apr 2024, at 16:05, Miguel Luis <[e-mail]> wrote:
> > > 
> > > 
> > >     
> > >> On 26 Apr 2024, at 13:51, Jonathan Cameron <[e-mail]> wrote:
> > >> 
> > >> Separate code paths, combined with a flag set in acpi_processor.c to
> > >> indicate a struct acpi_processor was for a hotplugged CPU ensured that
> > >> per CPU data was only set up the first time that a CPU was initialized.
> > >> This appears to be unnecessary as the paths can be combined by letting
> > >> the online logic also handle any CPUs online at the time of driver load.
> > >> 
> > >> Motivation for this change, beyond simplification, is that ARM64
> > >> virtual CPU HP uses the same code paths for hotplug and cold path in
> > >> acpi_processor.c so had no easy way to set the flag for hotplug only.
> > >> Removing this necessity will enable ARM64 vCPU HP to reuse the existing
> > >> code paths.
> > >> 
> > >> Leave noisy pr_info() in place but update it to not state the CPU
> > >> was hotplugged.    
> > 
> > On a second thought, do we want to keep it? Can't we just assume that no 
> > news is good news while keeping the warn right after __acpi_processor_start ?  
> 
> Good question - my inclination was to keep this in place for now as removing
> it would remove a source of information people may expect on x86 hotplug.
> 
> Then maybe propose dropping it as overly noisy kernel as a follow up
> patch after this series is merged.  Felt like a potential rat hole I didn't
> want to go down if I could avoid it.
> 
> If any x86 experts want to shout that no one cares then I'll happily drop
> the print.  We've carefully made it so that on arm64 we have no way to tell
> if this is hotplug or normal cpu bring up so we can't just print it on
> hotplug.

I'm being silly. This is just one of the messages shouting out hotplug
happened and for that matter only occurs at online anyway which is trivially
detected.

There is a much more informative
  ACPI: CPU3: has been hot-added message
for example on the actual hotplug event.

Let's drop it for v9.

There is also a stale comment about a flag being set that is no longer
the case that I'll drop.

> 
> Jonathan
> 
> 
> > 
> > Miguel
> >   
> > >> 
> > >> Acked-by: Rafael J. Wysocki <[e-mail]>
> > >> Reviewed-by: Hanjun Guo <[e-mail]>
> > >> Tested-by: Miguel Luis <[e-mail]>
> > >> Reviewed-by: Gavin Shan <[e-mail]>
> > >> Signed-off-by: Jonathan Cameron <[e-mail]>
> > >> 
> > >> ---
> > >> v8: No change
> > >> ---
> > >> drivers/acpi/acpi_processor.c   |  1 -
> > >> drivers/acpi/processor_driver.c | 44 ++++++++++-----------------------
> > >> include/acpi/processor.h        |  2 +-
> > >> 3 files changed, 14 insertions(+), 33 deletions(-)
> > >> 
> > >> diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
> > >> index 7a0dd35d62c9..7fc924aeeed0 100644
> > >> --- a/drivers/acpi/acpi_processor.c
> > >> +++ b/drivers/acpi/acpi_processor.c
> > >> @@ -216,7 +216,6 @@ static int acpi_processor_hotadd_init(struct acpi_processor *pr)
> > >> * gets online for the first time.
> > >> */
> > >> pr_info("CPU%d has been hot-added\n", pr->id);
> > >> - pr->flags.need_hotplug_init = 1;
> > >> 
> > >> out:
> > >> cpus_write_unlock();
> > >> diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
> > >> index 67db60eda370..55782eac3ff1 100644
> > >> --- a/drivers/acpi/processor_driver.c
> > >> +++ b/drivers/acpi/processor_driver.c
> > >> @@ -33,7 +33,6 @@ MODULE_AUTHOR("Paul Diefenbaugh");
> > >> MODULE_DESCRIPTION("ACPI Processor Driver");
> > >> MODULE_LICENSE("GPL");
> > >> 
> > >> -static int acpi_processor_start(struct device *dev);
> > >> static int acpi_processor_stop(struct device *dev);
> > >> 
> > >> static const struct acpi_device_id processor_device_ids[] = {
> > >> @@ -47,7 +46,6 @@ static struct device_driver acpi_processor_driver = {
> > >> .name = "processor",
> > >> .bus = &cpu_subsys,
> > >> .acpi_match_table = processor_device_ids,
> > >> - .probe = acpi_processor_start,
> > >> .remove = acpi_processor_stop,
> > >> };
> > >> 
> > >> @@ -115,12 +113,10 @@ static int acpi_soft_cpu_online(unsigned int cpu)
> > >> * CPU got physically hotplugged and onlined for the first time:
> > >> * Initialize missing things.
> > >> */
> > >> - if (pr->flags.need_hotplug_init) {
> > >> + if (!pr->flags.previously_online) {
> > >> int ret;
> > >> 
> > >> - pr_info("Will online and init hotplugged CPU: %d\n",
> > >> - pr->id);
> > >> - pr->flags.need_hotplug_init = 0;
> > >> + pr_info("Will online and init CPU: %d\n", pr->id);
> > >> ret = __acpi_processor_start(device);
> > >> WARN(ret, "Failed to start CPU: %d\n", pr->id);
> > >> } else {
> > >> @@ -167,9 +163,6 @@ static int __acpi_processor_start(struct acpi_device *device)
> > >> if (!pr)
> > >> return -ENODEV;
> > >> 
> > >> - if (pr->flags.need_hotplug_init)
> > >> - return 0;
> > >> -
> > >> result = acpi_cppc_processor_probe(pr);
> > >> if (result && !IS_ENABLED(CONFIG_ACPI_CPU_FREQ_PSS))
> > >> dev_dbg(&device->dev, "CPPC data invalid or not present\n");
> > >> @@ -185,32 +178,21 @@ static int __acpi_processor_start(struct acpi_device *device)
> > >> 
> > >> status = acpi_install_notify_handler(device->handle, ACPI_DEVICE_NOTIFY,
> > >>    acpi_processor_notify, device);
> > >> - if (ACPI_SUCCESS(status))
> > >> - return 0;
> > >> + if (!ACPI_SUCCESS(status)) {
> > >> + result = -ENODEV;
> > >> + goto err_thermal_exit;
> > >> + }
> > >> + pr->flags.previously_online = 1;
> > >> 
> > >> - result = -ENODEV;
> > >> - acpi_processor_thermal_exit(pr, device);
> > >> + return 0;
> > >> 
> > >> +err_thermal_exit:
> > >> + acpi_processor_thermal_exit(pr, device);
> > >> err_power_exit:
> > >> acpi_processor_power_exit(pr);
> > >> return result;
> > >> }
> > >> 
> > >> -static int acpi_processor_start(struct device *dev)
> > >> -{
> > >> - struct acpi_device *device = ACPI_COMPANION(dev);
> > >> - int ret;
> > >> -
> > >> - if (!device)
> > >> - return -ENODEV;
> > >> -
> > >> - /* Protect against concurrent CPU hotplug operations */
> > >> - cpu_hotplug_disable();
> > >> - ret = __acpi_processor_start(device);
> > >> - cpu_hotplug_enable();
> > >> - return ret;
> > >> -}
> > >> -
> > >> static int acpi_processor_stop(struct device *dev)
> > >> {
> > >> struct acpi_device *device = ACPI_COMPANION(dev);
> > >> @@ -279,9 +261,9 @@ static int __init acpi_processor_driver_init(void)
> > >> if (result < 0)
> > >> return result;
> > >> 
> > >> - result = cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN,
> > >> -   "acpi/cpu-drv:online",
> > >> -   acpi_soft_cpu_online, NULL);
> > >> + result = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
> > >> +   "acpi/cpu-drv:online",
> > >> +   acpi_soft_cpu_online, NULL);
> > >> if (result < 0)
> > >> goto err;
> > >> hp_online = result;
> > >> diff --git a/include/acpi/processor.h b/include/acpi/processor.h
> > >> index 3f34ebb27525..e6f6074eadbf 100644
> > >> --- a/include/acpi/processor.h
> > >> +++ b/include/acpi/processor.h
> > >> @@ -217,7 +217,7 @@ struct acpi_processor_flags {
> > >> u8 has_lpi:1;
> > >> u8 power_setup_done:1;
> > >> u8 bm_rld_set:1;
> > >> - u8 need_hotplug_init:1;
> > >> + u8 previously_online:1;    
> > > 
> > > Reviewed-by: Miguel Luis <[e-mail]>
> > > 
> > > Miguel
> > >     
> > >> };
> > >> 
> > >> struct acpi_processor {
> > >> -- 
> > >> 2.39.2
> > >>     
> > >     
> >   
>

Patch

diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
index 7a0dd35d62c9..7fc924aeeed0 100644
--- a/drivers/acpi/acpi_processor.c
+++ b/drivers/acpi/acpi_processor.c
@@ -216,7 +216,6 @@  static int acpi_processor_hotadd_init(struct acpi_processor *pr)
 	 * gets online for the first time.
 	 */
 	pr_info("CPU%d has been hot-added\n", pr->id);
-	pr->flags.need_hotplug_init = 1;
 
 out:
 	cpus_write_unlock();
diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
index 67db60eda370..55782eac3ff1 100644
--- a/drivers/acpi/processor_driver.c
+++ b/drivers/acpi/processor_driver.c
@@ -33,7 +33,6 @@  MODULE_AUTHOR("Paul Diefenbaugh");
 MODULE_DESCRIPTION("ACPI Processor Driver");
 MODULE_LICENSE("GPL");
 
-static int acpi_processor_start(struct device *dev);
 static int acpi_processor_stop(struct device *dev);
 
 static const struct acpi_device_id processor_device_ids[] = {
@@ -47,7 +46,6 @@  static struct device_driver acpi_processor_driver = {
 	.name = "processor",
 	.bus = &cpu_subsys,
 	.acpi_match_table = processor_device_ids,
-	.probe = acpi_processor_start,
 	.remove = acpi_processor_stop,
 };
 
@@ -115,12 +113,10 @@  static int acpi_soft_cpu_online(unsigned int cpu)
 	 * CPU got physically hotplugged and onlined for the first time:
 	 * Initialize missing things.
 	 */
-	if (pr->flags.need_hotplug_init) {
+	if (!pr->flags.previously_online) {
 		int ret;
 
-		pr_info("Will online and init hotplugged CPU: %d\n",
-			pr->id);
-		pr->flags.need_hotplug_init = 0;
+		pr_info("Will online and init CPU: %d\n", pr->id);
 		ret = __acpi_processor_start(device);
 		WARN(ret, "Failed to start CPU: %d\n", pr->id);
 	} else {
@@ -167,9 +163,6 @@  static int __acpi_processor_start(struct acpi_device *device)
 	if (!pr)
 		return -ENODEV;
 
-	if (pr->flags.need_hotplug_init)
-		return 0;
-
 	result = acpi_cppc_processor_probe(pr);
 	if (result && !IS_ENABLED(CONFIG_ACPI_CPU_FREQ_PSS))
 		dev_dbg(&device->dev, "CPPC data invalid or not present\n");
@@ -185,32 +178,21 @@  static int __acpi_processor_start(struct acpi_device *device)
 
 	status = acpi_install_notify_handler(device->handle, ACPI_DEVICE_NOTIFY,
 					     acpi_processor_notify, device);
-	if (ACPI_SUCCESS(status))
-		return 0;
+	if (!ACPI_SUCCESS(status)) {
+		result = -ENODEV;
+		goto err_thermal_exit;
+	}
+	pr->flags.previously_online = 1;
 
-	result = -ENODEV;
-	acpi_processor_thermal_exit(pr, device);
+	return 0;
 
+err_thermal_exit:
+	acpi_processor_thermal_exit(pr, device);
 err_power_exit:
 	acpi_processor_power_exit(pr);
 	return result;
 }
 
-static int acpi_processor_start(struct device *dev)
-{
-	struct acpi_device *device = ACPI_COMPANION(dev);
-	int ret;
-
-	if (!device)
-		return -ENODEV;
-
-	/* Protect against concurrent CPU hotplug operations */
-	cpu_hotplug_disable();
-	ret = __acpi_processor_start(device);
-	cpu_hotplug_enable();
-	return ret;
-}
-
 static int acpi_processor_stop(struct device *dev)
 {
 	struct acpi_device *device = ACPI_COMPANION(dev);
@@ -279,9 +261,9 @@  static int __init acpi_processor_driver_init(void)
 	if (result < 0)
 		return result;
 
-	result = cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN,
-					   "acpi/cpu-drv:online",
-					   acpi_soft_cpu_online, NULL);
+	result = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
+				   "acpi/cpu-drv:online",
+				   acpi_soft_cpu_online, NULL);
 	if (result < 0)
 		goto err;
 	hp_online = result;
diff --git a/include/acpi/processor.h b/include/acpi/processor.h
index 3f34ebb27525..e6f6074eadbf 100644
--- a/include/acpi/processor.h
+++ b/include/acpi/processor.h
@@ -217,7 +217,7 @@  struct acpi_processor_flags {
 	u8 has_lpi:1;
 	u8 power_setup_done:1;
 	u8 bm_rld_set:1;
-	u8 need_hotplug_init:1;
+	u8 previously_online:1;
 };
 
 struct acpi_processor {