Received-SPF: pass (google.com: domain of linux-kernel+bounces-43184-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240106191502.29126-1-quic_manafm@quicinc.com>
 <CAJZ5v0gE6eEpALrfxHvCd5TRqjB+v8pffG4CKLTVXiSvuiWhHg@mail.gmail.com>
 <d7b82fc8-0ed8-80b8-9eb8-c77f9277178f@quicinc.com> <CAJZ5v0g4hnRqRCseRnTjfEF+-2=ZT8U9=2m9FODqh3G8eDd=Sw@mail.gmail.com>
 <921c2f90-fb8b-4e70-9e3d-6e185fec03b6@linaro.org> <CAJZ5v0h+93YBsYsA5rOvzp+b3JMGyjUStHA=J8P7ynv+-ym-4g@mail.gmail.com>
 <41b284d7-e31f-48b5-8b21-0dca3e23cb1c@linaro.org> <CAJZ5v0ina=7R6x6Ff=8_rRR9Kkmz2tkojbs_WWCN=JPmzhg+HQ@mail.gmail.com>
In-Reply-To: <CAJZ5v0ina=7R6x6Ff=8_rRR9Kkmz2tkojbs_WWCN=JPmzhg+HQ@mail.gmail.com>
From: "Rafael J. Wysocki" <rafael@kernel.org>
Date: Mon, 29 Jan 2024 18:07:20 +0100
Message-ID: <CAJZ5v0hM9Y+cV7WrEHe6WjzQ0ATnBce4NO1wxvZh=fcLWPkqKA@mail.gmail.com>
Subject: Re: [PATCH] thermal/sysfs: Always enable hysteresis write support
To: Daniel Lezcano <daniel.lezcano@linaro.org>, 
	Manaf Meethalavalappu Pallikunhi <quic_manafm@quicinc.com>
Cc: Zhang Rui <rui.zhang@intel.com>, Lukasz Luba <lukasz.luba@arm.com>, linux-pm@vger.kernel.org, 
	linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Mon, Jan 22, 2024 at 11:19=E2=80=AFAM Rafael J. Wysocki <rafael@kernel.o=
rg> wrote:
>
> On Thu, Jan 18, 2024 at 11:25=E2=80=AFAM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
> >
> > On 17/01/2024 19:49, Rafael J. Wysocki wrote:
> > > On Wed, Jan 17, 2024 at 5:57=E2=80=AFPM Daniel Lezcano
> > > <daniel.lezcano@linaro.org> wrote:
> > >>
> > >> On 10/01/2024 13:48, Rafael J. Wysocki wrote:
> > >>> Hi Manaf,
> > >>>
> > >>> On Wed, Jan 10, 2024 at 9:17=E2=80=AFAM Manaf Meethalavalappu Palli=
kunhi
> > >>> <quic_manafm@quicinc.com> wrote:
> > >>>>
> > >>>> Hi Rafael,
> > >>>>
> > >>>> On 1/9/2024 7:12 PM, Rafael J. Wysocki wrote:
> > >>>>
> > >>>> On Sat, Jan 6, 2024 at 8:16=E2=80=AFPM Manaf Meethalavalappu Palli=
kunhi
> > >>>> <quic_manafm@quicinc.com> wrote:
> > >>>>
> > >>>> The commit 2e38a2a981b2("thermal/core: Add a generic
> > >>>> thermal_zone_set_trip() function") adds the support to update
> > >>>> trip hysteresis even if set_trip_hyst() operation is not defined.
> > >>>> But during hysteresis attribute creation, if this operation is
> > >>>> defined then only it enables hysteresis write access. It leads
> > >>>> to a case where hysteresis sysfs will be read only for a thermal
> > >>>> zone when its set_trip_hyst() operation is not defined.
> > >>>>
> > >>>> Which is by design.
> > >>>>
> > >>>> I think it is regression after recent re-work. If a sensor is regi=
stered with thermal framework via thermal_of,
> > >>>>
> > >>>> sensor driver doesn't need to know the trip configuration and noth=
ing to do with set_trip_hyst() in driver.
> > >>>>
> > >>>> Without this change, if a sensor needs to be monitored from usersp=
ace(trip/hysteresis),
> > >>>
> > >>> What exactly do you mean by "monitored" here?
> > >>>
> > >>>> it is enforcing sensor driver to add  dummy set_trip_hyst() operat=
ion. Correct me otherwise
> > >>>
> > >>> With the current design, whether or not trip properties can be upda=
ted
> > >>> by user space is a thermal zone property expressed by the presence =
of
> > >>> the set_trip_* operations, so yes, whoever registers the thermal zo=
ne
> > >>> needs to provide those so that user space can update the trip
> > >>> properties.
> > >>>
> > >>>> For some thermal zone types (eg. acpi), updating trip hysteresis v=
ia
> > >>>> sysfs might lead to incorrect behavior.
> > >>>>
> > >>>> To address this issue, is it okay to  guard  hysteresis write perm=
ission under CONFIG_THERMAL_WRITABLE_TRIPS defconfig ?
> > >>>
> > >>> Not really, because it would affect all of the thermal zones then.
> > >>
> > >> It seems like there is an inconsistency here with the writable trip
> > >> points and the writable hysteresis [1].
> > >>
> > >> My understanding is it does not make sense to have the hysteresis
> > >> writable even if the driver has a hysteresis dedicated ops. The code
> > >> allowing to change the hysteresis was done regardless the consistenc=
y
> > >> with the trip temperature change and writable trip points kernel opt=
ion IMO.
> > >>
> > >> It would make sense to have:
> > >>
> > >> if enabled(CONFIG_WRITABLE_TRIP_POINT)
> > >>    -> trip_temp RW
> > >>    -> trip_hyst RW
> > >> else
> > >>    -> trip temp RO
> > >>    -> trip hyst RO
> > >> fi
> > >>
> > >> But if the interface exists since a long time, we may not want to ch=
ange
> > >> it, right ?
> > >
> > > If the platform firmware implements hysteresis by changing trip
> > > temperature (as recommended by the ACPI specification, for example),
> > > modifying the trip hysteresis via sysfs is simply incorrect and user
> > > space may not know that.
> > >
> > >> However, we can take the opportunity to introduce a new 'user' trip
> > >> point type in order to let the userspace to have dedicated trip poin=
t
> > >> and receive temperature notifications [2]
> > >>
> > >>> TBH, the exact scenario in which user space needs to update trip
> > >>> hysteresis is not particularly clear to me, so can you provide some
> > >>> more details, please?
> > >>
> > >> IIUC changing the hysteresis value is useful because the temperature
> > >> speed will vary given the thermal contribution of the components
> > >> surrounding the thermal zone, that includes the ambient temperature.
> > >>
> > >> However, that may apply to slow speed temperature sensor like the sk=
in
> > >> temperature sensor where we may to do small hysteresis variation.
> > >>
> > >> The places managed by the kernel have an insane temperature transiti=
on
> > >> speed. The userspace is unable to follow this speed and manage the
> > >> hysteresis on the fly.
> > >>
> > >> So that brings us to userspace trip point handling again.
> > >
> > > Well, I've already said that whether hysteresis can be modified via
> > > sysfs is a property of a thermal zone.
> >
> > > It may as well be a trip property, for example expressed via a (new)
> > > trip flag set in the trips table used for thermal zone registration.
> >
> > Yes, a trip property makes more sense.
> >
> > I'm a bit lost about WRITABLE_TRIP_POINT, writable hysteresis, read-onl=
y
> > temperature trip.
> >
> > Can we have a hysteresis writable but not the temperature ?
> >
> > You mentioned above "modifying the trip hysteresis via sysfs is simply
> > incorrect", so shall we allow that at the end?
> >
> > Is it possible to recap the situation?
>
> Sure, which is a good idea BTW.
>
> First off, the callers of thermal_zone_device_register_with_trips()
> need to pass a mask of writeable trip points to it.  If the mask is 0,
> none of the trip attributes are writeable for any trips.
>
> However, the mask only takes effect if CONFIG_THERMAL_WRITABLE_TRIPS
> is set.  Otherwise, it is not taken into account at all.
>
> Now, if CONFIG_THERMAL_WRITABLE_TRIPS is set, it only affects the trip
> temperature, which is a bit inconsistent.
>
> Moreover, the hysteresis is allowed to be updated unconditionally if
> tz->ops->set_trip_hyst is not NULL, which is even more inconsistent.
>
> So, because it already is only enabled if the creator of the thermal
> zone asks for it explicitly, it would be fine by me to simply allow
> the hysteresis to be updated if the temperature is allowed to be
> updated.
>
> IOW, something like the patch below (unstested, white space messed-up by =
gmail).
>
> If this looks OK to everyone from the functionality perspective, I can
> submit it properly with a changelog etc.
>
> ---
>  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_show;
> -        if (tz->ops->set_trip_hyst) {
> +        if (IS_ENABLED(CONFIG_THERMAL_WRITABLE_TRIPS) &&
> +            mask & (1 << indx)) {
>              tz->trip_hyst_attrs[indx].attr.attr.mode |=3D S_IWUSR;
>              tz->trip_hyst_attrs[indx].attr.store =3D
>                      trip_point_hyst_store;

So it looks like I need to submit this, even though I'm not sure if
anyone cares.

In any case, I care about consistency.