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

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

[Radivoje Jovanovic]

From: Radivoje Jovanovic <radivoje.jovanovic@intel.com>

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

Signed-off-by: Radivoje Jovanovic <radivoje.jovanovic@intel.com>
---
 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)) {
+			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);
-- 
1.9.1

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

[Punit Agrawal]

Radivoje Jovanovic <radivoje.jovanovic@linux.intel.com> writes:

> From: Radivoje Jovanovic <radivoje.jovanovic@intel.com>
>
> 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).

> Signed-off-by: Radivoje Jovanovic <radivoje.jovanovic@intel.com>
> ---
>  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)) {
> +			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);

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

[Radivoje Jovanovic]

Hi Agarwal,

On Fri, 24 Jul 2015 16:26:12 +0100
Punit Agrawal <punit.agrawal@arm.com> wrote:

> Radivoje Jovanovic <radivoje.jovanovic@linux.intel.com> writes:
> 
> > From: Radivoje Jovanovic <radivoje.jovanovic@intel.com>
> >
> > 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.

> 
> > Signed-off-by: Radivoje Jovanovic <radivoje.jovanovic@intel.com>
> > ---
> >  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)) {
> > +			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

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

[Punit Agrawal]

[ adding Viresh ]

Radivoje Jovanovic <radivoje.jovanovic@linux.intel.com> writes:

> Hi Agarwal,
>
> On Fri, 24 Jul 2015 16:26:12 +0100
> Punit Agrawal <punit.agrawal@arm.com> wrote:
>
>> Radivoje Jovanovic <radivoje.jovanovic@linux.intel.com> writes:
>> 
>> > From: Radivoje Jovanovic <radivoje.jovanovic@intel.com>
>> >
>> > 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@intel.com>
>> > ---
>> >  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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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@arm.com> wrote:

Hi Agarwal,

> [ adding Viresh ]
> 
> Radivoje Jovanovic <radivoje.jovanovic@linux.intel.com> writes:
> 
> > Hi Agarwal,
> >
> > On Fri, 24 Jul 2015 16:26:12 +0100
> > Punit Agrawal <punit.agrawal@arm.com> wrote:
> >
> >> Radivoje Jovanovic <radivoje.jovanovic@linux.intel.com> writes:
> >> 
> >> > From: Radivoje Jovanovic <radivoje.jovanovic@intel.com>
> >> >
> >> > 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@intel.com>
> >> > ---
> >> >  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@vger.kernel.org
> > 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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Viresh Kumar]

Cc'ing Rafael as well..

On 29-07-15, 17:46, Punit Agrawal wrote:
> [ adding Viresh ]

Thanks. That earned me few more patches ;)

> Radivoje Jovanovic <radivoje.jovanovic@linux.intel.com> writes:
> 
> > Hi Agarwal,
> >
> > On Fri, 24 Jul 2015 16:26:12 +0100
> > Punit Agrawal <punit.agrawal@arm.com> wrote:
> >
> >> Radivoje Jovanovic <radivoje.jovanovic@linux.intel.com> writes:
> >> 
> >> > From: Radivoje Jovanovic <radivoje.jovanovic@intel.com>
> >> >
> >> > 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.

Sorry, but with what I understood it doesn't make sense. And I can be wrong
here, so please don't laugh at me :)

So, we have two external suppliers to policy->max here:
- user space: which decides the maximum frequency the policy can ever achieve.
- thermal: which decides the maximum safe frequency the policy should ever be
  set to.

We need to set policy->max based on what user requested, but keeping in mind the
thermal limitations.

So if the clipped-freq from thermal is higher than what user has requested, we
don't need to do anything.

But if the clipped-freq is lower than what user has requested, then we need to
correct that to keep the system in safe range.

That's what the code is doing as well.

Now coming to the change you made. What you are saying is, we should report
current state based on the value of policy->max. But why?

policy->max can be lesser than clipped-freq (set by thermal), and the current
state of thermal clipped-freq isn't what policy->max gives.

Now, I didn't understood when you said "cpu_cooling doesn't change the state
since it believes it is in correct state". Can you please explain that with some
example?

-- 
viresh

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

[Radivoje Jovanovic]

On Thu, 30 Jul 2015 13:35:41 +0530
Viresh Kumar <viresh.kumar@linaro.org> wrote:

Hi Viresh,

> Cc'ing Rafael as well..
> 
> On 29-07-15, 17:46, Punit Agrawal wrote:
> > [ adding Viresh ]
> 
> Thanks. That earned me few more patches ;)
> 
> > Radivoje Jovanovic <radivoje.jovanovic@linux.intel.com> writes:
> > 
> > > Hi Agarwal,
> > >
> > > On Fri, 24 Jul 2015 16:26:12 +0100
> > > Punit Agrawal <punit.agrawal@arm.com> wrote:
> > >
> > >> Radivoje Jovanovic <radivoje.jovanovic@linux.intel.com> writes:
> > >> 
> > >> > From: Radivoje Jovanovic <radivoje.jovanovic@intel.com>
> > >> >
> > >> > 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.
> 
> Sorry, but with what I understood it doesn't make sense. And I can be
> wrong here, so please don't laugh at me :)
> 
> So, we have two external suppliers to policy->max here:
> - user space: which decides the maximum frequency the policy can ever
> achieve.
> - thermal: which decides the maximum safe frequency the policy should
> ever be set to.
> 
> We need to set policy->max based on what user requested, but keeping
> in mind the thermal limitations.
> 
> So if the clipped-freq from thermal is higher than what user has
> requested, we don't need to do anything.
> 
> But if the clipped-freq is lower than what user has requested, then
> we need to correct that to keep the system in safe range.
> 
> That's what the code is doing as well.
> 
> Now coming to the change you made. What you are saying is, we should
> report current state based on the value of policy->max. But why?
> 
> policy->max can be lesser than clipped-freq (set by thermal), and the
> current state of thermal clipped-freq isn't what policy->max gives.
> 
> Now, I didn't understood when you said "cpu_cooling doesn't change
> the state since it believes it is in correct state". Can you please
> explain that with some example?
> 
In this case both userspace thermal solution and cpu_cooling are
changing policy->max and the userspace solution will let governor or HW
(depends on architecture) decide the clipped-freq. Now let us say that
cpu_cooling has 4 available states 0-3 and let us say that cpu_cooling
has set the state 1 as the last state. Now userspace component comes in
and changes the state of the system that matches cpu_cooling state 0.
cpu_cooling is unaware of this change and does not change the local
cur_state. Now the temperature changes and cpu_cooling should change
the system state to 1 (userspace component malfunctioned and is not
picking up this change) but since the cur_state is already at 1
cpu_cooling will not do anything since it believes it is in the correct
state. Hope this explains it better

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

[Viresh Kumar]

Thanks.

I will try to add more layman terms here to map cooling state with
frequencies. So, the cooling state 0 maps to the highest frequency the
cpufreq table supports, and the highest cooling state n maps to the
lowest frequency. Right ?

On 30-07-15, 13:21, Radivoje Jovanovic wrote:
> In this case both userspace thermal solution and cpu_cooling are
> changing policy->max and the userspace solution will let governor or HW
> (depends on architecture) decide the clipped-freq. Now let us say that
> cpu_cooling has 4 available states 0-3

Lets say: 0 == 1.2 GHz
          1 == 1.1 GHz
          2 == 1 GHz
          3 == 800 MHz

> and let us say that cpu_cooling
> has set the state 1 as the last state.

i.e. cpu_cooling says "don't go over 1.1 GHz"..

> Now userspace component comes in
> and changes the state of the system that matches cpu_cooling state 0.

So, policy->max reaches 1.2 GHz and that is not in sync with
cpu_cooling. Right ?

> cpu_cooling is unaware of this change and does not change the local
> cur_state.

That's where I think you one of us might be incorrect. At this point
when policy->max is changed to 1.2 GHz, a notifier will get issued to
cpu_cooling, which will bring policy->max again to 1.1 GHz and so
things will be back in control.

> Now the temperature changes and cpu_cooling should change
> the system state to 1 (userspace component malfunctioned and is not
> picking up this change) but since the cur_state is already at 1
> cpu_cooling will not do anything since it believes it is in the correct
> state. Hope this explains it better

-- 
viresh

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

[Radivoje Jovanovic]

On Fri, 31 Jul 2015 08:48:41 +0530
Viresh Kumar <viresh.kumar@linaro.org> wrote:

> Thanks.
> 
> I will try to add more layman terms here to map cooling state with
> frequencies. So, the cooling state 0 maps to the highest frequency the
> cpufreq table supports, and the highest cooling state n maps to the
> lowest frequency. Right ?
> 
> On 30-07-15, 13:21, Radivoje Jovanovic wrote:
> > In this case both userspace thermal solution and cpu_cooling are
> > changing policy->max and the userspace solution will let governor
> > or HW (depends on architecture) decide the clipped-freq. Now let us
> > say that cpu_cooling has 4 available states 0-3
> 
> Lets say: 0 == 1.2 GHz
>           1 == 1.1 GHz
>           2 == 1 GHz
>           3 == 800 MHz
> 
> > and let us say that cpu_cooling
> > has set the state 1 as the last state.
> 
> i.e. cpu_cooling says "don't go over 1.1 GHz"..
> 
> > Now userspace component comes in
> > and changes the state of the system that matches cpu_cooling state
> > 0.
> 
> So, policy->max reaches 1.2 GHz and that is not in sync with
> cpu_cooling. Right ?
> 
> > cpu_cooling is unaware of this change and does not change the local
> > cur_state.
> 
> That's where I think you one of us might be incorrect. At this point
> when policy->max is changed to 1.2 GHz, a notifier will get issued to
> cpu_cooling, which will bring policy->max again to 1.1 GHz and so
> things will be back in control.
I just looked over the notifier in the current upstream (my patch was
made on our production kernel which is 3.14 and has old notifier
implementation with notifier_device in place) and I see your point.
I agree with you that this patch is trivial for the current
implementation since the notifier, as it is currently, will enforce
cpu_cooling policy change at every CPUFREQ_ADJUST which would cause
problems in our current implementation. In our implementation there is
a cpufreq driver that will also change policies during CPUFREQ_ADJUST,
once the request comes from the underlying FW so there would be a fight
who gets there first since cpu_cooling will change the policy in
CPUFREQ_ADJUST notifier_chain and the driver would do the same thing.
It seems to me that better implementation of the cpu_cooling notifer
would be to keep the flag and change the policy in CPUFREQ_ADJUST only
when the change was requested by cpu_cooling, and update the current
state of cpufreq_cooling_device during CPUFREQ_NOTIFY event.
What do you think?

