Re: [PATCH v2] cpuidle: Add sanity check for exit latency and target residency

[Artem Bityutskiy <artem.bityutskiy@linux.intel.com>]

On Fri, 2025-11-07 at 20:07 +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Make __cpuidle_driver_init() fail if the exit latency of one of the
> driver's idle states is less than its exit latency which would break
> cpuidle assumptions.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

LGTM

Reviewed-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>


By the way, I have a more paranoid validation patch, which validates
latency and also more. Sure I can rebase it later on top of this
patch. 

I did not have time to polish it yet, but sharing just in case there is
a quick feedback.

From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
Subject: [PATCH] cpuidle: Add idle states validation

Validate the idle states table provided by the underlying idle driver. For
example, validate that deeper idle states have greater latency and target
residency compared to shallower states.

Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
---
 drivers/cpuidle/driver.c | 58 ++++++++++++++++++++++++++++++++++++++--
 1 file changed, 56 insertions(+), 2 deletions(-)

diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c
index 9bbfa594c4425..6bcedad534dd9 100644
--- a/drivers/cpuidle/driver.c
+++ b/drivers/cpuidle/driver.c
@@ -20,6 +20,10 @@
 
 #include "cpuidle.h"
 
+/* Maximum allowed latency and target residency values */
+#define MAX_LATENCY 50000 /* 50 milliseconds */
+#define MAX_RESIDENCY 1000000 /* 1 second */
+
 DEFINE_SPINLOCK(cpuidle_driver_lock);
 
 #ifdef CONFIG_CPU_IDLE_MULTIPLE_DRIVERS
@@ -148,11 +152,46 @@ static void cpuidle_setup_broadcast_timer(void *arg)
 		tick_broadcast_disable();
 }
 
+/**
+ * validate_state - Validate an idle state.
+ * @state: The C-state to validate.
+ * @prev_state: The previous idle state or NULL.
+ *
+ * Return: 0 if the idle state is valid or -EINVAL otherwise.
+ */
+static int validate_state(struct cpuidle_state *s, struct cpuidle_state *prev_s)
+{
+	if (s->exit_latency == 0)
+		return -EINVAL;
+
+	if (s->exit_latency > MAX_LATENCY)
+		return -EINVAL;
+
+	if (s->target_residency > MAX_RESIDENCY)
+		return -EINVAL;
+
+	if (s->target_residency < s->exit_latency)
+		return -EINVAL;
+
+	if (!prev_s)
+		return 0;
+
+	if (s->exit_latency <= prev_s->exit_latency)
+		return -EINVAL;
+
+	if (s->target_residency <= prev_s->target_residency)
+		return -EINVAL;
+
+	return 0;
+}
+
 /**
  * __cpuidle_driver_init - initialize the driver's internal data
  * @drv: a valid pointer to a struct cpuidle_driver
+ *
+ * Return: 0 on success, -EINVAL in case of invalid C-state data.
  */
-static void __cpuidle_driver_init(struct cpuidle_driver *drv)
+static int __cpuidle_driver_init(struct cpuidle_driver *drv)
 {
 	int i;
 
@@ -166,6 +205,17 @@ static void __cpuidle_driver_init(struct cpuidle_driver *drv)
 
 	for (i = 0; i < drv->state_count; i++) {
 		struct cpuidle_state *s = &drv->states[i];
+		struct cpuidle_state *prev_s;
+
+		if (i > 0)
+			prev_s = &drv->states[i - 1];
+		else
+			prev_s = NULL;
+
+		if (validate_state(s, prev_s)) {
+			pr_err("Idle state '%s' validation failed\n", s->name);
+			return -EINVAL;
+		}
 
 		/*
 		 * Look for the timer stop flag in the different states and if
@@ -194,6 +244,8 @@ static void __cpuidle_driver_init(struct cpuidle_driver *drv)
 		else
 			s->exit_latency = div_u64(s->exit_latency_ns, NSEC_PER_USEC);
 	}
+
+	return 0;
 }
 
 /**
@@ -223,7 +275,9 @@ static int __cpuidle_register_driver(struct cpuidle_driver *drv)
 	if (cpuidle_disabled())
 		return -ENODEV;
 
-	__cpuidle_driver_init(drv);
+	ret = __cpuidle_driver_init(drv);
+	if (ret)
+		return ret;
 
 	ret = __cpuidle_set_driver(drv);
 	if (ret)
-- 
2.51.0