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

[Zhang, Rui]

On Tue, 2023-06-06 at 17:37 -0700, 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.
> 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.
> 
> 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*/

what is the expected behavior of a disabled zone?

IMO, the hardware may not be functional at this point, and reading the
temperature should be avoided, as we do in
__thermal_zone_device_update().

should we just return failure in this case?

userspace should poke the temp attribute for enabled zones only.

thanks,
rui
> +               if (r)
> +                       return r;
> +       } else {
> +               mutex_lock(&tz->lock);
> +               temperature = tz->temperature;
> +               mutex_unlock(&tz->lock);
> +       }
>  
>         return sprintf(buf, "%d\n", temperature);
>  }