[PATCH] cpuidle: dt: fix opencoded for_each_cpu() in idle_state_valid()

[PATCH] cpuidle: dt: fix opencoded for_each_cpu() in idle_state_valid()

[Yury Norov]

From: Yury Norov [NVIDIA] <yury.norov@gmail.com>

The function opencodes the for_each_cpu_from() by using an open for-loop.
Fix that in sake of readability.

While there, drop the 'valid' variable as it's pretty useless here.

Signed-off-by: Yury Norov [NVIDIA] <yury.norov@gmail.com>
---
 drivers/cpuidle/dt_idle_states.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/cpuidle/dt_idle_states.c b/drivers/cpuidle/dt_idle_states.c
index 97feb7d8fb23..558d49838990 100644
--- a/drivers/cpuidle/dt_idle_states.c
+++ b/drivers/cpuidle/dt_idle_states.c
@@ -98,7 +98,6 @@ static bool idle_state_valid(struct device_node *state_node, unsigned int idx,
 {
 	int cpu;
 	struct device_node *cpu_node, *curr_state_node;
-	bool valid = true;
 
 	/*
 	 * Compare idle state phandles for index idx on all CPUs in the
@@ -107,20 +106,17 @@ static bool idle_state_valid(struct device_node *state_node, unsigned int idx,
 	 * retrieved from. If a mismatch is found bail out straight
 	 * away since we certainly hit a firmware misconfiguration.
 	 */
-	for (cpu = cpumask_next(cpumask_first(cpumask), cpumask);
-	     cpu < nr_cpu_ids; cpu = cpumask_next(cpu, cpumask)) {
+	cpu = cpumask_first(cpumask) + 1;
+	for_each_cpu_from(cpu, cpumask) {
 		cpu_node = of_cpu_device_node_get(cpu);
 		curr_state_node = of_get_cpu_state_node(cpu_node, idx);
-		if (state_node != curr_state_node)
-			valid = false;
-
 		of_node_put(curr_state_node);
 		of_node_put(cpu_node);
-		if (!valid)
-			break;
+		if (state_node != curr_state_node)
+			return false;
 	}
 
-	return valid;
+	return true;
 }
 
 /**
-- 
2.43.0

Re: [PATCH] cpuidle: dt: fix opencoded for_each_cpu() in idle_state_valid()

[Rafael J. Wysocki]

On Wed, Jun 4, 2025 at 11:39 PM Yury Norov <yury.norov@gmail.com> wrote:
>
> From: Yury Norov [NVIDIA] <yury.norov@gmail.com>
>
> The function opencodes the for_each_cpu_from() by using an open for-loop.
> Fix that in sake of readability.
>
> While there, drop the 'valid' variable as it's pretty useless here.
>
> Signed-off-by: Yury Norov [NVIDIA] <yury.norov@gmail.com>
> ---
>  drivers/cpuidle/dt_idle_states.c | 14 +++++---------
>  1 file changed, 5 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/cpuidle/dt_idle_states.c b/drivers/cpuidle/dt_idle_states.c
> index 97feb7d8fb23..558d49838990 100644
> --- a/drivers/cpuidle/dt_idle_states.c
> +++ b/drivers/cpuidle/dt_idle_states.c
> @@ -98,7 +98,6 @@ static bool idle_state_valid(struct device_node *state_node, unsigned int idx,
>  {
>         int cpu;
>         struct device_node *cpu_node, *curr_state_node;
> -       bool valid = true;
>
>         /*
>          * Compare idle state phandles for index idx on all CPUs in the
> @@ -107,20 +106,17 @@ static bool idle_state_valid(struct device_node *state_node, unsigned int idx,
>          * retrieved from. If a mismatch is found bail out straight
>          * away since we certainly hit a firmware misconfiguration.
>          */
> -       for (cpu = cpumask_next(cpumask_first(cpumask), cpumask);
> -            cpu < nr_cpu_ids; cpu = cpumask_next(cpu, cpumask)) {
> +       cpu = cpumask_first(cpumask) + 1;

Doing

cpu = cpumask_next(cpumask_first(cpumask), cpumask);

here might save a few iterations for sparse cpumasks.

> +       for_each_cpu_from(cpu, cpumask) {
>                 cpu_node = of_cpu_device_node_get(cpu);
>                 curr_state_node = of_get_cpu_state_node(cpu_node, idx);
> -               if (state_node != curr_state_node)
> -                       valid = false;
> -
>                 of_node_put(curr_state_node);
>                 of_node_put(cpu_node);
> -               if (!valid)
> -                       break;
> +               if (state_node != curr_state_node)
> +                       return false;
>         }
>
> -       return valid;
> +       return true;
>  }
>
>  /**
> --
> 2.43.0
>

Re: [PATCH] cpuidle: dt: fix opencoded for_each_cpu() in idle_state_valid()

[Yury Norov]

On Wed, Jul 02, 2025 at 08:27:21PM +0200, Rafael J. Wysocki wrote:
> On Wed, Jun 4, 2025 at 11:39 PM Yury Norov <yury.norov@gmail.com> wrote:
> >
> > From: Yury Norov [NVIDIA] <yury.norov@gmail.com>
> >
> > The function opencodes the for_each_cpu_from() by using an open for-loop.
> > Fix that in sake of readability.
> >
> > While there, drop the 'valid' variable as it's pretty useless here.
> >
> > Signed-off-by: Yury Norov [NVIDIA] <yury.norov@gmail.com>
> > ---
> >  drivers/cpuidle/dt_idle_states.c | 14 +++++---------
> >  1 file changed, 5 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/cpuidle/dt_idle_states.c b/drivers/cpuidle/dt_idle_states.c
> > index 97feb7d8fb23..558d49838990 100644
> > --- a/drivers/cpuidle/dt_idle_states.c
> > +++ b/drivers/cpuidle/dt_idle_states.c
> > @@ -98,7 +98,6 @@ static bool idle_state_valid(struct device_node *state_node, unsigned int idx,
> >  {
> >         int cpu;
> >         struct device_node *cpu_node, *curr_state_node;
> > -       bool valid = true;
> >
> >         /*
> >          * Compare idle state phandles for index idx on all CPUs in the
> > @@ -107,20 +106,17 @@ static bool idle_state_valid(struct device_node *state_node, unsigned int idx,
> >          * retrieved from. If a mismatch is found bail out straight
> >          * away since we certainly hit a firmware misconfiguration.
> >          */
> > -       for (cpu = cpumask_next(cpumask_first(cpumask), cpumask);
> > -            cpu < nr_cpu_ids; cpu = cpumask_next(cpu, cpumask)) {
> > +       cpu = cpumask_first(cpumask) + 1;
> 
> Doing
> 
> cpu = cpumask_next(cpumask_first(cpumask), cpumask);
> 
> here might save a few iterations for sparse cpumasks.

For that it's better to use cpumask_nth(1).

I believe there will be no benefit in calling cpumask_next() before
entering the loop because for_each_cpu_from() is based on it, and it
will be called anyways.

I can send v2 with cpumask_nth() change if you like it better. Please
let me know.

Thanks,
Yury
 
> > +       for_each_cpu_from(cpu, cpumask) {
> >                 cpu_node = of_cpu_device_node_get(cpu);
> >                 curr_state_node = of_get_cpu_state_node(cpu_node, idx);
> > -               if (state_node != curr_state_node)
> > -                       valid = false;
> > -
> >                 of_node_put(curr_state_node);
> >                 of_node_put(cpu_node);
> > -               if (!valid)
> > -                       break;
> > +               if (state_node != curr_state_node)
> > +                       return false;
> >         }
> >
> > -       return valid;
> > +       return true;
> >  }
> >
> >  /**
> > --
> > 2.43.0
> >

Re: [PATCH] cpuidle: dt: fix opencoded for_each_cpu() in idle_state_valid()

[Rafael J. Wysocki]

On Mon, Jul 7, 2025 at 5:31 PM Yury Norov <yury.norov@gmail.com> wrote:
>
> On Wed, Jul 02, 2025 at 08:27:21PM +0200, Rafael J. Wysocki wrote:
> > On Wed, Jun 4, 2025 at 11:39 PM Yury Norov <yury.norov@gmail.com> wrote:
> > >
> > > From: Yury Norov [NVIDIA] <yury.norov@gmail.com>
> > >
> > > The function opencodes the for_each_cpu_from() by using an open for-loop.
> > > Fix that in sake of readability.
> > >
> > > While there, drop the 'valid' variable as it's pretty useless here.
> > >
> > > Signed-off-by: Yury Norov [NVIDIA] <yury.norov@gmail.com>
> > > ---
> > >  drivers/cpuidle/dt_idle_states.c | 14 +++++---------
> > >  1 file changed, 5 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/drivers/cpuidle/dt_idle_states.c b/drivers/cpuidle/dt_idle_states.c
> > > index 97feb7d8fb23..558d49838990 100644
> > > --- a/drivers/cpuidle/dt_idle_states.c
> > > +++ b/drivers/cpuidle/dt_idle_states.c
> > > @@ -98,7 +98,6 @@ static bool idle_state_valid(struct device_node *state_node, unsigned int idx,
> > >  {
> > >         int cpu;
> > >         struct device_node *cpu_node, *curr_state_node;
> > > -       bool valid = true;
> > >
> > >         /*
> > >          * Compare idle state phandles for index idx on all CPUs in the
> > > @@ -107,20 +106,17 @@ static bool idle_state_valid(struct device_node *state_node, unsigned int idx,
> > >          * retrieved from. If a mismatch is found bail out straight
> > >          * away since we certainly hit a firmware misconfiguration.
> > >          */
> > > -       for (cpu = cpumask_next(cpumask_first(cpumask), cpumask);
> > > -            cpu < nr_cpu_ids; cpu = cpumask_next(cpu, cpumask)) {
> > > +       cpu = cpumask_first(cpumask) + 1;
> >
> > Doing
> >
> > cpu = cpumask_next(cpumask_first(cpumask), cpumask);
> >
> > here might save a few iterations for sparse cpumasks.
>
> For that it's better to use cpumask_nth(1).
>
> I believe there will be no benefit in calling cpumask_next() before
> entering the loop because for_each_cpu_from() is based on it, and it
> will be called anyways.

Fair enough.

Re: [PATCH] cpuidle: dt: fix opencoded for_each_cpu() in idle_state_valid()

[Rafael J. Wysocki]

On Mon, Jul 7, 2025 at 6:11 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Mon, Jul 7, 2025 at 5:31 PM Yury Norov <yury.norov@gmail.com> wrote:
> >
> > On Wed, Jul 02, 2025 at 08:27:21PM +0200, Rafael J. Wysocki wrote:
> > > On Wed, Jun 4, 2025 at 11:39 PM Yury Norov <yury.norov@gmail.com> wrote:
> > > >
> > > > From: Yury Norov [NVIDIA] <yury.norov@gmail.com>
> > > >
> > > > The function opencodes the for_each_cpu_from() by using an open for-loop.
> > > > Fix that in sake of readability.
> > > >
> > > > While there, drop the 'valid' variable as it's pretty useless here.
> > > >
> > > > Signed-off-by: Yury Norov [NVIDIA] <yury.norov@gmail.com>
> > > > ---
> > > >  drivers/cpuidle/dt_idle_states.c | 14 +++++---------
> > > >  1 file changed, 5 insertions(+), 9 deletions(-)
> > > >
> > > > diff --git a/drivers/cpuidle/dt_idle_states.c b/drivers/cpuidle/dt_idle_states.c
> > > > index 97feb7d8fb23..558d49838990 100644
> > > > --- a/drivers/cpuidle/dt_idle_states.c
> > > > +++ b/drivers/cpuidle/dt_idle_states.c
> > > > @@ -98,7 +98,6 @@ static bool idle_state_valid(struct device_node *state_node, unsigned int idx,
> > > >  {
> > > >         int cpu;
> > > >         struct device_node *cpu_node, *curr_state_node;
> > > > -       bool valid = true;
> > > >
> > > >         /*
> > > >          * Compare idle state phandles for index idx on all CPUs in the
> > > > @@ -107,20 +106,17 @@ static bool idle_state_valid(struct device_node *state_node, unsigned int idx,
> > > >          * retrieved from. If a mismatch is found bail out straight
> > > >          * away since we certainly hit a firmware misconfiguration.
> > > >          */
> > > > -       for (cpu = cpumask_next(cpumask_first(cpumask), cpumask);
> > > > -            cpu < nr_cpu_ids; cpu = cpumask_next(cpu, cpumask)) {
> > > > +       cpu = cpumask_first(cpumask) + 1;
> > >
> > > Doing
> > >
> > > cpu = cpumask_next(cpumask_first(cpumask), cpumask);
> > >
> > > here might save a few iterations for sparse cpumasks.
> >
> > For that it's better to use cpumask_nth(1).
> >
> > I believe there will be no benefit in calling cpumask_next() before
> > entering the loop because for_each_cpu_from() is based on it, and it
> > will be called anyways.
>
> Fair enough.

And so applied as 6.17 material, thanks!