lkml.org 
[lkml]   [2020]   [Dec]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH] thermal/core: Emit a warning if the thermal zone is updated without ops
    From
    Date
    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

    \
     
     \ /
      Last update: 2020-12-08 16:21    [W:6.097 / U:0.032 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site