lkml.org 
[lkml]   [2013]   [May]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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@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,
    >
    >
    >
    >


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

    Eduardo Valentin

    [unhandled content-type:application/pgp-signature]
    \
     
     \ /
      Last update: 2013-05-30 06:21    [W:3.265 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site