of: property: fw_devlink: Add support for remote-endpoint

PATCH Superseded Saravana Kannan 3 participants 4 messages Mar 30, 2021 – Apr 2, 2021 Device tree

Share & tools

Saravana Kannan

author

2021-03-30 18:51 UTC #0 lore raw

PATCH 1/1 of: property: fw_devlink: Add support for remote-endpoint

remote-endpoint property seems to always come in pairs where two devices point to each other. So, we can't really tell from DT if there is a functional probe order dependency between these two devices.

However, there can be other dependencies between two devices that point to each other with remote-endpoint. This non-remote-endpoint dependency combined with one of the remote-endpoint dependency can lead to a cyclic dependency[1].

To avoid this cyclic dependency from incorrectly blocking probes, fw_devlink needs to be made aware of remote-endpoint dependencies even though remote-endpoint dependencies by themselves won't affect probe ordering (because fw_devlink will see the cyclic dependency between remote-endpoint devices and ignore the dependencies that cause the cycle).

Also, if a device ever needs to know if a non-probe-blocking remote-endpoint has finished probing, it can now use the sync_state() to figure it out.

[1] - https://lore.kernel.org/lkml/CAGETcx9Snf23wrXqjDhJiTok9M3GcoVYDSyNYSMj9QnSRrA=cA@mail.gmail.com/#t

Fixes ea718c699055 ("Revert "Revert "driver core: Set fw_devlink=on by default""")

Reported-by Stephen Boyd <[email protected]>

Signed-off-by Saravana Kannan <[email protected]>

---
Rob/Greg,

This needs to go into driver-core due to the Fixes.

-Saravana

 drivers/of/property.c | 48 ++++++++++++++++++++++++++++---------------
 1 file changed, 32 insertions(+), 16 deletions(-)

drivers/of/property.c +32 ~~-16~~

index 5036a362f52e..2bb3158c9e43 100644
@@ -1038,6 +1038,25 @@ static bool of_is_ancestor_of(struct device_node *test_ancestor,
 	return false;
 }
 
+static struct device_node *of_get_compat_node(struct device_node *np)
+{
+	of_node_get(np);
+
+	while (np) {
+		if (!of_device_is_available(np)) {
+			of_node_put(np);
+			np = NULL;
+		}
+
+		if (of_find_property(np, "compatible", NULL))
+			break;
+
+		np = of_get_next_parent(np);
+	}
+
+	return np;
+}
+
 /**
  * of_link_to_phandle - Add fwnode link to supplier from supplier phandle
  * @con_np: consumer device tree node

@@ -1061,25 +1080,11 @@ static int of_link_to_phandle(struct device_node *con_np,
 	struct device *sup_dev;
 	struct device_node *tmp_np = sup_np;
 
-	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) {
-
-		/* Don't allow linking to a disabled supplier */
-		if (!of_device_is_available(sup_np)) {
-			of_node_put(sup_np);
-			sup_np = NULL;
-		}
-
-		if (of_find_property(sup_np, "compatible", NULL))
-			break;
-
-		sup_np = of_get_next_parent(sup_np);
-	}
-
+	sup_np = of_get_compat_node(sup_np);
 	if (!sup_np) {
 		pr_debug("Not linking %pOFP to %pOFP - No device\n",
 			 con_np, tmp_np);

@@ -1225,6 +1230,8 @@ static struct device_node *parse_##fname(struct device_node *np,	     \
  * @parse_prop.prop_name: Name of property holding a phandle value
  * @parse_prop.index: For properties holding a list of phandles, this is the
  *		      index into the list
+ * @optional: The property can be an optional dependency.
+ * @node_not_dev: The consumer node containing the property is never a device.
  *
  * Returns:
  * parse_prop() return values are

@@ -1236,6 +1243,7 @@ struct supplier_bindings {
 	struct device_node *(*parse_prop)(struct device_node *np,
 					  const char *prop_name, int index);
 	bool optional;
+	bool node_not_dev;
 };
 
 DEFINE_SIMPLE_PROP(clocks, "clocks", "#clock-cells")

@@ -1260,6 +1268,7 @@ DEFINE_SIMPLE_PROP(pinctrl5, "pinctrl-5", NULL)
 DEFINE_SIMPLE_PROP(pinctrl6, "pinctrl-6", NULL)
 DEFINE_SIMPLE_PROP(pinctrl7, "pinctrl-7", NULL)
 DEFINE_SIMPLE_PROP(pinctrl8, "pinctrl-8", NULL)
+DEFINE_SIMPLE_PROP(remote_endpoint, "remote-endpoint", NULL)
 DEFINE_SUFFIX_PROP(regulators, "-supply", NULL)
 DEFINE_SUFFIX_PROP(gpio, "-gpio", "#gpio-cells")
 DEFINE_SUFFIX_PROP(gpios, "-gpios", "#gpio-cells")

@@ -1334,6 +1343,7 @@ static const struct supplier_bindings of_supplier_bindings[] = {
 	{ .parse_prop = parse_pinctrl6, },
 	{ .parse_prop = parse_pinctrl7, },
 	{ .parse_prop = parse_pinctrl8, },
+	{ .parse_prop = parse_remote_endpoint, .node_not_dev = true, },
 	{ .parse_prop = parse_gpio_compat, },
 	{ .parse_prop = parse_interrupts, },
 	{ .parse_prop = parse_regulators, },

@@ -1378,10 +1388,16 @@ static int of_link_property(struct device_node *con_np, const char *prop_name)
 		}
 
 		while ((phandle = s->parse_prop(con_np, prop_name, i))) {
+			struct device_node *con_dev_np;
+
+			con_dev_np = s->node_not_dev
+					? of_get_compat_node(con_np)
+					: of_node_get(con_np);
 			matched = true;
 			i++;
-			of_link_to_phandle(con_np, phandle);
+			of_link_to_phandle(con_dev_np, phandle);
 			of_node_put(phandle);
+			of_node_put(con_dev_np);
 		}
 		s++;
 	}

