Skip to contentSkip to conversation

PM / core: Introduce some helper for better Code reuse

PATCH In review Yangtao Li 3 participants 7 messages Mar 16, 2019 – Apr 10, 2019

Subsystems (4)
Driver core Suspend to RAM Hibernation Power management core
lore
Share & tools
lore

Yangtao Li

author
#0 lore raw

cover PM / core: Introduce some helper for better Code reuse

This patch set introduces some functions and macros that help reduce code duplication.

Yangtao Li (4):
PM / core: Introduce dpm_async_fn() helper
PM / core: Introduce DEVICE_SUSPEND_FUNC() helper macro PM / core: Introduce ASYNC_RESUME_FUNC() helper macro PM / core: Introduce ASYNC_SUSPEND_FUNC() helper macro

 drivers/base/power/main.c | 182 ++++++++++++--------------------------
 1 file changed, 59 insertions(+), 123 deletions(-)
Signature · 1 line
2.17.0
author
#1 lore raw

PATCH 1/4 PM / core: Introduce dpm_async_fn() helper

When we want to execute device pm functions asynchronously, we'll do the following for the device:

1) reinit_completion(&dev->power.completion);
2) Check if the device enables asynchronous suspend.
3) If necessary, execute the corresponding function asynchronously.

There are a lot of such repeated operations here, in fact we can avoid this. So introduce dpm_async_fn() to have better code readability and reuse.

And use this function to do some cleanup.

Signed-off-by Yangtao Li <[email protected]>
---
 drivers/base/power/main.c | 62 +++++++++++++++------------------------
 1 file changed, 23 insertions(+), 39 deletions(-)
drivers/base/power/main.c +23 -39
index eddb54057ed6..cb44bb6b2b66 100644
@@ -706,6 +706,19 @@ static bool is_async(struct device *dev)
 		&& !pm_trace_is_enabled();
 }
 
