linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] PM: simplify the code logic on device_shutdown and suspend
@ 2018-08-20  9:18 Pingfan Liu
  2018-08-20  9:18 ` [PATCH 1/3] drivers/base: move device_shutdown() to base/power Pingfan Liu
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Pingfan Liu @ 2018-08-20  9:18 UTC (permalink / raw)
  To: linux-pm; +Cc: Pingfan Liu, Greg Kroah-Hartman, Rafael J. Wysocki, linux-kernel

At present, the "parent <- child" and "suppiler <- consumer" ordering info
are stored duplicate in two places dpm_list and devices_kset, and
corresponding, there are two sets of routines to manipulate them. The patch
pushes the dpm_list and dpm_list_mtx out of CONFIG_PM, and let
device_shutdown() use them to implement shutdown seq. Comparing to original
code, this patch tries to simplify the code at the cost of a extra mutex if
without CONFIG_PM and nothing if with CONFIG_PM. The cost on dpm_list_mtx
can be ignored, since the device hot add/remove is not very frequently.

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: "Rafael J. Wysocki" <rafael@kernel.org> 
Cc: linux-kernel@vger.kernel.org
---
Pingfan Liu (3):
  drivers/base: move device_shutdown() to base/power
  PM/shutdown: device_shutdown() uses the order info in dpm_list instead
    of     devices_kset
  drivers/base: clean up unused devices_kset_move_*() functions

 drivers/base/base.h           |   1 -
 drivers/base/core.c           | 126 ------------------------------------------
 drivers/base/power/Makefile   |   1 +
 drivers/base/power/main.c     | 119 +++++++++++----------------------------
 drivers/base/power/power.h    |  25 ++++++---
 drivers/base/power/shutdown.c | 123 +++++++++++++++++++++++++++++++++++++++++
 include/linux/pm.h            |   7 +--
 7 files changed, 173 insertions(+), 229 deletions(-)
 create mode 100644 drivers/base/power/shutdown.c

-- 
2.7.4


^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH 1/3] drivers/base: move device_shutdown() to base/power
  2018-08-20  9:18 [PATCH 0/3] PM: simplify the code logic on device_shutdown and suspend Pingfan Liu
@ 2018-08-20  9:18 ` Pingfan Liu
  2018-09-14  9:07   ` Rafael J. Wysocki
  2018-08-20  9:18 ` [PATCH 2/3] PM/shutdown: device_shutdown() uses the order info in dpm_list instead of devices_kset Pingfan Liu
  2018-08-20  9:18 ` [PATCH 3/3] drivers/base: clean up unused devices_kset_move_*() functions Pingfan Liu
  2 siblings, 1 reply; 6+ messages in thread
From: Pingfan Liu @ 2018-08-20  9:18 UTC (permalink / raw)
  To: linux-pm; +Cc: Pingfan Liu, Greg Kroah-Hartman, Rafael J. Wysocki, linux-kernel

Consider the shutdown as a system state transition, i.e. something like
suspend, hibernate, hence move it under the base/power. (This is a first
step to unify the duplicate code logic on devices_kset and dpm_list.)

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
---
 drivers/base/core.c           | 70 ---------------------------------------
 drivers/base/power/Makefile   |  1 +
 drivers/base/power/shutdown.c | 76 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 77 insertions(+), 70 deletions(-)
 create mode 100644 drivers/base/power/shutdown.c

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 04bbcd7..7c83384 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -2854,76 +2854,6 @@ int device_move(struct device *dev, struct device *new_parent,
 }
 EXPORT_SYMBOL_GPL(device_move);
 
-/**
- * device_shutdown - call ->shutdown() on each device to shutdown.
- */
-void device_shutdown(void)
-{
-	struct device *dev, *parent;
-
-	wait_for_device_probe();
-	device_block_probing();
-
-	spin_lock(&devices_kset->list_lock);
-	/*
-	 * Walk the devices list backward, shutting down each in turn.
-	 * Beware that device unplug events may also start pulling
-	 * devices offline, even as the system is shutting down.
-	 */
-	while (!list_empty(&devices_kset->list)) {
-		dev = list_entry(devices_kset->list.prev, struct device,
-				kobj.entry);
-
-		/*
-		 * hold reference count of device's parent to
-		 * prevent it from being freed because parent's
-		 * lock is to be held
-		 */
-		parent = get_device(dev->parent);
-		get_device(dev);
-		/*
-		 * Make sure the device is off the kset list, in the
-		 * event that dev->*->shutdown() doesn't remove it.
-		 */
-		list_del_init(&dev->kobj.entry);
-		spin_unlock(&devices_kset->list_lock);
-
-		/* hold lock to avoid race with probe/release */
-		if (parent)
-			device_lock(parent);
-		device_lock(dev);
-
-		/* Don't allow any more runtime suspends */
-		pm_runtime_get_noresume(dev);
-		pm_runtime_barrier(dev);
-
-		if (dev->class && dev->class->shutdown_pre) {
-			if (initcall_debug)
-				dev_info(dev, "shutdown_pre\n");
-			dev->class->shutdown_pre(dev);
-		}
-		if (dev->bus && dev->bus->shutdown) {
-			if (initcall_debug)
-				dev_info(dev, "shutdown\n");
-			dev->bus->shutdown(dev);
-		} else if (dev->driver && dev->driver->shutdown) {
-			if (initcall_debug)
-				dev_info(dev, "shutdown\n");
-			dev->driver->shutdown(dev);
-		}
-
-		device_unlock(dev);
-		if (parent)
-			device_unlock(parent);
-
-		put_device(dev);
-		put_device(parent);
-
-		spin_lock(&devices_kset->list_lock);
-	}
-	spin_unlock(&devices_kset->list_lock);
-}
-
 /*
  * Device logging functions
  */
diff --git a/drivers/base/power/Makefile b/drivers/base/power/Makefile
index e1bb691..057ddbf 100644
--- a/drivers/base/power/Makefile
+++ b/drivers/base/power/Makefile
@@ -1,4 +1,5 @@
 # SPDX-License-Identifier: GPL-2.0
+obj-y	+= shutdown.o
 obj-$(CONFIG_PM)	+= sysfs.o generic_ops.o common.o qos.o runtime.o wakeirq.o
 obj-$(CONFIG_PM_SLEEP)	+= main.o wakeup.o
 obj-$(CONFIG_PM_TRACE_RTC)	+= trace.o
diff --git a/drivers/base/power/shutdown.c b/drivers/base/power/shutdown.c
new file mode 100644
index 0000000..c405d09
--- /dev/null
+++ b/drivers/base/power/shutdown.c
@@ -0,0 +1,76 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/device.h>
+#include <linux/mutex.h>
+#include <linux/pm_runtime.h>
+
+#include "../base.h"
+#include "power.h"
+/**
+ * device_shutdown - call ->shutdown() on each device to shutdown.
+ */
+void device_shutdown(void)
+{
+	struct device *dev, *parent;
+
+	wait_for_device_probe();
+	device_block_probing();
+
+	spin_lock(&devices_kset->list_lock);
+	/*
+	 * Walk the devices list backward, shutting down each in turn.
+	 * Beware that device unplug events may also start pulling
+	 * devices offline, even as the system is shutting down.
+	 */
+	while (!list_empty(&devices_kset->list)) {
+		dev = list_entry(devices_kset->list.prev, struct device,
+				kobj.entry);
+
+		/*
+		 * hold reference count of device's parent to
+		 * prevent it from being freed because parent's
+		 * lock is to be held
+		 */
+		parent = get_device(dev->parent);
+		get_device(dev);
+		/*
+		 * Make sure the device is off the kset list, in the
+		 * event that dev->*->shutdown() doesn't remove it.
+		 */
+		list_del_init(&dev->kobj.entry);
+		spin_unlock(&devices_kset->list_lock);
+
+		/* hold lock to avoid race with probe/release */
+		if (parent)
+			device_lock(parent);
+		device_lock(dev);
+
+		/* Don't allow any more runtime suspends */
+		pm_runtime_get_noresume(dev);
+		pm_runtime_barrier(dev);
+
+		if (dev->class && dev->class->shutdown_pre) {
+			if (initcall_debug)
+				dev_info(dev, "shutdown_pre\n");
+			dev->class->shutdown_pre(dev);
+		}
+		if (dev->bus && dev->bus->shutdown) {
+			if (initcall_debug)
+				dev_info(dev, "shutdown\n");
+			dev->bus->shutdown(dev);
+		} else if (dev->driver && dev->driver->shutdown) {
+			if (initcall_debug)
+				dev_info(dev, "shutdown\n");
+			dev->driver->shutdown(dev);
+		}
+
+		device_unlock(dev);
+		if (parent)
+			device_unlock(parent);
+
+		put_device(dev);
+		put_device(parent);
+
+		spin_lock(&devices_kset->list_lock);
+	}
+	spin_unlock(&devices_kset->list_lock);
+}
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [PATCH 2/3] PM/shutdown: device_shutdown() uses the order info in dpm_list instead of devices_kset
  2018-08-20  9:18 [PATCH 0/3] PM: simplify the code logic on device_shutdown and suspend Pingfan Liu
  2018-08-20  9:18 ` [PATCH 1/3] drivers/base: move device_shutdown() to base/power Pingfan Liu
