[PATCH 0/3] PM / devfreq: Add devfreq_frequency tracepoint to track frequency change

[PATCH 0/3] PM / devfreq: Add devfreq_frequency tracepoint to track frequency change

[Chanwoo Choi]

Add devfreq_tracepoint to track the correct timing of frequency change
with following information:
- device name
- current frequency
- previous frequency
- load when change frequency
- tracepoint path : /sys/kernel/debug/tracing/events/devfreq_frequency

And add devfreq_update_target() function to unify the frequency change code
on both devfreq core and devfreq passive governor because there are redundant
duplicate code. Lastly, Use fixed indentation size to improve readability
for 'devfreq_monitor' tracepoint.

Matthias already sent the patch[1]. Make patch3 by editing patch[1].
[1]https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg2108015.html

Chanwoo Choi (2):
  trace: events: devfreq: Use fixed indentation size to improve readability
  PM / devfreq: Unify frequency change to devfreq_update_target func

Matthias Kaehlcke (1):
  PM / devfreq: Add tracepoint for frequency changes

 drivers/devfreq/devfreq.c          | 37 +++++++++++++++++++++-----
 drivers/devfreq/governor.h         |  1 +
 drivers/devfreq/governor_passive.c | 42 +++++++-----------------------
 include/trace/events/devfreq.h     | 30 ++++++++++++++++++++-
 4 files changed, 70 insertions(+), 40 deletions(-)

-- 
2.17.1

[PATCH 1/3] trace: events: devfreq: Use fixed indentation size to improve readability

[Chanwoo Choi]

Each tracepoint infromation consist of the different size value.
So, in order to improve the readability, use the fixed indentation size.

Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
---
 include/trace/events/devfreq.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/trace/events/devfreq.h b/include/trace/events/devfreq.h
index cf5b8772175d..bd36d28d16bc 100644
--- a/include/trace/events/devfreq.h
+++ b/include/trace/events/devfreq.h
@@ -29,7 +29,7 @@ TRACE_EVENT(devfreq_monitor,
 		__assign_str(dev_name, dev_name(&devfreq->dev));
 	),
 
-	TP_printk("dev_name=%s freq=%lu polling_ms=%u load=%lu",
+	TP_printk("dev_name=%-30s freq=%-12lu polling_ms=%-3u load=%-2lu",
 		__get_str(dev_name), __entry->freq, __entry->polling_ms,
 		__entry->total_time == 0 ? 0 :
 			(100 * __entry->busy_time) / __entry->total_time)
-- 
2.17.1

[PATCH 2/3] PM / devfreq: Unify frequency change to devfreq_update_target func

[Chanwoo Choi]

The update_devfreq() and update_passive_devfreq() have the duplicate
code when changing the target frequency on final stage. So, unify
frequency change code to devfreq_update_target() to remove the
duplicate code and to centralize the frequency change code.

Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
---
 drivers/devfreq/devfreq.c          | 29 ++++++++++++++++-----
 drivers/devfreq/governor.h         |  1 +
 drivers/devfreq/governor_passive.c | 42 +++++++-----------------------
 3 files changed, 33 insertions(+), 39 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 379aaaabf25d..5b069a8a1026 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -384,18 +384,19 @@ static int devfreq_set_target(struct devfreq *devfreq, unsigned long new_freq,
 	return err;
 }
 
-/* Load monitoring helper functions for governors use */
-
 /**
- * update_devfreq() - Reevaluate the device and configure frequency.
+ * devfreq_update_target() - Reevaluate the device and configure frequency
+ *			   on the final stage.
  * @devfreq:	the devfreq instance.
+ * @freq:	the new frequency of parent device. This argument
+ *		is only used for devfreq device using passive governor.
  *
- * Note: Lock devfreq->lock before calling update_devfreq
- *	 This function is exported for governors.
+ * Note: Lock devfreq->lock before calling devfreq_update_target. This function
+ *	 should be only used by both update_devfreq() and devfreq governors.
  */
-int update_devfreq(struct devfreq *devfreq)
+int devfreq_update_target(struct devfreq *devfreq, unsigned long freq)
 {
-	unsigned long freq, min_freq, max_freq;
+	unsigned long min_freq, max_freq;
 	int err = 0;
 	u32 flags = 0;
 
@@ -420,7 +421,21 @@ int update_devfreq(struct devfreq *devfreq)
 	}
 
 	return devfreq_set_target(devfreq, freq, flags);
+}
+EXPORT_SYMBOL(devfreq_update_target);
+
+/* Load monitoring helper functions for governors use */
 
+/**
+ * update_devfreq() - Reevaluate the device and configure frequency.
+ * @devfreq:	the devfreq instance.
+ *
+ * Note: Lock devfreq->lock before calling update_devfreq
+ *	 This function is exported for governors.
+ */
+int update_devfreq(struct devfreq *devfreq)
+{
+	return devfreq_update_target(devfreq, 0L);
 }
 EXPORT_SYMBOL(update_devfreq);
 
diff --git a/drivers/devfreq/governor.h b/drivers/devfreq/governor.h
index eb6392d397b3..871150be4391 100644
--- a/drivers/devfreq/governor.h
+++ b/drivers/devfreq/governor.h
@@ -85,6 +85,7 @@ int devfreq_add_governor(struct devfreq_governor *governor);
 int devfreq_remove_governor(struct devfreq_governor *governor);
 
 int devfreq_update_status(struct devfreq *devfreq, unsigned long freq);
+int devfreq_update_target(struct devfreq *devfreq, unsigned long freq);
 
 static inline int devfreq_update_stats(struct devfreq *df)
 {
diff --git a/drivers/devfreq/governor_passive.c b/drivers/devfreq/governor_passive.c
index 432a4cc683f7..8deb071d5d26 100644
--- a/drivers/devfreq/governor_passive.c
+++ b/drivers/devfreq/governor_passive.c
@@ -92,36 +92,6 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
 	return ret;
 }
 
-static int update_devfreq_passive(struct devfreq *devfreq, unsigned long freq)
-{
-	int ret;
-
-	if (!devfreq->governor)
-		return -EINVAL;
-
-	mutex_lock_nested(&devfreq->lock, SINGLE_DEPTH_NESTING);
-
-	ret = devfreq->governor->get_target_freq(devfreq, &freq);
-	if (ret < 0)
-		goto out;
-
-	ret = devfreq->profile->target(devfreq->dev.parent, &freq, 0);
-	if (ret < 0)
-		goto out;
-
-	if (devfreq->profile->freq_table
-		&& (devfreq_update_status(devfreq, freq)))
-		dev_err(&devfreq->dev,
-			"Couldn't update frequency transition information.\n");
-
-	devfreq->previous_freq = freq;
-
-out:
-	mutex_unlock(&devfreq->lock);
-
-	return 0;
-}
-
 static int devfreq_passive_notifier_call(struct notifier_block *nb,
 				unsigned long event, void *ptr)
 {
@@ -131,17 +101,25 @@ static int devfreq_passive_notifier_call(struct notifier_block *nb,
 	struct devfreq *parent = (struct devfreq *)data->parent;
 	struct devfreq_freqs *freqs = (struct devfreq_freqs *)ptr;
 	unsigned long freq = freqs->new;
+	int ret = 0;
 
+	mutex_lock_nested(&devfreq->lock, SINGLE_DEPTH_NESTING);
 	switch (event) {
 	case DEVFREQ_PRECHANGE:
 		if (parent->previous_freq > freq)
-			update_devfreq_passive(devfreq, freq);
+			ret = devfreq_update_target(devfreq, freq);
+
 		break;
 	case DEVFREQ_POSTCHANGE:
 		if (parent->previous_freq < freq)
-			update_devfreq_passive(devfreq, freq);
+			ret = devfreq_update_target(devfreq, freq);
 		break;
 	}
+	mutex_unlock(&devfreq->lock);
+
+	if (ret < 0)
+		dev_warn(&devfreq->dev,
+			"failed to update devfreq using passive governor\n");
 
 	return NOTIFY_DONE;
 }
-- 
2.17.1

[PATCH 3/3] PM / devfreq: Add tracepoint for frequency changes

[Chanwoo Choi]

From: Matthias Kaehlcke <mka@chromium.org>

Add a tracepoint for frequency changes of devfreq devices and
use it.

Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
[cw00.choi: Move print position of tracepoint and add more information]
Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
---
 drivers/devfreq/devfreq.c      |  8 ++++++++
 include/trace/events/devfreq.h | 28 ++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 5b069a8a1026..d4c4aa050efa 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -369,6 +369,14 @@ static int devfreq_set_target(struct devfreq *devfreq, unsigned long new_freq,
 		return err;
 	}
 
+	/*
+	 * Print devfreq_frequency trace information between DEVFREQ_PRECHANGE
+	 * and DEVFREQ_POSTCHANGE because for showing the correct frequency
+	 * change order of between devfreq device and passive devfreq device.
+	 */
+	if (trace_devfreq_frequency_enabled() && new_freq != cur_freq)
+		trace_devfreq_frequency(devfreq, new_freq, cur_freq);
+
 	freqs.new = new_freq;
 	devfreq_notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE);
 
