Re: [PATCH] thermal/sysfs: Always enable hysteresis write support

[Rafael J. Wysocki]

On Mon, Jan 22, 2024 at 11:19 AM Rafael J. Wysocki <rafael@xxxxxxxxxx> wrote:
>
> On Thu, Jan 18, 2024 at 11:25 AM Daniel Lezcano
> <daniel.lezcano@xxxxxxxxxx> wrote:
> >
> > On 17/01/2024 19:49, Rafael J. Wysocki wrote:
> > > On Wed, Jan 17, 2024 at 5:57 PM Daniel Lezcano
> > > <daniel.lezcano@xxxxxxxxxx> wrote:
> > >>
> > >> On 10/01/2024 13:48, Rafael J. Wysocki wrote:
> > >>> Hi Manaf,
> > >>>
> > >>> On Wed, Jan 10, 2024 at 9:17 AM Manaf Meethalavalappu Pallikunhi
> > >>> <quic_manafm@xxxxxxxxxxx> wrote:
> > >>>>
> > >>>> Hi Rafael,
> > >>>>
> > >>>> On 1/9/2024 7:12 PM, Rafael J. Wysocki wrote:
> > >>>>
> > >>>> On Sat, Jan 6, 2024 at 8:16 PM Manaf Meethalavalappu Pallikunhi
> > >>>> <quic_manafm@xxxxxxxxxxx> 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 registered with thermal framework via thermal_of,
> > >>>>
> > >>>> sensor driver doesn't need to know the trip configuration and nothing to do with set_trip_hyst() in driver.
> > >>>>
> > >>>> Without this change, if a sensor needs to be monitored from userspace(trip/hysteresis),
> > >>>
> > >>> What exactly do you mean by "monitored" here?
> > >>>
> > >>>> it is enforcing sensor driver to add  dummy set_trip_hyst() operation. Correct me otherwise
> > >>>
> > >>> With the current design, whether or not trip properties can be updated
> > >>> by user space is a thermal zone property expressed by the presence of
> > >>> the set_trip_* operations, so yes, whoever registers the thermal zone
> > >>> needs to provide those so that user space can update the trip
> > >>> properties.
> > >>>
> > >>>> For some thermal zone types (eg. acpi), updating trip hysteresis via
> > >>>> sysfs might lead to incorrect behavior.
> > >>>>
> > >>>> To address this issue, is it okay to  guard  hysteresis write permission 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 consistency
> > >> with the trip temperature change and writable trip points kernel option 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 change
> > >> 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 point
> > >> 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 skin
> > >> temperature sensor where we may to do small hysteresis variation.
> > >>
> > >> The places managed by the kernel have an insane temperature transition
> > >> 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-only
> > 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
> ===================================================================
> --- 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(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;

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.