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

[Rafael J. Wysocki]

On Wed, Jan 31, 2024 at 7:18 PM Daniel Lezcano
<daniel.lezcano@xxxxxxxxxx> wrote:
>
> On 29/01/2024 21:40, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> >
> > 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@xxxxxxxxxxx
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> > ---
> >
> > 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;
> >
> >
> >
>
> --