| Message ID | 20220810060040.321697-10-saravanak@google.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | fw_devlink improvements | expand |
Hi, * Saravana Kannan <saravanak@google.com> [220810 05:54]: > The driver core now: > - Has the parent device of a supplier pick up the consumers if the > supplier never has a device created for it. > - Ignores a supplier if the supplier has no parent device and will never > be probed by a driver > > And already prevents creating a device link with the consumer as a > supplier of a parent. > > So, we no longer need to find the "compatible" node of the supplier or > do any other checks in of_link_to_phandle(). We simply need to make sure > that the supplier is available in DT. This patch fixes booting for me, so it should be applied as a fix and tagged with: Fixes: 5a46079a9645 ("PM: domains: Delete usage of driver_deferred_probe_check_state()") If there are dependencies to the other patches in this series, it might make sense to revert commit 5a46079a9645 instead. Anyways, thanks for fixing the issue, for this patch: Reviewed-by: Tony Lindgren <tony@atomide.com> Tested-by: Tony Lindgren <tony@atomide.com> For the process, looks like the earlier series got merged despite the issues reported. And we had non-booting Linux next for at least some SoCs for weeks. And now we are about to have a non-booting -rc1 unless things get fixed fast. Annoying glitches, sigh.. Regards, Tony
On Fri, Aug 12, 2022 at 2:47 AM Tony Lindgren <tony@atomide.com> wrote: > > Hi, > > * Saravana Kannan <saravanak@google.com> [220810 05:54]: > > The driver core now: > > - Has the parent device of a supplier pick up the consumers if the > > supplier never has a device created for it. > > - Ignores a supplier if the supplier has no parent device and will never > > be probed by a driver > > > > And already prevents creating a device link with the consumer as a > > supplier of a parent. > > > > So, we no longer need to find the "compatible" node of the supplier or > > do any other checks in of_link_to_phandle(). We simply need to make sure > > that the supplier is available in DT. > > This patch fixes booting for me, so it should be applied as a fix and > tagged with: > > Fixes: 5a46079a9645 ("PM: domains: Delete usage of driver_deferred_probe_check_state()") > > If there are dependencies to the other patches in this series, it might > make sense to revert commit 5a46079a9645 instead. Yes, there are dependencies on the rest of the patches in this series. For linux-next, I think we should pick up this series once we get more Tested-bys. So if 5a46079a9645 is causing any regression in stable branches, we should pick up the revert series [1] instead of this series we are replying to. [1] - https://lore.kernel.org/all/20220727185012.3255200-1-saravanak@google.com/ > Anyways, thanks for fixing the issue, for this patch: > > Reviewed-by: Tony Lindgren <tony@atomide.com> > Tested-by: Tony Lindgren <tony@atomide.com> Thanks! > For the process, looks like the earlier series got merged despite the > issues reported. If I'm not mistaken, the issues were reported after the series got picked up. And the series got some tested-by s before it was picked up. And once it's in git history, we obviously can't drop it. > And we had non-booting Linux next for at least some SoCs > for weeks. And now we are about to have a non-booting -rc1 unless things > get fixed fast. Annoying glitches, sigh.. Sorry for breaking some boards -- so mean "creative" corner cases :) This rewrite is way more flexible (removes a lot of limitations in fw_devlink) and hopefully this handles all the corner cases. We'll see. -Saravana
* Saravana Kannan <saravanak@google.com> [220813 00:30]: > On Fri, Aug 12, 2022 at 2:47 AM Tony Lindgren <tony@atomide.com> wrote: > > > > Hi, > > > > * Saravana Kannan <saravanak@google.com> [220810 05:54]: > > > The driver core now: > > > - Has the parent device of a supplier pick up the consumers if the > > > supplier never has a device created for it. > > > - Ignores a supplier if the supplier has no parent device and will never > > > be probed by a driver > > > > > > And already prevents creating a device link with the consumer as a > > > supplier of a parent. > > > > > > So, we no longer need to find the "compatible" node of the supplier or > > > do any other checks in of_link_to_phandle(). We simply need to make sure > > > that the supplier is available in DT. > > > > This patch fixes booting for me, so it should be applied as a fix and > > tagged with: > > > > Fixes: 5a46079a9645 ("PM: domains: Delete usage of driver_deferred_probe_check_state()") > > > > If there are dependencies to the other patches in this series, it might > > make sense to revert commit 5a46079a9645 instead. > > Yes, there are dependencies on the rest of the patches in this series. > For linux-next, I think we should pick up this series once we get more > Tested-bys. > > So if 5a46079a9645 is causing any regression in stable branches, we > should pick up the revert series [1] instead of this series we are > replying to. Agreed we should apply the reverts in [1] for v6.0-rc series. At least several generations of the TI 32-bit ARM SoCs are failing to boot otherwise. Regards, Tony
On Mon, Aug 15, 2022 at 3:31 AM Tony Lindgren <tony@atomide.com> wrote: > > * Saravana Kannan <saravanak@google.com> [220813 00:30]: > > On Fri, Aug 12, 2022 at 2:47 AM Tony Lindgren <tony@atomide.com> wrote: > > > > > > Hi, > > > > > > * Saravana Kannan <saravanak@google.com> [220810 05:54]: > > > > The driver core now: > > > > - Has the parent device of a supplier pick up the consumers if the > > > > supplier never has a device created for it. > > > > - Ignores a supplier if the supplier has no parent device and will never > > > > be probed by a driver > > > > > > > > And already prevents creating a device link with the consumer as a > > > > supplier of a parent. > > > > > > > > So, we no longer need to find the "compatible" node of the supplier or > > > > do any other checks in of_link_to_phandle(). We simply need to make sure > > > > that the supplier is available in DT. > > > > > > This patch fixes booting for me, so it should be applied as a fix and > > > tagged with: > > > > > > Fixes: 5a46079a9645 ("PM: domains: Delete usage of driver_deferred_probe_check_state()") > > > > > > If there are dependencies to the other patches in this series, it might > > > make sense to revert commit 5a46079a9645 instead. > > > > Yes, there are dependencies on the rest of the patches in this series. > > For linux-next, I think we should pick up this series once we get more > > Tested-bys. > > > > So if 5a46079a9645 is causing any regression in stable branches, we > > should pick up the revert series [1] instead of this series we are > > replying to. > > Agreed we should apply the reverts in [1] for v6.0-rc series. At least > several generations of the TI 32-bit ARM SoCs are failing to boot > otherwise. Actually I wasn't clear in my earlier email. I meant to say "releases branches", as in 5.19.xxx and not "stable branches". So for 5.19.xxx we'd pick up these reverts. And for v6.0-rc if my other patch series [1] fixes the issue, I'd rather apply [1] than this series. Because this series is meant to be temporary (I'll be reverting this in the future). -Saravana [1] - https://lore.kernel.org/lkml/20220810060040.321697-1-saravanak@google.com/
diff --git a/drivers/of/property.c b/drivers/of/property.c index 967f79b59016..98ca0399a354 100644 --- a/drivers/of/property.c +++ b/drivers/of/property.c @@ -1060,20 +1060,6 @@ of_fwnode_device_get_match_data(const struct fwnode_handle *fwnode, return of_device_get_match_data(dev); } -static bool of_is_ancestor_of(struct device_node *test_ancestor, - struct device_node *child) -{ - of_node_get(child); - while (child) { - if (child == test_ancestor) { - of_node_put(child); - return true; - } - child = of_get_next_parent(child); - } - return false; -} - static struct device_node *of_get_compat_node(struct device_node *np) { of_node_get(np); @@ -1104,71 +1090,27 @@ static struct device_node *of_get_compat_node_parent(struct device_node *np) return node; } -/** - * of_link_to_phandle - Add fwnode link to supplier from supplier phandle - * @con_np: consumer device tree node - * @sup_np: 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 fwnode link successfully created to supplier - * - -EINVAL if the supplier link is invalid and should not be created - * - -ENODEV if struct device will never be create for supplier - */ -static int of_link_to_phandle(struct device_node *con_np, +static void of_link_to_phandle(struct device_node *con_np, struct device_node *sup_np) { - struct device *sup_dev; - struct device_node *tmp_np = sup_np; + struct device_node *tmp_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. - */ - 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); - return -ENODEV; - } + /* Check that sup_np and its ancestors are available. */ + while (tmp_np) { + if (of_fwnode_handle(tmp_np)->dev) { + of_node_put(tmp_np); + break; + } - /* - * 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(con_np, sup_np)) { - pr_debug("Not linking %pOFP to %pOFP - is descendant\n", - con_np, sup_np); - of_node_put(sup_np); - return -EINVAL; - } + if (!of_device_is_available(tmp_np)) { + of_node_put(tmp_np); + return; + } - /* - * Don't create links to "early devices" that won't have struct devices - * created for them. - */ - sup_dev = get_dev_from_fwnode(&sup_np->fwnode); - if (!sup_dev && - (of_node_check_flag(sup_np, OF_POPULATED) || - sup_np->fwnode.flags & FWNODE_FLAG_NOT_DEVICE)) { - pr_debug("Not linking %pOFP to %pOFP - No struct device\n", - con_np, sup_np); - of_node_put(sup_np); - return -ENODEV; + tmp_np = of_get_next_parent(tmp_np); } - put_device(sup_dev); fwnode_link_add(of_fwnode_handle(con_np), of_fwnode_handle(sup_np)); - of_node_put(sup_np); - - return 0; } /**
The driver core now: - Has the parent device of a supplier pick up the consumers if the supplier never has a device created for it. - Ignores a supplier if the supplier has no parent device and will never be probed by a driver And already prevents creating a device link with the consumer as a supplier of a parent. So, we no longer need to find the "compatible" node of the supplier or do any other checks in of_link_to_phandle(). We simply need to make sure that the supplier is available in DT. Signed-off-by: Saravana Kannan <saravanak@google.com> --- drivers/of/property.c | 84 +++++++------------------------------------ 1 file changed, 13 insertions(+), 71 deletions(-)