[PATCH 0/4] of: property: fw_devlink misc fixes

[PATCH 0/4] of: property: fw_devlink misc fixes

[Nicolas Saenz Julienne]

As I'm interested in using this feature to fine-tune Raspberry Pi 4's
device probe dependencies, I tried to get the board to boot with
fw_devlink=on. As of today's linux-next the board won't boot with that
option. I tried to address the underlying issues.

---

Nicolas Saenz Julienne (4):
  of: property: Fix create device links for all child-supplier
    dependencies
  of: property: Do not link to disabled devices
  of: property: Move of_link_to_phandle()
  of: property: Avoid linking devices with circular dependencies

 drivers/of/property.c | 190 +++++++++++++++++++++++++++---------------
 1 file changed, 124 insertions(+), 66 deletions(-)

-- 
2.26.0

[PATCH 1/4] of: property: Fix create device links for all child-supplier dependencies

[Nicolas Saenz Julienne]

Upon adding a new platform device we scan its properties and its
children's properties in order to create a consumer/supplier
relationship between the device and the property supplier.

That said, it's possible for some of the node's children to be disabled,
which will create links that'll never be fulfilled.

To get around this, use the for_each_available_child_of_node() function
instead of for_each_available_node() when iterating over the node's
children.

Fixes: d4387cd11741 ("of: property: Create device links for all child-supplier depencencies")
Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
---
 drivers/of/property.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/of/property.c b/drivers/of/property.c
