Date:	Wed, 29 May 2013 23:37:42 -0400
From:	Eduardo Valentin <eduardo.valentin@...com>
To:	Zhang Rui <rui.zhang@...el.com>
CC:	Eduardo Valentin <eduardo.valentin@...com>,
	<ruslan.ruslichenko@...com>, Durgadoss R <durgadoss.r@...el.com>,
	<linux-pm@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [RFT][PATCH 1/1] thermal: step_wise: return instance->target
 by default

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@...el.com>
>> Cc: Durgadoss R <durgadoss.r@...el.com>
>> Cc: linux-pm@...r.kernel.org
>> Cc: linux-kernel@...r.kernel.org
>> Reported-by: Ruslan Ruslichenko <ruslan.ruslichenko@...com>
>> Signed-of-by: Eduardo Valentin <eduardo.valentin@...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,
> 
> 
> 
> 


-- 
You have got to be excited about what you are doing. (L. Lamport)

Eduardo Valentin


Download attachment "signature.asc" of type "application/pgp-signature" (296 bytes)

Powered by blists - more mailing lists