Signature · 1 line

2.31.0.291.g576ba9dcdaf-goog

Stephen Boyd

Tested-by on v1 1/1

2021-03-30 19:37 UTC #1 lore raw

Quoting Saravana Kannan (2021-03-30 11:50:55)

Quoted 20 lines

remote-endpoint property seems to always come in pairs where two devices point to each other. So, we can't really tell from DT if there is a functional probe order dependency between these two devices.
However, there can be other dependencies between two devices that point to each other with remote-endpoint. This non-remote-endpoint dependency combined with one of the remote-endpoint dependency can lead to a cyclic dependency[1].
To avoid this cyclic dependency from incorrectly blocking probes, fw_devlink needs to be made aware of remote-endpoint dependencies even though remote-endpoint dependencies by themselves won't affect probe ordering (because fw_devlink will see the cyclic dependency between remote-endpoint devices and ignore the dependencies that cause the cycle).
Also, if a device ever needs to know if a non-probe-blocking remote-endpoint has finished probing, it can now use the sync_state() to figure it out.

Quoted 5 lines

[1] - https://lore.kernel.org/lkml/CAGETcx9Snf23wrXqjDhJiTok9M3GcoVYDSyNYSMj9QnSRrA=cA@mail.gmail.com/#t
Fixes ea718c699055 ("Revert "Revert "driver core: Set fw_devlink=on by default""")
Reported-by Stephen Boyd <[email protected]>
Signed-off-by Saravana Kannan <[email protected]>
---

Tested-by Stephen Boyd <[email protected]>

drivers/of/property.c static struct device_node *parse_##fname(struct device_node *np, \ · line 1233

Quoted 9 lines

drivers/of/property.c +1 -0

index 5036a362f52e..2bb3158c9e43 100644
@@ -1225,6 +1230,8 @@ static struct device_node *parse_##fname(struct device_node *np,       \
  * @parse_prop.prop_name: Name of property holding a phandle value
  * @parse_prop.index: For properties holding a list of phandles, this is the
  *                   index into the list
+ * @optional: The property can be an optional dependency.

This bit conflicted for me on linux-next today so I dropped it in favor of 3915fed92365 ("of: property: Provide missing member description and remove excess param").

Quoted 4 lines

drivers/of/property.c +1 -0

