Re: [PATCH RFC 06/22] drivers: base: Use present CPUs in GENERIC_CPU_DEVICES

Shaoqin Huang <shahuang@xxxxxxxxxx> · Thu, 9 Nov 2023 18:59:14 +0800

On 11/9/23 18:29, Russell King (Oracle) wrote:

On Thu, Nov 09, 2023 at 06:09:32PM +0800, Shaoqin Huang wrote:

Hi Russell,

On 11/7/23 18:29, Russell King (Oracle) wrote:

From: James Morse <james.morse@xxxxxxx>

Three of the five ACPI architectures create sysfs entries using
register_cpu() for present CPUs, whereas arm64, riscv and all
GENERIC_CPU_DEVICES do this for possible CPUs.

Registering a CPU is what causes them to show up in sysfs.

It makes very little sense to register all possible CPUs. Registering
a CPU is what triggers the udev notifications allowing user-space to
react to newly added CPUs.

To allow all five ACPI architectures to use GENERIC_CPU_DEVICES, change
it to use for_each_present_cpu(). Making the ACPI architectures use
GENERIC_CPU_DEVICES is a pre-requisite step to centralise their
cpu_register() logic, before moving it into the ACPI processor driver.
When ACPI is disabled this work would be done by
cpu_dev_register_generic().

What do you actually mean about when ACPI is disabled this work would be

Firstly, please note that "you" is not appropriate here. This is James'
commit message, not mine.

Oh, Sorry for that.

done by cpu_dev_register_generic()? Is the work means register the cpu?

When ACPI is disabled _and_ CONFIG_GENERIC_CPU_DEVICES is enabled, then
cpu_dev_register_generic() will call arch_register_cpu() for each present
CPU after this commit, rather than for each _possible_ CPU (which is the
actual code change here.)

I'm not quite understand that, and how about when ACPI is enabled, which
function do this work?

This is what happens later in the series.

"drivers: base: Allow parts of GENERIC_CPU_DEVICES to be overridden"
adds a test for CONFIG_GENERIC_CPU_DEVICES, so this will only be used
with architectures using GENERIC_CPU_DEVICES. Then in:

"ACPI: processor: Register all CPUs from acpi_processor_get_info()"
which is not part of this series, this adds a call to arch_register_cpu()
in the ACPI code, and disables this path via a test for !acpi_disabled.

Essentially, this path gets used to register the present CPUs when
firmware (ACPI) isn't going to be registering the present CPUs.

I've changed this to:

"It makes very little sense to register all possible CPUs. Registering
a CPU is what triggers the udev notifications allowing user-space to
react to newly added CPUs.

"To allow all five ACPI architectures to use GENERIC_CPU_DEVICES, change
it to use for_each_present_cpu().

"Making the ACPI architectures use GENERIC_CPU_DEVICES is a pre-requisite
step to centralise their register_cpu() logic, before moving it into the
ACPI processor driver. When we add support for register CPUs from ACPI
in a later patch, we will avoid registering CPUs in this path."

which I hope makes it clearer.

Thanks for your great explanation. Change commit message to this makes 

me understand well.

Thanks,
Shaoqin

After this change, openrisc and hexagon systems that use the max_cpus
command line argument would not see the other CPUs present in sysfs.
This should not be a problem as these CPUs can't bre brought online as

                                              ^ nit: can't be

Thanks, I'll fix that.