Re: [PATCH] driver core: Extend device_is_dependent()

[Saravana Kannan]

On Thu, Jan 14, 2021 at 11:38 AM Rafael J. Wysocki <rafael@xxxxxxxxxx> wrote:
>
> On Thu, Jan 14, 2021 at 8:32 PM Saravana Kannan <saravanak@xxxxxxxxxx> wrote:
> >
> > On Thu, Jan 14, 2021 at 10:41 AM Rafael J. Wysocki <rjw@xxxxxxxxxxxxx> wrote:
> > >
> > > From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> > >
> > > 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@xxxxxxxxx>
> > > Reported-by: Stephan Gerhold <stephan@xxxxxxxxxxx>
> > > ---
> > >  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?
>
> Which of the loops do you mean?

Sorry, the list of consumers loop.

> There are two of them and both need
> to do the check in each step AFAICS.

I don't think we need it in the "loop through children" one. Here's why.

We already make sure:
1. The prospective supplier (target) is not a child of the prospective
consumer (dev).
2. The prospective supplier (target) is not a consumer of the
prospective consumer (dev) or any of its children.

To address the problem in the commit, we need to make changes to make sure:
3. The ancestor of prospective supplier (ancestor of target) is not a
child of prospective consumer (dev)
4. The ancestor of prospective supplier (ancestor of target) is not a
consumer of the prospective consumer (dev) or any of its children.

But (3) would be caught automatically when we do (1). Because if (3)
is true, (1) would also be true.
So, what's left is (4), for which my suggestion should be sufficient?

Does it make sense? Or am I missing anything else that needs to be checked?

-Saravana

>
> > Something like:
> >
> >                 if (link->consumer == target ||
> > device_is_ancestor(link->consumer, target))
> >                         return 1;
>
> So would this be sufficient?