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

[Daniel Lezcano]

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?



--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog