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

[Daniel Lezcano <daniel.lezcano@linaro.org>]

On 17/01/2024 19:49, Rafael J. Wysocki wrote:
> On Wed, Jan 17, 2024 at 5:57 PM 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 AM Manaf Meethalavalappu Pallikunhi
>>> <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 PM Manaf Meethalavalappu Pallikunhi
>>>> <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 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