| Message ID | 20250307080303.2660506-1-ping.bai@nxp.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v4] cpuidle: Init cpuidle only for present CPUs | expand |
On Fri, Mar 07, 2025 at 04:03:03PM +0800, Jacky Bai wrote: > for_each_possible_cpu() is currently used to initialize cpuidle > in below cpuidle drivers: > drivers/cpuidle/cpuidle-arm.c > drivers/cpuidle/cpuidle-big_little.c > drivers/cpuidle/cpuidle-psci.c > drivers/cpuidle/cpuidle-riscv-sbi.c > > However, in cpu_dev_register_generic(), for_each_present_cpu() > is used to register CPU devices which means the CPU devices are > only registered for present CPUs and not all possible CPUs. > > With nosmp or maxcpus=0, only the boot CPU is present, lead > to the failure: > > | Failed to register cpuidle device for cpu1 > > Then rollback to cancel all CPUs' cpuidle registration. > > Change for_each_possible_cpu() to for_each_present_cpu() in the > above cpuidle drivers to ensure it only registers cpuidle devices > for CPUs that are actually present. > > Fixes: b0c69e1214bc ("drivers: base: Use present CPUs in GENERIC_CPU_DEVICES") > Reviewed-by: Dhruva Gole <d-gole@ti.com> > Reviewed-by: Sudeep Holla <sudeep.holla@arm.com> > Tested-by: Yuanjie Yang <quic_yuanjiey@quicinc.com> > Signed-off-by: Jacky Bai <ping.bai@nxp.com> > --- > - v4 changes: > - add changes for other cpuidle driver that has the similar issue > as cpuidle-pcsi driver. > > - v3 changes: > - improve the changelog as suggested by Sudeep > --- > drivers/cpuidle/cpuidle-arm.c | 8 ++++---- > drivers/cpuidle/cpuidle-big_little.c | 2 +- > drivers/cpuidle/cpuidle-psci.c | 4 ++-- > drivers/cpuidle/cpuidle-riscv-sbi.c | 4 ++-- Why have you spared drivers/cpuidle/cpuidle-qcom-spm.c ? IIUC the issue exists there as well.
Hi Sudeep, > Subject: Re: [PATCH v4] cpuidle: Init cpuidle only for present CPUs > > On Fri, Mar 07, 2025 at 04:03:03PM +0800, Jacky Bai wrote: > > for_each_possible_cpu() is currently used to initialize cpuidle in > > below cpuidle drivers: > > drivers/cpuidle/cpuidle-arm.c > > drivers/cpuidle/cpuidle-big_little.c > > drivers/cpuidle/cpuidle-psci.c > > drivers/cpuidle/cpuidle-riscv-sbi.c > > > > However, in cpu_dev_register_generic(), for_each_present_cpu() is used > > to register CPU devices which means the CPU devices are only > > registered for present CPUs and not all possible CPUs. > > > > With nosmp or maxcpus=0, only the boot CPU is present, lead to the > > failure: > > > > | Failed to register cpuidle device for cpu1 > > > > Then rollback to cancel all CPUs' cpuidle registration. > > > > Change for_each_possible_cpu() to for_each_present_cpu() in the above > > cpuidle drivers to ensure it only registers cpuidle devices for CPUs > > that are actually present. > > > > Fixes: b0c69e1214bc ("drivers: base: Use present CPUs in > > GENERIC_CPU_DEVICES") > > Reviewed-by: Dhruva Gole <d-gole@ti.com> > > Reviewed-by: Sudeep Holla <sudeep.holla@arm.com> > > Tested-by: Yuanjie Yang <quic_yuanjiey@quicinc.com> > > Signed-off-by: Jacky Bai <ping.bai@nxp.com> > > --- > > - v4 changes: > > - add changes for other cpuidle driver that has the similar issue > > as cpuidle-pcsi driver. > > > > - v3 changes: > > - improve the changelog as suggested by Sudeep > > --- > > drivers/cpuidle/cpuidle-arm.c | 8 ++++---- > > drivers/cpuidle/cpuidle-big_little.c | 2 +- > > drivers/cpuidle/cpuidle-psci.c | 4 ++-- > > drivers/cpuidle/cpuidle-riscv-sbi.c | 4 ++-- > > > Why have you spared drivers/cpuidle/cpuidle-qcom-spm.c ? IIUC the issue > exists there as well. > For qcom-spm driver, it has below code logic to handle no cpu device case, and no rollback to cancel the whole cpuidle registration. So I just leave it as it is. Do we need to update it? for_each_possible_cpu(cpu) { ret = spm_cpuidle_register(&pdev->dev, cpu); if (ret && ret != -ENODEV) { dev_err(&pdev->dev, "Cannot register for CPU%d: %d\n", cpu, ret); } } BR Jacky Bai > -- > Regards, > Sudeep
On Fri, Mar 07, 2025 at 10:02:14AM +0000, Jacky Bai wrote: > Hi Sudeep, > > > Subject: Re: [PATCH v4] cpuidle: Init cpuidle only for present CPUs > > > > On Fri, Mar 07, 2025 at 04:03:03PM +0800, Jacky Bai wrote: > > > for_each_possible_cpu() is currently used to initialize cpuidle in > > > below cpuidle drivers: > > > drivers/cpuidle/cpuidle-arm.c > > > drivers/cpuidle/cpuidle-big_little.c > > > drivers/cpuidle/cpuidle-psci.c > > > drivers/cpuidle/cpuidle-riscv-sbi.c > > > > > > However, in cpu_dev_register_generic(), for_each_present_cpu() is used > > > to register CPU devices which means the CPU devices are only > > > registered for present CPUs and not all possible CPUs. > > > > > > With nosmp or maxcpus=0, only the boot CPU is present, lead to the > > > failure: > > > > > > | Failed to register cpuidle device for cpu1 > > > > > > Then rollback to cancel all CPUs' cpuidle registration. > > > > > > Change for_each_possible_cpu() to for_each_present_cpu() in the above > > > cpuidle drivers to ensure it only registers cpuidle devices for CPUs > > > that are actually present. > > > > > > Fixes: b0c69e1214bc ("drivers: base: Use present CPUs in > > > GENERIC_CPU_DEVICES") > > > Reviewed-by: Dhruva Gole <d-gole@ti.com> > > > Reviewed-by: Sudeep Holla <sudeep.holla@arm.com> > > > Tested-by: Yuanjie Yang <quic_yuanjiey@quicinc.com> > > > Signed-off-by: Jacky Bai <ping.bai@nxp.com> > > > --- > > > - v4 changes: > > > - add changes for other cpuidle driver that has the similar issue > > > as cpuidle-pcsi driver. > > > > > > - v3 changes: > > > - improve the changelog as suggested by Sudeep > > > --- > > > drivers/cpuidle/cpuidle-arm.c | 8 ++++---- > > > drivers/cpuidle/cpuidle-big_little.c | 2 +- > > > drivers/cpuidle/cpuidle-psci.c | 4 ++-- > > > drivers/cpuidle/cpuidle-riscv-sbi.c | 4 ++-- > > > > > > Why have you spared drivers/cpuidle/cpuidle-qcom-spm.c ? IIUC the issue > > exists there as well. > > > > For qcom-spm driver, it has below code logic to handle no cpu device case, and > no rollback to cancel the whole cpuidle registration. So I just leave it as it is. > Do we need to update it? > > for_each_possible_cpu(cpu) { > ret = spm_cpuidle_register(&pdev->dev, cpu); Did you look into this function ?
> Subject: Re: [PATCH v4] cpuidle: Init cpuidle only for present CPUs > > On Fri, Mar 07, 2025 at 10:02:14AM +0000, Jacky Bai wrote: > > Hi Sudeep, > > > > > Subject: Re: [PATCH v4] cpuidle: Init cpuidle only for present CPUs > > > > > > On Fri, Mar 07, 2025 at 04:03:03PM +0800, Jacky Bai wrote: > > > > for_each_possible_cpu() is currently used to initialize cpuidle in > > > > below cpuidle drivers: > > > > drivers/cpuidle/cpuidle-arm.c > > > > drivers/cpuidle/cpuidle-big_little.c > > > > drivers/cpuidle/cpuidle-psci.c > > > > drivers/cpuidle/cpuidle-riscv-sbi.c > > > > > > > > However, in cpu_dev_register_generic(), for_each_present_cpu() is > > > > used to register CPU devices which means the CPU devices are only > > > > registered for present CPUs and not all possible CPUs. > > > > > > > > With nosmp or maxcpus=0, only the boot CPU is present, lead to the > > > > failure: > > > > > > > > | Failed to register cpuidle device for cpu1 > > > > > > > > Then rollback to cancel all CPUs' cpuidle registration. > > > > > > > > Change for_each_possible_cpu() to for_each_present_cpu() in the > > > > above cpuidle drivers to ensure it only registers cpuidle devices > > > > for CPUs that are actually present. > > > > > > > > Fixes: b0c69e1214bc ("drivers: base: Use present CPUs in > > > > GENERIC_CPU_DEVICES") > > > > Reviewed-by: Dhruva Gole <d-gole@ti.com> > > > > Reviewed-by: Sudeep Holla <sudeep.holla@arm.com> > > > > Tested-by: Yuanjie Yang <quic_yuanjiey@quicinc.com> > > > > Signed-off-by: Jacky Bai <ping.bai@nxp.com> > > > > --- > > > > - v4 changes: > > > > - add changes for other cpuidle driver that has the similar issue > > > > as cpuidle-pcsi driver. > > > > > > > > - v3 changes: > > > > - improve the changelog as suggested by Sudeep > > > > --- > > > > drivers/cpuidle/cpuidle-arm.c | 8 ++++---- > > > > drivers/cpuidle/cpuidle-big_little.c | 2 +- > > > > drivers/cpuidle/cpuidle-psci.c | 4 ++-- > > > > drivers/cpuidle/cpuidle-riscv-sbi.c | 4 ++-- > > > > > > > > > Why have you spared drivers/cpuidle/cpuidle-qcom-spm.c ? IIUC the > > > issue exists there as well. > > > > > > > For qcom-spm driver, it has below code logic to handle no cpu device > > case, and no rollback to cancel the whole cpuidle registration. So I just leave > it as it is. > > Do we need to update it? > > > > for_each_possible_cpu(cpu) { > > ret = spm_cpuidle_register(&pdev->dev, cpu); > > Did you look into this function ? Yes, at the very beginning of this function it will check if the cpu device is available, if not, directly return -ENODEV, something I misunderstood? BR > > -- > Regards, > Sudeep
On Fri, Mar 07, 2025 at 10:10:50AM +0000, Jacky Bai wrote: > > Subject: Re: [PATCH v4] cpuidle: Init cpuidle only for present CPUs > > > > On Fri, Mar 07, 2025 at 10:02:14AM +0000, Jacky Bai wrote: > > > Hi Sudeep, > > > > > > > Subject: Re: [PATCH v4] cpuidle: Init cpuidle only for present CPUs > > > > > > > > On Fri, Mar 07, 2025 at 04:03:03PM +0800, Jacky Bai wrote: > > > > > for_each_possible_cpu() is currently used to initialize cpuidle in > > > > > below cpuidle drivers: > > > > > drivers/cpuidle/cpuidle-arm.c > > > > > drivers/cpuidle/cpuidle-big_little.c > > > > > drivers/cpuidle/cpuidle-psci.c > > > > > drivers/cpuidle/cpuidle-riscv-sbi.c > > > > > > > > > > However, in cpu_dev_register_generic(), for_each_present_cpu() is > > > > > used to register CPU devices which means the CPU devices are only > > > > > registered for present CPUs and not all possible CPUs. > > > > > > > > > > With nosmp or maxcpus=0, only the boot CPU is present, lead to the > > > > > failure: > > > > > > > > > > | Failed to register cpuidle device for cpu1 > > > > > > > > > > Then rollback to cancel all CPUs' cpuidle registration. > > > > > > > > > > Change for_each_possible_cpu() to for_each_present_cpu() in the > > > > > above cpuidle drivers to ensure it only registers cpuidle devices > > > > > for CPUs that are actually present. > > > > > > > > > > Fixes: b0c69e1214bc ("drivers: base: Use present CPUs in > > > > > GENERIC_CPU_DEVICES") > > > > > Reviewed-by: Dhruva Gole <d-gole@ti.com> > > > > > Reviewed-by: Sudeep Holla <sudeep.holla@arm.com> > > > > > Tested-by: Yuanjie Yang <quic_yuanjiey@quicinc.com> > > > > > Signed-off-by: Jacky Bai <ping.bai@nxp.com> > > > > > --- > > > > > - v4 changes: > > > > > - add changes for other cpuidle driver that has the similar issue > > > > > as cpuidle-pcsi driver. > > > > > > > > > > - v3 changes: > > > > > - improve the changelog as suggested by Sudeep > > > > > --- > > > > > drivers/cpuidle/cpuidle-arm.c | 8 ++++---- > > > > > drivers/cpuidle/cpuidle-big_little.c | 2 +- > > > > > drivers/cpuidle/cpuidle-psci.c | 4 ++-- > > > > > drivers/cpuidle/cpuidle-riscv-sbi.c | 4 ++-- > > > > > > > > > > > > Why have you spared drivers/cpuidle/cpuidle-qcom-spm.c ? IIUC the > > > > issue exists there as well. > > > > > > > > > > For qcom-spm driver, it has below code logic to handle no cpu device > > > case, and no rollback to cancel the whole cpuidle registration. So I just leave > > it as it is. > > > Do we need to update it? > > > > > > for_each_possible_cpu(cpu) { > > > ret = spm_cpuidle_register(&pdev->dev, cpu); > > > > Did you look into this function ? > > Yes, at the very beginning of this function it will check if the cpu device > is available, if not, directly return -ENODEV, something I misunderstood? > So why do you think spm_cpuidle_register() does anything different than psci_idle_init_cpu(). They do exactly same check and yet you apply the change for psci_idle_init_cpu() but not for spm_cpuidle_register(). What am I missing ?
> Subject: Re: [PATCH v4] cpuidle: Init cpuidle only for present CPUs > > On Fri, Mar 07, 2025 at 10:10:50AM +0000, Jacky Bai wrote: > > > Subject: Re: [PATCH v4] cpuidle: Init cpuidle only for present CPUs > > > > > > On Fri, Mar 07, 2025 at 10:02:14AM +0000, Jacky Bai wrote: > > > > Hi Sudeep, > > > > > > > > > Subject: Re: [PATCH v4] cpuidle: Init cpuidle only for present > > > > > CPUs > > > > > > > > > > On Fri, Mar 07, 2025 at 04:03:03PM +0800, Jacky Bai wrote: > > > > > > for_each_possible_cpu() is currently used to initialize > > > > > > cpuidle in below cpuidle drivers: > > > > > > drivers/cpuidle/cpuidle-arm.c > > > > > > drivers/cpuidle/cpuidle-big_little.c > > > > > > drivers/cpuidle/cpuidle-psci.c > > > > > > drivers/cpuidle/cpuidle-riscv-sbi.c > > > > > > > > > > > > However, in cpu_dev_register_generic(), for_each_present_cpu() > > > > > > is used to register CPU devices which means the CPU devices > > > > > > are only registered for present CPUs and not all possible CPUs. > > > > > > > > > > > > With nosmp or maxcpus=0, only the boot CPU is present, lead to > > > > > > the > > > > > > failure: > > > > > > > > > > > > | Failed to register cpuidle device for cpu1 > > > > > > > > > > > > Then rollback to cancel all CPUs' cpuidle registration. > > > > > > > > > > > > Change for_each_possible_cpu() to for_each_present_cpu() in > > > > > > the above cpuidle drivers to ensure it only registers cpuidle > > > > > > devices for CPUs that are actually present. > > > > > > > > > > > > Fixes: b0c69e1214bc ("drivers: base: Use present CPUs in > > > > > > GENERIC_CPU_DEVICES") > > > > > > Reviewed-by: Dhruva Gole <d-gole@ti.com> > > > > > > Reviewed-by: Sudeep Holla <sudeep.holla@arm.com> > > > > > > Tested-by: Yuanjie Yang <quic_yuanjiey@quicinc.com> > > > > > > Signed-off-by: Jacky Bai <ping.bai@nxp.com> > > > > > > --- > > > > > > - v4 changes: > > > > > > - add changes for other cpuidle driver that has the similar issue > > > > > > as cpuidle-pcsi driver. > > > > > > > > > > > > - v3 changes: > > > > > > - improve the changelog as suggested by Sudeep > > > > > > --- > > > > > > drivers/cpuidle/cpuidle-arm.c | 8 ++++---- > > > > > > drivers/cpuidle/cpuidle-big_little.c | 2 +- > > > > > > drivers/cpuidle/cpuidle-psci.c | 4 ++-- > > > > > > drivers/cpuidle/cpuidle-riscv-sbi.c | 4 ++-- > > > > > > > > > > > > > > > Why have you spared drivers/cpuidle/cpuidle-qcom-spm.c ? IIUC > > > > > the issue exists there as well. > > > > > > > > > > > > > For qcom-spm driver, it has below code logic to handle no cpu > > > > device case, and no rollback to cancel the whole cpuidle > > > > registration. So I just leave > > > it as it is. > > > > Do we need to update it? > > > > > > > > for_each_possible_cpu(cpu) { > > > > ret = spm_cpuidle_register(&pdev->dev, cpu); > > > > > > Did you look into this function ? > > > > Yes, at the very beginning of this function it will check if the cpu > > device is available, if not, directly return -ENODEV, something I > misunderstood? > > > > So why do you think spm_cpuidle_register() does anything different than > psci_idle_init_cpu(). They do exactly same check and yet you apply the > change for psci_idle_init_cpu() but not for spm_cpuidle_register(). > What am I missing ? > Thank you, I got your point now. let me update it in next version. :) BR > -- > Regards, > Sudeep
diff --git a/drivers/cpuidle/cpuidle-arm.c b/drivers/cpuidle/cpuidle-arm.c index caba6f4bb1b7..e044fefdb816 100644 --- a/drivers/cpuidle/cpuidle-arm.c +++ b/drivers/cpuidle/cpuidle-arm.c @@ -137,9 +137,9 @@ static int __init arm_idle_init_cpu(int cpu) /* * arm_idle_init - Initializes arm cpuidle driver * - * Initializes arm cpuidle driver for all CPUs, if any CPU fails - * to register cpuidle driver then rollback to cancel all CPUs - * registration. + * Initializes arm cpuidle driver for all present CPUs, if any + * CPU fails to register cpuidle driver then rollback to cancel + * all CPUs registration. */ static int __init arm_idle_init(void) { @@ -147,7 +147,7 @@ static int __init arm_idle_init(void) struct cpuidle_driver *drv; struct cpuidle_device *dev; - for_each_possible_cpu(cpu) { + for_each_present_cpu(cpu) { ret = arm_idle_init_cpu(cpu); if (ret) goto out_fail; diff --git a/drivers/cpuidle/cpuidle-big_little.c b/drivers/cpuidle/cpuidle-big_little.c index 74972deda0ea..4abba42fcc31 100644 --- a/drivers/cpuidle/cpuidle-big_little.c +++ b/drivers/cpuidle/cpuidle-big_little.c @@ -148,7 +148,7 @@ static int __init bl_idle_driver_init(struct cpuidle_driver *drv, int part_id) if (!cpumask) return -ENOMEM; - for_each_possible_cpu(cpu) + for_each_present_cpu(cpu) if (smp_cpuid_part(cpu) == part_id) cpumask_set_cpu(cpu, cpumask); diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c index dd8d776d6e39..b46a83f5ffe4 100644 --- a/drivers/cpuidle/cpuidle-psci.c +++ b/drivers/cpuidle/cpuidle-psci.c @@ -403,7 +403,7 @@ static int psci_idle_init_cpu(struct device *dev, int cpu) /* * psci_idle_probe - Initializes PSCI cpuidle driver * - * Initializes PSCI cpuidle driver for all CPUs, if any CPU fails + * Initializes PSCI cpuidle driver for all present CPUs, if any CPU fails * to register cpuidle driver then rollback to cancel all CPUs * registration. */ @@ -413,7 +413,7 @@ static int psci_cpuidle_probe(struct platform_device *pdev) struct cpuidle_driver *drv; struct cpuidle_device *dev; - for_each_possible_cpu(cpu) { + for_each_present_cpu(cpu) { ret = psci_idle_init_cpu(&pdev->dev, cpu); if (ret) goto out_fail; diff --git a/drivers/cpuidle/cpuidle-riscv-sbi.c b/drivers/cpuidle/cpuidle-riscv-sbi.c index 0c92a628bbd4..0fe1ece9fbdc 100644 --- a/drivers/cpuidle/cpuidle-riscv-sbi.c +++ b/drivers/cpuidle/cpuidle-riscv-sbi.c @@ -529,8 +529,8 @@ static int sbi_cpuidle_probe(struct platform_device *pdev) return ret; } - /* Initialize CPU idle driver for each CPU */ - for_each_possible_cpu(cpu) { + /* Initialize CPU idle driver for each present CPU */ + for_each_present_cpu(cpu) { ret = sbi_cpuidle_init_cpu(&pdev->dev, cpu); if (ret) { pr_debug("HART%ld: idle driver init failed\n",
for_each_possible_cpu() is currently used to initialize cpuidle in below cpuidle drivers: drivers/cpuidle/cpuidle-arm.c drivers/cpuidle/cpuidle-big_little.c drivers/cpuidle/cpuidle-psci.c drivers/cpuidle/cpuidle-riscv-sbi.c However, in cpu_dev_register_generic(), for_each_present_cpu() is used to register CPU devices which means the CPU devices are only registered for present CPUs and not all possible CPUs. With nosmp or maxcpus=0, only the boot CPU is present, lead to the failure: | Failed to register cpuidle device for cpu1 Then rollback to cancel all CPUs' cpuidle registration. Change for_each_possible_cpu() to for_each_present_cpu() in the above cpuidle drivers to ensure it only registers cpuidle devices for CPUs that are actually present. Fixes: b0c69e1214bc ("drivers: base: Use present CPUs in GENERIC_CPU_DEVICES") Reviewed-by: Dhruva Gole <d-gole@ti.com> Reviewed-by: Sudeep Holla <sudeep.holla@arm.com> Tested-by: Yuanjie Yang <quic_yuanjiey@quicinc.com> Signed-off-by: Jacky Bai <ping.bai@nxp.com> --- - v4 changes: - add changes for other cpuidle driver that has the similar issue as cpuidle-pcsi driver. - v3 changes: - improve the changelog as suggested by Sudeep --- drivers/cpuidle/cpuidle-arm.c | 8 ++++---- drivers/cpuidle/cpuidle-big_little.c | 2 +- drivers/cpuidle/cpuidle-psci.c | 4 ++-- drivers/cpuidle/cpuidle-riscv-sbi.c | 4 ++-- 4 files changed, 9 insertions(+), 9 deletions(-)