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

Groups > linux.kernel > #1306547 > unrolled thread

[RFC PATCH 08/19] cpufreq: fix warning for cpufreq_init_policy unlocked access to cpufreq_governor_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 08/19] cpufreq: fix warning for cpufreq_init_policy unlocked access to cpufreq_governor_list Juri Lelli <juri.lelli@arm.com> - 2016-01-11 18:40 +0100
    Re: [RFC PATCH 08/19] cpufreq: fix warning for cpufreq_init_policy  unlocked access to cpufreq_governor_list Viresh Kumar <viresh.kumar@linaro.org> - 2016-01-12 11:10 +0100
      Re: [RFC PATCH 08/19] cpufreq: fix warning for cpufreq_init_policy  unlocked access to cpufreq_governor_list Juri Lelli <juri.lelli@arm.com> - 2016-01-12 17:00 +0100
        Re: [RFC PATCH 08/19] cpufreq: fix warning for cpufreq_init_policy  unlocked access to cpufreq_governor_list Viresh Kumar <viresh.kumar@linaro.org> - 2016-01-13 07:10 +0100
          Re: [RFC PATCH 08/19] cpufreq: fix warning for cpufreq_init_policy  unlocked access to cpufreq_governor_list Juri Lelli <juri.lelli@arm.com> - 2016-01-14 17:40 +0100

#1306547 — [RFC PATCH 08/19] cpufreq: fix warning for cpufreq_init_policy unlocked access to cpufreq_governor_list

cpufreq_init_policy calls find_governor, which iterates through
cpufreq_governor_list.  cpufreq_governor_mutex has to be held before
calling that function or the following warning will be generated:

