drivers: base: Device links removal fix and cleanup

PATCH In review Rafael J. Wysocki 4 participants 8 messages May 14, 2021 Driver core

lore

Share & tools

lore

Rafael J. Wysocki

author

2021-05-14 12:12 UTC #0 lore raw

cover drivers: base: Device links removal fix and cleanup

Hi Greg,

Patch [1/2] fixes a device link removal issue that may trigger a "scheduling while atomic" error in some situations and patch [2/2] is a related cleanup on top of it.

Cheers,
Rafael

Rafael J. Wysocki

author

2021-05-14 12:12 UTC #1 lore raw

PATCH 1/2 drivers: base: Fix device link removal

From: Rafael J. Wysocki <[email protected]>

When device_link_free() drops references to the supplier and consumer devices of the device link going away and the reference being dropped turns out to be the last one for any of those device objects, its ->release callback will be invoked and it may sleep which goes against the SRCU callback execution requirements.

To address this issue, make the device link removal code carry out the device_link_free() actions preceded by SRCU synchronization from a separate work item (the "long" workqueue is used for that, because it does not matter when the device link memory is released and it may take time to get to that point) instead of using SRCU callbacks.

While at it, make the code work analogously when SRCU is not enabled to reduce the differences between the SRCU and non-SRCU cases.

Fixes 843e600b8a2b ("driver core: Fix sleeping in invalid context during device link deletion")

Reported-by chenxiang (M) <[email protected]>

Tested-by chenxiang (M) <[email protected]>

Signed-off-by Rafael J. Wysocki <[email protected]>

Reviewed-by Saravana Kannan <[email protected]>

---
 drivers/base/core.c    |   37 +++++++++++++++++++++++--------------
 include/linux/device.h |    6 ++----
 2 files changed, 25 insertions(+), 18 deletions(-)

drivers/base/core.c +23 ~~-14~~

Index: linux-pm/drivers/base/core.c
===================================================================
@@ -193,6 +193,11 @@ int device_links_read_lock_held(void)
 {
 	return srcu_read_lock_held(&device_links_srcu);
 }
+
+static void device_link_synchronize_removal(void)
+{
+	synchronize_srcu(&device_links_srcu);
+}
 #else /* !CONFIG_SRCU */
 static DECLARE_RWSEM(device_links_lock);

@@ -223,6 +228,10 @@ int device_links_read_lock_held(void)
 	return lockdep_is_held(&device_links_lock);
 }
 #endif
+
+static inline void device_link_synchronize_removal(void)
+{
+}
 #endif /* !CONFIG_SRCU */
 
 static bool device_is_ancestor(struct device *dev, struct device *target)

@@ -444,8 +453,13 @@ static struct attribute *devlink_attrs[]
 };
 ATTRIBUTE_GROUPS(devlink);
 
-static void device_link_free(struct device_link *link)
+static void device_link_release_fn(struct work_struct *work)
 {
+	struct device_link *link = container_of(work, struct device_link, rm_work);
+
+	/* Ensure that all references to the link object have been dropped. */
+	device_link_synchronize_removal();
+
 	while (refcount_dec_not_one(&link->rpm_active))
 		pm_runtime_put(link->supplier);

@@ -454,24 +468,19 @@ static void device_link_free(struct devi
 	kfree(link);
 }
 
-#ifdef CONFIG_SRCU
-static void __device_link_free_srcu(struct rcu_head *rhead)
-{
-	device_link_free(container_of(rhead, struct device_link, rcu_head));
-}
-
 static void devlink_dev_release(struct device *dev)
 {
 	struct device_link *link = to_devlink(dev);
 
-	call_srcu(&device_links_srcu, &link->rcu_head, __device_link_free_srcu);
-}
-#else
-static void devlink_dev_release(struct device *dev)
-{
-	device_link_free(to_devlink(dev));
+	INIT_WORK(&link->rm_work, device_link_release_fn);
+	/*
+	 * It may take a while to complete this work because of the SRCU
+	 * synchronization in device_link_release_fn() and if the consumer or
+	 * supplier devices get deleted when it runs, so put it into the "long"
+	 * workqueue.
+	 */
+	queue_work(system_long_wq, &link->rm_work);
 }
-#endif
 
 static struct class devlink_class = {
 	.name = "devlink",

include/linux/device.h +2 -4

Index: linux-pm/include/linux/device.h
===================================================================
@@ -570,7 +570,7 @@ struct device {
  * @flags: Link flags.
  * @rpm_active: Whether or not the consumer device is runtime-PM-active.
  * @kref: Count repeated addition of the same link.
- * @rcu_head: An RCU head to use for deferred execution of SRCU callbacks.
+ * @rm_work: Work structure used for removing the link.
  * @supplier_preactivated: Supplier has been made active before consumer probe.
  */
 struct device_link {

@@ -583,9 +583,7 @@ struct device_link {
 	u32 flags;
 	refcount_t rpm_active;
 	struct kref kref;
-#ifdef CONFIG_SRCU
-	struct rcu_head rcu_head;
-#endif
+	struct work_struct rm_work;
 	bool supplier_preactivated; /* Owned by consumer probe. */
 };

Rafael J. Wysocki

author

2021-05-14 12:12 UTC #2 lore raw

PATCH 2/2 drivers: base: Reduce device link removal code duplication

In reply to Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

Reduce device link removal code duplication between the cases when SRCU is enabled and when it is disabled by moving the only differing piece of it (which is the removal of the link from the consumer and supplier lists) into a separate wrapper function (defined differently for each of the cases in question).

No intentional functional impact.

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/base/core.c |   31 +++++++++++++------------------
 1 file changed, 13 insertions(+), 18 deletions(-)

drivers/base/core.c +13 ~~-18~~

Index: linux-pm/drivers/base/core.c
===================================================================
@@ -198,6 +198,12 @@ static void device_link_synchronize_remo
 {
 	synchronize_srcu(&device_links_srcu);
 }
+
+static void device_link_remove_from_lists(struct device_link *link)
+{
+	list_del_rcu(&link->s_node);
+	list_del_rcu(&link->c_node);
+}
 #else /* !CONFIG_SRCU */
 static DECLARE_RWSEM(device_links_lock);

@@ -232,6 +238,12 @@ int device_links_read_lock_held(void)
 static inline void device_link_synchronize_removal(void)
 {
 }
+
+static void device_link_remove_from_lists(struct device_link *link)
+{
+	list_del(&link->s_node);
+	list_del(&link->c_node);
+}
 #endif /* !CONFIG_SRCU */
 
 static bool device_is_ancestor(struct device *dev, struct device *target)

@@ -854,7 +866,6 @@ out:
 }
 EXPORT_SYMBOL_GPL(device_link_add);
 
-#ifdef CONFIG_SRCU
 static void __device_link_del(struct kref *kref)
 {
 	struct device_link *link = container_of(kref, struct device_link, kref);

@@ -864,25 +875,9 @@ static void __device_link_del(struct kre
 
 	pm_runtime_drop_link(link);
 
-	list_del_rcu(&link->s_node);
-	list_del_rcu(&link->c_node);
+	device_link_remove_from_lists(link);
 	device_unregister(&link->link_dev);
 }
-#else /* !CONFIG_SRCU */
-static void __device_link_del(struct kref *kref)
-{
-	struct device_link *link = container_of(kref, struct device_link, kref);
-
-	dev_info(link->consumer, "Dropping the link to %s\n",
-		 dev_name(link->supplier));
-
-	pm_runtime_drop_link(link);
-
-	list_del(&link->s_node);
-	list_del(&link->c_node);
-	device_unregister(&link->link_dev);
-}
-#endif /* !CONFIG_SRCU */
 
 static void device_link_put_kref(struct device_link *link)
 {

Saravana Kannan

2021-05-14 16:05 UTC #3 lore raw

Re: [PATCH v1 2/2] drivers: base: Reduce device link removal code duplication

Quoted 55 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

No intentional functional impact.

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/base/core.c |   31 +++++++++++++------------------
 1 file changed, 13 insertions(+), 18 deletions(-)

Index: linux-pm/drivers/base/core.c
===================================================================

drivers/base/core.c +12 -1

@@ -198,6 +198,12 @@ static void device_link_synchronize_remo
 {
        synchronize_srcu(&device_links_srcu);
 }
+
+static void device_link_remove_from_lists(struct device_link *link)
+{
+       list_del_rcu(&link->s_node);
+       list_del_rcu(&link->c_node);
+}
 #else /* !CONFIG_SRCU */
 static DECLARE_RWSEM(device_links_lock);

@@ -232,6 +238,12 @@ int device_links_read_lock_held(void)
 static inline void device_link_synchronize_removal(void)
 {
 }
+
+static void device_link_remove_from_lists(struct device_link *link)
+{
+       list_del(&link->s_node);
+       list_del(&link->c_node);
+}
 #endif /* !CONFIG_SRCU */
 
 static bool device_is_ancestor(struct device *dev, struct device *target)

@@ -854,7 +866,6 @@ out:
 }
 EXPORT_SYMBOL_GPL(device_link_add);
 
-#ifdef CONFIG_SRCU
 static void __device_link_del(struct kref *kref)
 {
        struct device_link *link = container_of(kref, struct device_link, kref);

drivers/base/core.c static void __device_link_del(struct kre · line 878

Quoted 6 lines

drivers/base/core.c +1 -2

@@ -864,25 +875,9 @@ static void __device_link_del(struct kre
 
        pm_runtime_drop_link(link);
 
-       list_del_rcu(&link->s_node);
-       list_del_rcu(&link->c_node);
+       device_link_remove_from_lists(link);

Remind me again why we can't do the synchronize_srcu() here (I'm not too familiar with the SRCU API semantics)? Is it because synchronize_srcu() can take indefinitely long? I just vaguely remember it does some checks during CPUs going idle (which can be a long time later) but I'm not sure if that's the earliest you can synchronize. If it's not indefinitely long and we just need to wait for other SRCU critical sections to exit, maybe we can just synchronize here and make the code a lot simpler?

This function is anyway called in a sleepable context.

-Saravana

Quoted 23 lines

drivers/base/core.c +0 ~~-15~~

        device_unregister(&link->link_dev);
 }
-#else /* !CONFIG_SRCU */
-static void __device_link_del(struct kref *kref)
-{
-       struct device_link *link = container_of(kref, struct device_link, kref);
-
-       dev_info(link->consumer, "Dropping the link to %s\n",
-                dev_name(link->supplier));
-
-       pm_runtime_drop_link(link);
-
-       list_del(&link->s_node);
-       list_del(&link->c_node);
-       device_unregister(&link->link_dev);
-}
-#endif /* !CONFIG_SRCU */
 
 static void device_link_put_kref(struct device_link *link)
 {

Rafael J. Wysocki

maintainer

2021-05-14 18:33 UTC #4 lore raw

Re: [PATCH v1 2/2] drivers: base: Reduce device link removal code duplication

Quoted 60 lines · Saravana Kannan

Quoted 58 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

No intentional functional impact.

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/base/core.c |   31 +++++++++++++------------------
 1 file changed, 13 insertions(+), 18 deletions(-)

Index: linux-pm/drivers/base/core.c
===================================================================

drivers/base/core.c +12 -1

@@ -198,6 +198,12 @@ static void device_link_synchronize_remo
 {
        synchronize_srcu(&device_links_srcu);
 }
+
+static void device_link_remove_from_lists(struct device_link *link)
+{
+       list_del_rcu(&link->s_node);
+       list_del_rcu(&link->c_node);
+}
 #else /* !CONFIG_SRCU */
 static DECLARE_RWSEM(device_links_lock);

@@ -232,6 +238,12 @@ int device_links_read_lock_held(void)
 static inline void device_link_synchronize_removal(void)
 {
 }
+
+static void device_link_remove_from_lists(struct device_link *link)
+{
+       list_del(&link->s_node);
+       list_del(&link->c_node);
+}
 #endif /* !CONFIG_SRCU */
 
 static bool device_is_ancestor(struct device *dev, struct device *target)

@@ -854,7 +866,6 @@ out:
 }
 EXPORT_SYMBOL_GPL(device_link_add);
 
-#ifdef CONFIG_SRCU
 static void __device_link_del(struct kref *kref)
 {
        struct device_link *link = container_of(kref, struct device_link, kref);

@@ -864,25 +875,9 @@ static void __device_link_del(struct kre
 
        pm_runtime_drop_link(link);

Quoted 7 lines

drivers/base/core.c static void __device_link_del(struct kre · line 878
Quoted 3 lines
drivers/base/core.c +1 -2
-       list_del_rcu(&link->s_node);
-       list_del_rcu(&link->c_node);
+       device_link_remove_from_lists(link);
Remind me again why we can't do the synchronize_srcu() here (I'm not too familiar with the SRCU API semantics)? Is it because synchronize_srcu() can take indefinitely long?

Not indefinitely, but it may take time. And because it is not actually useful before we end up freeing the device link memory. And I'd rather not do it under the device links write lock.

Quoted 6 lines

I just vaguely remember
it does some checks during CPUs going idle (which can be a long time later) but I'm not sure if that's the earliest you can synchronize. If it's not indefinitely long and we just need to wait for other SRCU critical sections to exit, maybe we can just synchronize here and make the code a lot simpler?

Well, maybe not "a lot".

Quoted 1 line

This function is anyway called in a sleepable context.

But I'm not sure how long this context expects to be sleeping and sleeping under a mutex potentially blocks others.

Saravana Kannan

Reviewed-by

2021-05-14 18:39 UTC #5 lore raw

Re: [PATCH v1 2/2] drivers: base: Reduce device link removal code duplication

Quoted 70 lines · Rafael J. Wysocki

Quoted 60 lines · Saravana Kannan
Quoted 58 lines · Rafael J. Wysocki
From: Rafael J. Wysocki <[email protected]>
Reduce device link removal code duplication between the cases when SRCU is enabled and when it is disabled by moving the only differing piece of it (which is the removal of the link from the consumer and supplier lists) into a separate wrapper function (defined differently for each of the cases in question).
No intentional functional impact.
Signed-off-by Rafael J. Wysocki <[email protected]>
---
 drivers/base/core.c |   31 +++++++++++++------------------
 1 file changed, 13 insertions(+), 18 deletions(-)
Index: linux-pm/drivers/base/core.c
===================================================================
drivers/base/core.c +12 -1
@@ -198,6 +198,12 @@ static void device_link_synchronize_remo
 {
        synchronize_srcu(&device_links_srcu);
 }
+
+static void device_link_remove_from_lists(struct device_link *link)
+{
+       list_del_rcu(&link->s_node);
+       list_del_rcu(&link->c_node);
+}
 #else /* !CONFIG_SRCU */
 static DECLARE_RWSEM(device_links_lock);
 
@@ -232,6 +238,12 @@ int device_links_read_lock_held(void)
 static inline void device_link_synchronize_removal(void)
 {
 }
+
+static void device_link_remove_from_lists(struct device_link *link)
+{
+       list_del(&link->s_node);
+       list_del(&link->c_node);
+}
 #endif /* !CONFIG_SRCU */
 
 static bool device_is_ancestor(struct device *dev, struct device *target)
@@ -854,7 +866,6 @@ out:
 }
 EXPORT_SYMBOL_GPL(device_link_add);
 
-#ifdef CONFIG_SRCU
 static void __device_link_del(struct kref *kref)
 {
        struct device_link *link = container_of(kref, struct device_link, kref);
@@ -864,25 +875,9 @@ static void __device_link_del(struct kre
 
        pm_runtime_drop_link(link);
 
Quoted 7 lines
drivers/base/core.c static void __device_link_del(struct kre · line 878
Quoted 3 lines
drivers/base/core.c +1 -2
-       list_del_rcu(&link->s_node);
-       list_del_rcu(&link->c_node);
+       device_link_remove_from_lists(link);
Remind me again why we can't do the synchronize_srcu() here (I'm not too familiar with the SRCU API semantics)? Is it because synchronize_srcu() can take indefinitely long?

Quoted 1 line

Not indefinitely, but it may take time.

More than if we had used normal mutex around these I suppose.

Quoted 11 lines

And because it is not
actually useful before we end up freeing the device link memory. And I'd rather not do it under the device links write lock.
Quoted 6 lines
I just vaguely remember
it does some checks during CPUs going idle (which can be a long time later) but I'm not sure if that's the earliest you can synchronize. If it's not indefinitely long and we just need to wait for other SRCU critical sections to exit, maybe we can just synchronize here and make the code a lot simpler?

Quoted 6 lines

Well, maybe not "a lot".
Quoted 1 line
This function is anyway called in a sleepable context.
But I'm not sure how long this context expects to be sleeping and sleeping under a mutex potentially blocks others.

Ack.

Reviewed-by Saravana Kannan <[email protected]>

-Saravana

Greg Kroah-Hartman

maintainer

2021-05-14 12:34 UTC #6 lore raw

In reply to Rafael J. Wysocki

Quoted 5 lines · Rafael J. Wysocki

Hi Greg,
Patch [1/2] fixes a device link removal issue that may trigger a "scheduling while atomic" error in some situations and patch [2/2] is a related cleanup on top of it.

Thanks for these, I'll take them on Monday after -rc2 is out.

greg k-h

Rafael J. Wysocki

maintainer

2021-05-14 12:57 UTC #7 lore raw

Quoted 9 lines · Greg Kroah-Hartman

Quoted 5 lines · Rafael J. Wysocki
Hi Greg,
Patch [1/2] fixes a device link removal issue that may trigger a "scheduling while atomic" error in some situations and patch [2/2] is a related cleanup on top of it.
Thanks for these, I'll take them on Monday after -rc2 is out.

Thank you!