[PATCH 0/3] cpufreq: Minor cleanups

[PATCH 0/3] cpufreq: Minor cleanups

[Viresh Kumar]

Hi Rafael,

This series contains few cleanups, with no dependency between the patches.

--
Viresh

Viresh Kumar (3):
  cpufreq: Optimize cpufreq_show_cpus()
  cpufreq: Panic if policy is active in cpufreq_policy_free()
  cpufreq: Drop unnecessary cpus locking from store()

 drivers/cpufreq/cpufreq.c | 33 +++++++++++++++------------------
 1 file changed, 15 insertions(+), 18 deletions(-)

-- 
2.31.1.272.g89b43f80a514

[PATCH 1/3] cpufreq: Optimize cpufreq_show_cpus()

[Viresh Kumar]

Instead of specially adding a space for each CPU, except the first one,
lets add space for each of them and remove it at the end.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 2cad42774164..e24aa5d4bca5 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -843,12 +843,14 @@ ssize_t cpufreq_show_cpus(const struct cpumask *mask, char *buf)
 	unsigned int cpu;
 
 	for_each_cpu(cpu, mask) {
-		if (i)
-			i += scnprintf(&buf[i], (PAGE_SIZE - i - 2), " ");
-		i += scnprintf(&buf[i], (PAGE_SIZE - i - 2), "%u", cpu);
+		i += scnprintf(&buf[i], (PAGE_SIZE - i - 2), "%u ", cpu);
 		if (i >= (PAGE_SIZE - 5))
 			break;
 	}
+
+	/* Remove the extra space at the end */
+	i--;
+
 	i += sprintf(&buf[i], "\n");
 	return i;
 }
-- 
2.31.1.272.g89b43f80a514

[PATCH 2/3] cpufreq: Panic if policy is active in cpufreq_policy_free()

[Viresh Kumar]

With the new design in place, show/store callbacks check if the policy
is active or not before proceeding further and cpufreq_policy_free()
must be called after emptying policy->cpus mask, i.e. inactive policy.

Lets make sure we don't get a bug around this later and catch this early
by putting a BUG_ON() within cpufreq_policy_free().

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index e24aa5d4bca5..53d163a84e06 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1284,6 +1284,12 @@ static void cpufreq_policy_free(struct cpufreq_policy *policy)
 	unsigned long flags;
 	int cpu;
 
+	/*
+	 * The callers must ensure the policy is inactive by now, to avoid any
+	 * races with show()/store() callbacks.
+	 */
+	BUG_ON(!policy_is_inactive(policy));
+
 	/* Remove policy from list */
 	write_lock_irqsave(&cpufreq_driver_lock, flags);
 	list_del(&policy->policy_list);
-- 
2.31.1.272.g89b43f80a514

[PATCH V2 2/3] cpufreq: Panic if policy is active in cpufreq_policy_free()

[Viresh Kumar]

With the new design in place, to avoid potential races show() and
store() callbacks check if the policy is active or not before proceeding
any further. And in order to guarantee that cpufreq_policy_free() must
be called after clearing the policy->cpus mask, i.e. by marking it
inactive.

Lets make sure we don't get a bug around this later and catch this early
by putting a BUG_ON() within cpufreq_policy_free().

Also update cpufreq_online() a bit to make sure we clear the cpus mask
for each error case before calling cpufreq_policy_free().

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
V2: Update cpufreq_online() and changelog.

 drivers/cpufreq/cpufreq.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index e24aa5d4bca5..0f8245731783 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1284,6 +1284,12 @@ static void cpufreq_policy_free(struct cpufreq_policy *policy)
 	unsigned long flags;
 	int cpu;
 
+	/*
+	 * The callers must ensure the policy is inactive by now, to avoid any
+	 * races with show()/store() callbacks.
+	 */
+	BUG_ON(!policy_is_inactive(policy));
+
 	/* Remove policy from list */
 	write_lock_irqsave(&cpufreq_driver_lock, flags);
 	list_del(&policy->policy_list);
@@ -1538,8 +1544,6 @@ static int cpufreq_online(unsigned int cpu)
 	for_each_cpu(j, policy->real_cpus)
 		remove_cpu_dev_symlink(policy, j, get_cpu_device(j));
 
-	cpumask_clear(policy->cpus);
-
 out_offline_policy:
 	if (cpufreq_driver->offline)
 		cpufreq_driver->offline(policy);
@@ -1549,6 +1553,7 @@ static int cpufreq_online(unsigned int cpu)
 		cpufreq_driver->exit(policy);
 
 out_free_policy:
