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

[Radivoje Jovanovic]

On Wed, 29 Jul 2015 17:46:37 +0100
Punit Agrawal <punit.agrawal@xxxxxxx> wrote:

Hi Agarwal,

> [ 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?

good call. I will wait for Viresh to respond. Unless there is a better
suggestion I will push a new patch within a few days

> 
> >> > +			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-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/