[PATCH v1 0/2] fw_devlink: Improve cycle detection in DT

[PATCH v1 0/2] fw_devlink: Improve cycle detection in DT

[Saravana Kannan]

Patch 2/2 explain the series. Just using a cover letter to thread the
series and add CC's.

-Saravana

Saravana Kannan (2):
  driver core: Add device_is_dependent() to linux/device.h
  of: property: Improve cycle detection when one of the devices is never
    added

 drivers/base/core.c    |  2 +-
 drivers/of/property.c  | 44 ++++++++++++++++++++++++++++++++++++------
 include/linux/device.h |  1 +
 3 files changed, 40 insertions(+), 7 deletions(-)

-- 
2.27.0.278.ge193c7cf3a9-goog

[PATCH v1 1/2] driver core: Add device_is_dependent() to linux/device.h

[Saravana Kannan]

DT implementation of fw_devlink needs this function to detect cycles. So
make it available.

Signed-off-by: Saravana Kannan <saravanak@google.com>
---
 drivers/base/core.c    | 2 +-
 include/linux/device.h | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 139cdf7e7327..509b13610b56 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -118,7 +118,7 @@ int device_links_read_lock_held(void)
  * Check if @target depends on @dev or any device dependent on it (its child or
  * its consumer etc).  Return 1 if that is the case or 0 otherwise.
  */
-static int device_is_dependent(struct device *dev, void *target)
+int device_is_dependent(struct device *dev, void *target)
 {
 	struct device_link *link;
 	int ret;
diff --git a/include/linux/device.h b/include/linux/device.h
index ac8e37cd716a..5c618effc51e 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -829,6 +829,7 @@ extern int device_change_owner(struct device *dev, kuid_t kuid, kgid_t kgid);
 extern const char *device_get_devnode(struct device *dev,
 				      umode_t *mode, kuid_t *uid, kgid_t *gid,
 				      const char **tmp);
+extern int device_is_dependent(struct device *dev, void *target);
 
 static inline bool device_supports_offline(struct device *dev)
 {
-- 
2.27.0.278.ge193c7cf3a9-goog

[PATCH v1 2/2] of: property: Improve cycle detection when one of the devices is never added

[Saravana Kannan]

Consider this example where -> means LHS device is a consumer of RHS
device and indentation represents "child of" of the previous device.

Device A -> Device C

Device B -> Device A
	Device C

Without this commit:
1. Device A is added.
2. Device A is added to waiting for supplier list (Device C)
3. Device B is added
4. Device B is linked as a consumer to Device A
5. Device A doesn't probe because it's waiting for Device C to be added.
6. Device B doesn't probe because Device A hasn't probed.
7. Device C will never be added because it's parent hasn't probed.

So, Device A, B and C will be in a probe/add deadlock.

This commit detects this scenario and stops trying to create a device
link between Device A and Device C since doing so would create a cycle:
Device A -> Devic C -(parent)-> Device B -> Device A.

With this commit:
1. Device A is added.
3. Device B is added
4. Device B is linked as a consumer to Device A
5. Device A probes.
6. Device B probes because Device A has probed.
7. Device C is added and probed.

Signed-off-by: Saravana Kannan <saravanak@google.com>
---
 drivers/of/property.c | 44 +++++++++++++++++++++++++++++++++++++------
 1 file changed, 38 insertions(+), 6 deletions(-)

diff --git a/drivers/of/property.c b/drivers/of/property.c
index 1f2086f4e7ce..7eebe21274a4 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -1014,6 +1014,20 @@ static bool of_is_ancestor_of(struct device_node *test_ancestor,
 	return false;
 }
 
+static struct device *of_get_next_parent_dev(struct device_node *np)
+{
+	struct device *dev = NULL;
+
+	of_node_get(np);
+	do {
+		np = of_get_next_parent(np);
+		if (np)
+			dev = get_dev_from_fwnode(&np->fwnode);
+	} while (np && !dev);
+	of_node_put(np);
+	return dev;
+}
+
 /**
  * of_link_to_phandle - Add device link to supplier from supplier phandle
  * @dev: consumer device
@@ -1035,10 +1049,9 @@ static bool of_is_ancestor_of(struct device_node *test_ancestor,
 static int of_link_to_phandle(struct device *dev, struct device_node *sup_np,
 			      u32 dl_flags)
 {
-	struct device *sup_dev;
+	struct device *sup_dev, *sup_par_dev;
 	int ret = 0;
 	struct device_node *tmp_np = sup_np;
-	int is_populated;
 
 	of_node_get(sup_np);
 	/*
@@ -1075,16 +1088,35 @@ static int of_link_to_phandle(struct device *dev, struct device_node *sup_np,
 		return -EINVAL;
 	}
 	sup_dev = get_dev_from_fwnode(&sup_np->fwnode);
-	is_populated = of_node_check_flag(sup_np, OF_POPULATED);
-	of_node_put(sup_np);
-	if (!sup_dev && is_populated) {
+	if (!sup_dev && of_node_check_flag(sup_np, OF_POPULATED)) {
 		/* Early device without struct device. */
 		dev_dbg(dev, "Not linking to %pOFP - No struct device\n",
 			sup_np);
+		of_node_put(sup_np);
 		return -ENODEV;
 	} else if (!sup_dev) {
-		return -EAGAIN;
+		sup_par_dev = of_get_next_parent_dev(sup_np);
+		of_node_put(sup_np);
+
+		/*
+		 * DL_FLAG_SYNC_STATE_ONLY doesn't block probing, so cycle
+		 * detection isn't necessary and shouldn't be done.
+		 */
+		if (dl_flags & DL_FLAG_SYNC_STATE_ONLY)
+			return -EAGAIN;
+
+		/*
+		 * If devices haven't been created for any of the ancestors, we
+		 * can't check for cycles. So let's try again later.
+		 */
+		if (!sup_par_dev)
+			return -EAGAIN;
+
+		/* Cyclic dependency detected, don't try to link */
+		if (device_is_dependent(dev, sup_par_dev))
+			return -EINVAL;
 	}
+	of_node_put(sup_np);
 	if (!device_link_add(dev, sup_dev, dl_flags))
 		ret = -EINVAL;
 	put_device(sup_dev);
-- 
2.27.0.278.ge193c7cf3a9-goog

Re: [PATCH v1 2/2] of: property: Improve cycle detection when one of the devices is never added

[Saravana Kannan]

On Fri, Jun 5, 2020 at 5:36 PM Saravana Kannan <saravanak@google.com> wrote:
>
> Consider this example where -> means LHS device is a consumer of RHS
> device and indentation represents "child of" of the previous device.
>
> Device A -> Device C
>
> Device B -> Device A
>         Device C
>
> Without this commit:
> 1. Device A is added.
> 2. Device A is added to waiting for supplier list (Device C)
> 3. Device B is added
> 4. Device B is linked as a consumer to Device A
> 5. Device A doesn't probe because it's waiting for Device C to be added.
> 6. Device B doesn't probe because Device A hasn't probed.
> 7. Device C will never be added because it's parent hasn't probed.
>
> So, Device A, B and C will be in a probe/add deadlock.
>
> This commit detects this scenario and stops trying to create a device
> link between Device A and Device C since doing so would create a cycle:
> Device A -> Devic C -(parent)-> Device B -> Device A.
>
> With this commit:
> 1. Device A is added.
> 3. Device B is added
> 4. Device B is linked as a consumer to Device A
> 5. Device A probes.
> 6. Device B probes because Device A has probed.
> 7. Device C is added and probed.
>
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> ---
>  drivers/of/property.c | 44 +++++++++++++++++++++++++++++++++++++------
>  1 file changed, 38 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index 1f2086f4e7ce..7eebe21274a4 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -1014,6 +1014,20 @@ static bool of_is_ancestor_of(struct device_node *test_ancestor,
>         return false;
>  }
>
> +static struct device *of_get_next_parent_dev(struct device_node *np)
> +{
> +       struct device *dev = NULL;
> +
> +       of_node_get(np);
> +       do {
> +               np = of_get_next_parent(np);
> +               if (np)
> +                       dev = get_dev_from_fwnode(&np->fwnode);
> +       } while (np && !dev);
> +       of_node_put(np);
> +       return dev;
> +}
> +
>  /**
>   * of_link_to_phandle - Add device link to supplier from supplier phandle
>   * @dev: consumer device
> @@ -1035,10 +1049,9 @@ static bool of_is_ancestor_of(struct device_node *test_ancestor,
>  static int of_link_to_phandle(struct device *dev, struct device_node *sup_np,
>                               u32 dl_flags)
>  {
> -       struct device *sup_dev;
> +       struct device *sup_dev, *sup_par_dev;
>         int ret = 0;
>         struct device_node *tmp_np = sup_np;
> -       int is_populated;
>
>         of_node_get(sup_np);
>         /*
> @@ -1075,16 +1088,35 @@ static int of_link_to_phandle(struct device *dev, struct device_node *sup_np,
>                 return -EINVAL;
>         }
>         sup_dev = get_dev_from_fwnode(&sup_np->fwnode);
> -       is_populated = of_node_check_flag(sup_np, OF_POPULATED);
> -       of_node_put(sup_np);
> -       if (!sup_dev && is_populated) {
> +       if (!sup_dev && of_node_check_flag(sup_np, OF_POPULATED)) {
>                 /* Early device without struct device. */
>                 dev_dbg(dev, "Not linking to %pOFP - No struct device\n",
>                         sup_np);
> +               of_node_put(sup_np);
>                 return -ENODEV;
>         } else if (!sup_dev) {
> -               return -EAGAIN;
> +               sup_par_dev = of_get_next_parent_dev(sup_np);
> +               of_node_put(sup_np);
> +
> +               /*
> +                * DL_FLAG_SYNC_STATE_ONLY doesn't block probing, so cycle
> +                * detection isn't necessary and shouldn't be done.
> +                */
> +               if (dl_flags & DL_FLAG_SYNC_STATE_ONLY)
> +                       return -EAGAIN;

I need to put_device(sup_par_dev) before I return here and at other places
below. I'll send a v2 later to fix this.


-Saravana

> +
> +               /*
> +                * If devices haven't been created for any of the ancestors, we
> +                * can't check for cycles. So let's try again later.
> +                */
> +               if (!sup_par_dev)
> +                       return -EAGAIN;
> +
> +               /* Cyclic dependency detected, don't try to link */
> +               if (device_is_dependent(dev, sup_par_dev))
> +                       return -EINVAL;
>         }
> +       of_node_put(sup_np);
>         if (!device_link_add(dev, sup_dev, dl_flags))
>                 ret = -EINVAL;
>         put_device(sup_dev);
> --
> 2.27.0.278.ge193c7cf3a9-goog
>