Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1306568 > unrolled thread

[RFC PATCH 05/19] cpufreq: assert locking when accessing cpufreq_policy_list

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [RFC PATCH 05/19] cpufreq: assert locking when accessing cpufreq_policy_list Juri Lelli <juri.lelli@arm.com> - 2016-01-11 18:50 +0100
    Re: [RFC PATCH 05/19] cpufreq: assert locking when accessing  cpufreq_policy_list Viresh Kumar <viresh.kumar@linaro.org> - 2016-01-12 10:40 +0100
      Re: [RFC PATCH 05/19] cpufreq: assert locking when accessing  cpufreq_policy_list Juri Lelli <juri.lelli@arm.com> - 2016-01-12 12:50 +0100
        Re: [RFC PATCH 05/19] cpufreq: assert locking when accessing  cpufreq_policy_list Viresh Kumar <viresh.kumar@linaro.org> - 2016-01-13 07:10 +0100

#1306568 — [RFC PATCH 05/19] cpufreq: assert locking when accessing cpufreq_policy_list

cpufreq_policy_list is guarded by cpufreq_driver_lock. Add appropriate
locking assertions to check that we always access the list while holding
the associated lock.

Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
Signed-off-by: Juri Lelli <juri.lelli@arm.com>
---
 drivers/cpufreq/cpufreq.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 00a00cd..63d6efb 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -65,6 +65,7 @@ static bool suitable_policy(struct cpufreq_policy *policy, bool active)
 static struct cpufreq_policy *next_policy(struct cpufreq_policy *policy,
 					  bool active)
 {
+	lockdep_assert_held(&cpufreq_driver_lock);
 	do {
 		policy = list_next_entry(policy, policy_list);
 
@@ -80,6 +81,7 @@ static struct cpufreq_policy *first_policy(bool active)
 {
 	struct cpufreq_policy *policy;
 
+	lockdep_assert_held(&cpufreq_driver_lock);
 	/* No policies in the list */
 	if (list_empty(&cpufreq_policy_list))
 		return NULL;
@@ -2430,6 +2432,7 @@ int cpufreq_register_driver(struct cpufreq_driver *driver_data)
 	if (ret)
 		goto err_boost_unreg;
 
+	lockdep_assert_held(&cpufreq_driver_lock);
 	if (!(cpufreq_driver->flags & CPUFREQ_STICKY) &&
 	    list_empty(&cpufreq_policy_list)) {
 		/* if all ->init() calls failed, unregister */
-- 
2.2.2

[toc] | [next] | [standalone]

#1307154 — Re: [RFC PATCH 05/19] cpufreq: assert locking when accessing cpufreq_policy_list

On 11-01-16, 17:35, Juri Lelli wrote:
> cpufreq_policy_list is guarded by cpufreq_driver_lock. Add appropriate
> locking assertions to check that we always access the list while holding
> the associated lock.
> 
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Viresh Kumar <viresh.kumar@linaro.org>
> Signed-off-by: Juri Lelli <juri.lelli@arm.com>
> ---
>  drivers/cpufreq/cpufreq.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 00a00cd..63d6efb 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -65,6 +65,7 @@ static bool suitable_policy(struct cpufreq_policy *policy, bool active)
>  static struct cpufreq_policy *next_policy(struct cpufreq_policy *policy,
>  					  bool active)
>  {
> +	lockdep_assert_held(&cpufreq_driver_lock);
>  	do {
>  		policy = list_next_entry(policy, policy_list);
>  
> @@ -80,6 +81,7 @@ static struct cpufreq_policy *first_policy(bool active)
>  {
>  	struct cpufreq_policy *policy;
>  
> +	lockdep_assert_held(&cpufreq_driver_lock);

Because both first_policy() and next_policy() are parts of
for_each_suitable_policy() macro, checking this in first_policy() is
sufficient. next_policy() isn't designed to be used by any other code.

>  	/* No policies in the list */
>  	if (list_empty(&cpufreq_policy_list))
>  		return NULL;
> @@ -2430,6 +2432,7 @@ int cpufreq_register_driver(struct cpufreq_driver *driver_data)
>  	if (ret)
>  		goto err_boost_unreg;
>  
> +	lockdep_assert_held(&cpufreq_driver_lock);

Why do you need a cpufreq_driver_lock here? And the above change
should generate a lockdep here as the lock isn't taken right now.

>  	if (!(cpufreq_driver->flags & CPUFREQ_STICKY) &&
>  	    list_empty(&cpufreq_policy_list)) {
>  		/* if all ->init() calls failed, unregister */
> -- 
> 2.2.2

-- 
viresh

[toc] | [prev] | [next] | [standalone]

#1307307 — Re: [RFC PATCH 05/19] cpufreq: assert locking when accessing cpufreq_policy_list

Hi,

On 12/01/16 15:04, Viresh Kumar wrote:
> On 11-01-16, 17:35, Juri Lelli wrote:
> > cpufreq_policy_list is guarded by cpufreq_driver_lock. Add appropriate
> > locking assertions to check that we always access the list while holding
> > the associated lock.
> > 
> > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> > Cc: Viresh Kumar <viresh.kumar@linaro.org>
> > Signed-off-by: Juri Lelli <juri.lelli@arm.com>
> > ---
> >  drivers/cpufreq/cpufreq.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > index 00a00cd..63d6efb 100644
> > --- a/drivers/cpufreq/cpufreq.c
> > +++ b/drivers/cpufreq/cpufreq.c
> > @@ -65,6 +65,7 @@ static bool suitable_policy(struct cpufreq_policy *policy, bool active)
> >  static struct cpufreq_policy *next_policy(struct cpufreq_policy *policy,
> >  					  bool active)
> >  {
> > +	lockdep_assert_held(&cpufreq_driver_lock);
> >  	do {
> >  		policy = list_next_entry(policy, policy_list);
> >  
> > @@ -80,6 +81,7 @@ static struct cpufreq_policy *first_policy(bool active)
> >  {
> >  	struct cpufreq_policy *policy;
> >  
> > +	lockdep_assert_held(&cpufreq_driver_lock);
> 
> Because both first_policy() and next_policy() are parts of
> for_each_suitable_policy() macro, checking this in first_policy() is
> sufficient. next_policy() isn't designed to be used by any other code.
> 

But next_policy is called multiple times as part of
for_each_suitable_policy().  What if someone thinks she/he can release
cpufreq_driver_lock inside for_each_(in)active_policy() loop? Not that
it makes sense, but don't you think it could happen?

> >  	/* No policies in the list */
> >  	if (list_empty(&cpufreq_policy_list))
> >  		return NULL;
> > @@ -2430,6 +2432,7 @@ int cpufreq_register_driver(struct cpufreq_driver *driver_data)
> >  	if (ret)
> >  		goto err_boost_unreg;
> >  
> > +	lockdep_assert_held(&cpufreq_driver_lock);
> 
> Why do you need a cpufreq_driver_lock here? And the above change
> should generate a lockdep here as the lock isn't taken right now.
> 

Because you are checking cpufreq_policy_list to see if it's empty. And
it generates a lockdep warning, yes; fixed by next patch. Maybe putting
fixes before warnings, as you are suggesting, is better.

Thanks,

- Juri

> >  	if (!(cpufreq_driver->flags & CPUFREQ_STICKY) &&
> >  	    list_empty(&cpufreq_policy_list)) {
> >  		/* if all ->init() calls failed, unregister */
> > -- 
> > 2.2.2
> 
> -- 
> viresh
> 

[toc] | [prev] | [next] | [standalone]

#1308066 — Re: [RFC PATCH 05/19] cpufreq: assert locking when accessing cpufreq_policy_list

On 12-01-16, 11:44, Juri Lelli wrote:
> But next_policy is called multiple times as part of
> for_each_suitable_policy().  What if someone thinks she/he can release
> cpufreq_driver_lock inside for_each_(in)active_policy() loop? Not that
> it makes sense, but don't you think it could happen?

Okay, I don't have strong opinion about using that only in the first
routine. No issues.

> > >  	/* No policies in the list */
> > >  	if (list_empty(&cpufreq_policy_list))
> > >  		return NULL;
> > > @@ -2430,6 +2432,7 @@ int cpufreq_register_driver(struct cpufreq_driver *driver_data)
> > >  	if (ret)
> > >  		goto err_boost_unreg;
> > >  
> > > +	lockdep_assert_held(&cpufreq_driver_lock);
> > 
> > Why do you need a cpufreq_driver_lock here? And the above change
> > should generate a lockdep here as the lock isn't taken right now.
> > 
> 
> Because you are checking cpufreq_policy_list to see if it's empty. And
> it generates a lockdep warning, yes; fixed by next patch. Maybe putting
> fixes before warnings, as you are suggesting, is better.

Well, locking isn't required because we think we need to protect every
access of a variable (like cpufreq_policy_list here). But we need to
protect its access from possible races.

What I am saying is, we can't have a race here. And so no need to lock
it down.

-- 
viresh

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web