diff --git a/include/trace/events/devfreq.h b/include/trace/events/devfreq.h
index bd36d28d16bc..7627c620bbda 100644
--- a/include/trace/events/devfreq.h
+++ b/include/trace/events/devfreq.h
@@ -8,6 +8,34 @@
 #include <linux/devfreq.h>
 #include <linux/tracepoint.h>
 
+TRACE_EVENT(devfreq_frequency,
+	TP_PROTO(struct devfreq *devfreq, unsigned long freq,
+		 unsigned long prev_freq),
+
+	TP_ARGS(devfreq, freq, prev_freq),
+
+	TP_STRUCT__entry(
+		__string(dev_name, dev_name(&devfreq->dev))
+		__field(unsigned long, freq)
+		__field(unsigned long, prev_freq)
+		__field(unsigned long, busy_time)
+		__field(unsigned long, total_time)
+	),
+
+	TP_fast_assign(
+		__assign_str(dev_name, dev_name(&devfreq->dev));
+		__entry->freq = freq;
+		__entry->prev_freq = prev_freq;
+		__entry->busy_time = devfreq->last_status.busy_time;
+		__entry->total_time = devfreq->last_status.total_time;
+	),
+
+	TP_printk("dev_name=%-30s freq=%-12lu prev_freq=%-12lu load=%-2lu",
+		__get_str(dev_name), __entry->freq, __entry->prev_freq,
+		__entry->total_time == 0 ? 0 :
+			(100 * __entry->busy_time) / __entry->total_time)
+);
+
 TRACE_EVENT(devfreq_monitor,
 	TP_PROTO(struct devfreq *devfreq),
 
-- 
2.17.1

Re: [PATCH 0/3] PM / devfreq: Add devfreq_frequency tracepoint to track frequency change

[Chanwoo Choi]

On 10/8/20 4:54 PM, Chanwoo Choi wrote:
> Add devfreq_tracepoint to track the correct timing of frequency change
> with following information:
> - device name
> - current frequency
> - previous frequency
> - load when change frequency
> - tracepoint path : /sys/kernel/debug/tracing/events/devfreq_frequency
> 
> And add devfreq_update_target() function to unify the frequency change code
> on both devfreq core and devfreq passive governor because there are redundant
> duplicate code. Lastly, Use fixed indentation size to improve readability
> for 'devfreq_monitor' tracepoint.
> 
> Matthias already sent the patch[1]. Make patch3 by editing patch[1].
> [1]https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg2108015.html
> 
> Chanwoo Choi (2):
>   trace: events: devfreq: Use fixed indentation size to improve readability
>   PM / devfreq: Unify frequency change to devfreq_update_target func
> 
> Matthias Kaehlcke (1):
>   PM / devfreq: Add tracepoint for frequency changes
> 
>  drivers/devfreq/devfreq.c          | 37 +++++++++++++++++++++-----
>  drivers/devfreq/governor.h         |  1 +
>  drivers/devfreq/governor_passive.c | 42 +++++++-----------------------
>  include/trace/events/devfreq.h     | 30 ++++++++++++++++++++-
>  4 files changed, 70 insertions(+), 40 deletions(-)
> 

Applied them. Thanks.

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics