linux-pm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/3] thermal/debugfs: Fix handling of cdev states and mitigation episodes in progress
@ 2024-04-25 14:01 Rafael J. Wysocki
  2024-04-25 14:03 ` [PATCH v2 1/3] thermal/debugfs: Create records for cdev states as they get used Rafael J. Wysocki
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: Rafael J. Wysocki @ 2024-04-25 14:01 UTC (permalink / raw)
  To: Linux PM; +Cc: LKML, Rafael J. Wysocki, Lukasz Luba, Daniel Lezcano

Hi Everyone,

This is an update of

https://lore.kernel.org/linux-pm/5774279.DvuYhMxLoT@kreacher/

and the only non-trivial difference between it and the v1 is a small
rebase of the second patch (the v1 of which didn't apply).

It generally has been based on top of

https://lore.kernel.org/linux-pm/12427744.O9o76ZdvQC@kreacher/

but it should apply on top of the linux-next branch in linux-pm.git as well.

It is present in the thermal-core-next branch in that tree, along with the
above series.

Thanks!




^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v2 1/3] thermal/debugfs: Create records for cdev states as they get used
  2024-04-25 14:01 [PATCH v2 0/3] thermal/debugfs: Fix handling of cdev states and mitigation episodes in progress Rafael J. Wysocki
@ 2024-04-25 14:03 ` Rafael J. Wysocki
  2024-04-25 19:08   ` Lukasz Luba
  2024-04-25 14:04 ` [PATCH v2 2/3] thermal/debugfs: Pass cooling device state to thermal_debug_cdev_add() Rafael J. Wysocki
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 9+ messages in thread
From: Rafael J. Wysocki @ 2024-04-25 14:03 UTC (permalink / raw)
  To: Linux PM; +Cc: LKML, Rafael J. Wysocki, Lukasz Luba, Daniel Lezcano

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

Because thermal_debug_cdev_state_update() only creates a duration record
for the old state of a cooling device, if its new state is used for the
first time, there will be no record for it and cdev_dt_seq_show() will
not print the duration information for it even though it contains code
to compute the duration value in that case.

Address this by making thermal_debug_cdev_state_update() create a
duration record for the new state if there is none.

Fixes: 755113d76786 ("thermal/debugfs: Add thermal cooling device debugfs information")
Reported-by: Lukasz Luba <lukasz.luba@arm.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/thermal/thermal_debugfs.c |    8 ++++++++
 1 file changed, 8 insertions(+)

Index: linux-pm/drivers/thermal/thermal_debugfs.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_debugfs.c
+++ linux-pm/drivers/thermal/thermal_debugfs.c
@@ -433,6 +433,14 @@ void thermal_debug_cdev_state_update(con
 	}
 
 	cdev_dbg->current_state = new_state;
