[PATCH v2 0/3] fw_devlink overlapping cycles fix

[PATCH v2 0/3] fw_devlink overlapping cycles fix

[Saravana Kannan]

This is mainly a bug fix with a additional logging improvement. Lots of
details in Patch 2/3.

Greg,

Can you please pull these into 6.8-rcX after Xu gives the Tested-by?

Xu,

Can you please test this whole series and give your Tested-by if it
fixes your issue? Also, from now on, to debug cycles you just need to
search for debug level logs with "cycle:" in the message and convert
them all to info level logs.

v1 -> v2:
Patch 3: Tweaked the log messages and the commit text.

Saravana Kannan (3):
  driver core: Fix device_link_flag_is_sync_state_only()
  driver core: fw_devlink: Improve detection of overlapping cycles
  driver core: fw_devlink: Improve logs for cycle detection

 drivers/base/core.c | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

-- 
2.43.0.594.gd9cf4e227d-goog

[PATCH v2 1/3] driver core: Fix device_link_flag_is_sync_state_only()

[Saravana Kannan]

device_link_flag_is_sync_state_only() correctly returns true on the flags
of an existing device link that only implements sync_state() functionality.
However, it incorrectly and confusingly returns false if it's called with
DL_FLAG_SYNC_STATE_ONLY.

This bug doesn't manifest in any of the existing calls to this function,
but fix this confusing behavior to avoid future bugs.

Fixes: 67cad5c67019 ("driver core: fw_devlink: Add DL_FLAG_CYCLE support to device links")
Signed-off-by: Saravana Kannan <saravanak@google.com>
---
 drivers/base/core.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 14d46af40f9a..52215c4c7209 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -284,10 +284,12 @@ static bool device_is_ancestor(struct device *dev, struct device *target)
 	return false;
 }
 
+#define DL_MARKER_FLAGS		(DL_FLAG_INFERRED | \
+				 DL_FLAG_CYCLE | \
+				 DL_FLAG_MANAGED)
 static inline bool device_link_flag_is_sync_state_only(u32 flags)
 {
-	return (flags & ~(DL_FLAG_INFERRED | DL_FLAG_CYCLE)) ==
-		(DL_FLAG_SYNC_STATE_ONLY | DL_FLAG_MANAGED);
+	return (flags & ~DL_MARKER_FLAGS) == DL_FLAG_SYNC_STATE_ONLY;
 }
 
 /**
-- 
2.43.0.594.gd9cf4e227d-goog

[PATCH v2 2/3] driver core: fw_devlink: Improve detection of overlapping cycles

[Saravana Kannan]

fw_devlink can detect most overlapping/intersecting cycles. However it was
missing a few corner cases because of an incorrect optimization logic that
tries to avoid repeating cycle detection for devices that are already
marked as part of a cycle.

Here's an example provided by Xu Yang (edited for clarity):

                    usb
                  +-----+
   tcpc           |     |
  +-----+         |  +--|
  |     |----------->|EP|
  |--+  |         |  +--|
  |EP|<-----------|     |
  |--+  |         |  B  |
  |     |         +-----+
  |  A  |            |
  +-----+            |
     ^     +-----+   |
     |     |     |   |
     +-----|  C  |<--+
           |     |
           +-----+
           usb-phy

Node A (tcpc) will be populated as device 1-0050.
Node B (usb) will be populated as device 38100000.usb.
Node C (usb-phy) will be populated as device 381f0040.usb-phy.

The description below uses the notation:
consumer --> supplier
child ==> parent

1. Node C is populated as device C. No cycles detected because cycle
   detection is only run when a fwnode link is converted to a device link.

2. Node B is populated as device B. As we convert B --> C into a device
   link we run cycle detection and find and mark the device link/fwnode
   link cycle:
   C--> A --> B.EP ==> B --> C

3. Node A is populated as device A. As we convert C --> A into a device
   link, we see it's already part of a cycle (from step 2) and don't run
   cycle detection. Thus we miss detecting the cycle:
   A --> B.EP ==> B --> A.EP ==> A

Looking at it another way, A depends on B in one way:
A --> B.EP ==> B

But B depends on A in two ways and we only detect the first:
B --> C --> A
B --> A.EP ==> A

To detect both of these, we remove the incorrect optimization attempt in
step 3 and run cycle detection even if the fwnode link from which the
device link is being created has already been marked as part of a cycle.

Reported-by: Xu Yang <xu.yang_2@nxp.com>
Closes: https://lore.kernel.org/lkml/DU2PR04MB8822693748725F85DC0CB86C8C792@DU2PR04MB8822.eurprd04.prod.outlook.com/
Fixes: 3fb16866b51d ("driver core: fw_devlink: Make cycle detection more robust")
Signed-off-by: Saravana Kannan <saravanak@google.com>
---
 drivers/base/core.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 52215c4c7209..e3d666461835 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -2060,9 +2060,14 @@ static int fw_devlink_create_devlink(struct device *con,
 
 	/*
 	 * SYNC_STATE_ONLY device links don't block probing and supports cycles.
-	 * So cycle detection isn't necessary and shouldn't be done.
+	 * So, one might expect that cycle detection isn't necessary for them.
+	 * However, if the device link was marked as SYNC_STATE_ONLY because
+	 * it's part of a cycle, then we still need to do cycle detection. This
+	 * is because the consumer and supplier might be part of multiple cycles
+	 * and we need to detect all those cycles.
 	 */