@ 2018-08-20  9:18 ` Pingfan Liu
  2018-08-20  9:18 ` [PATCH 3/3] drivers/base: clean up unused devices_kset_move_*() functions Pingfan Liu
  2 siblings, 0 replies; 6+ messages in thread
From: Pingfan Liu @ 2018-08-20  9:18 UTC (permalink / raw)
  To: linux-pm; +Cc: Pingfan Liu, Greg Kroah-Hartman, Rafael J. Wysocki, linux-kernel

At present, the "parent <- child" and "suppiler <- consumer" ordering info
are stored duplicate in two places dpm_list and devices_kset, and
corresponding, there are two sets of routines to manipulate them. The patch
pushes the dpm_list and dpm_list_mtx out of CONFIG_PM, and let
device_shutdown() use them to implement shutdown seq. Comparing to original
code, this patch tries to simplify the code at the cost of a extra mutex if
without CONFIG_PM and nothing if with CONFIG_PM. The cost on dpm_list_mtx
can be ignored, since the device hot add/remove is not very frequently.

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
---
 drivers/base/power/main.c     | 119 ++++++++++++------------------------------
 drivers/base/power/power.h    |  25 +++++----
 drivers/base/power/shutdown.c |  63 +++++++++++++++++++---
 include/linux/pm.h            |   7 +--
 4 files changed, 104 insertions(+), 110 deletions(-)

diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
index 3f68e29..7b2f316 100644
--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -49,14 +49,12 @@ typedef int (*pm_callback_t)(struct device *);
  * dpm_list_mutex.
  */
 
-LIST_HEAD(dpm_list);
 static LIST_HEAD(dpm_prepared_list);
 static LIST_HEAD(dpm_suspended_list);
 static LIST_HEAD(dpm_late_early_list);
 static LIST_HEAD(dpm_noirq_list);
 
 struct suspend_stats suspend_stats;
-static DEFINE_MUTEX(dpm_list_mtx);
 static pm_message_t pm_transition;
 
 static int async_error;
@@ -98,59 +96,6 @@ void device_pm_sleep_init(struct device *dev)
 	init_completion(&dev->power.completion);
 	complete_all(&dev->power.completion);
 	dev->power.wakeup = NULL;
-	INIT_LIST_HEAD(&dev->power.entry);
-}
-
-/**
- * device_pm_lock - Lock the list of active devices used by the PM core.
- */
-void device_pm_lock(void)
-{
-	mutex_lock(&dpm_list_mtx);
-}
-
-/**
- * device_pm_unlock - Unlock the list of active devices used by the PM core.
- */
-void device_pm_unlock(void)
-{
-	mutex_unlock(&dpm_list_mtx);
-}
-
-/**
- * device_pm_add - Add a device to the PM core's list of active devices.
- * @dev: Device to add to the list.
- */
-void device_pm_add(struct device *dev)
-{
-	pr_debug("PM: Adding info for %s:%s\n",
-		 dev->bus ? dev->bus->name : "No Bus", dev_name(dev));
-	device_pm_check_callbacks(dev);
-	mutex_lock(&dpm_list_mtx);
-	if (dev->parent && dev->parent->power.is_prepared)
-		dev_warn(dev, "parent %s should not be sleeping\n",
-			dev_name(dev->parent));
-	list_add_tail(&dev->power.entry, &dpm_list);
-	dev->power.in_dpm_list = true;
-	mutex_unlock(&dpm_list_mtx);
-}
-
-/**
- * device_pm_remove - Remove a device from the PM core's list of active devices.
- * @dev: Device to be removed from the list.
- */
-void device_pm_remove(struct device *dev)
-{
-	pr_debug("PM: Removing info for %s:%s\n",
-		 dev->bus ? dev->bus->name : "No Bus", dev_name(dev));
-	complete_all(&dev->power.completion);
-	mutex_lock(&dpm_list_mtx);
-	list_del_init(&dev->power.entry);
-	dev->power.in_dpm_list = false;
-	mutex_unlock(&dpm_list_mtx);
-	device_wakeup_disable(dev);
-	pm_runtime_remove(dev);
-	device_pm_check_callbacks(dev);
 }
 
 /**
@@ -714,7 +659,7 @@ void dpm_noirq_resume_devices(pm_message_t state)
 	ktime_t starttime = ktime_get();
 
 	trace_suspend_resume(TPS("dpm_resume_noirq"), state.event, true);
-	mutex_lock(&dpm_list_mtx);
+	device_pm_lock();
 	pm_transition = state;
 
 	/*
@@ -734,7 +679,7 @@ void dpm_noirq_resume_devices(pm_message_t state)
 		dev = to_device(dpm_noirq_list.next);
 		get_device(dev);
 		list_move_tail(&dev->power.entry, &dpm_late_early_list);
-		mutex_unlock(&dpm_list_mtx);
+		device_pm_unlock();
 
 		if (!is_async(dev)) {
 			int error;
@@ -748,10 +693,10 @@ void dpm_noirq_resume_devices(pm_message_t state)
 			}
 		}
 
-		mutex_lock(&dpm_list_mtx);
+		device_pm_lock();
 		put_device(dev);
 	}
-	mutex_unlock(&dpm_list_mtx);
+	device_pm_unlock();
 	async_synchronize_full();
 	dpm_show_time(starttime, state, 0, "noirq");
 	trace_suspend_resume(TPS("dpm_resume_noirq"), state.event, false);
@@ -871,7 +816,7 @@ void dpm_resume_early(pm_message_t state)
 	ktime_t starttime = ktime_get();
 
 	trace_suspend_resume(TPS("dpm_resume_early"), state.event, true);
-	mutex_lock(&dpm_list_mtx);
+	device_pm_lock();
 	pm_transition = state;
 
 	/*
@@ -891,7 +836,7 @@ void dpm_resume_early(pm_message_t state)
 		dev = to_device(dpm_late_early_list.next);
 		get_device(dev);
 		list_move_tail(&dev->power.entry, &dpm_suspended_list);
-		mutex_unlock(&dpm_list_mtx);
+		device_pm_unlock();
 
 		if (!is_async(dev)) {
 			int error;
@@ -904,10 +849,10 @@ void dpm_resume_early(pm_message_t state)
 				pm_dev_err(dev, state, " early", error);
 			}
 		}
-		mutex_lock(&dpm_list_mtx);
+		device_pm_lock();
 		put_device(dev);
 	}
-	mutex_unlock(&dpm_list_mtx);
+	device_pm_unlock();
 	async_synchronize_full();
 	dpm_show_time(starttime, state, 0, "early");
 	trace_suspend_resume(TPS("dpm_resume_early"), state.event, false);
@@ -1039,7 +984,7 @@ void dpm_resume(pm_message_t state)
 	trace_suspend_resume(TPS("dpm_resume"), state.event, true);
 	might_sleep();
 
-	mutex_lock(&dpm_list_mtx);
+	device_pm_lock();
 	pm_transition = state;
 	async_error = 0;
 
@@ -1057,7 +1002,7 @@ void dpm_resume(pm_message_t state)
 		if (!is_async(dev)) {
 			int error;
 
-			mutex_unlock(&dpm_list_mtx);
+			device_pm_unlock();
 
 			error = device_resume(dev, state, false);
 			if (error) {
@@ -1067,13 +1012,13 @@ void dpm_resume(pm_message_t state)
 				pm_dev_err(dev, state, "", error);
 			}
 
-			mutex_lock(&dpm_list_mtx);
+			device_pm_lock();
 		}
 		if (!list_empty(&dev->power.entry))
 			list_move_tail(&dev->power.entry, &dpm_prepared_list);
 		put_device(dev);
 	}
-	mutex_unlock(&dpm_list_mtx);
+	device_pm_unlock();
 	async_synchronize_full();
 	dpm_show_time(starttime, state, 0, NULL);
 
@@ -1140,24 +1085,24 @@ void dpm_complete(pm_message_t state)
 	might_sleep();
 
 	INIT_LIST_HEAD(&list);
-	mutex_lock(&dpm_list_mtx);
+	device_pm_lock();
 	while (!list_empty(&dpm_prepared_list)) {
 		struct device *dev = to_device(dpm_prepared_list.prev);
 
 		get_device(dev);
 		dev->power.is_prepared = false;
 		list_move(&dev->power.entry, &list);
-		mutex_unlock(&dpm_list_mtx);
+		device_pm_unlock();
 
 		trace_device_pm_callback_start(dev, "", state.event);
 		device_complete(dev, state);
 		trace_device_pm_callback_end(dev, 0);
 
-		mutex_lock(&dpm_list_mtx);
+		device_pm_lock();
 		put_device(dev);
 	}
 	list_splice(&list, &dpm_list);
-	mutex_unlock(&dpm_list_mtx);
+	device_pm_unlock();
 
 	/* Allow device probing and trigger re-probing of deferred devices */
 	device_unblock_probing();
