Linux Power Management development
 help / color / mirror / Atom feed
* [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