Messages in this thread | | | From | "Rafael J. Wysocki" <> | Date | Wed, 31 Jan 2024 19:41:52 +0100 | Subject | Re: [PATCH v1] thermal: sysfs: Make trip hysteresis writable along with trip temperature |
| |
On Wed, Jan 31, 2024 at 7:18 PM Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > > 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.
I mean to drop CONFIG_THERMAL_WRITABLE_TRIPS and make the writable trip masks used during zone registration always work. Sorry for the confusion.
> 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.
There are a few. I think around 50% of the thermal_zone_device_register_with_trips() callers pass non-empty writable trip points masks.
> > --- > > 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/ ?
Yes, and I'm not sure what happened here, because my local copy of the patch is correct.
I'll send a v2 shortly.
> > + mask & (1 << indx)) { > > tz->trip_hyst_attrs[indx].attr.attr.mode |= S_IWUSR; > > tz->trip_hyst_attrs[indx].attr.store = > > trip_point_hyst_store; > > > > > > > > --
| |