[v2] driver core: Extend device_is_dependent()
diff mbox series

Message ID 17705994.d592GUb2YH@kreacher
State Accepted
Commit 3d1cf435e201d1fd63e4346b141881aed086effd
Headers show
Series
  • [v2] driver core: Extend device_is_dependent()
Related show

Commit Message

Rafael J. Wysocki Jan. 15, 2021, 6:30 p.m. UTC
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

If the device passed as the target (second argument) to
device_is_dependent() is not completely registered (that is, it has
been initialized, but not added yet), but the parent pointer of it
is set, it may be missing from the list of the parent's children
and device_for_each_child() called by device_is_dependent() cannot
be relied on to catch that dependency.

For this reason, modify device_is_dependent() to check the ancestors
of the target device by following its parent pointer in addition to
the device_for_each_child() walk.

Fixes: 9ed9895370ae ("driver core: Functional dependencies tracking support")
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Reported-by: Stephan Gerhold <stephan@gerhold.net>
Tested-by: Stephan Gerhold <stephan@gerhold.net>
---

-> v2:
   * Improve the changelog.
   * Add a comment to explain the reason for the extra check.
   * Add tags.

   No code changes.

---
 drivers/base/core.c |   17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

Comments

Saravana Kannan Jan. 18, 2021, 10:01 p.m. UTC | #1
On Fri, Jan 15, 2021 at 10:30 AM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> If the device passed as the target (second argument) to
> device_is_dependent() is not completely registered (that is, it has
> been initialized, but not added yet), but the parent pointer of it
> is set, it may be missing from the list of the parent's children
> and device_for_each_child() called by device_is_dependent() cannot
> be relied on to catch that dependency.
>
> For this reason, modify device_is_dependent() to check the ancestors
> of the target device by following its parent pointer in addition to
> the device_for_each_child() walk.
>
> Fixes: 9ed9895370ae ("driver core: Functional dependencies tracking support")
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Reported-by: Stephan Gerhold <stephan@gerhold.net>
> Tested-by: Stephan Gerhold <stephan@gerhold.net>
> ---
>
> -> v2:
>    * Improve the changelog.
>    * Add a comment to explain the reason for the extra check.
>    * Add tags.
>
>    No code changes.
>
> ---
>  drivers/base/core.c |   17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
>
> Index: linux-pm/drivers/base/core.c
> ===================================================================
> --- linux-pm.orig/drivers/base/core.c
> +++ linux-pm/drivers/base/core.c
> @@ -208,6 +208,16 @@ int device_links_read_lock_held(void)
>  #endif
>  #endif /* !CONFIG_SRCU */
>
> +static bool device_is_ancestor(struct device *dev, struct device *target)
> +{
> +       while (target->parent) {
> +               target = target->parent;
> +               if (dev == target)
> +                       return true;
> +       }
> +       return false;
> +}
> +
>  /**
>   * device_is_dependent - Check if one device depends on another one
>   * @dev: Device to check dependencies for.
> @@ -221,7 +231,12 @@ int device_is_dependent(struct device *d
>         struct device_link *link;
>         int ret;
>
> -       if (dev == target)
> +       /*
> +        * The "ancestors" check is needed to catch the case when the target
> +        * device has not been completely initialized yet and it is still
> +        * missing from the list of children of its parent device.
> +        */
> +       if (dev == target || device_is_ancestor(dev, target))
>                 return 1;
>
>         ret = device_for_each_child(dev, target, device_is_dependent);
>

Reviewed-by: Saravana Kannan <saravanak@google.com>

fw_devlink_relax_cycle() needs a similar fix. Want me to handle that
as a separate patch to driver-core-next? Or do you want to combine
that in this patch?

-Saravana
Rafael J. Wysocki Jan. 19, 2021, 1:23 p.m. UTC | #2
On Mon, Jan 18, 2021 at 11:03 PM Saravana Kannan <saravanak@google.com> wrote:
>
> On Fri, Jan 15, 2021 at 10:30 AM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > If the device passed as the target (second argument) to
> > device_is_dependent() is not completely registered (that is, it has
> > been initialized, but not added yet), but the parent pointer of it
> > is set, it may be missing from the list of the parent's children
> > and device_for_each_child() called by device_is_dependent() cannot
> > be relied on to catch that dependency.
> >
> > For this reason, modify device_is_dependent() to check the ancestors
> > of the target device by following its parent pointer in addition to
> > the device_for_each_child() walk.
> >
> > Fixes: 9ed9895370ae ("driver core: Functional dependencies tracking support")
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > Reported-by: Stephan Gerhold <stephan@gerhold.net>
> > Tested-by: Stephan Gerhold <stephan@gerhold.net>
> > ---
> >
> > -> v2:
> >    * Improve the changelog.
> >    * Add a comment to explain the reason for the extra check.
> >    * Add tags.
> >
> >    No code changes.
> >
> > ---
> >  drivers/base/core.c |   17 ++++++++++++++++-
> >  1 file changed, 16 insertions(+), 1 deletion(-)
> >
> > Index: linux-pm/drivers/base/core.c
> > ===================================================================
> > --- linux-pm.orig/drivers/base/core.c
> > +++ linux-pm/drivers/base/core.c
> > @@ -208,6 +208,16 @@ int device_links_read_lock_held(void)
> >  #endif
> >  #endif /* !CONFIG_SRCU */
> >
> > +static bool device_is_ancestor(struct device *dev, struct device *target)
> > +{
> > +       while (target->parent) {
> > +               target = target->parent;
> > +               if (dev == target)
> > +                       return true;
> > +       }
> > +       return false;
> > +}
> > +
> >  /**
> >   * device_is_dependent - Check if one device depends on another one
> >   * @dev: Device to check dependencies for.
> > @@ -221,7 +231,12 @@ int device_is_dependent(struct device *d
> >         struct device_link *link;
> >         int ret;
> >
> > -       if (dev == target)
> > +       /*
> > +        * The "ancestors" check is needed to catch the case when the target
> > +        * device has not been completely initialized yet and it is still
> > +        * missing from the list of children of its parent device.
> > +        */
> > +       if (dev == target || device_is_ancestor(dev, target))
> >                 return 1;
> >
> >         ret = device_for_each_child(dev, target, device_is_dependent);
> >
>
> Reviewed-by: Saravana Kannan <saravanak@google.com>
>
> fw_devlink_relax_cycle() needs a similar fix. Want me to handle that
> as a separate patch to driver-core-next? Or do you want to combine
> that in this patch?

