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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240410204415.00005597@Huawei.com>
Date: Wed, 10 Apr 2024 20:44:15 +0100
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: Miguel Luis <miguel.luis@...cle.com>
CC: "Rafael J. Wysocki" <rafael@...nel.org>, Len Brown <lenb@...nel.org>,
	"linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"rmk+kernel@...linux.org.uk" <rmk+kernel@...linux.org.uk>
Subject: Re: [RFC PATCH 3/4] ACPI: processor: refactor
 acpi_processor_get_info: isolate acpi_{map|unmap}_cpu under
 CONFIG_ACPI_HOTPLUG_CPU

On Wed, 10 Apr 2024 18:29:34 +0000
Miguel Luis <miguel.luis@...cle.com> wrote:

> > On 10 Apr 2024, at 13:23, Jonathan Cameron <Jonathan.Cameron@...wei.com> wrote:
> > 
> > On Tue,  9 Apr 2024 15:05:32 +0000
> > Miguel Luis <miguel.luis@...cle.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@...cle.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;
> >> }
> >>   
> >   
>