+static bool dpm_async_fn(struct device *dev, async_func_t func)
+{
+	reinit_completion(&dev->power.completion);
+
+	if (is_async(dev)) {
+		get_device(dev);
+		async_schedule(func, dev);
+		return true;
+	}
+
+	return false;
+}
+
 static void async_resume_noirq(void *data, async_cookie_t cookie)
 {
 	struct device *dev = (struct device *)data;

@@ -732,13 +745,8 @@ void dpm_noirq_resume_devices(pm_message_t state)
 	 * in case the starting of async threads is
 	 * delayed by non-async resuming devices.
 	 */
-	list_for_each_entry(dev, &dpm_noirq_list, power.entry) {
-		reinit_completion(&dev->power.completion);
-		if (is_async(dev)) {
-			get_device(dev);
-			async_schedule_dev(async_resume_noirq, dev);
-		}
-	}
+	list_for_each_entry(dev, &dpm_noirq_list, power.entry)
+		dpm_async_fn(dev, async_resume_noirq);
 
 	while (!list_empty(&dpm_noirq_list)) {
 		dev = to_device(dpm_noirq_list.next);

@@ -889,13 +897,8 @@ void dpm_resume_early(pm_message_t state)
 	 * in case the starting of async threads is
 	 * delayed by non-async resuming devices.
 	 */
-	list_for_each_entry(dev, &dpm_late_early_list, power.entry) {
-		reinit_completion(&dev->power.completion);
-		if (is_async(dev)) {
-			get_device(dev);
-			async_schedule_dev(async_resume_early, dev);
-		}
-	}
+	list_for_each_entry(dev, &dpm_late_early_list, power.entry)
+		dpm_async_fn(dev, async_resume_early);
 
 	while (!list_empty(&dpm_late_early_list)) {
 		dev = to_device(dpm_late_early_list.next);

@@ -1053,13 +1056,8 @@ void dpm_resume(pm_message_t state)
 	pm_transition = state;
 	async_error = 0;
 
-	list_for_each_entry(dev, &dpm_suspended_list, power.entry) {
-		reinit_completion(&dev->power.completion);
-		if (is_async(dev)) {
-			get_device(dev);
-			async_schedule_dev(async_resume, dev);
-		}
-	}
+	list_for_each_entry(dev, &dpm_suspended_list, power.entry)
+		dpm_async_fn(dev, async_resume);
 
 	while (!list_empty(&dpm_suspended_list)) {
 		dev = to_device(dpm_suspended_list.next);

@@ -1373,13 +1371,9 @@ static void async_suspend_noirq(void *data, async_cookie_t cookie)
 
 static int device_suspend_noirq(struct device *dev)
 {
-	reinit_completion(&dev->power.completion);
-
-	if (is_async(dev)) {
-		get_device(dev);
-		async_schedule_dev(async_suspend_noirq, dev);
+	if (dpm_async_fn(dev, async_suspend_noirq))
 		return 0;
-	}
+
 	return __device_suspend_noirq(dev, pm_transition, false);
 }
 

@@ -1576,13 +1570,8 @@ static void async_suspend_late(void *data, async_cookie_t cookie)
 
 static int device_suspend_late(struct device *dev)
 {
-	reinit_completion(&dev->power.completion);
-
-	if (is_async(dev)) {
-		get_device(dev);
-		async_schedule_dev(async_suspend_late, dev);
+	if (dpm_async_fn(dev, async_suspend_late))
 		return 0;
-	}
 
 	return __device_suspend_late(dev, pm_transition, false);
 }

@@ -1842,13 +1831,8 @@ static void async_suspend(void *data, async_cookie_t cookie)
 
 static int device_suspend(struct device *dev)
 {
-	reinit_completion(&dev->power.completion);
-
-	if (is_async(dev)) {
-		get_device(dev);
-		async_schedule_dev(async_suspend, dev);
+	if (dpm_async_fn(dev, async_suspend))
 		return 0;
-	}
 
 	return __device_suspend(dev, pm_transition, false);
 }
Signature · 1 line
2.17.0
#2 lore raw

Re: [PATCH 1/4] PM / core: Introduce dpm_async_fn() helper

Quoted 7 lines · CEST Yangtao Li

When we want to execute device pm functions asynchronously, we'll do the following for the device:

1) reinit_completion(&dev->power.completion);
2) Check if the device enables asynchronous suspend.
3) If necessary, execute the corresponding function asynchronously.

Quoted 7 lines

There are a lot of such repeated operations here, in fact we can avoid this. So introduce dpm_async_fn() to have better code readability and reuse.

And use this function to do some cleanup.

Signed-off-by Yangtao Li <[email protected]>

I'm queuing up this one, but the [2-4/4] are not convincing.

Thanks!

author
#3 lore raw

PATCH 2/4 PM / core: Introduce DEVICE_SUSPEND_FUNC() helper macro

In reply to Yangtao Li

The devices_suspend_noirq, device_suspend_late, device_suspen functions are basically the same. As we have seen:

static int device_xxx(struct device *dev)

{
       if (dpm_async_fn(dev, async_xxx))
               return 0;

       return __device_xxx(dev, pm_transition, false);
}

The DEVICE_SUSPEND_FUNC() helper macro can decrease code duplication.

Signed-off-by Yangtao Li <[email protected]>
---
 drivers/base/power/main.c | 33 ++++++++++++---------------------
 1 file changed, 12 insertions(+), 21 deletions(-)
drivers/base/power/main.c +12 -21
index cb44bb6b2b66..6026bda5e787 100644
@@ -719,6 +719,15 @@ static bool dpm_async_fn(struct device *dev, async_func_t func)
 	return false;
 }
 
