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