| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <51A6C986.2000700@ti.com>
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