Re: [RFC PATCH v2 06/35] arm64: setup: Switch over to GENERIC_CPU_DEVICES using arch_register_cpu()

From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
To: James Morse <james.morse@arm.com>
Cc: <linux-pm@vger.kernel.org>, <loongarch@lists.linux.dev>,
	<linux-acpi@vger.kernel.org>, <linux-arch@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>,
	<linux-arm-kernel@lists.infradead.org>,
	<linux-riscv@lists.infradead.org>, <kvmarm@lists.linux.dev>,
	<x86@kernel.org>, Salil Mehta <salil.mehta@huawei.com>,
	Russell King <linux@armlinux.org.uk>,
	Jean-Philippe Brucker <jean-philippe@linaro.org>,
	<jianyong.wu@arm.com>, <justin.he@arm.com>
Subject: Re: [RFC PATCH v2 06/35] arm64: setup: Switch over to GENERIC_CPU_DEVICES using arch_register_cpu()
Date: Thu, 14 Sep 2023 12:27:15 +0100	[thread overview]
Message-ID: <20230914122715.000076be@Huawei.com> (raw)
In-Reply-To: <20230913163823.7880-7-james.morse@arm.com>

On Wed, 13 Sep 2023 16:37:54 +0000
James Morse <james.morse@arm.com> wrote:

> To allow ACPI's _STA value to hide CPUs that are present, but not
> available to online right now due to VMM or firmware policy, the
> register_cpu() call needs to be made by the ACPI machinery when ACPI
> is in use. This allows it to hide CPUs that are unavailable from sysfs.
> 
> Switching to GENERIC_CPU_DEVICES is an intermediate step to allow all
> five ACPI architectures to be modified at once.
> 
> Switch over to GENERIC_CPU_DEVICES, and provide an arch_register_cpu()
> that populates the hotpluggable flag. arch_register_cpu() is also the
> interface the ACPI machinery expects.
> 
> The struct cpu in struct cpuinfo_arm64 is never used directly, remove
> it to use the one GENERIC_CPU_DEVICES provides.
> 
> This changes the CPUs visible in sysfs from possible to present, but
> on arm64 smp_prepare_cpus() ensures these are the same.
> 
> Signed-off-by: James Morse <james.morse@arm.com>

After this the earlier question about ordering of cpu_dev_init()
and node_dev_init() is relevant.   

Why won't node_dev_init() call
get_cpu_devce() which queries per_cpu(cpu_sys_devices)
and get NULL as we haven't yet filled that in?

Or does it do so but that doesn't matter as well create the
relevant links later?

I've not had enough coffee yet today so might be missing the
obvious!

Jonathan

> ---
>  arch/arm64/Kconfig           |  1 +
>  arch/arm64/include/asm/cpu.h |  1 -
>  arch/arm64/kernel/setup.c    | 13 ++++---------
>  3 files changed, 5 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index b10515c0200b..7b3990abf87a 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -132,6 +132,7 @@ config ARM64
>  	select GENERIC_ARCH_TOPOLOGY
>  	select GENERIC_CLOCKEVENTS_BROADCAST
>  	select GENERIC_CPU_AUTOPROBE
> +	select GENERIC_CPU_DEVICES
>  	select GENERIC_CPU_VULNERABILITIES
>  	select GENERIC_EARLY_IOREMAP
>  	select GENERIC_IDLE_POLL_SETUP
> diff --git a/arch/arm64/include/asm/cpu.h b/arch/arm64/include/asm/cpu.h
> index e749838b9c5d..887bd0d992bb 100644
> --- a/arch/arm64/include/asm/cpu.h
> +++ b/arch/arm64/include/asm/cpu.h
> @@ -38,7 +38,6 @@ struct cpuinfo_32bit {
>  };
>  
>  struct cpuinfo_arm64 {
> -	struct cpu	cpu;
>  	struct kobject	kobj;
>  	u64		reg_ctr;
>  	u64		reg_cntfrq;
> diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
> index 417a8a86b2db..165bd2c0dd5a 100644
> --- a/arch/arm64/kernel/setup.c
> +++ b/arch/arm64/kernel/setup.c
> @@ -402,19 +402,14 @@ static inline bool cpu_can_disable(unsigned int cpu)
>  	return false;
>  }
>  
> -static int __init topology_init(void)
> +int arch_register_cpu(int num)
>  {
> -	int i;
> +	struct cpu *cpu = &per_cpu(cpu_devices, num);
>  
> -	for_each_possible_cpu(i) {
> -		struct cpu *cpu = &per_cpu(cpu_data.cpu, i);
> -		cpu->hotpluggable = cpu_can_disable(i);
> -		register_cpu(cpu, i);
> -	}
> +	cpu->hotpluggable = cpu_can_disable(num);
>  
> -	return 0;
> +	return register_cpu(cpu, num);
>  }
> -subsys_initcall(topology_init);
>  
>  static void dump_kernel_offset(void)
>  {