index b4916dcc9e725..a8c2b13521b27 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -1296,7 +1296,7 @@ static int of_link_to_suppliers(struct device *dev,
 		if (of_link_property(dev, con_np, p->name))
 			ret = -ENODEV;
 
-	for_each_child_of_node(con_np, child)
+	for_each_available_child_of_node(con_np, child)
 		if (of_link_to_suppliers(dev, child) && !ret)
 			ret = -EAGAIN;
 
-- 
2.26.0

Re: [PATCH 1/4] of: property: Fix create device links for all child-supplier dependencies

[Saravana Kannan]

On Wed, Apr 15, 2020 at 8:06 AM Nicolas Saenz Julienne
<nsaenzjulienne@suse.de> wrote:
>
> Upon adding a new platform device we scan its properties and its
> children's properties in order to create a consumer/supplier
> relationship between the device and the property supplier.
>
> That said, it's possible for some of the node's children to be disabled,
> which will create links that'll never be fulfilled.
>
> To get around this, use the for_each_available_child_of_node() function
> instead of for_each_available_node() when iterating over the node's
> children.
>
> Fixes: d4387cd11741 ("of: property: Create device links for all child-supplier depencencies")
> Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> ---
>  drivers/of/property.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index b4916dcc9e725..a8c2b13521b27 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -1296,7 +1296,7 @@ static int of_link_to_suppliers(struct device *dev,
>                 if (of_link_property(dev, con_np, p->name))
>                         ret = -ENODEV;
>
> -       for_each_child_of_node(con_np, child)
> +       for_each_available_child_of_node(con_np, child)
>                 if (of_link_to_suppliers(dev, child) && !ret)
>                         ret = -EAGAIN;

Thanks. Good catch. I have plenty of disabled devices in my test
platform. But I guess I never ran into this scenario.

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

-Saravana

Re: [PATCH 1/4] of: property: Fix create device links for all child-supplier dependencies

[Saravana Kannan]

Actually a few more nits about the commit text.

On Wed, Apr 15, 2020 at 8:06 AM Nicolas Saenz Julienne
<nsaenzjulienne@suse.de> wrote:
>
> Upon adding a new platform device we scan its properties and its

This code runs for all devices created from a DT node. Not just
platform devices. So fix this paragraph appropriately?

Upon adding a new device from a DT node, we scan... ?

-Saravana

> children's properties in order to create a consumer/supplier
> relationship between the device and the property supplier.
>
> That said, it's possible for some of the node's children to be disabled,
> which will create links that'll never be fulfilled.
>
> To get around this, use the for_each_available_child_of_node() function
> instead of for_each_available_node() when iterating over the node's
> children.
>
> Fixes: d4387cd11741 ("of: property: Create device links for all child-supplier depencencies")
> Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> ---
>  drivers/of/property.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index b4916dcc9e725..a8c2b13521b27 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -1296,7 +1296,7 @@ static int of_link_to_suppliers(struct device *dev,
>                 if (of_link_property(dev, con_np, p->name))
>                         ret = -ENODEV;
>
> -       for_each_child_of_node(con_np, child)
> +       for_each_available_child_of_node(con_np, child)
>                 if (of_link_to_suppliers(dev, child) && !ret)
>                         ret = -EAGAIN;
>
> --
> 2.26.0
>

Re: [PATCH 1/4] of: property: Fix create device links for all child-supplier dependencies

[Nicolas Saenz Julienne]

[-- Attachment #1: Type: text/plain, Size: 500 bytes --]

On Wed, 2020-04-15 at 11:22 -0700, Saravana Kannan wrote:
> Actually a few more nits about the commit text.
> 
> On Wed, Apr 15, 2020 at 8:06 AM Nicolas Saenz Julienne
> <nsaenzjulienne@suse.de> wrote:
> > Upon adding a new platform device we scan its properties and its
> 
> This code runs for all devices created from a DT node. Not just
> platform devices. So fix this paragraph appropriately?
> 
> Upon adding a new device from a DT node, we scan... ?

Noted.

Regards,
Nicolas


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 2/4] of: property: Do not link to disabled devices

[Nicolas Saenz Julienne]

When creating a consumer/supplier relationship between two devices, make
sure the supplier node is actually active. Otherwise this will create a
device link that will never be fulfilled. This, in the worst case
scenario, will hang the system during boot.

Note that, in practice, the fact that a device-tree represented
consumer/supplier relationship isn't fulfilled will not prevent devices
from successfully probing.

Fixes: a3e1d1a7f5fc ("of: property: Add functional dependency link from DT bindings")
Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
---
 drivers/of/property.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/of/property.c b/drivers/of/property.c
index a8c2b13521b27..487685ff8bb19 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -1052,6 +1052,13 @@ static int of_link_to_phandle(struct device *dev, struct device_node *sup_np,
 		return -ENODEV;
 	}
 
+	/* Don't allow linking a device node as consumer of a disabled node */
+	if (!of_device_is_available(sup_np)) {
+		dev_dbg(dev, "Not linking to %pOFP - Not available\n", sup_np);
+		of_node_put(sup_np);
+		return -ENODEV;
+	}
+
 	/*
 	 * Don't allow linking a device node as a consumer of one of its
 	 * descendant nodes. By definition, a child node can't be a functional
-- 
2.26.0

Re: [PATCH 2/4] of: property: Do not link to disabled devices

[Saravana Kannan]

On Wed, Apr 15, 2020 at 8:06 AM Nicolas Saenz Julienne
<nsaenzjulienne@suse.de> wrote:
>
> When creating a consumer/supplier relationship between two devices, make
> sure the supplier node is actually active. Otherwise this will create a
> device link that will never be fulfilled. This, in the worst case
> scenario, will hang the system during boot.
>
> Note that, in practice, the fact that a device-tree represented
> consumer/supplier relationship isn't fulfilled will not prevent devices
> from successfully probing.
>
> Fixes: a3e1d1a7f5fc ("of: property: Add functional dependency link from DT bindings")
> Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> ---
>  drivers/of/property.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index a8c2b13521b27..487685ff8bb19 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -1052,6 +1052,13 @@ static int of_link_to_phandle(struct device *dev, struct device_node *sup_np,
>                 return -ENODEV;
>         }
>
> +       /* Don't allow linking a device node as consumer of a disabled node */
> +       if (!of_device_is_available(sup_np)) {
> +               dev_dbg(dev, "Not linking to %pOFP - Not available\n", sup_np);
> +               of_node_put(sup_np);
> +               return -ENODEV;
> +       }
> +

Again, surprised I haven't hit this situation with the number of
disabled devices I have.

The idea is right, but the implementation can be better. I think this
check needs to be the first check after the of_node_get(sup_np) --
before we do any of the "walk up to find the device" part.

Otherwise, you could have a supplier device (the one with compatible
prop) that's available with a child node that's disabled. And the
phandle could be pointing to that disabled child node. If you don't do
this as the first check, you might still try to form a pointless
device link. It won't affect probing (because the actual struct device
will probe) but it's still a pointless device link and a pointless
delay in probing, etc.

-Saravana

Re: [PATCH 2/4] of: property: Do not link to disabled devices

[Nicolas Saenz Julienne]

[-- Attachment #1: Type: text/plain, Size: 2457 bytes --]

On Wed, 2020-04-15 at 11:30 -0700, Saravana Kannan wrote:
> On Wed, Apr 15, 2020 at 8:06 AM Nicolas Saenz Julienne
> <nsaenzjulienne@suse.de> wrote:
> > When creating a consumer/supplier relationship between two devices, make
> > sure the supplier node is actually active. Otherwise this will create a
> > device link that will never be fulfilled. This, in the worst case
> > scenario, will hang the system during boot.
> > 
> > Note that, in practice, the fact that a device-tree represented
> > consumer/supplier relationship isn't fulfilled will not prevent devices
> > from successfully probing.
> > 
> > Fixes: a3e1d1a7f5fc ("of: property: Add functional dependency link from DT
> > bindings")
> > Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> > ---
> >  drivers/of/property.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/drivers/of/property.c b/drivers/of/property.c
> > index a8c2b13521b27..487685ff8bb19 100644
> > --- a/drivers/of/property.c
> > +++ b/drivers/of/property.c
> > @@ -1052,6 +1052,13 @@ static int of_link_to_phandle(struct device *dev,
> > struct device_node *sup_np,
> >                 return -ENODEV;
> >         }
> > 
> > +       /* Don't allow linking a device node as consumer of a disabled node
> > */
> > +       if (!of_device_is_available(sup_np)) {
> > +               dev_dbg(dev, "Not linking to %pOFP - Not available\n",
> > sup_np);
> > +               of_node_put(sup_np);
> > +               return -ENODEV;
> > +       }
> > +
> 
> Again, surprised I haven't hit this situation with the number of
> disabled devices I have.

I'll point out to the example that triggered this issue on my reply to patch
#4.

> The idea is right, but the implementation can be better. I think this
> check needs to be the first check after the of_node_get(sup_np) --
> before we do any of the "walk up to find the device" part.
> 
> Otherwise, you could have a supplier device (the one with compatible
> prop) that's available with a child node that's disabled. And the
> phandle could be pointing to that disabled child node. If you don't do
> this as the first check, you might still try to form a pointless
> device link. It won't affect probing (because the actual struct device
> will probe) but it's still a pointless device link and a pointless
> delay in probing, etc.

Agree, I'll update the patch.

Regards,
Nicolas


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/4] of: property: Do not link to disabled devices

[Saravana Kannan]

On Thu, Apr 16, 2020 at 4:37 AM Nicolas Saenz Julienne
<nsaenzjulienne@suse.de> wrote:
>
> On Wed, 2020-04-15 at 11:30 -0700, Saravana Kannan wrote:
> > On Wed, Apr 15, 2020 at 8:06 AM Nicolas Saenz Julienne
> > <nsaenzjulienne@suse.de> wrote:
> > > When creating a consumer/supplier relationship between two devices, make
> > > sure the supplier node is actually active. Otherwise this will create a
> > > device link that will never be fulfilled. This, in the worst case
> > > scenario, will hang the system during boot.
> > >
> > > Note that, in practice, the fact that a device-tree represented
> > > consumer/supplier relationship isn't fulfilled will not prevent devices
> > > from successfully probing.
> > >
> > > Fixes: a3e1d1a7f5fc ("of: property: Add functional dependency link from DT
> > > bindings")
> > > Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> > > ---
> > >  drivers/of/property.c | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > >
> > > diff --git a/drivers/of/property.c b/drivers/of/property.c
> > > index a8c2b13521b27..487685ff8bb19 100644
> > > --- a/drivers/of/property.c
> > > +++ b/drivers/of/property.c
> > > @@ -1052,6 +1052,13 @@ static int of_link_to_phandle(struct device *dev,
> > > struct device_node *sup_np,
> > >                 return -ENODEV;
> > >         }
> > >
> > > +       /* Don't allow linking a device node as consumer of a disabled node
> > > */
> > > +       if (!of_device_is_available(sup_np)) {
> > > +               dev_dbg(dev, "Not linking to %pOFP - Not available\n",
> > > sup_np);
> > > +               of_node_put(sup_np);
> > > +               return -ENODEV;
> > > +       }
> > > +
> >
> > Again, surprised I haven't hit this situation with the number of
> > disabled devices I have.
>
> I'll point out to the example that triggered this issue on my reply to patch
> #4.
>
> > The idea is right, but the implementation can be better. I think this
> > check needs to be the first check after the of_node_get(sup_np) --
> > before we do any of the "walk up to find the device" part.
> >
> > Otherwise, you could have a supplier device (the one with compatible
> > prop) that's available with a child node that's disabled. And the
> > phandle could be pointing to that disabled child node. If you don't do
> > this as the first check, you might still try to form a pointless
> > device link. It won't affect probing (because the actual struct device
> > will probe) but it's still a pointless device link and a pointless
> > delay in probing, etc.
>
> Agree, I'll update the patch.

I thought about it more. I think you should do this check in the loop
that's walking up to the "compatible" node because any node in that
path having status=disabled would/should disable this supplier if I
understand DT correctly. Technically we need to do this all the way up
to the root, but we'll do that if we have actual reports of that
causing problems. Otherwise, it's just wasteful.

-Saravana

[PATCH 3/4] of: property: Move of_link_to_phandle()

[Nicolas Saenz Julienne]

This is done in order to simplify further changes.

Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
---
 drivers/of/property.c | 145 +++++++++++++++++++++---------------------
 1 file changed, 73 insertions(+), 72 deletions(-)

diff --git a/drivers/of/property.c b/drivers/of/property.c
index 487685ff8bb19..2c7978ef22be1 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -1014,78 +1014,6 @@ static bool of_is_ancestor_of(struct device_node *test_ancestor,
 	return false;
 }
 
-/**
- * of_link_to_phandle - Add device link to supplier from supplier phandle
- * @dev: consumer device
- * @sup_np: phandle to supplier device tree node
- *
- * Given a phandle to a supplier device tree node (@sup_np), this function
- * finds the device that owns the supplier device tree node and creates a
- * device link from @dev consumer device to the supplier device. This function
- * doesn't create device links for invalid scenarios such as trying to create a
- * link with a parent device as the consumer of its child device. In such
- * cases, it returns an error.
- *
- * Returns:
- * - 0 if link successfully created to supplier
- * - -EAGAIN if linking to the supplier should be reattempted
- * - -EINVAL if the supplier link is invalid and should not be created
- * - -ENODEV if there is no device that corresponds to the supplier phandle
- */
-static int of_link_to_phandle(struct device *dev, struct device_node *sup_np,
-			      u32 dl_flags)
-{
-	struct device *sup_dev;
-	int ret = 0;
-	struct device_node *tmp_np = sup_np;
-	int is_populated;
-
-	of_node_get(sup_np);
-	/*
-	 * Find the device node that contains the supplier phandle.  It may be
-	 * @sup_np or it may be an ancestor of @sup_np.
-	 */
-	while (sup_np && !of_find_property(sup_np, "compatible", NULL))
-		sup_np = of_get_next_parent(sup_np);
-	if (!sup_np) {
-		dev_dbg(dev, "Not linking to %pOFP - No device\n", tmp_np);
-		return -ENODEV;
-	}
-
-	/* Don't allow linking a device node as consumer of a disabled node */
-	if (!of_device_is_available(sup_np)) {
-		dev_dbg(dev, "Not linking to %pOFP - Not available\n", sup_np);
-		of_node_put(sup_np);
-		return -ENODEV;
-	}
-
-	/*
-	 * Don't allow linking a device node as a consumer of one of its
-	 * descendant nodes. By definition, a child node can't be a functional
-	 * dependency for the parent node.
-	 */
-	if (of_is_ancestor_of(dev->of_node, sup_np)) {
-		dev_dbg(dev, "Not linking to %pOFP - is descendant\n", sup_np);
-		of_node_put(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) {
-		/* Early device without struct device. */
-		dev_dbg(dev, "Not linking to %pOFP - No struct device\n",
-			sup_np);
-		return -ENODEV;
-	} else if (!sup_dev) {
-		return -EAGAIN;
-	}
-	if (!device_link_add(dev, sup_dev, dl_flags))
-		ret = -EAGAIN;
-	put_device(sup_dev);
-	return ret;
-}
-
 /**
  * parse_prop_cells - Property parsing function for suppliers
  *
@@ -1243,6 +1171,79 @@ static const struct supplier_bindings of_supplier_bindings[] = {
 	{}
 };
 
+/**
+ * of_link_to_phandle - Add device link to supplier from supplier phandle
+ * @dev: consumer device
+ * @sup_np: phandle to supplier device tree node
+ *
+ * Given a phandle to a supplier device tree node (@sup_np), this function
+ * finds the device that owns the supplier device tree node and creates a
+ * device link from @dev consumer device to the supplier device. This function
+ * doesn't create device links for invalid scenarios such as trying to create a
+ * link with a parent device as the consumer of its child device. In such
+ * cases, it returns an error.
+ *
+ * Returns:
+ * - 0 if link successfully created to supplier
+ * - -EAGAIN if linking to the supplier should be reattempted
+ * - -EINVAL if the supplier link is invalid and should not be created
+ * - -ENODEV if there is no device that corresponds to the supplier phandle
+ */
+static int of_link_to_phandle(struct device *dev, struct device_node *sup_np,
+			      u32 dl_flags)
+{
+	struct device *sup_dev;
+	int ret = 0;
+	struct device_node *tmp_np = sup_np;
+	int is_populated;
+
+	of_node_get(sup_np);
+	/*
+	 * Find the device node that contains the supplier phandle.  It may be
+	 * @sup_np or it may be an ancestor of @sup_np.
+	 */
+	while (sup_np && !of_find_property(sup_np, "compatible", NULL))
+		sup_np = of_get_next_parent(sup_np);
+	if (!sup_np) {
+		dev_dbg(dev, "Not linking to %pOFP - No device\n", tmp_np);
+		return -ENODEV;
+	}
+
+	/* Don't allow linking a device node as consumer of a disabled node */
+	if (!of_device_is_available(sup_np)) {
+		dev_dbg(dev, "Not linking to %pOFP - Not available\n", sup_np);
+		of_node_put(sup_np);
+		return -ENODEV;
+	}
+
+	/*
+	 * Don't allow linking a device node as a consumer of one of its
+	 * descendant nodes. By definition, a child node can't be a functional
+	 * dependency for the parent node.
+	 */
+	if (of_is_ancestor_of(dev->of_node, sup_np)) {
+		dev_dbg(dev, "Not linking to %pOFP - is descendant\n", sup_np);
+		of_node_put(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) {
+		/* Early device without struct device. */
+		dev_dbg(dev, "Not linking to %pOFP - No struct device\n",
+			sup_np);
+		return -ENODEV;
+	} else if (!sup_dev) {
+		return -EAGAIN;
+	}
+
+	if (!device_link_add(dev, sup_dev, dl_flags))
+		ret = -EAGAIN;
+	put_device(sup_dev);
+	return ret;
+}
+
 /**
  * of_link_property - Create device links to suppliers listed in a property
  * @dev: Consumer device
-- 
2.26.0

[PATCH 4/4] of: property: Avoid linking devices with circular dependencies

[Nicolas Saenz Julienne]

When creating a consumer/supplier relationship between devices it's
essential to make sure they aren't supplying each other creating a
circular dependency.

Introduce a new function to check if such circular dependency exists
between two device nodes and use it in of_link_to_phandle().

Fixes: a3e1d1a7f5fc ("of: property: Add functional dependency link from DT bindings")
Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
---

NOTE:
 I feel of_link_is_circular() is a little dense, and could benefit from
 some abstraction/refactoring. That said, I'd rather get some feedback,
 before spending time on it.

 drivers/of/property.c | 50 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/drivers/of/property.c b/drivers/of/property.c
index 2c7978ef22be1..74a5190408c3b 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -1171,6 +1171,44 @@ static const struct supplier_bindings of_supplier_bindings[] = {
 	{}
 };
 
+/**
+ * of_link_is_circular - Make sure potential link isn't circular
+ *
+ * @sup_np: Supplier device
+ * @con_np: Consumer device
+ *
+ * This function checks if @sup_np's properties contain a reference to @con_np.
+ *
+ * Will return true if there's a circular dependency and false otherwise.
+ */
+static bool of_link_is_circular(struct device_node *sup_np,
+				struct device_node *con_np)
+{
+	const struct supplier_bindings *s = of_supplier_bindings;
+	struct device_node *tmp;
+	bool matched = false;
+	struct property *p;
+	int i = 0;
+
+	for_each_property_of_node(sup_np, p) {
+		while (!matched && s->parse_prop) {
+			while ((tmp = s->parse_prop(sup_np, p->name, i))) {
+				matched = true;
+				i++;
+
+				if (tmp == con_np)
+					return true;
+			}
+			i = 0;
+			s++;
+		}
+		s = of_supplier_bindings;
+		matched = false;
+	}
+
+	return false;
+}
+
 /**
  * of_link_to_phandle - Add device link to supplier from supplier phandle
  * @dev: consumer device
@@ -1216,6 +1254,18 @@ static int of_link_to_phandle(struct device *dev, struct device_node *sup_np,
 		return -ENODEV;
 	}
 
+	/*
+	 * It is possible for consumer device nodes to also supply the device
+	 * node they are consuming from. Creating an unwarranted circular
+	 * dependency.
+	 */
+	if (of_link_is_circular(sup_np, dev->of_node)) {
+		dev_dbg(dev, "Not linking to %pOFP - Circular dependency\n",
+			sup_np);
+		of_node_put(sup_np);
+		return -ENODEV;
+	}
+
 	/*
 	 * Don't allow linking a device node as a consumer of one of its
 	 * descendant nodes. By definition, a child node can't be a functional
-- 
2.26.0

Re: [PATCH 4/4] of: property: Avoid linking devices with circular dependencies

[Saravana Kannan]

On Wed, Apr 15, 2020 at 8:06 AM Nicolas Saenz Julienne
<nsaenzjulienne@suse.de> wrote:
>
> When creating a consumer/supplier relationship between devices it's
> essential to make sure they aren't supplying each other creating a
> circular dependency.

Kinda correct. But fw_devlink is not just about optimizing probing.
It's also about ensuring sync_state() callbacks work correctly when
drivers are built as modules. And for that to work, circular
"SYNC_STATE_ONLY" device links are allowed. I've explained it in a bit
more detail here [1].

> Introduce a new function to check if such circular dependency exists
> between two device nodes and use it in of_link_to_phandle().
>
> Fixes: a3e1d1a7f5fc ("of: property: Add functional dependency link from DT bindings")
> Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> ---
>
> NOTE:
>  I feel of_link_is_circular() is a little dense, and could benefit from
>  some abstraction/refactoring. That said, I'd rather get some feedback,
>  before spending time on it.

Good call :)

>  drivers/of/property.c | 50 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 50 insertions(+)
>
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index 2c7978ef22be1..74a5190408c3b 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -1171,6 +1171,44 @@ static const struct supplier_bindings of_supplier_bindings[] = {
>         {}
>  };
>
> +/**
> + * of_link_is_circular - Make sure potential link isn't circular
> + *
> + * @sup_np: Supplier device
> + * @con_np: Consumer device
> + *
> + * This function checks if @sup_np's properties contain a reference to @con_np.
> + *
> + * Will return true if there's a circular dependency and false otherwise.
> + */
> +static bool of_link_is_circular(struct device_node *sup_np,
> +                               struct device_node *con_np)
> +{
> +       const struct supplier_bindings *s = of_supplier_bindings;
> +       struct device_node *tmp;
> +       bool matched = false;
> +       struct property *p;
> +       int i = 0;
> +
> +       for_each_property_of_node(sup_np, p) {
> +               while (!matched && s->parse_prop) {
> +                       while ((tmp = s->parse_prop(sup_np, p->name, i))) {
> +                               matched = true;
> +                               i++;
> +
> +                               if (tmp == con_np)
> +                                       return true;
> +                       }
> +                       i = 0;
> +                       s++;
> +               }
> +               s = of_supplier_bindings;
> +               matched = false;
> +       }
> +
> +       return false;
> +}

This only catches circular links made out of 2 devices. If we really
needed such a function that worked correctly to catch bigger
"circles", you'd need to recurse and it'll get super wasteful and
ugly.

Thankfully, device_link_add() already checks for circular dependencies
when we need it and it's much cheaper because the links are at a
device level and not examined at a property level.

Is this a real problem you are hitting with the Raspberry Pi 4's? If
so can you give an example in its DT where you are hitting this?

I'll have to NACK this patch for reasons mentioned above and in [1].
However, I think I have a solution that should work for what I'm
guessing is your real problem. But let me see the description of the
real scenario before I claim to have a solution.

-Saravana

[1] - https://lore.kernel.org/lkml/20191028220027.251605-1-saravanak@google.com/

Re: [PATCH 4/4] of: property: Avoid linking devices with circular dependencies

[Nicolas Saenz Julienne]

[-- Attachment #1: Type: text/plain, Size: 3123 bytes --]

On Wed, 2020-04-15 at 11:52 -0700, Saravana Kannan wrote:
> On Wed, Apr 15, 2020 at 8:06 AM Nicolas Saenz Julienne
> <nsaenzjulienne@suse.de> wrote:
> > When creating a consumer/supplier relationship between devices it's
> > essential to make sure they aren't supplying each other creating a
> > circular dependency.
> 
> Kinda correct. But fw_devlink is not just about optimizing probing.
> It's also about ensuring sync_state() callbacks work correctly when
> drivers are built as modules. And for that to work, circular
> "SYNC_STATE_ONLY" device links are allowed. I've explained it in a bit
> more detail here [1].

Understood.

[...]

> This only catches circular links made out of 2 devices. If we really
> needed such a function that worked correctly to catch bigger
> "circles", you'd need to recurse and it'll get super wasteful and
> ugly.

Yeah, I was kind of expecting this reply :).

> Thankfully, device_link_add() already checks for circular dependencies
> when we need it and it's much cheaper because the links are at a
> device level and not examined at a property level.
> 
> Is this a real problem you are hitting with the Raspberry Pi 4's? If
> so can you give an example in its DT where you are hitting this?

So the DT bit that triggered all this series is in
'arch/arm/boot/dts/bcm283x.dtsi'. Namely the interaction between
'cprman@7e101000' and 'dsi@7e209000.' Both are clock providers and both are
clock consumers of each other.

Well I had a second deeper look at the issue, here is how the circular
dependency breaks the boot process (A being soc, B being cprman and C being
dsi):

Device node A
	Device node B -> C
	Device node C -> B

The probe sequence is the following (with DL_FLAG_AUTOPROBE_CONSUMER):
1. A device is added, the rest of devices are siblings, nothing is done
2. B device is added, C device doesn't exist, B is added to
   'wait_for_suppliers' list with 'need_for_probe' flag set.
3. C device is added, B is picked up from 'wait_for_suppliers' list, device
   link created with B consuming from C.
4. C is then parsed, and tried to be linked with B as a consumer this time.
   This fails after testing for circular deps (by device_is_dependent()) during
   device_link_add(). This leaves C in the 'wait_for_suppliers' list *for ever*
   as every further attempt at add_link() on C will fail.

-> Ultimately this prevents C for ever being probed, which also prevents B from
   being probed. Which isn't good as B is the main clock provider of the system.

Note that B can live without C. I think some clock re-parenting will not be
accessible, but that's all.

> I'll have to NACK this patch for reasons mentioned above and in [1].
> However, I think I have a solution that should work for what I'm
> guessing is your real problem. But let me see the description of the
> real scenario before I claim to have a solution.

My intuition would be, upon getting a circular dep from device_is_dependent()
with DL_FLAG_AUTOPROBE_CONSUMER to switch need_for_probe to false on both
devices.

Regards,
Nicolas


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 4/4] of: property: Avoid linking devices with circular dependencies

[Saravana Kannan]

On Thu, Apr 16, 2020 at 9:01 AM Nicolas Saenz Julienne
<nsaenzjulienne@suse.de> wrote:
>
> On Wed, 2020-04-15 at 11:52 -0700, Saravana Kannan wrote:
> > On Wed, Apr 15, 2020 at 8:06 AM Nicolas Saenz Julienne
> > <nsaenzjulienne@suse.de> wrote:
> > > When creating a consumer/supplier relationship between devices it's
> > > essential to make sure they aren't supplying each other creating a
> > > circular dependency.
> >
> > Kinda correct. But fw_devlink is not just about optimizing probing.
> > It's also about ensuring sync_state() callbacks work correctly when
> > drivers are built as modules. And for that to work, circular
> > "SYNC_STATE_ONLY" device links are allowed. I've explained it in a bit
> > more detail here [1].
>
> Understood.
>
> [...]
>
> > This only catches circular links made out of 2 devices. If we really
> > needed such a function that worked correctly to catch bigger
> > "circles", you'd need to recurse and it'll get super wasteful and
> > ugly.
>
> Yeah, I was kind of expecting this reply :).
>
> > Thankfully, device_link_add() already checks for circular dependencies
> > when we need it and it's much cheaper because the links are at a
> > device level and not examined at a property level.
> >
> > Is this a real problem you are hitting with the Raspberry Pi 4's? If
> > so can you give an example in its DT where you are hitting this?
>
> So the DT bit that triggered all this series is in
> 'arch/arm/boot/dts/bcm283x.dtsi'. Namely the interaction between
> 'cprman@7e101000' and 'dsi@7e209000.' Both are clock providers and both are
> clock consumers of each other.
>
> Well I had a second deeper look at the issue, here is how the circular
> dependency breaks the boot process (A being soc, B being cprman and C being
> dsi):
>
> Device node A
>         Device node B -> C
>         Device node C -> B
>
> The probe sequence is the following (with DL_FLAG_AUTOPROBE_CONSUMER):
> 1. A device is added, the rest of devices are siblings, nothing is done
> 2. B device is added, C device doesn't exist, B is added to
>    'wait_for_suppliers' list with 'need_for_probe' flag set.
> 3. C device is added, B is picked up from 'wait_for_suppliers' list, device
>    link created with B consuming from C.
> 4. C is then parsed, and tried to be linked with B as a consumer this time.
>    This fails after testing for circular deps (by device_is_dependent()) during
>    device_link_add(). This leaves C in the 'wait_for_suppliers' list *for ever*
>    as every further attempt at add_link() on C will fail.
>
> -> Ultimately this prevents C for ever being probed, which also prevents B from
>    being probed. Which isn't good as B is the main clock provider of the system.
>
> Note that B can live without C. I think some clock re-parenting will not be
> accessible, but that's all.
>
> > I'll have to NACK this patch for reasons mentioned above and in [1].
> > However, I think I have a solution that should work for what I'm
> > guessing is your real problem. But let me see the description of the
> > real scenario before I claim to have a solution.
>
> My intuition would be, upon getting a circular dep from device_is_dependent()
> with DL_FLAG_AUTOPROBE_CONSUMER to switch need_for_probe to false on both
> devices.

The problem with that is the devices will start trying to probe and
then defer due to other suppliers that are needed for probing but
haven't been linked yet. So it'll go a bit against what you are trying
to do. Also it doesn't solve the problem of already created links that
are wrong.

I'll send out a patch in reply to your email. I've been meaning to
send that outside of this discussion. It doesn't cover all cases of
cycles, but it'll cover most cases and I think it should fix your case
too.

For a more comprehensive fix, I'd like to do something like what I
explain here [1]. That should be doable for your driver too if you
want to try that approach. But I haven't heard Rob/Frank's opinion on
that.

-Saravana
[1] - https://lore.kernel.org/lkml/CAGETcx_2vdjSWc3BBN-N2WrtJP90ZnH-2vE=2iVuHuaE1YmMWQ@mail.gmail.com/

[PATCH v1] of: property: Don't retry device_link_add() upon failure

[Saravana Kannan]

When of_link_to_phandle() was implemented initially, there was no way to
tell if device_link_add() was failing because the supplier device hasn't
been parsed yet, hasn't been added yet, the links were creating a cycle,
etc. Some of these were transient errors that'd go away at a later
point.

However, with the current set of improved checks, if device_link_add()
fails, it'll only be for permanent errors like cycles or out-of-memory
errors.

Also, with the addition of DL_FLAG_SYNC_STATE_ONLY flag [1] to device
links, all the valid dependency cycles due to "proxy" device links
(needed for correctness of sync_state() device callback) will never fail
device_link_add() due to cycles.

So, continuing to retry failing device links (by returning -EAGAIN) is
no longer useful. At worst, it prevents platforms from setting
fw_devlink=on (or better) because it prevents proper boot up. So, let's
not do that anymore.

[1] - https://lore.kernel.org/lkml/20191028220027.251605-1-saravanak@google.com/
Cc: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Saravana Kannan <saravanak@google.com>
---
 drivers/of/property.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/of/property.c b/drivers/of/property.c
index 252e4f600155..ee1bc267f975 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -1074,7 +1074,7 @@ static int of_link_to_phandle(struct device *dev, struct device_node *sup_np,
 		return -EAGAIN;
 	}
 	if (!device_link_add(dev, sup_dev, dl_flags))
-		ret = -EAGAIN;
+		ret = -EINVAL;
 	put_device(sup_dev);
 	return ret;
 }
-- 
2.26.1.301.g55bc3eb7cb9-goog

Re: [PATCH v1] of: property: Don't retry device_link_add() upon failure

[Nicolas Saenz Julienne]

[-- Attachment #1: Type: text/plain, Size: 1382 bytes --]

On Thu, 2020-04-16 at 13:58 -0700, Saravana Kannan wrote:
> When of_link_to_phandle() was implemented initially, there was no way to
> tell if device_link_add() was failing because the supplier device hasn't
> been parsed yet, hasn't been added yet, the links were creating a cycle,
> etc. Some of these were transient errors that'd go away at a later
> point.
> 
> However, with the current set of improved checks, if device_link_add()
> fails, it'll only be for permanent errors like cycles or out-of-memory
> errors.
> 
> Also, with the addition of DL_FLAG_SYNC_STATE_ONLY flag [1] to device
> links, all the valid dependency cycles due to "proxy" device links
> (needed for correctness of sync_state() device callback) will never fail
> device_link_add() due to cycles.
> 
> So, continuing to retry failing device links (by returning -EAGAIN) is
> no longer useful. At worst, it prevents platforms from setting
> fw_devlink=on (or better) because it prevents proper boot up. So, let's
> not do that anymore.
> 
> [1] - 
> https://lore.kernel.org/lkml/20191028220027.251605-1-saravanak@google.com/
> Cc: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> ---

Tested-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>

Thanks!
Nicolas


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1] of: property: Don't retry device_link_add() upon failure

[Rob Herring]

On Thu, Apr 16, 2020 at 3:58 PM Saravana Kannan <saravanak@google.com> wrote:
>
> When of_link_to_phandle() was implemented initially, there was no way to
> tell if device_link_add() was failing because the supplier device hasn't
> been parsed yet, hasn't been added yet, the links were creating a cycle,
> etc. Some of these were transient errors that'd go away at a later
> point.
>
> However, with the current set of improved checks, if device_link_add()
> fails, it'll only be for permanent errors like cycles or out-of-memory
> errors.

What improved checks? The series from Nicolas?

Is there a dependency between this and Nicolas' series?

Should this go to stable?


>
> Also, with the addition of DL_FLAG_SYNC_STATE_ONLY flag [1] to device
> links, all the valid dependency cycles due to "proxy" device links
> (needed for correctness of sync_state() device callback) will never fail
> device_link_add() due to cycles.
>
> So, continuing to retry failing device links (by returning -EAGAIN) is
> no longer useful. At worst, it prevents platforms from setting
> fw_devlink=on (or better) because it prevents proper boot up. So, let's
> not do that anymore.
>
> [1] - https://lore.kernel.org/lkml/20191028220027.251605-1-saravanak@google.com/
> Cc: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> ---
>  drivers/of/property.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index 252e4f600155..ee1bc267f975 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -1074,7 +1074,7 @@ static int of_link_to_phandle(struct device *dev, struct device_node *sup_np,
>                 return -EAGAIN;
>         }
>         if (!device_link_add(dev, sup_dev, dl_flags))
> -               ret = -EAGAIN;
> +               ret = -EINVAL;
>         put_device(sup_dev);
>         return ret;
>  }
> --
> 2.26.1.301.g55bc3eb7cb9-goog
>

Re: [PATCH v1] of: property: Don't retry device_link_add() upon failure

[Saravana Kannan]

On Fri, Apr 17, 2020 at 11:16 AM Rob Herring <robh+dt@kernel.org> wrote:
>
> On Thu, Apr 16, 2020 at 3:58 PM Saravana Kannan <saravanak@google.com> wrote:
> >
> > When of_link_to_phandle() was implemented initially, there was no way to
> > tell if device_link_add() was failing because the supplier device hasn't
> > been parsed yet, hasn't been added yet, the links were creating a cycle,
> > etc. Some of these were transient errors that'd go away at a later
> > point.
> >
> > However, with the current set of improved checks, if device_link_add()
> > fails, it'll only be for permanent errors like cycles or out-of-memory
> > errors.
>
> What improved checks? The series from Nicolas?
>

Checking for OF_POPULATED and getting the device using get_dev_from_fwnode().
OF_POPULATED ensures the node has been parsed. get_dev_from_fwnode()
ensures the device has been added to driver core.

> Is there a dependency between this and Nicolas' series?

No.

> Should this go to stable?

Kind of a grey area. I mean, if of/fw_devlink is already letting a
platform boot all the way, this doesn't fix anything. I doubt anyone
in a stable kernel is turning on this feature if it affects device
probing. I'd say the same for Nicolas' series too. It allows more
platforms to work, but if a platform is fully working, it doesn't
improve anything.