+#define DEVICE_SUSPEND_FUNC(__func, __name)				\
+static int __func(struct device *dev)					\
+{									\
+	if (dpm_async_fn(dev, async_ ## __name))			\
+		return 0;						\
+									\
+	return __device_ ## __name(dev, pm_transition, false);		\
+}
+
 static void async_resume_noirq(void *data, async_cookie_t cookie)
 {
 	struct device *dev = (struct device *)data;

@@ -1369,13 +1378,7 @@ static void async_suspend_noirq(void *data, async_cookie_t cookie)
 	put_device(dev);
 }
 
-static int device_suspend_noirq(struct device *dev)
-{
-	if (dpm_async_fn(dev, async_suspend_noirq))
-		return 0;
-
-	return __device_suspend_noirq(dev, pm_transition, false);
-}
+DEVICE_SUSPEND_FUNC(device_suspend_noirq, suspend_noirq);
 
 void dpm_noirq_begin(void)
 {

@@ -1568,13 +1571,7 @@ static void async_suspend_late(void *data, async_cookie_t cookie)
 	put_device(dev);
 }
 
-static int device_suspend_late(struct device *dev)
-{
-	if (dpm_async_fn(dev, async_suspend_late))
-		return 0;
-
-	return __device_suspend_late(dev, pm_transition, false);
-}
+DEVICE_SUSPEND_FUNC(device_suspend_late, suspend_late);
 
 /**
  * dpm_suspend_late - Execute "late suspend" callbacks for all devices.

@@ -1829,13 +1826,7 @@ static void async_suspend(void *data, async_cookie_t cookie)
 	put_device(dev);
 }
 
-static int device_suspend(struct device *dev)
-{
-	if (dpm_async_fn(dev, async_suspend))
-		return 0;
-
-	return __device_suspend(dev, pm_transition, false);
-}
+DEVICE_SUSPEND_FUNC(device_suspend, suspend);
 
 /**
  * dpm_suspend - Execute "suspend" callbacks for all non-sysdev devices.
Signature · 1 line
2.17.0
author
#4 lore raw

PATCH 3/4 PM / core: Introduce ASYNC_RESUME_FUNC() helper macro

In reply to Yangtao Li

The async_resume_noirq, async_resume_early, async_resume functions are basically the same. As we have seen:

static void async_xxx(void *data, async_cookie_t cookie)

{
	struct device *dev = (struct device *)data;
	int error;

	error = device_xxx(dev, pm_transition, true);
	if (error)
		pm_dev_err(dev, pm_transition, " async", error);
	put_device(dev);
}

The ASYNC_RESUME_FUNC() helper macro can decrease code duplication.

Signed-off-by Yangtao Li <[email protected]>
---
 drivers/base/power/main.c | 46 +++++++++++++--------------------------
 1 file changed, 15 insertions(+), 31 deletions(-)
drivers/base/power/main.c +15 -31
index 6026bda5e787..d512bee9d9ca 100644
@@ -728,18 +728,21 @@ static int __func(struct device *dev) 					\
 	return __device_ ## __name(dev, pm_transition, false);		\
 }
 
-static void async_resume_noirq(void *data, async_cookie_t cookie)
-{
-	struct device *dev = (struct device *)data;
-	int error;
-
-	error = device_resume_noirq(dev, pm_transition, true);
-	if (error)
-		pm_dev_err(dev, pm_transition, " async", error);
-
-	put_device(dev);
+#define ASYNC_RESUME_FUNC(__func, __name)				\
+static void __func(void *data, async_cookie_t cookie)			\
+{									\
+	struct device *dev = (struct device *)data;			\
+	int error;							\
+									\
+	error = device_ ## __name(dev, pm_transition, true);		\
+	if (error)							\
+		pm_dev_err(dev, pm_transition, " async", error);	\
+									\
+	put_device(dev);						\
 }
 
+ASYNC_RESUME_FUNC(async_resume_noirq, resume_noirq);
+
 void dpm_noirq_resume_devices(pm_message_t state)
 {
 	struct device *dev;

@@ -876,17 +879,7 @@ static int device_resume_early(struct device *dev, pm_message_t state, bool asyn
 	return error;
 }
 
-static void async_resume_early(void *data, async_cookie_t cookie)
-{
-	struct device *dev = (struct device *)data;
-	int error;
-
-	error = device_resume_early(dev, pm_transition, true);
-	if (error)
-		pm_dev_err(dev, pm_transition, " async", error);
-
-	put_device(dev);
-}
+ASYNC_RESUME_FUNC(async_resume_early, resume_early);
 
 /**
  * dpm_resume_early - Execute "early resume" callbacks for all devices.

@@ -1035,16 +1028,7 @@ static int device_resume(struct device *dev, pm_message_t state, bool async)
 	return error;
 }
 
-static void async_resume(void *data, async_cookie_t cookie)
-{
-	struct device *dev = (struct device *)data;
-	int error;
-
-	error = device_resume(dev, pm_transition, true);
-	if (error)
-		pm_dev_err(dev, pm_transition, " async", error);
-	put_device(dev);
-}
+ASYNC_RESUME_FUNC(async_resume, resume);
 
 /**
  * dpm_resume - Execute "resume" callbacks for non-sysdev devices.
Signature · 1 line
2.17.0
author
#5 lore raw

PATCH 4/4 PM / core: Introduce ASYNC_SUSPEND_FUNC() helper macro

In reply to Yangtao Li

The async_suspend_noirq, async_suspend_late, async_suspend functions are basically the same. As we have seen:

static void async_xxx(void *data, async_cookie_t cookie)

{
	struct device *dev = (struct device *)data;
	int error;

	error = __device_xxx(dev, pm_transition, true);
	if (error) {
		dpm_save_failed_dev(dev_name(dev));
		pm_dev_err(dev, pm_transition, " async", error);
	}

	put_device(dev);
}

The ASYNC_SUSPEND_FUNC() helper macro can decrease code duplication.

Signed-off-by Yangtao Li <[email protected]>
---
 drivers/base/power/main.c | 55 ++++++++++++---------------------------
 1 file changed, 16 insertions(+), 39 deletions(-)
drivers/base/power/main.c +16 -39
index d512bee9d9ca..3882dc5fee9f 100644
@@ -1348,20 +1348,22 @@ static int __device_suspend_noirq(struct device *dev, pm_message_t state, bool a
 	return error;
 }
 
-static void async_suspend_noirq(void *data, async_cookie_t cookie)
-{
-	struct device *dev = (struct device *)data;
-	int error;
-
-	error = __device_suspend_noirq(dev, pm_transition, true);
-	if (error) {
-		dpm_save_failed_dev(dev_name(dev));
-		pm_dev_err(dev, pm_transition, " async", error);
-	}
-
-	put_device(dev);
+#define ASYNC_SUSPEND_FUNC(__func, __name)				\
+static void __func(void *data, async_cookie_t cookie)			\
+{									\
+	struct device *dev = (struct device *)data;			\
+	int error;							\
+									\
+	error = __device_ ## __name(dev, pm_transition, true);		\
+	if (error) {							\
+		dpm_save_failed_dev(dev_name(dev));			\
+		pm_dev_err(dev, pm_transition, " async", error);	\
+	}								\
+									\
+	put_device(dev);						\
 }
 
+ASYNC_SUSPEND_FUNC(async_suspend_noirq, suspend_noirq);
 DEVICE_SUSPEND_FUNC(device_suspend_noirq, suspend_noirq);
 
 void dpm_noirq_begin(void)

@@ -1542,19 +1544,7 @@ static int __device_suspend_late(struct device *dev, pm_message_t state, bool as
 	return error;
 }
 
-static void async_suspend_late(void *data, async_cookie_t cookie)
-{
-	struct device *dev = (struct device *)data;
-	int error;
-
-	error = __device_suspend_late(dev, pm_transition, true);
-	if (error) {
-		dpm_save_failed_dev(dev_name(dev));
-		pm_dev_err(dev, pm_transition, " async", error);
-	}
-	put_device(dev);
-}
-
+ASYNC_SUSPEND_FUNC(async_suspend_late, suspend_late);
 DEVICE_SUSPEND_FUNC(device_suspend_late, suspend_late);
 
 /**

@@ -1796,20 +1786,7 @@ static int __device_suspend(struct device *dev, pm_message_t state, bool async)
 	return error;
 }
 
-static void async_suspend(void *data, async_cookie_t cookie)
-{
-	struct device *dev = (struct device *)data;
-	int error;
-
-	error = __device_suspend(dev, pm_transition, true);
-	if (error) {
-		dpm_save_failed_dev(dev_name(dev));
-		pm_dev_err(dev, pm_transition, " async", error);
-	}
-
-	put_device(dev);
-}
-
+ASYNC_SUSPEND_FUNC(async_suspend, suspend);
 DEVICE_SUSPEND_FUNC(device_suspend, suspend);
 
 /**
Signature · 1 line
2.17.0
#6 lore raw
In reply to Yangtao Li
Quoted 11 lines · Yangtao Li

This patch set introduces some functions and macros that help reduce code duplication.

Yangtao Li (4):
PM / core: Introduce dpm_async_fn() helper
PM / core: Introduce DEVICE_SUSPEND_FUNC() helper macro PM / core: Introduce ASYNC_RESUME_FUNC() helper macro PM / core: Introduce ASYNC_SUSPEND_FUNC() helper macro

 drivers/base/power/main.c | 182 ++++++++++++--------------------------
 1 file changed, 59 insertions(+), 123 deletions(-)

I'm not a big fan. Yes, you got line count down. But no, I do not think "beauty" of the macros makes it worth it.
Pavel

Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html