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

Is missing perhaps?  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.

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