+	cpumask_clear(policy->cpus);
 	up_write(&policy->rwsem);
 
 	cpufreq_policy_free(policy);
-- 
2.31.1.272.g89b43f80a514

Re: [PATCH V2 2/3] cpufreq: Panic if policy is active in cpufreq_policy_free()

[Rafael J. Wysocki]

On Fri, May 27, 2022 at 5:53 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> With the new design in place, to avoid potential races show() and
> store() callbacks check if the policy is active or not before proceeding
> any further. And in order to guarantee that cpufreq_policy_free() must
> be called after clearing the policy->cpus mask, i.e. by marking it
> inactive.
>
> Lets make sure we don't get a bug around this later and catch this early
> by putting a BUG_ON() within cpufreq_policy_free().
>
> Also update cpufreq_online() a bit to make sure we clear the cpus mask
> for each error case before calling cpufreq_policy_free().
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> V2: Update cpufreq_online() and changelog.
>
>  drivers/cpufreq/cpufreq.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index e24aa5d4bca5..0f8245731783 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -1284,6 +1284,12 @@ static void cpufreq_policy_free(struct cpufreq_policy *policy)
>         unsigned long flags;
>         int cpu;
>
> +       /*
> +        * The callers must ensure the policy is inactive by now, to avoid any
> +        * races with show()/store() callbacks.
> +        */
> +       BUG_ON(!policy_is_inactive(policy));

I'm not a super-big fan of this change.

First off, crashing the kernel outright here because of possible races
appears a bit excessive to me.

Second, it looks like we are worrying about the code running before
the wait_for_completion() call in cpufreq_policy_put_kobj(), because
after that call no one can be running show() or store().  So why don't
we reorder the wait_for_completion() call with respect to the code in
question instead?

> +
>         /* Remove policy from list */
>         write_lock_irqsave(&cpufreq_driver_lock, flags);
>         list_del(&policy->policy_list);
> @@ -1538,8 +1544,6 @@ static int cpufreq_online(unsigned int cpu)
>         for_each_cpu(j, policy->real_cpus)
>                 remove_cpu_dev_symlink(policy, j, get_cpu_device(j));
>
> -       cpumask_clear(policy->cpus);
> -
>  out_offline_policy:
>         if (cpufreq_driver->offline)
>                 cpufreq_driver->offline(policy);
> @@ -1549,6 +1553,7 @@ static int cpufreq_online(unsigned int cpu)
>                 cpufreq_driver->exit(policy);
>
>  out_free_policy:
> +       cpumask_clear(policy->cpus);
>         up_write(&policy->rwsem);
>
>         cpufreq_policy_free(policy);
> --
> 2.31.1.272.g89b43f80a514
>

Re: [PATCH V2 2/3] cpufreq: Panic if policy is active in cpufreq_policy_free()

[Viresh Kumar]

On 14-06-22, 15:59, Rafael J. Wysocki wrote:
> On Fri, May 27, 2022 at 5:53 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > With the new design in place, to avoid potential races show() and
> > store() callbacks check if the policy is active or not before proceeding
> > any further. And in order to guarantee that cpufreq_policy_free() must
> > be called after clearing the policy->cpus mask, i.e. by marking it
> > inactive.
> >
> > Lets make sure we don't get a bug around this later and catch this early
> > by putting a BUG_ON() within cpufreq_policy_free().
> >
> > Also update cpufreq_online() a bit to make sure we clear the cpus mask
> > for each error case before calling cpufreq_policy_free().
> >
> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> > ---
> > V2: Update cpufreq_online() and changelog.
> >
> >  drivers/cpufreq/cpufreq.c | 9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > index e24aa5d4bca5..0f8245731783 100644
> > --- a/drivers/cpufreq/cpufreq.c
> > +++ b/drivers/cpufreq/cpufreq.c
> > @@ -1284,6 +1284,12 @@ static void cpufreq_policy_free(struct cpufreq_policy *policy)
> >         unsigned long flags;
> >         int cpu;
> >
> > +       /*
> > +        * The callers must ensure the policy is inactive by now, to avoid any
> > +        * races with show()/store() callbacks.
> > +        */
> > +       BUG_ON(!policy_is_inactive(policy));
> 
> I'm not a super-big fan of this change.
> 
> First off, crashing the kernel outright here because of possible races
> appears a bit excessive to me.
> 
> Second, it looks like we are worrying about the code running before
> the wait_for_completion() call in cpufreq_policy_put_kobj(), because
> after that call no one can be running show() or store().  So why don't
> we reorder the wait_for_completion() call with respect to the code in
> question instead?

