[RFC,v3,04/21] ACPI: processor: Register all CPUs from acpi_processor_get_info()

Commit Message

Russell King (Oracle) Dec. 13, 2023, 12:49 p.m. UTC

From: James Morse <[e-mail]>

To allow ACPI to skip the call to arch_register_cpu() when the _STA
value indicates the CPU can't be brought online right now, move the
arch_register_cpu() call into acpi_processor_get_info().

Systems can still be booted with 'acpi=off', or not include an
ACPI description at all. For these, the CPUs continue to be
registered by cpu_dev_register_generic().

This moves the CPU register logic back to a subsys_initcall(),
while the memory nodes will have been registered earlier.

Signed-off-by: James Morse <[e-mail]>
Reviewed-by: Gavin Shan <[e-mail]>
Tested-by: Miguel Luis <[e-mail]>
Tested-by: Vishnu Pajjuri <[e-mail]>
Tested-by: Jianyong Wu <[e-mail]>
Signed-off-by: Russell King (Oracle) <[e-mail]>
---
Changes since RFC v2:
 * Fixup comment in acpi_processor_get_info() (Gavin Shan)
 * Add comment in cpu_dev_register_generic() (Gavin Shan)
---
 drivers/acpi/acpi_processor.c | 12 ++++++++++++
 drivers/base/cpu.c            |  6 +++++-
 2 files changed, 17 insertions(+), 1 deletion(-)

Comments

Jonathan Cameron Dec. 14, 2023, 5:38 p.m. UTC | #1

On Wed, 13 Dec 2023 12:49:31 +0000
Russell King (Oracle) <[e-mail]> wrote:

> From: James Morse <[e-mail]>
> 
> To allow ACPI to skip the call to arch_register_cpu() when the _STA
> value indicates the CPU can't be brought online right now, move the
> arch_register_cpu() call into acpi_processor_get_info().
> 
> Systems can still be booted with 'acpi=off', or not include an
> ACPI description at all. For these, the CPUs continue to be
> registered by cpu_dev_register_generic().
> 
> This moves the CPU register logic back to a subsys_initcall(),
> while the memory nodes will have been registered earlier.
> 
> Signed-off-by: James Morse <[e-mail]>
> Reviewed-by: Gavin Shan <[e-mail]>
> Tested-by: Miguel Luis <[e-mail]>
> Tested-by: Vishnu Pajjuri <[e-mail]>
> Tested-by: Jianyong Wu <[e-mail]>
> Signed-off-by: Russell King (Oracle) <[e-mail]>
LGTM as well.

Reviewed-by: Jonathan Cameron <[e-mail]>

Rafael J. Wysocki Dec. 18, 2023, 8:30 p.m. UTC | #2

On Wed, Dec 13, 2023 at 1:49 PM Russell King <[e-mail]> wrote:
>
> From: James Morse <[e-mail]>
>
> To allow ACPI to skip the call to arch_register_cpu() when the _STA
> value indicates the CPU can't be brought online right now, move the
> arch_register_cpu() call into acpi_processor_get_info().

This kind of looks backwards to me and has a potential to become
super-confusing.

I would instead add a way for the generic code to ask the platform
firmware whether or not the given CPU is enabled and so it can be
registered.

> Systems can still be booted with 'acpi=off', or not include an
> ACPI description at all. For these, the CPUs continue to be
> registered by cpu_dev_register_generic().
>
> This moves the CPU register logic back to a subsys_initcall(),
> while the memory nodes will have been registered earlier.

Isn't this somewhat risky?