[    8.100161] cpu cpu0: bL_cpufreq_init: CPU 0 initialized
[    8.164477] ------------[ cut here ]------------
[    8.225164] WARNING: CPU: 2 PID: 1 at kernel/drivers/cpufreq/cpufreq.c:512 find_governor+0x57/0x68()
[    8.356296] Modules linked in:
[    8.411252] CPU: 2 PID: 1 Comm: swapper/0 Not tainted 4.4.0-rc2+ #298
[    8.477501] Hardware name: ARM-Versatile Express
[    8.538416] [<c0014215>] (unwind_backtrace) from [<c0010e25>] (show_stack+0x11/0x14)
[    8.657973] [<c0010e25>] (show_stack) from [<c02eca5d>] (dump_stack+0x55/0x78)
[    8.778347] [<c02eca5d>] (dump_stack) from [<c00202cd>] (warn_slowpath_common+0x59/0x84)
[    8.888775] usb 1-1.2: new high-speed USB device number 3 using isp1760
[    8.981012] [<c00202cd>] (warn_slowpath_common) from [<c002030f>] (warn_slowpath_null+0x17/0x1c)
[    8.993193] usb-storage 1-1.2:1.0: USB Mass Storage device detected
[    8.995167] scsi host0: usb-storage 1-1.2:1.0
[    9.260384] [<c002030f>] (warn_slowpath_null) from [<c03b7caf>] (find_governor+0x57/0x68)
[    9.395241] [<c03b7caf>] (find_governor) from [<c03b917d>] (cpufreq_init_policy+0x21/0x50)
[    9.532811] [<c03b917d>] (cpufreq_init_policy) from [<c03b9391>] (cpufreq_online+0x1e5/0x530)
[    9.676622] [<c03b9391>] (cpufreq_online) from [<c033fc9f>] (subsys_interface_register+0x53/0x78)
[    9.826894] [<c033fc9f>] (subsys_interface_register) from [<c03b986f>] (cpufreq_register_driver+0x9f/0x108)
[    9.981174] [<c03b986f>] (cpufreq_register_driver) from [<c03bb9b1>] (bL_cpufreq_register+0x49/0x98)
[   10.002780] scsi 0:0:0:0: Direct-Access     General  USB Flash Disk   1.0  PQ: 0 ANSI: 2
[   10.024039] sd 0:0:0:0: [sda] 7831552 512-byte logical blocks: (4.00 GB/3.73 GiB)
[   10.030505] sd 0:0:0:0: [sda] Write Protect is off
[   10.030544] sd 0:0:0:0: [sda] Mode Sense: 03 00 00 00
[   10.039631] sd 0:0:0:0: [sda] No Caching mode page found
[   10.039672] sd 0:0:0:0: [sda] Assuming drive cache: write through
[   10.093331]  sda: sda1 sda2
[   10.138827] sd 0:0:0:0: [sda] Attached SCSI removable disk
[   10.883930] [<c03bb9b1>] (bL_cpufreq_register) from [<c034203f>] (platform_drv_probe+0x3b/0x6c)
[   11.024538] [<c034203f>] (platform_drv_probe) from [<c0340e2f>] (driver_probe_device+0x16f/0x1c0)
[   11.167217] [<c0340e2f>] (driver_probe_device) from [<c0340ed7>] (__driver_attach+0x57/0x58)
[   11.308084] [<c0340ed7>] (__driver_attach) from [<c033fd41>] (bus_for_each_dev+0x2d/0x4c)
[   11.448056] [<c033fd41>] (bus_for_each_dev) from [<c034077f>] (bus_add_driver+0xa3/0x14c)
[   11.587859] [<c034077f>] (bus_add_driver) from [<c0341727>] (driver_register+0x3b/0x88)
[   11.726813] [<c0341727>] (driver_register) from [<c0009613>] (do_one_initcall+0x5b/0x150)
[   11.866247] [<c0009613>] (do_one_initcall) from [<c0732b45>] (kernel_init_freeable+0x18d/0x22c)
[   12.008765] [<c0732b45>] (kernel_init_freeable) from [<c04f24ed>] (kernel_init+0xd/0xa4)
[   12.150461] [<c04f24ed>] (kernel_init) from [<c000dfb9>] (ret_from_fork+0x11/0x38)
[   12.294473] ---[ end trace 545905b1fdc9cd96 ]---
[   12.371823] atkbd serio0: keyboard reset failed on 1c060000.kmi
[   12.372910] cpu cpu1: bL_cpufreq_init: CPU 1 initialized
[   12.373741] ------------[ cut here ]------------

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 | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 7dae7f3..d065435 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -969,6 +969,7 @@ static int cpufreq_init_policy(struct cpufreq_policy *policy)
 
 	memcpy(&new_policy, policy, sizeof(*policy));
 
+	mutex_lock(&cpufreq_governor_mutex);
 	/* Update governor of new_policy to the governor used before hotplug */
 	gov = find_governor(policy->last_governor);
 	if (gov)
@@ -976,6 +977,7 @@ static int cpufreq_init_policy(struct cpufreq_policy *policy)
 				policy->governor->name, policy->cpu);
 	else
 		gov = CPUFREQ_DEFAULT_GOVERNOR;
+	mutex_unlock(&cpufreq_governor_mutex);
 
 	new_policy.governor = gov;
 
-- 
2.2.2

[toc] | [next] | [standalone]

#1307200 — Re: [RFC PATCH 08/19] cpufreq: fix warning for cpufreq_init_policy unlocked access to cpufreq_governor_list

On 11-01-16, 17:35, Juri Lelli wrote:
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 7dae7f3..d065435 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -969,6 +969,7 @@ static int cpufreq_init_policy(struct cpufreq_policy *policy)
>  
>  	memcpy(&new_policy, policy, sizeof(*policy));
>  
> +	mutex_lock(&cpufreq_governor_mutex);
>  	/* Update governor of new_policy to the governor used before hotplug */
>  	gov = find_governor(policy->last_governor);

You should take the lock within find_governor() instead, i.e.  around
the while loop.

>  	if (gov)
> @@ -976,6 +977,7 @@ static int cpufreq_init_policy(struct cpufreq_policy *policy)
>  				policy->governor->name, policy->cpu);
>  	else
>  		gov = CPUFREQ_DEFAULT_GOVERNOR;
> +	mutex_unlock(&cpufreq_governor_mutex);
>  
>  	new_policy.governor = gov;
>  
> -- 
> 2.2.2

