From: Rafael J. Wysocki <[email protected]>
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/[email protected]
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
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(-)
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;
On 29/01/2024 21:40, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> 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/[email protected]
> Signed-off-by: Rafael J. Wysocki <[email protected]>
> ---
>
> 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;
>
>
>
--
<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
On Wed, Jan 31, 2024 at 7:18 PM Daniel Lezcano
<[email protected]> wrote:
>
> On 29/01/2024 21:40, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <[email protected]>
> >
> > 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/[email protected]
> > Signed-off-by: Rafael J. Wysocki <[email protected]>
> > ---
> >
> > 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;
> >
> >
> >
>
> --
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
> <[email protected]> wrote:
> >
> > On 29/01/2024 21:40, Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki <[email protected]>
> > >
> > > 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/[email protected]
> > > Signed-off-by: Rafael J. Wysocki <[email protected]>
> > > ---
> > >
> > > 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