thermal/core: Emit a warning if the thermal zone is updated without ops

Message ID 20201207190530.30334-1-daniel.lezcano@linaro.org
State New
Headers show
Series
  • thermal/core: Emit a warning if the thermal zone is updated without ops
Related show

Commit Message

Daniel Lezcano Dec. 7, 2020, 7:05 p.m.
The actual code is silently ignoring a thermal zone update when a
driver is requesting it without a get_temp ops set.

That looks not correct, as the caller should not have called this
function if the thermal zone is unable to read the temperature.

That makes the code less robust as the check won't detect the driver
is inconsistently using the thermal API and that does not help to
improve the framework as these circumvolutions hide the problem at the
source.

In order to detect the situation when it happens, let's add a warning
when the update is requested without the get_temp() ops set.

Any warning emitted will have to be fixed at the source of the
problem: the caller must not call thermal_zone_device_update if there
is not get_temp callback set.

As the check is done in thermal_zone_get_temperature() via the
update_temperature() function, it is pointless to have the check and
the WARN in the thermal_zone_device_update() function. Just remove the
check and let the next call to raise the warning.

Cc: Thara Gopinath <thara.gopinath@linaro.org>
Cc: Amit Kucheria <amitk@kernel.org>
Cc: linux-pm@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

---
 drivers/thermal/thermal_core.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

-- 
2.17.1

Comments

Lukasz Luba Dec. 8, 2020, 9:36 a.m. | #1
Hi Daniel,

On 12/7/20 7:05 PM, Daniel Lezcano wrote:
> The actual code is silently ignoring a thermal zone update when a

> driver is requesting it without a get_temp ops set.

> 

> That looks not correct, as the caller should not have called this

> function if the thermal zone is unable to read the temperature.

> 

> That makes the code less robust as the check won't detect the driver

> is inconsistently using the thermal API and that does not help to

> improve the framework as these circumvolutions hide the problem at the

> source.


Make sense.

> 

> In order to detect the situation when it happens, let's add a warning

> when the update is requested without the get_temp() ops set.

> 

> Any warning emitted will have to be fixed at the source of the

> problem: the caller must not call thermal_zone_device_update if there

> is not get_temp callback set.

> 

> As the check is done in thermal_zone_get_temperature() via the

> update_temperature() function, it is pointless to have the check and

> the WARN in the thermal_zone_device_update() function. Just remove the

> check and let the next call to raise the warning.

> 

> Cc: Thara Gopinath <thara.gopinath@linaro.org>

> Cc: Amit Kucheria <amitk@kernel.org>

> Cc: linux-pm@vger.kernel.org

> Cc: linux-kernel@vger.kernel.org

> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

> ---

>   drivers/thermal/thermal_core.c | 16 ++++++++--------

>   1 file changed, 8 insertions(+), 8 deletions(-)

> 

> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c

> index 90e38cc199f4..1bd23ff2247b 100644

> --- a/drivers/thermal/thermal_core.c

> +++ b/drivers/thermal/thermal_core.c

> @@ -448,17 +448,17 @@ static void handle_thermal_trip(struct thermal_zone_device *tz, int trip)

>   	monitor_thermal_zone(tz);

>   }

>   

> -static void update_temperature(struct thermal_zone_device *tz)

> +static int update_temperature(struct thermal_zone_device *tz)

>   {

>   	int temp, ret;

>   

>   	ret = thermal_zone_get_temp(tz, &temp);

>   	if (ret) {

>   		if (ret != -EAGAIN)

> -			dev_warn(&tz->device,

> -				 "failed to read out thermal zone (%d)\n",

> -				 ret);

> -		return;

> +			dev_warn_once(&tz->device,

> +				      "failed to read out thermal zone (%d)\n",

> +				      ret);

> +		return ret;

>   	}

>   

>   	mutex_lock(&tz->lock);

> @@ -469,6 +469,8 @@ static void update_temperature(struct thermal_zone_device *tz)

>   	trace_thermal_temperature(tz);

>   

>   	thermal_genl_sampling_temp(tz->id, temp);

> +

> +	return 0;

>   }

>   

>   static void thermal_zone_device_init(struct thermal_zone_device *tz)

> @@ -553,11 +555,9 @@ void thermal_zone_device_update(struct thermal_zone_device *tz,

>   	if (atomic_read(&in_suspend))

>   		return;

>   

> -	if (!tz->ops->get_temp)

> +	if (update_temperature(tz))

>   		return;

>   

> -	update_temperature(tz);

> -


I think the patch does a bit more. Previously we continued running the
code below even when the thermal_zone_get_temp() returned an error (due
to various reasons). Now we stop and probably would not schedule next
polling, not calling:
handle_thermal_trip() and monitor_thermal_zone()

I would left update_temperature(tz) as it was and not check the return.
The function thermal_zone_get_temp() can protect itself from missing
tz->ops->get_temp(), so we should be safe.

What do you think?

Regards,
Lukasz
Daniel Lezcano Dec. 8, 2020, 1:51 p.m. | #2
Hi Lukasz,

On 08/12/2020 10:36, Lukasz Luba wrote:
> Hi Daniel,


[ ... ]

>>     static void thermal_zone_device_init(struct thermal_zone_device *tz)

>> @@ -553,11 +555,9 @@ void thermal_zone_device_update(struct

>> thermal_zone_device *tz,

>>       if (atomic_read(&in_suspend))

>>           return;

>>   -    if (!tz->ops->get_temp)

>> +    if (update_temperature(tz))

>>           return;

>>   -    update_temperature(tz);

>> -

> 

> I think the patch does a bit more. Previously we continued running the

> code below even when the thermal_zone_get_temp() returned an error (due

> to various reasons). Now we stop and probably would not schedule next

> polling, not calling:

> handle_thermal_trip() and monitor_thermal_zone()


I agree there is a change in the behavior.

> I would left update_temperature(tz) as it was and not check the return.

> The function thermal_zone_get_temp() can protect itself from missing

> tz->ops->get_temp(), so we should be safe.

> 

> What do you think?


Does it make sense to handle the trip point if we are unable to read the
temperature?

The lines following the update_temperature() are:

 - thermal_zone_set_trips() which needs a correct tz->temperature

 - handle_thermal_trip() which needs a correct tz->temperature to
compare with

 - monitor_thermal_zone() which needs a consistent tz->passive. This one
is updated by the governor which is in an inconsistent state because the
temperature is not updated.

The problem I see here is how the interrupt mode and the polling mode
are existing in the same code path.

The interrupt mode can call thermal_notify_framework() for critical/hot
trip points without being followed by a monitoring. But for the other
trip points, the get_temp is needed.

IMHO, we should return if update_temperature() is failing.

Perhaps, it would make sense to simply prevent to register a thermal
zone if the get_temp ops is not defined.

AFAICS, if the interrupt mode without get_temp callback are for hot and
critical trip points which can be directly invoked from the sensor via a
specified callback, no thermal zone would be needed in this case.



-- 
<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
Lukasz Luba Dec. 8, 2020, 2:37 p.m. | #3
On 12/8/20 1:51 PM, Daniel Lezcano wrote:
> 

> Hi Lukasz,

> 

> On 08/12/2020 10:36, Lukasz Luba wrote:

>> Hi Daniel,

> 

> [ ... ]

> 

>>>      static void thermal_zone_device_init(struct thermal_zone_device *tz)

>>> @@ -553,11 +555,9 @@ void thermal_zone_device_update(struct

>>> thermal_zone_device *tz,

>>>        if (atomic_read(&in_suspend))

>>>            return;

>>>    -    if (!tz->ops->get_temp)

>>> +    if (update_temperature(tz))

>>>            return;

>>>    -    update_temperature(tz);

>>> -

>>

>> I think the patch does a bit more. Previously we continued running the

>> code below even when the thermal_zone_get_temp() returned an error (due

>> to various reasons). Now we stop and probably would not schedule next

>> polling, not calling:

>> handle_thermal_trip() and monitor_thermal_zone()

> 

> I agree there is a change in the behavior.

> 

>> I would left update_temperature(tz) as it was and not check the return.

>> The function thermal_zone_get_temp() can protect itself from missing

>> tz->ops->get_temp(), so we should be safe.

>>

>> What do you think?

> 

> Does it make sense to handle the trip point if we are unable to read the

> temperature?

> 

> The lines following the update_temperature() are:

> 

>   - thermal_zone_set_trips() which needs a correct tz->temperature

> 

>   - handle_thermal_trip() which needs a correct tz->temperature to

> compare with

> 

>   - monitor_thermal_zone() which needs a consistent tz->passive. This one

> is updated by the governor which is in an inconsistent state because the

> temperature is not updated.

> 

> The problem I see here is how the interrupt mode and the polling mode

> are existing in the same code path.

> 

> The interrupt mode can call thermal_notify_framework() for critical/hot

> trip points without being followed by a monitoring. But for the other

> trip points, the get_temp is needed.


Yes, I agree that we can bail out when there is no .get_temp() callback
and even not schedule next polling in such case.
But I am just not sure if we can bail out and not schedule the next
polling, when there is .get_temp() populated and the driver returned
an error only at that moment, e.g. indicating some internal temporary,
issue like send queue full, so such as -EBUSY, or -EAGAIN, etc.
The thermal_zone_get_temp() would pass the error to update_temperature()
but we return, losing the next try. We would not check the temperature
again.

> 

> IMHO, we should return if update_temperature() is failing.

> 

> Perhaps, it would make sense to simply prevent to register a thermal

> zone if the get_temp ops is not defined.

> 

> AFAICS, if the interrupt mode without get_temp callback are for hot and

> critical trip points which can be directly invoked from the sensor via a

> specified callback, no thermal zone would be needed in this case.

> 

> 

>
Lukasz Luba Dec. 8, 2020, 2:56 p.m. | #4
On 12/8/20 2:37 PM, Lukasz Luba wrote:
> 

> 

> On 12/8/20 1:51 PM, Daniel Lezcano wrote:

>>

>> Hi Lukasz,

>>

>> On 08/12/2020 10:36, Lukasz Luba wrote:

>>> Hi Daniel,

>>

>> [ ... ]

>>

>>>>      static void thermal_zone_device_init(struct thermal_zone_device 

>>>> *tz)

>>>> @@ -553,11 +555,9 @@ void thermal_zone_device_update(struct

>>>> thermal_zone_device *tz,

>>>>        if (atomic_read(&in_suspend))

>>>>            return;

>>>>    -    if (!tz->ops->get_temp)

>>>> +    if (update_temperature(tz))

>>>>            return;

>>>>    -    update_temperature(tz);

>>>> -

>>>

>>> I think the patch does a bit more. Previously we continued running the

>>> code below even when the thermal_zone_get_temp() returned an error (due

>>> to various reasons). Now we stop and probably would not schedule next

>>> polling, not calling:

>>> handle_thermal_trip() and monitor_thermal_zone()

>>

>> I agree there is a change in the behavior.

>>

>>> I would left update_temperature(tz) as it was and not check the return.

>>> The function thermal_zone_get_temp() can protect itself from missing

>>> tz->ops->get_temp(), so we should be safe.

>>>

>>> What do you think?

>>

>> Does it make sense to handle the trip point if we are unable to read the

>> temperature?

>>

>> The lines following the update_temperature() are:

>>

>>   - thermal_zone_set_trips() which needs a correct tz->temperature

>>

>>   - handle_thermal_trip() which needs a correct tz->temperature to

>> compare with

>>

>>   - monitor_thermal_zone() which needs a consistent tz->passive. This one

>> is updated by the governor which is in an inconsistent state because the

>> temperature is not updated.

>>

>> The problem I see here is how the interrupt mode and the polling mode

>> are existing in the same code path.

>>

>> The interrupt mode can call thermal_notify_framework() for critical/hot

>> trip points without being followed by a monitoring. But for the other

>> trip points, the get_temp is needed.

> 

> Yes, I agree that we can bail out when there is no .get_temp() callback

> and even not schedule next polling in such case.

> But I am just not sure if we can bail out and not schedule the next

> polling, when there is .get_temp() populated and the driver returned

> an error only at that moment, e.g. indicating some internal temporary,

> issue like send queue full, so such as -EBUSY, or -EAGAIN, etc.

> The thermal_zone_get_temp() would pass the error to update_temperature()

> but we return, losing the next try. We would not check the temperature

> again.

> 


Some links to point you to such code where sensor has to deal
with protocol and various error reasons [1][2][3]


[1] 
https://elixir.bootlin.com/linux/latest/source/drivers/firmware/arm_scmi/sensors.c#L230
[2] 
https://elixir.bootlin.com/linux/latest/source/drivers/firmware/arm_scmi/driver.c#L425
[3] 
https://elixir.bootlin.com/linux/latest/source/drivers/firmware/arm_scmi/driver.c#L474
Daniel Lezcano Dec. 8, 2020, 3:19 p.m. | #5
On 08/12/2020 15:37, Lukasz Luba wrote:
> 

> 

> On 12/8/20 1:51 PM, Daniel Lezcano wrote:

>>

>> Hi Lukasz,

>>

>> On 08/12/2020 10:36, Lukasz Luba wrote:

>>> Hi Daniel,

>>

>> [ ... ]

>>

>>>>      static void thermal_zone_device_init(struct thermal_zone_device

>>>> *tz)

>>>> @@ -553,11 +555,9 @@ void thermal_zone_device_update(struct

>>>> thermal_zone_device *tz,

>>>>        if (atomic_read(&in_suspend))

>>>>            return;

>>>>    -    if (!tz->ops->get_temp)

>>>> +    if (update_temperature(tz))

>>>>            return;

>>>>    -    update_temperature(tz);

>>>> -

>>>

>>> I think the patch does a bit more. Previously we continued running the

>>> code below even when the thermal_zone_get_temp() returned an error (due

>>> to various reasons). Now we stop and probably would not schedule next

>>> polling, not calling:

>>> handle_thermal_trip() and monitor_thermal_zone()

>>

>> I agree there is a change in the behavior.

>>

>>> I would left update_temperature(tz) as it was and not check the return.

>>> The function thermal_zone_get_temp() can protect itself from missing

>>> tz->ops->get_temp(), so we should be safe.

>>>

>>> What do you think?

>>

>> Does it make sense to handle the trip point if we are unable to read the

>> temperature?

>>

>> The lines following the update_temperature() are:

>>

>>   - thermal_zone_set_trips() which needs a correct tz->temperature

>>

>>   - handle_thermal_trip() which needs a correct tz->temperature to

>> compare with

>>

>>   - monitor_thermal_zone() which needs a consistent tz->passive. This one

>> is updated by the governor which is in an inconsistent state because the

>> temperature is not updated.

>>

>> The problem I see here is how the interrupt mode and the polling mode

>> are existing in the same code path.

>>

>> The interrupt mode can call thermal_notify_framework() for critical/hot

>> trip points without being followed by a monitoring. But for the other

>> trip points, the get_temp is needed.

> 

> Yes, I agree that we can bail out when there is no .get_temp() callback

> and even not schedule next polling in such case.

> But I am just not sure if we can bail out and not schedule the next

> polling, when there is .get_temp() populated and the driver returned

> an error only at that moment, e.g. indicating some internal temporary,

> issue like send queue full, so such as -EBUSY, or -EAGAIN, etc.

> The thermal_zone_get_temp() would pass the error to update_temperature()

> but we return, losing the next try. We would not check the temperature

> again.


Hmm, right. I agree with your point.

What about the following changes:

 - Add the new APIs:

   thermal_zone_device_critical(struct thermal_zone_device *tz);
     => emergency poweroff

   thermal_zone_device_hot(struct thermal_zone_device *tz);
     => userspace notification

 - Add a big fat WARN when thermal_zone_device_update is called with
.get_temp == NULL because that must not happen.

If the .get_temp is NULL it is because we only have a HOT/CRITICAL
thermal trip points where we don't care about the temperature and
governor decision, right ?





-- 
<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
Lukasz Luba Dec. 9, 2020, 10:41 a.m. | #6
On 12/8/20 3:19 PM, Daniel Lezcano wrote:
> On 08/12/2020 15:37, Lukasz Luba wrote:

>>

>>

>> On 12/8/20 1:51 PM, Daniel Lezcano wrote:

>>>

>>> Hi Lukasz,

>>>

>>> On 08/12/2020 10:36, Lukasz Luba wrote:

>>>> Hi Daniel,

>>>

>>> [ ... ]

>>>

>>>>>       static void thermal_zone_device_init(struct thermal_zone_device

>>>>> *tz)

>>>>> @@ -553,11 +555,9 @@ void thermal_zone_device_update(struct

>>>>> thermal_zone_device *tz,

>>>>>         if (atomic_read(&in_suspend))

>>>>>             return;

>>>>>     -    if (!tz->ops->get_temp)

>>>>> +    if (update_temperature(tz))

>>>>>             return;

>>>>>     -    update_temperature(tz);

>>>>> -

>>>>

>>>> I think the patch does a bit more. Previously we continued running the

>>>> code below even when the thermal_zone_get_temp() returned an error (due

>>>> to various reasons). Now we stop and probably would not schedule next

>>>> polling, not calling:

>>>> handle_thermal_trip() and monitor_thermal_zone()

>>>

>>> I agree there is a change in the behavior.

>>>

>>>> I would left update_temperature(tz) as it was and not check the return.

>>>> The function thermal_zone_get_temp() can protect itself from missing

>>>> tz->ops->get_temp(), so we should be safe.

>>>>

>>>> What do you think?

>>>

>>> Does it make sense to handle the trip point if we are unable to read the

>>> temperature?

>>>

>>> The lines following the update_temperature() are:

>>>

>>>    - thermal_zone_set_trips() which needs a correct tz->temperature

>>>

>>>    - handle_thermal_trip() which needs a correct tz->temperature to

>>> compare with

>>>

>>>    - monitor_thermal_zone() which needs a consistent tz->passive. This one

>>> is updated by the governor which is in an inconsistent state because the

>>> temperature is not updated.

>>>

>>> The problem I see here is how the interrupt mode and the polling mode

>>> are existing in the same code path.

>>>

>>> The interrupt mode can call thermal_notify_framework() for critical/hot

>>> trip points without being followed by a monitoring. But for the other

>>> trip points, the get_temp is needed.

>>

>> Yes, I agree that we can bail out when there is no .get_temp() callback

>> and even not schedule next polling in such case.

>> But I am just not sure if we can bail out and not schedule the next

>> polling, when there is .get_temp() populated and the driver returned

>> an error only at that moment, e.g. indicating some internal temporary,

>> issue like send queue full, so such as -EBUSY, or -EAGAIN, etc.

>> The thermal_zone_get_temp() would pass the error to update_temperature()

>> but we return, losing the next try. We would not check the temperature

>> again.

> 

> Hmm, right. I agree with your point.

> 

> What about the following changes:

> 

>   - Add the new APIs:

> 

>     thermal_zone_device_critical(struct thermal_zone_device *tz);

>       => emergency poweroff

> 

>     thermal_zone_device_hot(struct thermal_zone_device *tz);

>       => userspace notification


They look promising, I have to look into the existing code.
When they would be called?

> 

>   - Add a big fat WARN when thermal_zone_device_update is called with

> .get_temp == NULL because that must not happen.


Good idea

> 

> If the .get_temp is NULL it is because we only have a HOT/CRITICAL

> thermal trip points where we don't care about the temperature and

> governor decision, right ?

> 


That is a good question. Let me dig into the code. I would say yes - we
don't have to hassle with governor in this circumstances.
Daniel Lezcano Dec. 9, 2020, 12:20 p.m. | #7
On 09/12/2020 11:41, Lukasz Luba wrote:
> 

> 

> On 12/8/20 3:19 PM, Daniel Lezcano wrote:

>> On 08/12/2020 15:37, Lukasz Luba wrote:

>>>

>>>

>>> On 12/8/20 1:51 PM, Daniel Lezcano wrote:

>>>>

>>>> Hi Lukasz,

>>>>

>>>> On 08/12/2020 10:36, Lukasz Luba wrote:

>>>>> Hi Daniel,

>>>>

>>>> [ ... ]

>>>>

>>>>>>       static void thermal_zone_device_init(struct thermal_zone_device

>>>>>> *tz)

>>>>>> @@ -553,11 +555,9 @@ void thermal_zone_device_update(struct

>>>>>> thermal_zone_device *tz,

>>>>>>         if (atomic_read(&in_suspend))

>>>>>>             return;

>>>>>>     -    if (!tz->ops->get_temp)

>>>>>> +    if (update_temperature(tz))

>>>>>>             return;

>>>>>>     -    update_temperature(tz);

>>>>>> -

>>>>>

>>>>> I think the patch does a bit more. Previously we continued running the

>>>>> code below even when the thermal_zone_get_temp() returned an error

>>>>> (due

>>>>> to various reasons). Now we stop and probably would not schedule next

>>>>> polling, not calling:

>>>>> handle_thermal_trip() and monitor_thermal_zone()

>>>>

>>>> I agree there is a change in the behavior.

>>>>

>>>>> I would left update_temperature(tz) as it was and not check the

>>>>> return.

>>>>> The function thermal_zone_get_temp() can protect itself from missing

>>>>> tz->ops->get_temp(), so we should be safe.

>>>>>

>>>>> What do you think?

>>>>

>>>> Does it make sense to handle the trip point if we are unable to read

>>>> the

>>>> temperature?

>>>>

>>>> The lines following the update_temperature() are:

>>>>

>>>>    - thermal_zone_set_trips() which needs a correct tz->temperature

>>>>

>>>>    - handle_thermal_trip() which needs a correct tz->temperature to

>>>> compare with

>>>>

>>>>    - monitor_thermal_zone() which needs a consistent tz->passive.

>>>> This one

>>>> is updated by the governor which is in an inconsistent state because

>>>> the

>>>> temperature is not updated.

>>>>

>>>> The problem I see here is how the interrupt mode and the polling mode

>>>> are existing in the same code path.

>>>>

>>>> The interrupt mode can call thermal_notify_framework() for critical/hot

>>>> trip points without being followed by a monitoring. But for the other

>>>> trip points, the get_temp is needed.

>>>

>>> Yes, I agree that we can bail out when there is no .get_temp() callback

>>> and even not schedule next polling in such case.

>>> But I am just not sure if we can bail out and not schedule the next

>>> polling, when there is .get_temp() populated and the driver returned

>>> an error only at that moment, e.g. indicating some internal temporary,

>>> issue like send queue full, so such as -EBUSY, or -EAGAIN, etc.

>>> The thermal_zone_get_temp() would pass the error to update_temperature()

>>> but we return, losing the next try. We would not check the temperature

>>> again.

>>

>> Hmm, right. I agree with your point.

>>

>> What about the following changes:

>>

>>   - Add the new APIs:

>>

>>     thermal_zone_device_critical(struct thermal_zone_device *tz);

>>       => emergency poweroff

>>

>>     thermal_zone_device_hot(struct thermal_zone_device *tz);

>>       => userspace notification

>

> They look promising, I have to look into the existing code.

> When they would be called?


They can be called directly by the driver when there is no get_temp
callback instead of calling thermal_zone_device_update, and by the usual
code path via handle_critical_trip function.

Also that can solve the issue [1] when registering a device which is
already too hot [1] by adding the ops in the thermal zone.

[1] https://lkml.org/lkml/2020/11/28/166

>>   - Add a big fat WARN when thermal_zone_device_update is called with

>> .get_temp == NULL because that must not happen.

> 

> Good idea

> 

>>

>> If the .get_temp is NULL it is because we only have a HOT/CRITICAL

>> thermal trip points where we don't care about the temperature and

>> governor decision, right ?

>>

> 

> That is a good question. Let me dig into the code. I would say yes - we

> don't have to hassle with governor in this circumstances.



-- 
<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
Lukasz Luba Dec. 9, 2020, 3:55 p.m. | #8
On 12/9/20 12:20 PM, Daniel Lezcano wrote:
> On 09/12/2020 11:41, Lukasz Luba wrote:

>>

>>

>> On 12/8/20 3:19 PM, Daniel Lezcano wrote:

>>> On 08/12/2020 15:37, Lukasz Luba wrote:

>>>>

>>>>

>>>> On 12/8/20 1:51 PM, Daniel Lezcano wrote:

>>>>>

>>>>> Hi Lukasz,

>>>>>

>>>>> On 08/12/2020 10:36, Lukasz Luba wrote:

>>>>>> Hi Daniel,

>>>>>

>>>>> [ ... ]

>>>>>

>>>>>>>        static void thermal_zone_device_init(struct thermal_zone_device

>>>>>>> *tz)

>>>>>>> @@ -553,11 +555,9 @@ void thermal_zone_device_update(struct

>>>>>>> thermal_zone_device *tz,

>>>>>>>          if (atomic_read(&in_suspend))

>>>>>>>              return;

>>>>>>>      -    if (!tz->ops->get_temp)

>>>>>>> +    if (update_temperature(tz))

>>>>>>>              return;

>>>>>>>      -    update_temperature(tz);

>>>>>>> -

>>>>>>

>>>>>> I think the patch does a bit more. Previously we continued running the

>>>>>> code below even when the thermal_zone_get_temp() returned an error

>>>>>> (due

>>>>>> to various reasons). Now we stop and probably would not schedule next

>>>>>> polling, not calling:

>>>>>> handle_thermal_trip() and monitor_thermal_zone()

>>>>>

>>>>> I agree there is a change in the behavior.

>>>>>

>>>>>> I would left update_temperature(tz) as it was and not check the

>>>>>> return.

>>>>>> The function thermal_zone_get_temp() can protect itself from missing

>>>>>> tz->ops->get_temp(), so we should be safe.

>>>>>>

>>>>>> What do you think?

>>>>>

>>>>> Does it make sense to handle the trip point if we are unable to read

>>>>> the

>>>>> temperature?

>>>>>

>>>>> The lines following the update_temperature() are:

>>>>>

>>>>>     - thermal_zone_set_trips() which needs a correct tz->temperature

>>>>>

>>>>>     - handle_thermal_trip() which needs a correct tz->temperature to

>>>>> compare with

>>>>>

>>>>>     - monitor_thermal_zone() which needs a consistent tz->passive.

>>>>> This one

>>>>> is updated by the governor which is in an inconsistent state because

>>>>> the

>>>>> temperature is not updated.

>>>>>

>>>>> The problem I see here is how the interrupt mode and the polling mode

>>>>> are existing in the same code path.

>>>>>

>>>>> The interrupt mode can call thermal_notify_framework() for critical/hot

>>>>> trip points without being followed by a monitoring. But for the other

>>>>> trip points, the get_temp is needed.

>>>>

>>>> Yes, I agree that we can bail out when there is no .get_temp() callback

>>>> and even not schedule next polling in such case.

>>>> But I am just not sure if we can bail out and not schedule the next

>>>> polling, when there is .get_temp() populated and the driver returned

>>>> an error only at that moment, e.g. indicating some internal temporary,

>>>> issue like send queue full, so such as -EBUSY, or -EAGAIN, etc.

>>>> The thermal_zone_get_temp() would pass the error to update_temperature()

>>>> but we return, losing the next try. We would not check the temperature

>>>> again.

>>>

>>> Hmm, right. I agree with your point.

>>>

>>> What about the following changes:

>>>

>>>    - Add the new APIs:

>>>

>>>      thermal_zone_device_critical(struct thermal_zone_device *tz);

>>>        => emergency poweroff

>>>

>>>      thermal_zone_device_hot(struct thermal_zone_device *tz);

>>>        => userspace notification

>>

>> They look promising, I have to look into the existing code.

>> When they would be called?

> 

> They can be called directly by the driver when there is no get_temp

> callback instead of calling thermal_zone_device_update, and by the usual

> code path via handle_critical_trip function.

> 

> Also that can solve the issue [1] when registering a device which is

> already too hot [1] by adding the ops in the thermal zone.

> 

> [1] https://lkml.org/lkml/2020/11/28/166

> 



Thank you for the link. I went through these discussions. Let me add
some comment below your posted RFC.

Patch

diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index 90e38cc199f4..1bd23ff2247b 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -448,17 +448,17 @@  static void handle_thermal_trip(struct thermal_zone_device *tz, int trip)
 	monitor_thermal_zone(tz);
 }
 
-static void update_temperature(struct thermal_zone_device *tz)
+static int update_temperature(struct thermal_zone_device *tz)
 {
 	int temp, ret;
 
 	ret = thermal_zone_get_temp(tz, &temp);
 	if (ret) {
 		if (ret != -EAGAIN)
-			dev_warn(&tz->device,
-				 "failed to read out thermal zone (%d)\n",
-				 ret);
-		return;
+			dev_warn_once(&tz->device,
+				      "failed to read out thermal zone (%d)\n",
+				      ret);
+		return ret;
 	}
 
 	mutex_lock(&tz->lock);
@@ -469,6 +469,8 @@  static void update_temperature(struct thermal_zone_device *tz)
 	trace_thermal_temperature(tz);
 
 	thermal_genl_sampling_temp(tz->id, temp);
+
+	return 0;
 }
 
 static void thermal_zone_device_init(struct thermal_zone_device *tz)
@@ -553,11 +555,9 @@  void thermal_zone_device_update(struct thermal_zone_device *tz,
 	if (atomic_read(&in_suspend))
 		return;
 
-	if (!tz->ops->get_temp)
+	if (update_temperature(tz))
 		return;
 
-	update_temperature(tz);
-
 	thermal_zone_set_trips(tz);
 
 	tz->notify_event = event;