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

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.

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

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.