Re: [RFC PATCH 3/4] ACPI: processor: refactor acpi_processor_get_info: isolate acpi_{map|unmap}_cpu under CONFIG_ACPI_HOTPLUG_CPU

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Thu, 11 Apr 2024 10:52:13 +0000
Miguel Luis <miguel.luis@oracle.com> wrote:

> > On 10 Apr 2024, at 19:44, Jonathan Cameron <jonathan.cameron@huawei.com> wrote:
> > 
> > On Wed, 10 Apr 2024 18:29:34 +0000
> > Miguel Luis <miguel.luis@oracle.com> wrote:
> >   
> >>> On 10 Apr 2024, at 13:23, Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> >>> 
> >>> On Tue,  9 Apr 2024 15:05:32 +0000
> >>> Miguel Luis <miguel.luis@oracle.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?

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.


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