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(-)
diff --git a/drivers/of/property.c b/drivers/of/property.c
index 5036a362f52e..2bb3158c9e43 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -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++;
}
--
2.31.0.291.g576ba9dcdaf-goog
Quoting Saravana Kannan (2021-03-30 11:50:55)
> 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]>
> ---
Tested-by: Stephen Boyd <[email protected]>
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index 5036a362f52e..2bb3158c9e43 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -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").
> + * @node_not_dev: The consumer node containing the property is never a device.
> *
> * Returns:
> * parse_prop() return values are
On Tue, Mar 30, 2021 at 12:36 PM Stephen Boyd <[email protected]> wrote:
>
> Quoting Saravana Kannan (2021-03-30 11:50:55)
> > 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]>
> > ---
>
> Tested-by: Stephen Boyd <[email protected]>
Thanks!
>
> > diff --git a/drivers/of/property.c b/drivers/of/property.c
> > index 5036a362f52e..2bb3158c9e43 100644
> > --- a/drivers/of/property.c
> > +++ b/drivers/of/property.c
> > @@ -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
On Tue, Mar 30, 2021 at 11:50:55AM -0700, Saravana Kannan wrote:
> 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.
Now picked up, thanks.
greg k-h