* [PATCH] [RFC] cpufreq: can't raise max frequency with cpu_thermal @ 2012-12-18 8:29 Sonny Rao 2012-12-18 16:03 ` Doug Anderson 2012-12-19 4:17 ` amit daniel kachhap 0 siblings, 2 replies; 6+ messages in thread From: Sonny Rao @ 2012-12-18 8:29 UTC (permalink / raw) To: linux-pm Cc: linux-kernel, Zhang Rui, Amit Daniel Kachhap, Doug Anderson, Sameer Nanda, Sonny Rao The cpu_thermal generic thermal management code has a bug where once max cpu frequency has been lowered in sysfs (scaling_max_freq) it is not possible to raise the max back up later. The bug is that the notifer gets called by __cpufreq_set_policy() before the user policy max is raised, and is incorrectly trying to enforce the max frequency policy even when we are trying to change the policy. It is also not clear why this driver is looking at the user policy since it is primarily supposed to enforce thermal policy, not user set policy. Signed-off-by: Sonny Rao <sonnyrao@chromium.org> --- drivers/thermal/cpu_cooling.c | 4 ---- 1 files changed, 0 insertions(+), 4 deletions(-) diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c index 836828e..63bc708 100644 --- a/drivers/thermal/cpu_cooling.c +++ b/drivers/thermal/cpu_cooling.c @@ -219,10 +219,6 @@ static int cpufreq_thermal_notifier(struct notifier_block *nb, if (cpumask_test_cpu(policy->cpu, ¬ify_device->allowed_cpus)) max_freq = notify_device->cpufreq_val; - /* Never exceed user_policy.max*/ - if (max_freq > policy->user_policy.max) - max_freq = policy->user_policy.max; - if (policy->max != max_freq) cpufreq_verify_within_limits(policy, 0, max_freq); -- 1.7.7.3 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] [RFC] cpufreq: can't raise max frequency with cpu_thermal 2012-12-18 8:29 [PATCH] [RFC] cpufreq: can't raise max frequency with cpu_thermal Sonny Rao @ 2012-12-18 16:03 ` Doug Anderson 2012-12-19 4:17 ` amit daniel kachhap 1 sibling, 0 replies; 6+ messages in thread From: Doug Anderson @ 2012-12-18 16:03 UTC (permalink / raw) To: Sonny Rao Cc: linux-pm, linux-kernel@vger.kernel.org, Zhang Rui, Amit Daniel Kachhap, Sameer Nanda, Subash On Tue, Dec 18, 2012 at 12:29 AM, Sonny Rao <sonnyrao@chromium.org> wrote: > The cpu_thermal generic thermal management code has a bug where once > max cpu frequency has been lowered in sysfs (scaling_max_freq) it is > not possible to raise the max back up later. The bug is that the > notifer gets called by __cpufreq_set_policy() before the user policy > max is raised, and is incorrectly trying to enforce the max frequency > policy even when we are trying to change the policy. It is also not > clear why this driver is looking at the user policy since it is > primarily supposed to enforce thermal policy, not user set policy. > > Signed-off-by: Sonny Rao <sonnyrao@chromium.org> > --- > drivers/thermal/cpu_cooling.c | 4 ---- > 1 files changed, 0 insertions(+), 4 deletions(-) > > diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c > index 836828e..63bc708 100644 > --- a/drivers/thermal/cpu_cooling.c > +++ b/drivers/thermal/cpu_cooling.c > @@ -219,10 +219,6 @@ static int cpufreq_thermal_notifier(struct notifier_block *nb, > if (cpumask_test_cpu(policy->cpu, ¬ify_device->allowed_cpus)) > max_freq = notify_device->cpufreq_val; > > - /* Never exceed user_policy.max*/ > - if (max_freq > policy->user_policy.max) > - max_freq = policy->user_policy.max; > - > if (policy->max != max_freq) > cpufreq_verify_within_limits(policy, 0, max_freq); > > -- > 1.7.7.3 > Sonny's change matches what the "ACPI version" of this code (drivers/acpi/processor_thermal.c) does as well. I would certainly be interested to know why the code was added here in the first place. Amit: do you know? Reviewed-by: Doug Anderson <dianders@chromium.org> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] [RFC] cpufreq: can't raise max frequency with cpu_thermal 2012-12-18 8:29 [PATCH] [RFC] cpufreq: can't raise max frequency with cpu_thermal Sonny Rao 2012-12-18 16:03 ` Doug Anderson @ 2012-12-19 4:17 ` amit daniel kachhap 2012-12-19 5:45 ` Doug Anderson 1 sibling, 1 reply; 6+ messages in thread From: amit daniel kachhap @ 2012-12-19 4:17 UTC (permalink / raw) To: Sonny Rao; +Cc: linux-pm, linux-kernel, Zhang Rui, Doug Anderson, Sameer Nanda On Tue, Dec 18, 2012 at 12:29 AM, Sonny Rao <sonnyrao@chromium.org> wrote: > The cpu_thermal generic thermal management code has a bug where once > max cpu frequency has been lowered in sysfs (scaling_max_freq) it is > not possible to raise the max back up later. The bug is that the > notifer gets called by __cpufreq_set_policy() before the user policy > max is raised, and is incorrectly trying to enforce the max frequency > policy even when we are trying to change the policy. It is also not > clear why this driver is looking at the user policy since it is > primarily supposed to enforce thermal policy, not user set policy. Hi Sunny, I am not sure if this change is needed. There is a check in cpufreq_thermal_notifier function to return 0 if notify_device == NOTIFY_INVALID. So the user will be always able to change the max frequency in normal situation. Did you tested this for some corner cases? The reason behind putting this check is that I don't want to override the user constraints. Thanks, Amit Daniel > > Signed-off-by: Sonny Rao <sonnyrao@chromium.org> > --- > drivers/thermal/cpu_cooling.c | 4 ---- > 1 files changed, 0 insertions(+), 4 deletions(-) > > diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c > index 836828e..63bc708 100644 > --- a/drivers/thermal/cpu_cooling.c > +++ b/drivers/thermal/cpu_cooling.c > @@ -219,10 +219,6 @@ static int cpufreq_thermal_notifier(struct notifier_block *nb, > if (cpumask_test_cpu(policy->cpu, ¬ify_device->allowed_cpus)) > max_freq = notify_device->cpufreq_val; > > - /* Never exceed user_policy.max*/ > - if (max_freq > policy->user_policy.max) > - max_freq = policy->user_policy.max; > - > if (policy->max != max_freq) > cpufreq_verify_within_limits(policy, 0, max_freq); > > -- > 1.7.7.3 > > -- > 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/ ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] [RFC] cpufreq: can't raise max frequency with cpu_thermal 2012-12-19 4:17 ` amit daniel kachhap @ 2012-12-19 5:45 ` Doug Anderson 2012-12-26 19:32 ` amit daniel kachhap 0 siblings, 1 reply; 6+ messages in thread From: Doug Anderson @ 2012-12-19 5:45 UTC (permalink / raw) To: amit daniel kachhap Cc: Sonny Rao, linux-pm, linux-kernel@vger.kernel.org, Zhang Rui, Sameer Nanda Amit, On Tue, Dec 18, 2012 at 8:17 PM, amit daniel kachhap <amit.daniel@samsung.com> wrote: > On Tue, Dec 18, 2012 at 12:29 AM, Sonny Rao <sonnyrao@chromium.org> wrote: >> The cpu_thermal generic thermal management code has a bug where once >> max cpu frequency has been lowered in sysfs (scaling_max_freq) it is >> not possible to raise the max back up later. The bug is that the >> notifer gets called by __cpufreq_set_policy() before the user policy >> max is raised, and is incorrectly trying to enforce the max frequency >> policy even when we are trying to change the policy. It is also not >> clear why this driver is looking at the user policy since it is >> primarily supposed to enforce thermal policy, not user set policy. > > Hi Sunny, > > I am not sure if this change is needed. Do you have a machine that's running with your code? Can you go into sysfs (/sys/devices/system/cpu/cpu0/cpufreq/) and try lowering then raising the max frequency by doing something like this (assumes that you can scale down to 200MHz): cd /sys/devices/system/cpu/cpu0/cpufreq/ OLD_VAL=$(cat scaling_max_freq) cat scaling_min_freq > scaling_max_freq echo ${OLD_VAL} > scaling_max_freq echo "$(cat scaling_max_freq) should be ${OLD_VAL}. Is it?" ...when I run the above without Sonny's patch on my system I see: 200000 should be 1700000. Is it? ...after Sonny's patch then the above works. > There is a check in cpufreq_thermal_notifier function to return 0 if > notify_device == NOTIFY_INVALID. So the user will be always able to > change the max frequency in normal situation. Did you tested this for > some corner cases? > The reason behind putting this check is that I don't want to override > the user constraints. > > Thanks, > Amit Daniel > >> >> Signed-off-by: Sonny Rao <sonnyrao@chromium.org> >> --- >> drivers/thermal/cpu_cooling.c | 4 ---- >> 1 files changed, 0 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c >> index 836828e..63bc708 100644 >> --- a/drivers/thermal/cpu_cooling.c >> +++ b/drivers/thermal/cpu_cooling.c >> @@ -219,10 +219,6 @@ static int cpufreq_thermal_notifier(struct notifier_block *nb, >> if (cpumask_test_cpu(policy->cpu, ¬ify_device->allowed_cpus)) >> max_freq = notify_device->cpufreq_val; >> >> - /* Never exceed user_policy.max*/ >> - if (max_freq > policy->user_policy.max) >> - max_freq = policy->user_policy.max; >> - >> if (policy->max != max_freq) >> cpufreq_verify_within_limits(policy, 0, max_freq); >> >> -- >> 1.7.7.3 >> >> -- >> 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/ -Doug ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] [RFC] cpufreq: can't raise max frequency with cpu_thermal 2012-12-19 5:45 ` Doug Anderson @ 2012-12-26 19:32 ` amit daniel kachhap 2012-12-29 21:04 ` Sonny Rao 0 siblings, 1 reply; 6+ messages in thread From: amit daniel kachhap @ 2012-12-26 19:32 UTC (permalink / raw) To: Doug Anderson Cc: Sonny Rao, linux-pm, linux-kernel@vger.kernel.org, Zhang Rui, Sameer Nanda On Tue, Dec 18, 2012 at 9:45 PM, Doug Anderson <dianders@chromium.org> wrote: > Amit, > > On Tue, Dec 18, 2012 at 8:17 PM, amit daniel kachhap > <amit.daniel@samsung.com> wrote: >> On Tue, Dec 18, 2012 at 12:29 AM, Sonny Rao <sonnyrao@chromium.org> wrote: >>> The cpu_thermal generic thermal management code has a bug where once >>> max cpu frequency has been lowered in sysfs (scaling_max_freq) it is >>> not possible to raise the max back up later. The bug is that the >>> notifer gets called by __cpufreq_set_policy() before the user policy >>> max is raised, and is incorrectly trying to enforce the max frequency >>> policy even when we are trying to change the policy. It is also not >>> clear why this driver is looking at the user policy since it is >>> primarily supposed to enforce thermal policy, not user set policy. >> >> Hi Sunny, >> >> I am not sure if this change is needed. > > Do you have a machine that's running with your code? Can you go into > sysfs (/sys/devices/system/cpu/cpu0/cpufreq/) and try lowering then > raising the max frequency by doing something like this (assumes that > you can scale down to 200MHz): > > cd /sys/devices/system/cpu/cpu0/cpufreq/ > OLD_VAL=$(cat scaling_max_freq) > cat scaling_min_freq > scaling_max_freq > echo ${OLD_VAL} > scaling_max_freq > > echo "$(cat scaling_max_freq) should be ${OLD_VAL}. Is it?" > > ...when I run the above without Sonny's patch on my system I see: > 200000 should be 1700000. Is it? > > ...after Sonny's patch then the above works. Hi Doug, I tested the above steps on exynos origen board with all cpufreq cooling configs enabled in kernel version 3.8-rc1. In my tests I am able to vary scaling_max_freq to all values. Also I am in normal temperature threshold. So basically I am not able to reproduce the error reported, Thanks, Amit Daniel > >> There is a check in cpufreq_thermal_notifier function to return 0 if >> notify_device == NOTIFY_INVALID. So the user will be always able to >> change the max frequency in normal situation. Did you tested this for >> some corner cases? >> The reason behind putting this check is that I don't want to override >> the user constraints. >> >> Thanks, >> Amit Daniel >> >>> >>> Signed-off-by: Sonny Rao <sonnyrao@chromium.org> >>> --- >>> drivers/thermal/cpu_cooling.c | 4 ---- >>> 1 files changed, 0 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c >>> index 836828e..63bc708 100644 >>> --- a/drivers/thermal/cpu_cooling.c >>> +++ b/drivers/thermal/cpu_cooling.c >>> @@ -219,10 +219,6 @@ static int cpufreq_thermal_notifier(struct notifier_block *nb, >>> if (cpumask_test_cpu(policy->cpu, ¬ify_device->allowed_cpus)) >>> max_freq = notify_device->cpufreq_val; >>> >>> - /* Never exceed user_policy.max*/ >>> - if (max_freq > policy->user_policy.max) >>> - max_freq = policy->user_policy.max; >>> - >>> if (policy->max != max_freq) >>> cpufreq_verify_within_limits(policy, 0, max_freq); >>> >>> -- >>> 1.7.7.3 >>> >>> -- >>> 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/ > > -Doug > -- > 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/ ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] [RFC] cpufreq: can't raise max frequency with cpu_thermal 2012-12-26 19:32 ` amit daniel kachhap @ 2012-12-29 21:04 ` Sonny Rao 0 siblings, 0 replies; 6+ messages in thread From: Sonny Rao @ 2012-12-29 21:04 UTC (permalink / raw) To: amit daniel kachhap Cc: Doug Anderson, linux-pm, linux-kernel@vger.kernel.org, Zhang Rui, Sameer Nanda On Wed, Dec 26, 2012 at 11:32 AM, amit daniel kachhap <amit.daniel@samsung.com> wrote: > > On Tue, Dec 18, 2012 at 9:45 PM, Doug Anderson <dianders@chromium.org> wrote: > > Amit, > > > > On Tue, Dec 18, 2012 at 8:17 PM, amit daniel kachhap > > <amit.daniel@samsung.com> wrote: > >> On Tue, Dec 18, 2012 at 12:29 AM, Sonny Rao <sonnyrao@chromium.org> wrote: > >>> The cpu_thermal generic thermal management code has a bug where once > >>> max cpu frequency has been lowered in sysfs (scaling_max_freq) it is > >>> not possible to raise the max back up later. The bug is that the > >>> notifer gets called by __cpufreq_set_policy() before the user policy > >>> max is raised, and is incorrectly trying to enforce the max frequency > >>> policy even when we are trying to change the policy. It is also not > >>> clear why this driver is looking at the user policy since it is > >>> primarily supposed to enforce thermal policy, not user set policy. > >> > >> Hi Sunny, > >> > >> I am not sure if this change is needed. > > > > Do you have a machine that's running with your code? Can you go into > > sysfs (/sys/devices/system/cpu/cpu0/cpufreq/) and try lowering then > > raising the max frequency by doing something like this (assumes that > > you can scale down to 200MHz): > > > > cd /sys/devices/system/cpu/cpu0/cpufreq/ > > OLD_VAL=$(cat scaling_max_freq) > > cat scaling_min_freq > scaling_max_freq > > echo ${OLD_VAL} > scaling_max_freq > > > > echo "$(cat scaling_max_freq) should be ${OLD_VAL}. Is it?" > > > > ...when I run the above without Sonny's patch on my system I see: > > 200000 should be 1700000. Is it? > > > > ...after Sonny's patch then the above works. > Hi Doug, > > I tested the above steps on exynos origen board with all cpufreq > cooling configs enabled in kernel version 3.8-rc1. > In my tests I am able to vary scaling_max_freq to all values. Also I > am in normal temperature threshold. So basically I am not able to > reproduce the error reported, > > Thanks, > Amit Daniel Hi, thanks for checking it out. I'm a bit surprised that you couldn't reproduce it, but it might just be that it will only manifest on a platform which is using that driver - like Exynos 5? We'll try it out on a 3.8-rc on an Exynos and let you know if we see it or not. Thanks again, Sonny > > > > >> There is a check in cpufreq_thermal_notifier function to return 0 if > >> notify_device == NOTIFY_INVALID. So the user will be always able to > >> change the max frequency in normal situation. Did you tested this for > >> some corner cases? > >> The reason behind putting this check is that I don't want to override > >> the user constraints. > >> > >> Thanks, > >> Amit Daniel > >> > >>> > >>> Signed-off-by: Sonny Rao <sonnyrao@chromium.org> > >>> --- > >>> drivers/thermal/cpu_cooling.c | 4 ---- > >>> 1 files changed, 0 insertions(+), 4 deletions(-) > >>> > >>> diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c > >>> index 836828e..63bc708 100644 > >>> --- a/drivers/thermal/cpu_cooling.c > >>> +++ b/drivers/thermal/cpu_cooling.c > >>> @@ -219,10 +219,6 @@ static int cpufreq_thermal_notifier(struct notifier_block *nb, > >>> if (cpumask_test_cpu(policy->cpu, ¬ify_device->allowed_cpus)) > >>> max_freq = notify_device->cpufreq_val; > >>> > >>> - /* Never exceed user_policy.max*/ > >>> - if (max_freq > policy->user_policy.max) > >>> - max_freq = policy->user_policy.max; > >>> - > >>> if (policy->max != max_freq) > >>> cpufreq_verify_within_limits(policy, 0, max_freq); > >>> > >>> -- > >>> 1.7.7.3 > >>> > >>> -- > >>> 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/ > > > > -Doug > > -- > > 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/ ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-12-29 21:05 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-12-18 8:29 [PATCH] [RFC] cpufreq: can't raise max frequency with cpu_thermal Sonny Rao 2012-12-18 16:03 ` Doug Anderson 2012-12-19 4:17 ` amit daniel kachhap 2012-12-19 5:45 ` Doug Anderson 2012-12-26 19:32 ` amit daniel kachhap 2012-12-29 21:04 ` Sonny Rao
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).