RE: [PATCH 1/3][v3] Thermal: initialize thermal zone device correctly

From: Zhang, Rui
Date: Tue Jan 12 2016 - 08:35:27 EST

Next message: Maxime Ripard: "[PATCH v2 0/2] regulator: Add support for parallel regulators"
Previous message: Lee Jones: "Re: [PATCH 1/3] ARM: STi: Add DT defines for co-processor reset lines"
In reply to: Chen, Yu C: "RE: [PATCH 1/3][v3] Thermal: initialize thermal zone device correctly"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

> -----Original Message-----
> From: Chen, Yu C
> Sent: Tuesday, January 12, 2016 2:42 PM
> To: Eduardo Valentin <edubezval@xxxxxxxxx>; Zhang, Rui
> <rui.zhang@xxxxxxxxx>
> Cc: javi.merino@xxxxxxx; linux-pm@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; stable@xxxxxxxxxxxxxxx
> Subject: RE: [PATCH 1/3][v3] Thermal: initialize thermal zone device correctly
> Importance: High
>
> Hi Eduardo,
> Thanks for your review and sorry for that I missed your email.
>
> > -----Original Message-----
> > From: linux-pm-owner@xxxxxxxxxxxxxxx [mailto:linux-pm-
> > owner@xxxxxxxxxxxxxxx] On Behalf Of Eduardo Valentin
> > Sent: Friday, January 01, 2016 2:44 AM
> > To: Chen, Yu C
> > Cc: Zhang, Rui; javi.merino@xxxxxxx; linux-pm@xxxxxxxxxxxxxxx; linux-
> > kernel@xxxxxxxxxxxxxxx; stable@xxxxxxxxxxxxxxx
> > Subject: Re: [PATCH 1/3][v3] Thermal: initialize thermal zone device
> > correctly
> >
> > For some reason, I thought Rui had picked this already.
> >
> > Anyways, here are a couple of comments:
> >
> > On Fri, Oct 30, 2015 at 04:31:47PM +0800, Chen Yu wrote:
> > > From: Zhang Rui <rui.zhang@xxxxxxxxx>
> > >
> > > After thermal zone device registered, as we have not read any
> > > temperature before, thus tz->temperature should not be 0, which
> > > actually means 0C, and thermal trend is not available.
> > > In this case, we need specially handling for the first
> > > thermal_zone_device_update().
> > >
> > > Both thermal core framework and step_wise governor is enhanced to
> > > handle this. And since the step_wise governor is the only one that
> > > uses trends, so it's the only thermal governor that needs to be
> > > updated.
> > >
> > > CC: <stable@xxxxxxxxxxxxxxx> #3.18+
> > > Tested-by: Manuel Krause <manuelkrause@xxxxxxxxxxxx>
> > > Tested-by: szegad <szegadlo@xxxxxxxxxxxxxx>
> > > Tested-by: prash <prash.n.rao@xxxxxxxxx>
> > > Tested-by: amish <ammdispose-arch@xxxxxxxxx>
> > > Tested-by: Matthias <morpheusxyz123@xxxxxxxx>
> > > Reviewed-by: Javi Merino <javi.merino@xxxxxxx>
> > > Signed-off-by: Zhang Rui <rui.zhang@xxxxxxxxx>
> > > Signed-off-by: Chen Yu <yu.c.chen@xxxxxxxxx>
> > > ---
> > > drivers/thermal/step_wise.c | 17 +++++++++++++++--
> > > drivers/thermal/thermal_core.c | 19 +++++++++++++++++--
> > > drivers/thermal/thermal_core.h | 1 +
> >
> > I would prefer if you could split this patch in two. One for thermal
> > core another one for step wise.
> [Yu] It would be better if we can split this patch into two, for stable material.
> What do you think, Rui?

[Zhang, Rui] No, first of all, we cannot take one patch for upstream and then split it into two for stable, second, I'd say I'm okay if the code is organized in two patches as Eduardo described, but at the same time, I don't think it's a problem if it's sent within one patch.

