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 11 Apr 2024, at 13:57, Jonathan Cameron <jonathan.cameron@xxxxxxxxxx> wrote:
> 
> On Thu, 11 Apr 2024 10:52:13 +0000
> Miguel Luis <miguel.luis@xxxxxxxxxx> wrote:
> 
>>> On 10 Apr 2024, at 19:44, Jonathan Cameron <jonathan.cameron@xxxxxxxxxx> wrote:
>>> 
>>> On Wed, 10 Apr 2024 18:29:34 +0000
>>> Miguel Luis <miguel.luis@xxxxxxxxxx> wrote:
>>> 
>>>>> 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?  
>>> 
>>> 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?
> 
> Sure, you could make it do that with a bunch of checks on the
> config being enabled, but currently I don't see the overlap in
> shared code as being sufficient for that to make sense.
> 
> The discussion before was assuming that things like the acpi_map_cpu
> calls might do stuff that is wanted in the make_enabled() case.
> 
> Given they don't do anything that we want there I don't see sharing
> the code as useful.
> 
> I am however in favor of renaming those hotplug only calls to something
> more meaningful so no one 'thinks' they may be relevant in the
> enabling only case!
> 
> Jonathan
> 
> p.s. I'm smashing the outputs of the thread with Rafael into a coherent
> patch set at the moment, perhaps seeing that will make it clearer what
> is going on.  I got distracted by fixing numa node handling this morning
> but that's now pushed out for a follow on series.
> 

Thanks! Looking forward to see v5.

Miguel

> 
>> 
>> 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@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;
>>>>>> }