Messages in this thread | | | From | Saravana Kannan <> | Date | Thu, 14 Jan 2021 11:31:12 -0800 | Subject | Re: [PATCH] driver core: Extend device_is_dependent() |
| |
On Thu, Jan 14, 2021 at 10:41 AM Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > When adding a new device link, device_is_dependent() is used to > check whether or not the prospective supplier device does not > depend on the prospective consumer one to avoid adding loops > to the graph of device dependencies. > > However, device_is_dependent() does not take the ancestors of > the target device into account, so it may not detect an existing > reverse dependency if, for example, the parent of the target > device depends on the device passed as its first argument. > > For this reason, extend device_is_dependent() to also check if > the device passed as its first argument is an ancestor of the > target one and return 1 if that is the case. > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > Reported-by: Stephan Gerhold <stephan@gerhold.net> > --- > drivers/base/core.c | 12 +++++++++++- > 1 file changed, 11 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,7 @@ int device_is_dependent(struct device *d > struct device_link *link; > int ret; > > - if (dev == target) > + if (dev == target || device_is_ancestor(dev, target)) > return 1; > > ret = device_for_each_child(dev, target, device_is_dependent); >
The code works, but it's not at all obvious what it's doing. Because, at first glance, it's easy to mistakenly think that it's trying to catch this case: dev <- child1 <- child2 <- target
Maybe it's clearer if we do this check inside the loop? Something like:
if (link->consumer == target || device_is_ancestor(link->consumer, target)) return 1;
-Saravana
| |