lkml.org 
[lkml]   [2024]   [Jan]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v1] thermal: sysfs: Make trip hysteresis writable along with trip temperature
From
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


\
 
 \ /
  Last update: 2024-05-27 14:43    [W:0.061 / U:0.056 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site