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
===================================================================
--- 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)
{
On Fri, May 14, 2021 at 5:12 AM Rafael J. Wysocki <[email protected]> wrote:
>
> 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
> ===================================================================
> --- 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)
> {
>
>
>
On Fri, May 14, 2021 at 6:05 PM Saravana Kannan <[email protected]> wrote:
>
> On Fri, May 14, 2021 at 5:12 AM Rafael J. Wysocki <[email protected]> wrote:
> >
> > 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
> > ===================================================================
> > --- 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.
On Fri, May 14, 2021 at 11:33 AM Rafael J. Wysocki <[email protected]> wrote:
>
> On Fri, May 14, 2021 at 6:05 PM Saravana Kannan <[email protected]> wrote:
> >
> > On Fri, May 14, 2021 at 5:12 AM Rafael J. Wysocki <[email protected]> wrote:
> > >
> > > 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
> > > ===================================================================
> > > --- 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 <[email protected]>
-Saravana