@@ -1385,7 +1330,7 @@ int dpm_noirq_suspend_devices(pm_message_t state)
 	int error = 0;
 
 	trace_suspend_resume(TPS("dpm_suspend_noirq"), state.event, true);
-	mutex_lock(&dpm_list_mtx);
+	device_pm_lock();
 	pm_transition = state;
 	async_error = 0;
 
@@ -1393,11 +1338,11 @@ int dpm_noirq_suspend_devices(pm_message_t state)
 		struct device *dev = to_device(dpm_late_early_list.prev);
 
 		get_device(dev);
-		mutex_unlock(&dpm_list_mtx);
+		device_pm_unlock();
 
 		error = device_suspend_noirq(dev);
 
-		mutex_lock(&dpm_list_mtx);
+		device_pm_lock();
 		if (error) {
 			pm_dev_err(dev, state, " noirq", error);
 			dpm_save_failed_dev(dev_name(dev));
@@ -1411,7 +1356,7 @@ int dpm_noirq_suspend_devices(pm_message_t state)
 		if (async_error)
 			break;
 	}
-	mutex_unlock(&dpm_list_mtx);
+	device_pm_unlock();
 	async_synchronize_full();
 	if (!error)
 		error = async_error;
@@ -1586,7 +1531,7 @@ int dpm_suspend_late(pm_message_t state)
 	int error = 0;
 
 	trace_suspend_resume(TPS("dpm_suspend_late"), state.event, true);
