Skip to contentSkip to conversation

PM / core: fix deferred probe breaking suspend resume order

PATCH New Feng Kan 3 participants 4 messages Apr 10, 2018 – Apr 30, 2018

Subsystems (2)
lore
Share & tools
lore

Feng Kan

author
#0 lore raw

PATCH v4 1/1 PM / core: fix deferred probe breaking suspend resume order

When bridge and its endpoint is enumerated the devices are added to the dpm list. Afterward, the bridge defers probe when IOMMU is not ready. This causes the bridge to be moved to the end of the dpm list when deferred probe kicks in. The order of the dpm list for bridge and endpoint is reversed.

Add reordering code to move the bridge and its children and consumers to the end of the pm list so the order for suspend and resume is not altered. The code also move device and its children and consumers to the tail of device_kset list if it is registered.

---
V4:
1. additional comment change from Rafael
V3:
1. additional code comment changes
V2:
1. change patch title from "move device and its children..."
2. move define based on Bjorn's comment
3. rename function name and comment content

 drivers/base/base.h |  3 +++
 drivers/base/core.c | 20 ++++++++++++++++++++
 drivers/base/dd.c   |  4 +---
 3 files changed, 24 insertions(+), 3 deletions(-)
drivers/base/base.h +3 -0
index d800de6..a75c302 100644
@@ -161,3 +161,6 @@ static inline void module_remove_driver(struct device_driver *drv) { }
 extern void device_links_no_driver(struct device *dev);
 extern bool device_links_busy(struct device *dev);
 extern void device_links_unbind_consumers(struct device *dev);
+
+/* device pm support */
+void device_pm_move_to_tail(struct device *dev);
drivers/base/core.c +20 -0
index 110230d..eaa5d9f 100644
@@ -148,6 +148,26 @@ static int device_reorder_to_tail(struct device *dev, void *not_used)
 }
 
 /**
+ * device_pm_move_to_tail - Move set of devices to the end of device lists
+ * @dev: Device to move
+ *
+ * This is a device_reorder_to_tail() wrapper taking the requisite locks.
+ *
+ * It moves the @dev along with all of its children and all of its consumers
+ * to the ends of the device_kset and dpm_list, recursively.
+ */
+void device_pm_move_to_tail(struct device *dev)
+{
+	int idx;
+
+	idx = device_links_read_lock();
+	device_pm_lock();
+	device_reorder_to_tail(dev, NULL);
+	device_pm_unlock();
+	device_links_read_unlock(idx);
+}
+
+/**
  * device_link_add - Create a link between two devices.
  * @consumer: Consumer end of the link.
  * @supplier: Supplier end of the link.
drivers/base/dd.c +1 -3
index 2c964f5..96fab29 100644
@@ -123,9 +123,7 @@ static void deferred_probe_work_func(struct work_struct *work)
 		 * the list is a good order for suspend but deferred
 		 * probe makes that very unsafe.
 		 */
-		device_pm_lock();
-		device_pm_move_last(dev);
-		device_pm_unlock();
+		device_pm_move_to_tail(dev);
 
 		dev_dbg(dev, "Retrying from deferred list\n");
 		if (initcall_debug && !initcalls_done)
Signature · 1 line
1.8.3.1
#1 lore raw

PATCH v4 1/1 PM / core: fix deferred probe breaking suspend resume order

Quoted 6 lines · CEST Feng Kan

When bridge and its endpoint is enumerated the devices are added to the dpm list. Afterward, the bridge defers probe when IOMMU is not ready. This causes the bridge to be moved to the end of the dpm list when deferred probe kicks in. The order of the dpm list for bridge and endpoint is reversed.

Quoted 7 lines

Add reordering code to move the bridge and its children and consumers to the end of the pm list so the order for suspend and resume is not altered. The code also move device and its children and consumers to the tail of device_kset list if it is registered.

I'm going to queue this up for 4.18-rc1.

If anyone, and Greg in particular, has any objections of concerns regarding that, please let me know.

Quoted 73 lines

---
V4:
1. additional comment change from Rafael
V3:
1. additional code comment changes
V2:
1. change patch title from "move device and its children..."
2. move define based on Bjorn's comment
3. rename function name and comment content

 drivers/base/base.h |  3 +++
 drivers/base/core.c | 20 ++++++++++++++++++++
 drivers/base/dd.c   |  4 +---
 3 files changed, 24 insertions(+), 3 deletions(-)
drivers/base/base.h +3 -0
index d800de6..a75c302 100644
@@ -161,3 +161,6 @@ static inline void module_remove_driver(struct device_driver *drv) { }
 extern void device_links_no_driver(struct device *dev);
 extern bool device_links_busy(struct device *dev);
 extern void device_links_unbind_consumers(struct device *dev);
