| Message ID | 043703bd1914d52340653c3cc31207e505df6139.1436348436.git.viresh.kumar@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Wed, 2015-07-08 at 15:12 +0530, Viresh Kumar wrote: > cpufreq_init_policy() can fail, and we don't do anything except a call > to ->exit() on that. The policy should be freed if this happens. > > Lets do it properly. > > Reported-by: "Jon Medhurst (Tixy)" <tixy@linaro.org> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- I tried these patches without the earlier "cpufreq: Initialize the governor again while restoring policy" patch. The result is that the error when bringing a cpu online is with flagged up with a kernel message: cpufreq: cpufreq_add_dev: Failed to initialize policy for cpu: 1 (-16) and afterwards, the sysfs entries that I was poking and causing the crash aren't present. So looks like this patch has done what we want, and cleaned things up after an error. So... Tested-by: Jon Medhurst <tixy@linaro.org> Thanks for the prompt fix. > drivers/cpufreq/cpufreq.c | 20 +++++++++++--------- > 1 file changed, 11 insertions(+), 9 deletions(-) > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > index b7aac8eec525..006299214d2e 100644 > --- a/drivers/cpufreq/cpufreq.c > +++ b/drivers/cpufreq/cpufreq.c > @@ -1051,11 +1051,10 @@ static int cpufreq_add_dev_interface(struct cpufreq_policy *policy, > return cpufreq_add_dev_symlink(policy); > } > > -static void cpufreq_init_policy(struct cpufreq_policy *policy) > +static int cpufreq_init_policy(struct cpufreq_policy *policy) > { > struct cpufreq_governor *gov = NULL; > struct cpufreq_policy new_policy; > - int ret = 0; > > memcpy(&new_policy, policy, sizeof(*policy)); > > @@ -1074,12 +1073,7 @@ static void cpufreq_init_policy(struct cpufreq_policy *policy) > cpufreq_parse_governor(gov->name, &new_policy.policy, NULL); > > /* set default policy */ > - ret = cpufreq_set_policy(policy, &new_policy); > - if (ret) { > - pr_debug("setting policy failed\n"); > - if (cpufreq_driver->exit) > - cpufreq_driver->exit(policy); > - } > + return cpufreq_set_policy(policy, &new_policy); > } > > static int cpufreq_add_policy_cpu(struct cpufreq_policy *policy, > @@ -1376,7 +1370,12 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif) > write_unlock_irqrestore(&cpufreq_driver_lock, flags); > } > > - cpufreq_init_policy(policy); > + ret = cpufreq_init_policy(policy); > + if (ret) { > + pr_err("%s: Failed to initialize policy for cpu: %d (%d)\n", > + __func__, cpu, ret); > + goto out_remove_policy_notify; > + } > > if (!recover_policy) { > policy->user_policy.policy = policy->policy; > @@ -1396,6 +1395,9 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif) > > return 0; > > +out_remove_policy_notify: > + /* cpufreq_policy_free() will notify based on this */ > + recover_policy = true; > out_exit_policy: > up_write(&policy->rwsem); > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On 08-07-15, 12:17, Jon Medhurst (Tixy) wrote: > I tried these patches without the earlier "cpufreq: Initialize the > governor again while restoring policy" patch. > > The result is that the error when bringing a cpu online is with flagged > up with a kernel message: > > cpufreq: cpufreq_add_dev: Failed to initialize policy for cpu: 1 (-16) > > and afterwards, the sysfs entries that I was poking and causing the > crash aren't present. So looks like this patch has done what we want, > and cleaned things up after an error. So... > > Tested-by: Jon Medhurst <tixy@linaro.org> > > Thanks for the prompt fix. And thanks for your help in getting these tested :)
On Thu, 2015-07-16 at 02:32 +0200, Rafael J. Wysocki wrote: > On Wednesday, July 08, 2015 04:50:23 PM Viresh Kumar wrote: > > On 08-07-15, 12:17, Jon Medhurst (Tixy) wrote: > > > I tried these patches without the earlier "cpufreq: Initialize the > > > governor again while restoring policy" patch. > > > > > > The result is that the error when bringing a cpu online is with flagged > > > up with a kernel message: > > > > > > cpufreq: cpufreq_add_dev: Failed to initialize policy for cpu: 1 (-16) > > > > > > and afterwards, the sysfs entries that I was poking and causing the > > > crash aren't present. So looks like this patch has done what we want, > > > and cleaned things up after an error. So... > > > > > > Tested-by: Jon Medhurst <tixy@linaro.org> > > > > > > Thanks for the prompt fix. > > > > And thanks for your help in getting these tested :) > > Both queued up for 4.3, thanks! The crash I was getting was a regression caused by changes that went into 4.2-rc1. Indeed, the first patch from Viresh is marked: Fixes: 18bf3a124ef8 ("cpufreq: Mark policy->governor = NULL for inactive policies") For 4.2-rc And I am having to carry that first patch to keep two ARM big.LITTLE platforms working.
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index b7aac8eec525..006299214d2e 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -1051,11 +1051,10 @@ static int cpufreq_add_dev_interface(struct cpufreq_policy *policy, return cpufreq_add_dev_symlink(policy); } -static void cpufreq_init_policy(struct cpufreq_policy *policy) +static int cpufreq_init_policy(struct cpufreq_policy *policy) { struct cpufreq_governor *gov = NULL; struct cpufreq_policy new_policy; - int ret = 0; memcpy(&new_policy, policy, sizeof(*policy)); @@ -1074,12 +1073,7 @@ static void cpufreq_init_policy(struct cpufreq_policy *policy) cpufreq_parse_governor(gov->name, &new_policy.policy, NULL); /* set default policy */ - ret = cpufreq_set_policy(policy, &new_policy); - if (ret) { - pr_debug("setting policy failed\n"); - if (cpufreq_driver->exit) - cpufreq_driver->exit(policy); - } + return cpufreq_set_policy(policy, &new_policy); } static int cpufreq_add_policy_cpu(struct cpufreq_policy *policy, @@ -1376,7 +1370,12 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif) write_unlock_irqrestore(&cpufreq_driver_lock, flags); } - cpufreq_init_policy(policy); + ret = cpufreq_init_policy(policy); + if (ret) { + pr_err("%s: Failed to initialize policy for cpu: %d (%d)\n", + __func__, cpu, ret); + goto out_remove_policy_notify; + } if (!recover_policy) { policy->user_policy.policy = policy->policy; @@ -1396,6 +1395,9 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif) return 0; +out_remove_policy_notify: + /* cpufreq_policy_free() will notify based on this */ + recover_policy = true; out_exit_policy: up_write(&policy->rwsem);
cpufreq_init_policy() can fail, and we don't do anything except a call to ->exit() on that. The policy should be freed if this happens. Lets do it properly. Reported-by: "Jon Medhurst (Tixy)" <tixy@linaro.org> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- drivers/cpufreq/cpufreq.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-)