-	mutex_lock(&dpm_list_mtx);
+	device_pm_lock();
 	pm_transition = state;
 	async_error = 0;
 
@@ -1594,11 +1539,11 @@ int dpm_suspend_late(pm_message_t state)
 		struct device *dev = to_device(dpm_suspended_list.prev);
 
 		get_device(dev);
-		mutex_unlock(&dpm_list_mtx);
+		device_pm_unlock();
 
 		error = device_suspend_late(dev);
 
-		mutex_lock(&dpm_list_mtx);
+		device_pm_lock();
 		if (!list_empty(&dev->power.entry))
 			list_move(&dev->power.entry, &dpm_late_early_list);
 
@@ -1613,7 +1558,7 @@ int dpm_suspend_late(pm_message_t state)
 		if (async_error)
 			break;
 	}
-	mutex_unlock(&dpm_list_mtx);
+	device_pm_unlock();
 	async_synchronize_full();
 	if (!error)
 		error = async_error;
@@ -1851,18 +1796,18 @@ int dpm_suspend(pm_message_t state)
 
 	cpufreq_suspend();
 
-	mutex_lock(&dpm_list_mtx);
+	device_pm_lock();
 	pm_transition = state;
 	async_error = 0;
 	while (!list_empty(&dpm_prepared_list)) {
 		struct device *dev = to_device(dpm_prepared_list.prev);
 
 		get_device(dev);
-		mutex_unlock(&dpm_list_mtx);
+		device_pm_unlock();
 
 		error = device_suspend(dev);
 
-		mutex_lock(&dpm_list_mtx);
+		device_pm_lock();
 		if (error) {
 			pm_dev_err(dev, state, "", error);
 			dpm_save_failed_dev(dev_name(dev));
@@ -1875,7 +1820,7 @@ int dpm_suspend(pm_message_t state)
 		if (async_error)
 			break;
 	}
-	mutex_unlock(&dpm_list_mtx);
+	device_pm_unlock();
 	async_synchronize_full();
 	if (!error)
 		error = async_error;
@@ -1989,18 +1934,18 @@ int dpm_prepare(pm_message_t state)
 	 */
 	device_block_probing();
 
-	mutex_lock(&dpm_list_mtx);
+	device_pm_lock();
 	while (!list_empty(&dpm_list)) {
 		struct device *dev = to_device(dpm_list.next);
 
 		get_device(dev);
-		mutex_unlock(&dpm_list_mtx);
+		device_pm_unlock();
 
 		trace_device_pm_callback_start(dev, "", state.event);
 		error = device_prepare(dev, state);
 		trace_device_pm_callback_end(dev, error);
 
-		mutex_lock(&dpm_list_mtx);
+		device_pm_lock();
 		if (error) {
 			if (error == -EAGAIN) {
 				put_device(dev);
@@ -2018,7 +1963,7 @@ int dpm_prepare(pm_message_t state)
 			list_move_tail(&dev->power.entry, &dpm_prepared_list);
 		put_device(dev);
 	}
-	mutex_unlock(&dpm_list_mtx);
+	device_pm_unlock();
 	trace_suspend_resume(TPS("dpm_prepare"), state.event, false);
 	return error;
 }
diff --git a/drivers/base/power/power.h b/drivers/base/power/power.h
index c511def..4578d97 100644
--- a/drivers/base/power/power.h
+++ b/drivers/base/power/power.h
@@ -1,15 +1,31 @@
 /* SPDX-License-Identifier: GPL-2.0 */
 #include <linux/pm_qos.h>
 
+extern struct mutex dpm_list_mtx;
+
+static inline void device_pm_lock(void)
+{
+	mutex_lock(&dpm_list_mtx);
+}
+
+static inline void device_pm_unlock(void)
+{
+	mutex_unlock(&dpm_list_mtx);
+}
+
 static inline void device_pm_init_common(struct device *dev)
 {
 	if (!dev->power.early_init) {
 		spin_lock_init(&dev->power.lock);
+		INIT_LIST_HEAD(&dev->power.entry);
 		dev->power.qos = NULL;
 		dev->power.early_init = true;
 	}
 }
 
+extern void device_pm_add(struct device *dev);
+extern void device_pm_remove(struct device *dev);
+
 #ifdef CONFIG_PM
 
 static inline void pm_runtime_early_init(struct device *dev)
@@ -104,8 +120,6 @@ static inline struct device *to_device(struct list_head *entry)
 }
 
 extern void device_pm_sleep_init(struct device *dev);
-extern void device_pm_add(struct device *);
-extern void device_pm_remove(struct device *);
 extern void device_pm_move_before(struct device *, struct device *);
 extern void device_pm_move_after(struct device *, struct device *);
 extern void device_pm_move_last(struct device *);
@@ -120,13 +134,6 @@ static inline bool device_pm_initialized(struct device *dev)
 
 static inline void device_pm_sleep_init(struct device *dev) {}
 
-static inline void device_pm_add(struct device *dev) {}
-
-static inline void device_pm_remove(struct device *dev)
-{
-	pm_runtime_remove(dev);
-}
-
 static inline void device_pm_move_before(struct device *deva,
 					 struct device *devb) {}
 static inline void device_pm_move_after(struct device *deva,
diff --git a/drivers/base/power/shutdown.c b/drivers/base/power/shutdown.c
index c405d09..be8e247 100644
--- a/drivers/base/power/shutdown.c
+++ b/drivers/base/power/shutdown.c
@@ -5,6 +5,50 @@
 
 #include "../base.h"
 #include "power.h"
+
+LIST_HEAD(dpm_list);
+DEFINE_MUTEX(dpm_list_mtx);
+
+/**
+ * device_pm_add - Add a device to the PM core's list of active devices.
+ * @dev: Device to add to the list.
+ */
+void device_pm_add(struct device *dev)
+{
+	pr_debug("PM: Adding info for %s:%s\n",
+		 dev->bus ? dev->bus->name : "No Bus", dev_name(dev));
+	device_pm_check_callbacks(dev);
+	device_pm_lock();
+#ifdef CONFIG_PM_SLEEP
+	if (dev->parent && dev->parent->power.is_prepared)
+		dev_warn(dev, "parent %s should not be sleeping\n",
+			dev_name(dev->parent));
+#endif
+	list_add_tail(&dev->power.entry, &dpm_list);
+	dev->power.in_dpm_list = true;
+	device_pm_unlock();
+}
+
+/**
+ * device_pm_remove - Remove a device from the PM core's list of active devices.
+ * @dev: Device to be removed from the list.
+ */
+void device_pm_remove(struct device *dev)
+{
+	pr_debug("PM: Removing info for %s:%s\n",
+		 dev->bus ? dev->bus->name : "No Bus", dev_name(dev));
+#ifdef CONFIG_PM_SLEEP
+	complete_all(&dev->power.completion);
+#endif
+	device_pm_lock();
+	list_del_init(&dev->power.entry);
+	dev->power.in_dpm_list = false;
+	device_pm_unlock();
+	device_wakeup_disable(dev);
+	pm_runtime_remove(dev);
+	device_pm_check_callbacks(dev);
+}
+
 /**
  * device_shutdown - call ->shutdown() on each device to shutdown.
  */
@@ -15,15 +59,18 @@ void device_shutdown(void)
 	wait_for_device_probe();
 	device_block_probing();
 
-	spin_lock(&devices_kset->list_lock);
+	/*
+	 * only care about race with hotplug. And shutdown excludes suspend
+	 */
+	device_pm_lock();
 	/*
 	 * Walk the devices list backward, shutting down each in turn.
 	 * Beware that device unplug events may also start pulling
 	 * devices offline, even as the system is shutting down.
 	 */
-	while (!list_empty(&devices_kset->list)) {
-		dev = list_entry(devices_kset->list.prev, struct device,
-				kobj.entry);
+	while (!list_empty(&dpm_list)) {
+		dev = list_entry(dpm_list.prev, struct device,
+				power.entry);
 
 		/*
 		 * hold reference count of device's parent to
@@ -36,8 +83,8 @@ void device_shutdown(void)
 		 * Make sure the device is off the kset list, in the
 		 * event that dev->*->shutdown() doesn't remove it.
 		 */
-		list_del_init(&dev->kobj.entry);
-		spin_unlock(&devices_kset->list_lock);
+		list_del_init(&dev->power.entry);
+		device_pm_unlock();
 
 		/* hold lock to avoid race with probe/release */
 		if (parent)
@@ -70,7 +117,7 @@ void device_shutdown(void)
 		put_device(dev);
 		put_device(parent);
 
-		spin_lock(&devices_kset->list_lock);
+		device_pm_lock();
 	}
-	spin_unlock(&devices_kset->list_lock);
+	device_pm_unlock();
 }
diff --git a/include/linux/pm.h b/include/linux/pm.h
index e723b78..77d8a50 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -595,8 +595,8 @@ struct dev_pm_info {
 	bool			direct_complete:1;	/* Owned by the PM core */
 	u32			driver_flags;
 	spinlock_t		lock;
-#ifdef CONFIG_PM_SLEEP
 	struct list_head	entry;
+#ifdef CONFIG_PM_SLEEP
 	struct completion	completion;
 	struct wakeup_source	*wakeup;
 	bool			wakeup_path:1;
@@ -721,7 +721,6 @@ struct dev_pm_domain {
  */
 
 #ifdef CONFIG_PM_SLEEP
-extern void device_pm_lock(void);
 extern void dpm_resume_start(pm_message_t state);
 extern void dpm_resume_end(pm_message_t state);
 extern void dpm_noirq_resume_devices(pm_message_t state);
@@ -731,7 +730,6 @@ extern void dpm_resume_early(pm_message_t state);
 extern void dpm_resume(pm_message_t state);
 extern void dpm_complete(pm_message_t state);
 
-extern void device_pm_unlock(void);
 extern int dpm_suspend_end(pm_message_t state);
 extern int dpm_suspend_start(pm_message_t state);
 extern void dpm_noirq_begin(void);
@@ -778,9 +776,6 @@ extern bool dev_pm_smart_suspend_and_suspended(struct device *dev);
 
 #else /* !CONFIG_PM_SLEEP */
 
-#define device_pm_lock() do {} while (0)
-#define device_pm_unlock() do {} while (0)
-
 static inline int dpm_suspend_start(pm_message_t state)
 {
 	return 0;
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [PATCH 3/3] drivers/base: clean up unused devices_kset_move_*() functions
  2018-08-20  9:18 [PATCH 0/3] PM: simplify the code logic on device_shutdown and suspend Pingfan Liu
  2018-08-20  9:18 ` [PATCH 1/3] drivers/base: move device_shutdown() to base/power Pingfan Liu
  2018-08-20  9:18 ` [PATCH 2/3] PM/shutdown: device_shutdown() uses the order info in dpm_list instead of devices_kset Pingfan Liu
@ 2018-08-20  9:18 ` Pingfan Liu
  2 siblings, 0 replies; 6+ messages in thread
From: Pingfan Liu @ 2018-08-20  9:18 UTC (permalink / raw)
  To: linux-pm; +Cc: Pingfan Liu, Greg Kroah-Hartman, Rafael J. Wysocki, linux-kernel

Now, shutdown seq is unified with PM, and there is no need to maintain the
"parent <- child" or "supplier <- consumer" order in devices_kset any more,
hence removing the corresponding routines

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
---
 drivers/base/base.h |  1 -
 drivers/base/core.c | 56 -----------------------------------------------------
 2 files changed, 57 deletions(-)

diff --git a/drivers/base/base.h b/drivers/base/base.h
index 7a419a7..fcb213b 100644
--- a/drivers/base/base.h
+++ b/drivers/base/base.h
@@ -133,7 +133,6 @@ extern void device_unblock_probing(void);
 
 /* /sys/devices directory */
 extern struct kset *devices_kset;
-extern void devices_kset_move_last(struct device *dev);
 
 #if defined(CONFIG_MODULES) && defined(CONFIG_SYSFS)
 extern void module_add_driver(struct module *mod, struct device_driver *drv);
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 7c83384..99fdc57 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -127,13 +127,6 @@ static int device_reorder_to_tail(struct device *dev, void *not_used)
 {
 	struct device_link *link;
 
-	/*
-	 * Devices that have not been registered yet will be put to the ends
-	 * of the lists during the registration, so skip them here.
-	 */
-	if (device_is_registered(dev))
-		devices_kset_move_last(dev);
-
 	if (device_pm_initialized(dev))
 		device_pm_move_last(dev);
 
@@ -1316,52 +1309,6 @@ static DEVICE_ATTR_RO(dev);
 struct kset *devices_kset;
 
 /**
- * devices_kset_move_before - Move device in the devices_kset's list.
- * @deva: Device to move.
- * @devb: Device @deva should come before.
- */
-static void devices_kset_move_before(struct device *deva, struct device *devb)
-{
-	if (!devices_kset)
-		return;
-	pr_debug("devices_kset: Moving %s before %s\n",
-		 dev_name(deva), dev_name(devb));
-	spin_lock(&devices_kset->list_lock);
-	list_move_tail(&deva->kobj.entry, &devb->kobj.entry);
-	spin_unlock(&devices_kset->list_lock);
-}
-
-/**
- * devices_kset_move_after - Move device in the devices_kset's list.
- * @deva: Device to move
- * @devb: Device @deva should come after.
- */
-static void devices_kset_move_after(struct device *deva, struct device *devb)
-{
-	if (!devices_kset)
-		return;
-	pr_debug("devices_kset: Moving %s after %s\n",
-		 dev_name(deva), dev_name(devb));
-	spin_lock(&devices_kset->list_lock);
-	list_move(&deva->kobj.entry, &devb->kobj.entry);
-	spin_unlock(&devices_kset->list_lock);
-}
-
-/**
- * devices_kset_move_last - move the device to the end of devices_kset's list.
- * @dev: device to move
- */
-void devices_kset_move_last(struct device *dev)
-{
-	if (!devices_kset)
-		return;
-	pr_debug("devices_kset: Moving %s to end of list\n", dev_name(dev));
-	spin_lock(&devices_kset->list_lock);
-	list_move_tail(&dev->kobj.entry, &devices_kset->list);
-	spin_unlock(&devices_kset->list_lock);
-}
-
-/**
  * device_create_file - create sysfs attribute file for device.
  * @dev: device.
  * @attr: device attribute descriptor.
@@ -2834,15 +2781,12 @@ int device_move(struct device *dev, struct device *new_parent,
 		break;
 	case DPM_ORDER_DEV_AFTER_PARENT:
 		device_pm_move_after(dev, new_parent);
-		devices_kset_move_after(dev, new_parent);
 		break;
 	case DPM_ORDER_PARENT_BEFORE_DEV:
 		device_pm_move_before(new_parent, dev);
-		devices_kset_move_before(new_parent, dev);
 		break;
 	case DPM_ORDER_DEV_LAST:
 		device_pm_move_last(dev);
-		devices_kset_move_last(dev);
 		break;
 	}
 
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH 1/3] drivers/base: move device_shutdown() to base/power
  2018-08-20  9:18 ` [PATCH 1/3] drivers/base: move device_shutdown() to base/power Pingfan Liu
@ 2018-09-14  9:07   ` Rafael J. Wysocki
  2018-09-19  3:56     ` Pingfan Liu
  0 siblings, 1 reply; 6+ messages in thread
From: Rafael J. Wysocki @ 2018-09-14  9:07 UTC (permalink / raw)
  To: Pingfan Liu; +Cc: linux-pm, Greg Kroah-Hartman, Rafael J. Wysocki, linux-kernel

On Monday, August 20, 2018 11:18:35 AM CEST Pingfan Liu wrote:
> Consider the shutdown as a system state transition, i.e. something like
> suspend, hibernate, hence move it under the base/power. (This is a first
> step to unify the duplicate code logic on devices_kset and dpm_list.)

I don't really think that device_shutodwn() belongs in base/power/.

Moving it to a separate file sounds like a good idea, but let that file
reside in base/ proper.

Then, I would separate the dpm_list definition, locking etc out of
base/power/, move it to the new file containing device_shutdown() and
make the code in base/power/ refer to it (you may want to move the list
entry list head from dev_pm_info to struct device directly while at that
too).

Then, it should be straightforward enough to switch device_shutdown() over to
using it.

Thanks,
Rafael


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH 1/3] drivers/base: move device_shutdown() to base/power
  2018-09-14  9:07   ` Rafael J. Wysocki
@ 2018-09-19  3:56     ` Pingfan Liu
  0 siblings, 0 replies; 6+ messages in thread
From: Pingfan Liu @ 2018-09-19  3:56 UTC (permalink / raw)
  To: Rafael J . Wysocki
  Cc: linux-pm, Greg Kroah-Hartman, Rafael Wysocki, linux-kernel

On Fri, Sep 14, 2018 at 5:10 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> On Monday, August 20, 2018 11:18:35 AM CEST Pingfan Liu wrote:
> > Consider the shutdown as a system state transition, i.e. something like
> > suspend, hibernate, hence move it under the base/power. (This is a first
> > step to unify the duplicate code logic on devices_kset and dpm_list.)
>
> I don't really think that device_shutodwn() belongs in base/power/.
>
> Moving it to a separate file sounds like a good idea, but let that file
> reside in base/ proper.
>
> Then, I would separate the dpm_list definition, locking etc out of
> base/power/, move it to the new file containing device_shutdown() and
> make the code in base/power/ refer to it (you may want to move the list
> entry list head from dev_pm_info to struct device directly while at that
> too).
>
> Then, it should be straightforward enough to switch device_shutdown() over to
> using it.
>
Thanks for your advice. I will find a time slot to redo it.

Regards,
Pingfan

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-09-19  3:57 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-20  9:18 [PATCH 0/3] PM: simplify the code logic on device_shutdown and suspend Pingfan Liu
2018-08-20  9:18 ` [PATCH 1/3] drivers/base: move device_shutdown() to base/power Pingfan Liu
2018-09-14  9:07   ` Rafael J. Wysocki
2018-09-19  3:56     ` Pingfan Liu
2018-08-20  9:18 ` [PATCH 2/3] PM/shutdown: device_shutdown() uses the order info in dpm_list instead of devices_kset Pingfan Liu
2018-08-20  9:18 ` [PATCH 3/3] drivers/base: clean up unused devices_kset_move_*() functions Pingfan Liu

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