+
+/* device pm support */
+void device_pm_move_to_tail(struct device *dev);
drivers/base/core.c +20 -0
index 110230d..eaa5d9f 100644
@@ -148,6 +148,26 @@ static int device_reorder_to_tail(struct device *dev, void *not_used)
 }
 
 /**
+ * device_pm_move_to_tail - Move set of devices to the end of device lists
+ * @dev: Device to move
+ *
+ * This is a device_reorder_to_tail() wrapper taking the requisite locks.
+ *
+ * It moves the @dev along with all of its children and all of its consumers
+ * to the ends of the device_kset and dpm_list, recursively.
+ */
+void device_pm_move_to_tail(struct device *dev)
+{
+	int idx;
+
+	idx = device_links_read_lock();
+	device_pm_lock();
+	device_reorder_to_tail(dev, NULL);
+	device_pm_unlock();
+	device_links_read_unlock(idx);
+}
+
+/**
  * device_link_add - Create a link between two devices.
  * @consumer: Consumer end of the link.
  * @supplier: Supplier end of the link.
drivers/base/dd.c +1 -3
index 2c964f5..96fab29 100644
@@ -123,9 +123,7 @@ static void deferred_probe_work_func(struct work_struct *work)
 		 * the list is a good order for suspend but deferred
 		 * probe makes that very unsafe.
 		 */
-		device_pm_lock();
-		device_pm_move_last(dev);
-		device_pm_unlock();
+		device_pm_move_to_tail(dev);
 
 		dev_dbg(dev, "Retrying from deferred list\n");
 		if (initcall_debug && !initcalls_done)
maintainer
#2 lore raw

PATCH v4 1/1 PM / core: fix deferred probe breaking suspend resume order

Quoted 12 lines · Rafael J. Wysocki
Quoted 11 lines · CEST Feng Kan

When bridge and its endpoint is enumerated the devices are added to the dpm list. Afterward, the bridge defers probe when IOMMU is not ready. This causes the bridge to be moved to the end of the dpm list when deferred probe kicks in. The order of the dpm list for bridge and endpoint is reversed.

Add reordering code to move the bridge and its children and consumers to the end of the pm list so the order for suspend and resume is not altered. The code also move device and its children and consumers to the tail of device_kset list if it is registered.

Quoted 7 lines
Quoted 2 lines

I'm going to queue this up for 4.18-rc1.

If anyone, and Greg in particular, has any objections of concerns regarding that, please let me know.

No objection from me:

#3 lore raw

PATCH v4 1/1 PM / core: fix deferred probe breaking suspend resume order

In reply to Feng Kan
Quoted 76 lines · CEST Feng Kan

When bridge and its endpoint is enumerated the devices are added to the dpm list. Afterward, the bridge defers probe when IOMMU is not ready. This causes the bridge to be moved to the end of the dpm list when deferred probe kicks in. The order of the dpm list for bridge and endpoint is reversed.

Add reordering code to move the bridge and its children and consumers to the end of the pm list so the order for suspend and resume is not altered. The code also move device and its children and consumers to the tail of device_kset list if it is registered.

---
V4:
1. additional comment change from Rafael
V3:
1. additional code comment changes
V2:
1. change patch title from "move device and its children..."
2. move define based on Bjorn's comment
3. rename function name and comment content

 drivers/base/base.h |  3 +++
 drivers/base/core.c | 20 ++++++++++++++++++++
 drivers/base/dd.c   |  4 +---
 3 files changed, 24 insertions(+), 3 deletions(-)
drivers/base/base.h +3 -0
index d800de6..a75c302 100644
@@ -161,3 +161,6 @@ static inline void module_remove_driver(struct device_driver *drv) { }
 extern void device_links_no_driver(struct device *dev);
 extern bool device_links_busy(struct device *dev);
 extern void device_links_unbind_consumers(struct device *dev);
+
+/* device pm support */
+void device_pm_move_to_tail(struct device *dev);
drivers/base/core.c +20 -0
index 110230d..eaa5d9f 100644
@@ -148,6 +148,26 @@ static int device_reorder_to_tail(struct device *dev, void *not_used)
 }
 
 /**
+ * device_pm_move_to_tail - Move set of devices to the end of device lists
+ * @dev: Device to move
+ *
+ * This is a device_reorder_to_tail() wrapper taking the requisite locks.
+ *
+ * It moves the @dev along with all of its children and all of its consumers
+ * to the ends of the device_kset and dpm_list, recursively.
+ */
+void device_pm_move_to_tail(struct device *dev)
+{
+	int idx;
+
+	idx = device_links_read_lock();
+	device_pm_lock();
+	device_reorder_to_tail(dev, NULL);
+	device_pm_unlock();
+	device_links_read_unlock(idx);
+}
+
+/**
  * device_link_add - Create a link between two devices.
  * @consumer: Consumer end of the link.
  * @supplier: Supplier end of the link.
drivers/base/dd.c +0 -0
index 2c964f5..96fab29 100644

drivers/base/dd.c static void deferred_probe_work_func(struct work_struct *work) · line 129

Quoted 10 lines
drivers/base/dd.c +1 -3
@@ -123,9 +123,7 @@ static void deferred_probe_work_func(struct work_struct *work)
 		 * the list is a good order for suspend but deferred
 		 * probe makes that very unsafe.
 		 */
-		device_pm_lock();
-		device_pm_move_last(dev);
-		device_pm_unlock();
+		device_pm_move_to_tail(dev);
 
 		dev_dbg(dev, "Retrying from deferred list\n");
 		if (initcall_debug && !initcalls_done)

Queued up for 4.18 with the Greg's ACK.