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>
---

v1 -> v2: Make __cpuidle_driver_init() fail if the check is not passed (Artem).

---
 drivers/cpuidle/driver.c |   16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

--- a/drivers/cpuidle/driver.c
+++ b/drivers/cpuidle/driver.c
@@ -152,7 +152,7 @@ static void cpuidle_setup_broadcast_time
  * __cpuidle_driver_init - initialize the driver's internal data
  * @drv: a valid pointer to a struct cpuidle_driver
  */
-static void __cpuidle_driver_init(struct cpuidle_driver *drv)
+static int __cpuidle_driver_init(struct cpuidle_driver *drv)
 {
 	int i;
 
@@ -193,7 +193,17 @@ static void __cpuidle_driver_init(struct
 			s->exit_latency_ns =  0;
 		else
 			s->exit_latency = div_u64(s->exit_latency_ns, NSEC_PER_USEC);
+
+		/*
+		 * Ensure that the exit latency of a CPU idle state does not
+		 * exceed its target residency which is assumed in cpuidle in
+		 * multiple places.
+		 */
+		if (s->exit_latency_ns > s->target_residency_ns)
+			return -EINVAL;
 	}
+
+	return 0;
 }
 
 /**
@@ -223,7 +233,9 @@ static int __cpuidle_register_driver(str
 	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)

On 11/7/25 19:07, 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

s/exit latency/target residency/
for the second one here.

Apart from that
Reviewed-by: Christian Loehle <christian.loehle@arm.com>

> cpuidle assumptions.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> 
> v1 -> v2: Make __cpuidle_driver_init() fail if the check is not passed (Artem).
> 
> ---
>  drivers/cpuidle/driver.c |   16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> --- a/drivers/cpuidle/driver.c
> +++ b/drivers/cpuidle/driver.c
> @@ -152,7 +152,7 @@ static void cpuidle_setup_broadcast_time
>   * __cpuidle_driver_init - initialize the driver's internal data
>   * @drv: a valid pointer to a struct cpuidle_driver
>   */
> -static void __cpuidle_driver_init(struct cpuidle_driver *drv)
> +static int __cpuidle_driver_init(struct cpuidle_driver *drv)
>  {
>  	int i;
>  
> @@ -193,7 +193,17 @@ static void __cpuidle_driver_init(struct
>  			s->exit_latency_ns =  0;
>  		else
>  			s->exit_latency = div_u64(s->exit_latency_ns, NSEC_PER_USEC);
> +
> +		/*
> +		 * Ensure that the exit latency of a CPU idle state does not
> +		 * exceed its target residency which is assumed in cpuidle in
> +		 * multiple places.
> +		 */
> +		if (s->exit_latency_ns > s->target_residency_ns)
> +			return -EINVAL;
>  	}
> +
> +	return 0;
>  }
>  
>  /**
> @@ -223,7 +233,9 @@ static int __cpuidle_register_driver(str
>  	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)
> 
> 
> 
>

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

On Sat, Nov 8, 2025 at 9:49 AM Artem Bityutskiy
<artem.bityutskiy@linux.intel.com> wrote:
>
> 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.

That should be rather straightforward.

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

The change above will break the polling state AFAICS.

> +
> +       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;

Well, is this really necessary?  Nothing depends on this ordering AFAICS.

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

I would use the ternary operator here:

              if (validate_state(s, i ? &drv->states[i - 1] : NULL)) {

or just pass drv and i to validate_state().

But this needs to be done when all of the values are known. ->

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

-> Which is here.

>         }
> +
> +       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)
> --

On Sat, Nov 8, 2025 at 12:02 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Sat, Nov 8, 2025 at 9:49 AM Artem Bityutskiy
> <artem.bityutskiy@linux.intel.com> wrote:
> >
> > 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.
>
> That should be rather straightforward.
>
> > 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;
>
> The change above will break the polling state AFAICS.
>
> > +
> > +       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;
>
> Well, is this really necessary?  Nothing depends on this ordering AFAICS.

Yes, it is, there are assumptions in the governors regarding this.
Sorry for the noise.

On Sat, 2025-11-08 at 12:48 +0100, Rafael J. Wysocki wrote:
> > > +static int validate_state(struct cpuidle_state *s, struct cpuidle_state *prev_s)
> > > +{
> > > +       if (s->exit_latency == 0)
> > > +               return -EINVAL;
> > 
> > The change above will break the polling state AFAICS.

Thanks for feedback. Yes. I first implemented this in intel_idle.c,
where this made sense, but then moved to cpuidle.c and did not notice.

Thanks,
Artem.