Re: [RFC PATCH 4/4] ACPI: processor: refactor acpi_processor_remove: isolate acpi_unmap_cpu under CONFIG_ACPI_HOTPLUG_CPU

[Miguel Luis]

> On 10 Apr 2024, at 13:31, Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx> wrote:
> 
> On Tue,  9 Apr 2024 15:05:33 +0000
> Miguel Luis <miguel.luis@xxxxxxxxxx> wrote:
> 
>> acpi_unmap_cpu is architecture dependent. Isolate it.
>> The pre-processor guard for detach may now be restricted to
>> cpu unmap.
>> 
>> Signed-off-by: Miguel Luis <miguel.luis@xxxxxxxxxx>
> Again the why question isn't answered by the patch description.
> 
> I assume this is to try and resolve the remove question of releasing
> resources that was outstanding on vCPU HP v4 series Russell posted.
> 
> I've not looked as closely at the remove path as the add one yet, but
> my gut feeling is same issue applies.
> This code that runs in here should not be dependent on whether
> CONFIG_ACPI_HOTPLUG_CPU is enabled or not.

I agree.

>  What we do for the
> make disabled flow should not run a few of the steps in 
> acpi_processor_remove() we should make that clear by calling
> a different function that doesn't have those steps.
> 

Perhaps this got answered already elsewhere but is it OK for the detach handler
to be out of CONFIG_ACPI_HOTPLUG_CPU ?

Miguel

>> ---
>> drivers/acpi/acpi_processor.c | 13 +++++++++----
>> 1 file changed, 9 insertions(+), 4 deletions(-)
>> 
>> diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processorc
>> index c6e2f64a056b..edcd6a8d4735 100644
>> --- a/drivers/acpi/acpi_processor.c
>> +++ b/drivers/acpi/acpi_processor.c
>> @@ -492,6 +492,14 @@ static int acpi_processor_add(struct acpi_device *device,
>> }
>> 
>> #ifdef CONFIG_ACPI_HOTPLUG_CPU
>> +static void acpi_processor_hotunplug_unmap_cpu(struct acpi_processor *pr)
>> +{
>> + acpi_unmap_cpu(pr->id);
>> +}
>> +#else
>> +static void acpi_processor_hotunplug_unmap_cpu(struct acpi_processor *pr) {}
>> +#endif /* CONFIG_ACPI_HOTPLUG_CPU */
>> +
>> /* Removal */
>> static void acpi_processor_remove(struct acpi_device *device)
>> {
>> @@ -524,7 +532,7 @@ static void acpi_processor_remove(struct acpi_device *device)
>> 
>> /* Remove the CPU. */
>> arch_unregister_cpu(pr->id);
>> - acpi_unmap_cpu(pr->id);
>> + acpi_processor_hotunplug_unmap_cpu(pr);
>> 
>> cpus_write_unlock();
>> cpu_maps_update_done();
>> @@ -535,7 +543,6 @@ static void acpi_processor_remove(struct acpi_device *device)
>> free_cpumask_var(pr->throttling.shared_cpu_map);
>> kfree(pr);
>> }
>> -#endif /* CONFIG_ACPI_HOTPLUG_CPU */
>> 
>> #ifdef CONFIG_ARCH_MIGHT_HAVE_ACPI_PDC
>> bool __init processor_physically_present(acpi_handle handle)
>> @@ -660,9 +667,7 @@ static const struct acpi_device_id processor_device_ids[] = {
>> static struct acpi_scan_handler processor_handler = {
>> .ids = processor_device_ids,
>> .attach = acpi_processor_add,
>> -#ifdef CONFIG_ACPI_HOTPLUG_CPU
>> .detach = acpi_processor_remove,
>> -#endif
>> .hotplug = {
>> .enabled = true,
>> },
> 
>