| Message ID | 20120918201231.GF8474@google.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Tue, Sep 18, 2012 at 1:12 PM, Tejun Heo <tj@kernel.org> wrote: > So, with work_on_cpu() reimplementation just posted[1], we can do the > following instead. Functionally it's about the same but less ugly. > Ugly as it may be, I think the previous open coded version is better > suited as a fix and for -stable. Thoughts? I have to say, since the work_on_cpu() reimplementation seems to seriously simplify the code, and removes more lines than it adds, and makes this patch smaller than your original patch, I would personally prefer this approach instead anyway. It's what we want long-range, isn't it? And it's smaller and simpler. Sure, it might be a *conceptually* bigger change, but since it's both prettier and *practically* smaller, I do like it more. Even at this stage of -rc (and even for backporting to -stable). Can we get some quick testing of this two-patch series from the people who saw the original K8 cpufreq issue? Duncan? Linus -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday, September 18, 2012, Tejun Heo wrote: > So, with work_on_cpu() reimplementation just posted[1], we can do the > following instead. Functionally it's about the same but less ugly. > Ugly as it may be, I think the previous open coded version is better > suited as a fix and for -stable. Thoughts? I'd prefer this one to go into v3.7 along with the work_on_cpu() patch and then the open-coded version to go to -stable after the mainline one has got some testing coverage. Thanks, Rafael > [1] https://lkml.org/lkml/2012/9/18/430 > > drivers/cpufreq/powernow-k8.c | 63 ++++++++++++++++++++++-------------------- > 1 file changed, 34 insertions(+), 29 deletions(-) > > diff --git a/drivers/cpufreq/powernow-k8.c b/drivers/cpufreq/powernow-k8.c > index c0e8164..1a40935 100644 > --- a/drivers/cpufreq/powernow-k8.c > +++ b/drivers/cpufreq/powernow-k8.c > @@ -35,7 +35,6 @@ > #include <linux/slab.h> > #include <linux/string.h> > #include <linux/cpumask.h> > -#include <linux/sched.h> /* for current / set_cpus_allowed() */ > #include <linux/io.h> > #include <linux/delay.h> > > @@ -1139,16 +1138,23 @@ static int transition_frequency_pstate(struct powernow_k8_data *data, > return res; > } > > -/* Driver entry point to switch to the target frequency */ > -static int powernowk8_target(struct cpufreq_policy *pol, > - unsigned targfreq, unsigned relation) > +struct powernowk8_target_arg { > + struct cpufreq_policy *pol; > + unsigned targfreq; > + unsigned relation; > +}; > + > +static long powernowk8_target_fn(void *arg) > { > - cpumask_var_t oldmask; > + struct powernowk8_target_arg *pta = arg; > + struct cpufreq_policy *pol = pta->pol; > + unsigned targfreq = pta->targfreq; > + unsigned relation = pta->relation; > struct powernow_k8_data *data = per_cpu(powernow_data, pol->cpu); > u32 checkfid; > u32 checkvid; > unsigned int newstate; > - int ret = -EIO; > + int ret; > > if (!data) > return -EINVAL; > @@ -1156,29 +1162,16 @@ static int powernowk8_target(struct cpufreq_policy *pol, > checkfid = data->currfid; > checkvid = data->currvid; > > - /* only run on specific CPU from here on. */ > - /* This is poor form: use a workqueue or smp_call_function_single */ > - if (!alloc_cpumask_var(&oldmask, GFP_KERNEL)) > - return -ENOMEM; > - > - cpumask_copy(oldmask, tsk_cpus_allowed(current)); > - set_cpus_allowed_ptr(current, cpumask_of(pol->cpu)); > - > - if (smp_processor_id() != pol->cpu) { > - printk(KERN_ERR PFX "limiting to cpu %u failed\n", pol->cpu); > - goto err_out; > - } > - > if (pending_bit_stuck()) { > printk(KERN_ERR PFX "failing targ, change pending bit set\n"); > - goto err_out; > + return -EIO; > } > > pr_debug("targ: cpu %d, %d kHz, min %d, max %d, relation %d\n", > pol->cpu, targfreq, pol->min, pol->max, relation); > > if (query_current_values_with_pending_wait(data)) > - goto err_out; > + return -EIO; > > if (cpu_family != CPU_HW_PSTATE) { > pr_debug("targ: curr fid 0x%x, vid 0x%x\n", > @@ -1196,7 +1189,7 @@ static int powernowk8_target(struct cpufreq_policy *pol, > > if (cpufreq_frequency_table_target(pol, data->powernow_table, > targfreq, relation, &newstate)) > - goto err_out; > + return -EIO; > > mutex_lock(&fidvid_mutex); > > @@ -1209,9 +1202,8 @@ static int powernowk8_target(struct cpufreq_policy *pol, > ret = transition_frequency_fidvid(data, newstate); > if (ret) { > printk(KERN_ERR PFX "transition frequency failed\n"); > - ret = 1; > mutex_unlock(&fidvid_mutex); > - goto err_out; > + return 1; > } > mutex_unlock(&fidvid_mutex); > > @@ -1220,12 +1212,25 @@ static int powernowk8_target(struct cpufreq_policy *pol, > data->powernow_table[newstate].index); > else > pol->cur = find_khz_freq_from_fid(data->currfid); > - ret = 0; > > -err_out: > - set_cpus_allowed_ptr(current, oldmask); > - free_cpumask_var(oldmask); > - return ret; > + return 0; > +} > + > +/* Driver entry point to switch to the target frequency */ > +static int powernowk8_target(struct cpufreq_policy *pol, > + unsigned targfreq, unsigned relation) > +{ > + struct powernowk8_target_arg pta = { .pol = pol, .targfreq = targfreq, > + .relation = relation }; > + > + /* > + * Must run on @pol->cpu. cpufreq core is responsible for ensuring > + * that we're bound to the current CPU and pol->cpu stays online. > + */ > + if (smp_processor_id() == pol->cpu) > + return powernowk8_target_fn(&pta); > + else > + return work_on_cpu(pol->cpu, powernowk8_target_fn, &pta); > } > > /* Driver entry point to verify the policy and range of frequencies */ > > -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/cpufreq/powernow-k8.c b/drivers/cpufreq/powernow-k8.c index c0e8164..1a40935 100644 --- a/drivers/cpufreq/powernow-k8.c +++ b/drivers/cpufreq/powernow-k8.c @@ -35,7 +35,6 @@ #include <linux/slab.h> #include <linux/string.h> #include <linux/cpumask.h> -#include <linux/sched.h> /* for current / set_cpus_allowed() */ #include <linux/io.h> #include <linux/delay.h> @@ -1139,16 +1138,23 @@ static int transition_frequency_pstate(struct powernow_k8_data *data, return res; } -/* Driver entry point to switch to the target frequency */ -static int powernowk8_target(struct cpufreq_policy *pol, - unsigned targfreq, unsigned relation) +struct powernowk8_target_arg { + struct cpufreq_policy *pol; + unsigned targfreq; + unsigned relation; +}; + +static long powernowk8_target_fn(void *arg) { - cpumask_var_t oldmask; + struct powernowk8_target_arg *pta = arg; + struct cpufreq_policy *pol = pta->pol; + unsigned targfreq = pta->targfreq; + unsigned relation = pta->relation; struct powernow_k8_data *data = per_cpu(powernow_data, pol->cpu); u32 checkfid; u32 checkvid; unsigned int newstate; - int ret = -EIO; + int ret; if (!data) return -EINVAL; @@ -1156,29 +1162,16 @@ static int powernowk8_target(struct cpufreq_policy *pol, checkfid = data->currfid; checkvid = data->currvid; - /* only run on specific CPU from here on. */ - /* This is poor form: use a workqueue or smp_call_function_single */ - if (!alloc_cpumask_var(&oldmask, GFP_KERNEL)) - return -ENOMEM; - - cpumask_copy(oldmask, tsk_cpus_allowed(current)); - set_cpus_allowed_ptr(current, cpumask_of(pol->cpu)); - - if (smp_processor_id() != pol->cpu) { - printk(KERN_ERR PFX "limiting to cpu %u failed\n", pol->cpu); - goto err_out; - } - if (pending_bit_stuck()) { printk(KERN_ERR PFX "failing targ, change pending bit set\n"); - goto err_out; + return -EIO; } pr_debug("targ: cpu %d, %d kHz, min %d, max %d, relation %d\n", pol->cpu, targfreq, pol->min, pol->max, relation); if (query_current_values_with_pending_wait(data)) - goto err_out; + return -EIO; if (cpu_family != CPU_HW_PSTATE) { pr_debug("targ: curr fid 0x%x, vid 0x%x\n", @@ -1196,7 +1189,7 @@ static int powernowk8_target(struct cpufreq_policy *pol, if (cpufreq_frequency_table_target(pol, data->powernow_table, targfreq, relation, &newstate)) - goto err_out; + return -EIO; mutex_lock(&fidvid_mutex); @@ -1209,9 +1202,8 @@ static int powernowk8_target(struct cpufreq_policy *pol, ret = transition_frequency_fidvid(data, newstate); if (ret) { printk(KERN_ERR PFX "transition frequency failed\n"); - ret = 1; mutex_unlock(&fidvid_mutex); - goto err_out; + return 1; } mutex_unlock(&fidvid_mutex); @@ -1220,12 +1212,25 @@ static int powernowk8_target(struct cpufreq_policy *pol, data->powernow_table[newstate].index); else pol->cur = find_khz_freq_from_fid(data->currfid); - ret = 0; -err_out: - set_cpus_allowed_ptr(current, oldmask); - free_cpumask_var(oldmask); - return ret; + return 0; +} + +/* Driver entry point to switch to the target frequency */ +static int powernowk8_target(struct cpufreq_policy *pol, + unsigned targfreq, unsigned relation) +{ + struct powernowk8_target_arg pta = { .pol = pol, .targfreq = targfreq, + .relation = relation }; + + /* + * Must run on @pol->cpu. cpufreq core is responsible for ensuring + * that we're bound to the current CPU and pol->cpu stays online. + */ + if (smp_processor_id() == pol->cpu) + return powernowk8_target_fn(&pta); + else + return work_on_cpu(pol->cpu, powernowk8_target_fn, &pta); } /* Driver entry point to verify the policy and range of frequencies */