I would prefer that to be a different patch because of the Fixes: tag.

Also fw_devlink_relax_cycle() is in linux-next only for now, so it is
better to avoid mixing a mainline fix with updating that one.
Rafael J. Wysocki Jan. 19, 2021, 3:09 p.m. UTC | #3
On Fri, Jan 15, 2021 at 7:31 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> If the device passed as the target (second argument) to
> device_is_dependent() is not completely registered (that is, it has
> been initialized, but not added yet), but the parent pointer of it
> is set, it may be missing from the list of the parent's children
> and device_for_each_child() called by device_is_dependent() cannot
> be relied on to catch that dependency.
>
> For this reason, modify device_is_dependent() to check the ancestors
> of the target device by following its parent pointer in addition to
> the device_for_each_child() walk.
>
> Fixes: 9ed9895370ae ("driver core: Functional dependencies tracking support")
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Reported-by: Stephan Gerhold <stephan@gerhold.net>
> Tested-by: Stephan Gerhold <stephan@gerhold.net>

Greg, are you going to pick up this one or do you want me to take care of it?

It has been reviewed by Saravana.

> ---
>
> -> v2:
>    * Improve the changelog.
>    * Add a comment to explain the reason for the extra check.
>    * Add tags.
>
>    No code changes.
>
> ---
>  drivers/base/core.c |   17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
>
> Index: linux-pm/drivers/base/core.c
> ===================================================================
> --- linux-pm.orig/drivers/base/core.c
> +++ linux-pm/drivers/base/core.c
> @@ -208,6 +208,16 @@ int device_links_read_lock_held(void)
>  #endif
>  #endif /* !CONFIG_SRCU */
>
> +static bool device_is_ancestor(struct device *dev, struct device *target)
> +{
> +       while (target->parent) {
> +               target = target->parent;
> +               if (dev == target)
> +                       return true;
> +       }
> +       return false;
> +}
> +
>  /**
>   * device_is_dependent - Check if one device depends on another one
>   * @dev: Device to check dependencies for.
> @@ -221,7 +231,12 @@ int device_is_dependent(struct device *d
>         struct device_link *link;
>         int ret;
>
> -       if (dev == target)
> +       /*
> +        * The "ancestors" check is needed to catch the case when the target
> +        * device has not been completely initialized yet and it is still
> +        * missing from the list of children of its parent device.
> +        */
> +       if (dev == target || device_is_ancestor(dev, target))
>                 return 1;
>
>         ret = device_for_each_child(dev, target, device_is_dependent);
>
>
>
Greg Kroah-Hartman Jan. 19, 2021, 3:21 p.m. UTC | #4
On Tue, Jan 19, 2021 at 04:09:14PM +0100, Rafael J. Wysocki wrote:
> On Fri, Jan 15, 2021 at 7:31 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > If the device passed as the target (second argument) to
> > device_is_dependent() is not completely registered (that is, it has
> > been initialized, but not added yet), but the parent pointer of it
> > is set, it may be missing from the list of the parent's children
> > and device_for_each_child() called by device_is_dependent() cannot
> > be relied on to catch that dependency.
> >
> > For this reason, modify device_is_dependent() to check the ancestors
> > of the target device by following its parent pointer in addition to
> > the device_for_each_child() walk.
> >
> > Fixes: 9ed9895370ae ("driver core: Functional dependencies tracking support")
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > Reported-by: Stephan Gerhold <stephan@gerhold.net>
> > Tested-by: Stephan Gerhold <stephan@gerhold.net>
> 
> Greg, are you going to pick up this one or do you want me to take care of it?
> 
> It has been reviewed by Saravana.

I'll take it, thanks.  sorry for the delay.

greg k-h

Patch
diff mbox series

Index: linux-pm/drivers/base/core.c
===================================================================
--- linux-pm.orig/drivers/base/core.c
+++ linux-pm/drivers/base/core.c
@@ -208,6 +208,16 @@  int device_links_read_lock_held(void)
 #endif
 #endif /* !CONFIG_SRCU */
 
+static bool device_is_ancestor(struct device *dev, struct device *target)
+{
+	while (target->parent) {
+		target = target->parent;
+		if (dev == target)
+			return true;
+	}
+	return false;
+}
+
 /**
  * device_is_dependent - Check if one device depends on another one
  * @dev: Device to check dependencies for.
@@ -221,7 +231,12 @@  int device_is_dependent(struct device *d
 	struct device_link *link;
 	int ret;
 
-	if (dev == target)
+	/*
+	 * The "ancestors" check is needed to catch the case when the target
+	 * device has not been completely initialized yet and it is still
+	 * missing from the list of children of its parent device.
+	 */
+	if (dev == target || device_is_ancestor(dev, target))
 		return 1;
 
 	ret = device_for_each_child(dev, target, device_is_dependent);