[PATCH 0/8] Finish thermal zone structure encapsulation

[PATCH 0/8] Finish thermal zone structure encapsulation

[Daniel Lezcano]

The series are the last changes to self-encapsulate the thermal zone device
structure in order to protect wild accesses to the thermal zone device
internals, especially the trip points and the locks.

Meanwhile the latest changes, a new driver has come via the network tree which
directly accesses the thermal zone device structure information and uses the
old thermal_zone_device_register function. The first patch does the change to
use the accessor and the new registering function. It is expected to have the
changes to go through the thermal tree along with the other patches.

The second patch adds a macro to warn at compilation time if the
thermal_core.h is included in a file not belonging to the thermal core
code. One warning will happen with the nvidia drivers but this is in
the way to be solved.

The third patch reorders the headers inclusion in the core code.

The fourth patch provides an API to update the thermal trips of a
thermal zone. This function takes care of the locking and the thermal
zone update.

The next patches makes the int340x drivers to use the thermal trip
update above and the different accessors for thermal zone structure.

Finally the last patch moves the thermal zone device structure from
the exported thermal.h header to the thermal core private header.

Daniel Lezcano (8):
  net/mlx5: Update the driver with the recent thermal changes
  thermal/core: Hardening the self-encapsulation
  thermal/core: Reorder the headers inclusion
  thermal/core: Update the generic trip points
  thermal/drivers/int3400: Use thermal zone device wrappers
  thermal/drivers/int340x: Do not check the thermal zone state
  thermal/drivers/int340x: Use thermal zone device trip update
  thermal/core: Move the thermal zone structure to the private core
    header

 .../net/ethernet/mellanox/mlx5/core/thermal.c | 15 ++--
 drivers/thermal/gov_bang_bang.c               |  1 +
 drivers/thermal/gov_fair_share.c              |  1 +
 drivers/thermal/gov_power_allocator.c         |  7 +-
 drivers/thermal/gov_step_wise.c               |  1 +
 drivers/thermal/gov_user_space.c              |  1 +
 .../intel/int340x_thermal/int3400_thermal.c   | 44 +++++-----
 .../int340x_thermal/int340x_thermal_zone.c    | 17 ++--
 drivers/thermal/thermal_acpi.c                |  1 +
 drivers/thermal/thermal_core.c                | 47 +++++------
 drivers/thermal/thermal_core.h                | 82 +++++++++++++++++++
 drivers/thermal/thermal_helpers.c             |  1 +
 drivers/thermal/thermal_hwmon.c               |  1 +
 drivers/thermal/thermal_netlink.c             |  1 +
 drivers/thermal/thermal_of.c                  |  1 +
 drivers/thermal/thermal_sysfs.c               |  1 +
 drivers/thermal/thermal_trip.c                | 79 ++++++++++++++++++
 include/linux/thermal.h                       | 79 +-----------------
 18 files changed, 241 insertions(+), 139 deletions(-)

-- 
2.34.1

[PATCH 1/8] net/mlx5: Update the driver with the recent thermal changes

[Daniel Lezcano]

The thermal framework is migrating to the generic trip points. The set
of changes also implies a self-encapsulation of the thermal zone
device structure where the internals are no longer directly accessible
but with accessors.

Use the new API instead, so the next changes can be pushed in the
thermal framework without this driver failing to compile.

No functional changes intended.

