Received-SPF: pass (google.com: domain of linux-kernel+bounces-139270-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Wed, 10 Apr 2024 20:44:15 +0100
From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
To: Miguel Luis <miguel.luis@oracle.com>
CC: "Rafael J. Wysocki" <rafael@kernel.org>, Len Brown <lenb@kernel.org>,
	"linux-acpi@vger.kernel.org" <linux-acpi@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"rmk+kernel@armlinux.org.uk" <rmk+kernel@armlinux.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
Message-ID: <20240410204415.00005597@Huawei.com>
In-Reply-To: <AC422669-2869-4C05-B8CD-4C94BDE24012@oracle.com>
References: <20240409150536.9933-1-miguel.luis@oracle.com>
	<20240409150536.9933-4-miguel.luis@oracle.com>
	<20240410142359.00003dea@Huawei.com>
	<AC422669-2869-4C05-B8CD-4C94BDE24012@oracle.com>
Organization: Huawei Technologies Research and Development (UK) Ltd.
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable

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:
> >=20
> > On Tue,  9 Apr 2024 15:05:32 +0000
> > Miguel Luis <miguel.luis@oracle.com> wrote:
> >  =20
> >> 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. =20
> >=20
> > Should we consider renaming acpi_map_cpu() to arch_acpi_map_cpu()
> > to make the arch specific nature of that call more obvious? =20
>=20
> 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.
>=20
> Question is what to do when there=E2=80=99s a use-case which dismisses ac=
pi_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 al=
low
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 cal=
ler)
is misleading.

Jonathan


>=20
> 1) export it and do nothing - it would be creating unnecessary dependency.
>=20
> 2) evaluate whether calling it is exclusive to the CPU HP path and keep i=
t wrapped
> into CONFIG_ACPI_HOTPLUG_CPU.
>=20
> Option (2) is the current approach on this RFC. IIUC acpi_map_cpu is sole=
ly
> used for CPU HP and the same applies to acpi_unmap_cpu.
>=20
> > I think that has caused more confusion in the discussion than
> > whether it is hotplug specific or not. =20
>=20
> Indeed. Within the CPU HP path there are these arch specific intricacies.
>=20
> >=20
> > As mentioned in patch 2, fairly sure this needs to go before that
> > patch. =20
>=20
> 2 and 3 depend on each to be self-contained as CPU HP wouldn=E2=80=99t wo=
rk without late
> CPU initialisation I think.
>=20
> Miguel
>=20
> >=20
> > Jonathan
> >  =20
> >>=20
> >> Signed-off-by: Miguel Luis <miguel.luis@oracle.com>
> >> ---
> >> drivers/acpi/acpi_processor.c | 17 +++++++++++++++--
> >> 1 file changed, 15 insertions(+), 2 deletions(-)
> >>=20
> >> diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_process=
or.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(str=
uct acpi_processor *pr)
> >> pr_info("CPU%d has been hot-added\n", pr->id);
> >> pr->flags.need_hotplug_init =3D 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 *p=
r)
> >> +{
> >> + acpi_unmap_cpu(pr->id);
> >> +}
> >> #else
> >> static void acpi_processor_hotplug_delay_init(struct acpi_processor *p=
r) {}
> >> +static int acpi_processor_hotplug_map_cpu(struct acpi_processor *pr)
> >> +{
> >> + return 0;
> >> +}
> >> +static void acpi_processor_hotplug_unmap_cpu(struct acpi_processor *p=
r) {}
> >> #endif /* CONFIG_ACPI_HOTPLUG_CPU */
> >>=20
> >> /* Enumerate extra CPUs */
> >> @@ -215,13 +228,13 @@ static int acpi_processor_enumerate_extra(struct=
 acpi_processor *pr)
> >> cpu_maps_update_begin();
> >> cpus_write_lock();
> >>=20
> >> - ret =3D acpi_map_cpu(pr->handle, pr->phys_id, pr->acpi_id, &pr->id);
> >> + ret =3D acpi_processor_hotplug_map_cpu(pr);
> >> if (ret)
> >> goto out;
> >>=20
> >> ret =3D arch_register_cpu(pr->id);
> >> if (ret) {
> >> - acpi_unmap_cpu(pr->id);
> >> + acpi_processor_hotplug_unmap_cpu(pr);
> >> goto out;
> >> }
> >>  =20
> >  =20
>=20