-	if (!(flags & DL_FLAG_SYNC_STATE_ONLY)) {
+	if (!device_link_flag_is_sync_state_only(flags) ||
+	    flags & DL_FLAG_CYCLE) {
 		device_links_write_lock();
 		if (__fw_devlink_relax_cycles(con, sup_handle)) {
 			__fwnode_link_cycle(link);
-- 
2.43.0.594.gd9cf4e227d-goog

[PATCH v2 3/3] driver core: fw_devlink: Improve logs for cycle detection

[Saravana Kannan]

The links in a cycle are not all logged in a consistent manner or not
logged at all. Make them consistent by adding a "cycle:" string and log all
the link in the cycles (even the child ==> parent dependency) so that it's
easier to debug cycle detection code. Also, mark the start and end of a
cycle so it's easy to tell when multiple cycles are logged back to back.

Signed-off-by: Saravana Kannan <saravanak@google.com>
---
 drivers/base/core.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index e3d666461835..9828da9b933c 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -125,7 +125,7 @@ static void __fwnode_link_del(struct fwnode_link *link)
  */
 static void __fwnode_link_cycle(struct fwnode_link *link)
 {
-	pr_debug("%pfwf: Relaxing link with %pfwf\n",
+	pr_debug("%pfwf: cycle: depends on %pfwf\n",
 		 link->consumer, link->supplier);
 	link->flags |= FWLINK_FLAG_CYCLE;
 }
@@ -1945,6 +1945,7 @@ static bool __fw_devlink_relax_cycles(struct device *con,
 
 	/* Termination condition. */
 	if (sup_dev == con) {
+		pr_debug("----- cycle: start -----\n");
 		ret = true;
 		goto out;
 	}
@@ -1976,8 +1977,11 @@ static bool __fw_devlink_relax_cycles(struct device *con,
 	else
 		par_dev = fwnode_get_next_parent_dev(sup_handle);
 
-	if (par_dev && __fw_devlink_relax_cycles(con, par_dev->fwnode))
+	if (par_dev && __fw_devlink_relax_cycles(con, par_dev->fwnode)) {
+		pr_debug("%pfwf: cycle: child of %pfwf\n", sup_handle,
+			 par_dev->fwnode);
 		ret = true;
+	}
 
 	if (!sup_dev)
 		goto out;
@@ -1993,6 +1997,8 @@ static bool __fw_devlink_relax_cycles(struct device *con,
 
 		if (__fw_devlink_relax_cycles(con,
 					      dev_link->supplier->fwnode)) {
+			pr_debug("%pfwf: cycle: depends on %pfwf\n", sup_handle,
+				 dev_link->supplier->fwnode);
 			fw_devlink_relax_link(dev_link);
 			dev_link->flags |= DL_FLAG_CYCLE;
 			ret = true;
@@ -2072,6 +2078,7 @@ static int fw_devlink_create_devlink(struct device *con,
 		if (__fw_devlink_relax_cycles(con, sup_handle)) {
 			__fwnode_link_cycle(link);
 			flags = fw_devlink_get_flags(link->flags);
+			pr_debug("----- cycle: end -----\n");
 			dev_info(con, "Fixed dependency cycle(s) with %pfwf\n",
 				 sup_handle);
 		}
-- 
2.43.0.594.gd9cf4e227d-goog

RE: [EXT] [PATCH v2 0/3] fw_devlink overlapping cycles fix

[Xu Yang]

Hi Saravana,

> Subject: [EXT] [PATCH v2 0/3] fw_devlink overlapping cycles fix
>
> This is mainly a bug fix with a additional logging improvement. Lots of
> details in Patch 2/3.
> 
> Greg,
> 
> Can you please pull these into 6.8-rcX after Xu gives the Tested-by?
> 
> Xu,
> 
> Can you please test this whole series and give your Tested-by if it
> fixes your issue? Also, from now on, to debug cycles you just need to
> search for debug level logs with "cycle:" in the message and convert
> them all to info level logs.

Okay. I see.

> 
> v1 -> v2:
> Patch 3: Tweaked the log messages and the commit text.
> 
> Saravana Kannan (3):
>   driver core: Fix device_link_flag_is_sync_state_only()
>   driver core: fw_devlink: Improve detection of overlapping cycles
>   driver core: fw_devlink: Improve logs for cycle detection

For this series:

Tested-by: Xu Yang <xu.yang_2@nxp.com>

Thanks,
Xu Yang

Re: [PATCH v2 2/3] driver core: fw_devlink: Improve detection of overlapping cycles

[Geert Uytterhoeven]

Hi Saravana,

On Fri, Feb 2, 2024 at 10:57 AM Saravana Kannan <saravanak@google.com> wrote:
> fw_devlink can detect most overlapping/intersecting cycles. However it was
> missing a few corner cases because of an incorrect optimization logic that
> tries to avoid repeating cycle detection for devices that are already
> marked as part of a cycle.

Nice (I assume it's due to this patch ;-), with v6.8-rc5 I see much fewer
dependency cycle messages.

E.g. on Salvator-XS:

-platform fea80000.csi2: Fixed dependency cycle(s) with
/soc/video@e6ef7000/ports/port@1/endpoint@0
-platform fea80000.csi2: Fixed dependency cycle(s) with
/soc/video@e6ef6000/ports/port@1/endpoint@0
-platform fea80000.csi2: Fixed dependency cycle(s) with
/soc/video@e6ef5000/ports/port@1/endpoint@0
-platform fea80000.csi2: Fixed dependency cycle(s) with
/soc/video@e6ef4000/ports/port@1/endpoint@0
-platform fea80000.csi2: Fixed dependency cycle(s) with
/soc/video@e6ef3000/ports/port@1/endpoint@0
-platform fea80000.csi2: Fixed dependency cycle(s) with
/soc/video@e6ef2000/ports/port@1/endpoint@0
-platform fea80000.csi2: Fixed dependency cycle(s) with
/soc/video@e6ef1000/ports/port@1/endpoint@0
-platform fea80000.csi2: Fixed dependency cycle(s) with
/soc/video@e6ef0000/ports/port@1/endpoint@0
-platform feaa0000.csi2: Fixed dependency cycle(s) with
/soc/video@e6ef3000/ports/port@1/endpoint@2
-platform feaa0000.csi2: Fixed dependency cycle(s) with
/soc/video@e6ef2000/ports/port@1/endpoint@2
-platform feaa0000.csi2: Fixed dependency cycle(s) with
/soc/video@e6ef1000/ports/port@1/endpoint@2
-platform feaa0000.csi2: Fixed dependency cycle(s) with
/soc/video@e6ef0000/ports/port@1/endpoint@2
-platform fead0000.hdmi: Fixed dependency cycle(s) with
/soc/sound@ec500000/ports/port@1/endpoint
-platform feae0000.hdmi: Fixed dependency cycle(s) with
/soc/sound@ec500000/ports/port@2/endpoint
-platform feb00000.display: Fixed dependency cycle(s) with
/soc/hdmi@feae0000/ports/port@0/endpoint
-platform feb00000.display: Fixed dependency cycle(s) with
/soc/hdmi@fead0000/ports/port@0/endpoint
-platform hdmi0-out: Fixed dependency cycle(s) with
/soc/hdmi@fead0000/ports/port@1/endpoint
-platform hdmi1-out: Fixed dependency cycle(s) with
/soc/hdmi@feae0000/ports/port@1/endpoint
-platform vga-encoder: Fixed dependency cycle(s) with /vga/port/endpoint
-platform vga-encoder: Fixed dependency cycle(s) with
/soc/display@feb00000/ports/port@0/endpoint

-i2c 2-0010: Fixed dependency cycle(s) with
/soc/sound@ec500000/ports/port@0/endpoint
-i2c 2-0010: Fixed dependency cycle(s) with /soc/sound@ec500000

-i2c 4-0070: Fixed dependency cycle(s) with
/soc/csi2@fea80000/ports/port@0/endpoint
-i2c 4-0070: Fixed dependency cycle(s) with
/soc/csi2@feaa0000/ports/port@0/endpoint
-i2c 4-0070: Fixed dependency cycle(s) with /hdmi-in/port/endpoint
-i2c 4-0070: Fixed dependency cycle(s) with /cvbs-in/port/endpoint

FTR, the only remaining ones (on Salvator-XS) are:

    platform soc: Fixed dependency cycle(s) with
/soc/interrupt-controller@f1010000
    platform e6060000.pinctrl: Fixed dependency cycle(s) with
/soc/pinctrl@e6060000/scif_clk

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 2/3] driver core: fw_devlink: Improve detection of overlapping cycles

[Saravana Kannan]

On Mon, Feb 19, 2024 at 2:59 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Saravana,
>
> On Fri, Feb 2, 2024 at 10:57 AM Saravana Kannan <saravanak@google.com> wrote:
> > fw_devlink can detect most overlapping/intersecting cycles. However it was
> > missing a few corner cases because of an incorrect optimization logic that
> > tries to avoid repeating cycle detection for devices that are already
> > marked as part of a cycle.
>
> Nice (I assume it's due to this patch ;-), with v6.8-rc5 I see much fewer
> dependency cycle messages.

Thanks!

It's not due to this patch. It's due to this other series:
https://lore.kernel.org/lkml/20240207011803.2637531-1-saravanak@google.com/

I forget why the cycle warning doesn't show up between fea80000.csi2
and video@e6ef7000, but I had the same improvement on my test device
and there was a valid reason why it doesn't show up. So, an
unintentional, but non-buggy benefit of that series.

-Saravana

>
> E.g. on Salvator-XS:
>
> -platform fea80000.csi2: Fixed dependency cycle(s) with
> /soc/video@e6ef7000/ports/port@1/endpoint@0
> -platform fea80000.csi2: Fixed dependency cycle(s) with
> /soc/video@e6ef6000/ports/port@1/endpoint@0
> -platform fea80000.csi2: Fixed dependency cycle(s) with
> /soc/video@e6ef5000/ports/port@1/endpoint@0
> -platform fea80000.csi2: Fixed dependency cycle(s) with
> /soc/video@e6ef4000/ports/port@1/endpoint@0
> -platform fea80000.csi2: Fixed dependency cycle(s) with
> /soc/video@e6ef3000/ports/port@1/endpoint@0
> -platform fea80000.csi2: Fixed dependency cycle(s) with
> /soc/video@e6ef2000/ports/port@1/endpoint@0
> -platform fea80000.csi2: Fixed dependency cycle(s) with
> /soc/video@e6ef1000/ports/port@1/endpoint@0
> -platform fea80000.csi2: Fixed dependency cycle(s) with
> /soc/video@e6ef0000/ports/port@1/endpoint@0
> -platform feaa0000.csi2: Fixed dependency cycle(s) with
> /soc/video@e6ef3000/ports/port@1/endpoint@2
> -platform feaa0000.csi2: Fixed dependency cycle(s) with
> /soc/video@e6ef2000/ports/port@1/endpoint@2
> -platform feaa0000.csi2: Fixed dependency cycle(s) with
> /soc/video@e6ef1000/ports/port@1/endpoint@2
> -platform feaa0000.csi2: Fixed dependency cycle(s) with
> /soc/video@e6ef0000/ports/port@1/endpoint@2
> -platform fead0000.hdmi: Fixed dependency cycle(s) with
> /soc/sound@ec500000/ports/port@1/endpoint
> -platform feae0000.hdmi: Fixed dependency cycle(s) with
> /soc/sound@ec500000/ports/port@2/endpoint
> -platform feb00000.display: Fixed dependency cycle(s) with
> /soc/hdmi@feae0000/ports/port@0/endpoint
> -platform feb00000.display: Fixed dependency cycle(s) with
> /soc/hdmi@fead0000/ports/port@0/endpoint
> -platform hdmi0-out: Fixed dependency cycle(s) with
> /soc/hdmi@fead0000/ports/port@1/endpoint
> -platform hdmi1-out: Fixed dependency cycle(s) with
> /soc/hdmi@feae0000/ports/port@1/endpoint
> -platform vga-encoder: Fixed dependency cycle(s) with /vga/port/endpoint
> -platform vga-encoder: Fixed dependency cycle(s) with
> /soc/display@feb00000/ports/port@0/endpoint
>
> -i2c 2-0010: Fixed dependency cycle(s) with
> /soc/sound@ec500000/ports/port@0/endpoint
> -i2c 2-0010: Fixed dependency cycle(s) with /soc/sound@ec500000
>
> -i2c 4-0070: Fixed dependency cycle(s) with
> /soc/csi2@fea80000/ports/port@0/endpoint
> -i2c 4-0070: Fixed dependency cycle(s) with
> /soc/csi2@feaa0000/ports/port@0/endpoint
> -i2c 4-0070: Fixed dependency cycle(s) with /hdmi-in/port/endpoint
> -i2c 4-0070: Fixed dependency cycle(s) with /cvbs-in/port/endpoint
>
> FTR, the only remaining ones (on Salvator-XS) are:
>
>     platform soc: Fixed dependency cycle(s) with
> /soc/interrupt-controller@f1010000
>     platform e6060000.pinctrl: Fixed dependency cycle(s) with
> /soc/pinctrl@e6060000/scif_clk
>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds