Patchwork [RFT,1/1] thermal: step_wise: return instance->target by default

login
register
mail settings
Submitter Eduardo Valentin
Date May 29, 2013, 10:58 p.m.
Message ID <1369868285-18049-1-git-send-email-eduardo.valentin@ti.com>
Download mbox | patch
Permalink /patch/2633071/
State Accepted, archived
Delegated to: Zhang Rui
Headers show

Comments

Eduardo Valentin - May 29, 2013, 10:58 p.m.
In case the trend is not changing or when there is no
request for throttling, it is expected that the instance
would not change its requested target. This patch improves
the code implementation to cover for this expected behavior.

With current implementation, the instance will always
reset to cdev.cur_state, even in not expected cases,
like those mentioned above.

This patch changes the step_wise governor implementation
of get_target so that we accomplish:
(a) - default value will be current instance->target, so
we do not change the thermal instance target unnecessarily.
(b) - the code now it is clear about what is the intention.
There is a clear statement of what are the expected outcomes
(c) - removal of hardcoded constants, now it is put in use
the THERMAL_NO_TARGET macro.
(d) - variable names are also improved so that reader can
clearly understand the difference between instance cur target,
next target and cdev cur_state.

Cc: Zhang Rui <rui.zhang@intel.com>
Cc: Durgadoss R <durgadoss.r@intel.com>
Cc: linux-pm@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Reported-by: Ruslan Ruslichenko <ruslan.ruslichenko@ti.com>
Signed-of-by: Eduardo Valentin <eduardo.valentin@ti.com>
---
 drivers/thermal/step_wise.c | 29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)
---

Hello all,

I am requesting for tests on this patch. Based on an internal
discussion with Ruslan, I concluded that this code needs improvement.

Ruslan, I did not keep your original code because I believe the
get_target_state needs a better implementation for code readiness.
Besides, I also believe we are facing the bug of emul_temp in your case [1],
so this patch is not really fixing anything, but improving the
code quality and making sure the instance behaves as expected.
The fact you see the cooling device stuck at 1 is most probably because
the thermal core uses trend computed by the driver, not by emul_temp.

I have implemented a different improvement as you may find below. But
I kept a Reported-by under your name.

In any case, because I believe this change in step_wise is significant,
I am sending this patch for broader review and I kindly ask interested
audience for testing it.

[1] - https://patchwork.kernel.org/patch/2632831/
Zhang Rui - May 30, 2013, 1:42 a.m.
On Wed, 2013-05-29 at 18:58 -0400, Eduardo Valentin wrote:
> In case the trend is not changing or when there is no
> request for throttling, it is expected that the instance
> would not change its requested target. This patch improves
> the code implementation to cover for this expected behavior.
> 
right. agreed.

> With current implementation, the instance will always
> reset to cdev.cur_state, even in not expected cases,
> like those mentioned above.
> 
> This patch changes the step_wise governor implementation
> of get_target so that we accomplish:
> (a) - default value will be current instance->target, so
> we do not change the thermal instance target unnecessarily.

> (b) - the code now it is clear about what is the intention.
> There is a clear statement of what are the expected outcomes
> (c) - removal of hardcoded constants, now it is put in use
> the THERMAL_NO_TARGET macro.

> (d) - variable names are also improved so that reader can
> clearly understand the difference between instance cur target,
> next target and cdev cur_state.
> 
> Cc: Zhang Rui <rui.zhang@intel.com>
> Cc: Durgadoss R <durgadoss.r@intel.com>
> Cc: linux-pm@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Reported-by: Ruslan Ruslichenko <ruslan.ruslichenko@ti.com>
> Signed-of-by: Eduardo Valentin <eduardo.valentin@ti.com>
> ---
>  drivers/thermal/step_wise.c | 29 ++++++++++++++++++-----------
>  1 file changed, 18 insertions(+), 11 deletions(-)
> ---
> 
> Hello all,
> 
> I am requesting for tests on this patch. Based on an internal
> discussion with Ruslan, I concluded that this code needs improvement.
> 
> Ruslan, I did not keep your original code because I believe the
> get_target_state needs a better implementation for code readiness.
> Besides, I also believe we are facing the bug of emul_temp in your case [1],
> so this patch is not really fixing anything, but improving the
> code quality and making sure the instance behaves as expected.
> The fact you see the cooling device stuck at 1 is most probably because
> the thermal core uses trend computed by the driver, not by emul_temp.
> 
> I have implemented a different improvement as you may find below. But
> I kept a Reported-by under your name.
> 
it would be good to let me know what the problem is.
As I'm fixing a couple of thermal bugs recently.
Most of them are suspend/hibernate related, and I've been changing this
piece of code a lot.

