linux-kernel - Re: [PATCH v2 2/2] thermal: core: Add sanity check for polling_delay and passive

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <402ede79-5eda-48fc-8eb8-5d89ffe6bd41@linaro.org>
Date: Mon, 8 Jul 2024 14:06:33 +0200
From: Daniel Lezcano <daniel.lezcano@...aro.org>
To: "Rafael J. Wysocki" <rjw@...ysocki.net>,
 Linux PM <linux-pm@...r.kernel.org>
Cc: LKML <linux-kernel@...r.kernel.org>, Lukasz Luba <lukasz.luba@....com>
Subject: Re: [PATCH v2 2/2] thermal: core: Add sanity check for polling_delay
 and passive_delay

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