Date: Thu, 11 Apr 2024 10:52:13 +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 3/4] ACPI: processor: refactor
 acpi_processor_get_info: isolate acpi_{map|unmap}_cpu under
 CONFIG_ACPI_HOTPLUG_CPU



> On 10 Apr 2024, at 19:44, Jonathan Cameron <jonathan.cameron@...wei.com> wrote:
> 
> On Wed, 10 Apr 2024 18:29:34 +0000
> Miguel Luis <miguel.luis@...cle.com> wrote:
> 
>>> On 10 Apr 2024, at 13:23, Jonathan Cameron <Jonathan.Cameron@...wei.com> wrote:
>>> 
>>> On Tue,  9 Apr 2024 15:05:32 +0000
>>> Miguel Luis <miguel.luis@...cle.com> 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?
> 
> I'm not sure what you mean by dismisses?
> 

I mean when acpi_map_cpu is not needed.

> Is missing perhaps?

Yes.

>  If that is what you mean, I think it's a mistake to allow
> that code to be called from a path that isn't dependent on
> CONFIG_ACPI_HOTPLUG_CPU.
> It makes no sense to do so and stubbing it out to give
> the impression that the calling it does make sense (when looking at the caller)
> is misleading.

OK, that would be what not to do.

acpi_processor_enumerate_extra could deal with make_present and make_enabled while
a stub would still be needed for make_present since it depends on
CONFIG_ACPI_HOTPLUG_CPU?

Miguel

> 
> Jonathan
> 
> 
>> 
>> 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@...cle.com>
>>>> ---
>>>> 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;
>>>> }
>>>> 
>>> 
>> 
> 

Powered by blists - more mailing lists