Messages in this thread | ![/](/images/icornerl.gif) | | From | Miguel Luis <> | Subject | Re: [RFC PATCH 3/4] ACPI: processor: refactor acpi_processor_get_info: isolate acpi_{map|unmap}_cpu under CONFIG_ACPI_HOTPLUG_CPU | Date | Wed, 10 Apr 2024 18:29:34 +0000 |
| |
> 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?
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; >> } >> >
| ![\](/images/icornerr.gif) |