[PATCH v1 0/2] drivers: base: Device links removal fix and cleanup

[PATCH v1 0/2] drivers: base: Device links removal fix and cleanup

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

Cheers,
Rafael

[PATCH v1 1/2] drivers: base: Fix device link removal

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

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) <chenxiang66@hisilicon.com>
Tested-by: chenxiang (M) <chenxiang66@hisilicon.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Reviewed-by: Saravana Kannan <saravanak@google.com>
---
 drivers/base/core.c    |   37 +++++++++++++++++++++++--------------
 include/linux/device.h |    6 ++----
 2 files changed, 25 insertions(+), 18 deletions(-)

Index: linux-pm/drivers/base/core.c
===================================================================
--- linux-pm.orig/drivers/base/core.c
+++ 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",
Index: linux-pm/include/linux/device.h
===================================================================
--- linux-pm.orig/include/linux/device.h
+++ 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. */
 };
 

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

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

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 <rafael.j.wysocki@intel.com>
---
 drivers/base/core.c |   31 +++++++++++++------------------
 1 file changed, 13 insertions(+), 18 deletions(-)

Index: linux-pm/drivers/base/core.c
===================================================================
--- linux-pm.orig/drivers/base/core.c
+++ 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)
 {

Re: [PATCH v1 0/2] drivers: base: Device links removal fix and cleanup

[Greg Kroah-Hartman]

On Fri, May 14, 2021 at 02:08:16PM +0200, Rafael J. Wysocki wrote:
> 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

Re: [PATCH v1 0/2] drivers: base: Device links removal fix and cleanup

[Rafael J. Wysocki]

On Fri, May 14, 2021 at 2:34 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Fri, May 14, 2021 at 02:08:16PM +0200, Rafael J. Wysocki wrote:
> > 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!

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

[Saravana Kannan]

On Fri, May 14, 2021 at 5:12 AM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> 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 <rafael.j.wysocki@intel.com>
> ---
>  drivers/base/core.c |   31 +++++++++++++------------------
>  1 file changed, 13 insertions(+), 18 deletions(-)
>
> Index: linux-pm/drivers/base/core.c
> ===================================================================
> --- linux-pm.orig/drivers/base/core.c
> +++ 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);

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

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

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

[Rafael J. Wysocki]

On Fri, May 14, 2021 at 6:05 PM Saravana Kannan <saravanak@google.com> wrote:
>
> On Fri, May 14, 2021 at 5:12 AM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > 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 <rafael.j.wysocki@intel.com>
> > ---
> >  drivers/base/core.c |   31 +++++++++++++------------------
> >  1 file changed, 13 insertions(+), 18 deletions(-)
> >
> > Index: linux-pm/drivers/base/core.c
> > ===================================================================
> > --- linux-pm.orig/drivers/base/core.c
> > +++ 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);
>
> 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.

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

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

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

[Saravana Kannan]

On Fri, May 14, 2021 at 11:33 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Fri, May 14, 2021 at 6:05 PM Saravana Kannan <saravanak@google.com> wrote:
> >
> > On Fri, May 14, 2021 at 5:12 AM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > >
> > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > >
> > > 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 <rafael.j.wysocki@intel.com>
> > > ---
> > >  drivers/base/core.c |   31 +++++++++++++------------------
> > >  1 file changed, 13 insertions(+), 18 deletions(-)
> > >
> > > Index: linux-pm/drivers/base/core.c
> > > ===================================================================
> > > --- linux-pm.orig/drivers/base/core.c
> > > +++ 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);
> >
> > 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.

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

>  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.
>
> > 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".
>
> > 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 <saravanak@google.com>

-Saravana