Re: [PATCH] thermal/cpu_cooling: remove local cooling state variable

[Punit Agrawal]

[ adding Viresh ]

Radivoje Jovanovic <radivoje.jovanovic@xxxxxxxxxxxxxxx> writes:

> Hi Agarwal,
>
> On Fri, 24 Jul 2015 16:26:12 +0100
> Punit Agrawal <punit.agrawal@xxxxxxx> wrote:
>
>> Radivoje Jovanovic <radivoje.jovanovic@xxxxxxxxxxxxxxx> writes:
>> 
>> > From: Radivoje Jovanovic <radivoje.jovanovic@xxxxxxxxx>
>> >
>> > there is no need to keep local state variable. if another driver
>> > changes the policy under our feet the cpu_cooling driver will
>> > have the wrong state. Get current state from the policy directly
>> > instead
>> >
>> 
>> Although the patch below looks good, it does add additional
>> processing. I was wondering in what situation do you observe the
>> problem $SUBJECT solves?
>> 
>> Presumably, the policy caps are tighter than those imposed by the cpu
>> cooling device (cpufreq_thermal_notifier should take care of this).
>
> we are using this solution on the platfrom which has user space
> component control cpufreq throttling. However, user space 
> component has its limitations so we are using cpu_cooling as a 
> critical backup. Due to this cpu_cooling does not have correct state
> as a current state so when the change is needed cpu_cooling does
> not make the change since it believes it is in the "correct" state.
> I agree that there is slight increase in processing, but in the case 
> when user space is changing the policy the notifier will not have
> access to the current state of the cpu_cooling to change it
> appropriately.
>

Makes sense. Thanks for the explanation.

One comment below.

>> 
>> > Signed-off-by: Radivoje Jovanovic <radivoje.jovanovic@xxxxxxxxx>
>> > ---
>> >  drivers/thermal/cpu_cooling.c | 27 ++++++++++++++++++---------
>> >  1 file changed, 18 insertions(+), 9 deletions(-)
>> >
>> > diff --git a/drivers/thermal/cpu_cooling.c
>> > b/drivers/thermal/cpu_cooling.c index 6509c61..94ba2da 100644
>> > --- a/drivers/thermal/cpu_cooling.c
>> > +++ b/drivers/thermal/cpu_cooling.c
>> > @@ -66,8 +66,6 @@ struct power_table {
>> >   *	registered.
>> >   * @cool_dev: thermal_cooling_device pointer to keep track of the
>> >   *	registered cooling device.
>> > - * @cpufreq_state: integer value representing the current state of
>> > cpufreq
>> > - *	cooling	devices.
>> >   * @cpufreq_val: integer value representing the absolute value of
>> > the clipped
>> >   *	frequency.
>> >   * @max_level: maximum cooling level. One less than total number
>> > of valid @@ -90,7 +88,6 @@ struct power_table {
>> >  struct cpufreq_cooling_device {
>> >  	int id;
>> >  	struct thermal_cooling_device *cool_dev;
>> > -	unsigned int cpufreq_state;
>> >  	unsigned int cpufreq_val;
>> >  	unsigned int max_level;
>> >  	unsigned int *freq_table;	/* In descending order */
>> > @@ -486,10 +483,19 @@ static int cpufreq_get_cur_state(struct
>> > thermal_cooling_device *cdev, unsigned long *state)
>> >  {
>> >  	struct cpufreq_cooling_device *cpufreq_device =
>> > cdev->devdata; -
>> > -	*state = cpufreq_device->cpufreq_state;
>> > -
>> > -	return 0;
>> > +	struct cpufreq_policy policy;
>> > +	struct cpumask *mask = &cpufreq_device->allowed_cpus;
>> > +	unsigned int cpu = cpumask_any(mask);
>> > +	unsigned int cur_state;
>> > +
>> > +	if (!cpufreq_get_policy(&policy, cpu)) {

The above call returns an error for an offline cpu, but you can still
get a valid policy if any of the allowed_cpus are online. It might make
sense to loop over allowed_cpus until the call succeeds or you run out
of cpus.

Viresh, do you have a better suggestion?

>> > +			cur_state = get_level(cpufreq_device,
>> > policy.max);
>> > +			if (cur_state != THERMAL_CSTATE_INVALID) {
>> > +				*state = cur_state;
>> > +				return 0;
>> > +			}
>> > +	}
>> > +	return -EINVAL;
>> >  }
>> >  
>> >  /**
>> > @@ -508,17 +514,20 @@ static int cpufreq_set_cur_state(struct
>> > thermal_cooling_device *cdev, struct cpufreq_cooling_device
>> > *cpufreq_device = cdev->devdata; unsigned int cpu =
>> > cpumask_any(&cpufreq_device->allowed_cpus); unsigned int clip_freq;
>> > +	unsigned long cur_state;
>> >  
>> >  	/* Request state should be less than max_level */
>> >  	if (WARN_ON(state > cpufreq_device->max_level))
>> >  		return -EINVAL;
>> >  
>> > +	if (cpufreq_get_cur_state(cpufreq_device->cool_dev,
>> > &cur_state))
>> > +		return -EINVAL;
>> > +
>> >  	/* Check if the old cooling action is same as new cooling
>> > action */
>> > -	if (cpufreq_device->cpufreq_state == state)
>> > +	if (cur_state == state)
>> >  		return 0;
>> >  
>> >  	clip_freq = cpufreq_device->freq_table[state];
>> > -	cpufreq_device->cpufreq_state = state;
>> >  	cpufreq_device->cpufreq_val = clip_freq;
>> >  
>> >  	cpufreq_update_policy(cpu);
>
> Thanks
> Ogi
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
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/