| 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: Tue, 24 Sep 2013 16:11:24 +0800
From: Hongbo Zhang <hongbo.zhang@...escale.com>
To: Mark Rutland <mark.rutland@....com>
CC: Eduardo Valentin <eduardo.valentin@...com>,
"swarren@...dotorg.org" <swarren@...dotorg.org>,
Pawel Moll <Pawel.Moll@....com>,
"ian.campbell@...rix.com" <ian.campbell@...rix.com>,
"rob.herring@...xeda.com" <rob.herring@...xeda.com>,
"linux@...ck-us.net" <linux@...ck-us.net>,
"rui.zhang@...el.com" <rui.zhang@...el.com>,
"wni@...dia.com" <wni@...dia.com>,
"joe@...ches.com" <joe@...ches.com>,
"grant.likely@...aro.org" <grant.likely@...aro.org>,
"durgadoss.r@...el.com" <durgadoss.r@...el.com>,
"lm-sensors@...sensors.org" <lm-sensors@...sensors.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>
Subject: Re: [PATCHv6 02/16] drivers: thermal: introduce device tree parser
On 09/23/2013 06:40 PM, Mark Rutland wrote:
> Hi Eduardo,
>
> Apologies for having taken so long to get back you on this.
>
> I have several comments on the binding and the way it's parsed.
>
> On Wed, Sep 18, 2013 at 10:35:36PM +0100, Eduardo Valentin wrote:
>> This patch introduces a device tree bindings for
>> describing the hardware thermal behavior and limits.
>> Also a parser to read and interpret the data and feed
>> it in the thermal framework is presented.
>>
>> This patch introduces a thermal data parser for device
>> tree. The parsed data is used to build thermal zones
>> and thermal binding parameters. The output data
>> can then be used to deploy thermal policies.
>>
>> This patch adds also documentation regarding this
>> API and how to define tree nodes to use
>> this infrastructure.
>>
>> Note that, in order to be able to have control
>> on the sensor registration on the DT thermal zone,
>> it was required to allow changing the thermal zone
>> .get_temp callback. For this reason, this patch
>> also removes the 'const' modifier from the .ops
>> field of thermal zone devices.
>>
>> Cc: Zhang Rui <rui.zhang@...el.com>
>> Cc: linux-pm@...r.kernel.org
>> Cc: linux-kernel@...r.kernel.org
>> Signed-off-by: Eduardo Valentin <eduardo.valentin@...com>
>> ---
>> .../devicetree/bindings/thermal/thermal.txt | 498 +++++++++++++
>> drivers/thermal/Kconfig | 13 +
>> drivers/thermal/Makefile | 1 +
>> drivers/thermal/of-thermal.c | 775 +++++++++++++++++++++
>> drivers/thermal/thermal_core.c | 9 +-
>> drivers/thermal/thermal_core.h | 9 +
>> include/dt-bindings/thermal/thermal.h | 27 +
>> include/linux/thermal.h | 28 +-
>> 8 files changed, 1357 insertions(+), 3 deletions(-)
>> create mode 100644 Documentation/devicetree/bindings/thermal/thermal.txt
>> create mode 100644 drivers/thermal/of-thermal.c
>> create mode 100644 include/dt-bindings/thermal/thermal.h
>>
>> ---
>>
>> Hello all,
>>
>> Thanks Joe Perches for the effort of reviewing this code, I really appreciate it.
>>
>> Here is a version with a refactored init function without leaks in the failure
>> patch. I also added a couple of comments to better understand the intention of
>> that function. Besides, when there are no memory available, the function
>> rolls back what ever thermal zones have been added.
>>
>> Thanks,
>>
>> Eduardo
>>
>> diff --git a/Documentation/devicetree/bindings/thermal/thermal.txt b/Documentation/devicetree/bindings/thermal/thermal.txt
>> new file mode 100644
>> index 0000000..6664533
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/thermal/thermal.txt
>> @@ -0,0 +1,498 @@
>> +* Thermal Framework Device Tree descriptor
>> +
>> +Generic binding to provide a way of defining hardware thermal
>> +structure using device tree. A thermal structure includes thermal
>> +zones and their components, such as trip points, polling intervals,
>> +sensors and cooling devices binding descriptors.
>> +
>> +The target of device tree thermal descriptors is to describe only
>> +the hardware thermal aspects, not how the system must control or which
>> +algorithm or policy must be taken in place.
>> +
>> +There are five types of nodes involved to describe thermal bindings:
>> +- sensors: used to describe the device source of temperature sensing;
>> +- cooling devices: used to describe devices source of power dissipation control;
>> +- trip points: used to describe points in temperature domain defined to
>> +make the system aware of hardware limits;
>> +- cooling attachments: used to describe links between trip points and
>> +cooling devices;
> I think "attachments" sounds a bit odd, though I don't have a better
> naming suggestion.
"map" or "mapping" is better?
>> +- thermal zones: used to describe thermal data within the hardware;
>> +
>> +It follows a description of each type of these device tree nodes.
>> +
>> +* Sensor devices
>> +
>> +Sensor devices are nodes providing temperature sensing capabilities on thermal
>> +zones. Typical devices are I2C ADC converters and bandgaps. Theses are nodes
>> +providing temperature data to thermal zones. Temperature sensor devices may
>> +control one or more internal sensors.
>> +
>> +Required property:
>> +- #sensor-cells: Used to provide sensor device specific information
>> + while referring to it. Must be at least 1, in order
>> + to identify uniquely the sensor instances within
>> + the IC. See thermal zone binding for more details
>> + on how consumers refer to sensor devices.
> I don't see why this needs to be at least one cell -- if an IC only has
> one temperature sensor it should be fine for this to be zero and have no
> ambiguity.
>
> It may make sense to call these thermal sensor devices, there are plenty
> of other sensors we might want to describe in the dt for other purposes,
> and we can impose some consistency if we remove the ambiguity.
>
>> +
>> +* Cooling device nodes
>> +
>> +Cooling devices are nodes providing control on power dissipation. There
>> +are essentially two ways to provide control on power dissipation. First
>> +is by means of regulating device performance, which is known as passive
>> +cooling. Second is by means of activating devices in order to remove
>> +the dissipated heat, which is known as active cooling, e.g. regulating
>> +fan speeds. In both cases, cooling devices shall have a way to determine
>> +the level of cooling.
>> +
>> +Required property:
>> +- cooling-min-level: A unsigned integer indicating the smallest
>> + cooling level accepted. Typically 0.
>> +- cooling-max-level: An unsigned integer indicating the largest
>> + cooling level accepted.
> I'm not sure what a "cooling level" means. It seems a bit abstract. Is
> this binding specific?
It means cooling ability of a cooling device, such as speed of a fan.
But it is better to use cooling-min/max-state I think, because the
thermal management code is using "cooling state", concepts in dt and c
should be identical.
> How does this relate to cooling-cells? These seem to be a fixed size.
I also think cooling-cells is redundant.
>> +- #cooling-cells: Used to provide cooling device specific information
>> + while referring to it. Must be at least 2, in order
>> + to specify minimum and maximum cooling level used
>> + in the reference. See Cooling device attachments section
>> + below for more details on how consumers refer to
>> + cooling devices.
> Are these cooling cells always expected to cover min and max, and are
> min and max always expected to take the same number of cells? The above
> cooling-*-level properties imply they each take up one cell.
>
> Does #cooling-cells = <3> make sense?. If this covers additional
> information (e.g. which fan on a multi-fan controller), how does the OS
> determine which cells are min and max, and how do these relate to
> cooling-level-min and cooling-level-max?
>
>> +
>> +* Trip points
>> +
>> +The trip node is a node to describe a point in the temperature domain
>> +in which the system takes an action. This node describes just the point,
>> +not the action.
> I'm still not sure on this, as it sounds like embedding policy into the
> DT, rather than a description of the hardware. I realise we need to
> prevent devices burning out, so describing the max acceptable
> temperature and possibly a preferred range makes sense, but do we need
> this level of flexibility? Maybe we do.
>
Putting platform data into dt is acceptable, right?
In a narrow sense, trip points are platform data, but in a broad sense,
both trip point itself and its corresponding cooling device are all
platform data.
>> +
>> +Required properties:
>> +- temperature: the trip temperature level, in milliCelsius.
> Preferably, s/milliCelsius/millicelsius/ over the document.
>
> Given that we have signed values elsewhere, is this signed or unsigned?
>
>> +- hysteresis: a (low) hysteresis value on 'temperature'. This is a
>> + relative value, in milliCelsius.
>> +- type: the trip type. Here is the type mapping:
>> + THERMAL_TRIP_ACTIVE 0: A trip point to enable active cooling
>> + THERMAL_TRIP_PASSIVE 1: A trip point to enable passive cooling
>> + THERMAL_TRIP_HOT 2: A trip point to notify emergency
>> + THERMAL_TRIP_CRITICAL 3: Hardware not reliable.
>> +
>> +Refer to include/dt-bindings/thermal/thermal.h for definition of these consts.
> Why not use a string for this? We do so for other type parameters in
> other bindings (see dr_mode in
> Documentation/devicetree/bindings/usb/generic.txt, phy-mode for ethernet
> PHYs, etc).
>
>> +
>> +* Cooling device attachments
>> +
>> +The cooling device attachments node is a node to describe how cooling devices
>> +get assigned to trip points of the zone. The cooling devices are expected
>> +to be loaded in the target system.
>> +
>> +Required properties:
>> +- cooling-device: A phandle of a cooling device with its parameters,
>> + referring to which cooling device is used in this
>> + binding. The required parameters are: the minimum
>> + cooling level and the maximum cooling level used
>> + in this attach.
> Might this be a list in general? The code doesn't have to support that
> yet.
>
> It would be nice to have a name for the cells after a phandle which
> describe the cooling device's configuration. You've called them
> parameters here, but it probably makes sense to call them a
> cooling-specifier (following clock-specifier and interrupt-specifier).
>
>> +- trip: A phandle of a trip point node within the same thermal
>> + zone.
>> +
>> +Optional property:
>> +- contribution: The cooling contribution to the thermal zone of the
>> + referred cooling device at the referred trip point.
>> + The contribution is a value from 0 to 100. The sum
>> + of all cooling contributions within a thermal zone
>> + must never exceed 100.
> This is a bit arbitrary. Couldn't we sum all contributions to find the
> total automatically, so this could be a simpler ratio?
>
>> +
>> +Note: Using the THERMAL_NO_LIMIT (-1L) constant in the cooling-device phandle
>> +limit parameters means:
>> +(i) - minimum level allowed for minimum cooling level used in the reference.
>> +(ii) - maximum level allowed for maximum cooling level used in the reference.
>> +Refer to include/dt-bindings/thermal/thermal.h for definition of this constant.
>> +
>> +* Thermal zones
>> +
>> +The thermal-zone node is the node containing all the required info
>> +for describing a thermal zone, including its cdev bindings. The thermal_zone
>> +node must contain, apart from its own properties, one node containing
>> +trip nodes and one node containing all the zone cooling attachments.
> s/cdev/cooling device/ ?
>
>> +
>> +Required properties:
>> +- passive-delay: The maximum number of milliseconds to wait between polls
>> + when performing passive cooling.
>> +- polling-delay: The maximum number of milliseconds to wait between polls
>> + when checking this thermal zone.
> How about polling-delay-passive, polling-delay-active? I'm still not
>
>> +- sensors: A list of sensor phandles and their parameters. The
>> + required parameter is the sensor id, in order to
>> + identify internal sensors when the sensor IC features
>> + several sensing units.
> As mentioned above, I'm not sure you even need that one cell.
>
> - sensors: A list of sensor phandle + thermal-sensor-specifier
> cells describing the sensors monitoring the thermal
> zone.
>
>> +- trips: A sub-node containing several trip point nodes required
>> + to describe the thermal zone.
>> +- cooling-attachments A sub-node containing several cooling device attaches
>> + nodes, used to describe the relation between trips
>> + and cooling devices.
>> +
>> +Optional property:
>> +- coefficients: An array of integers (one signed cell) containing
>> + coefficients to compose a linear relation between
>> + the sensors described in the sensors property.
>> + Coefficients defaults to 1, in case this property
>> + is not specified. A simple linear polynomial is used:
>> + Z = c0 * x0 + c1 + x1 + ... + c(n-1) * x(n-1) + cn.
>> +
>> + The coefficients are ordered and they match with sensors
>> + by means of sensor ID. Additional coefficients are
>> + interpreted as constant offsets.
> What is the end result of this? Presumably this is meant to result in an
> estimate of the average temperature of the thermal zone, rather than an
> arbitrary value? This should be mentioned.
>
> This doesn't seem to be used the in the code below...
>
>> +
>> +Note: The delay properties are bound to the maximum dT/dt (temperature
>> +derivative over time) in two situations for a thermal zone:
>> +(i) - when active cooling is activated (passive-delay); and
>> +(ii) - when the zone just needs to be monitored (polling-delay).
>> +The maximum dT/dt is highly bound to hardware power consumption and dissipation
>> +capability.
> I'm not sure what you mean by this, could you elaborate?
>
> I guess you mean that the delays should be chosen to account for said
> max dT/dt, such that a device can't unexpectedly cross several trip
> boundaries between polls?
>
>> +
>> +* Examples
>> +
>> +Below are several examples on how to use thermal data descriptors
>> +using device tree bindings:
>> +
>> +(a) - CPU thermal zone
>> +
>> +The CPU thermal zone example below describes how to setup one thermal zone
>> +using one single sensor as temperature source and many cooling devices and
>> +power dissipation control sources.
>> +
>> +#include <dt-bindings/thermal/thermal.h>
>> +
>> +cpus {
>> + cpu0: cpu@0 {
>> + ...
>> + cooling-min-level = <0>;
>> + cooling-max-level = <3>;
>> + #cooling-cells = <2>; /* min followed by max */
>> + };
> What do those min and max mean in this context? What do the values in
> the range [0,3] correspond to?
>
> I'm not sure it makes sense to describe passive cooling in this way --
> the precise way an OS does less work on a device is fundamentally tied
> to the design of the OS (it might be able to rate-limit requests, it
> might be able to clock the device down, it might only be able to turn
> the device off).
>
> I'm not sure there is anything we can describe for passive cooling
> (beyond the thermal limits the OS should attempt to stick to).
>
>> + ...
>> +};
>> +
>> +&i2c1 {
>> + ...
>> + fan0: fan@...8 {
>> + ...
>> + cooling-min-level = <0>;
>> + cooling-max-level = <9>;
>> + #cooling-cells = <2>; /* min followed by max */
>> + };
> What do min and max mean here for the fan?
>
It means the speed level of a fan. Even if a fan can run at a continuous
speed range, but when it is controlled under thermal management
framework, it can only run at discrete speeds.
>> +};
>> +
>> +bandgap0: bandgap@...000ED00 {
>> + ...
>> + #sensor-cells = <1>;
>> +};
>> +
>> +cpu-thermal: cpu-thermal {
> How do the thermal zones get probed?
>
>> + passive-delay = <250>; /* milliseconds */
>> + polling-delay = <1000>; /* milliseconds */
>> +
>> + /* sensor ID */
>> + sensors = <&bandgap0 0>;
>> +
>> + trips {
>> + cpu-alert0: cpu-alert {
>> + temperature = <90000>; /* milliCelsius */
>> + hysteresis = <2000>; /* milliCelsius */
>> + type = <THERMAL_TRIP_ACTIVE>;
>> + };
>> + cpu-alert1: cpu-alert {
>> + temperature = <100000>; /* milliCelsius */
>> + hysteresis = <2000>; /* milliCelsius */
>> + type = <THERMAL_TRIP_PASSIVE>;
>> + };
>> + cpu-crit: cpu-crit {
>> + temperature = <125000>; /* milliCelsius */
>> + hysteresis = <2000>; /* milliCelsius */
>> + type = <THERMAL_TRIP_CRITICAL>;
>> + };
>> + };
>> +
>> + cooling-attachments {
>> + attach0 {
>> + trip = <&cpu-alert0>;
>> + cooling-device = <&fan0 THERMAL_NO_LIMITS 4>;
>> + };
>> + attach1 {
>> + trip = <&cpu-alert1>;
>> + cooling-device = <&fan0 5 THERMAL_NO_LIMITS>;
>> + };
>> + attach2 {
>> + trip = <&cpu-alert1>;
>> + cooling-device =
>> + <&cpu0 THERMAL_NO_LIMITS THERMAL_NO_LIMITS>;
>> + };
>> + };
> Was there a good reason for splitting trips and attachment?
This is for adding/removing cooling device dynamically.
>> +};
>> +
>> +In the example above, the ADC sensor at address 0x0000ED00 is used to monitor
>> +the zone 'cpu-thermal' using its the sensor 0. The fan0, a fan device controlled
>> +via I2C bus 1, at adress 0x48, is used to remove the heat out of the thermal
>> +zone 'cpu-thermal' using its cooling levels from its minimum to 4, when it
>> +reaches trip point 'cpu-alert0' at 90C, as an example of active cooling. The
>> +same cooling device is used at 'cpu-alert1', but from 5 to its maximum level.
>> +The cpu@0 device is also linked to the same thermal zone, 'cpu-thermal', as a
>> +passive cooling device, using all its cooling levels at trip point 'cpu-alert1',
>> +which is a trip point at 100C.
>> +
>
--
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