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

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Fri, 26 Apr 2024 18:49:49 +0100
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:

> On Fri, 26 Apr 2024 17:21:41 +0000
> Miguel Luis <miguel.luis@oracle.com> wrote:
> 
> > Hi Jonathan, 
> >   
> > > On 26 Apr 2024, at 16:05, Miguel Luis <miguel.luis@oracle.com> wrote:
> > > 
> > > 
> > >     
> > >> On 26 Apr 2024, at 13:51, Jonathan Cameron <Jonathan.Cameron@huawei.com> 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 <rafael.j.wysocki@intel.com>
> > >> Reviewed-by: Hanjun Guo <guohanjun@huawei.com>
> > >> Tested-by: Miguel Luis <miguel.luis@oracle.com>
> > >> Reviewed-by: Gavin Shan <gshan@redhat.com>
> > >> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > >> 
> > >> ---
> > >> 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 <miguel.luis@oracle.com>
> > > 
> > > Miguel
> > >     
> > >> };
> > >> 
> > >> struct acpi_processor {
> > >> -- 
> > >> 2.39.2
> > >>     
> > >     
> >   
>