linux-kernel - Re: [RFC PATCH 4/4] ACPI: processor: refactor acpi_processor_remove: isolate acpi_unmap_cpu under CONFIG_ACPI_HOTPLUG

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <25259BC7-8CDB-4714-AEDE-CFFD14986148@oracle.com>
Date: Thu, 11 Apr 2024 11:02:37 +0000
From: Miguel Luis <miguel.luis@...cle.com>
To: Jonathan Cameron <Jonathan.Cameron@...wei.com>
CC: "Rafael J. Wysocki" <rafael@...nel.org>, Len Brown <lenb@...nel.org>,
        "linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "rmk+kernel@...linux.org.uk" <rmk+kernel@...linux.org.uk>
Subject: Re: [RFC PATCH 4/4] ACPI: processor: refactor acpi_processor_remove:
 isolate acpi_unmap_cpu under CONFIG_ACPI_HOTPLUG_CPU



> On 10 Apr 2024, at 13:31, Jonathan Cameron <Jonathan.Cameron@...wei.com> wrote:
> 
> On Tue,  9 Apr 2024 15:05:33 +0000
> Miguel Luis <miguel.luis@...cle.com> 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@...cle.com>
> 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,
>> },
> 
>