From: Dan Carpenter <[email protected]>
Currently, if we cannot find the correct thermal zone then this error
path returns NULL and it would lead to an Oops in the caller. Return
ERR_PTR(-EINVAL) instead.
Fixes: 3bd52ac87347 ("thermal/of: Rework the thermal device tree initialization")
Signed-off-by: Dan Carpenter <[email protected]>
Link: https://lore.kernel.org/r/YvDzovkMCQecPDjz@kili
Signed-off-by: Daniel Lezcano <[email protected]>
---
drivers/thermal/thermal_of.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/thermal/thermal_of.c b/drivers/thermal/thermal_of.c
index c2bb5954b21e..368eb58e97cf 100644
--- a/drivers/thermal/thermal_of.c
+++ b/drivers/thermal/thermal_of.c
@@ -368,6 +368,7 @@ static struct device_node *of_thermal_zone_find(struct device_node *sensor, int
}
}
}
+ tz = ERR_PTR(-EINVAL);
out:
of_node_put(np);
return tz;
--
2.34.1
The previous version of the OF code was returning -ENODEV if no
thermal zones description was found or if the lookup of the sensor in
the thermal zones was not found.
The backend drivers are expecting this return value as an information
about skipping the sensor initialization and considered as normal.
Fix the return value by replacing -EINVAL by -ENODEV
Signed-off-by: Daniel Lezcano <[email protected]>
---
drivers/thermal/thermal_of.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/thermal/thermal_of.c b/drivers/thermal/thermal_of.c
index 368eb58e97cf..4210c18ef7b2 100644
--- a/drivers/thermal/thermal_of.c
+++ b/drivers/thermal/thermal_of.c
@@ -329,7 +329,7 @@ static struct device_node *of_thermal_zone_find(struct device_node *sensor, int
np = of_find_node_by_name(NULL, "thermal-zones");
if (!np) {
pr_err("Unable to find thermal zones description\n");
- return ERR_PTR(-EINVAL);
+ return ERR_PTR(-ENODEV);
}
/*
@@ -368,7 +368,7 @@ static struct device_node *of_thermal_zone_find(struct device_node *sensor, int
}
}
}
- tz = ERR_PTR(-EINVAL);
+ tz = ERR_PTR(-ENODEV);
out:
of_node_put(np);
return tz;
--
2.34.1
When the thermal zone description was converted to yaml schema, the
required 'trips' property was forgotten.
The initial text bindings was describing:
"
[ ... ]
* Thermal zone nodes
The thermal zone node is the node containing all the required info
for describing a thermal zone, including its cooling device bindings. The
thermal zone node must contain, apart from its own properties, one sub-node
containing trip nodes and one sub-node containing all the zone cooling maps.
Required properties:
- polling-delay: The maximum number of milliseconds to wait between polls
Type: unsigned when checking this thermal zone.
Size: one cell
- polling-delay-passive: The maximum number of milliseconds to wait
Type: unsigned between polls when performing passive cooling.
Size: one cell
- thermal-sensors: A list of thermal sensor phandles and sensor specifier
Type: list of used while monitoring the thermal zone.
phandles + sensor
specifier
- trips: A sub-node which is a container of only trip point nodes
Type: sub-node required to describe the thermal zone.
Optional property:
- cooling-maps: A sub-node which is a container of only cooling device
Type: sub-node map nodes, used to describe the relation between trips
and cooling devices.
[ ... ]
"
Now the schema describes:
"
[ ... ]
required:
- polling-delay
- polling-delay-passive
- thermal-sensors
[ ... ]
"
Add the missing 'trips' property in the required properties.
Fixed: 1202a442a31fd ("dt-bindings: thermal: Add yaml bindings for thermal zones")
Signed-off-by: Daniel Lezcano <[email protected]>
---
Documentation/devicetree/bindings/thermal/thermal-zones.yaml | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/devicetree/bindings/thermal/thermal-zones.yaml b/Documentation/devicetree/bindings/thermal/thermal-zones.yaml
index 2d34f3ccb257..8d2c6d74b605 100644
--- a/Documentation/devicetree/bindings/thermal/thermal-zones.yaml
+++ b/Documentation/devicetree/bindings/thermal/thermal-zones.yaml
@@ -214,6 +214,7 @@ patternProperties:
- polling-delay
- polling-delay-passive
- thermal-sensors
+ - trips
additionalProperties: false
--
2.34.1
On 8/8/22 11:09, Daniel Lezcano wrote:
> The previous version of the OF code was returning -ENODEV if no
> thermal zones description was found or if the lookup of the sensor in
> the thermal zones was not found.
>
> The backend drivers are expecting this return value as an information
> about skipping the sensor initialization and considered as normal.
>
> Fix the return value by replacing -EINVAL by -ENODEV
>
> Signed-off-by: Daniel Lezcano <[email protected]>
> ---
> drivers/thermal/thermal_of.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/thermal/thermal_of.c b/drivers/thermal/thermal_of.c
> index 368eb58e97cf..4210c18ef7b2 100644
> --- a/drivers/thermal/thermal_of.c
> +++ b/drivers/thermal/thermal_of.c
> @@ -329,7 +329,7 @@ static struct device_node *of_thermal_zone_find(struct device_node *sensor, int
> np = of_find_node_by_name(NULL, "thermal-zones");
> if (!np) {
> pr_err("Unable to find thermal zones description\n");
I really don't like that error message: People will see it (and complain)
whenever a sensor registers and there is no thermal zone, even though that
is perfectly normal (see description above).
Guenter
> - return ERR_PTR(-EINVAL);
> + return ERR_PTR(-ENODEV);
> }
>
> /*
> @@ -368,7 +368,7 @@ static struct device_node *of_thermal_zone_find(struct device_node *sensor, int
> }
> }
> }
> - tz = ERR_PTR(-EINVAL);
> + tz = ERR_PTR(-ENODEV);
> out:
> of_node_put(np);
> return tz;
Am 2022-08-08 20:35, schrieb Guenter Roeck:
> On 8/8/22 11:09, Daniel Lezcano wrote:
>> The previous version of the OF code was returning -ENODEV if no
>> thermal zones description was found or if the lookup of the sensor in
>> the thermal zones was not found.
>>
>> The backend drivers are expecting this return value as an information
>> about skipping the sensor initialization and considered as normal.
>>
>> Fix the return value by replacing -EINVAL by -ENODEV
>>
>> Signed-off-by: Daniel Lezcano <[email protected]>
>> ---
>> drivers/thermal/thermal_of.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/thermal/thermal_of.c
>> b/drivers/thermal/thermal_of.c
>> index 368eb58e97cf..4210c18ef7b2 100644
>> --- a/drivers/thermal/thermal_of.c
>> +++ b/drivers/thermal/thermal_of.c
>> @@ -329,7 +329,7 @@ static struct device_node
>> *of_thermal_zone_find(struct device_node *sensor, int
>> np = of_find_node_by_name(NULL, "thermal-zones");
>> if (!np) {
>> pr_err("Unable to find thermal zones description\n");
>
> I really don't like that error message: People will see it (and
> complain)
> whenever a sensor registers and there is no thermal zone, even though
> that
> is perfectly normal (see description above).
I can second that, and there actually two error messages:
[ 6.156983] thermal_sys: Unable to find thermal zones description
[ 6.163125] thermal_sys: Failed to find thermal zone for hwmon id=0
On the sl28 board with the qoriq_thermal driver:
[ 1.917940] thermal_sys: Failed to find thermal zone for tmu id=2
[ 1.929231] thermal_sys: Failed to find thermal zone for tmu id=3
[ 1.940519] thermal_sys: Failed to find thermal zone for tmu id=4
[ 1.951814] thermal_sys: Failed to find thermal zone for tmu id=5
[ 1.963109] thermal_sys: Failed to find thermal zone for tmu id=6
[ 1.974399] thermal_sys: Failed to find thermal zone for tmu id=7
[ 1.985690] thermal_sys: Failed to find thermal zone for tmu id=8
[ 1.996980] thermal_sys: Failed to find thermal zone for tmu id=9
[ 2.008274] thermal_sys: Failed to find thermal zone for tmu id=10
[ 2.019656] thermal_sys: Failed to find thermal zone for tmu id=11
[ 2.031037] thermal_sys: Failed to find thermal zone for tmu id=12
[ 2.048942] thermal_sys: Failed to find thermal zone for tmu id=13
[ 2.060320] thermal_sys: Failed to find thermal zone for tmu id=14
[ 2.071700] thermal_sys: Failed to find thermal zone for tmu id=15
Btw. the driver seems to always register 16 sensors regardless how
many the actual hardware has (or rather: are described in the DT).
-michael
Am 2022-08-08 20:09, schrieb Daniel Lezcano:
> The previous version of the OF code was returning -ENODEV if no
> thermal zones description was found or if the lookup of the sensor in
> the thermal zones was not found.
>
> The backend drivers are expecting this return value as an information
> about skipping the sensor initialization and considered as normal.
>
> Fix the return value by replacing -EINVAL by -ENODEV
>
> Signed-off-by: Daniel Lezcano <[email protected]>
Tested-by: Michael Walle <[email protected]>
On 08/08/2022 20:35, Guenter Roeck wrote:
> On 8/8/22 11:09, Daniel Lezcano wrote:
>> The previous version of the OF code was returning -ENODEV if no
>> thermal zones description was found or if the lookup of the sensor in
>> the thermal zones was not found.
>>
>> The backend drivers are expecting this return value as an information
>> about skipping the sensor initialization and considered as normal.
>>
>> Fix the return value by replacing -EINVAL by -ENODEV
>>
>> Signed-off-by: Daniel Lezcano <[email protected]>
>> ---
>> drivers/thermal/thermal_of.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/thermal/thermal_of.c b/drivers/thermal/thermal_of.c
>> index 368eb58e97cf..4210c18ef7b2 100644
>> --- a/drivers/thermal/thermal_of.c
>> +++ b/drivers/thermal/thermal_of.c
>> @@ -329,7 +329,7 @@ static struct device_node
>> *of_thermal_zone_find(struct device_node *sensor, int
>> np = of_find_node_by_name(NULL, "thermal-zones");
>> if (!np) {
>> pr_err("Unable to find thermal zones description\n");
>
> I really don't like that error message: People will see it (and complain)
> whenever a sensor registers and there is no thermal zone, even though that
> is perfectly normal (see description above).
>
I agree, I'll change the message to:
pr_info("No thermal zones description\n");
sounds good ?
>> - return ERR_PTR(-EINVAL);
>> + return ERR_PTR(-ENODEV);
>> }
>> /*
>> @@ -368,7 +368,7 @@ static struct device_node
>> *of_thermal_zone_find(struct device_node *sensor, int
>> }
>> }
>> }
>> - tz = ERR_PTR(-EINVAL);
>> + tz = ERR_PTR(-ENODEV);
>> out:
>> of_node_put(np);
>> return tz;
>
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
On 08/08/2022 21:05, Michael Walle wrote:
> Am 2022-08-08 20:35, schrieb Guenter Roeck:
>> On 8/8/22 11:09, Daniel Lezcano wrote:
>>> The previous version of the OF code was returning -ENODEV if no
>>> thermal zones description was found or if the lookup of the sensor in
>>> the thermal zones was not found.
>>>
>>> The backend drivers are expecting this return value as an information
>>> about skipping the sensor initialization and considered as normal.
>>>
>>> Fix the return value by replacing -EINVAL by -ENODEV
>>>
>>> Signed-off-by: Daniel Lezcano <[email protected]>
>>> ---
>>> drivers/thermal/thermal_of.c | 4 ++--
>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/thermal/thermal_of.c b/drivers/thermal/thermal_of.c
>>> index 368eb58e97cf..4210c18ef7b2 100644
>>> --- a/drivers/thermal/thermal_of.c
>>> +++ b/drivers/thermal/thermal_of.c
>>> @@ -329,7 +329,7 @@ static struct device_node
>>> *of_thermal_zone_find(struct device_node *sensor, int
>>> np = of_find_node_by_name(NULL, "thermal-zones");
>>> if (!np) {
>>> pr_err("Unable to find thermal zones description\n");
>>
>> I really don't like that error message: People will see it (and complain)
>> whenever a sensor registers and there is no thermal zone, even though
>> that
>> is perfectly normal (see description above).
>
> I can second that, and there actually two error messages:
>
> [ 6.156983] thermal_sys: Unable to find thermal zones description
> [ 6.163125] thermal_sys: Failed to find thermal zone for hwmon id=0
Yeah, I can check:
np = of_thermal_zone_find(sensor, id);
And print the error if PTR_ERR(np) != ENODEV, otherwise silently return.
> On the sl28 board with the qoriq_thermal driver:
> [ 1.917940] thermal_sys: Failed to find thermal zone for tmu id=2
> [ 1.929231] thermal_sys: Failed to find thermal zone for tmu id=3
> [ 1.940519] thermal_sys: Failed to find thermal zone for tmu id=4
> [ 1.951814] thermal_sys: Failed to find thermal zone for tmu id=5
> [ 1.963109] thermal_sys: Failed to find thermal zone for tmu id=6
> [ 1.974399] thermal_sys: Failed to find thermal zone for tmu id=7
> [ 1.985690] thermal_sys: Failed to find thermal zone for tmu id=8
> [ 1.996980] thermal_sys: Failed to find thermal zone for tmu id=9
> [ 2.008274] thermal_sys: Failed to find thermal zone for tmu id=10
> [ 2.019656] thermal_sys: Failed to find thermal zone for tmu id=11
> [ 2.031037] thermal_sys: Failed to find thermal zone for tmu id=12
> [ 2.048942] thermal_sys: Failed to find thermal zone for tmu id=13
> [ 2.060320] thermal_sys: Failed to find thermal zone for tmu id=14
> [ 2.071700] thermal_sys: Failed to find thermal zone for tmu id=15
>
> Btw. the driver seems to always register 16 sensors regardless how
> many the actual hardware has (or rather: are described in the DT).
Yes, it may be nicer to rely on the compatible string to figure out the
sensors of the platform and call with full knowledge the registering
function. But anyway ...
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
On 8/8/22 13:31, Daniel Lezcano wrote:
> On 08/08/2022 20:35, Guenter Roeck wrote:
>> On 8/8/22 11:09, Daniel Lezcano wrote:
>>> The previous version of the OF code was returning -ENODEV if no
>>> thermal zones description was found or if the lookup of the sensor in
>>> the thermal zones was not found.
>>>
>>> The backend drivers are expecting this return value as an information
>>> about skipping the sensor initialization and considered as normal.
>>>
>>> Fix the return value by replacing -EINVAL by -ENODEV
>>>
>>> Signed-off-by: Daniel Lezcano <[email protected]>
>>> ---
>>> drivers/thermal/thermal_of.c | 4 ++--
>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/thermal/thermal_of.c b/drivers/thermal/thermal_of.c
>>> index 368eb58e97cf..4210c18ef7b2 100644
>>> --- a/drivers/thermal/thermal_of.c
>>> +++ b/drivers/thermal/thermal_of.c
>>> @@ -329,7 +329,7 @@ static struct device_node *of_thermal_zone_find(struct device_node *sensor, int
>>> np = of_find_node_by_name(NULL, "thermal-zones");
>>> if (!np) {
>>> pr_err("Unable to find thermal zones description\n");
>>
>> I really don't like that error message: People will see it (and complain)
>> whenever a sensor registers and there is no thermal zone, even though that
>> is perfectly normal (see description above).
>>
>
> I agree, I'll change the message to:
>
> pr_info("No thermal zones description\n");
>
> sounds good ?
>
I think it should be silent or at best pr_debug.
Guenter
>>> - return ERR_PTR(-EINVAL);
>>> + return ERR_PTR(-ENODEV);
>>> }
>>> /*
>>> @@ -368,7 +368,7 @@ static struct device_node *of_thermal_zone_find(struct device_node *sensor, int
>>> }
>>> }
>>> }
>>> - tz = ERR_PTR(-EINVAL);
>>> + tz = ERR_PTR(-ENODEV);
>>> out:
>>> of_node_put(np);
>>> return tz;
>>
>
>