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

[Daniel Lezcano <daniel.lezcano@linaro.org>]

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