diff mbox series

[v1,9/9] of: property: Simplify of_link_to_phandle()

Message ID 20220810060040.321697-10-saravanak@google.com (mailing list archive)
State Handled Elsewhere, archived
Headers show
Series fw_devlink improvements | expand

Commit Message

Saravana Kannan Aug. 10, 2022, 6 a.m. UTC
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(-)

Comments

Tony Lindgren Aug. 12, 2022, 9:47 a.m. UTC | #1
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
Saravana Kannan Aug. 13, 2022, 12:37 a.m. UTC | #2
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
Tony Lindgren Aug. 15, 2022, 10:31 a.m. UTC | #3
* 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
Saravana Kannan Aug. 15, 2022, 9 p.m. UTC | #4
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 mbox series

Patch

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;
 }
 
 /**