lkml.org 
[lkml]   [2024]   [Jan]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH] thermal/sysfs: Always enable hysteresis write support
From
Hi Rafael,

On 1/10/2024 6:18 PM, 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?

There can be userspace thermal manager/clients(eg: thermal HAL in
android, thermal manager daemon etc. ) with different trip
pairs(temperature and hysteresis) for its own thermal management,
temperature reporting, thermal tuning etc.

This client can update trip and hysteresis dynamically via thermal zone
trip point sysfs nodes for event violation notification irrespective of
kernel thermal zone devicetree trip values.

This was supporting until this rework without any set_trip_* ops from
sensor driver.

>
>> 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

If you look at current code, it is allowing to set trip temperature
without set_trip_temp() operation, only hysteresis is not allowed.

As I mentioned above cases, userspace sysfs update is usecase/client
driven, not always a sensor driver specific requirement especially a
sensor is registered via thermal_of.  Not sure adding a dummy ops in
every sensor driver to achieve above requirement is right solution here.

> 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.
>
> 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?

I hope I explained more information in above comment. let me know otherwise.

Thanks,

Manaf

>
> Thanks!

\
 
 \ /
  Last update: 2024-01-11 15:14    [W:0.116 / U:0.596 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site