linux-kernel - Re: [PATCH] thermal/core: Emit a warning if the thermal zone is updated without ops

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Wed, 9 Dec 2020 15:55:33 +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/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.