Messages in this thread | | | Subject | Re: [RFT][PATCH 1/1] thermal: step_wise: return instance->target by default | From | Zhang Rui <> | Date | Thu, 30 May 2013 09:42:09 +0800 |
| |
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,
| |