Re: Trip points crossed not detected when no cooling device bound

Rafael J. Wysocki
2024-06-28T12:26:19Z

On Fri, Jun 28, 2024 at 12:49 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Fri, Jun 28, 2024 at 10:04 AM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
> >
> > On 27/06/2024 20:23, Rafael J. Wysocki wrote:
> > > On Thu, Jun 27, 2024 at 6:30 PM Daniel Lezcano
> > > <daniel.lezcano@linaro.org> wrote:
> > >>
> > >> On 27/06/2024 11:54, Rafael J. Wysocki wrote:
> > >>> On Thu, Jun 27, 2024 at 12:24 AM Daniel Lezcano
> > >>> <daniel.lezcano@linaro.org> wrote:
> > >>>>
> > >>>> On 26/06/2024 23:21, Daniel Lezcano wrote:
> > >>>>
> > >>>> [ ... ]
> > >>>>
> > >>>>>> Oh, I see where the problem can be. If the zone is polling only, it
> > >>>>>> will not rearm the timer when the current zone temperature is invalid
> > >>>>>> after the above commit, so does the attached patch help?
> > >>>>>
> > >>>>> At this point, I went far when bisecting another problem and I ended up
> > >>>>> screwing my config file. So I had to generate a new one from the default
> > >>>>> config. Since then the issue is no longer happening which sounds very
> > >>>>> strange to me.
> > >>>>>
> > >>>>> I'm still investigating but if you have a suggestion coming in mind, it
> > >>>>> would be welcome because I'm failing to find out what is going on ... :/
> > >>>>
> > >>>> I finally reproduced the issue. That happens when there is *no* cooling
> > >>>> device bound on *any* thermal zones.
> > >>>
> > >>> Interesting.
> > >>>
> > >>>> Your patch seems to fix the problem but I'm not sure to understand the
> > >>>> conditions of the bug
> > >>>
> > >>> It's probably the same as for commit 202aa0d4bb53:
> > >>> thermal_zone_device_init() sets tz->temperature to
> > >>> THERMAL_TEMP_INVALID and if the first invocation of
> > >>> __thermal_zone_get_temp() returns an error (because the .get_temp()
> > >>> callback returns an error), monitor_thermal_zone(). If polling is the
> > >>> only way in which the zone temperature can be updated, things go south
> > >>> because the timer is not set and there is no other way to set it. No
> > >>> updates will be coming
> > >>
> > >> If there is no polling delay (aka interrupt driven), the routine will
> > >> skip the _set_trips function and the monitor_thermal_zone() will do
> > >> nothing in this case, right ?
> > >
> > > _set_trips() looks at tz->temperature, however, and it doesn't make
> > > sense to call it if the latter is invalid.
> > >
> > > Same for handle_thermal_trip() and governor callbacks.
> > >
> > >> Even setting a label jump to "monitor:" the routine is broken AFAICT
> > >
> > > I beg to differ.
> > >
> > > Yes, monitor_thermal_zone() does nothing if there is no polling, but
> > > it needs to be called anyway because it checks whether or not polling
> > > is there in the first place.
> > >
> > > And if there is no polling, it is assumed that
> > > __thermal_zone_device_update() will be called by other means.
> >
> > AFAICT, the interrupt can fire and it will result in a
> > thermal_zone_device_update() but the interrupt must be setup by
> > __set_trips() which is skipped because of the invalid temperature.
>
> Say the zone temperature is invalid and __set_trips() is not skipped.
>
> You'll end up with nonsensical values of "low" and "high". Is this OK?
>
> If you rely on __set_trips() to set up the interrupts for you, it
> needs to be given a valid zone temperature and trip thresholds.
>
> Some special handling of invalid tz->temperature may be added to it, though.

Well, that's rather tricky because it is unclear what to do in this
case. If the zone temperature is unknown, the boundaries to pass to
the .set_trips() callbacks are unknown too.

One possible choice would be to make the interrupt trigger right away,
but that's equivalent to putting thermal_zone_device_update() on a
runqueue (with some extra overhead).

Another possible choice would be to set the lower boundary to the
possible minimum and the upper boundary to the temperature of the
first trip, but in that case tz->temperature may be equal to
THERMAL_TEMP_INVALID for a long time (basically until the first trip
is crossed on the way up) which seems a bit odd to me, so I'd rather
not do this either.

Instead, the new patch simply schedules another temperature check with
a delay if the zone temperature turns out to be invalid.

> > I've confirmed that with my evb board.
> >
> > - trips point
> > - no polling delay (interrupt based)
> > - no cooling device
> >
> > That does not work, there is no trip crossed notification.
>
> Yes, if the first zone temperature read fails and there are no
> interrupts coming to trigger next temperature updates, this is what's
> going to happen.
>
> I gather that the fix I sent doesn't help in this case?
>
> I'll send a v2 of it with .set_trips() taken into account.

Here: https://lore.kernel.org/linux-pm/2764814.mvXUDI8C0e@rjwysocki.net/