[PATCH v2 0/2] thermal: core: Sanitize polling delay values

[PATCH v2 0/2] thermal: core: Sanitize polling delay values

[Rafael J. Wysocki]

Hi Everyone,

This is a v2 of

https://lore.kernel.org/linux-pm/4585590.LvFx2qVVIh@rjwysocki.net/

which is being sent because the second patch in the v2 was incorrect.

These 2 patches deal with issues that may arise if the polling delay value used
for passive polling is at odds with the polling delay values used otherwise.

The first patch simply eliminates the need to worry about possible negative
polling delay values by changing the data type used for representing them
to unsigend int.

The other patch adds a sanity check to ensure that "passive" polling will not
be less frequent than "regular" polling.

Thanks!

[PATCH v2 1/2] thermal: core: Change passive_delay and polling_delay data type

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

It is better to use unsigned int as the data type for the passive_delay
and polling_delay arguments of thermal_zone_device_register_with_trips()
because they are implicitly cast to unsigned int anyway in
thermal_set_delay_jiffies() and if they happen to be negative at that
point, the resulting behavior may not be as desired.

Update the thermal_zone_device_register_with_trips() definition
accordingly.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

v1 -> v2: No changes

---
 drivers/thermal/thermal_core.c |    3 ++-
 include/linux/thermal.h        |    3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

Index: linux-pm/drivers/thermal/thermal_core.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.c
+++ linux-pm/drivers/thermal/thermal_core.c
@@ -1359,7 +1359,8 @@ thermal_zone_device_register_with_trips(
 					int num_trips, void *devdata,
 					const struct thermal_zone_device_ops *ops,
 					const struct thermal_zone_params *tzp,
-					int passive_delay, int polling_delay)
+					unsigned int passive_delay,
+					unsigned int polling_delay)
 {
 	const struct thermal_trip *trip = trips;
 	struct thermal_zone_device *tz;
Index: linux-pm/include/linux/thermal.h
===================================================================
--- linux-pm.orig/include/linux/thermal.h
+++ linux-pm/include/linux/thermal.h
@@ -221,7 +221,8 @@ struct thermal_zone_device *thermal_zone
 					int num_trips, void *devdata,
 					const struct thermal_zone_device_ops *ops,
 					const struct thermal_zone_params *tzp,
-					int passive_delay, int polling_delay);
+					unsigned int passive_delay,
+					unsigned int polling_delay);
 
 struct thermal_zone_device *thermal_tripless_zone_device_register(
 					const char *type,

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

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

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@intel.com>
---

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;
+
 	thermal_set_delay_jiffies(&tz->passive_delay_jiffies, passive_delay);
 	thermal_set_delay_jiffies(&tz->polling_delay_jiffies, polling_delay);
 

Re: [PATCH v2 1/2] thermal: core: Change passive_delay and polling_delay data type

[Daniel Lezcano]

On 05/07/2024 21:44, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> It is better to use unsigned int as the data type for the passive_delay
> and polling_delay arguments of thermal_zone_device_register_with_trips()
> because they are implicitly cast to unsigned int anyway in
> thermal_set_delay_jiffies() and if they happen to be negative at that
> point, the resulting behavior may not be as desired.
> 
> Update the thermal_zone_device_register_with_trips() definition
> accordingly.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>

-- 
<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

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

[Daniel Lezcano]

On 05/07/2024 21:46, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> 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@intel.com>
> ---
> 
> 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.

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


>   	thermal_set_delay_jiffies(&tz->passive_delay_jiffies, passive_delay);
>   	thermal_set_delay_jiffies(&tz->polling_delay_jiffies, polling_delay);


-- 
<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

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 2:12 PM Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>
> On 05/07/2024 21:46, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > 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@intel.com>
> > ---
> >
> > 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.

> 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.

Thanks!

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

[Daniel Lezcano]

On 08/07/2024 15:38, Rafael J. Wysocki wrote:
> On Mon, Jul 8, 2024 at 2:12 PM Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>>
>> On 05/07/2024 21:46, Rafael J. Wysocki wrote:
>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>
>>> 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@intel.com>
>>> ---
>>>
>>> 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. 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.

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.

-- 
<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

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 3:58 PM Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>
> On 08/07/2024 15:38, Rafael J. Wysocki wrote:
> > On Mon, Jul 8, 2024 at 2:12 PM Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
> >>
> >> On 05/07/2024 21:46, Rafael J. Wysocki wrote:
> >>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >>>
> >>> 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@intel.com>
> >>> ---
> >>>
> >>> 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.

> 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?

> 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.

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

[Daniel Lezcano]

On 08/07/2024 16:03, Rafael J. Wysocki wrote:
> On Mon, Jul 8, 2024 at 3:58 PM Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>>
>> On 08/07/2024 15:38, Rafael J. Wysocki wrote:
>>> On Mon, Jul 8, 2024 at 2:12 PM Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>>>>
>>>> On 05/07/2024 21:46, Rafael J. Wysocki wrote:
>>>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>>>
>>>>> 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@intel.com>
>>>>> ---
>>>>>
>>>>> 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.


-- 
<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

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@linaro.org> wrote:
>
> On 08/07/2024 16:03, Rafael J. Wysocki wrote:
> > On Mon, Jul 8, 2024 at 3:58 PM Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
> >>
> >> On 08/07/2024 15:38, Rafael J. Wysocki wrote:
> >>> On Mon, Jul 8, 2024 at 2:12 PM Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
> >>>>
> >>>> On 05/07/2024 21:46, Rafael J. Wysocki wrote:
> >>>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >>>>>
> >>>>> 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@intel.com>
> >>>>> ---
> >>>>>
> >>>>> 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.