Cc: Sandipan Patra <spatra@nvidia.com>
Cc: Gal Pressman <gal@nvidia.com>
Cc: Saeed Mahameed <saeedm@nvidia.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/net/ethernet/mellanox/mlx5/core/thermal.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/thermal.c b/drivers/net/ethernet/mellanox/mlx5/core/thermal.c
index e47fa6fb836f..20bb5eb266c1 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/thermal.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/thermal.c
@@ -45,7 +45,7 @@ static int mlx5_thermal_get_mtmp_temp(struct mlx5_core_dev *mdev, u32 id, int *p
 static int mlx5_thermal_get_temp(struct thermal_zone_device *tzdev,
 				 int *p_temp)
 {
-	struct mlx5_thermal *thermal = tzdev->devdata;
+	struct mlx5_thermal *thermal = thermal_zone_device_priv(tzdev);
 	struct mlx5_core_dev *mdev = thermal->mdev;
 	int err;
 
@@ -81,12 +81,13 @@ int mlx5_thermal_init(struct mlx5_core_dev *mdev)
 		return -ENOMEM;
 
 	thermal->mdev = mdev;
-	thermal->tzdev = thermal_zone_device_register(data,
-						      MLX5_THERMAL_NUM_TRIPS,
-						      MLX5_THERMAL_TRIP_MASK,
-						      thermal,
-						      &mlx5_thermal_ops,
-						      NULL, 0, MLX5_THERMAL_POLL_INT_MSEC);
+	thermal->tzdev = thermal_zone_device_register_with_trips(data,
+								 NULL,
+								 MLX5_THERMAL_NUM_TRIPS,
+								 MLX5_THERMAL_TRIP_MASK,
+								 thermal,
+								 &mlx5_thermal_ops,
+								 NULL, 0, MLX5_THERMAL_POLL_INT_MSEC);
 	if (IS_ERR(thermal->tzdev)) {
 		dev_err(mdev->device, "Failed to register thermal zone device (%s) %ld\n",
 			data, PTR_ERR(thermal->tzdev));
-- 
2.34.1

Re: [PATCH 1/8] net/mlx5: Update the driver with the recent thermal changes

[Simon Horman]

On Thu, May 25, 2023 at 04:01:28PM +0200, Daniel Lezcano wrote:
> The thermal framework is migrating to the generic trip points. The set
> of changes also implies a self-encapsulation of the thermal zone
> device structure where the internals are no longer directly accessible
> but with accessors.
> 
> Use the new API instead, so the next changes can be pushed in the
> thermal framework without this driver failing to compile.
> 
> No functional changes intended.
> 
> Cc: Sandipan Patra <spatra@nvidia.com>
> Cc: Gal Pressman <gal@nvidia.com>
> Cc: Saeed Mahameed <saeedm@nvidia.com>
> Cc: Jakub Kicinski <kuba@kernel.org>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

Reviewed-by: Simon Horman <simon.horman@corigine.com>

Re: [PATCH 1/8] net/mlx5: Update the driver with the recent thermal changes

[Saeed Mahameed]

On 25 May 16:01, Daniel Lezcano wrote:
>The thermal framework is migrating to the generic trip points. The set
>of changes also implies a self-encapsulation of the thermal zone
>device structure where the internals are no longer directly accessible
>but with accessors.
>
>Use the new API instead, so the next changes can be pushed in the
>thermal framework without this driver failing to compile.
>
>No functional changes intended.
>

I see this patch is part of a large series, do you expect me to re-post to
net-next or you are going to submit via another tree ? 

Re: [PATCH 1/8] net/mlx5: Update the driver with the recent thermal changes

[Daniel Lezcano]

On 01/06/2023 00:10, Saeed Mahameed wrote:
> On 25 May 16:01, Daniel Lezcano wrote:
>> The thermal framework is migrating to the generic trip points. The set
>> of changes also implies a self-encapsulation of the thermal zone
>> device structure where the internals are no longer directly accessible
>> but with accessors.
>>
>> Use the new API instead, so the next changes can be pushed in the
>> thermal framework without this driver failing to compile.
>>
>> No functional changes intended.
>>
> 
> I see this patch is part of a large series, do you expect me to re-post to
> net-next or you are going to submit via another tree ?

Sorry for the delay.

Yes, this patch is targeted for the thermal tree. The last patch of the 
series depends on it as it moves the thermal zone device structure to 
the private thermal header, thus the structure internals won't be 
accessible from this driver anymore.


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

[PATCH 2/8] thermal/core: Hardening the self-encapsulation

[Daniel Lezcano]

The thermal private header has leaked all around the drivers which
interacted with the core internals. The thermal zone structure which
was part of the exported header led also to a leakage of the fields
into the different drivers, making very difficult to improve the core
code without having to change the drivers.

Now we fixed how the thermal drivers were interacting with the thermal
zones (actually fixed how they should not interact). The thermal zone
structure has been moved to the private thermal core header. This
header has been removed from the different drivers and must belong to
the core code only. In order to prevent this private header to be
included again in the drivers, make explicit only the core code can
include this header by defining a THERMAL_CORE_SUBSYS macro. The
private header will contain a check against this macro.

The Tegra SoCtherm driver needs to access thermal_core.h to have the
get_thermal_instance() function definition. It is the only one
remaining driver which need to access the thermal_core.h header, so
the check will emit a warning at compilation time.

Thierry Reding is reworking the driver to get rid of this function [1]
and thus when the changes will be merged, the compilation warning will
be converted to a compilation error, closing definitively the door to
the drivers willing to play with the thermal zone device internals.

No functional changes intended.

[1] https://lore.kernel.org/all/20230414125721.1043589-1-thierry.reding@gmail.com/

Cc: Thierry Reding <thierry.reding@gmail.com>
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/thermal/gov_bang_bang.c       | 1 +
 drivers/thermal/gov_fair_share.c      | 1 +
 drivers/thermal/gov_power_allocator.c | 1 +
 drivers/thermal/gov_step_wise.c       | 1 +
 drivers/thermal/gov_user_space.c      | 1 +
 drivers/thermal/thermal_acpi.c        | 1 +
 drivers/thermal/thermal_core.c        | 1 +
 drivers/thermal/thermal_core.h        | 4 ++++
 drivers/thermal/thermal_helpers.c     | 1 +
 drivers/thermal/thermal_hwmon.c       | 1 +
 drivers/thermal/thermal_netlink.c     | 1 +
 drivers/thermal/thermal_of.c          | 1 +
 drivers/thermal/thermal_sysfs.c       | 1 +
 drivers/thermal/thermal_trip.c        | 1 +
 14 files changed, 17 insertions(+)

diff --git a/drivers/thermal/gov_bang_bang.c b/drivers/thermal/gov_bang_bang.c
index 1b121066521f..752c627075ba 100644
--- a/drivers/thermal/gov_bang_bang.c
+++ b/drivers/thermal/gov_bang_bang.c
@@ -11,6 +11,7 @@
 
 #include <linux/thermal.h>
 
+#define THERMAL_CORE_SUBSYS
 #include "thermal_core.h"
 
 static int thermal_zone_trip_update(struct thermal_zone_device *tz, int trip_id)
diff --git a/drivers/thermal/gov_fair_share.c b/drivers/thermal/gov_fair_share.c
index 03c2daeb6ee8..108cb5074594 100644
--- a/drivers/thermal/gov_fair_share.c
+++ b/drivers/thermal/gov_fair_share.c
@@ -13,6 +13,7 @@
 #include <linux/thermal.h>
 #include "thermal_trace.h"
 
+#define THERMAL_CORE_SUBSYS
 #include "thermal_core.h"
 
 /**
diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
index 8642f1096b91..d1c6ad92e5b4 100644
--- a/drivers/thermal/gov_power_allocator.c
+++ b/drivers/thermal/gov_power_allocator.c
@@ -14,6 +14,7 @@
 #define CREATE_TRACE_POINTS
 #include "thermal_trace_ipa.h"
 
+#define THERMAL_CORE_SUBSYS
 #include "thermal_core.h"
 
 #define INVALID_TRIP -1
diff --git a/drivers/thermal/gov_step_wise.c b/drivers/thermal/gov_step_wise.c
index 1050fb4d94c2..1c844004afe5 100644
--- a/drivers/thermal/gov_step_wise.c
+++ b/drivers/thermal/gov_step_wise.c
@@ -14,6 +14,7 @@
 #include <linux/minmax.h>
 #include "thermal_trace.h"
 
+#define THERMAL_CORE_SUBSYS
 #include "thermal_core.h"
 
 /*
diff --git a/drivers/thermal/gov_user_space.c b/drivers/thermal/gov_user_space.c
index 8bc1c22aaf03..8883c9ca930f 100644
--- a/drivers/thermal/gov_user_space.c
+++ b/drivers/thermal/gov_user_space.c
@@ -13,6 +13,7 @@
 #include <linux/slab.h>
 #include <linux/thermal.h>
 
+#define THERMAL_CORE_SUBSYS
 #include "thermal_core.h"
 
 static int user_space_bind(struct thermal_zone_device *tz)
diff --git a/drivers/thermal/thermal_acpi.c b/drivers/thermal/thermal_acpi.c
index 0e5698818f69..556c9f0cc40d 100644
--- a/drivers/thermal/thermal_acpi.c
+++ b/drivers/thermal/thermal_acpi.c
@@ -9,6 +9,7 @@
 #include <linux/acpi.h>
 #include <linux/units.h>
 
+#define THERMAL_CORE_SUBSYS
 #include "thermal_core.h"
 
 /*
diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index 842f678c1c3e..6bca97e27d59 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -24,6 +24,7 @@
 #define CREATE_TRACE_POINTS
 #include "thermal_trace.h"
 
+#define THERMAL_CORE_SUBSYS
 #include "thermal_core.h"
 #include "thermal_hwmon.h"
 
diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
index 3d4a787c6b28..84ada34ff079 100644
--- a/drivers/thermal/thermal_core.h
+++ b/drivers/thermal/thermal_core.h
@@ -14,6 +14,10 @@
 
 #include "thermal_netlink.h"
 
+#ifndef THERMAL_CORE_SUBSYS
+#warning This header can only be included by the thermal core code
+#endif
+
 /* Default Thermal Governor */
 #if defined(CONFIG_THERMAL_DEFAULT_GOV_STEP_WISE)
 #define DEFAULT_THERMAL_GOVERNOR       "step_wise"
diff --git a/drivers/thermal/thermal_helpers.c b/drivers/thermal/thermal_helpers.c
index cfba0965a22d..164c4627949e 100644
--- a/drivers/thermal/thermal_helpers.c
+++ b/drivers/thermal/thermal_helpers.c
@@ -19,6 +19,7 @@
 #include <linux/string.h>
 #include <linux/sysfs.h>
 
+#define THERMAL_CORE_SUBSYS
 #include "thermal_core.h"
 #include "thermal_trace.h"
 
diff --git a/drivers/thermal/thermal_hwmon.c b/drivers/thermal/thermal_hwmon.c
index fbe55509e307..3401258e55c6 100644
--- a/drivers/thermal/thermal_hwmon.c
+++ b/drivers/thermal/thermal_hwmon.c
@@ -17,6 +17,7 @@
 #include <linux/thermal.h>
 
 #include "thermal_hwmon.h"
+#define THERMAL_CORE_SUBSYS
 #include "thermal_core.h"
 
 /* hwmon sys I/F */
diff --git a/drivers/thermal/thermal_netlink.c b/drivers/thermal/thermal_netlink.c
index 08bc46c3ec7b..f3ac6432bf5f 100644
--- a/drivers/thermal/thermal_netlink.c
+++ b/drivers/thermal/thermal_netlink.c
@@ -11,6 +11,7 @@
 #include <net/genetlink.h>
 #include <uapi/linux/thermal.h>
 
+#define THERMAL_CORE_SUBSYS
 #include "thermal_core.h"
 
 static const struct genl_multicast_group thermal_genl_mcgrps[] = {
diff --git a/drivers/thermal/thermal_of.c b/drivers/thermal/thermal_of.c
index 6fb14e521197..0f2aca2d1dea 100644
--- a/drivers/thermal/thermal_of.c
+++ b/drivers/thermal/thermal_of.c
@@ -17,6 +17,7 @@
 #include <linux/types.h>
 #include <linux/string.h>
 
+#define THERMAL_CORE_SUBSYS
 #include "thermal_core.h"
 
 /***   functions parsing device tree nodes   ***/
diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
index 6c20c9f90a05..ccda2579ae82 100644
--- a/drivers/thermal/thermal_sysfs.c
+++ b/drivers/thermal/thermal_sysfs.c
@@ -19,6 +19,7 @@
 #include <linux/string.h>
 #include <linux/jiffies.h>
 
+#define THERMAL_CORE_SUBSYS
 #include "thermal_core.h"
 
 /* sys I/F for thermal zone */
diff --git a/drivers/thermal/thermal_trip.c b/drivers/thermal/thermal_trip.c
index 907f3a4d7bc8..61d927c35776 100644
--- a/drivers/thermal/thermal_trip.c
+++ b/drivers/thermal/thermal_trip.c
@@ -7,6 +7,7 @@
  *
  * Thermal trips handling
  */
+#define THERMAL_CORE_SUBSYS
 #include "thermal_core.h"
 
 int __for_each_thermal_trip(struct thermal_zone_device *tz,
-- 
2.34.1

[PATCH 3/8] thermal/core: Reorder the headers inclusion

[Daniel Lezcano]

The next changes will move the thermal device structure inside the
thermal core code. Consequently, the traces must be included after
thermal_core.h as this one contains the thermal zone device structure
definition the traces need.

Reorder the inclusions.

No functional changes intended.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/thermal/gov_power_allocator.c | 6 +++---
 drivers/thermal/thermal_core.c        | 6 +++---
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
index d1c6ad92e5b4..6056ed15460b 100644
--- a/drivers/thermal/gov_power_allocator.c
+++ b/drivers/thermal/gov_power_allocator.c
@@ -11,12 +11,12 @@
 #include <linux/slab.h>
 #include <linux/thermal.h>
 
-#define CREATE_TRACE_POINTS
-#include "thermal_trace_ipa.h"
-
 #define THERMAL_CORE_SUBSYS
 #include "thermal_core.h"
 
+#define CREATE_TRACE_POINTS
+#include "thermal_trace_ipa.h" 
+
 #define INVALID_TRIP -1
 
 #define FRAC_BITS 10
diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index 6bca97e27d59..afcd4197babd 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -21,13 +21,13 @@
 #include <linux/of.h>
 #include <linux/suspend.h>
 
-#define CREATE_TRACE_POINTS
-#include "thermal_trace.h"
-
 #define THERMAL_CORE_SUBSYS
 #include "thermal_core.h"
 #include "thermal_hwmon.h"
 
+#define CREATE_TRACE_POINTS
+#include "thermal_trace.h"
+
 static DEFINE_IDA(thermal_tz_ida);
 static DEFINE_IDA(thermal_cdev_ida);
 
-- 
2.34.1

[PATCH 4/8] thermal/core: Update the generic trip points

[Daniel Lezcano]

At this point, the generic trip points rework allows to create a
thermal zone with a fixed number of trip points. This usage satisfy
almost all of the existing drivers.

A few remaining drivers have a mechanism where the firmware updates
the trip points. But there is no such update mechanism for the generic
trip points, thus those drivers can not be converted to the generic
approach.

This patch provides a function 'thermal_zone_trips_update()' allowing
to change the trip points of a thermal zone.

At the same time, with the logic the trip points array is passed as a
parameter to the thermal zone at creation time, we make our own
private trip points array by copying the one passed as parameter.

Note, no code has been found where the trip points update leads to a
refresh of the trip points in sysfs, so it is very unlikey the number
of trip points changes. However, for the sake of consistency it would
be nicer to have the trip points being refreshed in sysfs also, but
that could be done in a separate set of changes.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/thermal/thermal_core.c | 40 ++++++++---------
 drivers/thermal/thermal_core.h |  3 ++
 drivers/thermal/thermal_trip.c | 78 ++++++++++++++++++++++++++++++++++
 include/linux/thermal.h        |  4 ++
 4 files changed, 102 insertions(+), 23 deletions(-)

diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index afcd4197babd..3688b06401c8 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -1224,32 +1224,11 @@ thermal_zone_device_register_with_trips(const char *type, struct thermal_trip *t
 		return ERR_PTR(-EINVAL);
 	}
 
-	/*
-	 * Max trip count can't exceed 31 as the "mask >> num_trips" condition.
-	 * For example, shifting by 32 will result in compiler warning:
-	 * warning: right shift count >= width of type [-Wshift-count- overflow]
-	 *
-	 * Also "mask >> num_trips" will always be true with 32 bit shift.
-	 * E.g. mask = 0x80000000 for trip id 31 to be RW. Then
-	 * mask >> 32 = 0x80000000
-	 * This will result in failure for the below condition.
-	 *
-	 * Check will be true when the bit 31 of the mask is set.
-	 * 32 bit shift will cause overflow of 4 byte integer.
-	 */
-	if (num_trips > (BITS_PER_TYPE(int) - 1) || num_trips < 0 || mask >> num_trips) {
-		pr_err("Incorrect number of thermal trips\n");
-		return ERR_PTR(-EINVAL);
-	}
-
 	if (!ops) {
 		pr_err("Thermal zone device ops not defined\n");
 		return ERR_PTR(-EINVAL);
 	}
 
-	if (num_trips > 0 && (!ops->get_trip_type || !ops->get_trip_temp) && !trips)
-		return ERR_PTR(-EINVAL);
-
 	if (!thermal_class)
 		return ERR_PTR(-ENODEV);
 
@@ -1283,8 +1262,22 @@ thermal_zone_device_register_with_trips(const char *type, struct thermal_trip *t
 	tz->ops = ops;
 	tz->device.class = thermal_class;
 	tz->devdata = devdata;
-	tz->trips = trips;
-	tz->num_trips = num_trips;
+
+	if (trips) {
+		result = __thermal_zone_trips_update(tz, trips, num_trips, mask);
+		if (result)
+			goto remove_id;
+	} else {
+		/*
+		 * Legacy trip point handling
+		 */
+		if ((!tz->ops->get_trip_type || !tz->ops->get_trip_temp) && num_trips) {
+			result = -EINVAL;
+			goto remove_id;
+		}
+
+		tz->num_trips = num_trips;
+	}
 
 	thermal_set_delay_jiffies(&tz->passive_delay_jiffies, passive_delay);
 	thermal_set_delay_jiffies(&tz->polling_delay_jiffies, polling_delay);
@@ -1451,6 +1444,7 @@ void thermal_zone_device_unregister(struct thermal_zone_device *tz)
 	mutex_unlock(&tz->lock);
 
 	kfree(tz->tzp);
+	kfree(tz->trips);
 
 	put_device(&tz->device);
 
diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
index 84ada34ff079..c27a9930f904 100644
--- a/drivers/thermal/thermal_core.h
+++ b/drivers/thermal/thermal_core.h
@@ -125,6 +125,9 @@ void __thermal_zone_device_update(struct thermal_zone_device *tz,
 void __thermal_zone_set_trips(struct thermal_zone_device *tz);
 int __thermal_zone_get_trip(struct thermal_zone_device *tz, int trip_id,
 			    struct thermal_trip *trip);
+int __thermal_zone_trips_update(struct thermal_zone_device *tz,
+				struct thermal_trip *trips,
+				int num_trips, int mask);
 int __thermal_zone_get_temp(struct thermal_zone_device *tz, int *temp);
 
 /* sysfs I/F */
diff --git a/drivers/thermal/thermal_trip.c b/drivers/thermal/thermal_trip.c
index 61d927c35776..171b1902c01c 100644
--- a/drivers/thermal/thermal_trip.c
+++ b/drivers/thermal/thermal_trip.c
@@ -7,6 +7,8 @@
  *
  * Thermal trips handling
  */
+#include <linux/slab.h>
+
 #define THERMAL_CORE_SUBSYS
 #include "thermal_core.h"
 
@@ -181,3 +183,79 @@ int thermal_zone_set_trip(struct thermal_zone_device *tz, int trip_id,
 
 	return 0;
 }
+
+int __thermal_zone_trips_update(struct thermal_zone_device *tz, struct thermal_trip *trips,
+				int num_trips, int mask)
+{
+	struct thermal_trip *new_trips;
+
+	/*
+	 * Legacy trip point handling is incompatible with this
+	 * function
+	 */
+	if (tz->ops->get_trip_type || tz->ops->get_trip_temp) {
+		pr_err("Legacy trip points use incompatible function '%s'\n", __func__);
+		return -ENOSYS;
+	}
+	
+	/*
+	 * Max trip count can't exceed 31 as the "mask >> num_trips" condition.
+	 * For example, shifting by 32 will result in compiler warning:
+	 * warning: right shift count >= width of type [-Wshift-count- overflow]
+	 *
+	 * Also "mask >> num_trips" will always be true with 32 bit shift.
+	 * E.g. mask = 0x80000000 for trip id 31 to be RW. Then
+	 * mask >> 32 = 0x80000000
+	 * This will result in failure for the below condition.
+	 *
+	 * Check will be true when the bit 31 of the mask is set.
+	 * 32 bit shift will cause overflow of 4 byte integer.
+	 */
+	if (num_trips > (BITS_PER_TYPE(int) - 1) || num_trips < 0 || mask >> num_trips) {
+		pr_err("Incorrect number of thermal trips\n");
+		return -EINVAL;
+	}
+
+	/*
+	 * New generic trip point handling
+	 */
+	if (num_trips > 0 && !trips) {
+		pr_err("Inconsistent parameters\n");
+		return -EINVAL;
+	}
+
+	/*
+	 * Allocate our private trip points array structure
+	 */
+	new_trips = kmemdup(trips, sizeof(*trips) * num_trips, GFP_KERNEL);
+	if (!new_trips)
+		return -ENOMEM;
+	
+	/*
+	 * Newly allocated thermal zone will have the 'trips' field
+	 * NULL, kfree() is immune against that
+	 */
+	kfree(tz->trips);
+	tz->trips = new_trips;
+	tz->num_trips = num_trips;
+
+	return 0;
+}
+
+int thermal_zone_trips_update(struct thermal_zone_device *tz, struct thermal_trip *trips,
+			      int num_trips, int mask)
+{
+	int ret;
+
+	mutex_lock(&tz->lock);
+	ret = __thermal_zone_trips_update(tz, trips, num_trips, mask);
+	mutex_unlock(&tz->lock);
+
+	if (ret)
+		return ret;
+
+	__thermal_zone_device_update(tz, THERMAL_TRIP_CHANGED);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(thermal_zone_trips_update);
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index 87837094d549..83937256a01c 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -291,6 +291,10 @@ int thermal_zone_get_num_trips(struct thermal_zone_device *tz);
 
 int thermal_zone_get_crit_temp(struct thermal_zone_device *tz, int *temp);
 
+int thermal_zone_trips_update(struct thermal_zone_device *tz,
+			      struct thermal_trip *trips,
+			      int num_trips, int mask);
+
 #ifdef CONFIG_THERMAL_ACPI
 int thermal_acpi_active_trip_temp(struct acpi_device *adev, int id, int *ret_temp);
 int thermal_acpi_passive_trip_temp(struct acpi_device *adev, int *ret_temp);
-- 
2.34.1

Re: [PATCH 4/8] thermal/core: Update the generic trip points

[Rafael J. Wysocki]

On Thu, May 25, 2023 at 4:02 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> At this point, the generic trip points rework allows to create a
> thermal zone with a fixed number of trip points. This usage satisfy
> almost all of the existing drivers.
>
> A few remaining drivers have a mechanism where the firmware updates
> the trip points. But there is no such update mechanism for the generic
> trip points, thus those drivers can not be converted to the generic
> approach.
>
> This patch provides a function 'thermal_zone_trips_update()' allowing
> to change the trip points of a thermal zone.
>
> At the same time, with the logic the trip points array is passed as a
> parameter to the thermal zone at creation time, we make our own
> private trip points array by copying the one passed as parameter.

So the design seems to require the caller to create a new array of
trip points and pass it to thermal_zone_trips_update(), so it can
replace the zone's trips array with it.

If only one trip point changes and there are multiple defined, this is
rather not efficient.

Do you want to prevent the core from using stale trip points this way?
 If so, it should be stated here.

Moreover, at least in the cases when num_trips doesn't change, it
might be more efficient to walk the new trips[] array and only copy
the ones that have changed over their old versions.

I am also not sure if this interface is going to be convenient from
the user's perspective, especially if the trips get sorted by the core
(in the future).  They would need to recreate the entire trips array
every time from scratch, even if only one trip point changes, which
means quite a bit of churn for thermal zones with many trip points.

It might be better to allow them to update trips in place and notify
the core about the change, all under the zone lock to prevent the core
from using trips simultaneously.

And arguably, changing num_trips would be questionable due to the
sysfs consistency reasons mentioned below.

> Note, no code has been found where the trip points update leads to a
> refresh of the trip points in sysfs, so it is very unlikey the number
> of trip points changes. However, for the sake of consistency it would
> be nicer to have the trip points being refreshed in sysfs also, but
> that could be done in a separate set of changes.

So at this point user space has already enumerated the trip points, so
it may fail if some of them go away or it may not be able to use any
new trip points appearing in sysfs.

For this reason, until there is a way to notify user space about the
need to re-enumerate trip points (and user space indicates support for
it), the only trip point property that may change in sysfs is the
temperature.

> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  drivers/thermal/thermal_core.c | 40 ++++++++---------
>  drivers/thermal/thermal_core.h |  3 ++
>  drivers/thermal/thermal_trip.c | 78 ++++++++++++++++++++++++++++++++++
>  include/linux/thermal.h        |  4 ++
>  4 files changed, 102 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index afcd4197babd..3688b06401c8 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -1224,32 +1224,11 @@ thermal_zone_device_register_with_trips(const char *type, struct thermal_trip *t
>                 return ERR_PTR(-EINVAL);
>         }
>
> -       /*
> -        * Max trip count can't exceed 31 as the "mask >> num_trips" condition.
> -        * For example, shifting by 32 will result in compiler warning:
> -        * warning: right shift count >= width of type [-Wshift-count- overflow]
> -        *
> -        * Also "mask >> num_trips" will always be true with 32 bit shift.
> -        * E.g. mask = 0x80000000 for trip id 31 to be RW. Then
> -        * mask >> 32 = 0x80000000
> -        * This will result in failure for the below condition.
> -        *
> -        * Check will be true when the bit 31 of the mask is set.
> -        * 32 bit shift will cause overflow of 4 byte integer.
> -        */
> -       if (num_trips > (BITS_PER_TYPE(int) - 1) || num_trips < 0 || mask >> num_trips) {
> -               pr_err("Incorrect number of thermal trips\n");
> -               return ERR_PTR(-EINVAL);
> -       }
> -
>         if (!ops) {
>                 pr_err("Thermal zone device ops not defined\n");
>                 return ERR_PTR(-EINVAL);
>         }
>
> -       if (num_trips > 0 && (!ops->get_trip_type || !ops->get_trip_temp) && !trips)
> -               return ERR_PTR(-EINVAL);
> -
>         if (!thermal_class)
>                 return ERR_PTR(-ENODEV);
>
> @@ -1283,8 +1262,22 @@ thermal_zone_device_register_with_trips(const char *type, struct thermal_trip *t
>         tz->ops = ops;
>         tz->device.class = thermal_class;
>         tz->devdata = devdata;
> -       tz->trips = trips;
> -       tz->num_trips = num_trips;
> +
> +       if (trips) {
> +               result = __thermal_zone_trips_update(tz, trips, num_trips, mask);
> +               if (result)
> +                       goto remove_id;
> +       } else {
> +               /*
> +                * Legacy trip point handling
> +                */
> +               if ((!tz->ops->get_trip_type || !tz->ops->get_trip_temp) && num_trips) {
> +                       result = -EINVAL;
> +                       goto remove_id;
> +               }
> +
> +               tz->num_trips = num_trips;
> +       }
>
>         thermal_set_delay_jiffies(&tz->passive_delay_jiffies, passive_delay);
>         thermal_set_delay_jiffies(&tz->polling_delay_jiffies, polling_delay);
> @@ -1451,6 +1444,7 @@ void thermal_zone_device_unregister(struct thermal_zone_device *tz)
>         mutex_unlock(&tz->lock);
>
>         kfree(tz->tzp);
> +       kfree(tz->trips);
>
>         put_device(&tz->device);
>
> diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
> index 84ada34ff079..c27a9930f904 100644
> --- a/drivers/thermal/thermal_core.h
> +++ b/drivers/thermal/thermal_core.h
> @@ -125,6 +125,9 @@ void __thermal_zone_device_update(struct thermal_zone_device *tz,
>  void __thermal_zone_set_trips(struct thermal_zone_device *tz);
>  int __thermal_zone_get_trip(struct thermal_zone_device *tz, int trip_id,
>                             struct thermal_trip *trip);
> +int __thermal_zone_trips_update(struct thermal_zone_device *tz,
> +                               struct thermal_trip *trips,
> +                               int num_trips, int mask);
>  int __thermal_zone_get_temp(struct thermal_zone_device *tz, int *temp);
>
>  /* sysfs I/F */
> diff --git a/drivers/thermal/thermal_trip.c b/drivers/thermal/thermal_trip.c
> index 61d927c35776..171b1902c01c 100644
> --- a/drivers/thermal/thermal_trip.c
> +++ b/drivers/thermal/thermal_trip.c
> @@ -7,6 +7,8 @@
>   *
>   * Thermal trips handling
>   */
> +#include <linux/slab.h>
> +
>  #define THERMAL_CORE_SUBSYS
>  #include "thermal_core.h"
>
> @@ -181,3 +183,79 @@ int thermal_zone_set_trip(struct thermal_zone_device *tz, int trip_id,
>
>         return 0;
>  }
> +
> +int __thermal_zone_trips_update(struct thermal_zone_device *tz, struct thermal_trip *trips,
> +                               int num_trips, int mask)
> +{
> +       struct thermal_trip *new_trips;
> +
> +       /*
> +        * Legacy trip point handling is incompatible with this
> +        * function
> +        */
> +       if (tz->ops->get_trip_type || tz->ops->get_trip_temp) {
> +               pr_err("Legacy trip points use incompatible function '%s'\n", __func__);
> +               return -ENOSYS;
> +       }
> +
> +       /*
> +        * Max trip count can't exceed 31 as the "mask >> num_trips" condition.
> +        * For example, shifting by 32 will result in compiler warning:
> +        * warning: right shift count >= width of type [-Wshift-count- overflow]
> +        *
> +        * Also "mask >> num_trips" will always be true with 32 bit shift.
> +        * E.g. mask = 0x80000000 for trip id 31 to be RW. Then
> +        * mask >> 32 = 0x80000000
> +        * This will result in failure for the below condition.
> +        *
> +        * Check will be true when the bit 31 of the mask is set.
> +        * 32 bit shift will cause overflow of 4 byte integer.
> +        */
> +       if (num_trips > (BITS_PER_TYPE(int) - 1) || num_trips < 0 || mask >> num_trips) {
> +               pr_err("Incorrect number of thermal trips\n");
> +               return -EINVAL;
> +       }
> +
> +       /*
> +        * New generic trip point handling
> +        */
> +       if (num_trips > 0 && !trips) {
> +               pr_err("Inconsistent parameters\n");
> +               return -EINVAL;
> +       }
> +
> +       /*
> +        * Allocate our private trip points array structure
> +        */
> +       new_trips = kmemdup(trips, sizeof(*trips) * num_trips, GFP_KERNEL);
> +       if (!new_trips)
> +               return -ENOMEM;
> +
> +       /*
> +        * Newly allocated thermal zone will have the 'trips' field
> +        * NULL, kfree() is immune against that
> +        */
> +       kfree(tz->trips);
> +       tz->trips = new_trips;
> +       tz->num_trips = num_trips;
> +
> +       return 0;
> +}
> +
> +int thermal_zone_trips_update(struct thermal_zone_device *tz, struct thermal_trip *trips,
> +                             int num_trips, int mask)
> +{
> +       int ret;
> +
> +       mutex_lock(&tz->lock);
> +       ret = __thermal_zone_trips_update(tz, trips, num_trips, mask);
> +       mutex_unlock(&tz->lock);
> +
> +       if (ret)
> +               return ret;
> +
> +       __thermal_zone_device_update(tz, THERMAL_TRIP_CHANGED);

This is called under tz->lock in other places AFAICS.  Why not here?

> +
> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(thermal_zone_trips_update);
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index 87837094d549..83937256a01c 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -291,6 +291,10 @@ int thermal_zone_get_num_trips(struct thermal_zone_device *tz);
>
>  int thermal_zone_get_crit_temp(struct thermal_zone_device *tz, int *temp);
>
> +int thermal_zone_trips_update(struct thermal_zone_device *tz,
> +                             struct thermal_trip *trips,
> +                             int num_trips, int mask);
> +
>  #ifdef CONFIG_THERMAL_ACPI
>  int thermal_acpi_active_trip_temp(struct acpi_device *adev, int id, int *ret_temp);
>  int thermal_acpi_passive_trip_temp(struct acpi_device *adev, int *ret_temp);
> --

Re: [PATCH 4/8] thermal/core: Update the generic trip points

[Daniel Lezcano]

Hi Rafael,

thanks for the comments


On 20/06/2023 13:28, Rafael J. Wysocki wrote:
> On Thu, May 25, 2023 at 4:02 PM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
>>
>> At this point, the generic trip points rework allows to create a
>> thermal zone with a fixed number of trip points. This usage satisfy
>> almost all of the existing drivers.
>>
>> A few remaining drivers have a mechanism where the firmware updates
>> the trip points. But there is no such update mechanism for the generic
>> trip points, thus those drivers can not be converted to the generic
>> approach.
>>
>> This patch provides a function 'thermal_zone_trips_update()' allowing
>> to change the trip points of a thermal zone.
>>
>> At the same time, with the logic the trip points array is passed as a
>> parameter to the thermal zone at creation time, we make our own
>> private trip points array by copying the one passed as parameter.
> 
> So the design seems to require the caller to create a new array of
> trip points and pass it to thermal_zone_trips_update(), so it can
> replace the zone's trips array with it.
> 
> If only one trip point changes and there are multiple defined, this is
> rather not efficient.

This update is only for replacing the current trip array when one or 
several trip points are added or removed. We can see that in one or two 
drivers only.

This function is supposed to be called rarely (and I doubt there is 
really a lot of hardware sending notification to add/remove trip points).

For changing a trip point property like its temperature or its 
hysteresis, we keep the usual set_trip_point() function.

> Do you want to prevent the core from using stale trip points this way?
>   If so, it should be stated here.

No, that will be a side effect. We can put this point apart, it will be 
addressed in a cleanup series after everything is converted to the 
generic trip points.


> Moreover, at least in the cases when num_trips doesn't change, it
> might be more efficient to walk the new trips[] array and only copy
> the ones that have changed over their old versions.

IMO, that is over-engineered, especially for dedicating this 
optimization for a very few drivers and ultra rare usages.


> I am also not sure if this interface is going to be convenient from
> the user's perspective, especially if the trips get sorted by the core
> (in the future).  They would need to recreate the entire trips array
> every time from scratch, even if only one trip point changes, which
> means quite a bit of churn for thermal zones with many trip points.

Actually, the driver is not supposed to deal with the array. It can 
create the array on the stack, pass it to the 
thermal_zone_device_register_with_trips() function and forget about it.

The trip points array should not be used by a driver anymore.


> It might be better to allow them to update trips in place and notify
> the core about the change, all under the zone lock to prevent the core
> from using trips simultaneously.

I'm not sure to understand. The core code is called with this function 
and takes the lock.

> And arguably, changing num_trips would be questionable due to the
> sysfs consistency reasons mentioned below.

[ ... ]

>> Note, no code has been found where the trip points update leads to a
>> refresh of the trip points in sysfs, so it is very unlikey the number
>> of trip points changes. However, for the sake of consistency it would
>> be nicer to have the trip points being refreshed in sysfs also, but
>> that could be done in a separate set of changes.
> 
> So at this point user space has already enumerated the trip points, so
> it may fail if some of them go away or it may not be able to use any
> new trip points appearing in sysfs.

Yes, that is why I think the adding/removal of the trip points was never 
really supported. I would be very curious to see a platform with such a 
feature.

> For this reason, until there is a way to notify user space about the
> need to re-enumerate trip points (and user space indicates support for
> it), the only trip point property that may change in sysfs is the
> temperature.

The userspace can be notified when there is a change with:

THERMAL_GENL_EVENT_TZ_TRIP_CHANGE
THERMAL_GENL_EVENT_TZ_TRIP_ADD
THERMAL_GENL_EVENT_TZ_TRIP_DELETE

The last two ones are not implemented today but that could be done after 
as that would be a new feature.

Let me summarize the situation:

  - The trip point crossing events are not correctly detected because of 
how they are handled and we need to sort them out.

  - In order to sort them out, we need to convert the drivers to the 
generic trip point and remove all those get_trip_* | set_trip_* ops

  - Almost all the drivers are converted except the ACPI thermal and the 
intel_soc_dts_iosf drivers which are blocking the feature

  - The ACPI thermal driver can potentially add or remove a trip point 
but we are not sure that can happen

  - We need to decorrelate the trip id and the array index for the ACPI 
thermal driver

The generic trip points change is a big chunk of work and I would like 
to have some progress to fix the trip crossing detection along with the 
removal of the resulting dead code.

Given there may not be a real usage of the thermal trip number update, 
can we stay simple and keep the proposed change but forcing the same 
number of trip points ?

We can then improve the existing code if it is really needed


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

Re: [PATCH 4/8] thermal/core: Update the generic trip points

[Rafael J. Wysocki]

On Tue, Jun 20, 2023 at 8:35 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
>
> Hi Rafael,
>
> thanks for the comments
>
>
> On 20/06/2023 13:28, Rafael J. Wysocki wrote:
> > On Thu, May 25, 2023 at 4:02 PM Daniel Lezcano
> > <daniel.lezcano@linaro.org> wrote:
> >>
> >> At this point, the generic trip points rework allows to create a
> >> thermal zone with a fixed number of trip points. This usage satisfy
> >> almost all of the existing drivers.
> >>
> >> A few remaining drivers have a mechanism where the firmware updates
> >> the trip points. But there is no such update mechanism for the generic
> >> trip points, thus those drivers can not be converted to the generic
> >> approach.
> >>
> >> This patch provides a function 'thermal_zone_trips_update()' allowing
> >> to change the trip points of a thermal zone.
> >>
> >> At the same time, with the logic the trip points array is passed as a
> >> parameter to the thermal zone at creation time, we make our own
> >> private trip points array by copying the one passed as parameter.
> >
> > So the design seems to require the caller to create a new array of
> > trip points and pass it to thermal_zone_trips_update(), so it can
> > replace the zone's trips array with it.
> >
> > If only one trip point changes and there are multiple defined, this is
> > rather not efficient.
>
> This update is only for replacing the current trip array when one or
> several trip points are added or removed. We can see that in one or two
> drivers only.

Well, I'm not sure about this adding/removing idea, because of the
sysfs issue mentioned below.

Surely, some trip points can be made invalid by setting their
temperatures to the "invalid" value, but they cannot be removed,
because they are already represented in sysfs and may have been
enumerated by user space which will get confused by removing them.

Conversely, if new trip points are added, they will not appear in
sysfs and so user space will never become aware of them.

> This function is supposed to be called rarely (and I doubt there is
> really a lot of hardware sending notification to add/remove trip points).
>
> For changing a trip point property like its temperature or its
> hysteresis, we keep the usual set_trip_point() function.

Do you mean the ->set_trip_temp/hyst() zone callback?

But isn't that to be used by the core in order to notify the driver of
a change, rather than the other way around?

Here, we have a situation where the driver needs to let the core know
that something has changed.

> > Do you want to prevent the core from using stale trip points this way?
> >   If so, it should be stated here.
>
> No, that will be a side effect. We can put this point apart, it will be
> addressed in a cleanup series after everything is converted to the
> generic trip points.

I think we need to talk about the design.  It looks like you have one
in mind, but I'm not really sure what it is.

>
> > Moreover, at least in the cases when num_trips doesn't change, it
> > might be more efficient to walk the new trips[] array and only copy
> > the ones that have changed over their old versions.
>
> IMO, that is over-engineered, especially for dedicating this
> optimization for a very few drivers and ultra rare usages.

I'm not sure.

One thing to remember is that, in general, the core and the drivers
need to communicate both ways regarding trip points.  For example,
when a trip point is updated via sysfs, the core needs to tell the
driver about that.  Conversely, if a trip point is updated by the
platform firmware (say), the driver needs to tell the core about that.

> > I am also not sure if this interface is going to be convenient from
> > the user's perspective, especially if the trips get sorted by the core
> > (in the future).  They would need to recreate the entire trips array
> > every time from scratch, even if only one trip point changes, which
> > means quite a bit of churn for thermal zones with many trip points.
>
> Actually, the driver is not supposed to deal with the array. It can
> create the array on the stack, pass it to the
> thermal_zone_device_register_with_trips() function and forget about it.
>
> The trip points array should not be used by a driver anymore.

Well, say that the core invokes ->set_trip_temp() for a certain trip
point.  How is the driver going to know which trip point this is about
if it cannot access the trip points table in the zone?

> > It might be better to allow them to update trips in place and notify
> > the core about the change, all under the zone lock to prevent the core
> > from using trips simultaneously.
>
> I'm not sure to understand. The core code is called with this function
> and takes the lock.
>
> > And arguably, changing num_trips would be questionable due to the
> > sysfs consistency reasons mentioned below.
>
> [ ... ]
>
> >> Note, no code has been found where the trip points update leads to a
> >> refresh of the trip points in sysfs, so it is very unlikey the number
> >> of trip points changes. However, for the sake of consistency it would
> >> be nicer to have the trip points being refreshed in sysfs also, but
> >> that could be done in a separate set of changes.
> >
> > So at this point user space has already enumerated the trip points, so
> > it may fail if some of them go away or it may not be able to use any
> > new trip points appearing in sysfs.
>
> Yes, that is why I think the adding/removal of the trip points was never
> really supported. I would be very curious to see a platform with such a
> feature.
>
> > For this reason, until there is a way to notify user space about the
> > need to re-enumerate trip points (and user space indicates support for
> > it), the only trip point property that may change in sysfs is the
> > temperature.
>
> The userspace can be notified when there is a change with:
>
> THERMAL_GENL_EVENT_TZ_TRIP_CHANGE
> THERMAL_GENL_EVENT_TZ_TRIP_ADD
> THERMAL_GENL_EVENT_TZ_TRIP_DELETE
>
> The last two ones are not implemented today but that could be done after
> as that would be a new feature.
>
> Let me summarize the situation:
>
>   - The trip point crossing events are not correctly detected because of
> how they are handled and we need to sort them out.

Sure.

>   - In order to sort them out, we need to convert the drivers to the
> generic trip point and remove all those get_trip_* | set_trip_* ops

IIUC, there are cases when the trip point temperature may be set via
sysfs.  In some of them, the core needs to tell the driver what
temperature to set for the trip, so the hardware or the platform
notifies it of excursions.  The ->set_* callbacks are still going to
be needed for that if I'm not mistaken.

>   - Almost all the drivers are converted except the ACPI thermal and the
> intel_soc_dts_iosf drivers which are blocking the feature

Sorry about that.  Hopefully, I'll have some time to sort this out
during the next couple of weeks.

I still would like to discuss the design related to that, though.

>   - The ACPI thermal driver can potentially add or remove a trip point
> but we are not sure that can happen

It may as per the spec, but whether or not this case needs to be
handled is a good question.

This depends on whether or not user space is prepared to deal with
trip points appearing and going away.  I suspect that it isn't.

>   - We need to decorrelate the trip id and the array index for the ACPI
> thermal driver

That should be doable.

> The generic trip points change is a big chunk of work and I would like
> to have some progress to fix the trip crossing detection along with the
> removal of the resulting dead code.
>
> Given there may not be a real usage of the thermal trip number update,
> can we stay simple and keep the proposed change but forcing the same
> number of trip points ?

I suppose so.

> We can then improve the existing code if it is really needed

Re: [PATCH 4/8] thermal/core: Update the generic trip points

[Rafael J. Wysocki]

On Thu, May 25, 2023 at 4:02 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> At this point, the generic trip points rework allows to create a
> thermal zone with a fixed number of trip points. This usage satisfy
> almost all of the existing drivers.
>
> A few remaining drivers have a mechanism where the firmware updates
> the trip points. But there is no such update mechanism for the generic
> trip points, thus those drivers can not be converted to the generic
> approach.
>
> This patch provides a function 'thermal_zone_trips_update()' allowing
> to change the trip points of a thermal zone.
>
> At the same time, with the logic the trip points array is passed as a
> parameter to the thermal zone at creation time, we make our own
> private trip points array by copying the one passed as parameter.
>
> Note, no code has been found where the trip points update leads to a
> refresh of the trip points in sysfs, so it is very unlikey the number
> of trip points changes. However, for the sake of consistency it would
> be nicer to have the trip points being refreshed in sysfs also, but
> that could be done in a separate set of changes.
>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  drivers/thermal/thermal_core.c | 40 ++++++++---------
>  drivers/thermal/thermal_core.h |  3 ++
>  drivers/thermal/thermal_trip.c | 78 ++++++++++++++++++++++++++++++++++
>  include/linux/thermal.h        |  4 ++
>  4 files changed, 102 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index afcd4197babd..3688b06401c8 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -1224,32 +1224,11 @@ thermal_zone_device_register_with_trips(const char *type, struct thermal_trip *t
>                 return ERR_PTR(-EINVAL);
>         }
>
> -       /*
> -        * Max trip count can't exceed 31 as the "mask >> num_trips" condition.
> -        * For example, shifting by 32 will result in compiler warning:
> -        * warning: right shift count >= width of type [-Wshift-count- overflow]
> -        *
> -        * Also "mask >> num_trips" will always be true with 32 bit shift.
> -        * E.g. mask = 0x80000000 for trip id 31 to be RW. Then
> -        * mask >> 32 = 0x80000000
> -        * This will result in failure for the below condition.
> -        *
> -        * Check will be true when the bit 31 of the mask is set.
> -        * 32 bit shift will cause overflow of 4 byte integer.
> -        */
> -       if (num_trips > (BITS_PER_TYPE(int) - 1) || num_trips < 0 || mask >> num_trips) {
> -               pr_err("Incorrect number of thermal trips\n");
> -               return ERR_PTR(-EINVAL);
> -       }
> -
>         if (!ops) {
>                 pr_err("Thermal zone device ops not defined\n");
>                 return ERR_PTR(-EINVAL);
>         }
>
> -       if (num_trips > 0 && (!ops->get_trip_type || !ops->get_trip_temp) && !trips)
> -               return ERR_PTR(-EINVAL);
> -
>         if (!thermal_class)
>                 return ERR_PTR(-ENODEV);
>
> @@ -1283,8 +1262,22 @@ thermal_zone_device_register_with_trips(const char *type, struct thermal_trip *t
>         tz->ops = ops;
>         tz->device.class = thermal_class;
>         tz->devdata = devdata;
> -       tz->trips = trips;
> -       tz->num_trips = num_trips;
> +
> +       if (trips) {
> +               result = __thermal_zone_trips_update(tz, trips, num_trips, mask);
> +               if (result)
> +                       goto remove_id;
> +       } else {
> +               /*
> +                * Legacy trip point handling
> +                */
> +               if ((!tz->ops->get_trip_type || !tz->ops->get_trip_temp) && num_trips) {
> +                       result = -EINVAL;
> +                       goto remove_id;
> +               }
> +
> +               tz->num_trips = num_trips;
> +       }

Lest I forget, if I'm not mistaken, the above change would break the
int3403 driver that uses int340x_thermal_update_trips() to update trip
points in int3403_notify(), which handles notifications from the
platform firmware, and it updates them through the driver's private
pointer to the trips array used by the core with the assumption that
the core will notice the changes.

So it looks like at least this particular driver would need to be
updated before the $subject patch can be applied.

That said, I think that the ACPI thermal driver won't really need to
access the trips array after passing it to
thermal_zone_device_register_with_trips() and it may create a new
trips table every time the trip points are updated by the platform
firmware, but I'm not convinced about this design.

[PATCH 5/8] thermal/drivers/int3400: Use thermal zone device wrappers

[Daniel Lezcano]

The driver is accessing the thermal zone device structure but the
accessors are already existing and we want to consolidate the thermal
core code by preventing accesses to the internals from the drivers.

Let's use these accessors.

On the other side, the code is getting directly the temperature from
tz->temperature, but the temperature is a faked on, so we can replace
this access by the fake temp and remove the thermal zone device
structure access.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 .../thermal/intel/int340x_thermal/int3400_thermal.c  | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
index 810231b59dcd..66e34241b33a 100644
--- a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
+++ b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
@@ -15,6 +15,7 @@
 #define INT3400_THERMAL_TABLE_CHANGED 0x83
 #define INT3400_ODVP_CHANGED 0x88
 #define INT3400_KEEP_ALIVE 0xA0
+#define INT3400_FAKE_TEMP (20 * 1000) /* faked temp sensor with 20C */
 
 enum int3400_thermal_uuid {
 	INT3400_THERMAL_ACTIVE = 0,
@@ -453,6 +454,7 @@ static void int3400_notify(acpi_handle handle,
 			void *data)
 {
 	struct int3400_thermal_priv *priv = data;
+	struct device *dev;
 	char *thermal_prop[5];
 	int therm_event;
 
@@ -475,12 +477,14 @@ static void int3400_notify(acpi_handle handle,
 		return;
 	}
 
-	thermal_prop[0] = kasprintf(GFP_KERNEL, "NAME=%s", priv->thermal->type);
-	thermal_prop[1] = kasprintf(GFP_KERNEL, "TEMP=%d", priv->thermal->temperature);
+	dev = thermal_zone_device(priv->thermal);
+	
+	thermal_prop[0] = kasprintf(GFP_KERNEL, "NAME=%s", thermal_zone_device_type(priv->thermal));
+	thermal_prop[1] = kasprintf(GFP_KERNEL, "TEMP=%d", INT3400_FAKE_TEMP);
 	thermal_prop[2] = kasprintf(GFP_KERNEL, "TRIP=");
 	thermal_prop[3] = kasprintf(GFP_KERNEL, "EVENT=%d", therm_event);
 	thermal_prop[4] = NULL;
-	kobject_uevent_env(&priv->thermal->device.kobj, KOBJ_CHANGE, thermal_prop);
+	kobject_uevent_env(&dev->kobj, KOBJ_CHANGE, thermal_prop);
 	kfree(thermal_prop[0]);
 	kfree(thermal_prop[1]);
 	kfree(thermal_prop[2]);
@@ -490,7 +494,7 @@ static void int3400_notify(acpi_handle handle,
 static int int3400_thermal_get_temp(struct thermal_zone_device *thermal,
 			int *temp)
 {
-	*temp = 20 * 1000; /* faked temp sensor with 20C */
+	*temp = INT3400_FAKE_TEMP;
 	return 0;
 }
 
-- 
2.34.1

Re: [PATCH 5/8] thermal/drivers/int3400: Use thermal zone device wrappers

[Daniel Lezcano]

Hi Srinivas,

do you agree with the changes in patches 5 and 6 ?

Thanks

   -- Daniel


On 25/05/2023 16:01, Daniel Lezcano wrote:
> The driver is accessing the thermal zone device structure but the
> accessors are already existing and we want to consolidate the thermal
> core code by preventing accesses to the internals from the drivers.
> 
> Let's use these accessors.
> 
> On the other side, the code is getting directly the temperature from
> tz->temperature, but the temperature is a faked on, so we can replace
> this access by the fake temp and remove the thermal zone device
> structure access.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>   .../thermal/intel/int340x_thermal/int3400_thermal.c  | 12 ++++++++----
>   1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
> index 810231b59dcd..66e34241b33a 100644
> --- a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
> +++ b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
> @@ -15,6 +15,7 @@
>   #define INT3400_THERMAL_TABLE_CHANGED 0x83
>   #define INT3400_ODVP_CHANGED 0x88
>   #define INT3400_KEEP_ALIVE 0xA0
> +#define INT3400_FAKE_TEMP (20 * 1000) /* faked temp sensor with 20C */
>   
>   enum int3400_thermal_uuid {
>   	INT3400_THERMAL_ACTIVE = 0,
> @@ -453,6 +454,7 @@ static void int3400_notify(acpi_handle handle,
>   			void *data)
>   {
>   	struct int3400_thermal_priv *priv = data;
> +	struct device *dev;
>   	char *thermal_prop[5];
>   	int therm_event;
>   
> @@ -475,12 +477,14 @@ static void int3400_notify(acpi_handle handle,
>   		return;
>   	}
>   
> -	thermal_prop[0] = kasprintf(GFP_KERNEL, "NAME=%s", priv->thermal->type);
> -	thermal_prop[1] = kasprintf(GFP_KERNEL, "TEMP=%d", priv->thermal->temperature);
> +	dev = thermal_zone_device(priv->thermal);
> +	
> +	thermal_prop[0] = kasprintf(GFP_KERNEL, "NAME=%s", thermal_zone_device_type(priv->thermal));
> +	thermal_prop[1] = kasprintf(GFP_KERNEL, "TEMP=%d", INT3400_FAKE_TEMP);
>   	thermal_prop[2] = kasprintf(GFP_KERNEL, "TRIP=");
>   	thermal_prop[3] = kasprintf(GFP_KERNEL, "EVENT=%d", therm_event);
>   	thermal_prop[4] = NULL;
> -	kobject_uevent_env(&priv->thermal->device.kobj, KOBJ_CHANGE, thermal_prop);
> +	kobject_uevent_env(&dev->kobj, KOBJ_CHANGE, thermal_prop);
>   	kfree(thermal_prop[0]);
>   	kfree(thermal_prop[1]);
>   	kfree(thermal_prop[2]);
> @@ -490,7 +494,7 @@ static void int3400_notify(acpi_handle handle,
>   static int int3400_thermal_get_temp(struct thermal_zone_device *thermal,
>   			int *temp)
>   {
> -	*temp = 20 * 1000; /* faked temp sensor with 20C */
> +	*temp = INT3400_FAKE_TEMP;
>   	return 0;
>   }
>   

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

Re: [PATCH 5/8] thermal/drivers/int3400: Use thermal zone device wrappers

[srinivas pandruvada]

Hi Daniel,

On Mon, 2023-07-03 at 12:49 +0200, Daniel Lezcano wrote:
> 
> Hi Srinivas,
> 
> do you agree with the changes in patches 5 and 6 ?
> 
> Thanks
> 
>    -- Daniel
> 
> 
> On 25/05/2023 16:01, Daniel Lezcano wrote:
> > The driver is accessing the thermal zone device structure but the
> > accessors are already existing and we want to consolidate the
> > thermal
> > core code by preventing accesses to the internals from the drivers.
> > 
> > Let's use these accessors.
> > 
> > On the other side, the code is getting directly the temperature
> > from
> > tz->temperature, but the temperature is a faked on, so we can
> > replace
> > this access by the fake temp and remove the thermal zone device
> > structure access.
> > 
May be something simple description like this will be enough.

"
Use thermal core API to access thermal zone "type" field instead of
directly using the structure field.
While here, remove access to temperature field, as this driver is
reporting fake temperature, which can be replaced with
INT3400_FAKE_TEMP. Also replace hardcoded 20C with INT3400_FAKE_TEMP.
"

The change itself looks fine.

Thanks,
Srinivas

> > 

> > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> > ---
> >   .../thermal/intel/int340x_thermal/int3400_thermal.c  | 12
> > ++++++++----
> >   1 file changed, 8 insertions(+), 4 deletions(-)
> > 
> > diff --git
> > a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
> > b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
> > index 810231b59dcd..66e34241b33a 100644
> > --- a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
> > +++ b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
> > @@ -15,6 +15,7 @@
> >   #define INT3400_THERMAL_TABLE_CHANGED 0x83
> >   #define INT3400_ODVP_CHANGED 0x88
> >   #define INT3400_KEEP_ALIVE 0xA0
> > +#define INT3400_FAKE_TEMP (20 * 1000) /* faked temp sensor with
> > 20C */
> >   
> >   enum int3400_thermal_uuid {
> >         INT3400_THERMAL_ACTIVE = 0,
> > @@ -453,6 +454,7 @@ static void int3400_notify(acpi_handle handle,
> >                         void *data)
> >   {
> >         struct int3400_thermal_priv *priv = data;
> > +       struct device *dev;
> >         char *thermal_prop[5];
> >         int therm_event;
> >   
> > @@ -475,12 +477,14 @@ static void int3400_notify(acpi_handle
> > handle,
> >                 return;
> >         }
> >   
> > -       thermal_prop[0] = kasprintf(GFP_KERNEL, "NAME=%s", priv-
> > >thermal->type);
> > -       thermal_prop[1] = kasprintf(GFP_KERNEL, "TEMP=%d", priv-
> > >thermal->temperature);
> > +       dev = thermal_zone_device(priv->thermal);
> > +       
> > +       thermal_prop[0] = kasprintf(GFP_KERNEL, "NAME=%s",
> > thermal_zone_device_type(priv->thermal));
> > +       thermal_prop[1] = kasprintf(GFP_KERNEL, "TEMP=%d",
> > INT3400_FAKE_TEMP);
> >         thermal_prop[2] = kasprintf(GFP_KERNEL, "TRIP=");
> >         thermal_prop[3] = kasprintf(GFP_KERNEL, "EVENT=%d",
> > therm_event);
> >         thermal_prop[4] = NULL;
> > -       kobject_uevent_env(&priv->thermal->device.kobj,
> > KOBJ_CHANGE, thermal_prop);
> > +       kobject_uevent_env(&dev->kobj, KOBJ_CHANGE, thermal_prop);
> >         kfree(thermal_prop[0]);
> >         kfree(thermal_prop[1]);
> >         kfree(thermal_prop[2]);
> > @@ -490,7 +494,7 @@ static void int3400_notify(acpi_handle handle,
> >   static int int3400_thermal_get_temp(struct thermal_zone_device
> > *thermal,
> >                         int *temp)
> >   {
> > -       *temp = 20 * 1000; /* faked temp sensor with 20C */
> > +       *temp = INT3400_FAKE_TEMP;
> >         return 0;
> >   }
> >   
> 

Re: [PATCH 5/8] thermal/drivers/int3400: Use thermal zone device wrappers

[Daniel Lezcano]

Hi Srinivas,

thanks for your answer. What about the patch 6?


On 03/07/2023 18:15, srinivas pandruvada wrote:
> Hi Daniel,
> 
> On Mon, 2023-07-03 at 12:49 +0200, Daniel Lezcano wrote:
>>
>> Hi Srinivas,
>>
>> do you agree with the changes in patches 5 and 6 ?
>>
>> Thanks
>>
>>     -- Daniel
>>
>>
>> On 25/05/2023 16:01, Daniel Lezcano wrote:
>>> The driver is accessing the thermal zone device structure but the
>>> accessors are already existing and we want to consolidate the
>>> thermal
>>> core code by preventing accesses to the internals from the drivers.
>>>
>>> Let's use these accessors.
>>>
>>> On the other side, the code is getting directly the temperature
>>> from
>>> tz->temperature, but the temperature is a faked on, so we can
>>> replace
>>> this access by the fake temp and remove the thermal zone device
>>> structure access.
>>>
> May be something simple description like this will be enough.
> 
> "
> Use thermal core API to access thermal zone "type" field instead of
> directly using the structure field.
> While here, remove access to temperature field, as this driver is
> reporting fake temperature, which can be replaced with
> INT3400_FAKE_TEMP. Also replace hardcoded 20C with INT3400_FAKE_TEMP.
> "
> 
> The change itself looks fine.


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

Re: [PATCH 5/8] thermal/drivers/int3400: Use thermal zone device wrappers

[srinivas pandruvada]

Hi Daniel,

On Wed, 2023-07-05 at 12:41 +0200, Daniel Lezcano wrote:
> 
> Hi Srinivas,
> 
> thanks for your answer. What about the patch 6?
I was not CCed. But checked from LKML, 
The change looks good.

Thanks,
Srinivas

> 
> 
> On 03/07/2023 18:15, srinivas pandruvada wrote:
> > Hi Daniel,
> > 
> > On Mon, 2023-07-03 at 12:49 +0200, Daniel Lezcano wrote:
> > > 
> > > Hi Srinivas,
> > > 
> > > do you agree with the changes in patches 5 and 6 ?
> > > 
> > > Thanks
> > > 
> > >     -- Daniel
> > > 
> > > 
> > > On 25/05/2023 16:01, Daniel Lezcano wrote:
> > > > The driver is accessing the thermal zone device structure but
> > > > the
> > > > accessors are already existing and we want to consolidate the
> > > > thermal
> > > > core code by preventing accesses to the internals from the
> > > > drivers.
> > > > 
> > > > Let's use these accessors.
> > > > 
> > > > On the other side, the code is getting directly the temperature
> > > > from
> > > > tz->temperature, but the temperature is a faked on, so we can
> > > > replace
> > > > this access by the fake temp and remove the thermal zone device
> > > > structure access.
> > > > 
> > May be something simple description like this will be enough.
> > 
> > "
> > Use thermal core API to access thermal zone "type" field instead of
> > directly using the structure field.
> > While here, remove access to temperature field, as this driver is
> > reporting fake temperature, which can be replaced with
> > INT3400_FAKE_TEMP. Also replace hardcoded 20C with
> > INT3400_FAKE_TEMP.
> > "
> > 
> > The change itself looks fine.
> 
> 

Re: [PATCH 5/8] thermal/drivers/int3400: Use thermal zone device wrappers

[Daniel Lezcano]

On 05/07/2023 13:35, srinivas pandruvada wrote:
> Hi Daniel,
> 
> On Wed, 2023-07-05 at 12:41 +0200, Daniel Lezcano wrote:
>>
>> Hi Srinivas,
>>
>> thanks for your answer. What about the patch 6?
> I was not CCed. But checked from LKML,
> The change looks good.

Actually, you were Cc'ed, the mail may have been lost somehow.

Anyway, thanks for the review. I'll resend with the changelog fixed in 
patch 5 and your acked-by

   -- Daniel

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

[PATCH 6/8] thermal/drivers/int340x: Do not check the thermal zone state

[Daniel Lezcano]

The driver is accessing the thermal zone state to ensure the state is
different from the one we want to set.

We don't want the driver to access the thermal zone device internals.

Actually, the thermal core code already checks if the thermal zone's
state is different before calling this function, thus this check is
duplicate.

Remove it.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 .../intel/int340x_thermal/int3400_thermal.c   | 32 ++++++++-----------
 1 file changed, 14 insertions(+), 18 deletions(-)

diff --git a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
index 66e34241b33a..cba636d6784d 100644
--- a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
+++ b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
@@ -503,32 +503,28 @@ static int int3400_thermal_change_mode(struct thermal_zone_device *thermal,
 {
 	struct int3400_thermal_priv *priv = thermal_zone_device_priv(thermal);
 	int result = 0;
+	int enabled;
 
 	if (!priv)
 		return -EINVAL;
 
-	if (mode != thermal->mode) {
-		int enabled;
+	enabled = mode == THERMAL_DEVICE_ENABLED;
 
-		enabled = mode == THERMAL_DEVICE_ENABLED;
-
-		if (priv->os_uuid_mask) {
-			if (!enabled) {
-				priv->os_uuid_mask = 0;
-				result = set_os_uuid_mask(priv, priv->os_uuid_mask);
-			}
-			goto eval_odvp;
+	if (priv->os_uuid_mask) {
+		if (!enabled) {
+			priv->os_uuid_mask = 0;
+			result = set_os_uuid_mask(priv, priv->os_uuid_mask);
 		}
-
-		if (priv->current_uuid_index < 0 ||
-		    priv->current_uuid_index >= INT3400_THERMAL_MAXIMUM_UUID)
-			return -EINVAL;
-
-		result = int3400_thermal_run_osc(priv->adev->handle,
-						 int3400_thermal_uuids[priv->current_uuid_index],
-						 &enabled);
+		goto eval_odvp;
 	}
 
+	if (priv->current_uuid_index < 0 ||
+	    priv->current_uuid_index >= INT3400_THERMAL_MAXIMUM_UUID)
+		return -EINVAL;
+
+	result = int3400_thermal_run_osc(priv->adev->handle,
+					 int3400_thermal_uuids[priv->current_uuid_index],
+					 &enabled);
 eval_odvp:
 	evaluate_odvp(priv);
 
-- 
2.34.1

[PATCH 7/8] thermal/drivers/int340x: Use thermal zone device trip update

[Daniel Lezcano]

The current code takes the thermal zone device lock to update the
thermal trips.

Now we have an API allowing to update the thermal trip directly, so
use it and get ride of the thermal zone lock access.

Take also the opportunity to use the zone>-type accessor.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 .../int340x_thermal/int340x_thermal_zone.c      | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.c b/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.c
index 89cf007146ea..94697063f41e 100644
--- a/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.c
+++ b/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.c
@@ -58,7 +58,8 @@ static int int340x_thermal_set_trip_temp(struct thermal_zone_device *zone,
 
 static void int340x_thermal_critical(struct thermal_zone_device *zone)
 {
-	dev_dbg(&zone->device, "%s: critical temperature reached\n", zone->type);
+	dev_dbg(thermal_zone_device(zone), "%s: critical temperature reached\n",
+		thermal_zone_device_type(zone));
 }
 
 static struct thermal_zone_device_ops int340x_thermal_zone_ops = {
@@ -215,13 +216,15 @@ EXPORT_SYMBOL_GPL(int340x_thermal_zone_remove);
 void int340x_thermal_update_trips(struct int34x_thermal_zone *int34x_zone)
 {
 	struct acpi_device *zone_adev = int34x_zone->adev;
-	struct thermal_trip *zone_trips = int34x_zone->trips;
-	int trip_cnt = int34x_zone->zone->num_trips;
+	struct thermal_trip *zone_trips;
+	int trip_cnt = thermal_zone_get_num_trips(int34x_zone->zone);
 	int act_trip_nr = 0;
 	int i;
 
-	mutex_lock(&int34x_zone->zone->lock);
-
+	zone_trips = kmemdup(int34x_zone->trips, sizeof(*zone_trips) * trip_cnt, GFP_KERNEL);
+	if (!zone_trips)
+		return;
+	
 	for (i = int34x_zone->aux_trip_nr; i < trip_cnt; i++) {
 		int temp, err;
 
@@ -250,7 +253,9 @@ void int340x_thermal_update_trips(struct int34x_thermal_zone *int34x_zone)
 		zone_trips[i].temperature = temp;
 	}
 
-	mutex_unlock(&int34x_zone->zone->lock);
+	if (!thermal_zone_trips_update(int34x_zone->zone, zone_trips, trip_cnt,
+				       GENMASK_ULL((trip_cnt) - 1, 0)))
+		int34x_zone->trips = zone_trips;
 }
 EXPORT_SYMBOL_GPL(int340x_thermal_update_trips);
 
-- 
2.34.1

[PATCH 8/8] thermal/core: Move the thermal zone structure to the private core header

[Daniel Lezcano]

The code in the different drivers have been sanitized and they no
longer access the thermal zone structure internals. This one is just a
pointer passed around to the thermal core code as a handler.

Move the structure definition to the private core code header to
self-encapsulate the core code.

No functional changes intented.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/thermal/thermal_core.h | 75 ++++++++++++++++++++++++++++++++++
 include/linux/thermal.h        | 75 ----------------------------------
 2 files changed, 75 insertions(+), 75 deletions(-)

diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
index c27a9930f904..b93386640799 100644
--- a/drivers/thermal/thermal_core.h
+++ b/drivers/thermal/thermal_core.h
@@ -18,6 +18,81 @@
 #warning This header can only be included by the thermal core code
 #endif
 
+/**
+ * struct thermal_zone_device - structure for a thermal zone
+ * @id:		unique id number for each thermal zone
+ * @type:	the thermal zone device type
+ * @device:	&struct device for this thermal zone
+ * @trip_temp_attrs:	attributes for trip points for sysfs: trip temperature
+ * @trip_type_attrs:	attributes for trip points for sysfs: trip type
+ * @trip_hyst_attrs:	attributes for trip points for sysfs: trip hysteresis
+ * @mode:		current mode of this thermal zone
+ * @devdata:	private pointer for device private data
+ * @trips:	an array of struct thermal_trip
+ * @num_trips:	number of trip points the thermal zone supports
+ * @trips_disabled;	bitmap for disabled trips
+ * @passive_delay_jiffies: number of jiffies to wait between polls when
+ *			performing passive cooling.
+ * @polling_delay_jiffies: number of jiffies to wait between polls when
+ *			checking whether trip points have been crossed (0 for
+ *			interrupt driven systems)
+ * @temperature:	current temperature.  This is only for core code,
+ *			drivers should use thermal_zone_get_temp() to get the
+ *			current temperature
+ * @last_temperature:	previous temperature read
+ * @emul_temperature:	emulated temperature when using CONFIG_THERMAL_EMULATION
+ * @passive:		1 if you've crossed a passive trip point, 0 otherwise.
+ * @prev_low_trip:	the low current temperature if you've crossed a passive
+			trip point.
+ * @prev_high_trip:	the above current temperature if you've crossed a
+			passive trip point.
+ * @need_update:	if equals 1, thermal_zone_device_update needs to be invoked.
+ * @ops:	operations this &thermal_zone_device supports
+ * @tzp:	thermal zone parameters
+ * @governor:	pointer to the governor for this thermal zone
+ * @governor_data:	private pointer for governor data
+ * @thermal_instances:	list of &struct thermal_instance of this thermal zone
+ * @ida:	&struct ida to generate unique id for this zone's cooling
+ *		devices
+ * @lock:	lock to protect thermal_instances list
+ * @node:	node in thermal_tz_list (in thermal_core.c)
+ * @poll_queue:	delayed work for polling
+ * @notify_event: Last notification event
+ */
+struct thermal_zone_device {
+	int id;
+	char type[THERMAL_NAME_LENGTH];
+	struct device device;
+	struct attribute_group trips_attribute_group;
+	struct thermal_attr *trip_temp_attrs;
+	struct thermal_attr *trip_type_attrs;
+	struct thermal_attr *trip_hyst_attrs;
+	enum thermal_device_mode mode;
+	void *devdata;
+	struct thermal_trip *trips;
+	int num_trips;
+	unsigned long trips_disabled;	/* bitmap for disabled trips */
+	unsigned long passive_delay_jiffies;
+	unsigned long polling_delay_jiffies;
+	int temperature;
+	int last_temperature;
+	int emul_temperature;
+	int passive;
+	int prev_low_trip;
+	int prev_high_trip;
+	atomic_t need_update;
+	struct thermal_zone_device_ops *ops;
+	struct thermal_zone_params *tzp;
+	struct thermal_governor *governor;
+	void *governor_data;
+	struct list_head thermal_instances;
+	struct ida ida;
+	struct mutex lock;
+	struct list_head node;
+	struct delayed_work poll_queue;
+	enum thermal_notify_event notify_event;
+};
+
 /* Default Thermal Governor */
 #if defined(CONFIG_THERMAL_DEFAULT_GOV_STEP_WISE)
 #define DEFAULT_THERMAL_GOVERNOR       "step_wise"
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index 83937256a01c..0ea2da5c33ac 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -112,81 +112,6 @@ struct thermal_cooling_device {
 	struct list_head node;
 };
 
-/**
- * struct thermal_zone_device - structure for a thermal zone
- * @id:		unique id number for each thermal zone
- * @type:	the thermal zone device type
- * @device:	&struct device for this thermal zone
- * @trip_temp_attrs:	attributes for trip points for sysfs: trip temperature
- * @trip_type_attrs:	attributes for trip points for sysfs: trip type
- * @trip_hyst_attrs:	attributes for trip points for sysfs: trip hysteresis
- * @mode:		current mode of this thermal zone
- * @devdata:	private pointer for device private data
- * @trips:	an array of struct thermal_trip
- * @num_trips:	number of trip points the thermal zone supports
- * @trips_disabled;	bitmap for disabled trips
- * @passive_delay_jiffies: number of jiffies to wait between polls when
- *			performing passive cooling.
- * @polling_delay_jiffies: number of jiffies to wait between polls when
- *			checking whether trip points have been crossed (0 for
- *			interrupt driven systems)
- * @temperature:	current temperature.  This is only for core code,
- *			drivers should use thermal_zone_get_temp() to get the
- *			current temperature
- * @last_temperature:	previous temperature read
- * @emul_temperature:	emulated temperature when using CONFIG_THERMAL_EMULATION
- * @passive:		1 if you've crossed a passive trip point, 0 otherwise.
- * @prev_low_trip:	the low current temperature if you've crossed a passive
-			trip point.
- * @prev_high_trip:	the above current temperature if you've crossed a
-			passive trip point.
- * @need_update:	if equals 1, thermal_zone_device_update needs to be invoked.
- * @ops:	operations this &thermal_zone_device supports
- * @tzp:	thermal zone parameters
- * @governor:	pointer to the governor for this thermal zone
- * @governor_data:	private pointer for governor data
- * @thermal_instances:	list of &struct thermal_instance of this thermal zone
- * @ida:	&struct ida to generate unique id for this zone's cooling
- *		devices
- * @lock:	lock to protect thermal_instances list
- * @node:	node in thermal_tz_list (in thermal_core.c)
- * @poll_queue:	delayed work for polling
- * @notify_event: Last notification event
- */
-struct thermal_zone_device {
-	int id;
-	char type[THERMAL_NAME_LENGTH];
-	struct device device;
-	struct attribute_group trips_attribute_group;
-	struct thermal_attr *trip_temp_attrs;
-	struct thermal_attr *trip_type_attrs;
-	struct thermal_attr *trip_hyst_attrs;
-	enum thermal_device_mode mode;
-	void *devdata;
-	struct thermal_trip *trips;
-	int num_trips;
-	unsigned long trips_disabled;	/* bitmap for disabled trips */
-	unsigned long passive_delay_jiffies;
-	unsigned long polling_delay_jiffies;
-	int temperature;
-	int last_temperature;
-	int emul_temperature;
-	int passive;
-	int prev_low_trip;
-	int prev_high_trip;
-	atomic_t need_update;
-	struct thermal_zone_device_ops *ops;
-	struct thermal_zone_params *tzp;
-	struct thermal_governor *governor;
-	void *governor_data;
-	struct list_head thermal_instances;
-	struct ida ida;
-	struct mutex lock;
-	struct list_head node;
-	struct delayed_work poll_queue;
-	enum thermal_notify_event notify_event;
-};
-
 /**
  * struct thermal_governor - structure that holds thermal governor information
  * @name:	name of the governor
-- 
2.34.1

Re: [PATCH 0/8] Finish thermal zone structure encapsulation

[Daniel Lezcano]

Hi Rafael,

On 25/05/2023 16:01, Daniel Lezcano wrote:
> The series are the last changes to self-encapsulate the thermal zone device
> structure in order to protect wild accesses to the thermal zone device
> internals, especially the trip points and the locks.
> 
> Meanwhile the latest changes, a new driver has come via the network tree which
> directly accesses the thermal zone device structure information and uses the
> old thermal_zone_device_register function. The first patch does the change to
> use the accessor and the new registering function. It is expected to have the
> changes to go through the thermal tree along with the other patches.
> 
> The second patch adds a macro to warn at compilation time if the
> thermal_core.h is included in a file not belonging to the thermal core
> code. One warning will happen with the nvidia drivers but this is in
> the way to be solved.
> 
> The third patch reorders the headers inclusion in the core code.
> 
> The fourth patch provides an API to update the thermal trips of a
> thermal zone. This function takes care of the locking and the thermal
> zone update.
> 
> The next patches makes the int340x drivers to use the thermal trip
> update above and the different accessors for thermal zone structure.
> 
> Finally the last patch moves the thermal zone device structure from
> the exported thermal.h header to the thermal core private header.

I think we are all pretty busy for this cycle and that series did not 
raised any comments (yet?).

I would like to go forward with the generic trip points changes and 
finish the conversion so we can fix the trip points monitoring once and 
for all. This series consolidate the self-encapsulation of the thermal 
zone device to prevent changing the trip points directly in the thermal 
zone device structure.

Most of the patches of this series don't have functional changes.

If nobody has concern with it, I propose to pick them up in the next days.

> Daniel Lezcano (8):
>    net/mlx5: Update the driver with the recent thermal changes
>    thermal/core: Hardening the self-encapsulation
>    thermal/core: Reorder the headers inclusion
>    thermal/core: Update the generic trip points
>    thermal/drivers/int3400: Use thermal zone device wrappers
>    thermal/drivers/int340x: Do not check the thermal zone state
>    thermal/drivers/int340x: Use thermal zone device trip update
>    thermal/core: Move the thermal zone structure to the private core
>      header
> 
>   .../net/ethernet/mellanox/mlx5/core/thermal.c | 15 ++--
>   drivers/thermal/gov_bang_bang.c               |  1 +
>   drivers/thermal/gov_fair_share.c              |  1 +
>   drivers/thermal/gov_power_allocator.c         |  7 +-
>   drivers/thermal/gov_step_wise.c               |  1 +
>   drivers/thermal/gov_user_space.c              |  1 +
>   .../intel/int340x_thermal/int3400_thermal.c   | 44 +++++-----
>   .../int340x_thermal/int340x_thermal_zone.c    | 17 ++--
>   drivers/thermal/thermal_acpi.c                |  1 +
>   drivers/thermal/thermal_core.c                | 47 +++++------
>   drivers/thermal/thermal_core.h                | 82 +++++++++++++++++++
>   drivers/thermal/thermal_helpers.c             |  1 +
>   drivers/thermal/thermal_hwmon.c               |  1 +
>   drivers/thermal/thermal_netlink.c             |  1 +
>   drivers/thermal/thermal_of.c                  |  1 +
>   drivers/thermal/thermal_sysfs.c               |  1 +
>   drivers/thermal/thermal_trip.c                | 79 ++++++++++++++++++
>   include/linux/thermal.h                       | 79 +-----------------
>   18 files changed, 241 insertions(+), 139 deletions(-)
> 

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

Re: [PATCH 0/8] Finish thermal zone structure encapsulation

[Rafael J. Wysocki]

Hi Daniel,

Sorry for the delay.

On Tue, Jun 13, 2023 at 4:12 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
>
> Hi Rafael,
>
> On 25/05/2023 16:01, Daniel Lezcano wrote:
> > The series are the last changes to self-encapsulate the thermal zone device
> > structure in order to protect wild accesses to the thermal zone device
> > internals, especially the trip points and the locks.
> >
> > Meanwhile the latest changes, a new driver has come via the network tree which
> > directly accesses the thermal zone device structure information and uses the
> > old thermal_zone_device_register function. The first patch does the change to
> > use the accessor and the new registering function. It is expected to have the
> > changes to go through the thermal tree along with the other patches.
> >
> > The second patch adds a macro to warn at compilation time if the
> > thermal_core.h is included in a file not belonging to the thermal core
> > code. One warning will happen with the nvidia drivers but this is in
> > the way to be solved.
> >
> > The third patch reorders the headers inclusion in the core code.
> >
> > The fourth patch provides an API to update the thermal trips of a
> > thermal zone. This function takes care of the locking and the thermal
> > zone update.
> >
> > The next patches makes the int340x drivers to use the thermal trip
> > update above and the different accessors for thermal zone structure.
> >
> > Finally the last patch moves the thermal zone device structure from
> > the exported thermal.h header to the thermal core private header.
>
> I think we are all pretty busy for this cycle and that series did not
> raised any comments (yet?).
>
> I would like to go forward with the generic trip points changes and
> finish the conversion so we can fix the trip points monitoring once and
> for all. This series consolidate the self-encapsulation of the thermal
> zone device to prevent changing the trip points directly in the thermal
> zone device structure.
>
> Most of the patches of this series don't have functional changes.
>
> If nobody has concern with it, I propose to pick them up in the next days.

Unfortunately, it is not perfect.

I have no objections to the first 3 patches, so please feel free to
add ACKs from me to those.

However, patch [4/8] is questionable IMV, so please drop it for now.
I'll send my comments to it separately later today.

All of the int340* patches need ACKs from Srinivas, who was not
included in the CC of the original submission AFAICS, before they can
be pushed to linux-next.