> 
> > Now the temperature changes and cpu_cooling should change
> > the system state to 1 (userspace component malfunctioned and is not
> > picking up this change) but since the cur_state is already at 1
> > cpu_cooling will not do anything since it believes it is in the
> > correct state. Hope this explains it better
> 

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

[Viresh Kumar]

On 31-07-15, 08:30, Radivoje Jovanovic wrote:
> I just looked over the notifier in the current upstream (my patch was
> made on our production kernel which is 3.14 and has old notifier
> implementation with notifier_device in place) and I see your point.

That's disappointing. You were expected to check if the same problem
exists in mainline.

> I agree with you that this patch is trivial for the current
> implementation since the notifier, as it is currently, will enforce
> cpu_cooling policy change at every CPUFREQ_ADJUST which would cause
> problems in our current implementation. In our implementation there is
> a cpufreq driver that will also change policies during CPUFREQ_ADJUST,
> once the request comes from the underlying FW so there would be a fight
> who gets there first since cpu_cooling will change the policy in
> CPUFREQ_ADJUST notifier_chain and the driver would do the same thing.
> It seems to me that better implementation of the cpu_cooling notifer
> would be to keep the flag and change the policy in CPUFREQ_ADJUST only
> when the change was requested by cpu_cooling, and update the current
> state of cpufreq_cooling_device during CPUFREQ_NOTIFY event.
> What do you think?

I think the way cpu-cooling is written today, is an *ugly* hack. We hack
the notifier to change policy->max and no one is notified for it.

That's crap.

I would rather get some help from cpufreq core on that. Which can
provide some APIs to take care of thermal considerations.

Okay, I push that to my todo list. Will keep you all posted.

-- 
viresh

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

[Viresh Kumar]

On 01-08-15, 17:04, Viresh Kumar wrote:
> On 31-07-15, 08:30, Radivoje Jovanovic wrote:

> > I agree with you that this patch is trivial for the current
> > implementation since the notifier, as it is currently, will enforce
> > cpu_cooling policy change at every CPUFREQ_ADJUST which would cause
> > problems in our current implementation. In our implementation there is
> > a cpufreq driver that will also change policies during CPUFREQ_ADJUST,
> > once the request comes from the underlying FW so there would be a fight
> > who gets there first since cpu_cooling will change the policy in
> > CPUFREQ_ADJUST notifier_chain and the driver would do the same thing.

Okay, I had a detailed look this morning. cpufreq-notifier is designed
this way that policy->max can be updated by drivers.. So, that's fine.

Now coming to your problem. So, there are two users: fw and thermal,
which can affect policy->max. Now, both of them need to respect the
limits set by others and only decrease policy->max from the notifier
if it doesn't suit them.

I think it should work pretty well, unless you know you have triggered
a corner case somewhere, that I am not able to imagine.

Please let me know in case I am wrong.

-- 
viresh

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

[Radivoje Jovanovic]

On Mon, 3 Aug 2015 08:43:25 +0530
Viresh Kumar <viresh.kumar@linaro.org> wrote:

> On 01-08-15, 17:04, Viresh Kumar wrote:
> > On 31-07-15, 08:30, Radivoje Jovanovic wrote:
> 
> > > I agree with you that this patch is trivial for the current
> > > implementation since the notifier, as it is currently, will
> > > enforce cpu_cooling policy change at every CPUFREQ_ADJUST which
> > > would cause problems in our current implementation. In our
> > > implementation there is a cpufreq driver that will also change
> > > policies during CPUFREQ_ADJUST, once the request comes from the
> > > underlying FW so there would be a fight who gets there first
> > > since cpu_cooling will change the policy in CPUFREQ_ADJUST
> > > notifier_chain and the driver would do the same thing.
> 
> Okay, I had a detailed look this morning. cpufreq-notifier is designed
> this way that policy->max can be updated by drivers.. So, that's fine.
> 
> Now coming to your problem. So, there are two users: fw and thermal,
> which can affect policy->max. Now, both of them need to respect the
> limits set by others and only decrease policy->max from the notifier
> if it doesn't suit them.
> 
> I think it should work pretty well, unless you know you have triggered
> a corner case somewhere, that I am not able to imagine.
> 
> Please let me know in case I am wrong.
>
I will port the upstream driver to our platfrom, test for all corner
cases and update this thread once I have the data
Thank you for all the help