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

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

[Manaf Meethalavalappu Pallikunhi]

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.

Fix this by removing the check whether set_trip_hyst() operation
is defined or not during hysteresis attribute initialization.

Signed-off-by: Manaf Meethalavalappu Pallikunhi <quic_manafm@quicinc.com>
---
 drivers/thermal/thermal_sysfs.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
index eef40d4f3063..08be016d7221 100644
--- a/drivers/thermal/thermal_sysfs.c
+++ b/drivers/thermal/thermal_sysfs.c
@@ -504,13 +504,9 @@ static int create_trip_attrs(struct thermal_zone_device *tz, int mask)
 		sysfs_attr_init(&tz->trip_hyst_attrs[indx].attr.attr);
 		tz->trip_hyst_attrs[indx].attr.attr.name =
 					tz->trip_hyst_attrs[indx].name;
-		tz->trip_hyst_attrs[indx].attr.attr.mode = S_IRUGO;
+		tz->trip_hyst_attrs[indx].attr.attr.mode = S_IRUGO | S_IWUSR;
 		tz->trip_hyst_attrs[indx].attr.show = trip_point_hyst_show;
-		if (tz->ops->set_trip_hyst) {
-			tz->trip_hyst_attrs[indx].attr.attr.mode |= S_IWUSR;
-			tz->trip_hyst_attrs[indx].attr.store =
-					trip_point_hyst_store;
-		}
+		tz->trip_hyst_attrs[indx].attr.store = trip_point_hyst_store;
 		attrs[indx + tz->num_trips * 2] =
 					&tz->trip_hyst_attrs[indx].attr.attr;
 	}

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

[Rafael J. Wysocki]

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.

For some thermal zone types (eg. acpi), updating trip hysteresis via
sysfs might lead to incorrect behavior.

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

[Manaf Meethalavalappu Pallikunhi]

Resending to reflect to format

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.
I think it is regression after recent re-work.  If  a sensor  is 
registered witht 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), it is enforcing sensor driver 
to add  dummy set_trip_hyst() operation. Correct me otherwise.
> Which is by design.
>
> For some thermal zone types (eg. acpi), updating trip hysteresis via
> sysfs might lead to incorrect behavior.

To address this, is it okay to guard hysteresis write permission under 
CONFIG_THERMAL_WRITABLE_TRIPS flag ?

Thanks,

Manaf

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

[Rafael J. Wysocki]

On Wed, Jan 10, 2024 at 1:48 PM Manaf Meethalavalappu Pallikunhi
<quic_manafm@quicinc.com> wrote:
>
> Resending to reflect to format
>
> 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.
> I think it is regression after recent re-work.  If  a sensor  is
> registered witht 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), it is enforcing sensor driver
> to add  dummy set_trip_hyst() operation. Correct me otherwise.
> > Which is by design.
> >
> > For some thermal zone types (eg. acpi), updating trip hysteresis via
> > sysfs might lead to incorrect behavior.
>
> To address this, is it okay to guard hysteresis write permission under
> CONFIG_THERMAL_WRITABLE_TRIPS flag ?

I've already sent a reply to the original message.

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

[Rafael J. Wysocki]

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.

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?

Thanks!

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

[Manaf Meethalavalappu Pallikunhi]

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!

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

[Daniel Lezcano]

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 ?

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.

   -- Daniel

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/ABI/testing/sysfs-class-thermal?h=v6.7#n66

[2] https://lpc.events/event/17/contributions/1423/contribution.pdf

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

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

[Rafael J. Wysocki]

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.

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

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

[Rafael J. Wysocki]

On Thu, Jan 18, 2024 at 11:25 AM 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 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?

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;

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@kernel.org> wrote:
>
> On Thu, Jan 18, 2024 at 11:25 AM 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 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?
>
> 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.

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

[Daniel Lezcano]

Hi Rafael,

On 29/01/2024 18:07, Rafael J. Wysocki wrote:

[ ... ]

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

Yeah, sorry for the delay. I'll have a look at it tomorrow

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

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

[Rafael J. Wysocki]

Hi Daniel,

On Mon, Jan 29, 2024 at 6:54 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> Hi Rafael,
>
> On 29/01/2024 18:07, Rafael J. Wysocki wrote:
>
> [ ... ]
>
> >> 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.
>
> Yeah, sorry for the delay. I'll have a look at it tomorrow

Thanks!

Posted here with some additional remarks:

https://lore.kernel.org/linux-pm/4565526.LvFx2qVVIh@kreacher/

Let's continue the discussion there.