Re: [RFC PATCH 3/4] ACPI: processor: refactor acpi_processor_get_info: isolate acpi_{map|unmap}_cpu under CONFIG_ACPI_HOTPLUG_CPU

[Miguel Luis]

> On 10 Apr 2024, at 13:23, Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx> wrote:
> 
> On Tue,  9 Apr 2024 15:05:32 +0000
> Miguel Luis <miguel.luis@xxxxxxxxxx> wrote:
> 
>> mapping and unmaping a cpu at the stage of extra cpu enumeration is
>> architecture specific which depends on CONFIG_ACPI_HOTPLUG_CPU so let's
>> isolate that functionality from architecture independent one.
> 
> Should we consider renaming acpi_map_cpu() to arch_acpi_map_cpu()
> to make the arch specific nature of that call more obvious?

Not sure about the pattern to use here but that seems fine to me. Current usage
is architectures export acpi_map_cpu from the acpi interface and do their
thing.

Question is what to do when there’s a use-case which dismisses acpi_map_cpu and
it gets called on the code path?

1) export it and do nothing - it would be creating unnecessary dependency.

2) evaluate whether calling it is exclusive to the CPU HP path and keep it wrapped
into CONFIG_ACPI_HOTPLUG_CPU.

Option (2) is the current approach on this RFC. IIUC acpi_map_cpu is solely
used for CPU HP and the same applies to acpi_unmap_cpu.

> I think that has caused more confusion in the discussion than
> whether it is hotplug specific or not.

Indeed. Within the CPU HP path there are these arch specific intricacies.

> 
> As mentioned in patch 2, fairly sure this needs to go before that
> patch.

2 and 3 depend on each to be self-contained as CPU HP wouldn’t work without late
CPU initialisation I think.

Miguel

> 
> Jonathan
> 
>> 
>> Signed-off-by: Miguel Luis <miguel.luis@xxxxxxxxxx>
>> ---
>> drivers/acpi/acpi_processor.c | 17 +++++++++++++++--
>> 1 file changed, 15 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
>> index 9ea58b61d741..c6e2f64a056b 100644
>> --- a/drivers/acpi/acpi_processor.c
>> +++ b/drivers/acpi/acpi_processor.c
>> @@ -194,8 +194,21 @@ static void acpi_processor_hotplug_delay_init(struct acpi_processor *pr)
>> pr_info("CPU%d has been hot-added\n", pr->id);
>> pr->flags.need_hotplug_init = 1;
>> }
>> +static int acpi_processor_hotplug_map_cpu(struct acpi_processor *pr)
>> +{
>> + return acpi_map_cpu(pr->handle, pr->phys_id, pr->acpi_id, &pr->id);
>> +}
>> +static void acpi_processor_hotplug_unmap_cpu(struct acpi_processor *pr)
>> +{
>> + acpi_unmap_cpu(pr->id);
>> +}
>> #else
>> static void acpi_processor_hotplug_delay_init(struct acpi_processor *pr) {}
>> +static int acpi_processor_hotplug_map_cpu(struct acpi_processor *pr)
>> +{
>> + return 0;
>> +}
>> +static void acpi_processor_hotplug_unmap_cpu(struct acpi_processor *pr) {}
>> #endif /* CONFIG_ACPI_HOTPLUG_CPU */
>> 
>> /* Enumerate extra CPUs */
>> @@ -215,13 +228,13 @@ static int acpi_processor_enumerate_extra(struct acpi_processor *pr)
>> cpu_maps_update_begin();
>> cpus_write_lock();
>> 
>> - ret = acpi_map_cpu(pr->handle, pr->phys_id, pr->acpi_id, &pr->id);
>> + ret = acpi_processor_hotplug_map_cpu(pr);
>> if (ret)
>> goto out;
>> 
>> ret = arch_register_cpu(pr->id);
>> if (ret) {
>> - acpi_unmap_cpu(pr->id);
>> + acpi_processor_hotplug_unmap_cpu(pr);
>> goto out;
>> }
>> 
>