RE: [PATCH v6 06/16] ACPI: processor: Register deferred CPUs from acpi_processor_get_info()

[Salil Mehta]

>  From: Jonathan Cameron <jonathan.cameron@xxxxxxxxxx>
>  Sent: Wednesday, April 17, 2024 2:19 PM
>  
>  From: James Morse <james.morse@xxxxxxx>
>  
>  The arm64 specific arch_register_cpu() call may defer CPU registration until
>  the ACPI interpreter is available and the _STA method can be evaluated.
>  
>  If this occurs, then a second attempt is made in acpi_processor_get_info().
>  Note that the arm64 specific call has not yet been added so for now this will
>  be called for the original hotplug case.
>  
>  For architectures that do not defer until the ACPI Processor driver loads
>  (e.g. x86), for initially present CPUs there will already be a CPU device. If
>  present do not try to register again.
>  
>  Systems can still be booted with 'acpi=off', or not include an ACPI
>  description at all as in these cases arch_register_cpu() will not have
>  deferred registration when first called.
>  
>  This moves the CPU register logic back to a subsys_initcall(), while the
>  memory nodes will have been registered earlier.
>  Note this is where the call was prior to the cleanup series so there should be
>  no side effects of moving it back again for this specific case.
>  
>  [PATCH 00/21] Initial cleanups for vCPU HP.
>  https://lore.kernel.org/all/ZVyz%2FVe5pPu8AWoA@xxxxxxxxxxxxxxxxxxxxx/
>  
>  e.g. 5b95f94c3b9f ("x86/topology: Switch over to GENERIC_CPU_DEVICES")
>  
>  Signed-off-by: James Morse <james.morse@xxxxxxx>
>  Reviewed-by: Gavin Shan <gshan@xxxxxxxxxx>
>  Tested-by: Miguel Luis <miguel.luis@xxxxxxxxxx>
>  Tested-by: Vishnu Pajjuri <vishnu@xxxxxxxxxxxxxxxxxxxxxx>
>  Tested-by: Jianyong Wu <jianyong.wu@xxxxxxx>
>  Signed-off-by: Russell King (Oracle) <rmk+kernel@xxxxxxxxxxxxxxx>
>  Co-developed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
>  Signed-off-by: Joanthan Cameron <Jonathan.Cameron@xxxxxxxxxx>
>  ---
>  v6: Squash the two paths for conventional CPU Hotplug and arm64
>      vCPU HP.
>  v5: Update commit message to make it clear this is moving the
>      init back to where it was until very recently.
>  
>      No longer change the condition in the earlier registration point
>      as that will be handled by the arm64 registration routine
>      deferring until called again here.
>  ---
>   drivers/acpi/acpi_processor.c | 12 +++++++++++-
>   1 file changed, 11 insertions(+), 1 deletion(-)
>  
>  diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processorc
>  index 7ecb13775d7f..0cac77961020 100644
>  --- a/drivers/acpi/acpi_processor.c
>  +++ b/drivers/acpi/acpi_processor.c
>  @@ -356,8 +356,18 @@ static int acpi_processor_get_info(struct
>  acpi_device *device)
>   	 *
>   	 *  NOTE: Even if the processor has a cpuid, it may not be present
>   	 *  because cpuid <-> apicid mapping is persistent now.
>  +	 *
>  +	 *  Note this allows 3 flows, it is up to the arch_register_cpu()
>  +	 *  call to reject any that are not supported on a given architecture.
>  +	 *  A) CPU becomes present.
>  +	 *  B) Previously invalid logical CPU ID (Same as becoming present)
>  +	 *  C) CPU already present and now being enabled (and wasn't
>  registered
>  +	 *     early on an arch that doesn't defer to here)
>   	 */
>  -	if (invalid_logical_cpuid(pr->id) || !cpu_present(pr->id)) {
>  +	if ((!invalid_logical_cpuid(pr->id) && cpu_present(pr->id) &&
>  +	     !get_cpu_device(pr->id)) ||
>  +	    invalid_logical_cpuid(pr->id) ||
>  +	    !cpu_present(pr->id)) {


Logic is clear but it is ugly. We should turn them into macro or inline.


Thanks
Salil.