Patchwork cpufreq: check if policy is inactive before calling __cpufreq_get

login
register
mail settings
Submitter Viresh Kumar
Date Jan. 7, 2019, 6:02 a.m.
Message ID <20190107060241.g7tc4uoa5225yglj@vireshk-i7>
Download mbox | patch
Permalink /patch/693557/
State New
Headers show

Comments

Viresh Kumar - Jan. 7, 2019, 6:02 a.m.
On 04-01-19, 11:42, Sudeep Holla wrote:
> cpuinfo_cur_freq gets current CPU frequency as detected by hardware
> while scaling_cur_freq last known CPU frequency. Some platforms may not
> allow checking the CPU frequency of an offline CPU or the associated
> resources may have been released via cpufreq_exit when the CPU gets
> offlined. If we attempt to get current frequency from the hardware,
> it may result in hang or crash.
> 
> For example on Juno, I see:
> 
> Unable to handle kernel NULL pointer dereference at virtual address 0000000000000188
> [0000000000000188] pgd=0000000000000000
> Internal error: Oops: 96000004 [#1] PREEMPT SMP
> Modules linked in:
> CPU: 5 PID: 4202 Comm: cat Not tainted 4.20.0-08251-ga0f2c0318a15-dirty #87
> Hardware name: ARM LTD ARM Juno Development Platform/ARM Juno Development Platform
> pstate: 40000005 (nZcv daif -PAN -UAO)
> pc : scmi_cpufreq_get_rate+0x34/0xb0
> lr : scmi_cpufreq_get_rate+0x34/0xb0
> Call trace:
>  scmi_cpufreq_get_rate+0x34/0xb0
>  __cpufreq_get+0x34/0xc0
>  show_cpuinfo_cur_freq+0x24/0x78
>  show+0x40/0x60
>  sysfs_kf_seq_show+0xc0/0x148
>  kernfs_seq_show+0x44/0x50
>  seq_read+0xd4/0x480
>  kernfs_fop_read+0x15c/0x208
>  __vfs_read+0x60/0x188
>  vfs_read+0x94/0x150
>  ksys_read+0x6c/0xd8
>  __arm64_sys_read+0x24/0x30
>  el0_svc_common+0x78/0x100
>  el0_svc_handler+0x38/0x78
>  el0_svc+0x8/0xc
> ---[ end trace 3d1024e58f77f6b2 ]---
> 
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Viresh Kumar <viresh.kumar@linaro.org>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  drivers/cpufreq/cpufreq.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Hi Viresh, Rafael,
> 
> I don't know the reason why we call __cpufreq_get here instead of
> cpufreq_get which checks if the policy is active or not.

We stopped removing the policy sysfs files, when the last CPU of the policy
exits, sometime back and probably missed this combination.

> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 6f23ebb395f1..cfc502385855 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -712,7 +712,7 @@ store_one(scaling_max_freq, max);
>  static ssize_t show_cpuinfo_cur_freq(struct cpufreq_policy *policy,
>  					char *buf)
>  {
> -	unsigned int cur_freq = __cpufreq_get(policy);
> +	unsigned int cur_freq = cpufreq_get(policy->cpu);
>  
>  	if (cur_freq)
>  		return sprintf(buf, "%u\n", cur_freq);

I will fix it a bit differently:
Sudeep Holla - Jan. 7, 2019, 9:40 a.m.
On Mon, Jan 07, 2019 at 11:32:41AM +0530, Viresh Kumar wrote:
> On 04-01-19, 11:42, Sudeep Holla wrote:
> > cpuinfo_cur_freq gets current CPU frequency as detected by hardware
> > while scaling_cur_freq last known CPU frequency. Some platforms may not
> > allow checking the CPU frequency of an offline CPU or the associated
> > resources may have been released via cpufreq_exit when the CPU gets
> > offlined. If we attempt to get current frequency from the hardware,
> > it may result in hang or crash.
> >
> > For example on Juno, I see:
> >
> > Unable to handle kernel NULL pointer dereference at virtual address 0000000000000188
> > [0000000000000188] pgd=0000000000000000
> > Internal error: Oops: 96000004 [#1] PREEMPT SMP
> > Modules linked in:
> > CPU: 5 PID: 4202 Comm: cat Not tainted 4.20.0-08251-ga0f2c0318a15-dirty #87
> > Hardware name: ARM LTD ARM Juno Development Platform/ARM Juno Development Platform
> > pstate: 40000005 (nZcv daif -PAN -UAO)
> > pc : scmi_cpufreq_get_rate+0x34/0xb0
> > lr : scmi_cpufreq_get_rate+0x34/0xb0
> > Call trace:
> >  scmi_cpufreq_get_rate+0x34/0xb0
> >  __cpufreq_get+0x34/0xc0
> >  show_cpuinfo_cur_freq+0x24/0x78
> >  show+0x40/0x60
> >  sysfs_kf_seq_show+0xc0/0x148
> >  kernfs_seq_show+0x44/0x50
> >  seq_read+0xd4/0x480
> >  kernfs_fop_read+0x15c/0x208
> >  __vfs_read+0x60/0x188
> >  vfs_read+0x94/0x150
> >  ksys_read+0x6c/0xd8
> >  __arm64_sys_read+0x24/0x30
> >  el0_svc_common+0x78/0x100
> >  el0_svc_handler+0x38/0x78
> >  el0_svc+0x8/0xc
> > ---[ end trace 3d1024e58f77f6b2 ]---
> >
> > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> > Cc: Viresh Kumar <viresh.kumar@linaro.org>
> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > ---
> >  drivers/cpufreq/cpufreq.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > Hi Viresh, Rafael,
> >
> > I don't know the reason why we call __cpufreq_get here instead of
> > cpufreq_get which checks if the policy is active or not.
>
> We stopped removing the policy sysfs files, when the last CPU of the policy
> exits, sometime back and probably missed this combination.
>

Ah OK, I did a quick search as I did remember something similar but
didn't get the answer for this.

> > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > index 6f23ebb395f1..cfc502385855 100644
> > --- a/drivers/cpufreq/cpufreq.c
> > +++ b/drivers/cpufreq/cpufreq.c
> > @@ -712,7 +712,7 @@ store_one(scaling_max_freq, max);
> >  static ssize_t show_cpuinfo_cur_freq(struct cpufreq_policy *policy,
> >  					char *buf)
> >  {
> > -	unsigned int cur_freq = __cpufreq_get(policy);
> > +	unsigned int cur_freq = cpufreq_get(policy->cpu);
> >
> >  	if (cur_freq)
> >  		return sprintf(buf, "%u\n", cur_freq);
>
> I will fix it a bit differently:
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 81de54cce637..8b1591804ec8 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -1541,17 +1541,16 @@ static unsigned int __cpufreq_get(struct cpufreq_policy *policy)
>  {
>  	unsigned int ret_freq = 0;
>
> -	if (!cpufreq_driver->get)
> +	if (unlikely(policy_is_inactive(policy)) || !cpufreq_driver->get)
>  		return ret_freq;
>
>  	ret_freq = cpufreq_driver->get(policy->cpu);
>
>  	/*
> -	 * Updating inactive policies is invalid, so avoid doing that.  Also
> -	 * if fast frequency switching is used with the given policy, the check
> +	 * If fast frequency switching is used with the given policy, the check
>  	 * against policy->cur is pointless, so skip it in that case too.
>  	 */
> -	if (unlikely(policy_is_inactive(policy)) || policy->fast_switch_enabled)
> +	if (policy->fast_switch_enabled)
>  		return ret_freq;
>
>  	if (ret_freq && policy->cur &&
> @@ -1580,10 +1579,7 @@ unsigned int cpufreq_get(unsigned int cpu)
>
>  	if (policy) {
>  		down_read(&policy->rwsem);
> -
> -		if (!policy_is_inactive(policy))
> -			ret_freq = __cpufreq_get(policy);
> -
> +		ret_freq = __cpufreq_get(policy);

This should work. Do you want to post this or want me to do ? Let me know.

--
Regards,
Sudeep
Viresh Kumar - Jan. 7, 2019, 10:59 a.m.
On 07-01-19, 09:40, Sudeep Holla wrote:
> This should work. Do you want to post this or want me to do ? Let me know.

You can send it.

Patch

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 81de54cce637..8b1591804ec8 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1541,17 +1541,16 @@  static unsigned int __cpufreq_get(struct cpufreq_policy *policy)
 {
 	unsigned int ret_freq = 0;
 
-	if (!cpufreq_driver->get)
+	if (unlikely(policy_is_inactive(policy)) || !cpufreq_driver->get)
 		return ret_freq;
 
 	ret_freq = cpufreq_driver->get(policy->cpu);
 
 	/*
-	 * Updating inactive policies is invalid, so avoid doing that.  Also
-	 * if fast frequency switching is used with the given policy, the check
+	 * If fast frequency switching is used with the given policy, the check
 	 * against policy->cur is pointless, so skip it in that case too.
 	 */
-	if (unlikely(policy_is_inactive(policy)) || policy->fast_switch_enabled)
+	if (policy->fast_switch_enabled)
 		return ret_freq;
 
 	if (ret_freq && policy->cur &&
@@ -1580,10 +1579,7 @@  unsigned int cpufreq_get(unsigned int cpu)
 
 	if (policy) {
 		down_read(&policy->rwsem);
-
-		if (!policy_is_inactive(policy))
-			ret_freq = __cpufreq_get(policy);
-
+		ret_freq = __cpufreq_get(policy);
 		up_read(&policy->rwsem);
 
 		cpufreq_cpu_put(policy);