> Signed-off-by: James Morse <[e-mail]>
> Reviewed-by: Gavin Shan <[e-mail]>
> Tested-by: Miguel Luis <[e-mail]>
> Tested-by: Vishnu Pajjuri <[e-mail]>
> Tested-by: Jianyong Wu <[e-mail]>
> Signed-off-by: Russell King (Oracle) <[e-mail]>
> ---
> Changes since RFC v2:
>  * Fixup comment in acpi_processor_get_info() (Gavin Shan)
>  * Add comment in cpu_dev_register_generic() (Gavin Shan)
> ---
>  drivers/acpi/acpi_processor.c | 12 ++++++++++++
>  drivers/base/cpu.c            |  6 +++++-
>  2 files changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
> index 0511f2bc10bc..e7ed4730cbbe 100644
> --- a/drivers/acpi/acpi_processor.c
> +++ b/drivers/acpi/acpi_processor.c
> @@ -314,6 +314,18 @@ static int acpi_processor_get_info(struct acpi_device *device)
>                         cpufreq_add_device("acpi-cpufreq");
>         }
>
> +       /*
> +        * Register CPUs that are present. get_cpu_device() is used to skip
> +        * duplicate CPU descriptions from firmware.
> +        */
> +       if (!invalid_logical_cpuid(pr->id) && cpu_present(pr->id) &&
> +           !get_cpu_device(pr->id)) {
> +               int ret = arch_register_cpu(pr->id);
> +
> +               if (ret)
> +                       return ret;
> +       }
> +
>         /*
>          *  Extra Processor objects may be enumerated on MP systems with
>          *  less than the max # of CPUs. They should be ignored _iff
> diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c
> index 47de0f140ba6..13d052bf13f4 100644
> --- a/drivers/base/cpu.c
> +++ b/drivers/base/cpu.c
> @@ -553,7 +553,11 @@ static void __init cpu_dev_register_generic(void)
>  {
>         int i, ret;
>
> -       if (!IS_ENABLED(CONFIG_GENERIC_CPU_DEVICES))
> +       /*
> +        * When ACPI is enabled, CPUs are registered via
> +        * acpi_processor_get_info().
> +        */
> +       if (!IS_ENABLED(CONFIG_GENERIC_CPU_DEVICES) || !acpi_disabled)
>                 return;
>
>         for_each_present_cpu(i) {
> --
> 2.30.2
>
>

Jonathan Cameron Jan. 22, 2024, 5:44 p.m. UTC | #3

On Mon, 18 Dec 2023 21:30:50 +0100
"Rafael J. Wysocki" <[e-mail]> wrote:

> On Wed, Dec 13, 2023 at 1:49 PM Russell King <[e-mail]> wrote:
> >
> > From: James Morse <[e-mail]>
> >
> > To allow ACPI to skip the call to arch_register_cpu() when the _STA
> > value indicates the CPU can't be brought online right now, move the
> > arch_register_cpu() call into acpi_processor_get_info().  
> 
> This kind of looks backwards to me and has a potential to become
> super-confusing.
> 
> I would instead add a way for the generic code to ask the platform
> firmware whether or not the given CPU is enabled and so it can be
> registered.

Hi Rafael,

The ACPI interpreter isn't up at this stage so we'd need to pull that
forwards. I'm not sure if we can pull the interpreter init early enough.

Perhaps pushing the registration back in all cases is the way to go?
Given the acpi interpretter is initialized via subsys_initcall() it would
need to be after that - I tried pushing cpu_dev_register_generic()
immediately after acpi_bus_init() and that seems fine.
We can't leave the rest of cpu_dev_init() that late because a bunch
of other stuff relies on it (CPU freq blows up first as a core_init()
on my setup).

So to make this work we need it to always move the registration later
than the necessary infrastructure, perhaps to subsys_initcall_sync()
as is done for missing CPUs (we'd need to combine the two given that
needs to run after this, or potentially just stop checking for acpi_disabled
and don't taint the kernel!).  I think this is probably the most consistent
option on basis it at least moves the registration to the same point
whatever is going on and can easily use the arch callback you suggest
to hide away the logic on deciding if a CPU is there or not.

What do you think is the best way to do this?


> 
> > Systems can still be booted with 'acpi=off', or not include ano
> > ACPI description at all. For these, the CPUs continue to be
> > registered by cpu_dev_register_generic().
> >
> > This moves the CPU register logic back to a subsys_initcall(),
> > while the memory nodes will have been registered earlier.  
> 
> Isn't this somewhat risky?
> 
> > Signed-off-by: James Morse <[e-mail]>
> > Reviewed-by: Gavin Shan <[e-mail]>
> > Tested-by: Miguel Luis <[e-mail]>
> > Tested-by: Vishnu Pajjuri <[e-mail]>
> > Tested-by: Jianyong Wu <[e-mail]>
> > Signed-off-by: Russell King (Oracle) <[e-mail]>
> > ---
> > Changes since RFC v2:
> >  * Fixup comment in acpi_processor_get_info() (Gavin Shan)
> >  * Add comment in cpu_dev_register_generic() (Gavin Shan)
> > ---
> >  drivers/acpi/acpi_processor.c | 12 ++++++++++++
> >  drivers/base/cpu.c            |  6 +++++-
> >  2 files changed, 17 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
> > index 0511f2bc10bc..e7ed4730cbbe 100644
> > --- a/drivers/acpi/acpi_processor.c
> > +++ b/drivers/acpi/acpi_processor.c
> > @@ -314,6 +314,18 @@ static int acpi_processor_get_info(struct acpi_device *device)
> >                         cpufreq_add_device("acpi-cpufreq");
> >         }
> >
> > +       /*
> > +        * Register CPUs that are present. get_cpu_device() is used to skip
> > +        * duplicate CPU descriptions from firmware.
> > +        */
> > +       if (!invalid_logical_cpuid(pr->id) && cpu_present(pr->id) &&
> > +           !get_cpu_device(pr->id)) {
> > +               int ret = arch_register_cpu(pr->id);
> > +
> > +               if (ret)
> > +                       return ret;
> > +       }
> > +
> >         /*
> >          *  Extra Processor objects may be enumerated on MP systems with
> >          *  less than the max # of CPUs. They should be ignored _iff
> > diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c
> > index 47de0f140ba6..13d052bf13f4 100644
> > --- a/drivers/base/cpu.c
> > +++ b/drivers/base/cpu.c
> > @@ -553,7 +553,11 @@ static void __init cpu_dev_register_generic(void)
> >  {
> >         int i, ret;
> >
> > -       if (!IS_ENABLED(CONFIG_GENERIC_CPU_DEVICES))
> > +       /*
> > +        * When ACPI is enabled, CPUs are registered via
> > +        * acpi_processor_get_info().
> > +        */
> > +       if (!IS_ENABLED(CONFIG_GENERIC_CPU_DEVICES) || !acpi_disabled)
> >                 return;
> >
> >         for_each_present_cpu(i) {
> > --
> > 2.30.2
> >
> >  
> 
> _______________________________________________
> linux-arm-kernel mailing list
> [e-mail]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Rafael J. Wysocki Jan. 22, 2024, 6:03 p.m. UTC | #4

On Mon, Jan 22, 2024 at 6:44 PM Jonathan Cameron
<[e-mail]> wrote:
>
> On Mon, 18 Dec 2023 21:30:50 +0100
> "Rafael J. Wysocki" <[e-mail]> wrote:
>
> > On Wed, Dec 13, 2023 at 1:49 PM Russell King <[e-mail]> wrote:
> > >
> > > From: James Morse <[e-mail]>
> > >
> > > To allow ACPI to skip the call to arch_register_cpu() when the _STA
> > > value indicates the CPU can't be brought online right now, move the
> > > arch_register_cpu() call into acpi_processor_get_info().
> >
> > This kind of looks backwards to me and has a potential to become
> > super-confusing.
> >
> > I would instead add a way for the generic code to ask the platform
> > firmware whether or not the given CPU is enabled and so it can be
> > registered.
>
> Hi Rafael,
>
> The ACPI interpreter isn't up at this stage so we'd need to pull that
> forwards. I'm not sure if we can pull the interpreter init early enough.

Well, this patch effectively defers the AP registration to the time
when acpi_processor_get_info() runs and the interpreter is up and
running then.

For consistency, it would be better to defer the AP registration in
general to that point.

> Perhaps pushing the registration back in all cases is the way to go?
> Given the acpi interpretter is initialized via subsys_initcall() it would
> need to be after that - I tried pushing cpu_dev_register_generic()
> immediately after acpi_bus_init() and that seems fine.

Sounds promising.

> We can't leave the rest of cpu_dev_init() that late because a bunch
> of other stuff relies on it (CPU freq blows up first as a core_init()
> on my setup).

I see.

> So to make this work we need it to always move the registration later
> than the necessary infrastructure, perhaps to subsys_initcall_sync()
> as is done for missing CPUs (we'd need to combine the two given that
> needs to run after this, or potentially just stop checking for acpi_disabled
> and don't taint the kernel!).  I think this is probably the most consistent
> option on basis it at least moves the registration to the same point
> whatever is going on and can easily use the arch callback you suggest
> to hide away the logic on deciding if a CPU is there or not.

I agree.

Russell King (Oracle) Jan. 22, 2024, 9:56 p.m. UTC | #5

On Mon, Dec 18, 2023 at 09:30:50PM +0100, Rafael J. Wysocki wrote:
> On Wed, Dec 13, 2023 at 1:49 PM Russell King <[e-mail]> wrote:
> > Systems can still be booted with 'acpi=off', or not include an
> > ACPI description at all. For these, the CPUs continue to be
> > registered by cpu_dev_register_generic().
> >
> > This moves the CPU register logic back to a subsys_initcall(),
> > while the memory nodes will have been registered earlier.
> 
> Isn't this somewhat risky?

Not really. The earlier full series moved the registration of CPUs
from subsys (by the various architecture specific code) into the
driver core - thus moving it much earlier. This patch merely
restores it back to the subsys initialisation stage.

There is a low chance that something _could_ have become reliant
on that change - and the longer it takes for this change to be
merged, the more risk there is that something could become reliant
on CPUs being registered very early.

Maybe this ought to be spelt out more explicitly that it's merely
restoring the point at which CPUs are registered.

Patch

diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
index 0511f2bc10bc..e7ed4730cbbe 100644
--- a/drivers/acpi/acpi_processor.c
+++ b/drivers/acpi/acpi_processor.c
@@ -314,6 +314,18 @@  static int acpi_processor_get_info(struct acpi_device *device)
 			cpufreq_add_device("acpi-cpufreq");
 	}
 
+	/*
+	 * Register CPUs that are present. get_cpu_device() is used to skip
+	 * duplicate CPU descriptions from firmware.
+	 */
+	if (!invalid_logical_cpuid(pr->id) && cpu_present(pr->id) &&
+	    !get_cpu_device(pr->id)) {
+		int ret = arch_register_cpu(pr->id);
+
+		if (ret)
+			return ret;
+	}
+
 	/*
 	 *  Extra Processor objects may be enumerated on MP systems with
 	 *  less than the max # of CPUs. They should be ignored _iff
diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c
index 47de0f140ba6..13d052bf13f4 100644
--- a/drivers/base/cpu.c
+++ b/drivers/base/cpu.c
@@ -553,7 +553,11 @@  static void __init cpu_dev_register_generic(void)
 {
 	int i, ret;
 
-	if (!IS_ENABLED(CONFIG_GENERIC_CPU_DEVICES))
+	/*
+	 * When ACPI is enabled, CPUs are registered via
+	 * acpi_processor_get_info().
+	 */
+	if (!IS_ENABLED(CONFIG_GENERIC_CPU_DEVICES) || !acpi_disabled)
 		return;
 
 	for_each_present_cpu(i) {