Re: [PATCH] cpuidle: ladder: Fix state index when only one idle state is registered

[Christian Loehle <christian.loehle@arm.com>]

[-- Attachment #1: Type: text/plain, Size: 3245 bytes --]

On 2/13/26 13:44, Christian Loehle wrote:
> On 2/13/26 08:29, Aboorva Devarajan wrote:
>> On Wed, 2026-02-11 at 15:00 +0000, Christian Loehle wrote:
>>> On 2/11/26 05:35, Aboorva Devarajan wrote:
>>>> On certain platforms (PowerNV systems without a power-mgt DT node),
>>>> cpuidle may register only a single idle state. In cases where that
>>>> single state is a polling state (state 0), the ladder governor may
>>>> incorrectly treat state 1 as the first usable state and pass an
>>>> out-of-bounds index. This can lead to a NULL enter callback being
>>>> invoked, ultimately resulting in a system crash.
>>>>
>>>> [   13.342636] cpuidle-powernv : Only Snooze is available
>>>> [   13.351854] Faulting instruction address: 0x00000000
>>>> [   13.376489] NIP [0000000000000000] 0x0
>>>> [   13.378351] LR  [c000000001e01974] cpuidle_enter_state+0x2c4/0x668
>>>>
>>>> Fix this by determining the first non-polling state index based on
>>>> the number of registered states, and by returning state 0 when only
>>>> one state is registered.
>>>>
>>>> Fixes: dc2251bf98c6 ("cpuidle: Eliminate the CPUIDLE_DRIVER_STATE_START symbol")
>>>> Signed-off-by: Aboorva Devarajan <aboorvad@linux.ibm.com>
>>>
>>> Agreed that the current behavior is a bug, but is there really much value
>>> in using a cpuidle governor with just a polling state?
>>> It's dead code and trivial to bail out of in cpuidle, right?
>>>
>>
>> Hi Christian,
>>
>> Thanks for the review.
>>
>> Other governors (teo, menu) already handle this single-state scenario
>> correctly. Fixing ladder's first_idx calculation seemed like the most
>> targeted fix, however since ladder is not widely used this is likely
>> to go unnoticed, it only popped up during testing with a missing
>> power-mgt device tree node.
>>
>> yes, adding a bail-out in the core cpuidle_select() is also trivial and
>> would benefit all governors uniformly. Setting stop_tick to false keeps
>> the tick running, which is correct for a single state configuration.
>>
>> Please let me know if you'd prefer this approach instead.
>>
>> ---
>>
>> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
>> index c7876e9e024f..ea082419f7db 100644
>> --- a/drivers/cpuidle/cpuidle.c
>> +++ b/drivers/cpuidle/cpuidle.c
>> @@ -359,6 +359,16 @@ noinstr int cpuidle_enter_state(struct
>> cpuidle_device *dev,
>>  int cpuidle_select(struct cpuidle_driver *drv, struct cpuidle_device
>> *dev,
>>                    bool *stop_tick)
>>  {
>> +       /*
>> +        * If there is only a single idle state (or none), there is
>> nothing
>> +        * meaningful for the governor to choose. Skip the governor and
>> +        * always use state 0 with the tick running.
>> +        */
>> +       if (unlikely(drv->state_count <= 1)) {
> 
> I think the unlikely isn't helping here, this just let the branch predictor
> handle this as it won't change anyway.
> 
>> +               *stop_tick = false;
>> +               return 0;
>> +       }
>> +
>>         return cpuidle_curr_governor->select(drv, dev, stop_tick);
>>  }
>>
> 
> I prefer this, additionally of course:

I've attached them as patches with a sign-off, feel free to pick them up as a series
or if you provide your signoff I can do that as well.

[-- Attachment #2: 0002-cpuidle-teo-Remove-single-state-handling.patch --]
[-- Type: text/x-patch, Size: 1073 bytes --]

From 56ba6cbec9aca645a51744261ecf0276072e001d Mon Sep 17 00:00:00 2001
From: Christian Loehle <christian.loehle@arm.com>
Date: Mon, 16 Feb 2026 11:02:55 +0000
Subject: [PATCH 2/2] cpuidle: teo: Remove single state handling

cpuidle systems where the governor has no choice because there's only
a single idle state are now handled by cpuidle core and bypass the
governor, so remove the related handling.

Signed-off-by: Christian Loehle <christian.loehle@arm.com>
---
 drivers/cpuidle/governors/teo.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/cpuidle/governors/teo.c b/drivers/cpuidle/governors/teo.c
index 81ac5fd58a1c..9b5b8c617806 100644
--- a/drivers/cpuidle/governors/teo.c
+++ b/drivers/cpuidle/governors/teo.c
@@ -317,12 +317,6 @@ static int teo_select(struct cpuidle_driver *drv, struct cpuidle_device *dev,
 	 */
 	cpu_data->sleep_length_ns = KTIME_MAX;
 
-	/* Check if there is any choice in the first place. */
-	if (drv->state_count < 2) {
-		idx = 0;
-		goto out_tick;
-	}
-
 	if (!dev->states_usage[0].disable)
 		idx = 0;
 
-- 
2.34.1


[-- Attachment #3: 0001-cpuidle-menu-Remove-single-state-handling.patch --]
[-- Type: text/x-patch, Size: 1167 bytes --]

From 1046bac618c05262d139071e55f4248d3121310d Mon Sep 17 00:00:00 2001
From: Christian Loehle <christian.loehle@arm.com>
Date: Mon, 16 Feb 2026 11:01:02 +0000
Subject: [PATCH 1/2] cpuidle: menu: Remove single state handling

cpuidle systems where the governor has no choice because there's only
a single idle state are now handled by cpuidle core and bypass the
governor, so remove the related handling.

Signed-off-by: Christian Loehle <christian.loehle@arm.com>
---
 drivers/cpuidle/governors/menu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/cpuidle/governors/menu.c b/drivers/cpuidle/governors/menu.c
index 64d6f7a1c776..fdfa5d7e10a6 100644
--- a/drivers/cpuidle/governors/menu.c
+++ b/drivers/cpuidle/governors/menu.c
@@ -271,7 +271,7 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev,
 		data->bucket = BUCKETS - 1;
 	}
 
-	if (unlikely(drv->state_count <= 1 || latency_req == 0) ||
+	if (unlikely(latency_req == 0) ||
 	    ((data->next_timer_ns < drv->states[1].target_residency_ns ||
 	      latency_req < drv->states[1].exit_latency_ns) &&
 	     !dev->states_usage[0].disable)) {
-- 
2.34.1