Long story short, your call for stable.

-Saravana

Re: [PATCH v1] of: property: Don't retry device_link_add() upon failure

[Rob Herring]

On Thu, 16 Apr 2020 13:58:38 -0700, Saravana Kannan wrote:
> When of_link_to_phandle() was implemented initially, there was no way to
> tell if device_link_add() was failing because the supplier device hasn't
> been parsed yet, hasn't been added yet, the links were creating a cycle,
> etc. Some of these were transient errors that'd go away at a later
> point.
> 
> However, with the current set of improved checks, if device_link_add()
> fails, it'll only be for permanent errors like cycles or out-of-memory
> errors.
> 
> Also, with the addition of DL_FLAG_SYNC_STATE_ONLY flag [1] to device
> links, all the valid dependency cycles due to "proxy" device links
> (needed for correctness of sync_state() device callback) will never fail
> device_link_add() due to cycles.
> 
> So, continuing to retry failing device links (by returning -EAGAIN) is
> no longer useful. At worst, it prevents platforms from setting
> fw_devlink=on (or better) because it prevents proper boot up. So, let's
> not do that anymore.
> 
> [1] - https://lore.kernel.org/lkml/20191028220027.251605-1-saravanak@google.com/
> Cc: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> ---
>  drivers/of/property.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

