Received-SPF: pass (google.com: domain of linux-kernel+bounces-46998-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
From: "Rafael J. Wysocki" <rjw@rjwysocki.net>
To: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>,
 Linux PM <linux-pm@vger.kernel.org>, LKML <linux-kernel@vger.kernel.org>,
 Lukasz Luba <lukasz.luba@arm.com>, Zhang Rui <rui.zhang@intel.com>,
 Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>,
 Manaf Meethalavalappu Pallikunhi <quic_manafm@quicinc.com>
Subject:
 Re: [PATCH v1] thermal: sysfs: Make trip hysteresis writable along with trip
 temperature
Date: Wed, 31 Jan 2024 20:01:53 +0100
Message-ID: <12386481.O9o76ZdvQC@kreacher>
In-Reply-To:
 <CAJZ5v0iZ0hyPYB3i6YdbiKueHGWoM3i6mPBnzGL9bB8wFxVSPw@mail.gmail.com>
References:
 <4565526.LvFx2qVVIh@kreacher>
 <7caf2f4d-d0d5-4622-b290-bb0396547f3c@linaro.org>
 <CAJZ5v0iZ0hyPYB3i6YdbiKueHGWoM3i6mPBnzGL9bB8wFxVSPw@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset="UTF-8"

On Wednesday, January 31, 2024 7:41:52 PM CET Rafael J. Wysocki wrote:
> On Wed, Jan 31, 2024 at 7:18=E2=80=AFPM 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_ma=
nafm@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 tr=
ip
> > >     mask used during registration will have any effect and this is qu=
ite
> > >     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.
>=20
> 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):

=2D--
 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
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
=2D-- 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.
=20
=2Dconfig THERMAL_WRITABLE_TRIPS
=2D	bool "Enable writable trip points"
=2D	help
=2D	  This option allows the system integrator to choose whether
=2D	  trip temperatures can be changed from userspace. The
=2D	  writable trips need to be specified when setting up the
=2D	  thermal zone but the choice here takes precedence.
=2D
=2D	  Say 'Y' here if you would like to allow userspace tools to
=2D	  change trip temperatures.
=2D
 choice
 	prompt "Default Thermal governor"
 	default THERMAL_DEFAULT_GOV_STEP_WISE
Index: linux-pm/drivers/thermal/thermal_sysfs.c
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
=2D-- 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 =3D S_IRUGO;
 		tz->trip_temp_attrs[indx].attr.show =3D trip_point_temp_show;
=2D		if (IS_ENABLED(CONFIG_THERMAL_WRITABLE_TRIPS) &&
=2D		    mask & (1 << indx)) {
+		if (mask & (1 << indx)) {
 			tz->trip_temp_attrs[indx].attr.attr.mode |=3D S_IWUSR;
 			tz->trip_temp_attrs[indx].attr.store =3D
 							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 =3D S_IRUGO;
 		tz->trip_hyst_attrs[indx].attr.show =3D trip_point_hyst_show;
=2D		if (IS_ENABLED(CONFIG_THERMAL_WRITABLE_TRIPS) &&
=2D		    mask & (1 << indx)) {
+		if (mask & (1 << indx)) {
 			tz->trip_hyst_attrs[indx].attr.attr.mode |=3D S_IWUSR;
 			tz->trip_hyst_attrs[indx].attr.store =3D
 					trip_point_hyst_store;
Index: linux-pm/drivers/thermal/intel/Kconfig
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
=2D-- 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
=2D	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
=2D	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
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
=2D-- 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=3Dy
 CONFIG_SENSORS_PWM_FAN=3Dy
 CONFIG_SENSORS_SY7636A=3Dy
 CONFIG_THERMAL_STATISTICS=3Dy
=2DCONFIG_THERMAL_WRITABLE_TRIPS=3Dy
 CONFIG_CPU_THERMAL=3Dy
 CONFIG_IMX_THERMAL=3Dy
 CONFIG_WATCHDOG=3Dy