No, I am not worrying about that race. I am just trying to make sure some change
in future doesn't break this assumption (that policy should be inactive by this
point). That's all. It all looks good for now.

May be a WARN instead of BUG if we don't want to crash.

-- 
viresh

Re: [PATCH V2 2/3] cpufreq: Panic if policy is active in cpufreq_policy_free()

[Rafael J. Wysocki]

On Wed, Jun 15, 2022 at 7:00 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 14-06-22, 15:59, Rafael J. Wysocki wrote:
> > On Fri, May 27, 2022 at 5:53 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > >
> > > With the new design in place, to avoid potential races show() and
> > > store() callbacks check if the policy is active or not before proceeding
> > > any further. And in order to guarantee that cpufreq_policy_free() must
> > > be called after clearing the policy->cpus mask, i.e. by marking it
> > > inactive.
> > >
> > > Lets make sure we don't get a bug around this later and catch this early
> > > by putting a BUG_ON() within cpufreq_policy_free().
> > >
> > > Also update cpufreq_online() a bit to make sure we clear the cpus mask
> > > for each error case before calling cpufreq_policy_free().
> > >
> > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> > > ---
> > > V2: Update cpufreq_online() and changelog.
> > >
> > >  drivers/cpufreq/cpufreq.c | 9 +++++++--
> > >  1 file changed, 7 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > > index e24aa5d4bca5..0f8245731783 100644
> > > --- a/drivers/cpufreq/cpufreq.c
> > > +++ b/drivers/cpufreq/cpufreq.c
> > > @@ -1284,6 +1284,12 @@ static void cpufreq_policy_free(struct cpufreq_policy *policy)
> > >         unsigned long flags;
> > >         int cpu;
> > >
> > > +       /*
> > > +        * The callers must ensure the policy is inactive by now, to avoid any
> > > +        * races with show()/store() callbacks.
> > > +        */
> > > +       BUG_ON(!policy_is_inactive(policy));
> >
> > I'm not a super-big fan of this change.
> >
> > First off, crashing the kernel outright here because of possible races
> > appears a bit excessive to me.
> >
> > Second, it looks like we are worrying about the code running before
> > the wait_for_completion() call in cpufreq_policy_put_kobj(), because
> > after that call no one can be running show() or store().  So why don't
> > we reorder the wait_for_completion() call with respect to the code in
> > question instead?
>
> No, I am not worrying about that race. I am just trying to make sure some change
> in future doesn't break this assumption (that policy should be inactive by this
> point). That's all. It all looks good for now.
>
> May be a WARN instead of BUG if we don't want to crash.

WARN_ON() would be somewhat better, but then I'm not sure if having a
full call trace in this case is really useful, because we know when
cpufreq_policy_free() can be called anyway.

Maybe just print a warning message.

Re: [PATCH V2 2/3] cpufreq: Panic if policy is active in cpufreq_policy_free()

[Viresh Kumar]

On 06-07-22, 15:49, Rafael J. Wysocki wrote:
> WARN_ON() would be somewhat better, but then I'm not sure if having a
> full call trace in this case is really useful, because we know when
> cpufreq_policy_free() can be called anyway.
> 
> Maybe just print a warning message.

The warning will get printed, yes, but I am sure everyone will end up
ignoring it, once it happens.

One of the benefits of printing the call-stack is people will take it
seriously and report it, and we won't miss a bug, if one gets in
somehow.

-- 
viresh

Re: [PATCH V2 2/3] cpufreq: Panic if policy is active in cpufreq_policy_free()

[Rafael J. Wysocki]

On Wed, Jul 6, 2022 at 5:23 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 06-07-22, 15:49, Rafael J. Wysocki wrote:
> > WARN_ON() would be somewhat better, but then I'm not sure if having a
> > full call trace in this case is really useful, because we know when
> > cpufreq_policy_free() can be called anyway.
> >
> > Maybe just print a warning message.
>
> The warning will get printed, yes, but I am sure everyone will end up
> ignoring it, once it happens.
>
> One of the benefits of printing the call-stack is people will take it
> seriously and report it, and we won't miss a bug, if one gets in
> somehow.

I'd rather not go into discussing things that people may or may not do and why.

My point is that if WARN_ON() gets converted to panic(), they will not
see the message at all and if the message gets printed, they will have
a chance to see it even in that case.  Whether or not they use that
chance as desirable is beyond the scope of engineering IMV.