Applied, thanks.

Rob

Re: [PATCH 0/4] of: property: fw_devlink misc fixes

[Saravana Kannan]

On Wed, Apr 15, 2020 at 8:06 AM Nicolas Saenz Julienne
<nsaenzjulienne@suse.de> wrote:
>
> As I'm interested in using this feature to fine-tune Raspberry Pi 4's

You've made my day! Finally another user outside of Android. :) If
this does improve the boot time, I'd be super interested to see the
numbers.

> device probe dependencies, I tried to get the board to boot with
> fw_devlink=on. As of today's linux-next the board won't boot with that
> option. I tried to address the underlying issues.

I'll review the patches. Apologies in advance if my explanations
aren't thorough. A bit swamped right now.

-Saravana

Re: [PATCH 0/4] of: property: fw_devlink misc fixes

[Nicolas Saenz Julienne]

[-- Attachment #1: Type: text/plain, Size: 1531 bytes --]

On Wed, 2020-04-15 at 11:17 -0700, Saravana Kannan wrote:
> On Wed, Apr 15, 2020 at 8:06 AM Nicolas Saenz Julienne
> <nsaenzjulienne@suse.de> wrote:
> > As I'm interested in using this feature to fine-tune Raspberry Pi 4's
> 
> You've made my day! Finally another user outside of Android. :) If
> this does improve the boot time, I'd be super interested to see the
> numbers.

Actually making the boot time faster isn't my main objective just a nice
possible side-effect. I'll give you some numbers nonetheless :).

I have two things in mind:
 - Exploring if fw_devlink=on can help us solve a rather convoluted device
   initialization depency we're seeing in RPi4. It could potentially prevent us
   from adding nasty platform specific driver code.
 - See if we can use all this information to fine-tune initrd generation on
   smaller arm devices with limited i/o speeds.

Do you have any plans in moving the default behavior to fw_devlink=on? If so
what is blocking us?

Also do you think it'd be reasonable to add a DT binding to set the desired
fw_devlink level? Something like a 'linux,fw_devlink' property under the
/chosen node.

> > device probe dependencies, I tried to get the board to boot with
> > fw_devlink=on. As of today's linux-next the board won't boot with that
> > option. I tried to address the underlying issues.
> 
> I'll review the patches. Apologies in advance if my explanations
> aren't thorough. A bit swamped right now.

They were pretty clear!

Thanks,
Nicolas


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 0/4] of: property: fw_devlink misc fixes

