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

[Daniel Lezcano]

Hi Eduardo,

On 07/06/2023 02:37, Eduardo Valentin wrote:
> From: Eduardo Valentin <eduval@xxxxxxxxxx>
>
> 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@xxxxxxxxxx> (supporter:THERMAL)
> Cc: Daniel Lezcano <daniel.lezcano@xxxxxxxxxx> (supporter:THERMAL)
> Cc: Amit Kucheria <amitk@xxxxxxxxxx> (reviewer:THERMAL)
> Cc: Zhang Rui <rui.zhang@xxxxxxxxx> (reviewer:THERMAL)
> Cc: linux-pm@xxxxxxxxxxxxxxx (open list:THERMAL)
> Cc: linux-kernel@xxxxxxxxxxxxxxx (open list)
>
> Signed-off-by: Eduardo Valentin <eduval@xxxxxxxxxx>
> ---
>   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