RE: [PATCH] thermal: cpu_cooling: Update always cpufreq policy with thermal constraints

[Yadwinder Singh Brar]

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@xxxxxxxxxxx>
> > ---
> >  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, &notify_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-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/