From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by smtp.subspace.kernel.org (Postfix) with ESMTP id BBCAA14AD20; Fri, 13 Feb 2026 13:44:43 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=217.140.110.172 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1770990285; cv=none; b=aYOZDmjf7z3Y9o6rwwrfH/I6e2VVzC6lTAwvgJCj3jKoQpNwI2epn4i+oYtETr0Ymavj8VMdsVjyDkagsH9qB9Wlx0rzxt+sOpP4wEqTd2x2b4TloJp30spAmv0a5mx7EKDARb392nFmJiCUh7BEkwLQLqUuMXzPc5lD1DgE2ws= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1770990285; c=relaxed/simple; bh=e1KK9/Flaa1UtwYQQSFPpDmB/yGazt0FwOkB4J8jOQ8=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=P2LFE+lPfkPrkUi1M9yp37OjutEfOf++chMzEyKmy+q05FdyrmR/5bi4uP0URx0M7WDmZludRtFUSDcTwUwJVa4a+70KWVMG1kd7bbdtVUiWMaDGcdU8jNejI/kiGVPACF7Q8fi8KBSKK9jj2sIU16n9oLqu6FgT7yFqSTha4hE= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arm.com; spf=pass smtp.mailfrom=arm.com; arc=none smtp.client-ip=217.140.110.172 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arm.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=arm.com Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 3C52E1570; Fri, 13 Feb 2026 05:44:36 -0800 (PST) Received: from [10.57.15.246] (unknown [10.57.15.246]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 794653F63F; Fri, 13 Feb 2026 05:44:41 -0800 (PST) Message-ID: Date: Fri, 13 Feb 2026 13:44:39 +0000 Precedence: bulk X-Mailing-List: linux-pm@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] cpuidle: ladder: Fix state index when only one idle state is registered To: Aboorva Devarajan , rafael@kernel.org Cc: daniel.lezcano@linaro.org, linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org References: <20260211053552.739337-1-aboorvad@linux.ibm.com> <7297173684f500e006a2997b92c927262221336f.camel@linux.ibm.com> Content-Language: en-US From: Christian Loehle In-Reply-To: <7297173684f500e006a2997b92c927262221336f.camel@linux.ibm.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 >> >> 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;