+
+	/*
+	 * Create a record for the new state if it is not there, so its
+	 * duration will be printed by cdev_dt_seq_show() as expected if it
+	 * runs before the next state transition.
+	 */
+	thermal_debugfs_cdev_record_get(thermal_dbg, cdev_dbg->durations, new_state);
+
 	transition = (old_state << 16) | new_state;
 
 	/*




^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v2 2/3] thermal/debugfs: Pass cooling device state to thermal_debug_cdev_add()
  2024-04-25 14:01 [PATCH v2 0/3] thermal/debugfs: Fix handling of cdev states and mitigation episodes in progress Rafael J. Wysocki
  2024-04-25 14:03 ` [PATCH v2 1/3] thermal/debugfs: Create records for cdev states as they get used Rafael J. Wysocki
@ 2024-04-25 14:04 ` Rafael J. Wysocki
  2024-04-25 20:32   ` Lukasz Luba
  2024-04-25 14:05 ` [PATCH v2 3/3] thermal/debugfs: Avoid printing zero duration for mitigation events in progress Rafael J. Wysocki
  2024-04-25 20:55 ` [PATCH v2 0/3] thermal/debugfs: Fix handling of cdev states and mitigation episodes " Lukasz Luba
  3 siblings, 1 reply; 9+ messages in thread
From: Rafael J. Wysocki @ 2024-04-25 14:04 UTC (permalink / raw)
  To: Linux PM; +Cc: LKML, Rafael J. Wysocki, Lukasz Luba, Daniel Lezcano

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

If cdev_dt_seq_show() runs before the first state transition of a cooling
device, it will not print any state residency information for it, even
though it might be reasonably expected to print residency information for
the initial state of the cooling device.

For this reason, rearrange the code to get the initial state of a cooling
device at the registration time and pass it to thermal_debug_cdev_add(),
so that the latter can create a duration record for that state which will
allow cdev_dt_seq_show() to print its residency information.

Fixes: 755113d76786 ("thermal/debugfs: Add thermal cooling device debugfs information")
Reported-by: Lukasz Luba <lukasz.luba@arm.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/thermal/thermal_core.c    |    9 +++++++--
 drivers/thermal/thermal_debugfs.c |   12 ++++++++++--
 drivers/thermal/thermal_debugfs.h |    4 ++--
 3 files changed, 19 insertions(+), 6 deletions(-)

Index: linux-pm/drivers/thermal/thermal_core.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.c
+++ linux-pm/drivers/thermal/thermal_core.c
@@ -935,6 +935,7 @@ __thermal_cooling_device_register(struct
 {
 	struct thermal_cooling_device *cdev;
 	struct thermal_zone_device *pos = NULL;
+	unsigned long current_state;
 	int id, ret;
 
 	if (!ops || !ops->get_max_state || !ops->get_cur_state ||
@@ -972,6 +973,10 @@ __thermal_cooling_device_register(struct
 	if (ret)
 		goto out_cdev_type;
 
+	ret = cdev->ops->get_cur_state(cdev, &current_state);
+	if (ret)
+		goto out_cdev_type;
+
 	thermal_cooling_device_setup_sysfs(cdev);
 
 	ret = dev_set_name(&cdev->device, "cooling_device%d", cdev->id);
@@ -985,6 +990,8 @@ __thermal_cooling_device_register(struct
 		return ERR_PTR(ret);
 	}
 
+	thermal_debug_cdev_add(cdev, current_state);
+
 	/* Add 'this' new cdev to the global cdev list */
 	mutex_lock(&thermal_list_lock);
 
