[PATCH v1 0/3] iwlwifi: mvm: Thermal management fixes

[PATCH v1 0/3] iwlwifi: mvm: Thermal management fixes

[Rafael J. Wysocki]

Hi Everyone,

There are a few thermal management shortcomings in the iwlwifi driver that are
addressed by this series.

First off, the fw_trips_index[] array field in struct iwl_mvm_thermal_device
is only populated and never read, and the code populating it has problems,
so patch [1/3] removes it.

Second, iwl_mvm_thermal_zone_register() populates the trip table after passing
it to thermal_zone_device_register_with_trips() which is too late, because it
can get used before it is populated.  It also may as well use THERMAL_TEMP_INVALID
as the "invalid temperature" value.  Both these issues are addressed by patch [2/3].

Finally, iwl_mvm_send_temp_report_ths_cmd() accesses the trip tables used during
thermal zone registration directly in order to obtain the current trip point
temperature values, which is not guaranteed to work in the future, because the
core will store the trips information in its own copy of the trip table - see
this patch series:

https://lore.kernel.org/linux-pm/2728491.mvXUDI8C0e@kreacher/

If possible, I'd like to route the $subject series through the thermal tree,
it is requisite for the above one.

Thanks!

[PATCH v1 1/3] iwlwifi: mvm: Drop unused fw_trips_index[] from iwl_mvm_thermal_device

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

The fw_trips_index[] array in struct iwl_mvm_thermal_device is only
populated, but never read, so drop it.

Note that the iwl_mvm_send_temp_report_ths_cmd() code populating
fw_trips_index[] is questionable, because it accesses a trips table
of a thermal zone directly, which is not guaranteed to work in the
future.

No functional impact.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/net/wireless/intel/iwlwifi/mvm/mvm.h |    2 --
 drivers/net/wireless/intel/iwlwifi/mvm/tt.c  |   13 +------------
 2 files changed, 1 insertion(+), 14 deletions(-)

