| Message ID | CAKohpo=OOPBYCD_K6RLw2YrJxhMokwrOww4QM2t2T4SnGObyQQ@mail.gmail.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
On Mon, Apr 15, 2013 at 7:51 PM, Sedat Dilek <sedat.dilek@gmail.com> wrote: > On Mon, Apr 15, 2013 at 7:22 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote: >> On 15 April 2013 21:37, Dirk Brandewie <dirk.brandewie@gmail.com> wrote: >>> If the intel_pstate driver is being used __cpufreq_governor() should NOT be >>> called intel_pstate does not implement the target() callback. >>> >>> Nathan's commit 5800043b2 changed the fence around the call to >>> __cpufreq_governor() in __cpufreq_remove_dev() here is the relevant hunk. >> >> No it isn't. >> >>> + if (has_target) >>> __cpufreq_governor(data, CPUFREQ_GOV_STOP); >> >> As it has taken care of this limitation. >> >> BUT some of my earlier patches haven't. :( >> Here is the fix (Sedat please try this and give your tested-by, use the attached >> patch as gmail might break what i am copying in mail).. >> >> Sorry for being late in fixing this issue, i am still down with Tonsil infection >> and fever.. Today only i got some power to fix it after seeing Dirk's mail. >> >> Your tested-by may help me to recover quickly :) >> > > Hehe. > Me myself and I was today chez-mon-docteur... Let's see the results on Thursday. > Again, get well soon. > > Tested against... > > "BROKEN" Linux-Next (next-20130411) with attached patchset (incl. > your cpufreq-next-fixes). > > Test-Case... > > CONFIG_X86_INTEL_PSTATE=y > > root# echo 0 > /sys/devices/system/cpu/cpu3/online > > Tested-by: Sedat Dilek <sedat.dilek@gmail.com> > > ...did not test on-reboot-case. > > ( Dirk promised to test as well... ) > Might be interesting as an extra-confirmation: root# echo 1 > /sys/devices/system/cpu/cpu3/online [ dmesg ] [ 556.101961] smpboot: Booting Node 0 Processor 3 APIC 0x3 [ 556.113158] Disabled fast string operations [ 556.116621] Intel pstate controlling: cpu 3 - Sedat - > - Sedat - > >> @Rafael: I will probably be down for one more week and so not doing any >> reviews for now... I do check important mails sent directly to me though. >> >> ------------x----------------------x------------------ >> >> From: Viresh Kumar <viresh.kumar@linaro.org> >> Date: Mon, 15 Apr 2013 22:43:57 +0530 >> Subject: [PATCH] cpufreq: Don't call __cpufreq_governor() for drivers without >> target() >> >> Some cpufreq drivers implement their own governor and so don't need us to call >> generic governors interface via __cpufreq_governor(). Few recent commits haven't >> obeyed this law well and we saw some regressions. >> >> This patch tries to fix this issue. >> >> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> >> --- >> drivers/cpufreq/cpufreq.c | 18 +++++++++++++----- >> 1 file changed, 13 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c >> index 3564947..a6f6595 100644 >> --- a/drivers/cpufreq/cpufreq.c >> +++ b/drivers/cpufreq/cpufreq.c >> @@ -858,13 +858,18 @@ static int cpufreq_add_policy_cpu(unsigned int >> cpu, unsigned int sibling, >> struct device *dev) >> { >> struct cpufreq_policy *policy; >> - int ret = 0; >> + int ret = 0, has_target = 0; >> unsigned long flags; >> >> policy = cpufreq_cpu_get(sibling); >> WARN_ON(!policy); >> >> - __cpufreq_governor(policy, CPUFREQ_GOV_STOP); >> + rcu_read_lock(); >> + has_target = !!rcu_dereference(cpufreq_driver)->target; >> + rcu_read_unlock(); >> + >> + if (has_target) >> + __cpufreq_governor(policy, CPUFREQ_GOV_STOP); >> >> lock_policy_rwsem_write(sibling); >> >> @@ -877,8 +882,10 @@ static int cpufreq_add_policy_cpu(unsigned int >> cpu, unsigned int sibling, >> >> unlock_policy_rwsem_write(sibling); >> >> - __cpufreq_governor(policy, CPUFREQ_GOV_START); >> - __cpufreq_governor(policy, CPUFREQ_GOV_LIMITS); >> + if (has_target) { >> + __cpufreq_governor(policy, CPUFREQ_GOV_START); >> + __cpufreq_governor(policy, CPUFREQ_GOV_LIMITS); >> + } >> >> ret = sysfs_create_link(&dev->kobj, &policy->kobj, "cpufreq"); >> if (ret) { >> @@ -1146,7 +1153,8 @@ static int __cpufreq_remove_dev(struct device >> *dev, struct subsys_interface *sif >> >> /* If cpu is last user of policy, free policy */ >> if (cpus == 1) { >> - __cpufreq_governor(data, CPUFREQ_GOV_POLICY_EXIT); >> + if (has_target) >> + __cpufreq_governor(data, CPUFREQ_GOV_POLICY_EXIT); >> >> lock_policy_rwsem_read(cpu); >> kobj = &data->kobj; -- 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 04/15/2013 10:51 AM, Sedat Dilek wrote: > On Mon, Apr 15, 2013 at 7:22 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote: >> On 15 April 2013 21:37, Dirk Brandewie <dirk.brandewie@gmail.com> wrote: >>> If the intel_pstate driver is being used __cpufreq_governor() should NOT be >>> called intel_pstate does not implement the target() callback. >>> >>> Nathan's commit 5800043b2 changed the fence around the call to >>> __cpufreq_governor() in __cpufreq_remove_dev() here is the relevant hunk. >> >> No it isn't. >> >>> + if (has_target) >>> __cpufreq_governor(data, CPUFREQ_GOV_STOP); >> >> As it has taken care of this limitation. >> >> BUT some of my earlier patches haven't. :( >> Here is the fix (Sedat please try this and give your tested-by, use the attached >> patch as gmail might break what i am copying in mail).. >> >> Sorry for being late in fixing this issue, i am still down with Tonsil infection >> and fever.. Today only i got some power to fix it after seeing Dirk's mail. >> >> Your tested-by may help me to recover quickly :) >> > > Hehe. > Me myself and I was today chez-mon-docteur... Let's see the results on Thursday. > Again, get well soon. > > Tested against... > > "BROKEN" Linux-Next (next-20130411) with attached patchset (incl. > your cpufreq-next-fixes). > > Test-Case... > > CONFIG_X86_INTEL_PSTATE=y > > root# echo 0 > /sys/devices/system/cpu/cpu3/online > > Tested-by: Sedat Dilek <sedat.dilek@gmail.com> > > ...did not test on-reboot-case. > > ( Dirk promised to test as well... ) > Tested with: while true do echo 0 > online echo 1 > online done For several minutes and rebooting several times seems to have fixed the issue. Nathan, Sorry for calling out your patch erroneously I should have paid closer attention. Viresh you can add my Tested-by: Thanks --Dirk > - Sedat - > >> @Rafael: I will probably be down for one more week and so not doing any >> reviews for now... I do check important mails sent directly to me though. >> >> ------------x----------------------x------------------ >> >> From: Viresh Kumar <viresh.kumar@linaro.org> >> Date: Mon, 15 Apr 2013 22:43:57 +0530 >> Subject: [PATCH] cpufreq: Don't call __cpufreq_governor() for drivers without >> target() >> >> Some cpufreq drivers implement their own governor and so don't need us to call >> generic governors interface via __cpufreq_governor(). Few recent commits haven't >> obeyed this law well and we saw some regressions. >> >> This patch tries to fix this issue. >> >> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> >> --- >> drivers/cpufreq/cpufreq.c | 18 +++++++++++++----- >> 1 file changed, 13 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c >> index 3564947..a6f6595 100644 >> --- a/drivers/cpufreq/cpufreq.c >> +++ b/drivers/cpufreq/cpufreq.c >> @@ -858,13 +858,18 @@ static int cpufreq_add_policy_cpu(unsigned int >> cpu, unsigned int sibling, >> struct device *dev) >> { >> struct cpufreq_policy *policy; >> - int ret = 0; >> + int ret = 0, has_target = 0; >> unsigned long flags; >> >> policy = cpufreq_cpu_get(sibling); >> WARN_ON(!policy); >> >> - __cpufreq_governor(policy, CPUFREQ_GOV_STOP); >> + rcu_read_lock(); >> + has_target = !!rcu_dereference(cpufreq_driver)->target; >> + rcu_read_unlock(); >> + >> + if (has_target) >> + __cpufreq_governor(policy, CPUFREQ_GOV_STOP); >> >> lock_policy_rwsem_write(sibling); >> >> @@ -877,8 +882,10 @@ static int cpufreq_add_policy_cpu(unsigned int >> cpu, unsigned int sibling, >> >> unlock_policy_rwsem_write(sibling); >> >> - __cpufreq_governor(policy, CPUFREQ_GOV_START); >> - __cpufreq_governor(policy, CPUFREQ_GOV_LIMITS); >> + if (has_target) { >> + __cpufreq_governor(policy, CPUFREQ_GOV_START); >> + __cpufreq_governor(policy, CPUFREQ_GOV_LIMITS); >> + } >> >> ret = sysfs_create_link(&dev->kobj, &policy->kobj, "cpufreq"); >> if (ret) { >> @@ -1146,7 +1153,8 @@ static int __cpufreq_remove_dev(struct device >> *dev, struct subsys_interface *sif >> >> /* If cpu is last user of policy, free policy */ >> if (cpus == 1) { >> - __cpufreq_governor(data, CPUFREQ_GOV_POLICY_EXIT); >> + if (has_target) >> + __cpufreq_governor(data, CPUFREQ_GOV_POLICY_EXIT); >> >> lock_policy_rwsem_read(cpu); >> kobj = &data->kobj; -- 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 Mon, Apr 15, 2013 at 7:57 PM, Sedat Dilek <sedat.dilek@gmail.com> wrote: > On Mon, Apr 15, 2013 at 7:51 PM, Sedat Dilek <sedat.dilek@gmail.com> wrote: >> On Mon, Apr 15, 2013 at 7:22 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote: >>> On 15 April 2013 21:37, Dirk Brandewie <dirk.brandewie@gmail.com> wrote: >>>> If the intel_pstate driver is being used __cpufreq_governor() should NOT be >>>> called intel_pstate does not implement the target() callback. >>>> >>>> Nathan's commit 5800043b2 changed the fence around the call to >>>> __cpufreq_governor() in __cpufreq_remove_dev() here is the relevant hunk. >>> >>> No it isn't. >>> >>>> + if (has_target) >>>> __cpufreq_governor(data, CPUFREQ_GOV_STOP); >>> >>> As it has taken care of this limitation. >>> >>> BUT some of my earlier patches haven't. :( >>> Here is the fix (Sedat please try this and give your tested-by, use the attached >>> patch as gmail might break what i am copying in mail).. >>> >>> Sorry for being late in fixing this issue, i am still down with Tonsil infection >>> and fever.. Today only i got some power to fix it after seeing Dirk's mail. >>> >>> Your tested-by may help me to recover quickly :) >>> >> >> Hehe. >> Me myself and I was today chez-mon-docteur... Let's see the results on Thursday. >> Again, get well soon. >> >> Tested against... >> >> "BROKEN" Linux-Next (next-20130411) with attached patchset (incl. >> your cpufreq-next-fixes). >> >> Test-Case... >> >> CONFIG_X86_INTEL_PSTATE=y >> >> root# echo 0 > /sys/devices/system/cpu/cpu3/online >> >> Tested-by: Sedat Dilek <sedat.dilek@gmail.com> >> >> ...did not test on-reboot-case. >> Reboot is also fine here. >> ( Dirk promised to test as well... ) >> Dirk confirmed your patch works for him. Good! - Sedat - > > Might be interesting as an extra-confirmation: > > root# echo 1 > /sys/devices/system/cpu/cpu3/online > > [ dmesg ] > > [ 556.101961] smpboot: Booting Node 0 Processor 3 APIC 0x3 > [ 556.113158] Disabled fast string operations > [ 556.116621] Intel pstate controlling: cpu 3 > > - Sedat - > >> - Sedat - >> >>> @Rafael: I will probably be down for one more week and so not doing any >>> reviews for now... I do check important mails sent directly to me though. >>> >>> ------------x----------------------x------------------ >>> >>> From: Viresh Kumar <viresh.kumar@linaro.org> >>> Date: Mon, 15 Apr 2013 22:43:57 +0530 >>> Subject: [PATCH] cpufreq: Don't call __cpufreq_governor() for drivers without >>> target() >>> >>> Some cpufreq drivers implement their own governor and so don't need us to call >>> generic governors interface via __cpufreq_governor(). Few recent commits haven't >>> obeyed this law well and we saw some regressions. >>> >>> This patch tries to fix this issue. >>> >>> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> >>> --- >>> drivers/cpufreq/cpufreq.c | 18 +++++++++++++----- >>> 1 file changed, 13 insertions(+), 5 deletions(-) >>> >>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c >>> index 3564947..a6f6595 100644 >>> --- a/drivers/cpufreq/cpufreq.c >>> +++ b/drivers/cpufreq/cpufreq.c >>> @@ -858,13 +858,18 @@ static int cpufreq_add_policy_cpu(unsigned int >>> cpu, unsigned int sibling, >>> struct device *dev) >>> { >>> struct cpufreq_policy *policy; >>> - int ret = 0; >>> + int ret = 0, has_target = 0; >>> unsigned long flags; >>> >>> policy = cpufreq_cpu_get(sibling); >>> WARN_ON(!policy); >>> >>> - __cpufreq_governor(policy, CPUFREQ_GOV_STOP); >>> + rcu_read_lock(); >>> + has_target = !!rcu_dereference(cpufreq_driver)->target; >>> + rcu_read_unlock(); >>> + >>> + if (has_target) >>> + __cpufreq_governor(policy, CPUFREQ_GOV_STOP); >>> >>> lock_policy_rwsem_write(sibling); >>> >>> @@ -877,8 +882,10 @@ static int cpufreq_add_policy_cpu(unsigned int >>> cpu, unsigned int sibling, >>> >>> unlock_policy_rwsem_write(sibling); >>> >>> - __cpufreq_governor(policy, CPUFREQ_GOV_START); >>> - __cpufreq_governor(policy, CPUFREQ_GOV_LIMITS); >>> + if (has_target) { >>> + __cpufreq_governor(policy, CPUFREQ_GOV_START); >>> + __cpufreq_governor(policy, CPUFREQ_GOV_LIMITS); >>> + } >>> >>> ret = sysfs_create_link(&dev->kobj, &policy->kobj, "cpufreq"); >>> if (ret) { >>> @@ -1146,7 +1153,8 @@ static int __cpufreq_remove_dev(struct device >>> *dev, struct subsys_interface *sif >>> >>> /* If cpu is last user of policy, free policy */ >>> if (cpus == 1) { >>> - __cpufreq_governor(data, CPUFREQ_GOV_POLICY_EXIT); >>> + if (has_target) >>> + __cpufreq_governor(data, CPUFREQ_GOV_POLICY_EXIT); >>> >>> lock_policy_rwsem_read(cpu); >>> kobj = &data->kobj; -- 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 Mon, Apr 15, 2013 at 7:22 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote: > On 15 April 2013 21:37, Dirk Brandewie <dirk.brandewie@gmail.com> wrote: >> If the intel_pstate driver is being used __cpufreq_governor() should NOT be >> called intel_pstate does not implement the target() callback. >> >> Nathan's commit 5800043b2 changed the fence around the call to >> __cpufreq_governor() in __cpufreq_remove_dev() here is the relevant hunk. > > No it isn't. > >> + if (has_target) >> __cpufreq_governor(data, CPUFREQ_GOV_STOP); > > As it has taken care of this limitation. > > BUT some of my earlier patches haven't. :( > Here is the fix (Sedat please try this and give your tested-by, use the attached > patch as gmail might break what i am copying in mail).. > > Sorry for being late in fixing this issue, i am still down with Tonsil infection > and fever.. Today only i got some power to fix it after seeing Dirk's mail. > > Your tested-by may help me to recover quickly :) > > @Rafael: I will probably be down for one more week and so not doing any > reviews for now... I do check important mails sent directly to me though. > Hi Viresh, can you sent a separate patch on this (with Reported/Tested-by#s)? AFAICS this is not in pm.git#linux-next? Regards, - Sedat - > ------------x----------------------x------------------ > > From: Viresh Kumar <viresh.kumar@linaro.org> > Date: Mon, 15 Apr 2013 22:43:57 +0530 > Subject: [PATCH] cpufreq: Don't call __cpufreq_governor() for drivers without > target() > > Some cpufreq drivers implement their own governor and so don't need us to call > generic governors interface via __cpufreq_governor(). Few recent commits haven't > obeyed this law well and we saw some regressions. > > This patch tries to fix this issue. > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > drivers/cpufreq/cpufreq.c | 18 +++++++++++++----- > 1 file changed, 13 insertions(+), 5 deletions(-) > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > index 3564947..a6f6595 100644 > --- a/drivers/cpufreq/cpufreq.c > +++ b/drivers/cpufreq/cpufreq.c > @@ -858,13 +858,18 @@ static int cpufreq_add_policy_cpu(unsigned int > cpu, unsigned int sibling, > struct device *dev) > { > struct cpufreq_policy *policy; > - int ret = 0; > + int ret = 0, has_target = 0; > unsigned long flags; > > policy = cpufreq_cpu_get(sibling); > WARN_ON(!policy); > > - __cpufreq_governor(policy, CPUFREQ_GOV_STOP); > + rcu_read_lock(); > + has_target = !!rcu_dereference(cpufreq_driver)->target; > + rcu_read_unlock(); > + > + if (has_target) > + __cpufreq_governor(policy, CPUFREQ_GOV_STOP); > > lock_policy_rwsem_write(sibling); > > @@ -877,8 +882,10 @@ static int cpufreq_add_policy_cpu(unsigned int > cpu, unsigned int sibling, > > unlock_policy_rwsem_write(sibling); > > - __cpufreq_governor(policy, CPUFREQ_GOV_START); > - __cpufreq_governor(policy, CPUFREQ_GOV_LIMITS); > + if (has_target) { > + __cpufreq_governor(policy, CPUFREQ_GOV_START); > + __cpufreq_governor(policy, CPUFREQ_GOV_LIMITS); > + } > > ret = sysfs_create_link(&dev->kobj, &policy->kobj, "cpufreq"); > if (ret) { > @@ -1146,7 +1153,8 @@ static int __cpufreq_remove_dev(struct device > *dev, struct subsys_interface *sif > > /* If cpu is last user of policy, free policy */ > if (cpus == 1) { > - __cpufreq_governor(data, CPUFREQ_GOV_POLICY_EXIT); > + if (has_target) > + __cpufreq_governor(data, CPUFREQ_GOV_POLICY_EXIT); > > lock_policy_rwsem_read(cpu); > kobj = &data->kobj; -- 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 Wednesday, April 17, 2013 04:04:46 PM Sedat Dilek wrote: > On Mon, Apr 15, 2013 at 7:22 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 15 April 2013 21:37, Dirk Brandewie <dirk.brandewie@gmail.com> wrote: > >> If the intel_pstate driver is being used __cpufreq_governor() should NOT be > >> called intel_pstate does not implement the target() callback. > >> > >> Nathan's commit 5800043b2 changed the fence around the call to > >> __cpufreq_governor() in __cpufreq_remove_dev() here is the relevant hunk. > > > > No it isn't. > > > >> + if (has_target) > >> __cpufreq_governor(data, CPUFREQ_GOV_STOP); > > > > As it has taken care of this limitation. > > > > BUT some of my earlier patches haven't. :( > > Here is the fix (Sedat please try this and give your tested-by, use the attached > > patch as gmail might break what i am copying in mail).. > > > > Sorry for being late in fixing this issue, i am still down with Tonsil infection > > and fever.. Today only i got some power to fix it after seeing Dirk's mail. > > > > Your tested-by may help me to recover quickly :) > > > > @Rafael: I will probably be down for one more week and so not doing any > > reviews for now... I do check important mails sent directly to me though. > > > > Hi Viresh, > > can you sent a separate patch on this (with Reported/Tested-by#s)? > AFAICS this is not in pm.git#linux-next? That's because I'm traveling and not pushing things to the tree. I'll start doing that again on Saturday. Till then, please apply the Viresh's patch on top of linux-next. Thanks, Rafael > > ------------x----------------------x------------------ > > > > From: Viresh Kumar <viresh.kumar@linaro.org> > > Date: Mon, 15 Apr 2013 22:43:57 +0530 > > Subject: [PATCH] cpufreq: Don't call __cpufreq_governor() for drivers without > > target() > > > > Some cpufreq drivers implement their own governor and so don't need us to call > > generic governors interface via __cpufreq_governor(). Few recent commits haven't > > obeyed this law well and we saw some regressions. > > > > This patch tries to fix this issue. > > > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > > --- > > drivers/cpufreq/cpufreq.c | 18 +++++++++++++----- > > 1 file changed, 13 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > > index 3564947..a6f6595 100644 > > --- a/drivers/cpufreq/cpufreq.c > > +++ b/drivers/cpufreq/cpufreq.c > > @@ -858,13 +858,18 @@ static int cpufreq_add_policy_cpu(unsigned int > > cpu, unsigned int sibling, > > struct device *dev) > > { > > struct cpufreq_policy *policy; > > - int ret = 0; > > + int ret = 0, has_target = 0; > > unsigned long flags; > > > > policy = cpufreq_cpu_get(sibling); > > WARN_ON(!policy); > > > > - __cpufreq_governor(policy, CPUFREQ_GOV_STOP); > > + rcu_read_lock(); > > + has_target = !!rcu_dereference(cpufreq_driver)->target; > > + rcu_read_unlock(); > > + > > + if (has_target) > > + __cpufreq_governor(policy, CPUFREQ_GOV_STOP); > > > > lock_policy_rwsem_write(sibling); > > > > @@ -877,8 +882,10 @@ static int cpufreq_add_policy_cpu(unsigned int > > cpu, unsigned int sibling, > > > > unlock_policy_rwsem_write(sibling); > > > > - __cpufreq_governor(policy, CPUFREQ_GOV_START); > > - __cpufreq_governor(policy, CPUFREQ_GOV_LIMITS); > > + if (has_target) { > > + __cpufreq_governor(policy, CPUFREQ_GOV_START); > > + __cpufreq_governor(policy, CPUFREQ_GOV_LIMITS); > > + } > > > > ret = sysfs_create_link(&dev->kobj, &policy->kobj, "cpufreq"); > > if (ret) { > > @@ -1146,7 +1153,8 @@ static int __cpufreq_remove_dev(struct device > > *dev, struct subsys_interface *sif > > > > /* If cpu is last user of policy, free policy */ > > if (cpus == 1) { > > - __cpufreq_governor(data, CPUFREQ_GOV_POLICY_EXIT); > > + if (has_target) > > + __cpufreq_governor(data, CPUFREQ_GOV_POLICY_EXIT); > > > > lock_policy_rwsem_read(cpu); > > kobj = &data->kobj;
On Monday, April 15, 2013 10:52:28 PM Viresh Kumar wrote: > On 15 April 2013 21:37, Dirk Brandewie <dirk.brandewie@gmail.com> wrote: > > If the intel_pstate driver is being used __cpufreq_governor() should NOT be > > called intel_pstate does not implement the target() callback. > > > > Nathan's commit 5800043b2 changed the fence around the call to > > __cpufreq_governor() in __cpufreq_remove_dev() here is the relevant hunk. > > No it isn't. > > > + if (has_target) > > __cpufreq_governor(data, CPUFREQ_GOV_STOP); > > As it has taken care of this limitation. > > BUT some of my earlier patches haven't. :( > Here is the fix (Sedat please try this and give your tested-by, use the attached > patch as gmail might break what i am copying in mail).. > > Sorry for being late in fixing this issue, i am still down with Tonsil infection > and fever.. Today only i got some power to fix it after seeing Dirk's mail. > > Your tested-by may help me to recover quickly :) > > @Rafael: I will probably be down for one more week and so not doing any > reviews for now... I do check important mails sent directly to me though. > > ------------x----------------------x------------------ > > From: Viresh Kumar <viresh.kumar@linaro.org> > Date: Mon, 15 Apr 2013 22:43:57 +0530 > Subject: [PATCH] cpufreq: Don't call __cpufreq_governor() for drivers without > target() > > Some cpufreq drivers implement their own governor and so don't need us to call > generic governors interface via __cpufreq_governor(). Few recent commits haven't > obeyed this law well and we saw some regressions. > > This patch tries to fix this issue. > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> Applied to linux-pm.git/linux-next, although please check the result, because the patchwork version of the patch wasn't quite applicable and I fixed it up manually. Thanks, Rafael > --- > drivers/cpufreq/cpufreq.c | 18 +++++++++++++----- > 1 file changed, 13 insertions(+), 5 deletions(-) > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > index 3564947..a6f6595 100644 > --- a/drivers/cpufreq/cpufreq.c > +++ b/drivers/cpufreq/cpufreq.c > @@ -858,13 +858,18 @@ static int cpufreq_add_policy_cpu(unsigned int > cpu, unsigned int sibling, > struct device *dev) > { > struct cpufreq_policy *policy; > - int ret = 0; > + int ret = 0, has_target = 0; > unsigned long flags; > > policy = cpufreq_cpu_get(sibling); > WARN_ON(!policy); > > - __cpufreq_governor(policy, CPUFREQ_GOV_STOP); > + rcu_read_lock(); > + has_target = !!rcu_dereference(cpufreq_driver)->target; > + rcu_read_unlock(); > + > + if (has_target) > + __cpufreq_governor(policy, CPUFREQ_GOV_STOP); > > lock_policy_rwsem_write(sibling); > > @@ -877,8 +882,10 @@ static int cpufreq_add_policy_cpu(unsigned int > cpu, unsigned int sibling, > > unlock_policy_rwsem_write(sibling); > > - __cpufreq_governor(policy, CPUFREQ_GOV_START); > - __cpufreq_governor(policy, CPUFREQ_GOV_LIMITS); > + if (has_target) { > + __cpufreq_governor(policy, CPUFREQ_GOV_START); > + __cpufreq_governor(policy, CPUFREQ_GOV_LIMITS); > + } > > ret = sysfs_create_link(&dev->kobj, &policy->kobj, "cpufreq"); > if (ret) { > @@ -1146,7 +1153,8 @@ static int __cpufreq_remove_dev(struct device > *dev, struct subsys_interface *sif > > /* If cpu is last user of policy, free policy */ > if (cpus == 1) { > - __cpufreq_governor(data, CPUFREQ_GOV_POLICY_EXIT); > + if (has_target) > + __cpufreq_governor(data, CPUFREQ_GOV_POLICY_EXIT); > > lock_policy_rwsem_read(cpu); > kobj = &data->kobj;
On 22 April 2013 05:00, Rafael J. Wysocki <rjw@sisk.pl> wrote: > Applied to linux-pm.git/linux-next, although please check the result, because > the patchwork version of the patch wasn't quite applicable and I fixed it up > manually. Yes it looks fine and that's why i have attached the patch with my email earlier. -- 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 Monday, April 22, 2013 08:44:30 AM Viresh Kumar wrote: > On 22 April 2013 05:00, Rafael J. Wysocki <rjw@sisk.pl> wrote: > > Applied to linux-pm.git/linux-next, although please check the result, because > > the patchwork version of the patch wasn't quite applicable and I fixed it up > > manually. > > Yes it looks fine and that's why i have attached the patch with my > email earlier. Yes, I forgot about the attachment and saw it again only after I had applied the patch. Thanks, Rafael
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 3564947..a6f6595 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -858,13 +858,18 @@ static int cpufreq_add_policy_cpu(unsigned int cpu, unsigned int sibling, struct device *dev) { struct cpufreq_policy *policy; - int ret = 0; + int ret = 0, has_target = 0; unsigned long flags; policy = cpufreq_cpu_get(sibling); WARN_ON(!policy); - __cpufreq_governor(policy, CPUFREQ_GOV_STOP); + rcu_read_lock(); + has_target = !!rcu_dereference(cpufreq_driver)->target; + rcu_read_unlock(); + + if (has_target) + __cpufreq_governor(policy, CPUFREQ_GOV_STOP); lock_policy_rwsem_write(sibling);