Re: [PATCH v1] thermal: sysfs: Make trip hysteresis writable along with trip temperature

[Rafael J. Wysocki]

On Wednesday, January 31, 2024 7:41:52 PM CET Rafael J. Wysocki wrote:
> On Wed, Jan 31, 2024 at 7:18 PM Daniel Lezcano
> <daniel.lezcano@xxxxxxxxxx> wrote:
> >
> > On 29/01/2024 21:40, Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> > >
> > > Trip point temperature can be modified via sysfs if
> > > CONFIG_THERMAL_WRITABLE_TRIPS is enabled and the thermal
> > > zone creator requested that the given trip be writable
> > > in the writable trips mask passed to the registration
> > > function.
> > >
> > > However, trip point hysteresis is treated differently - it is only
> > > writable if the thermal zone has a .set_trip_hyst() operation defined
> > > and neither CONFIG_THERMAL_WRITABLE_TRIPS, nor the writable trips mask
> > > supplied by the zone creator has any bearing on this.  That is
> > > inconsistent and confusing, and it generally does not meet user
> > > expectations.
> > >
> > > For this reason, modify create_trip_attrs() to handle trip point
> > > hysteresis in the same way as trip point temperature, so they both
> > > are writable at the same time regardless of what trip point operations
> > > are defined for the thermal zone.
> > >
> > > Link: https://lore.kernel.org/linux-pm/20240106191502.29126-1-quic_manafm@xxxxxxxxxxx
> > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> > > ---
> > >
> > > Notes:
> > >
> > >   * I don't think that CONFIG_THERMAL_WRITABLE_TRIPS is very useful.
> > >     The only thing controlled by it is whether or not the writable trip
> > >     mask used during registration will have any effect and this is quite
> > >     confusing.  Some drivers select it for this reason which seems a bit
> > >     odd to me.
> > >
> > >     Maybe it can be dropped after the patch below?
> >
> > Actually it is used from an userspace daemon to get threshold crossing
> > temperature which is then changed on the fly.
> 
> I mean to drop CONFIG_THERMAL_WRITABLE_TRIPS and make the writable
> trip masks used during zone registration always work.  Sorry for the
> confusion.

So for the record, this (and note that the symbol is clearly not used as
intended, because drivers select it and one platform sets it in defconfig):

---
 arch/arm/configs/imx_v6_v7_defconfig |    1 -
 drivers/thermal/Kconfig              |   11 -----------
 drivers/thermal/intel/Kconfig        |    2 --
 drivers/thermal/thermal_sysfs.c      |    8 +++-----
 4 files changed, 3 insertions(+), 19 deletions(-)

Index: linux-pm/drivers/thermal/Kconfig
===================================================================
--- linux-pm.orig/drivers/thermal/Kconfig
+++ linux-pm/drivers/thermal/Kconfig
@@ -83,17 +83,6 @@ config THERMAL_OF
 	  Say 'Y' here if you need to build thermal infrastructure
 	  based on device tree.
 
-config THERMAL_WRITABLE_TRIPS
-	bool "Enable writable trip points"
-	help
-	  This option allows the system integrator to choose whether
-	  trip temperatures can be changed from userspace. The
-	  writable trips need to be specified when setting up the
-	  thermal zone but the choice here takes precedence.
-
-	  Say 'Y' here if you would like to allow userspace tools to
-	  change trip temperatures.
-
 choice
 	prompt "Default Thermal governor"
 	default THERMAL_DEFAULT_GOV_STEP_WISE
Index: linux-pm/drivers/thermal/thermal_sysfs.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_sysfs.c
+++ linux-pm/drivers/thermal/thermal_sysfs.c
@@ -458,8 +458,7 @@ static int create_trip_attrs(struct ther
 						tz->trip_temp_attrs[indx].name;
 		tz->trip_temp_attrs[indx].attr.attr.mode = S_IRUGO;
 		tz->trip_temp_attrs[indx].attr.show = trip_point_temp_show;
-		if (IS_ENABLED(CONFIG_THERMAL_WRITABLE_TRIPS) &&
-		    mask & (1 << indx)) {
+		if (mask & (1 << indx)) {
 			tz->trip_temp_attrs[indx].attr.attr.mode |= S_IWUSR;
 			tz->trip_temp_attrs[indx].attr.store =
 							trip_point_temp_store;
@@ -474,8 +473,7 @@ 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 (IS_ENABLED(CONFIG_THERMAL_WRITABLE_TRIPS) &&
-		    mask & (1 << indx)) {
+		if (mask & (1 << indx)) {
 			tz->trip_hyst_attrs[indx].attr.attr.mode |= S_IWUSR;
 			tz->trip_hyst_attrs[indx].attr.store =
 					trip_point_hyst_store;
Index: linux-pm/drivers/thermal/intel/Kconfig
===================================================================
--- linux-pm.orig/drivers/thermal/intel/Kconfig
+++ linux-pm/drivers/thermal/intel/Kconfig
@@ -23,7 +23,6 @@ config X86_PKG_TEMP_THERMAL
 	tristate "X86 package temperature thermal driver"
 	depends on X86_THERMAL_VECTOR
 	select THERMAL_GOV_USER_SPACE
-	select THERMAL_WRITABLE_TRIPS
 	select INTEL_TCC
 	default m
 	help
@@ -47,7 +46,6 @@ config INTEL_SOC_DTS_THERMAL
 	tristate "Intel SoCs DTS thermal driver"
 	depends on X86 && PCI && ACPI
 	select INTEL_SOC_DTS_IOSF_CORE
-	select THERMAL_WRITABLE_TRIPS
 	help
 	  Enable this to register Intel SoCs (e.g. Bay Trail) platform digital
 	  temperature sensor (DTS). These SoCs have two additional DTSs in
Index: linux-pm/arch/arm/configs/imx_v6_v7_defconfig
===================================================================
--- linux-pm.orig/arch/arm/configs/imx_v6_v7_defconfig
+++ linux-pm/arch/arm/configs/imx_v6_v7_defconfig
@@ -228,7 +228,6 @@ CONFIG_SENSORS_IIO_HWMON=y
 CONFIG_SENSORS_PWM_FAN=y
 CONFIG_SENSORS_SY7636A=y
 CONFIG_THERMAL_STATISTICS=y
-CONFIG_THERMAL_WRITABLE_TRIPS=y
 CONFIG_CPU_THERMAL=y
 CONFIG_IMX_THERMAL=y
 CONFIG_WATCHDOG=y