+ * @node_not_dev: The consumer node containing the property is never a device.
  *
  * Returns:
  * parse_prop() return values are

Saravana Kannan

author

2021-03-30 19:47 UTC #2 lore raw

Quoted 22 lines · Stephen Boyd

Quoting Saravana Kannan (2021-03-30 11:50:55)
Quoted 20 lines
remote-endpoint property seems to always come in pairs where two devices point to each other. So, we can't really tell from DT if there is a functional probe order dependency between these two devices.
However, there can be other dependencies between two devices that point to each other with remote-endpoint. This non-remote-endpoint dependency combined with one of the remote-endpoint dependency can lead to a cyclic dependency[1].
To avoid this cyclic dependency from incorrectly blocking probes, fw_devlink needs to be made aware of remote-endpoint dependencies even though remote-endpoint dependencies by themselves won't affect probe ordering (because fw_devlink will see the cyclic dependency between remote-endpoint devices and ignore the dependencies that cause the cycle).
Also, if a device ever needs to know if a non-probe-blocking remote-endpoint has finished probing, it can now use the sync_state() to figure it out.

Quoted 7 lines

Quoted 5 lines
[1] - https://lore.kernel.org/lkml/CAGETcx9Snf23wrXqjDhJiTok9M3GcoVYDSyNYSMj9QnSRrA=cA@mail.gmail.com/#t
Fixes ea718c699055 ("Revert "Revert "driver core: Set fw_devlink=on by default""")
Reported-by Stephen Boyd <[email protected]>
Signed-off-by Saravana Kannan <[email protected]>
---
Tested-by Stephen Boyd <[email protected]>

Thanks!

Quoted 14 lines

drivers/of/property.c static struct device_node *parse_##fname(struct device_node *np, \ · line 1233
Quoted 10 lines
drivers/of/property.c +1 -0
index 5036a362f52e..2bb3158c9e43 100644
@@ -1225,6 +1230,8 @@ static struct device_node *parse_##fname(struct device_node *np,       \
  * @parse_prop.prop_name: Name of property holding a phandle value
  * @parse_prop.index: For properties holding a list of phandles, this is the
  *                   index into the list
+ * @optional: The property can be an optional dependency.
This bit conflicted for me on linux-next today so I dropped it in favor of 3915fed92365 ("of: property: Provide missing member description and remove excess param").

Ah looks like a change went into DT git repo but not in driver-core yet. Yeah, dropping this bit is fine.

Rob/Greg,

I'll leave it to you to deal with the conflict? I can't send to DT because the fix needs to land in driver-core because of boot issues and I can't resolve the conflict in driver-core because the conflicting change isn't in driver-core yet.

Thanks,
Saravana

Greg Kroah-Hartman

2021-04-02 15:07 UTC #3 lore raw

In reply to Saravana Kannan

Quoted 20 lines · Saravana Kannan

remote-endpoint property seems to always come in pairs where two devices point to each other. So, we can't really tell from DT if there is a functional probe order dependency between these two devices.
However, there can be other dependencies between two devices that point to each other with remote-endpoint. This non-remote-endpoint dependency combined with one of the remote-endpoint dependency can lead to a cyclic dependency[1].
To avoid this cyclic dependency from incorrectly blocking probes, fw_devlink needs to be made aware of remote-endpoint dependencies even though remote-endpoint dependencies by themselves won't affect probe ordering (because fw_devlink will see the cyclic dependency between remote-endpoint devices and ignore the dependencies that cause the cycle).
Also, if a device ever needs to know if a non-probe-blocking remote-endpoint has finished probing, it can now use the sync_state() to figure it out.

Quoted 8 lines

[1] - https://lore.kernel.org/lkml/CAGETcx9Snf23wrXqjDhJiTok9M3GcoVYDSyNYSMj9QnSRrA=cA@mail.gmail.com/#t
Fixes ea718c699055 ("Revert "Revert "driver core: Set fw_devlink=on by default""")
Reported-by Stephen Boyd <[email protected]>
Signed-off-by Saravana Kannan <[email protected]>
---
Rob/Greg,
This needs to go into driver-core due to the Fixes.

Now picked up, thanks.

greg k-h