| Message ID | 1415189785-18593-1-git-send-email-yadi.brar@samsung.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Eduardo Valentin |
| Headers | show |
Hello Yadwinder, On Wed, Nov 05, 2014 at 05:46:25PM +0530, Yadwinder Singh Brar wrote: > Existing code updates cupfreq policy only while executing > cpufreq_apply_cooling() function (i.e. when notify_device != NOTIFY_INVALID). Correct. The case you mention is when we receive a notification from cpufreq. But also, updates the cpufreq policy when the cooling device changes state, a call from thermal framework. > It doesn't apply constraints when cpufreq policy update happens from any other > place but it should update the cpufreq policy with thermal constraints every > time when there is a cpufreq policy update, to keep state of > cpufreq_cooling_device and max_feq of cpufreq policy in sync. I am not sure I follow you here. Can you please elaborate on 'any other places'? Could you please mention (also in the commit log) what are the case the current code does not cover? For instance, the cpufreq_apply_cooling gets called on notification coming from thermal subsystem, and for changes in cpufreq subsystem, cpufreq_thermal_notifier is called. > > This patch modifies code to maintain a global cpufreq_dev_list and get > corresponding cpufreq_cooling_device for policy's cpu from cpufreq_dev_list > when there is any policy update. OK. Please give real examples of when the current code fails to catch the event. BR, Eduardo Valentin > > Signed-off-by: Yadwinder Singh Brar <yadi.brar@samsung.com> > --- > drivers/thermal/cpu_cooling.c | 37 ++++++++++++++++++++----------------- > 1 files changed, 20 insertions(+), 17 deletions(-) > > diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c > index 1ab0018..5546278 100644 > --- a/drivers/thermal/cpu_cooling.c > +++ b/drivers/thermal/cpu_cooling.c > @@ -50,15 +50,14 @@ struct cpufreq_cooling_device { > unsigned int cpufreq_state; > unsigned int cpufreq_val; > struct cpumask allowed_cpus; > + struct list_head node; > }; > static DEFINE_IDR(cpufreq_idr); > static DEFINE_MUTEX(cooling_cpufreq_lock); > > static unsigned int cpufreq_dev_count; > > -/* notify_table passes value to the CPUFREQ_ADJUST callback function. */ > -#define NOTIFY_INVALID NULL > -static struct cpufreq_cooling_device *notify_device; > +static LIST_HEAD(cpufreq_dev_list); > > /** > * get_idr - function to get a unique id. > @@ -287,15 +286,12 @@ static int cpufreq_apply_cooling(struct cpufreq_cooling_device *cpufreq_device, > > cpufreq_device->cpufreq_state = cooling_state; > cpufreq_device->cpufreq_val = clip_freq; > - notify_device = cpufreq_device; > > for_each_cpu(cpuid, mask) { > if (is_cpufreq_valid(cpuid)) > cpufreq_update_policy(cpuid); > } > > - notify_device = NOTIFY_INVALID; > - > return 0; > } > > @@ -316,21 +312,27 @@ static int cpufreq_thermal_notifier(struct notifier_block *nb, > { > struct cpufreq_policy *policy = data; > unsigned long max_freq = 0; > + struct cpufreq_cooling_device *cpufreq_dev; > > - if (event != CPUFREQ_ADJUST || notify_device == NOTIFY_INVALID) > + if (event != CPUFREQ_ADJUST) > return 0; > > - if (cpumask_test_cpu(policy->cpu, ¬ify_device->allowed_cpus)) > - max_freq = notify_device->cpufreq_val; > - else > - return 0; > + mutex_lock(&cooling_cpufreq_lock); > + list_for_each_entry(cpufreq_dev, &cpufreq_dev_list, node) { > + if (!cpumask_test_cpu(policy->cpu, > + &cpufreq_dev->allowed_cpus)) > + continue; > > - /* Never exceed user_policy.max */ > - if (max_freq > policy->user_policy.max) > - max_freq = policy->user_policy.max; > + max_freq = cpufreq_dev->cpufreq_val; > > - if (policy->max != max_freq) > - cpufreq_verify_within_limits(policy, 0, max_freq); > + /* 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); > + } > + mutex_unlock(&cooling_cpufreq_lock); So, the problem is when we have several cpu cooling devices and you want to search for the real max among the existing cpu cooling devices? > > return 0; > } > @@ -486,7 +488,7 @@ __cpufreq_cooling_register(struct device_node *np, > cpufreq_register_notifier(&thermal_cpufreq_notifier_block, > CPUFREQ_POLICY_NOTIFIER); > cpufreq_dev_count++; > - > + list_add(&cpufreq_dev->node, &cpufreq_dev_list); > mutex_unlock(&cooling_cpufreq_lock); > > return cool_dev; > @@ -549,6 +551,7 @@ void cpufreq_cooling_unregister(struct thermal_cooling_device *cdev) > > cpufreq_dev = cdev->devdata; > mutex_lock(&cooling_cpufreq_lock); > + list_del(&cpufreq_dev->node); > cpufreq_dev_count--; > > /* Unregister the notifier for the last cpufreq cooling device */ > -- > 1.7.0.4 >
Hello Yadwinder, On Thu, Nov 06, 2014 at 04:26:27PM +0530, Yadwinder Singh Brar wrote: > Hello Eduardo Valentin, > > On Thursday, November 06, 2014 2:17 AM, Eduardo Valentin wrote: > > Hello Yadwinder, > > > > On Wed, Nov 05, 2014 at 05:46:25PM +0530, Yadwinder Singh Brar wrote: > > > Existing code updates cupfreq policy only while executing > > > cpufreq_apply_cooling() function (i.e. when notify_device != > > NOTIFY_INVALID). > > > > Correct. The case you mention is when we receive a notification from > > cpufreq. But also, updates the cpufreq policy when the cooling device > > changes state, a call from thermal framework. > > I mentioned thermal framework case only, existing code updates > cupfreq policy only when (notify_device != NOTIFY_INVALID), > which happens only when notification comes from cpufreq_update_policy > while changing cooling device's state(i.e. cpufreq_apply_cooling()). > In case of other notifications notify_device is always NOTIFY_INVALID. I see your point. > > > > > > It doesn't apply constraints when cpufreq policy update happens from > > > any other place but it should update the cpufreq policy with thermal > > > constraints every time when there is a cpufreq policy update, to keep > > > state of cpufreq_cooling_device and max_feq of cpufreq policy in > > sync. > > > > I am not sure I follow you here. Can you please elaborate on 'any other > > places'? Could you please mention (also in the commit log) what are the > > case the current code does not cover? For instance, the > > cpufreq_apply_cooling gets called on notification coming from thermal > > subsystem, and for changes in cpufreq subsystem, > > cpufreq_thermal_notifier is called. > > > > "Other places" mean possible places from where cpufreq_update_policy() > can be called at runtime, an instance in present kernel is cpufreq_resume(). > But since cpufreq_update_policy() is an exposed API, I think > for robustness, generic cpu cooling should be able to take care of any > possible case(in future as well). > OK. I understand now. Can you please improve your commit message by adding the descriptions you mentioned here? > > > > > > This patch modifies code to maintain a global cpufreq_dev_list and > > get > > > corresponding cpufreq_cooling_device for policy's cpu from > > > cpufreq_dev_list when there is any policy update. > > > > OK. Please give real examples of when the current code fails to catch > > the event. > > > > While resuming cpufreq updates cpufreq_policy for boot cpu and it > restores default policy->usr_policy irrespective of cooling device's > cpufreq_state since notification gets missed because (notify_device == > NOTIFY_INVALID). > Another thing, Rather I would say an issue, I observed is that > Userspace is able to change max_freq irrespective of cooling > device's state, as notification gets missed. > Include also the examples above you gave. Have you verified if with this patch the issue with userland goes away? > > > > BR, > > > > Eduardo Valentin > > > > > > > > Signed-off-by: Yadwinder Singh Brar <yadi.brar@samsung.com> > > > --- > > > drivers/thermal/cpu_cooling.c | 37 ++++++++++++++++++++----------- > > ------ > > > 1 files changed, 20 insertions(+), 17 deletions(-) > > > > > > diff --git a/drivers/thermal/cpu_cooling.c > > > b/drivers/thermal/cpu_cooling.c index 1ab0018..5546278 100644 > > > --- a/drivers/thermal/cpu_cooling.c > > > +++ b/drivers/thermal/cpu_cooling.c > > > @@ -50,15 +50,14 @@ struct cpufreq_cooling_device { > > > unsigned int cpufreq_state; > > > unsigned int cpufreq_val; > > > struct cpumask allowed_cpus; > > > + struct list_head node; > > > }; > > > static DEFINE_IDR(cpufreq_idr); > > > static DEFINE_MUTEX(cooling_cpufreq_lock); > > > > > > static unsigned int cpufreq_dev_count; > > > > > > -/* notify_table passes value to the CPUFREQ_ADJUST callback > > function. > > > */ -#define NOTIFY_INVALID NULL -static struct cpufreq_cooling_device > > > *notify_device; > > > +static LIST_HEAD(cpufreq_dev_list); > > > > > > /** > > > * get_idr - function to get a unique id. > > > @@ -287,15 +286,12 @@ static int cpufreq_apply_cooling(struct > > > cpufreq_cooling_device *cpufreq_device, > > > > > > cpufreq_device->cpufreq_state = cooling_state; > > > cpufreq_device->cpufreq_val = clip_freq; > > > - notify_device = cpufreq_device; > > > > > > for_each_cpu(cpuid, mask) { > > > if (is_cpufreq_valid(cpuid)) > > > cpufreq_update_policy(cpuid); > > > } > > > > > > - notify_device = NOTIFY_INVALID; > > > - > > > return 0; > > > } > > > > > > @@ -316,21 +312,27 @@ static int cpufreq_thermal_notifier(struct > > > notifier_block *nb, { > > > struct cpufreq_policy *policy = data; > > > unsigned long max_freq = 0; > > > + struct cpufreq_cooling_device *cpufreq_dev; > > > > > > - if (event != CPUFREQ_ADJUST || notify_device == NOTIFY_INVALID) > > > + if (event != CPUFREQ_ADJUST) > > > return 0; > > > > > > - if (cpumask_test_cpu(policy->cpu, ¬ify_device->allowed_cpus)) > > > - max_freq = notify_device->cpufreq_val; > > > - else > > > - return 0; > > > + mutex_lock(&cooling_cpufreq_lock); > > > + list_for_each_entry(cpufreq_dev, &cpufreq_dev_list, node) { > > > + if (!cpumask_test_cpu(policy->cpu, > > > + &cpufreq_dev->allowed_cpus)) > > > + continue; > > > > > > - /* Never exceed user_policy.max */ > > > - if (max_freq > policy->user_policy.max) > > > - max_freq = policy->user_policy.max; > > > + max_freq = cpufreq_dev->cpufreq_val; > > > > > I think I missed to post updated patch, > Actually it should be : > > + if (!cpufreq_dev->cpufreq_val) > + cpufreq_dev->cpufreq_val = get_cpu_frequency( > + > cpumask_any(&cpufreq_dev->allowed_cpus), > + cpufreq_dev->state); > + max_freq = cpufreq_dev->cpufreq_val; > > I will send another version of patch. ok. > > > > - if (policy->max != max_freq) > > > - cpufreq_verify_within_limits(policy, 0, max_freq); > > > + /* 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); > > > + } > > > + mutex_unlock(&cooling_cpufreq_lock); > > > > So, the problem is when we have several cpu cooling devices and you > > want to search for the real max among the existing cpu cooling devices? > > By max, I meant the maximun constraint (lowest frequency). > > Sorry I didn't get your question completely I think. > No, above code doesn't find max among existing cooling devices. > It simply finds cooling device corresponding to policy's cpu > and applies updates policy accordingly. yeah, that's what it does, but for all matching devices, correct? well, in fact your code is going through all cpu cooling devices that match the cpu ids and applying their max allowed freq, when they differ from policy->max. cpufreq_verify_within_limits will update the policy if your request is lower than policy max. Then you will, in the end, apply the lowest among the existing matching cpu cooling devices. Which, turns out to be the correct thing to do, since we are not doing it per request in single cooling devices. Can you please also add a comment about this strategy? BR, Eduardo Valentin > > Regards, > Yadwinder > > > > return 0; > > > } > > > @@ -486,7 +488,7 @@ __cpufreq_cooling_register(struct device_node > > *np, > > > cpufreq_register_notifier(&thermal_cpufreq_notifier_block, > > > CPUFREQ_POLICY_NOTIFIER); > > > cpufreq_dev_count++; > > > - > > > + list_add(&cpufreq_dev->node, &cpufreq_dev_list); > > > mutex_unlock(&cooling_cpufreq_lock); > > > > > > return cool_dev; > > > @@ -549,6 +551,7 @@ void cpufreq_cooling_unregister(struct > > > thermal_cooling_device *cdev) > > > > > > cpufreq_dev = cdev->devdata; > > > mutex_lock(&cooling_cpufreq_lock); > > > + list_del(&cpufreq_dev->node); > > > cpufreq_dev_count--; > > > > > > /* Unregister the notifier for the last cpufreq cooling device */ > > > -- > > > 1.7.0.4 > > > >
Hello Eduardo Valentin, On Thursday, November 06, 2014 2:17 AM, Eduardo Valentin wrote: > Hello Yadwinder, > > On Wed, Nov 05, 2014 at 05:46:25PM +0530, Yadwinder Singh Brar wrote: > > Existing code updates cupfreq policy only while executing > > cpufreq_apply_cooling() function (i.e. when notify_device != > NOTIFY_INVALID). > > Correct. The case you mention is when we receive a notification from > cpufreq. But also, updates the cpufreq policy when the cooling device > changes state, a call from thermal framework. I mentioned thermal framework case only, existing code updates cupfreq policy only when (notify_device != NOTIFY_INVALID), which happens only when notification comes from cpufreq_update_policy while changing cooling device's state(i.e. cpufreq_apply_cooling()). In case of other notifications notify_device is always NOTIFY_INVALID. > > > It doesn't apply constraints when cpufreq policy update happens from > > any other place but it should update the cpufreq policy with thermal > > constraints every time when there is a cpufreq policy update, to keep > > state of cpufreq_cooling_device and max_feq of cpufreq policy in > sync. > > I am not sure I follow you here. Can you please elaborate on 'any other > places'? Could you please mention (also in the commit log) what are the > case the current code does not cover? For instance, the > cpufreq_apply_cooling gets called on notification coming from thermal > subsystem, and for changes in cpufreq subsystem, > cpufreq_thermal_notifier is called. > "Other places" mean possible places from where cpufreq_update_policy() can be called at runtime, an instance in present kernel is cpufreq_resume(). But since cpufreq_update_policy() is an exposed API, I think for robustness, generic cpu cooling should be able to take care of any possible case(in future as well). > > > > This patch modifies code to maintain a global cpufreq_dev_list and > get > > corresponding cpufreq_cooling_device for policy's cpu from > > cpufreq_dev_list when there is any policy update. > > OK. Please give real examples of when the current code fails to catch > the event. > While resuming cpufreq updates cpufreq_policy for boot cpu and it restores default policy->usr_policy irrespective of cooling device's cpufreq_state since notification gets missed because (notify_device == NOTIFY_INVALID). Another thing, Rather I would say an issue, I observed is that Userspace is able to change max_freq irrespective of cooling device's state, as notification gets missed. > > BR, > > Eduardo Valentin > > > > > Signed-off-by: Yadwinder Singh Brar <yadi.brar@samsung.com> > > --- > > drivers/thermal/cpu_cooling.c | 37 ++++++++++++++++++++----------- > ------ > > 1 files changed, 20 insertions(+), 17 deletions(-) > > > > diff --git a/drivers/thermal/cpu_cooling.c > > b/drivers/thermal/cpu_cooling.c index 1ab0018..5546278 100644 > > --- a/drivers/thermal/cpu_cooling.c > > +++ b/drivers/thermal/cpu_cooling.c > > @@ -50,15 +50,14 @@ struct cpufreq_cooling_device { > > unsigned int cpufreq_state; > > unsigned int cpufreq_val; > > struct cpumask allowed_cpus; > > + struct list_head node; > > }; > > static DEFINE_IDR(cpufreq_idr); > > static DEFINE_MUTEX(cooling_cpufreq_lock); > > > > static unsigned int cpufreq_dev_count; > > > > -/* notify_table passes value to the CPUFREQ_ADJUST callback > function. > > */ -#define NOTIFY_INVALID NULL -static struct cpufreq_cooling_device > > *notify_device; > > +static LIST_HEAD(cpufreq_dev_list); > > > > /** > > * get_idr - function to get a unique id. > > @@ -287,15 +286,12 @@ static int cpufreq_apply_cooling(struct > > cpufreq_cooling_device *cpufreq_device, > > > > cpufreq_device->cpufreq_state = cooling_state; > > cpufreq_device->cpufreq_val = clip_freq; > > - notify_device = cpufreq_device; > > > > for_each_cpu(cpuid, mask) { > > if (is_cpufreq_valid(cpuid)) > > cpufreq_update_policy(cpuid); > > } > > > > - notify_device = NOTIFY_INVALID; > > - > > return 0; > > } > > > > @@ -316,21 +312,27 @@ static int cpufreq_thermal_notifier(struct > > notifier_block *nb, { > > struct cpufreq_policy *policy = data; > > unsigned long max_freq = 0; > > + struct cpufreq_cooling_device *cpufreq_dev; > > > > - if (event != CPUFREQ_ADJUST || notify_device == NOTIFY_INVALID) > > + if (event != CPUFREQ_ADJUST) > > return 0; > > > > - if (cpumask_test_cpu(policy->cpu, ¬ify_device->allowed_cpus)) > > - max_freq = notify_device->cpufreq_val; > > - else > > - return 0; > > + mutex_lock(&cooling_cpufreq_lock); > > + list_for_each_entry(cpufreq_dev, &cpufreq_dev_list, node) { > > + if (!cpumask_test_cpu(policy->cpu, > > + &cpufreq_dev->allowed_cpus)) > > + continue; > > > > - /* Never exceed user_policy.max */ > > - if (max_freq > policy->user_policy.max) > > - max_freq = policy->user_policy.max; > > + max_freq = cpufreq_dev->cpufreq_val; > > I think I missed to post updated patch, Actually it should be : + if (!cpufreq_dev->cpufreq_val) + cpufreq_dev->cpufreq_val = get_cpu_frequency( + cpumask_any(&cpufreq_dev->allowed_cpus), + cpufreq_dev->state); + max_freq = cpufreq_dev->cpufreq_val; I will send another version of patch. > > - if (policy->max != max_freq) > > - cpufreq_verify_within_limits(policy, 0, max_freq); > > + /* 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); > > + } > > + mutex_unlock(&cooling_cpufreq_lock); > > So, the problem is when we have several cpu cooling devices and you > want to search for the real max among the existing cpu cooling devices? > Sorry I didn't get your question completely I think. No, above code doesn't find max among existing cooling devices. It simply finds cooling device corresponding to policy's cpu and applies updates policy accordingly. Regards, Yadwinder > > return 0; > > } > > @@ -486,7 +488,7 @@ __cpufreq_cooling_register(struct device_node > *np, > > cpufreq_register_notifier(&thermal_cpufreq_notifier_block, > > CPUFREQ_POLICY_NOTIFIER); > > cpufreq_dev_count++; > > - > > + list_add(&cpufreq_dev->node, &cpufreq_dev_list); > > mutex_unlock(&cooling_cpufreq_lock); > > > > return cool_dev; > > @@ -549,6 +551,7 @@ void cpufreq_cooling_unregister(struct > > thermal_cooling_device *cdev) > > > > cpufreq_dev = cdev->devdata; > > mutex_lock(&cooling_cpufreq_lock); > > + list_del(&cpufreq_dev->node); > > cpufreq_dev_count--; > > > > /* Unregister the notifier for the last cpufreq cooling device */ > > -- > > 1.7.0.4 > > -- 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
Hello Eduardo Valentin, On Thursday, November 06, 2014 1:19 PM, Eduardo Valentin wrote: > Hello Yadwinder, > > On Thu, Nov 06, 2014 at 04:26:27PM +0530, Yadwinder Singh Brar wrote: > > Hello Eduardo Valentin, > > > > On Thursday, November 06, 2014 2:17 AM, Eduardo Valentin wrote: > > > Hello Yadwinder, > > > > > > On Wed, Nov 05, 2014 at 05:46:25PM +0530, Yadwinder Singh Brar > wrote: > > > > Existing code updates cupfreq policy only while executing > > > > cpufreq_apply_cooling() function (i.e. when notify_device != > > > NOTIFY_INVALID). > > > > > > Correct. The case you mention is when we receive a notification > from > > > cpufreq. But also, updates the cpufreq policy when the cooling > > > device changes state, a call from thermal framework. > > > > I mentioned thermal framework case only, existing code updates > cupfreq > > policy only when (notify_device != NOTIFY_INVALID), which happens > only > > when notification comes from cpufreq_update_policy while changing > > cooling device's state(i.e. cpufreq_apply_cooling()). > > In case of other notifications notify_device is always > NOTIFY_INVALID. > > I see your point. > > > > > > > > > > It doesn't apply constraints when cpufreq policy update happens > > > > from any other place but it should update the cpufreq policy with > > > > thermal constraints every time when there is a cpufreq policy > > > > update, to keep state of cpufreq_cooling_device and max_feq of > > > > cpufreq policy in > > > sync. > > > > > > I am not sure I follow you here. Can you please elaborate on 'any > > > other places'? Could you please mention (also in the commit log) > > > what are the case the current code does not cover? For instance, > the > > > cpufreq_apply_cooling gets called on notification coming from > > > thermal subsystem, and for changes in cpufreq subsystem, > > > cpufreq_thermal_notifier is called. > > > > > > > "Other places" mean possible places from where > cpufreq_update_policy() > > can be called at runtime, an instance in present kernel is > cpufreq_resume(). > > But since cpufreq_update_policy() is an exposed API, I think for > > robustness, generic cpu cooling should be able to take care of any > > possible case(in future as well). > > > > OK. I understand now. Can you please improve your commit message by > adding the descriptions you mentioned here? > Sure, will post updated patch. > > > > > > > > This patch modifies code to maintain a global cpufreq_dev_list > and > > > get > > > > corresponding cpufreq_cooling_device for policy's cpu from > > > > cpufreq_dev_list when there is any policy update. > > > > > > OK. Please give real examples of when the current code fails to > > > catch the event. > > > > > > > While resuming cpufreq updates cpufreq_policy for boot cpu and it > > restores default policy->usr_policy irrespective of cooling device's > > cpufreq_state since notification gets missed because (notify_device > == > > NOTIFY_INVALID). > > Another thing, Rather I would say an issue, I observed is that > > Userspace is able to change max_freq irrespective of cooling device's > > state, as notification gets missed. > > > > Include also the examples above you gave. > > Have you verified if with this patch the issue with userland goes away? > Yes, Userspace is not able to set scaling_max_freq more than the cooling device constraint(cpufreq_val). But I have a question here, Is it OK to silently set scaling_max_freq less than the requested value from userspace ? > > > > > > BR, > > > > > > Eduardo Valentin > > > > > > > > > > > Signed-off-by: Yadwinder Singh Brar <yadi.brar@samsung.com> > > > > --- > > > > drivers/thermal/cpu_cooling.c | 37 ++++++++++++++++++++------- > ---- > > > ------ > > > > 1 files changed, 20 insertions(+), 17 deletions(-) > > > > > > > > diff --git a/drivers/thermal/cpu_cooling.c > > > > b/drivers/thermal/cpu_cooling.c index 1ab0018..5546278 100644 > > > > --- a/drivers/thermal/cpu_cooling.c > > > > +++ b/drivers/thermal/cpu_cooling.c > > > > @@ -50,15 +50,14 @@ struct cpufreq_cooling_device { > > > > unsigned int cpufreq_state; > > > > unsigned int cpufreq_val; > > > > struct cpumask allowed_cpus; > > > > + struct list_head node; > > > > }; > > > > static DEFINE_IDR(cpufreq_idr); > > > > static DEFINE_MUTEX(cooling_cpufreq_lock); > > > > > > > > static unsigned int cpufreq_dev_count; > > > > > > > > -/* notify_table passes value to the CPUFREQ_ADJUST callback > > > function. > > > > */ -#define NOTIFY_INVALID NULL -static struct > > > > cpufreq_cooling_device *notify_device; > > > > +static LIST_HEAD(cpufreq_dev_list); > > > > > > > > /** > > > > * get_idr - function to get a unique id. > > > > @@ -287,15 +286,12 @@ static int cpufreq_apply_cooling(struct > > > > cpufreq_cooling_device *cpufreq_device, > > > > > > > > cpufreq_device->cpufreq_state = cooling_state; > > > > cpufreq_device->cpufreq_val = clip_freq; > > > > - notify_device = cpufreq_device; > > > > > > > > for_each_cpu(cpuid, mask) { > > > > if (is_cpufreq_valid(cpuid)) > > > > cpufreq_update_policy(cpuid); > > > > } > > > > > > > > - notify_device = NOTIFY_INVALID; > > > > - > > > > return 0; > > > > } > > > > > > > > @@ -316,21 +312,27 @@ static int cpufreq_thermal_notifier(struct > > > > notifier_block *nb, { > > > > struct cpufreq_policy *policy = data; > > > > unsigned long max_freq = 0; > > > > + struct cpufreq_cooling_device *cpufreq_dev; > > > > > > > > - if (event != CPUFREQ_ADJUST || notify_device == > NOTIFY_INVALID) > > > > + if (event != CPUFREQ_ADJUST) > > > > return 0; > > > > > > > > - if (cpumask_test_cpu(policy->cpu, ¬ify_device- > >allowed_cpus)) > > > > - max_freq = notify_device->cpufreq_val; > > > > - else > > > > - return 0; > > > > + mutex_lock(&cooling_cpufreq_lock); > > > > + list_for_each_entry(cpufreq_dev, &cpufreq_dev_list, node) { > > > > + if (!cpumask_test_cpu(policy->cpu, > > > > + &cpufreq_dev->allowed_cpus)) > > > > + continue; > > > > > > > > - /* Never exceed user_policy.max */ > > > > - if (max_freq > policy->user_policy.max) > > > > - max_freq = policy->user_policy.max; > > > > + max_freq = cpufreq_dev->cpufreq_val; > > > > > > > > I think I missed to post updated patch, Actually it should be : > > > > + if (!cpufreq_dev->cpufreq_val) > > + cpufreq_dev->cpufreq_val = get_cpu_frequency( > > + > > cpumask_any(&cpufreq_dev->allowed_cpus), > > + cpufreq_dev->state); > > + max_freq = cpufreq_dev->cpufreq_val; > > > > I will send another version of patch. > > ok. > > > > > > > - if (policy->max != max_freq) > > > > - cpufreq_verify_within_limits(policy, 0, max_freq); > > > > + /* 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); > > > > + } > > > > + mutex_unlock(&cooling_cpufreq_lock); > > > > > > So, the problem is when we have several cpu cooling devices and you > > > want to search for the real max among the existing cpu cooling > devices? > > > > > By max, I meant the maximun constraint (lowest frequency). > > > > > Sorry I didn't get your question completely I think. > > No, above code doesn't find max among existing cooling devices. > > It simply finds cooling device corresponding to policy's cpu and > > applies updates policy accordingly. > > yeah, that's what it does, but for all matching devices, correct? > Exactly !! though in existing kernel we don't have multiple devices yet. > well, in fact your code is going through all cpu cooling devices that > match the cpu ids and applying their max allowed freq, when they differ > from policy->max. cpufreq_verify_within_limits will update the policy > if your request is lower than policy max. Then you will, in the end, > apply the lowest among the existing matching cpu cooling devices. > yes .. > Which, turns out to be the correct thing to do, since we are not doing > it per request in single cooling devices. > > Can you please also add a comment about this strategy? > Sure. Best Regards, Yadwinder > > BR, > > Eduardo Valentin > > > > > Regards, > > Yadwinder > > > > > > return 0; > > > > } > > > > @@ -486,7 +488,7 @@ __cpufreq_cooling_register(struct device_node > > > *np, > > > > > cpufreq_register_notifier(&thermal_cpufreq_notifier_block, > > > > CPUFREQ_POLICY_NOTIFIER); > > > > cpufreq_dev_count++; > > > > - > > > > + list_add(&cpufreq_dev->node, &cpufreq_dev_list); > > > > mutex_unlock(&cooling_cpufreq_lock); > > > > > > > > return cool_dev; > > > > @@ -549,6 +551,7 @@ void cpufreq_cooling_unregister(struct > > > > thermal_cooling_device *cdev) > > > > > > > > cpufreq_dev = cdev->devdata; > > > > mutex_lock(&cooling_cpufreq_lock); > > > > + list_del(&cpufreq_dev->node); > > > > cpufreq_dev_count--; > > > > > > > > /* Unregister the notifier for the last cpufreq cooling > device > > > > */ > > > > -- > > > > 1.7.0.4 > > > > > > -- 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/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c index 1ab0018..5546278 100644 --- a/drivers/thermal/cpu_cooling.c +++ b/drivers/thermal/cpu_cooling.c @@ -50,15 +50,14 @@ struct cpufreq_cooling_device { unsigned int cpufreq_state; unsigned int cpufreq_val; struct cpumask allowed_cpus; + struct list_head node; }; static DEFINE_IDR(cpufreq_idr); static DEFINE_MUTEX(cooling_cpufreq_lock); static unsigned int cpufreq_dev_count; -/* notify_table passes value to the CPUFREQ_ADJUST callback function. */ -#define NOTIFY_INVALID NULL -static struct cpufreq_cooling_device *notify_device; +static LIST_HEAD(cpufreq_dev_list); /** * get_idr - function to get a unique id. @@ -287,15 +286,12 @@ static int cpufreq_apply_cooling(struct cpufreq_cooling_device *cpufreq_device, cpufreq_device->cpufreq_state = cooling_state; cpufreq_device->cpufreq_val = clip_freq; - notify_device = cpufreq_device; for_each_cpu(cpuid, mask) { if (is_cpufreq_valid(cpuid)) cpufreq_update_policy(cpuid); } - notify_device = NOTIFY_INVALID; - return 0; } @@ -316,21 +312,27 @@ static int cpufreq_thermal_notifier(struct notifier_block *nb, { struct cpufreq_policy *policy = data; unsigned long max_freq = 0; + struct cpufreq_cooling_device *cpufreq_dev; - if (event != CPUFREQ_ADJUST || notify_device == NOTIFY_INVALID) + if (event != CPUFREQ_ADJUST) return 0; - if (cpumask_test_cpu(policy->cpu, ¬ify_device->allowed_cpus)) - max_freq = notify_device->cpufreq_val; - else - return 0; + mutex_lock(&cooling_cpufreq_lock); + list_for_each_entry(cpufreq_dev, &cpufreq_dev_list, node) { + if (!cpumask_test_cpu(policy->cpu, + &cpufreq_dev->allowed_cpus)) + continue; - /* Never exceed user_policy.max */ - if (max_freq > policy->user_policy.max) - max_freq = policy->user_policy.max; + max_freq = cpufreq_dev->cpufreq_val; - if (policy->max != max_freq) - cpufreq_verify_within_limits(policy, 0, max_freq); + /* 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); + } + mutex_unlock(&cooling_cpufreq_lock); return 0; } @@ -486,7 +488,7 @@ __cpufreq_cooling_register(struct device_node *np, cpufreq_register_notifier(&thermal_cpufreq_notifier_block, CPUFREQ_POLICY_NOTIFIER); cpufreq_dev_count++; - + list_add(&cpufreq_dev->node, &cpufreq_dev_list); mutex_unlock(&cooling_cpufreq_lock); return cool_dev; @@ -549,6 +551,7 @@ void cpufreq_cooling_unregister(struct thermal_cooling_device *cdev) cpufreq_dev = cdev->devdata; mutex_lock(&cooling_cpufreq_lock); + list_del(&cpufreq_dev->node); cpufreq_dev_count--; /* Unregister the notifier for the last cpufreq cooling device */
Existing code updates cupfreq policy only while executing cpufreq_apply_cooling() function (i.e. when notify_device != NOTIFY_INVALID). It doesn't apply constraints when cpufreq policy update happens from any other place but it should update the cpufreq policy with thermal constraints every time when there is a cpufreq policy update, to keep state of cpufreq_cooling_device and max_feq of cpufreq policy in sync. This patch modifies code to maintain a global cpufreq_dev_list and get corresponding cpufreq_cooling_device for policy's cpu from cpufreq_dev_list when there is any policy update. Signed-off-by: Yadwinder Singh Brar <yadi.brar@samsung.com> --- drivers/thermal/cpu_cooling.c | 37 ++++++++++++++++++++----------------- 1 files changed, 20 insertions(+), 17 deletions(-)