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

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Mon, 18 Dec 2023 21:30:50 +0100
"Rafael J. Wysocki" <rafael@kernel.org> wrote:

> On Wed, Dec 13, 2023 at 1:49 PM Russell King <rmk+kernel@armlinux.org.uk> wrote:
> >
> > From: James Morse <james.morse@arm.com>
> >
> > 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 <james.morse@arm.com>
> > Reviewed-by: Gavin Shan <gshan@redhat.com>
> > Tested-by: Miguel Luis <miguel.luis@oracle.com>
> > Tested-by: Vishnu Pajjuri <vishnu@os.amperecomputing.com>
> > Tested-by: Jianyong Wu <jianyong.wu@arm.com>
> > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > ---
> > 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
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel