Message-ID: <90989e59-f880-93df-7fbf-74c26fa8258f@arm.com>
Date:   Wed, 9 Dec 2020 10:41:44 +0000
From:   Lukasz Luba <lukasz.luba@....com>
To:     Daniel Lezcano <daniel.lezcano@...aro.org>
Cc:     rui.zhang@...el.com, Thara Gopinath <thara.gopinath@...aro.org>,
        Amit Kucheria <amitk@...nel.org>, linux-pm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] thermal/core: Emit a warning if the thermal zone is
 updated without ops



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.

Powered by blists - more mailing lists