-- 
viresh

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

#1307575 — Re: [RFC PATCH 08/19] cpufreq: fix warning for cpufreq_init_policy unlocked access to cpufreq_governor_list

On 12/01/16 15:39, Viresh Kumar wrote:
> On 11-01-16, 17:35, Juri Lelli wrote:
> > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > index 7dae7f3..d065435 100644
> > --- a/drivers/cpufreq/cpufreq.c
> > +++ b/drivers/cpufreq/cpufreq.c
> > @@ -969,6 +969,7 @@ static int cpufreq_init_policy(struct cpufreq_policy *policy)
> >  
> >  	memcpy(&new_policy, policy, sizeof(*policy));
> >  
> > +	mutex_lock(&cpufreq_governor_mutex);
> >  	/* Update governor of new_policy to the governor used before hotplug */
> >  	gov = find_governor(policy->last_governor);
> 
> You should take the lock within find_governor() instead, i.e.  around
> the while loop.
> 

Other users (i.e., cpufreq_parse_governor and cpufreq_register_governor)
needs to take the mutex externally. So, we need to unify this behaviour.

Best,

- Juri

> >  	if (gov)
> > @@ -976,6 +977,7 @@ static int cpufreq_init_policy(struct cpufreq_policy *policy)
> >  				policy->governor->name, policy->cpu);
> >  	else
> >  		gov = CPUFREQ_DEFAULT_GOVERNOR;
> > +	mutex_unlock(&cpufreq_governor_mutex);
> >  
> >  	new_policy.governor = gov;
> >  
> > -- 
> > 2.2.2
> 
> -- 
> viresh
> 

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

#1308067 — Re: [RFC PATCH 08/19] cpufreq: fix warning for cpufreq_init_policy unlocked access to cpufreq_governor_list

On 12-01-16, 15:52, Juri Lelli wrote:
> Other users (i.e., cpufreq_parse_governor and cpufreq_register_governor)
> needs to take the mutex externally. So, we need to unify this behaviour.

No they don't have to.

And that's why I have been saying that we better nail down the exact
thing the mutex is supposed to protect.

There can be two cases here:
- It protects the governor list, in that case we can move it to
  find_governor().
- It guarantees that the governor pointer stays valid: That's not true
  as we are using the governor pointer outside of the lock.

And so I said, "No they don't have to" :)

-- 
viresh

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

#1309473 — Re: [RFC PATCH 08/19] cpufreq: fix warning for cpufreq_init_policy unlocked access to cpufreq_governor_list

On 13/01/16 11:37, Viresh Kumar wrote:
> On 12-01-16, 15:52, Juri Lelli wrote:
> > Other users (i.e., cpufreq_parse_governor and cpufreq_register_governor)
> > needs to take the mutex externally. So, we need to unify this behaviour.
> 
> No they don't have to.
> 
> And that's why I have been saying that we better nail down the exact
> thing the mutex is supposed to protect.
> 
> There can be two cases here:
> - It protects the governor list, in that case we can move it to
>   find_governor().
> - It guarantees that the governor pointer stays valid: That's not true
>   as we are using the governor pointer outside of the lock.
> 
> And so I said, "No they don't have to" :)
> 

But, don't we have to guarantee consinstency between multiple operations
on cpufreq_governor_list?

In cpufreq_register_governor() we have:

 mutex_lock(&cpufreq_governor_mutex);
 
 governor->initialized = 0;
 err = -EBUSY;
 if (!find_governor(governor->name)) {
 	err = 0;
 	list_add(&governor->governor_list, &cpufreq_governor_list);
 }
 
 mutex_unlock(&cpufreq_governor_mutex);

IIUC, find_governor and list_add have to be atomic. Couldn't someone
slip in right after find_governor and add the same governor to the list?

Thanks,

- Juri

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web