@@ -1000,8 +1007,6 @@ __thermal_cooling_device_register(struct
 
 	mutex_unlock(&thermal_list_lock);
 
-	thermal_debug_cdev_add(cdev);
-
 	return cdev;
 
 out_cooling_dev:
Index: linux-pm/drivers/thermal/thermal_debugfs.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_debugfs.c
+++ linux-pm/drivers/thermal/thermal_debugfs.c
@@ -466,8 +466,9 @@ void thermal_debug_cdev_state_update(con
  * Allocates a cooling device object for debug, initializes the
  * statistics and create the entries in sysfs.
  * @cdev: a pointer to a cooling device
+ * @state: current state of the cooling device
  */
-void thermal_debug_cdev_add(struct thermal_cooling_device *cdev)
+void thermal_debug_cdev_add(struct thermal_cooling_device *cdev, int state)
 {
 	struct thermal_debugfs *thermal_dbg;
 	struct cdev_debugfs *cdev_dbg;
@@ -484,9 +485,16 @@ void thermal_debug_cdev_add(struct therm
 		INIT_LIST_HEAD(&cdev_dbg->durations[i]);
 	}
 
-	cdev_dbg->current_state = 0;
+	cdev_dbg->current_state = state;
 	cdev_dbg->timestamp = ktime_get();
 
+	/*
+	 * Create a record for the initial cooling device state, so its
+	 * duration will be printed by cdev_dt_seq_show() as expected if it
+	 * runs before the first state transition.
+	 */
+	thermal_debugfs_cdev_record_get(thermal_dbg, cdev_dbg->durations, state);
+
 	debugfs_create_file("trans_table", 0400, thermal_dbg->d_top,
 			    thermal_dbg, &tt_fops);
 
Index: linux-pm/drivers/thermal/thermal_debugfs.h
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_debugfs.h
+++ linux-pm/drivers/thermal/thermal_debugfs.h
@@ -2,7 +2,7 @@
 
 #ifdef CONFIG_THERMAL_DEBUGFS
 void thermal_debug_init(void);
-void thermal_debug_cdev_add(struct thermal_cooling_device *cdev);
+void thermal_debug_cdev_add(struct thermal_cooling_device *cdev, int state);
 void thermal_debug_cdev_remove(struct thermal_cooling_device *cdev);
 void thermal_debug_cdev_state_update(const struct thermal_cooling_device *cdev, int state);
 void thermal_debug_tz_add(struct thermal_zone_device *tz);
@@ -14,7 +14,7 @@ void thermal_debug_tz_trip_down(struct t
 void thermal_debug_update_trip_stats(struct thermal_zone_device *tz);
 #else
 static inline void thermal_debug_init(void) {}
-static inline void thermal_debug_cdev_add(struct thermal_cooling_device *cdev) {}
+static inline void thermal_debug_cdev_add(struct thermal_cooling_device *cdev, int state) {}
 static inline void thermal_debug_cdev_remove(struct thermal_cooling_device *cdev) {}
 static inline void thermal_debug_cdev_state_update(const struct thermal_cooling_device *cdev,
 						   int state) {}




^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v2 3/3] thermal/debugfs: Avoid printing zero duration for mitigation events in progress
  2024-04-25 14:01 [PATCH v2 0/3] thermal/debugfs: Fix handling of cdev states and mitigation episodes in progress Rafael J. Wysocki
  2024-04-25 14:03 ` [PATCH v2 1/3] thermal/debugfs: Create records for cdev states as they get used Rafael J. Wysocki
  2024-04-25 14:04 ` [PATCH v2 2/3] thermal/debugfs: Pass cooling device state to thermal_debug_cdev_add() Rafael J. Wysocki
@ 2024-04-25 14:05 ` Rafael J. Wysocki
  2024-04-25 20:54   ` Lukasz Luba
  2024-04-25 20:55 ` [PATCH v2 0/3] thermal/debugfs: Fix handling of cdev states and mitigation episodes " Lukasz Luba
  3 siblings, 1 reply; 9+ messages in thread
From: Rafael J. Wysocki @ 2024-04-25 14:05 UTC (permalink / raw)
  To: Linux PM; +Cc: LKML, Rafael J. Wysocki, Lukasz Luba, Daniel Lezcano

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

If a thermal mitigation event is in progress, its duration value has
not been updated yet, so 0 will be printed as the event duration by
tze_seq_show() which is confusing.

Avoid doing that by marking the beginning of the event with the
KTIME_MIN duration value and making tze_seq_show() compute the current
event duration on the fly, in which case '>' will be printed instead of
'=' in the event duration value field.

Similarly, for trip points that have been crossed on the down, mark
the end of mitigation with the KTIME_MAX timestamp value and make
tze_seq_show() compute the current duration on the fly for the trip
points still involved in the mitigation, in which cases the duration
value printed by it will be prepended with a '>' character.

Fixes: 7ef01f228c9f ("thermal/debugfs: Add thermal debugfs information for mitigation episodes")
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/thermal/thermal_debugfs.c |   39 ++++++++++++++++++++++++++++++++------
 1 file changed, 33 insertions(+), 6 deletions(-)

Index: linux-pm/drivers/thermal/thermal_debugfs.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_debugfs.c
+++ linux-pm/drivers/thermal/thermal_debugfs.c
@@ -552,6 +552,7 @@ static struct tz_episode *thermal_debugf
 
 	INIT_LIST_HEAD(&tze->node);
 	tze->timestamp = now;
+	tze->duration = KTIME_MIN;
 
 	for (i = 0; i < tz->num_trips; i++) {
 		tze->trip_stats[i].min = INT_MAX;
@@ -680,6 +681,9 @@ void thermal_debug_tz_trip_down(struct t
 	tze->trip_stats[trip_id].duration =
 		ktime_add(delta, tze->trip_stats[trip_id].duration);
 
+	/* Mark the end of mitigation for this trip point. */
+	tze->trip_stats[trip_id].timestamp = KTIME_MAX;
+
 	/*
 	 * This event closes the mitigation as we are crossing the
 	 * last trip point the way down.
@@ -754,15 +758,25 @@ static int tze_seq_show(struct seq_file
 	struct thermal_trip_desc *td;
 	struct tz_episode *tze;
 	const char *type;
+	u64 duration_ms;
 	int trip_id;
+	char c;
 
 	tze = list_entry((struct list_head *)v, struct tz_episode, node);
 
-	seq_printf(s, ",-Mitigation at %lluus, duration=%llums\n",
-		   ktime_to_us(tze->timestamp),
-		   ktime_to_ms(tze->duration));
+	if (tze->duration == KTIME_MIN) {
+		/* Mitigation in progress. */
+		duration_ms = ktime_to_ms(ktime_sub(ktime_get(), tze->timestamp));
+		c = '>';
+	} else {
+		duration_ms = ktime_to_ms(tze->duration);
+		c = '=';
+	}
+
+	seq_printf(s, ",-Mitigation at %lluus, duration%c%llums\n",
+		   ktime_to_us(tze->timestamp), c, duration_ms);
 
-	seq_printf(s, "| trip |     type | temp(°mC) | hyst(°mC) |  duration  |  avg(°mC) |  min(°mC) |  max(°mC) |\n");
+	seq_printf(s, "| trip |     type | temp(°mC) | hyst(°mC) |  duration   |  avg(°mC) |  min(°mC) |  max(°mC) |\n");
 
 	for_each_trip_desc(tz, td) {
 		const struct thermal_trip *trip = &td->trip;
@@ -794,12 +808,25 @@ static int tze_seq_show(struct seq_file
 		else
 			type = "hot";
 
-		seq_printf(s, "| %*d | %*s | %*d | %*d | %*lld | %*d | %*d | %*d |\n",
+		if (trip_stats->timestamp != KTIME_MAX) {
+			/* Mitigation in progress. */
+			ktime_t delta = ktime_sub(ktime_get(),
+						  trip_stats->timestamp);
+
+			delta = ktime_add(delta, trip_stats->duration);
+			duration_ms = ktime_to_ms(delta);
+			c = '>';
+		} else {
+			duration_ms = ktime_to_ms(trip_stats->duration);
+			c = ' ';
+		}
+
+		seq_printf(s, "| %*d | %*s | %*d | %*d | %c%*lld | %*d | %*d | %*d |\n",
 			   4 , trip_id,
 			   8, type,
 			   9, trip->temperature,
 			   9, trip->hysteresis,
-			   10, ktime_to_ms(trip_stats->duration),
+			   c, 10, duration_ms,
 			   9, trip_stats->avg,
 			   9, trip_stats->min,
 			   9, trip_stats->max);




^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 1/3] thermal/debugfs: Create records for cdev states as they get used
  2024-04-25 14:03 ` [PATCH v2 1/3] thermal/debugfs: Create records for cdev states as they get used Rafael J. Wysocki
@ 2024-04-25 19:08   ` Lukasz Luba
  0 siblings, 0 replies; 9+ messages in thread
From: Lukasz Luba @ 2024-04-25 19:08 UTC (permalink / raw)
  To: Rafael J. Wysocki; +Cc: LKML, Linux PM, Rafael J. Wysocki, Daniel Lezcano



On 4/25/24 15:03, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Because thermal_debug_cdev_state_update() only creates a duration record
> for the old state of a cooling device, if its new state is used for the
> first time, there will be no record for it and cdev_dt_seq_show() will
> not print the duration information for it even though it contains code
> to compute the duration value in that case.
> 
> Address this by making thermal_debug_cdev_state_update() create a
> duration record for the new state if there is none.
> 
> Fixes: 755113d76786 ("thermal/debugfs: Add thermal cooling device debugfs information")
> Reported-by: Lukasz Luba <lukasz.luba@arm.com>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>   drivers/thermal/thermal_debugfs.c |    8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> Index: linux-pm/drivers/thermal/thermal_debugfs.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_debugfs.c
> +++ linux-pm/drivers/thermal/thermal_debugfs.c
> @@ -433,6 +433,14 @@ void thermal_debug_cdev_state_update(con
>   	}
>   
>   	cdev_dbg->current_state = new_state;
> +
> +	/*
> +	 * Create a record for the new state if it is not there, so its
> +	 * duration will be printed by cdev_dt_seq_show() as expected if it
> +	 * runs before the next state transition.
> +	 */
> +	thermal_debugfs_cdev_record_get(thermal_dbg, cdev_dbg->durations, new_state);
> +
>   	transition = (old_state << 16) | new_state;
>   
>   	/*
> 
> 
> 
> 

Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 2/3] thermal/debugfs: Pass cooling device state to thermal_debug_cdev_add()
  2024-04-25 14:04 ` [PATCH v2 2/3] thermal/debugfs: Pass cooling device state to thermal_debug_cdev_add() Rafael J. Wysocki
@ 2024-04-25 20:32   ` Lukasz Luba
  0 siblings, 0 replies; 9+ messages in thread
From: Lukasz Luba @ 2024-04-25 20:32 UTC (permalink / raw)
  To: Rafael J. Wysocki; +Cc: LKML, Rafael J. Wysocki, Daniel Lezcano, Linux PM



On 4/25/24 15:04, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> If cdev_dt_seq_show() runs before the first state transition of a cooling
> device, it will not print any state residency information for it, even
> though it might be reasonably expected to print residency information for
> the initial state of the cooling device.
> 
> For this reason, rearrange the code to get the initial state of a cooling
> device at the registration time and pass it to thermal_debug_cdev_add(),
> so that the latter can create a duration record for that state which will
> allow cdev_dt_seq_show() to print its residency information.
> 
> Fixes: 755113d76786 ("thermal/debugfs: Add thermal cooling device debugfs information")
> Reported-by: Lukasz Luba <lukasz.luba@arm.com>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>   drivers/thermal/thermal_core.c    |    9 +++++++--
>   drivers/thermal/thermal_debugfs.c |   12 ++++++++++--
>   drivers/thermal/thermal_debugfs.h |    4 ++--
>   3 files changed, 19 insertions(+), 6 deletions(-)
> 
> Index: linux-pm/drivers/thermal/thermal_core.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_core.c
> +++ linux-pm/drivers/thermal/thermal_core.c
> @@ -935,6 +935,7 @@ __thermal_cooling_device_register(struct
>   {
>   	struct thermal_cooling_device *cdev;
>   	struct thermal_zone_device *pos = NULL;
> +	unsigned long current_state;
>   	int id, ret;
>   
>   	if (!ops || !ops->get_max_state || !ops->get_cur_state ||
> @@ -972,6 +973,10 @@ __thermal_cooling_device_register(struct
>   	if (ret)
>   		goto out_cdev_type;
>   
> +	ret = cdev->ops->get_cur_state(cdev, &current_state);
> +	if (ret)
> +		goto out_cdev_type;
> +
>   	thermal_cooling_device_setup_sysfs(cdev);
>   
>   	ret = dev_set_name(&cdev->device, "cooling_device%d", cdev->id);
> @@ -985,6 +990,8 @@ __thermal_cooling_device_register(struct
>   		return ERR_PTR(ret);
>   	}
>   
> +	thermal_debug_cdev_add(cdev, current_state);
> +
>   	/* Add 'this' new cdev to the global cdev list */
>   	mutex_lock(&thermal_list_lock);
>   
> @@ -1000,8 +1007,6 @@ __thermal_cooling_device_register(struct
>   
>   	mutex_unlock(&thermal_list_lock);
>   
> -	thermal_debug_cdev_add(cdev);
> -
>   	return cdev;
>   
>   out_cooling_dev:
> Index: linux-pm/drivers/thermal/thermal_debugfs.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_debugfs.c
> +++ linux-pm/drivers/thermal/thermal_debugfs.c
> @@ -466,8 +466,9 @@ void thermal_debug_cdev_state_update(con
>    * Allocates a cooling device object for debug, initializes the
>    * statistics and create the entries in sysfs.
>    * @cdev: a pointer to a cooling device
> + * @state: current state of the cooling device
>    */
> -void thermal_debug_cdev_add(struct thermal_cooling_device *cdev)
> +void thermal_debug_cdev_add(struct thermal_cooling_device *cdev, int state)
>   {
>   	struct thermal_debugfs *thermal_dbg;
>   	struct cdev_debugfs *cdev_dbg;
> @@ -484,9 +485,16 @@ void thermal_debug_cdev_add(struct therm
>   		INIT_LIST_HEAD(&cdev_dbg->durations[i]);
>   	}
>   
> -	cdev_dbg->current_state = 0;
> +	cdev_dbg->current_state = state;
>   	cdev_dbg->timestamp = ktime_get();
>   
> +	/*
> +	 * Create a record for the initial cooling device state, so its
> +	 * duration will be printed by cdev_dt_seq_show() as expected if it
> +	 * runs before the first state transition.
> +	 */
> +	thermal_debugfs_cdev_record_get(thermal_dbg, cdev_dbg->durations, state);
> +
>   	debugfs_create_file("trans_table", 0400, thermal_dbg->d_top,
>   			    thermal_dbg, &tt_fops);
>   
> Index: linux-pm/drivers/thermal/thermal_debugfs.h
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_debugfs.h
> +++ linux-pm/drivers/thermal/thermal_debugfs.h
> @@ -2,7 +2,7 @@
>   
>   #ifdef CONFIG_THERMAL_DEBUGFS
>   void thermal_debug_init(void);
> -void thermal_debug_cdev_add(struct thermal_cooling_device *cdev);
> +void thermal_debug_cdev_add(struct thermal_cooling_device *cdev, int state);
>   void thermal_debug_cdev_remove(struct thermal_cooling_device *cdev);
>   void thermal_debug_cdev_state_update(const struct thermal_cooling_device *cdev, int state);
>   void thermal_debug_tz_add(struct thermal_zone_device *tz);
> @@ -14,7 +14,7 @@ void thermal_debug_tz_trip_down(struct t
>   void thermal_debug_update_trip_stats(struct thermal_zone_device *tz);
>   #else
>   static inline void thermal_debug_init(void) {}
> -static inline void thermal_debug_cdev_add(struct thermal_cooling_device *cdev) {}
> +static inline void thermal_debug_cdev_add(struct thermal_cooling_device *cdev, int state) {}
>   static inline void thermal_debug_cdev_remove(struct thermal_cooling_device *cdev) {}
>   static inline void thermal_debug_cdev_state_update(const struct thermal_cooling_device *cdev,
>   						   int state) {}
> 
> 
> 
> 

Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 3/3] thermal/debugfs: Avoid printing zero duration for mitigation events in progress
  2024-04-25 14:05 ` [PATCH v2 3/3] thermal/debugfs: Avoid printing zero duration for mitigation events in progress Rafael J. Wysocki
@ 2024-04-25 20:54   ` Lukasz Luba
  0 siblings, 0 replies; 9+ messages in thread
From: Lukasz Luba @ 2024-04-25 20:54 UTC (permalink / raw)
  To: Rafael J. Wysocki; +Cc: LKML, Linux PM, Rafael J. Wysocki, Daniel Lezcano



On 4/25/24 15:05, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> If a thermal mitigation event is in progress, its duration value has
> not been updated yet, so 0 will be printed as the event duration by
> tze_seq_show() which is confusing.
> 
> Avoid doing that by marking the beginning of the event with the
> KTIME_MIN duration value and making tze_seq_show() compute the current
> event duration on the fly, in which case '>' will be printed instead of
> '=' in the event duration value field.
> 
> Similarly, for trip points that have been crossed on the down, mark
> the end of mitigation with the KTIME_MAX timestamp value and make
> tze_seq_show() compute the current duration on the fly for the trip
> points still involved in the mitigation, in which cases the duration
> value printed by it will be prepended with a '>' character.
> 
> Fixes: 7ef01f228c9f ("thermal/debugfs: Add thermal debugfs information for mitigation episodes")
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>   drivers/thermal/thermal_debugfs.c |   39 ++++++++++++++++++++++++++++++++------
>   1 file changed, 33 insertions(+), 6 deletions(-)
> 
> Index: linux-pm/drivers/thermal/thermal_debugfs.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_debugfs.c
> +++ linux-pm/drivers/thermal/thermal_debugfs.c
> @@ -552,6 +552,7 @@ static struct tz_episode *thermal_debugf
>   
>   	INIT_LIST_HEAD(&tze->node);
>   	tze->timestamp = now;
> +	tze->duration = KTIME_MIN;
>   
>   	for (i = 0; i < tz->num_trips; i++) {
>   		tze->trip_stats[i].min = INT_MAX;
> @@ -680,6 +681,9 @@ void thermal_debug_tz_trip_down(struct t
>   	tze->trip_stats[trip_id].duration =
>   		ktime_add(delta, tze->trip_stats[trip_id].duration);
>   
> +	/* Mark the end of mitigation for this trip point. */
> +	tze->trip_stats[trip_id].timestamp = KTIME_MAX;
> +
>   	/*
>   	 * This event closes the mitigation as we are crossing the
>   	 * last trip point the way down.
> @@ -754,15 +758,25 @@ static int tze_seq_show(struct seq_file
>   	struct thermal_trip_desc *td;
>   	struct tz_episode *tze;
>   	const char *type;
> +	u64 duration_ms;
>   	int trip_id;
> +	char c;
>   
>   	tze = list_entry((struct list_head *)v, struct tz_episode, node);
>   
> -	seq_printf(s, ",-Mitigation at %lluus, duration=%llums\n",
> -		   ktime_to_us(tze->timestamp),
> -		   ktime_to_ms(tze->duration));
> +	if (tze->duration == KTIME_MIN) {
> +		/* Mitigation in progress. */
> +		duration_ms = ktime_to_ms(ktime_sub(ktime_get(), tze->timestamp));
> +		c = '>';
> +	} else {
> +		duration_ms = ktime_to_ms(tze->duration);
> +		c = '=';
> +	}
> +
> +	seq_printf(s, ",-Mitigation at %lluus, duration%c%llums\n",
> +		   ktime_to_us(tze->timestamp), c, duration_ms);
>   
> -	seq_printf(s, "| trip |     type | temp(°mC) | hyst(°mC) |  duration  |  avg(°mC) |  min(°mC) |  max(°mC) |\n");
> +	seq_printf(s, "| trip |     type | temp(°mC) | hyst(°mC) |  duration   |  avg(°mC) |  min(°mC) |  max(°mC) |\n");

So this one more space accounts for the new 'c' symbol in the rows
below that header, for the 'duration' column. Make sense.

>   
>   	for_each_trip_desc(tz, td) {
>   		const struct thermal_trip *trip = &td->trip;
> @@ -794,12 +808,25 @@ static int tze_seq_show(struct seq_file
>   		else
>   			type = "hot";
>   
> -		seq_printf(s, "| %*d | %*s | %*d | %*d | %*lld | %*d | %*d | %*d |\n",
> +		if (trip_stats->timestamp != KTIME_MAX) {
> +			/* Mitigation in progress. */
> +			ktime_t delta = ktime_sub(ktime_get(),
> +						  trip_stats->timestamp);
> +
> +			delta = ktime_add(delta, trip_stats->duration);
> +			duration_ms = ktime_to_ms(delta);
> +			c = '>';
> +		} else {
> +			duration_ms = ktime_to_ms(trip_stats->duration);
> +			c = ' ';
> +		}
> +
> +		seq_printf(s, "| %*d | %*s | %*d | %*d | %c%*lld | %*d | %*d | %*d |\n",
>   			   4 , trip_id,
>   			   8, type,
>   			   9, trip->temperature,
>   			   9, trip->hysteresis,
> -			   10, ktime_to_ms(trip_stats->duration),
> +			   c, 10, duration_ms,
>   			   9, trip_stats->avg,
>   			   9, trip_stats->min,
>   			   9, trip_stats->max);
> 
> 
> 
> 

The comments in code in this particular case helps, since treating
the KTIME_MIN/MAX values might become not obvious after a while.
That LGTM

Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 0/3] thermal/debugfs: Fix handling of cdev states and mitigation episodes in progress
  2024-04-25 14:01 [PATCH v2 0/3] thermal/debugfs: Fix handling of cdev states and mitigation episodes in progress Rafael J. Wysocki
                   ` (2 preceding siblings ...)
  2024-04-25 14:05 ` [PATCH v2 3/3] thermal/debugfs: Avoid printing zero duration for mitigation events in progress Rafael J. Wysocki
@ 2024-04-25 20:55 ` Lukasz Luba
  2024-04-26  9:55   ` Rafael J. Wysocki
  3 siblings, 1 reply; 9+ messages in thread
From: Lukasz Luba @ 2024-04-25 20:55 UTC (permalink / raw)
  To: Rafael J. Wysocki; +Cc: LKML, Linux PM, Rafael J. Wysocki, Daniel Lezcano

Hi Rafael,

On 4/25/24 15:01, Rafael J. Wysocki wrote:
> Hi Everyone,
> 
> This is an update of
> 
> https://lore.kernel.org/linux-pm/5774279.DvuYhMxLoT@kreacher/
> 
> and the only non-trivial difference between it and the v1 is a small
> rebase of the second patch (the v1 of which didn't apply).
> 
> It generally has been based on top of
> 
> https://lore.kernel.org/linux-pm/12427744.O9o76ZdvQC@kreacher/
> 
> but it should apply on top of the linux-next branch in linux-pm.git as well.
> 
> It is present in the thermal-core-next branch in that tree, along with the
> above series.
> 
> Thanks!
> 
> 
> 

I have also tested the patches, so feel free to add the tag as well:

Tested-by: Lukasz Luba <lukasz.luba@arm.com>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 0/3] thermal/debugfs: Fix handling of cdev states and mitigation episodes in progress
  2024-04-25 20:55 ` [PATCH v2 0/3] thermal/debugfs: Fix handling of cdev states and mitigation episodes " Lukasz Luba
@ 2024-04-26  9:55   ` Rafael J. Wysocki
  0 siblings, 0 replies; 9+ messages in thread
From: Rafael J. Wysocki @ 2024-04-26  9:55 UTC (permalink / raw)
  To: Lukasz Luba
  Cc: Rafael J. Wysocki, LKML, Linux PM, Rafael J. Wysocki, Daniel Lezcano

On Thu, Apr 25, 2024 at 10:55 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>
> Hi Rafael,
>
> On 4/25/24 15:01, Rafael J. Wysocki wrote:
> > Hi Everyone,
> >
> > This is an update of
> >
> > https://lore.kernel.org/linux-pm/5774279.DvuYhMxLoT@kreacher/
> >
> > and the only non-trivial difference between it and the v1 is a small
> > rebase of the second patch (the v1 of which didn't apply).
> >
> > It generally has been based on top of
> >
> > https://lore.kernel.org/linux-pm/12427744.O9o76ZdvQC@kreacher/
> >
> > but it should apply on top of the linux-next branch in linux-pm.git as well.
> >
> > It is present in the thermal-core-next branch in that tree, along with the
> > above series.
> >
> > Thanks!
> >
> >
> >
>
> I have also tested the patches, so feel free to add the tag as well:
>
> Tested-by: Lukasz Luba <lukasz.luba@arm.com>

Thank you!

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2024-04-26  9:55 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-25 14:01 [PATCH v2 0/3] thermal/debugfs: Fix handling of cdev states and mitigation episodes in progress Rafael J. Wysocki
2024-04-25 14:03 ` [PATCH v2 1/3] thermal/debugfs: Create records for cdev states as they get used Rafael J. Wysocki
2024-04-25 19:08   ` Lukasz Luba
2024-04-25 14:04 ` [PATCH v2 2/3] thermal/debugfs: Pass cooling device state to thermal_debug_cdev_add() Rafael J. Wysocki
2024-04-25 20:32   ` Lukasz Luba
2024-04-25 14:05 ` [PATCH v2 3/3] thermal/debugfs: Avoid printing zero duration for mitigation events in progress Rafael J. Wysocki
2024-04-25 20:54   ` Lukasz Luba
2024-04-25 20:55 ` [PATCH v2 0/3] thermal/debugfs: Fix handling of cdev states and mitigation episodes " Lukasz Luba
2024-04-26  9:55   ` Rafael J. Wysocki

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).