| 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
| ||
|
Date: Wed, 4 Apr 2012 09:27:19 +0300
From: Eduardo Valentin <eduardo.valentin@...com>
To: Amit Kachhap <amit.kachhap@...aro.org>
Cc: eduardo.valentin@...com, "R, Durgadoss" <durgadoss.r@...el.com>,
"linaro-dev@...ts.linaro.org" <linaro-dev@...ts.linaro.org>,
"patches@...aro.org" <patches@...aro.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>,
"linux-pm@...ts.linux-foundation.org"
<linux-pm@...ts.linux-foundation.org>,
"rob.lee@...aro.org" <rob.lee@...aro.org>
Subject: Re: [linux-pm] [PATCH 1/4] thermal: Add a new trip type to use
cooling device instance number
Hello,
On Wed, Apr 04, 2012 at 09:53:15AM +0530, Amit Kachhap wrote:
> Hi Eduardo,
>
> On 3 April 2012 19:45, Eduardo Valentin <eduardo.valentin@...com> wrote:
> > Hello,
> >
> > On Thu, Feb 23, 2012 at 04:50:14PM +0530, Amit Kachhap wrote:
> >> On 23 February 2012 12:16, R, Durgadoss <durgadoss.r@...el.com> wrote:
> >> > Hi Amit,
> >> >
> >> >> -----Original Message-----
> >> >> From: amit kachhap [mailto:amitdanielk@...il.com] On Behalf Of Amit Daniel
> >> >> Kachhap
> >> >> Sent: Wednesday, February 22, 2012 3:44 PM
> >> >> To: linux-pm@...ts.linux-foundation.org
> >> >> Cc: linux-kernel@...r.kernel.org; mjg59@...f.ucam.org; linux-
> >> >> acpi@...r.kernel.org; lenb@...nel.org; linaro-dev@...ts.linaro.org;
> >> >> amit.kachhap@...aro.org; R, Durgadoss; rob.lee@...aro.org; patches@...aro.org
> >> >> Subject: [PATCH 1/4] thermal: Add a new trip type to use cooling device
> >> >> instance number
> >> >>
> >> >> This patch adds a new trip type THERMAL_TRIP_STATE_ACTIVE. This
> >> >> trip behaves same as THERMAL_TRIP_ACTIVE but also passes the cooling
> >> >> device instance number. This helps the cooling device registered as
> >> >> different instances to perform appropriate cooling action decision in
> >> >> the set_cur_state call back function.
> >> >>
> >> >> Also since the trip temperature's are in ascending order so some logic
> >> >> is put in place to skip the un-necessary checks.
> >> >>
> >> >> Signed-off-by: Amit Daniel Kachhap <amit.kachhap@...aro.org>
> >> >> ---
> >> >> Documentation/thermal/sysfs-api.txt | 4 +-
> >> >> drivers/thermal/thermal_sys.c | 45 ++++++++++++++++++++++++++++++++--
> >> >> include/linux/thermal.h | 1 +
> >> >> 3 files changed, 45 insertions(+), 5 deletions(-)
> >> >>
> >> >> diff --git a/Documentation/thermal/sysfs-api.txt b/Documentation/thermal/sysfs-
> >> >> api.txt
> >> >> index 1733ab9..7a0c080 100644
> >> >> --- a/Documentation/thermal/sysfs-api.txt
> >> >> +++ b/Documentation/thermal/sysfs-api.txt
> >> >> @@ -184,8 +184,8 @@ trip_point_[0-*]_temp
> >> >>
> >> >> trip_point_[0-*]_type
> >> >> Strings which indicate the type of the trip point.
> >> >> - E.g. it can be one of critical, hot, passive, active[0-*] for ACPI
> >> >> - thermal zone.
> >> >> + E.g. it can be one of critical, hot, passive, active[0-1],
> >> >> + state-active[0-*] for ACPI thermal zone.
> >> >> RO, Optional
> >> >>
> >> >> cdev[0-*]
> >> >> diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
> >> >> index 220ce7e..d4c9b20 100644
> >> >> --- a/drivers/thermal/thermal_sys.c
> >> >> +++ b/drivers/thermal/thermal_sys.c
> >> >> @@ -192,6 +192,8 @@ trip_point_type_show(struct device *dev, struct
> >> >> device_attribute *attr,
> >> >> return sprintf(buf, "passive\n");
> >> >> case THERMAL_TRIP_ACTIVE:
> >> >> return sprintf(buf, "active\n");
> >> >> + case THERMAL_TRIP_STATE_ACTIVE:
> >> >> + return sprintf(buf, "state-active\n");
> >> >> default:
> >> >> return sprintf(buf, "unknown\n");
> >> >> }
> >> >> @@ -1034,10 +1036,10 @@ EXPORT_SYMBOL(thermal_cooling_device_unregister);
> >> >>
> >> >> void thermal_zone_device_update(struct thermal_zone_device *tz)
> >> >> {
> >> >> - int count, ret = 0;
> >> >> - long temp, trip_temp;
> >> >> + int count, ret = 0, inst_id;
> >> >> + long temp, trip_temp, max_state, last_trip_change = 0;
> >> >> enum thermal_trip_type trip_type;
> >> >> - struct thermal_cooling_device_instance *instance;
> >> >> + struct thermal_cooling_device_instance *instance, *state_instance;
> >> >> struct thermal_cooling_device *cdev;
> >> >>
> >> >> mutex_lock(&tz->lock);
> >> >> @@ -1086,6 +1088,43 @@ void thermal_zone_device_update(struct
> >> >> thermal_zone_device *tz)
> >> >> cdev->ops->set_cur_state(cdev, 0);
> >> >> }
> >> >> break;
> >> >> + case THERMAL_TRIP_STATE_ACTIVE:
> >> >> + list_for_each_entry(instance, &tz->cooling_devices,
> >> >> + node) {
> >> >> + if (instance->trip != count)
> >> >> + continue;
> >> >> +
> >> >> + if (temp <= last_trip_change)
> >> >> + continue;
> >> >> +
> >> >> + inst_id = 0;
> >> >> + /*
> >> >> + *For this instance how many instance of same
> >> >> + *cooling device occured before
> >> >> + */
> >> >> +
> >> >> + list_for_each_entry(state_instance,
> >> >> + &tz->cooling_devices, node) {
> >> >> + if (instance->cdev ==
> >> >> + state_instance->cdev)
> >> >> + inst_id++;
> >> >> + if (state_instance->trip == count)
> >> >> + break;
> >> >> + }
> >> >
> >> > Can you explain a bit more on this loop and the set_cur_state below ?
> >> > Sorry, I don't get the logic behind this..
> >>
> >> This loop is basically finding the instance id of the same cooling device.
> >> Say we have done like this,
> >> thermal_zone_bind_cooling_device(thermal, 2, cdev);
> >> thermal_zone_bind_cooling_device(thermal, 3, cdev);
> >> thermal_zone_bind_cooling_device(thermal, 4, cdev);
> >>
> >> In above same cooling device cdev is binded to trip no 2,3 and 4 with
> >> inst_id generated as 1,2,3 respectively. so set_cur_state for those
> >> trip reached will be called as,
> >> set_cur_state(cdev, 1);
> >> set_cur_state(cdev, 2);
> >> set_cur_state(cdev, 3);
> >
> > In this case, why a simple state = get_cur_state() followed by a
> > set_cur_state(++state) / set_cur_state(--state) is not enough?
>
> Thanks for looking into the patch. Well actually what you are
> suggesting is exactly happening in PASSIVE trip types where the states
> are incremented or decremented based on thermal trend. On the contrary
> what this part of code is doing is to jump to a fixed state as and
> when a trip point is reached. The cooling effect of a frequency level
> is known beforehand and hence jumping into that is safe and also this
> does not cause performance degradation by going into a much lower
> frequency state just for temperature stablization.
Yeah, I see that you want to match an instance of the cooling device with
a cooling state for that cooling device.
>
> >
> > Another thing is if we want to do jumps in the sequence?
> >
> > set_cur_state(cdev, 1);
> > set_cur_state(cdev, 3);
> > set_cur_state(cdev, 6);
> >
> > But for that we need a table mapping, trip vs. state.
> >
> >
> > What do you think?
> In the current thermal_zone_device_update implementation all the
> checks are currently based on increase in temperature. So even though
> we want to go to set_cur_state(cdev, 6) we have to follow
> set_cur_state(cdev, 1), set_cur_state(cdev, 2) ,..... and finally
> set_cur_state(cdev, 6). So mapping table may not be needed as state
> transition is one by one. This a type of limitation but I think there
> can be some kind of modification in the way the
> thermal_zone_device_update calls all the lower temperature cooling
> devices instead of jumping directly to the final trip point cooling
> devices.
But, because you also force the thing to be increasing / decreasing 1 by 1,
I don't understand why you don't have a similar implementation of the
PASSIVE type. Just get its cur state and set the next state based on the curr.
How that would be different than what you are currently doing?
If you want to have a 1 to 1 relation between cooling device instance and cooling
device state, then you could do the jumps I mentioned, but only if you would
have the cooling state reference stored somewhere.
What bugs me is the fact that you have to be iterating in the cooling device list to determine
the next cooling state.
> >
> >>
> >> Thanks,
> >> Amit D
> >>
> >> >
> >> > Thanks,
> >> > Durga
> >> >
> >> >> +
> >> >> + cdev = instance->cdev;
> >> >> + cdev->ops->get_max_state(cdev, &max_state);
> >> >> +
> >> >> + if ((temp >= trip_temp) &&
> >> >> + (inst_id <= max_state))
> >> >> + cdev->ops->set_cur_state(cdev, inst_id);
> >> >> + else if ((temp < trip_temp) &&
> >> >> + (--inst_id <= max_state))
> >> >> + cdev->ops->set_cur_state(cdev, inst_id);
> >> >> +
> >> >> + last_trip_change = trip_temp;
> >> >> + }
> >> >> + break;
> >> >> case THERMAL_TRIP_PASSIVE:
> >> >> if (temp >= trip_temp || tz->passive)
> >> >> thermal_zone_device_passive(tz, temp,
> >> >> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> >> >> index 796f1ff..8df901f 100644
> >> >> --- a/include/linux/thermal.h
> >> >> +++ b/include/linux/thermal.h
> >> >> @@ -42,6 +42,7 @@ enum thermal_trip_type {
> >> >> THERMAL_TRIP_PASSIVE,
> >> >> THERMAL_TRIP_HOT,
> >> >> THERMAL_TRIP_CRITICAL,
> >> >> + THERMAL_TRIP_STATE_ACTIVE,
> >> >> };
> >> >>
> >> >> struct thermal_zone_device_ops {
> >> >> --
> >> >> 1.7.1
> >> >
> >> _______________________________________________
> >> linux-pm mailing list
> >> linux-pm@...ts.linux-foundation.org
> >> https://lists.linuxfoundation.org/mailman/listinfo/linux-pm
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists