linux-kernel - Re: [PATCH 1/1] thermal: sysfs: avoid actual readings from sysfs

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 for Android: free password hash cracker in your pocket

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

Message-ID: <f26ac9a9-60af-a0fe-fccc-25bcd306f5a1@linaro.org>
Date:   Wed, 7 Jun 2023 11:24:21 +0200
From:   Daniel Lezcano <daniel.lezcano@...aro.org>
To:     Eduardo Valentin <evalenti@...nel.org>, eduval@...zon.com,
        rafael@...nel.org, linux-pm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Cc:     Amit Kucheria <amitk@...nel.org>, Zhang Rui <rui.zhang@...el.com>
Subject: Re: [PATCH 1/1] thermal: sysfs: avoid actual readings from sysfs


Hi Eduardo,

On 07/06/2023 02:37, Eduardo Valentin wrote:
> From: Eduardo Valentin <eduval@...zon.com>
> 
> As the thermal zone caches the current and last temperature
> value, the sysfs interface can use that instead of
> forcing an actual update or read from the device.

If the read fails, userspace can handle that by using the previous 
value. Do we really want to hide driver dysfunctions?

> This way, if multiple userspace requests are coming
> in, we avoid storming the device with multiple reads
> and potentially clogging the timing requirement
> for the governors.


Can you elaborate 'the timing requirement for the governors' ? I'm 
missing the point

> Cc: "Rafael J. Wysocki" <rafael@...nel.org> (supporter:THERMAL)
> Cc: Daniel Lezcano <daniel.lezcano@...aro.org> (supporter:THERMAL)
> Cc: Amit Kucheria <amitk@...nel.org> (reviewer:THERMAL)
> Cc: Zhang Rui <rui.zhang@...el.com> (reviewer:THERMAL)
> Cc: linux-pm@...r.kernel.org (open list:THERMAL)
> Cc: linux-kernel@...r.kernel.org (open list)
> 
> Signed-off-by: Eduardo Valentin <eduval@...zon.com>
> ---
>   drivers/thermal/thermal_sysfs.c | 21 ++++++++++++++++-----
>   1 file changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
> index b6daea2398da..a240c58d9e08 100644
> --- a/drivers/thermal/thermal_sysfs.c
> +++ b/drivers/thermal/thermal_sysfs.c
> @@ -35,12 +35,23 @@ static ssize_t
>   temp_show(struct device *dev, struct device_attribute *attr, char *buf)
>   {
>   	struct thermal_zone_device *tz = to_thermal_zone(dev);
> -	int temperature, ret;
> -
> -	ret = thermal_zone_get_temp(tz, &temperature);
> +	int temperature;
>   
> -	if (ret)
> -		return ret;
> +	/*
> +	 * don't force new update from external reads
> +	 * This way we avoid messing up with time constraints.
> +	 */
> +	if (tz->mode == THERMAL_DEVICE_DISABLED) {
> +		int r;
> +
> +		r = thermal_zone_get_temp(tz, &temperature); /* holds tz->lock*/
> +		if (r)
> +			return r;
> +	} else {
> +		mutex_lock(&tz->lock);
> +		temperature = tz->temperature;
> +		mutex_unlock(&tz->lock);
> +	}

No please, we are pushing since several weeks a lot of changes to 
encapsulate the thermal zone device structure and prevent external core 
components to use the internals directly. Even if we can consider the 
thermal_sysfs as part of the core code, that changes is not sysfs related.

>   	return sprintf(buf, "%d\n", temperature);
>   }

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