> >
> > > include/linux/thermal.h | 3 +++
> > > 4 files changed, 36 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/thermal/step_wise.c
> > > b/drivers/thermal/step_wise.c index 2f9f708..ea9366a 100644
> > > --- a/drivers/thermal/step_wise.c
> > > +++ b/drivers/thermal/step_wise.c
> > > @@ -63,6 +63,19 @@ static unsigned long get_target_state(struct
> > thermal_instance *instance,
> > > next_target = instance->target;
> > > dev_dbg(&cdev->device, "cur_state=%ld\n", cur_state);
> > >
> > > + if (!instance->initialized) {
> > > + if (throttle) {
> > > + next_target = (cur_state + 1) >= instance->upper ?
> > > + instance->upper :
> > > + ((cur_state + 1) < instance->lower ?
> > > + instance->lower : (cur_state + 1));
> > > + } else {
> > > + next_target = THERMAL_NO_TARGET;
> > > + }
> > > +
> > > + return next_target;
> > > + }
> > > +
> > > switch (trend) {
> > > case THERMAL_TREND_RAISING:
> > > if (throttle) {
> > > @@ -149,7 +162,7 @@ static void thermal_zone_trip_update(struct
> > thermal_zone_device *tz, int trip)
> > > dev_dbg(&instance->cdev->device, "old_target=%d,
> > target=%d\n",
> > > old_target, (int)instance->target);
> > >
> > > - if (old_target == instance->target)
> > > + if (instance->initialized && old_target == instance->target)
> > > continue;
> > >
> > > /* Activate a passive thermal instance */ @@ -161,7 +174,7
> > @@
> > > static void thermal_zone_trip_update(struct thermal_zone_device *tz,
> > > int
> > trip)
> > > instance->target == THERMAL_NO_TARGET)
> > > update_passive_instance(tz, trip_type, -1);
> > >
> > > -
> > > + instance->initialized = true;
> > > instance->cdev->updated = false; /* cdev needs update */
> > > }
> > >
> >
> > Considering that I understood the problem and your proposal well, I
> > would say these changes on step wise are the perfect case for setting
> > up a step_wise.bind_to_tz(). bind_to_tz() is already designed as an
> > opportunity for governor to check the thermal zone status at the time of
> binding.
> > Remember that moving to bind_to_tz() covers not only registration
> > time, but governor switching too (say, user chooses user_space, then
> step_wise).
>
> [Yu] The code change in step_wise. get_target_state is mainly for
> suspend/resume scenario, which is not involved with thermal
> zone/governor bindings IMO.

Agreed.

Thanks,
rui
> >
> > The above code seams to be correct, but after reviewing the code of
> > step_wise.throttle(), I would say it is already complicated and
> > deserves simplification, when possible.
> >
> >
> >
> > > diff --git a/drivers/thermal/thermal_core.c
> > > b/drivers/thermal/thermal_core.c index d9e525c..682bc1e 100644
> > > --- a/drivers/thermal/thermal_core.c
> > > +++ b/drivers/thermal/thermal_core.c
> > > @@ -532,8 +532,22 @@ static void update_temperature(struct
> > thermal_zone_device *tz)
> > > mutex_unlock(&tz->lock);
> > >
> > > trace_thermal_temperature(tz);
> > > - dev_dbg(&tz->device, "last_temperature=%d,
> > current_temperature=%d\n",
> > > - tz->last_temperature, tz->temperature);
> > > + if (tz->last_temperature == THERMAL_TEMP_INVALID)
> > > + dev_dbg(&tz->device, "last_temperature N/A,
> > current_temperature=%d\n",
> > > + tz->temperature);
> > > + else
> > > + dev_dbg(&tz->device, "last_temperature=%d,
> > current_temperature=%d\n",
> > > + tz->last_temperature, tz->temperature); }
> > > +
> > > +static void thermal_zone_device_reset(struct thermal_zone_device
> > > +*tz) {
> > > + struct thermal_instance *pos;
> > > +
> > > + tz->temperature = THERMAL_TEMP_INVALID;
> > > + tz->passive = 0;
> > > + list_for_each_entry(pos, &tz->thermal_instances, tz_node)
> > > + pos->initialized = false;
> > > }
> > >
> > > void thermal_zone_device_update(struct thermal_zone_device *tz)
> @@
> > > -1900,6 +1914,7 @@ struct thermal_zone_device
> > > *thermal_zone_device_register(const char *type,
> > >
> > > INIT_DELAYED_WORK(&(tz->poll_queue),
> > thermal_zone_device_check);
> > >
> > > + thermal_zone_device_reset(tz);
> > > thermal_zone_device_update(tz);
> > >
> > > return tz;
> > > diff --git a/drivers/thermal/thermal_core.h
> > > b/drivers/thermal/thermal_core.h index d7ac1fc..749d41a 100644
> > > --- a/drivers/thermal/thermal_core.h
> > > +++ b/drivers/thermal/thermal_core.h
> > > @@ -41,6 +41,7 @@ struct thermal_instance {
> > > struct thermal_zone_device *tz;
> > > struct thermal_cooling_device *cdev;
> > > int trip;
> > > + bool initialized;
> > > unsigned long upper; /* Highest cooling state for this trip point */
> > > unsigned long lower; /* Lowest cooling state for this trip point */
> > > unsigned long target; /* expected cooling state */
> > > diff --git a/include/linux/thermal.h b/include/linux/thermal.h index
> > > 157d366..5bcabc7 100644
> > > --- a/include/linux/thermal.h
> > > +++ b/include/linux/thermal.h
> > > @@ -43,6 +43,9 @@
> > > /* Default weight of a bound cooling device */ #define
> > > THERMAL_WEIGHT_DEFAULT 0
> > >
> > > +/* use value, which < 0K, to indicate an invalid/uninitialized
> > > +temperature
> > */
> > > +#define THERMAL_TEMP_INVALID -274000
> > > +
> > > /* Unit conversion macros */
> > > #define KELVIN_TO_CELSIUS(t) (long)(((long)t-2732 >= 0) ? \
> > > ((long)t-2732+5)/10 : ((long)t-2732-5)/10)
> > > --
> > > 1.8.4.2

Next message: Maxime Ripard: "[PATCH v2 0/2] regulator: Add support for parallel regulators"
Previous message: Lee Jones: "Re: [PATCH 1/3] ARM: STi: Add DT defines for co-processor reset lines"
In reply to: Chen, Yu C: "RE: [PATCH 1/3][v3] Thermal: initialize thermal zone device correctly"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]