thanks,
rui
> In any case, because I believe this change in step_wise is significant,
> I am sending this patch for broader review and I kindly ask interested
> audience for testing it.
> 
> [1] - https://patchwork.kernel.org/patch/2632831/
> 
> diff --git a/drivers/thermal/step_wise.c b/drivers/thermal/step_wise.c
> index 4d4ddae..769bfa3 100644
> --- a/drivers/thermal/step_wise.c
> +++ b/drivers/thermal/step_wise.c
> @@ -51,44 +51,51 @@ static unsigned long get_target_state(struct thermal_instance *instance,
>  {
>  	struct thermal_cooling_device *cdev = instance->cdev;
>  	unsigned long cur_state;
> +	unsigned long next_target;
>  
> +	/*
> +	 * We keep this instance the way it is by default.
> +	 * Otherwise, we use the current state of the
> +	 * cdev in use to determine the next_target.
> +	 */
>  	cdev->ops->get_cur_state(cdev, &cur_state);
> +	next_target = instance->target;
>  
>  	switch (trend) {
>  	case THERMAL_TREND_RAISING:
>  		if (throttle) {
> -			cur_state = cur_state < instance->upper ?
> +			next_target = cur_state < instance->upper ?
>  				    (cur_state + 1) : instance->upper;
> -			if (cur_state < instance->lower)
> -				cur_state = instance->lower;
> +			if (next_target < instance->lower)
> +				next_target = instance->lower;
>  		}
>  		break;
>  	case THERMAL_TREND_RAISE_FULL:
>  		if (throttle)
> -			cur_state = instance->upper;
> +			next_target = instance->upper;
>  		break;
>  	case THERMAL_TREND_DROPPING:
>  		if (cur_state == instance->lower) {
>  			if (!throttle)
> -				cur_state = -1;
> +				next_target = THERMAL_NO_TARGET;
>  		} else {
> -			cur_state -= 1;
> -			if (cur_state > instance->upper)
> -				cur_state = instance->upper;
> +			next_target = cur_state - 1;
> +			if (next_target > instance->upper)
> +				next_target = instance->upper;
>  		}
>  		break;
>  	case THERMAL_TREND_DROP_FULL:
>  		if (cur_state == instance->lower) {
>  			if (!throttle)
> -				cur_state = -1;
> +				next_target = THERMAL_NO_TARGET;
>  		} else
> -			cur_state = instance->lower;
> +			next_target = instance->lower;
>  		break;
>  	default:
>  		break;
>  	}
>  
> -	return cur_state;
> +	return next_target;
>  }
>  
>  static void update_passive_instance(struct thermal_zone_device *tz,


--
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
Eduardo Valentin - May 30, 2013, 3:37 a.m.
On 29-05-2013 21:42, Zhang Rui wrote:
> On Wed, 2013-05-29 at 18:58 -0400, Eduardo Valentin wrote:
>> In case the trend is not changing or when there is no
>> request for throttling, it is expected that the instance
>> would not change its requested target. This patch improves
>> the code implementation to cover for this expected behavior.
>>
> right. agreed.
> 
>> With current implementation, the instance will always
>> reset to cdev.cur_state, even in not expected cases,
>> like those mentioned above.
>>
>> This patch changes the step_wise governor implementation
>> of get_target so that we accomplish:
>> (a) - default value will be current instance->target, so
>> we do not change the thermal instance target unnecessarily.
> 
>> (b) - the code now it is clear about what is the intention.
>> There is a clear statement of what are the expected outcomes
>> (c) - removal of hardcoded constants, now it is put in use
>> the THERMAL_NO_TARGET macro.
> 
>> (d) - variable names are also improved so that reader can
>> clearly understand the difference between instance cur target,
>> next target and cdev cur_state.
>>
>> Cc: Zhang Rui <rui.zhang@intel.com>
>> Cc: Durgadoss R <durgadoss.r@intel.com>
>> Cc: linux-pm@vger.kernel.org
>> Cc: linux-kernel@vger.kernel.org
>> Reported-by: Ruslan Ruslichenko <ruslan.ruslichenko@ti.com>
>> Signed-of-by: Eduardo Valentin <eduardo.valentin@ti.com>
>> ---
>>  drivers/thermal/step_wise.c | 29 ++++++++++++++++++-----------
>>  1 file changed, 18 insertions(+), 11 deletions(-)
>> ---
>>
>> Hello all,
>>
>> I am requesting for tests on this patch. Based on an internal
>> discussion with Ruslan, I concluded that this code needs improvement.
>>
>> Ruslan, I did not keep your original code because I believe the
>> get_target_state needs a better implementation for code readiness.
>> Besides, I also believe we are facing the bug of emul_temp in your case [1],
>> so this patch is not really fixing anything, but improving the
>> code quality and making sure the instance behaves as expected.
>> The fact you see the cooling device stuck at 1 is most probably because
>> the thermal core uses trend computed by the driver, not by emul_temp.
>>
>> I have implemented a different improvement as you may find below. But
>> I kept a Reported-by under your name.
>>
> it would be good to let me know what the problem is.
> As I'm fixing a couple of thermal bugs recently.
> Most of them are suspend/hibernate related, and I've been changing this
> piece of code a lot.


Rui,

This specific patch does not address a bug per si. Just makes sure that
we avoid changing the target state of an instance when it is not
necessary to change it.

> 
> thanks,
> rui
>> In any case, because I believe this change in step_wise is significant,
>> I am sending this patch for broader review and I kindly ask interested
>> audience for testing it.
>>
>> [1] - https://patchwork.kernel.org/patch/2632831/

The patch above, on the other hand, does fix a bug.

>>
>> diff --git a/drivers/thermal/step_wise.c b/drivers/thermal/step_wise.c
>> index 4d4ddae..769bfa3 100644
>> --- a/drivers/thermal/step_wise.c
>> +++ b/drivers/thermal/step_wise.c
>> @@ -51,44 +51,51 @@ static unsigned long get_target_state(struct thermal_instance *instance,
>>  {
>>  	struct thermal_cooling_device *cdev = instance->cdev;
>>  	unsigned long cur_state;
>> +	unsigned long next_target;
>>  
>> +	/*
>> +	 * We keep this instance the way it is by default.
>> +	 * Otherwise, we use the current state of the
>> +	 * cdev in use to determine the next_target.
>> +	 */
>>  	cdev->ops->get_cur_state(cdev, &cur_state);
>> +	next_target = instance->target;
>>  
>>  	switch (trend) {
>>  	case THERMAL_TREND_RAISING:
>>  		if (throttle) {
>> -			cur_state = cur_state < instance->upper ?
>> +			next_target = cur_state < instance->upper ?
>>  				    (cur_state + 1) : instance->upper;
>> -			if (cur_state < instance->lower)
>> -				cur_state = instance->lower;
>> +			if (next_target < instance->lower)
>> +				next_target = instance->lower;
>>  		}
>>  		break;
>>  	case THERMAL_TREND_RAISE_FULL:
>>  		if (throttle)
>> -			cur_state = instance->upper;
>> +			next_target = instance->upper;
>>  		break;
>>  	case THERMAL_TREND_DROPPING:
>>  		if (cur_state == instance->lower) {
>>  			if (!throttle)
>> -				cur_state = -1;
>> +				next_target = THERMAL_NO_TARGET;
>>  		} else {
>> -			cur_state -= 1;
>> -			if (cur_state > instance->upper)
>> -				cur_state = instance->upper;
>> +			next_target = cur_state - 1;
>> +			if (next_target > instance->upper)
>> +				next_target = instance->upper;
>>  		}
>>  		break;
>>  	case THERMAL_TREND_DROP_FULL:
>>  		if (cur_state == instance->lower) {
>>  			if (!throttle)
>> -				cur_state = -1;
>> +				next_target = THERMAL_NO_TARGET;
>>  		} else
>> -			cur_state = instance->lower;
>> +			next_target = instance->lower;
>>  		break;
>>  	default:
>>  		break;
>>  	}
>>  
>> -	return cur_state;
>> +	return next_target;
>>  }
>>  
>>  static void update_passive_instance(struct thermal_zone_device *tz,
> 
> 
> 
>

Patch

diff --git a/drivers/thermal/step_wise.c b/drivers/thermal/step_wise.c
index 4d4ddae..769bfa3 100644
--- a/drivers/thermal/step_wise.c
+++ b/drivers/thermal/step_wise.c
@@ -51,44 +51,51 @@  static unsigned long get_target_state(struct thermal_instance *instance,
 {
 	struct thermal_cooling_device *cdev = instance->cdev;
 	unsigned long cur_state;
+	unsigned long next_target;
 
+	/*
+	 * We keep this instance the way it is by default.
+	 * Otherwise, we use the current state of the
+	 * cdev in use to determine the next_target.
+	 */
 	cdev->ops->get_cur_state(cdev, &cur_state);
+	next_target = instance->target;
 
 	switch (trend) {
 	case THERMAL_TREND_RAISING:
 		if (throttle) {
-			cur_state = cur_state < instance->upper ?
+			next_target = cur_state < instance->upper ?
 				    (cur_state + 1) : instance->upper;
-			if (cur_state < instance->lower)
-				cur_state = instance->lower;
+			if (next_target < instance->lower)
+				next_target = instance->lower;
 		}
 		break;
 	case THERMAL_TREND_RAISE_FULL:
 		if (throttle)
-			cur_state = instance->upper;
+			next_target = instance->upper;
 		break;
 	case THERMAL_TREND_DROPPING:
 		if (cur_state == instance->lower) {
 			if (!throttle)
-				cur_state = -1;
+				next_target = THERMAL_NO_TARGET;
 		} else {
-			cur_state -= 1;
-			if (cur_state > instance->upper)
-				cur_state = instance->upper;
+			next_target = cur_state - 1;
+			if (next_target > instance->upper)
+				next_target = instance->upper;
 		}
 		break;
 	case THERMAL_TREND_DROP_FULL:
 		if (cur_state == instance->lower) {
 			if (!throttle)
-				cur_state = -1;
+				next_target = THERMAL_NO_TARGET;
 		} else
-			cur_state = instance->lower;
+			next_target = instance->lower;
 		break;
 	default:
 		break;
 	}
 
-	return cur_state;
+	return next_target;
 }
 
 static void update_passive_instance(struct thermal_zone_device *tz,