Received-SPF: pass (google.com: domain of linux-kernel+bounces-46979-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <4565526.LvFx2qVVIh@kreacher> <7caf2f4d-d0d5-4622-b290-bb0396547f3c@linaro.org>
In-Reply-To: <7caf2f4d-d0d5-4622-b290-bb0396547f3c@linaro.org>
From: "Rafael J. Wysocki" <rafael@kernel.org>
Date: Wed, 31 Jan 2024 19:41:52 +0100
Message-ID: <CAJZ5v0iZ0hyPYB3i6YdbiKueHGWoM3i6mPBnzGL9bB8wFxVSPw@mail.gmail.com>
Subject: Re: [PATCH v1] thermal: sysfs: Make trip hysteresis writable along
 with trip temperature
To: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>, Linux PM <linux-pm@vger.kernel.org>, 
	LKML <linux-kernel@vger.kernel.org>, Lukasz Luba <lukasz.luba@arm.com>, 
	Zhang Rui <rui.zhang@intel.com>, 
	Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>, 
	Manaf Meethalavalappu Pallikunhi <quic_manafm@quicinc.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, Jan 31, 2024 at 7:18=E2=80=AFPM 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_mana=
fm@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 quit=
e
> >     confusing.  Some drivers select it for this reason which seems a bi=
t
> >     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 pass=
ed
> >     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 n=
ot
> >     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 usin=
g
> >     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
> > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> > --- 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 =3D S_IRUGO;
> >               tz->trip_hyst_attrs[indx].attr.show =3D trip_point_hyst_s=
how;
> > -             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 |=3D S_I=
WUSR;
> >                       tz->trip_hyst_attrs[indx].attr.store =3D
> >                                       trip_point_hyst_store;
> >
> >
> >
>
> --