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

[Aboorva Devarajan <aboorvad@linux.ibm.com>]

On Mon, 2026-02-16 at 11:05 +0000, Christian Loehle wrote:
> 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.

Hi Christian,

Thanks, I've included your patches and sent them as a series:
https://lore.kernel.org/all/20260216185005.1131593-1-aboorvad@linux.ibm.com/

Regards,
Aboorva