Re: [PATCH v2 2/2] thermal: core: Add sanity check for polling_delay and passive_delay

[Rafael J. Wysocki]

On Mon, Jul 8, 2024 at 4:32 PM Daniel Lezcano <daniel.lezcano@xxxxxxxxxx> wrote:
>
> On 08/07/2024 16:03, Rafael J. Wysocki wrote:
> > On Mon, Jul 8, 2024 at 3:58 PM Daniel Lezcano <daniel.lezcano@xxxxxxxxxx> wrote:
> >>
> >> On 08/07/2024 15:38, Rafael J. Wysocki wrote:
> >>> On Mon, Jul 8, 2024 at 2:12 PM Daniel Lezcano <daniel.lezcano@xxxxxxxxxx> wrote:
> >>>>
> >>>> On 05/07/2024 21:46, Rafael J. Wysocki wrote:
> >>>>> From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> >>>>>
> >>>>> If polling_delay is nonzero and passive_delay is 0, the thermal zone
> >>>>> will use polling except when tz->passive is nonzero, which does not make
> >>>>> sense.
> >>>>>
> >>>>> Also if polling_delay is nonzero and passive_delay is greater than
> >>>>> polling_delay, the thermal zone temperature will be updated less often
> >>>>> when tz->passive is nonzero.  This does not make sense either.
> >>>>>
> >>>>> Ensure that none of the above will happen.
> >>>>>
> >>>>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> >>>>> ---
> >>>>>
> >>>>> v1 -> v2: The patch actually matches the changelog
> >>>>>
> >>>>> ---
> >>>>>     drivers/thermal/thermal_core.c |    3 +++
> >>>>>     1 file changed, 3 insertions(+)
> >>>>>
> >>>>> Index: linux-pm/drivers/thermal/thermal_core.c
> >>>>> ===================================================================
> >>>>> --- linux-pm.orig/drivers/thermal/thermal_core.c
> >>>>> +++ linux-pm/drivers/thermal/thermal_core.c
> >>>>> @@ -1440,6 +1440,9 @@ thermal_zone_device_register_with_trips(
> >>>>>                 td->threshold = INT_MAX;
> >>>>>         }
> >>>>>
> >>>>> +     if (polling_delay && (passive_delay > polling_delay || !passive_delay))
> >>>>> +             passive_delay = polling_delay;
> >>>>
> >>>> Given this is a system misconfiguration, it would make more sense to
> >>>> bail out with -EINVAL. Assigning a default value in the back of the
> >>>> caller will never raise its attention and can make a bad configuration
> >>>> staying for a long time.
> >>>
> >>> This works except for the case mentioned below.
> >>>
> >>> I think that passive_delay > polling_delay can trigger a -EINVAL, but
> >>> (polling_delay && !passive_delay) cannot do it because it is regarded
> >>> as a valid case as per the below.
> >>
> >> Right I can see ATM only this as an illogic combination:
> >>
> >>          polling_delay && passive_delay &&
> >>          (polling_delay < passive_delay)
> >>
> >>>> That said, there are configurations with a passive delay set to zero but
> >>>> with a non zero polling delay. For instance, a thermal zone mitigated
> >>>> with a fan, so active trip points are set. Another example is when there
> >>>> is only critical trip points for a thermal zone.
> >>>>
> >>>> Actually there are multiple combinations with delays value which may
> >>>> look invalid but which are actually valid.
> >>>>
> >>>> For example, a setup with polling_delay > 0, passive_delay = 0, active
> >>>> trip points, cooling map to this active trips, passive trip points
> >>>> without cooling map.
> >>>>
> >>>> IMHO, it is better to do the configuration the system is asking for,
> >>>> even if it sounds weird
> >>>
> >>> Except that it doesn't work as expected because if passive_delay = 0,
> >>> polling is paused when tz->passive is set.
> >>
> >> Yes, but as there is no cooling map, there is no governor action, thus
> >> tz->passive is never set.
> >
> > In current linux-next, it is set when a passive trip is crossed on the way up.
>
> Ah, I see. AFAIR that was the gov_step_wise which was changing this
> value but based on the thermal instance.
>
> >> So we can have a passive polling equal to zero
> >> without being illegal as no passive mitigation will happen.
> >>
> >> The passive delay is really there only if there is a passive cooling
> >> device mapped to a passive trip point.
> >
> > Well, shouldn't user space get notified more often when passive
> > cooling is under way?
>
> (Assuming you meant "user space get notified when a passive trip point
> is crossed")
>
> Mmh, yes. I see the point.
>
>
> >> The polling delay is in charge of mitigating the active cooling device
> >> like a fan. So it is possible to mix an active trip point to mitigate
> >> with a fan and then put at a higher temperature a passive trip point
> >> with a higher sampling resolution.
> >
> > But it is not correct to pause polling when tz->passive is set.
>
> I'm not sure to get the comment.
>
> Just to clarify:
>
> trip A is active with a multi speed fan, polling every 1s
>
> trip B is passive with a cpufreq cooling device, polling every 100ms
>
> temp(tripA) < temp(tripB)
>
> When the trip A is crossed, the mitigation happens at <polling> rate.
> Assuming it fails to cool down, the fan continues to increase its speed
> until it reaches its max state.
>
> The temperature continues to increase and crosses the passive trip
> point. The fan speed stays at its maximum and the polling switches to
> the passive polling delay.

Yes, but if the passive polling delay happens to be zero, it will stop
the polling entirely until tz->passive becomes zero again.

I don't believe that this is correct.