Re: [PATCH v1] thermal: sysfs: Make trip hysteresis writable along with trip temperature

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

On 29/01/2024 21:40, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Trip point temperature can be modified via sysfs if
> CONFIG_THERMAL_WRITABLE_TRIPS is enabled and the thermal
> zone creator requested that the given trip be writable
> in the writable trips mask passed to the registration
> function.
> 
> However, trip point hysteresis is treated differently - it is only
> writable if the thermal zone has a .set_trip_hyst() operation defined
> and neither CONFIG_THERMAL_WRITABLE_TRIPS, nor the writable trips mask
> supplied by the zone creator has any bearing on this.  That is
> inconsistent and confusing, and it generally does not meet user
> expectations.
> 
> For this reason, modify create_trip_attrs() to handle trip point
> hysteresis in the same way as trip point temperature, so they both
> are writable at the same time regardless of what trip point operations
> are defined for the thermal zone.
> 
> Link: https://lore.kernel.org/linux-pm/20240106191502.29126-1-quic_manafm@quicinc.com
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> 
> Notes:
> 
>   * I don't think that CONFIG_THERMAL_WRITABLE_TRIPS is very useful.
>     The only thing controlled by it is whether or not the writable trip
>     mask used during registration will have any effect and this is quite
>     confusing.  Some drivers select it for this reason which seems a bit
>     odd to me.
> 
>     Maybe it can be dropped after the patch below?

Actually it is used from an userspace daemon to get threshold crossing 
temperature which is then changed on the fly.

Instead of using multiple trip points, they use one where they change 
the temperature after it crossed the threshold.

Usually the userspace tracks slow sensor temperature in order to set a 
specific set of limitations given a scenario. We are talking here about 
Android and thermal engines which are platform specific. For example, 
lower the battery charging speed if there is a game profile.

 From my POV, the thermal framework has been hacked via 
CONFIG_THERMAL_WRITABLE_TRIPS from userspace to get these threshold 
notification and to be honest I find that not sane. This should fall in 
a thermal debug section defaulting to 'no'.

So in some ways in agree with you. We should drop it or make it more 
debug oriented in order to prevent it to go in production.

But before doing that, we should provide a mechanism to userspace to 
specify an 'userspace' trip point. However, it is more complex than what 
it looks because the userspace should be able to specify a group of 
temperature (and hysteresis) in order to be notified when the boundaries 
are crossed and those can be dynamic.

I will provide a proposal in a separate thread in order to not pollute 
the discussion of this one.

>   * IMO the writable trips mask itself is quite cumbersome and it would be
>     better to mark individual trips as writable in the trips table passed
>     during registration.  This would be less prone to mistakes and it
>     would allow the code to check whether or not the given trip should
>     be writable (root can change sysfs file modes after all).  If I'm not
>     mistaken, this change should not be very hard to make, although it may
>     take some time to switch over all of the relevant drivers from using
>     the mask.

+1 +1 +1

I don't think they are so many drivers using this mask. All the drivers 
tied with a OF initialization are not impacted as the change will be in 
one site.

> ---
>   drivers/thermal/thermal_sysfs.c |    3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> Index: linux-pm/drivers/thermal/thermal_sysfs.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_sysfs.c
> +++ linux-pm/drivers/thermal/thermal_sysfs.c
> @@ -474,7 +474,8 @@ static int create_trip_attrs(struct ther
>   					tz->trip_hyst_attrs[indx].name;
>   		tz->trip_hyst_attrs[indx].attr.attr.mode = S_IRUGO;
>   		tz->trip_hyst_attrs[indx].attr.show = trip_point_hyst_show;
> -		if (tz->ops->set_trip_hyst) {
> +		if (IS_ENABLED(v) &&

                              ^^^

s/v/CONFIG_THERMAL_WRITABLE_TRIPS/ ?

> +		    mask & (1 << indx)) {
>   			tz->trip_hyst_attrs[indx].attr.attr.mode |= S_IWUSR;
>   			tz->trip_hyst_attrs[indx].attr.store =
>   					trip_point_hyst_store;
> 
> 
> 

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