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

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

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:

-------8<-------

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)) {
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;