* [PATCH v2 1/3] driver core: Fix device_link_flag_is_sync_state_only()
2024-02-02 9:56 [PATCH v2 0/3] fw_devlink overlapping cycles fix Saravana Kannan
@ 2024-02-02 9:56 ` Saravana Kannan
2024-02-02 9:56 ` [PATCH v2 2/3] driver core: fw_devlink: Improve detection of overlapping cycles Saravana Kannan
` (2 subsequent siblings)
3 siblings, 0 replies; 7+ messages in thread
From: Saravana Kannan @ 2024-02-02 9:56 UTC (permalink / raw)
To: Greg Kroah-Hartman, Rafael J. Wysocki, Saravana Kannan
Cc: Xu Yang, kernel-team, linux-kernel
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
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH v2 2/3] driver core: fw_devlink: Improve detection of overlapping cycles
2024-02-02 9:56 [PATCH v2 0/3] fw_devlink overlapping cycles fix Saravana Kannan
2024-02-02 9:56 ` [PATCH v2 1/3] driver core: Fix device_link_flag_is_sync_state_only() Saravana Kannan
@ 2024-02-02 9:56 ` Saravana Kannan
2024-02-19 10:59 ` Geert Uytterhoeven
2024-02-02 9:56 ` [PATCH v2 3/3] driver core: fw_devlink: Improve logs for cycle detection Saravana Kannan
2024-02-02 10:56 ` [EXT] [PATCH v2 0/3] fw_devlink overlapping cycles fix Xu Yang
3 siblings, 1 reply; 7+ messages in thread
From: Saravana Kannan @ 2024-02-02 9:56 UTC (permalink / raw)
To: Greg Kroah-Hartman, Rafael J. Wysocki, Saravana Kannan
Cc: Xu Yang, kernel-team, linux-kernel
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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2 2/3] driver core: fw_devlink: Improve detection of overlapping cycles
2024-02-02 9:56 ` [PATCH v2 2/3] driver core: fw_devlink: Improve detection of overlapping cycles Saravana Kannan
@ 2024-02-19 10:59 ` Geert Uytterhoeven
2024-02-20 6:55 ` Saravana Kannan
0 siblings, 1 reply; 7+ messages in thread
From: Geert Uytterhoeven @ 2024-02-19 10:59 UTC (permalink / raw)
To: Saravana Kannan
Cc: Greg Kroah-Hartman, Rafael J. Wysocki, Xu Yang, kernel-team,
linux-kernel, Linux-Renesas
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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2 2/3] driver core: fw_devlink: Improve detection of overlapping cycles
2024-02-19 10:59 ` Geert Uytterhoeven
@ 2024-02-20 6:55 ` Saravana Kannan
0 siblings, 0 replies; 7+ messages in thread
From: Saravana Kannan @ 2024-02-20 6:55 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Greg Kroah-Hartman, Rafael J. Wysocki, Xu Yang, kernel-team,
linux-kernel, Linux-Renesas
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 3/3] driver core: fw_devlink: Improve logs for cycle detection
2024-02-02 9:56 [PATCH v2 0/3] fw_devlink overlapping cycles fix Saravana Kannan
2024-02-02 9:56 ` [PATCH v2 1/3] driver core: Fix device_link_flag_is_sync_state_only() Saravana Kannan
2024-02-02 9:56 ` [PATCH v2 2/3] driver core: fw_devlink: Improve detection of overlapping cycles Saravana Kannan
@ 2024-02-02 9:56 ` Saravana Kannan
2024-02-02 10:56 ` [EXT] [PATCH v2 0/3] fw_devlink overlapping cycles fix Xu Yang
3 siblings, 0 replies; 7+ messages in thread
From: Saravana Kannan @ 2024-02-02 9:56 UTC (permalink / raw)
To: Greg Kroah-Hartman, Rafael J. Wysocki, Saravana Kannan
Cc: Xu Yang, kernel-team, linux-kernel
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
^ permalink raw reply [flat|nested] 7+ messages in thread* RE: [EXT] [PATCH v2 0/3] fw_devlink overlapping cycles fix
2024-02-02 9:56 [PATCH v2 0/3] fw_devlink overlapping cycles fix Saravana Kannan
` (2 preceding siblings ...)
2024-02-02 9:56 ` [PATCH v2 3/3] driver core: fw_devlink: Improve logs for cycle detection Saravana Kannan
@ 2024-02-02 10:56 ` Xu Yang
3 siblings, 0 replies; 7+ messages in thread
From: Xu Yang @ 2024-02-02 10:56 UTC (permalink / raw)
To: Saravana Kannan, Greg Kroah-Hartman, Rafael J. Wysocki
Cc: kernel-team, linux-kernel
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
^ permalink raw reply [flat|nested] 7+ messages in thread