* [PATCH v4] cpuidle: Init cpuidle only for present CPUs
@ 2025-03-07 8:03 Jacky Bai
2025-03-07 9:50 ` Sudeep Holla
0 siblings, 1 reply; 7+ messages in thread
From: Jacky Bai @ 2025-03-07 8:03 UTC (permalink / raw)
To: rafael, daniel.lezcano, lpieralisi, sudeep.holla, ulf.hansson,
james.morse, d-gole, anup, paul.walmsley, palmer, aou
Cc: linux-pm, linux-arm-kernel, linux-kernel, linux-riscv, imx,
khilman, quic_tingweiz, quic_yuanjiey
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(-)
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",
--
2.34.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v4] cpuidle: Init cpuidle only for present CPUs
2025-03-07 8:03 [PATCH v4] cpuidle: Init cpuidle only for present CPUs Jacky Bai
@ 2025-03-07 9:50 ` Sudeep Holla
2025-03-07 10:02 ` Jacky Bai
0 siblings, 1 reply; 7+ messages in thread
From: Sudeep Holla @ 2025-03-07 9:50 UTC (permalink / raw)
To: Jacky Bai
Cc: rafael, daniel.lezcano, lpieralisi, ulf.hansson, Sudeep Holla,
james.morse, d-gole, anup, paul.walmsley, palmer, aou, linux-pm,
linux-arm-kernel, linux-kernel, linux-riscv, imx, khilman,
quic_tingweiz, quic_yuanjiey
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.
--
Regards,
Sudeep
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH v4] cpuidle: Init cpuidle only for present CPUs
2025-03-07 9:50 ` Sudeep Holla
@ 2025-03-07 10:02 ` Jacky Bai
2025-03-07 10:04 ` Sudeep Holla
0 siblings, 1 reply; 7+ messages in thread
From: Jacky Bai @ 2025-03-07 10:02 UTC (permalink / raw)
To: Sudeep Holla
Cc: rafael@kernel.org, daniel.lezcano@linaro.org,
lpieralisi@kernel.org, ulf.hansson@linaro.org,
james.morse@arm.com, d-gole@ti.com, anup@brainfault.org,
paul.walmsley@sifive.com, palmer@dabbelt.com,
aou@eecs.berkeley.edu, linux-pm@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, linux-riscv@lists.infradead.org,
imx@lists.linux.dev, khilman@baylibre.com,
quic_tingweiz@quicinc.com, quic_yuanjiey@quicinc.com
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v4] cpuidle: Init cpuidle only for present CPUs
2025-03-07 10:02 ` Jacky Bai
@ 2025-03-07 10:04 ` Sudeep Holla
2025-03-07 10:10 ` Jacky Bai
0 siblings, 1 reply; 7+ messages in thread
From: Sudeep Holla @ 2025-03-07 10:04 UTC (permalink / raw)
To: Jacky Bai
Cc: rafael@kernel.org, daniel.lezcano@linaro.org,
lpieralisi@kernel.org, ulf.hansson@linaro.org,
james.morse@arm.com, d-gole@ti.com, anup@brainfault.org,
paul.walmsley@sifive.com, palmer@dabbelt.com,
aou@eecs.berkeley.edu, linux-pm@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, linux-riscv@lists.infradead.org,
imx@lists.linux.dev, khilman@baylibre.com,
quic_tingweiz@quicinc.com, quic_yuanjiey@quicinc.com
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 ?
--
Regards,
Sudeep
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH v4] cpuidle: Init cpuidle only for present CPUs
2025-03-07 10:04 ` Sudeep Holla
@ 2025-03-07 10:10 ` Jacky Bai
2025-03-07 10:19 ` Sudeep Holla
0 siblings, 1 reply; 7+ messages in thread
From: Jacky Bai @ 2025-03-07 10:10 UTC (permalink / raw)
To: Sudeep Holla
Cc: rafael@kernel.org, daniel.lezcano@linaro.org,
lpieralisi@kernel.org, ulf.hansson@linaro.org,
james.morse@arm.com, d-gole@ti.com, anup@brainfault.org,
paul.walmsley@sifive.com, palmer@dabbelt.com,
aou@eecs.berkeley.edu, linux-pm@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, linux-riscv@lists.infradead.org,
imx@lists.linux.dev, khilman@baylibre.com,
quic_tingweiz@quicinc.com, quic_yuanjiey@quicinc.com
> 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v4] cpuidle: Init cpuidle only for present CPUs
2025-03-07 10:10 ` Jacky Bai
@ 2025-03-07 10:19 ` Sudeep Holla
2025-03-07 10:26 ` Jacky Bai
0 siblings, 1 reply; 7+ messages in thread
From: Sudeep Holla @ 2025-03-07 10:19 UTC (permalink / raw)
To: Jacky Bai
Cc: rafael@kernel.org, daniel.lezcano@linaro.org,
lpieralisi@kernel.org, ulf.hansson@linaro.org,
james.morse@arm.com, d-gole@ti.com, anup@brainfault.org,
paul.walmsley@sifive.com, palmer@dabbelt.com,
aou@eecs.berkeley.edu, linux-pm@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, linux-riscv@lists.infradead.org,
imx@lists.linux.dev, khilman@baylibre.com,
quic_tingweiz@quicinc.com, quic_yuanjiey@quicinc.com
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 ?
--
Regards,
Sudeep
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH v4] cpuidle: Init cpuidle only for present CPUs
2025-03-07 10:19 ` Sudeep Holla
@ 2025-03-07 10:26 ` Jacky Bai
0 siblings, 0 replies; 7+ messages in thread
From: Jacky Bai @ 2025-03-07 10:26 UTC (permalink / raw)
To: Sudeep Holla
Cc: rafael@kernel.org, daniel.lezcano@linaro.org,
lpieralisi@kernel.org, ulf.hansson@linaro.org,
james.morse@arm.com, d-gole@ti.com, anup@brainfault.org,
paul.walmsley@sifive.com, palmer@dabbelt.com,
aou@eecs.berkeley.edu, linux-pm@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, linux-riscv@lists.infradead.org,
imx@lists.linux.dev, khilman@baylibre.com,
quic_tingweiz@quicinc.com, quic_yuanjiey@quicinc.com
> 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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2025-03-07 10:26 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-03-07 8:03 [PATCH v4] cpuidle: Init cpuidle only for present CPUs Jacky Bai
2025-03-07 9:50 ` Sudeep Holla
2025-03-07 10:02 ` Jacky Bai
2025-03-07 10:04 ` Sudeep Holla
2025-03-07 10:10 ` Jacky Bai
2025-03-07 10:19 ` Sudeep Holla
2025-03-07 10:26 ` Jacky Bai
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox