Re: [PATCH v2 RESEND] thermal core: add option to run PM_POST_SUSPEND asynchronously

[Radu Solea <radusolea@google.com>]

On Thu, Dec 14, 2023 at 10:26 AM Radu Solea <radusolea@google.com> wrote:
>
> On Thu, Dec 14, 2023 at 12:25 AM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
> >
> > On 14/12/2023 01:21, Radu Solea wrote:
> >
> > [ ... ]
> >
> > >>> I can't go into much detail about the hardware. But let's put it this
> > >>> way, if thermal_zone_device_update() takes 5 ms for each device (read
> > >>> temp, get trips, set trips, etc). Assume 5 onboard thermal zones, on a
> > >>> good day, ignoring system churn you'd get to around 25ms (already
> > >>> significant).
> > >>> Now on top of that add that these devices have multiple functions,
> > >>> like a PMIC for example. The resume sequence is the perfect time frame
> > >>> where you'd encounter more than one operation aimed at any one of
> > >>> these devices. Unless you have uncommonly smart drivers and devices,
> > >>> these will be queued.
> > >>> The driver in most cases will spin (hardly ideal, but realistic), even
> > >>> if they would yield the effect on the completion of the chain is at
> > >>> least the same or, likely, worse.
> > >>>
> > >>> To the patch itself, I realized I've been somewhat hamfisted.
> > >>> thermal_zone_device_init() should not be deferred, and likely should
> > >>> execute for all zones before the in_suspend lock-out is released. I'll
> > >>> correct that once we've landed on something.
> > >>>
> > >>> To my 50ms, it's almost the worst-case, but it happens way more often
> > >>> than would be comfortable.
> > >>
> > >> If you call monitor_thermal_zone() instead of
> > >> thermal_zone_device_update(), does it speed up the resume time ?
> > >>
> > > Looks like it does, I'll rework the patch. Don't think the config
> > > switch is useful anymore.
> >
> > Well, we probably have to go a bit further in the concept.
> >
> > There are some thermal zones which may need to be checked immediately
> > and others no. That depends on the temperature speed behavior.
> >
> > The higher is the temperature speed for a device, the lower is the
> > polling (including zero).
> >
> > So the monitoring delay can be used as a criteria to check if the
> > thermal zone needs an update or postpone the monitoring.
> >
> > As we don't want to change the current behavior, we can add a global
> > option telling the polling delay above which we call
> > monitor_thermal_zone() and defaulting to zero.
> >
> > Does it make sense?
> It does, alternatively we add a device tree flag to the tz and not add
> a side effect to the delay.
> Either works imo.
>
On a second, more careful look, I don't think using polling-delay is
going to work as expected here. A 0 delay is used for interrupt-driven
systems. thermal_zone_device_set_polling() will cancel work if the
delay is 0. So it looks like we effectively end up without a resume
update for those devices. We could reuse the delay-based approach with
a resume-specific work item so it doesn't get cancelled. On the other
hand I suspect that the threshold which we would add for async will
never move from zero.
> >
> > --
> > <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
> >