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

[Miguel Luis <miguel.luis@oracle.com>]

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 ?

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
>> 
>