Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <2073294.4OfjquceTg@kreacher>
In-Reply-To: <2073294.4OfjquceTg@kreacher>
From:   Saravana Kannan <saravanak@google.com>
Date:   Thu, 14 Jan 2021 11:31:12 -0800
Message-ID: <CAGETcx980TXe_Jur3LqpWoMwt0wG9BBvVdXfhAo3jU8-tgv=kw@mail.gmail.com>
Subject: Re: [PATCH] driver core: Extend device_is_dependent()
To:     "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Linux PM <linux-pm@vger.kernel.org>,
        Stephan Gerhold <stephan@gerhold.net>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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