diff mbox series

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

Message ID 4565526.LvFx2qVVIh@kreacher (mailing list archive)
State Superseded, archived
Headers show
Series [v1] thermal: sysfs: Make trip hysteresis writable along with trip temperature | expand

Commit Message

Rafael J. Wysocki Jan. 29, 2024, 8:40 p.m. UTC 
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

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@quicinc.com
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

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?

 * IMO the writable trips mask itself is quite cumbersome and it would be
   better to mark individual trips as writable in the trips table passed
   during registration.  This would be less prone to mistakes and it
   would allow the code to check whether or not the given trip should
   be writable (root can change sysfs file modes after all).  If I'm not
   mistaken, this change should not be very hard to make, although it may
   take some time to switch over all of the relevant drivers from using
   the mask.

---
 drivers/thermal/thermal_sysfs.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Daniel Lezcano Jan. 31, 2024, 6:18 p.m. UTC | #1 
On 29/01/2024 21:40, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> 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@quicinc.com
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> 
> 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.

Instead of using multiple trip points, they use one where they change 
the temperature after it crossed the threshold.

Usually the userspace tracks slow sensor temperature in order to set a 
specific set of limitations given a scenario. We are talking here about 
Android and thermal engines which are platform specific. For example, 
lower the battery charging speed if there is a game profile.

 From my POV, the thermal framework has been hacked via 
CONFIG_THERMAL_WRITABLE_TRIPS from userspace to get these threshold 
notification and to be honest I find that not sane. This should fall in 
a thermal debug section defaulting to 'no'.

So in some ways in agree with you. We should drop it or make it more 
debug oriented in order to prevent it to go in production.

But before doing that, we should provide a mechanism to userspace to 
specify an 'userspace' trip point. However, it is more complex than what 
it looks because the userspace should be able to specify a group of 
temperature (and hysteresis) in order to be notified when the boundaries 
are crossed and those can be dynamic.

I will provide a proposal in a separate thread in order to not pollute 
the discussion of this one.

>   * IMO the writable trips mask itself is quite cumbersome and it would be
>     better to mark individual trips as writable in the trips table passed
>     during registration.  This would be less prone to mistakes and it
>     would allow the code to check whether or not the given trip should
>     be writable (root can change sysfs file modes after all).  If I'm not
>     mistaken, this change should not be very hard to make, although it may
>     take some time to switch over all of the relevant drivers from using
>     the mask.

+1 +1 +1

I don't think they are so many drivers using this mask. All the drivers 
tied with a OF initialization are not impacted as the change will be in 
one site.

> ---
>   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(v) &&

                              ^^^

s/v/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;
> 
> 
>
Rafael J. Wysocki Jan. 31, 2024, 6:41 p.m. UTC | #2 
On Wed, Jan 31, 2024 at 7:18 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> On 29/01/2024 21:40, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > 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@quicinc.com
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >
> > 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.

> Instead of using multiple trip points, they use one where they change
> the temperature after it crossed the threshold.
>
> Usually the userspace tracks slow sensor temperature in order to set a
> specific set of limitations given a scenario. We are talking here about
> Android and thermal engines which are platform specific. For example,
> lower the battery charging speed if there is a game profile.
>
>  From my POV, the thermal framework has been hacked via
> CONFIG_THERMAL_WRITABLE_TRIPS from userspace to get these threshold
> notification and to be honest I find that not sane. This should fall in
> a thermal debug section defaulting to 'no'.
>
> So in some ways in agree with you. We should drop it or make it more
> debug oriented in order to prevent it to go in production.
>
> But before doing that, we should provide a mechanism to userspace to
> specify an 'userspace' trip point. However, it is more complex than what
> it looks because the userspace should be able to specify a group of
> temperature (and hysteresis) in order to be notified when the boundaries
> are crossed and those can be dynamic.
>
> I will provide a proposal in a separate thread in order to not pollute
> the discussion of this one.
>
> >   * IMO the writable trips mask itself is quite cumbersome and it would be
> >     better to mark individual trips as writable in the trips table passed
> >     during registration.  This would be less prone to mistakes and it
> >     would allow the code to check whether or not the given trip should
> >     be writable (root can change sysfs file modes after all).  If I'm not
> >     mistaken, this change should not be very hard to make, although it may
> >     take some time to switch over all of the relevant drivers from using
> >     the mask.
>
> +1 +1 +1
>
> I don't think they are so many drivers using this mask. All the drivers
> tied with a OF initialization are not impacted as the change will be in
> one site.

There are a few.  I think around 50% of the
thermal_zone_device_register_with_trips() callers pass non-empty
writable trip points masks.

> > ---
> >   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(v) &&
>
>                               ^^^
>
> s/v/CONFIG_THERMAL_WRITABLE_TRIPS/ ?

Yes, and I'm not sure what happened here, because my local copy of the
patch is correct.

I'll send a v2 shortly.

> > +                 mask & (1 << indx)) {
> >                       tz->trip_hyst_attrs[indx].attr.attr.mode |= S_IWUSR;
> >                       tz->trip_hyst_attrs[indx].attr.store =
> >                                       trip_point_hyst_store;
> >
> >
> >
>
> --
Rafael J. Wysocki Jan. 31, 2024, 7:01 p.m. UTC | #3 
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@linaro.org> wrote:
> >
> > On 29/01/2024 21:40, Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > >
> > > 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@quicinc.com
> > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > ---
> > >
> > > 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
diff mbox series

Patch

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(v) &&
+		    mask & (1 << indx)) {
 			tz->trip_hyst_attrs[indx].attr.attr.mode |= S_IWUSR;
 			tz->trip_hyst_attrs[indx].attr.store =
 					trip_point_hyst_store;