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