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.zh...@intel.com> >> Cc: Durgadoss R <durgados...@intel.com> >> Cc: linux...@vger.kernel.org >> Cc: linux-kernel@vger.kernel.org >> Reported-by: Ruslan Ruslichenko <ruslan.rusliche...@ti.com> >> Signed-of-by: Eduardo Valentin <eduardo.valen...@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
signature.asc
Description: OpenPGP digital signature