Re: [cpufreq] a6cb305191: kernel_BUG_at_drivers/cpufreq/cpufreq.c

[Viresh Kumar]

On 27-05-22, 11:13, kernel test robot wrote:
> [ 10.349308][ T200] cpufreq_online (drivers/cpufreq/cpufreq.c:1562) 
> [ 10.349314][ T200] cpufreq_add_dev (drivers/cpufreq/cpufreq.c:1578) 
> [ 10.349318][ T200] subsys_interface_register (drivers/base/bus.c:1036) 

Thanks. I have updated the patch now to fix this.

-- 
viresh

[PATCH 3/3] cpufreq: Drop unnecessary cpus locking from store()

[Viresh Kumar]

This change was introduced long back by:

commit 4f750c930822 ("cpufreq: Synchronize the cpufreq store_*() routines with CPU hotplug")

Since then, both cpufreq and hotplug core have been reworked and have
much better locking in place. The race mentioned in commit 4f750c930822
isn't possible anymore.

Drop the unnecessary locking.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c | 19 ++++---------------
 1 file changed, 4 insertions(+), 15 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 53d163a84e06..bb237d1ce5e7 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -973,21 +973,10 @@ static ssize_t store(struct kobject *kobj, struct attribute *attr,
 	if (!fattr->store)
 		return -EIO;
 
-	/*
-	 * cpus_read_trylock() is used here to work around a circular lock
-	 * dependency problem with respect to the cpufreq_register_driver().
-	 */
-	if (!cpus_read_trylock())
-		return -EBUSY;
-
-	if (cpu_online(policy->cpu)) {
-		down_write(&policy->rwsem);
-		if (likely(!policy_is_inactive(policy)))
-			ret = fattr->store(policy, buf, count);
-		up_write(&policy->rwsem);
-	}
-
-	cpus_read_unlock();
+	down_write(&policy->rwsem);
+	if (likely(!policy_is_inactive(policy)))
+		ret = fattr->store(policy, buf, count);
+	up_write(&policy->rwsem);
 
 	return ret;
 }
-- 
2.31.1.272.g89b43f80a514

Re: [PATCH 3/3] cpufreq: Drop unnecessary cpus locking from store()

[Rafael J. Wysocki]

On Thu, May 26, 2022 at 1:51 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> This change was introduced long back by:
>
> commit 4f750c930822 ("cpufreq: Synchronize the cpufreq store_*() routines with CPU hotplug")
>
> Since then, both cpufreq and hotplug core have been reworked and have
> much better locking in place. The race mentioned in commit 4f750c930822
> isn't possible anymore.
>
> Drop the unnecessary locking.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/cpufreq/cpufreq.c | 19 ++++---------------
>  1 file changed, 4 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 53d163a84e06..bb237d1ce5e7 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -973,21 +973,10 @@ static ssize_t store(struct kobject *kobj, struct attribute *attr,
>         if (!fattr->store)
>                 return -EIO;
>
> -       /*
> -        * cpus_read_trylock() is used here to work around a circular lock
> -        * dependency problem with respect to the cpufreq_register_driver().
> -        */
> -       if (!cpus_read_trylock())
> -               return -EBUSY;
> -
> -       if (cpu_online(policy->cpu)) {
> -               down_write(&policy->rwsem);
> -               if (likely(!policy_is_inactive(policy)))
> -                       ret = fattr->store(policy, buf, count);
> -               up_write(&policy->rwsem);
> -       }
> -
> -       cpus_read_unlock();
> +       down_write(&policy->rwsem);
> +       if (likely(!policy_is_inactive(policy)))
> +               ret = fattr->store(policy, buf, count);
> +       up_write(&policy->rwsem);
>
>         return ret;
>  }
> --

Applied along with the [1/3] as 5.20 material, thanks!

[PATCH 0/3] cpufreq: Minor cleanups

[Viresh Kumar]

Hi Rafael,

Here are a bunch of cleanups for cpufreq-core.

Viresh Kumar (3):
  cpufreq: Relocate handle_update() to kill its declaration
  cpufreq: Rename __cpufreq_governor() to cpufreq_governor()
  cpufreq: Remove 'policy->governor_enabled'

 drivers/cpufreq/cpufreq.c | 80 ++++++++++++++++++-----------------------------
 include/linux/cpufreq.h   |  1 -
 2 files changed, 30 insertions(+), 51 deletions(-)

-- 
2.7.1.410.g6faf27b