[Saravana Kannan]

On Thu, Apr 16, 2020 at 4:03 AM Nicolas Saenz Julienne
<nsaenzjulienne@suse.de> wrote:
>
> On Wed, 2020-04-15 at 11:17 -0700, Saravana Kannan wrote:
> > On Wed, Apr 15, 2020 at 8:06 AM Nicolas Saenz Julienne
> > <nsaenzjulienne@suse.de> wrote:
> > > As I'm interested in using this feature to fine-tune Raspberry Pi 4's
> >
> > You've made my day! Finally another user outside of Android. :) If
> > this does improve the boot time, I'd be super interested to see the
> > numbers.
>
> Actually making the boot time faster isn't my main objective just a nice
> possible side-effect. I'll give you some numbers nonetheless :).

Thanks!

> I have two things in mind:
>  - Exploring if fw_devlink=on can help us solve a rather convoluted device
>    initialization depency we're seeing in RPi4. It could potentially prevent us
>    from adding nasty platform specific driver code.

I hope it does! I've also noticed that fw_devlink avoids the need for
ugly hacks in drivers and side steps poorly written error handling in
drivers.

>  - See if we can use all this information to fine-tune initrd generation on
>    smaller arm devices with limited i/o speeds.

That's pretty cool. I have no idea how fw_devlink helps here, but I'm
glad it does :)

> Do you have any plans in moving the default behavior to fw_devlink=on? If so
> what is blocking us?

That's my eventual goal. The main reasons it hasn't been done yet are:
1. Cases like yours where there might be fake cycles.
2. Cases of DT with bad choice of properties. Say, something like
"nr-gpios" would cause error spew in the logs (not a functional
error).
3. Whatever other unknown reasons this might cause boot up issues.

I'm starting with trying to turn on fw_devlink=permissive so that
driver developers can stop playing chicken with initcall levels. Then
work towards setting fw_devlink=on (going to be a long road).

> Also do you think it'd be reasonable to add a DT binding to set the desired
> fw_devlink level? Something like a 'linux,fw_devlink' property under the
> /chosen node.

I don't mind that, but not sure if DT maintainers are okay with it.
But if we do have that, I'd still want the kernel command line to
override it.

Thanks,
Saravana