Index: linux-pm/drivers/net/wireless/intel/iwlwifi/mvm/mvm.h
===================================================================
--- linux-pm.orig/drivers/net/wireless/intel/iwlwifi/mvm/mvm.h
+++ linux-pm/drivers/net/wireless/intel/iwlwifi/mvm/mvm.h
@@ -539,12 +539,10 @@ struct iwl_mvm_tt_mgmt {
 /**
  *struct iwl_mvm_thermal_device - thermal zone related data
  * @temp_trips: temperature thresholds for report
- * @fw_trips_index: keep indexes to original array - temp_trips
  * @tzone: thermal zone device data
 */
 struct iwl_mvm_thermal_device {
 	struct thermal_trip trips[IWL_MAX_DTS_TRIPS];
-	u8 fw_trips_index[IWL_MAX_DTS_TRIPS];
 	struct thermal_zone_device *tzone;
 };
 
Index: linux-pm/drivers/net/wireless/intel/iwlwifi/mvm/tt.c
===================================================================
--- linux-pm.orig/drivers/net/wireless/intel/iwlwifi/mvm/tt.c
+++ linux-pm/drivers/net/wireless/intel/iwlwifi/mvm/tt.c
@@ -562,7 +562,7 @@ int iwl_mvm_send_temp_report_ths_cmd(str
 	struct temp_report_ths_cmd cmd = {0};
 	int ret;
 #ifdef CONFIG_THERMAL
-	int i, j, idx = 0;
+	int i, idx = 0;
 
 	lockdep_assert_held(&mvm->mutex);
 
@@ -588,17 +588,6 @@ int iwl_mvm_send_temp_report_ths_cmd(str
 	/*sort cmd array*/
 	sort(cmd.thresholds, idx, sizeof(s16), compare_temps, NULL);
 
-	/* we should save the indexes of trips because we sort
-	 * and compress the orginal array
-	 */
-	for (i = 0; i < idx; i++) {
-		for (j = 0; j < IWL_MAX_DTS_TRIPS; j++) {
-			if ((int)(le16_to_cpu(cmd.thresholds[i]) * 1000) ==
-			    mvm->tz_device.trips[j].temperature)
-				mvm->tz_device.fw_trips_index[i] = j;
-		}
-	}
-
 send:
 #endif
 	ret = iwl_mvm_send_cmd_pdu(mvm, WIDE_ID(PHY_OPS_GROUP,

[PATCH v1 2/3] iwlwifi: mvm: Populate trip table before registering thermal zone

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

The trip table in iwl_mvm_thermal_zone_register() is populated after
passing it to thermal_zone_device_register_with_trips(), so it may be
accessed (for instance, via sysfs) before it is ready.

To prevent that from happening, modify the function to populate the
trip table before calling thermal_zone_device_register_with_trips().

Also make the code use THERMAL_TEMP_INVALID as the "invalid temperature"
value which is also meaningful for the core.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/net/wireless/intel/iwlwifi/mvm/tt.c |   19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

Index: linux-pm/drivers/net/wireless/intel/iwlwifi/mvm/tt.c
===================================================================
--- linux-pm.orig/drivers/net/wireless/intel/iwlwifi/mvm/tt.c
+++ linux-pm/drivers/net/wireless/intel/iwlwifi/mvm/tt.c
@@ -575,7 +575,7 @@ int iwl_mvm_send_temp_report_ths_cmd(str
 
 	/* compress trips to cmd array, remove uninitialized values*/
 	for (i = 0; i < IWL_MAX_DTS_TRIPS; i++) {
-		if (mvm->tz_device.trips[i].temperature != INT_MIN) {
+		if (mvm->tz_device.trips[i].temperature != THERMAL_TEMP_INVALID) {
 			cmd.thresholds[idx++] =
 				cpu_to_le16((s16)(mvm->tz_device.trips[i].temperature / 1000));
 		}
@@ -675,6 +675,14 @@ static void iwl_mvm_thermal_zone_registe
 	BUILD_BUG_ON(ARRAY_SIZE(name) >= THERMAL_NAME_LENGTH);
 
 	sprintf(name, "iwlwifi_%u", atomic_inc_return(&counter) & 0xFF);
+	/*
+	 * 0 is a valid temperature,
+	 * so initialize the array with S16_MIN which invalid temperature
+	 */
+	for (i = 0 ; i < IWL_MAX_DTS_TRIPS; i++) {
+		mvm->tz_device.trips[i].temperature = THERMAL_TEMP_INVALID;
+		mvm->tz_device.trips[i].type = THERMAL_TRIP_PASSIVE;
+	}
 	mvm->tz_device.tzone = thermal_zone_device_register_with_trips(name,
 							mvm->tz_device.trips,
 							IWL_MAX_DTS_TRIPS,
@@ -693,15 +701,6 @@ static void iwl_mvm_thermal_zone_registe
 	if (ret) {
 		IWL_DEBUG_TEMP(mvm, "Failed to enable thermal zone\n");
 		thermal_zone_device_unregister(mvm->tz_device.tzone);
-		return;
-	}
-
-	/* 0 is a valid temperature,
-	 * so initialize the array with S16_MIN which invalid temperature
-	 */
-	for (i = 0 ; i < IWL_MAX_DTS_TRIPS; i++) {
-		mvm->tz_device.trips[i].temperature = INT_MIN;
-		mvm->tz_device.trips[i].type = THERMAL_TRIP_PASSIVE;
 	}
 }
 

[PATCH v1 3/3] iwlwifi: mvm: Use for_each_thermal_trip() for walking trip points

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

The code walking trip points in iwl_mvm_send_temp_report_ths_cmd()
reads the trip table passed to thermal_zone_device_register_with_trips()
in order to get the current trip temperatures, but this is not
guaranteed to work in the future, because the thermal zone will store
trip points information internally.

For this reason, make iwl_mvm_send_temp_report_ths_cmd() use
for_each_thermal_trip() as appropriate for walking trip points in a
given thermal zone.

No intentional functional impact, but it is requisite for future thermal
core improvements.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/net/wireless/intel/iwlwifi/mvm/tt.c |   40 +++++++++++++++++-----------
 1 file changed, 25 insertions(+), 15 deletions(-)

Index: linux-pm/drivers/net/wireless/intel/iwlwifi/mvm/tt.c
===================================================================
--- linux-pm.orig/drivers/net/wireless/intel/iwlwifi/mvm/tt.c
+++ linux-pm/drivers/net/wireless/intel/iwlwifi/mvm/tt.c
@@ -555,6 +555,22 @@ static int compare_temps(const void *a,
 	return ((s16)le16_to_cpu(*(__le16 *)a) -
 		(s16)le16_to_cpu(*(__le16 *)b));
 }
+
+struct iwl_trip_walk_data {
+	__le16 *thresholds;
+	int count;
+};
+
+static int iwl_trip_temp_cb(struct thermal_trip *trip, void *arg)
+{
+	struct iwl_trip_walk_data *twd = arg;
+
+	if (trip->temperature == THERMAL_TEMP_INVALID)
+		return 0;
+
+	twd->thresholds[twd->count++] = cpu_to_le16((s16)(trip->temperature / 1000));
+	return 0;
+}
 #endif
 
 int iwl_mvm_send_temp_report_ths_cmd(struct iwl_mvm *mvm)
@@ -562,31 +578,25 @@ int iwl_mvm_send_temp_report_ths_cmd(str
 	struct temp_report_ths_cmd cmd = {0};
 	int ret;
 #ifdef CONFIG_THERMAL
-	int i, idx = 0;
+	struct iwl_trip_walk_data twd = { .thresholds = cmd.thresholds, .count = 0 };
 
 	lockdep_assert_held(&mvm->mutex);
 
 	if (!mvm->tz_device.tzone)
 		goto send;
 
-	/* The driver holds array of temperature trips that are unsorted
-	 * and uncompressed, the FW should get it compressed and sorted
+	/*
+	 * The thermal core holds an array of temperature trips that are
+	 * unsorted and uncompressed, the FW should get it compressed and
+	 * sorted.
 	 */
 
 	/* compress trips to cmd array, remove uninitialized values*/
-	for (i = 0; i < IWL_MAX_DTS_TRIPS; i++) {
-		if (mvm->tz_device.trips[i].temperature != THERMAL_TEMP_INVALID) {
-			cmd.thresholds[idx++] =
-				cpu_to_le16((s16)(mvm->tz_device.trips[i].temperature / 1000));
-		}
-	}
-	cmd.num_temps = cpu_to_le32(idx);
-
-	if (!idx)
-		goto send;
+	for_each_thermal_trip(mvm->tz_device.tzone, iwl_trip_temp_cb, &twd);
 
-	/*sort cmd array*/
-	sort(cmd.thresholds, idx, sizeof(s16), compare_temps, NULL);
+	cmd.num_temps = cpu_to_le32(twd.count);
+	if (twd.count)
+		sort(cmd.thresholds, twd.count, sizeof(s16), compare_temps, NULL);
 
 send:
 #endif

Re: [PATCH v1 0/3] iwlwifi: mvm: Thermal management fixes

[Kalle Valo]

"Rafael J. Wysocki" <rjw@rjwysocki.net> writes:

> There are a few thermal management shortcomings in the iwlwifi driver that are
> addressed by this series.
>
> First off, the fw_trips_index[] array field in struct iwl_mvm_thermal_device
> is only populated and never read, and the code populating it has problems,
> so patch [1/3] removes it.
>
> Second, iwl_mvm_thermal_zone_register() populates the trip table after passing
> it to thermal_zone_device_register_with_trips() which is too late, because it
> can get used before it is populated.  It also may as well use THERMAL_TEMP_INVALID
> as the "invalid temperature" value.  Both these issues are addressed by patch [2/3].
>
> Finally, iwl_mvm_send_temp_report_ths_cmd() accesses the trip tables used during
> thermal zone registration directly in order to obtain the current trip point
> temperature values, which is not guaranteed to work in the future, because the
> core will store the trips information in its own copy of the trip table - see
> this patch series:
>
> https://lore.kernel.org/linux-pm/2728491.mvXUDI8C0e@kreacher/
>
> If possible, I'd like to route the $subject series through the thermal tree,
> it is requisite for the above one.

iwlwifi is getting a lot of patches lately, though I don't know if any
of them touch the thermal stuff. But if this patchset goes to the
thermal I am a bit worried about conflicts.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH v1 0/3] iwlwifi: mvm: Thermal management fixes

[Johannes Berg]

On Thu, 2024-02-08 at 08:13 +0200, Kalle Valo wrote:
> > 
> > If possible, I'd like to route the $subject series through the thermal tree,
> > it is requisite for the above one.
> 
> iwlwifi is getting a lot of patches lately, though I don't know if any
> of them touch the thermal stuff. But if this patchset goes to the
> thermal I am a bit worried about conflicts.

Should be OK, I checked now and apart from the trivial change in mvm.h
this is contained in tt.c, which isn't touched (even in our internal
feeder tree) after commit 0106cce5ad0c ("wifi: iwlwifi: mvm: drop NULL
pointer check in iwl_mvm_tzone_set_trip_temp()").

But I'll let Miri send an Acked-by to go through your tree, since she's
the maintainer :-)

johannes

RE: [PATCH v1 0/3] iwlwifi: mvm: Thermal management fixes

[Korenblit, Miriam Rachel]

> Hi Everyone,
> 
> There are a few thermal management shortcomings in the iwlwifi driver that are
> addressed by this series.
> 
> First off, the fw_trips_index[] array field in struct iwl_mvm_thermal_device is
> only populated and never read, and the code populating it has problems, so
> patch [1/3] removes it.
> 
> Second, iwl_mvm_thermal_zone_register() populates the trip table after passing
> it to thermal_zone_device_register_with_trips() which is too late, because it can
> get used before it is populated.  It also may as well use
> THERMAL_TEMP_INVALID as the "invalid temperature" value.  Both these issues
> are addressed by patch [2/3].
> 
> Finally, iwl_mvm_send_temp_report_ths_cmd() accesses the trip tables used
> during thermal zone registration directly in order to obtain the current trip point
> temperature values, which is not guaranteed to work in the future, because the
> core will store the trips information in its own copy of the trip table - see this
> patch series:
> 
> https://lore.kernel.org/linux-pm/2728491.mvXUDI8C0e@kreacher/
> 
> If possible, I'd like to route the $subject series through the thermal tree, it is
> requisite for the above one.
> 
> Thanks!
> 
> 
Acked-by: Miri Korenblit <Miriam.rachel.korenblit@intel.com>

RE: [PATCH v1 0/3] iwlwifi: mvm: Thermal management fixes

[Korenblit, Miriam Rachel]

> On Thu, 2024-02-08 at 08:13 +0200, Kalle Valo wrote:
> > >
> > > If possible, I'd like to route the $subject series through the
> > > thermal tree, it is requisite for the above one.
> >
> > iwlwifi is getting a lot of patches lately, though I don't know if any
> > of them touch the thermal stuff. But if this patchset goes to the
> > thermal I am a bit worried about conflicts.
> 
> Should be OK, I checked now and apart from the trivial change in mvm.h this is
> contained in tt.c, which isn't touched (even in our internal feeder tree) after
> commit 0106cce5ad0c ("wifi: iwlwifi: mvm: drop NULL pointer check in
> iwl_mvm_tzone_set_trip_temp()").
> 
> But I'll let Miri send an Acked-by to go through your tree, since she's the
> maintainer :-)
> 
> Johannes

Hi,
 
This seems fine to go through your tree, as Johannes said, we don't expect any conflicts.

Thanks,
Miri

Re: [PATCH v1 0/3] iwlwifi: mvm: Thermal management fixes

[Rafael J. Wysocki]

On Thu, Feb 8, 2024 at 2:26 PM Korenblit, Miriam Rachel
<miriam.rachel.korenblit@intel.com> wrote:
>
>
>
> > On Thu, 2024-02-08 at 08:13 +0200, Kalle Valo wrote:
> > > >
> > > > If possible, I'd like to route the $subject series through the
> > > > thermal tree, it is requisite for the above one.
> > >
> > > iwlwifi is getting a lot of patches lately, though I don't know if any
> > > of them touch the thermal stuff. But if this patchset goes to the
> > > thermal I am a bit worried about conflicts.
> >
> > Should be OK, I checked now and apart from the trivial change in mvm.h this is
> > contained in tt.c, which isn't touched (even in our internal feeder tree) after
> > commit 0106cce5ad0c ("wifi: iwlwifi: mvm: drop NULL pointer check in
> > iwl_mvm_tzone_set_trip_temp()").
> >
> > But I'll let Miri send an Acked-by to go through your tree, since she's the
> > maintainer :-)
> >
> > Johannes
>
> Hi,
>
> This seems fine to go through your tree, as Johannes said, we don't expect any conflicts.

Thank you!

I'll go ahead and queue it up, then.