| Message ID | 20210330185056.1022008-1-saravanak@google.com |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | [v1] of: property: fw_devlink: Add support for remote-endpoint | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | warning | total: 0 errors, 2 warnings, 97 lines checked |
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 <swboyd@chromium.org> > Signed-off-by: Saravana Kannan <saravanak@google.com> > --- Tested-by: Stephen Boyd <swboyd@chromium.org> > 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 <swboyd@chromium.org> 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 <swboyd@chromium.org> > > Signed-off-by: Saravana Kannan <saravanak@google.com> > > --- > > Tested-by: Stephen Boyd <swboyd@chromium.org> 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 <swboyd@chromium.org> > Signed-off-by: Saravana Kannan <saravanak@google.com> > --- > Rob/Greg, > > This needs to go into driver-core due to the Fixes. Now picked up, thanks. greg k-h
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++; }
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 <swboyd@chromium.org> Signed-off-by: Saravana Kannan <saravanak@google.com> --- 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(-)