iwlwifi: mvm: Thermal management fixes

PATCH In review Rafael J. Wysocki 5 participants 9 messages Feb 7, 2024 – Feb 8, 2024 Intel wireless wifi link (iwlwifi)

lore

Share & tools

lore

Rafael J. Wysocki

author

2024-02-07 19:12 UTC #0 lore raw

cover iwlwifi: mvm: Thermal management fixes

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!

Rafael J. Wysocki

author

2024-02-07 19:12 UTC #1 lore raw

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

From: Rafael J. Wysocki <[email protected]>

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 <[email protected]>

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

drivers/net/wireless/intel/iwlwifi/mvm/mvm.h +0 -2

Index: 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;
 };

drivers/net/wireless/intel/iwlwifi/mvm/tt.c +1 ~~-12~~

Index: 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,

Rafael J. Wysocki

author

2024-02-07 19:12 UTC #2 lore raw

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

In reply to Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

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 <[email protected]>

---
 drivers/net/wireless/intel/iwlwifi/mvm/tt.c |   19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

drivers/net/wireless/intel/iwlwifi/mvm/tt.c +9 ~~-10~~

Index: 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;
 	}
 }

Rafael J. Wysocki

author

2024-02-07 19:12 UTC #3 lore raw

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

In reply to Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

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 <[email protected]>

---
 drivers/net/wireless/intel/iwlwifi/mvm/tt.c |   40 +++++++++++++++++-----------
 1 file changed, 25 insertions(+), 15 deletions(-)

drivers/net/wireless/intel/iwlwifi/mvm/tt.c +25 ~~-15~~

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

Kalle Valo

2024-02-08 06:13 UTC #4 lore raw

In reply to Rafael J. Wysocki

Quoted 18 lines · Rafael J. Wysocki

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:

Quoted 4 lines

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.

Signature · 3 lines

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

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

Johannes Berg

2024-02-08 09:28 UTC #5 lore raw

Quoted 7 lines · Kalle Valo

Quoted 3 lines
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

Korenblit, Miriam Rachel

maintainer

2024-02-08 13:26 UTC #6 lore raw

Quoted 16 lines · Kalle Valo

Quoted 7 lines
Quoted 3 lines
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()").

Quoted 4 lines

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

Rafael J. Wysocki

2024-02-08 13:50 UTC #7 lore raw

Quoted 17 lines · Miriam Rachel

Quoted 17 lines · Kalle Valo
Quoted 7 lines
Quoted 3 lines
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()").

Quoted 8 lines

Quoted 4 lines
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.

Korenblit, Miriam Rachel

maintainer Acked-by on the v1 series

2024-02-08 13:25 UTC #8 lore raw

In reply to Rafael J. Wysocki

Quoted 21 lines

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:

Quoted 8 lines

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 <[email protected]>