Improve remote-endpoint parsing

PATCH Applied Saravana Kannan 5 participants 21 messages Feb 7, 2024 – Mar 1, 2024

Subsystems (2)

Stable branch Device tree

lore

Share & tools

lore

Saravana Kannan

author

2024-02-07 01:18 UTC #0 lore raw

v2 cover Improve remote-endpoint parsing

Some changes to do a more accurate parsing of remote-endpoints. Making fw_devlink a tiny bit more efficient and the debug logs a bit cleaner when trying to debug fw_devlink.

Rob,

Can we get this into 6.8-rcX please?

Thanks,
Saravana

v1->v2:
- Switched from fwnode_graph_get_port_parent() to of_graph_get_port_parent()
- Added Patch 3

Saravana Kannan (3):
of: property: Improve finding the consumer of a remote-endpoint
property
of: property: Improve finding the supplier of a remote-endpoint
property
of: property: Add in-ports/out-ports support to
of_graph_get_port_parent()

 drivers/of/property.c | 63 +++++++++++++++++--------------------------
 1 file changed, 24 insertions(+), 39 deletions(-)

Signature · 1 line

2.43.0.594.gd9cf4e227d-goog

Saravana Kannan

author

2024-02-07 01:18 UTC #1 lore raw

PATCH v2 1/3 of: property: Improve finding the consumer of a remote-endpoint property

We have a more accurate function to find the right consumer of a remote-endpoint property instead of searching for a parent with compatible string property. So, use that instead. While at it, make the code to find the consumer a bit more flexible and based on the property being parsed.

Fixes f7514a663016 ("of: property: fw_devlink: Add support for remote-endpoint")

Signed-off-by Saravana Kannan <[email protected]>

---
 drivers/of/property.c | 47 +++++++++----------------------------------
 1 file changed, 10 insertions(+), 37 deletions(-)

drivers/of/property.c +10 ~~-37~~

index 641a40cf5cf3..da70aaa62ca3 100644
@@ -1063,36 +1063,6 @@ of_fwnode_device_get_match_data(const struct fwnode_handle *fwnode,
 	return of_device_get_match_data(dev);
 }
 
-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_property_present(np, "compatible"))
-			break;
-
-		np = of_get_next_parent(np);
-	}
-
-	return np;
-}
-
-static struct device_node *of_get_compat_node_parent(struct device_node *np)
-{
-	struct device_node *parent, *node;
-
-	parent = of_get_parent(np);
-	node = of_get_compat_node(parent);
-	of_node_put(parent);
-
-	return node;
-}
-
 static void of_link_to_phandle(struct device_node *con_np,
 			      struct device_node *sup_np)
 {

@@ -1222,10 +1192,10 @@ 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
+ * @get_con_dev: If the consumer node containing the property is never converted
+ *		 to a struct device, implement this ops so fw_devlink can use it
+ *		 to find the true consumer.
  * @optional: Describes whether a supplier is mandatory or not
- * @node_not_dev: The consumer node containing the property is never converted
- *		  to a struct device. Instead, parse ancestor nodes for the
- *		  compatible property to find a node corresponding to a device.
  *
  * Returns:
  * parse_prop() return values are

@@ -1236,8 +1206,8 @@ static struct device_node *parse_##fname(struct device_node *np,	     \
 struct supplier_bindings {
 	struct device_node *(*parse_prop)(struct device_node *np,
 					  const char *prop_name, int index);
+	struct device_node *(*get_con_dev)(struct device_node *np);
 	bool optional;
-	bool node_not_dev;
 };
 
 DEFINE_SIMPLE_PROP(clocks, "clocks", "#clock-cells")

@@ -1352,7 +1322,10 @@ 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_remote_endpoint,
+		.get_con_dev = of_graph_get_port_parent,
+	},
 	{ .parse_prop = parse_pwms, },
 	{ .parse_prop = parse_resets, },
 	{ .parse_prop = parse_leds, },

@@ -1403,8 +1376,8 @@ 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_parent(con_np)
+			con_dev_np = s->get_con_dev
+					? s->get_con_dev(con_np)
 					: of_node_get(con_np);
 			matched = true;
 			i++;

Signature · 1 line

2.43.0.594.gd9cf4e227d-goog

Saravana Kannan

author

2024-02-07 01:18 UTC #2 lore raw

PATCH v2 2/3 of: property: Improve finding the supplier of a remote-endpoint property

In reply to Saravana Kannan

After commit 4a032827daa8 ("of: property: Simplify of_link_to_phandle()"), remote-endpoint properties created a fwnode link from the consumer device to the supplier endpoint. This is a tiny bit inefficient (not buggy) when trying to create device links or detecting cycles. So, improve this the same way we improved finding the consumer of a remote-endpoint property.

Fixes 4a032827daa8 ("of: property: Simplify of_link_to_phandle()")

Signed-off-by Saravana Kannan <[email protected]>

---
 drivers/of/property.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

drivers/of/property.c +11 -1

index da70aaa62ca3..7bb2d8e290de 100644
@@ -1232,7 +1232,6 @@ 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_SIMPLE_PROP(pwms, "pwms", "#pwm-cells")
 DEFINE_SIMPLE_PROP(resets, "resets", "#reset-cells")
 DEFINE_SIMPLE_PROP(leds, "leds", NULL)

@@ -1298,6 +1297,17 @@ static struct device_node *parse_interrupts(struct device_node *np,
 	return of_irq_parse_one(np, index, &sup_args) ? NULL : sup_args.np;
 }
 
+static struct device_node *parse_remote_endpoint(struct device_node *np,
+						 const char *prop_name,
+						 int index)
+{
+	/* Return NULL for index > 0 to signify end of remote-endpoints. */
+	if (!index || strcmp(prop_name, "remote-endpoint"))
+		return NULL;
+
+	return of_graph_get_remote_port_parent(np);
+}
+
 static const struct supplier_bindings of_supplier_bindings[] = {
 	{ .parse_prop = parse_clocks, },
 	{ .parse_prop = parse_interconnects, },

Signature · 1 line

2.43.0.594.gd9cf4e227d-goog

Luca Ceresoli

2024-02-23 16:18 UTC #3 lore raw

Re: [PATCH v2 2/3] of: property: Improve finding the supplier of a remote-endpoint property

Hello Saravana,

[+cc Hervé Codina]

Quoted 8 lines · On Tue, 6 Feb 2024 17:18:01 -0800 Saravana Kannan <[email protected]> wrote:

After commit 4a032827daa8 ("of: property: Simplify of_link_to_phandle()"), remote-endpoint properties created a fwnode link from the consumer device to the supplier endpoint. This is a tiny bit inefficient (not buggy) when trying to create device links or detecting cycles. So, improve this the same way we improved finding the consumer of a remote-endpoint property.
Fixes 4a032827daa8 ("of: property: Simplify of_link_to_phandle()")
Signed-off-by Saravana Kannan <[email protected]>

After rebasing my own branch on v6.8-rc5 from v6.8-rc1 I started getting unexpected warnings during device tree overlay removal. After a somewhat painful bisection I identified this patch as the one that triggers it all.

Quoted 12 lines

---

drivers/of/property.c +0 -1

@@ -1232,7 +1232,6 @@ 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_SIMPLE_PROP(pwms, "pwms", "#pwm-cells")
 DEFINE_SIMPLE_PROP(resets, "resets", "#reset-cells")
 DEFINE_SIMPLE_PROP(leds, "leds", NULL)

drivers/of/property.c static struct device_node *parse_interrupts(struct device_node *np, · line 1305

Quoted 9 lines

drivers/of/property.c +6 -0

@@ -1298,6 +1297,17 @@ static struct device_node *parse_interrupts(struct device_node *np,
 	return of_irq_parse_one(np, index, &sup_args) ? NULL : sup_args.np;
 }
 
+static struct device_node *parse_remote_endpoint(struct device_node *np,
+						 const char *prop_name,
+						 int index)
+{
+	/* Return NULL for index > 0 to signify end of remote-endpoints. */
+	if (!index || strcmp(prop_name, "remote-endpoint"))

There seem to be a bug here: "!index" should be "index > 0", as the comment suggests. Otherwise NULL is always returned.

I am going to send a quick patch for that, but haven't done so yet because it still won't solve the problem, so I wanted to open the topic here without further delay.

Even with the 'index > 0' fix I'm still getting pretty much the same:

[   34.836781] ------------[ cut here ]------------
[   34.841401] WARNING: CPU: 2 PID: 204 at drivers/base/devres.c:1064 devm_kfree+0x8c/0xfc
...
[   35.024751] Call trace:
[   35.027199]  devm_kfree+0x8c/0xfc
[   35.030520]  devm_drm_panel_bridge_release+0x54/0x64 [drm_kms_helper]
[   35.036990]  devres_release_group+0xe0/0x164
[   35.041264]  i2c_device_remove+0x38/0x9c
[   35.045196]  device_remove+0x4c/0x80
[   35.048774]  device_release_driver_internal+0x1d4/0x230
[   35.054003]  device_release_driver+0x18/0x24
[   35.058279]  bus_remove_device+0xcc/0x10c
[   35.062292]  device_del+0x15c/0x41c
[   35.065786]  device_unregister+0x18/0x34
[   35.069714]  i2c_unregister_device+0x54/0x88
[   35.073988]  of_i2c_notify+0x98/0x224
[   35.077656]  blocking_notifier_call_chain+0x6c/0xa0
[   35.082543]  __of_changeset_entry_notify+0x100/0x16c
[   35.087515]  __of_changeset_revert_notify+0x44/0x78
[   35.092398]  of_overlay_remove+0x114/0x1c4

...

By comparing the two versions I found that before removing the overlay:

* in the "working" case (with this patch reverted) I have:

   # ls /sys/class/devlink/ | grep 002c
   platform:hpbr--i2c:13-002c
   platform:panel-dsi-lvds--i2c:13-002c
   platform:regulator-sys-1v8--i2c:13-002c
   regulator:regulator.31--i2c:13-002c
   #

* in the "broken" case (v6.8-rc5 + s/!index/index > 0/ as mentioned):

   # ls /sys/class/devlink/ | grep 002c
   platform:hpbr--i2c:13-002c
   platform:regulator-sys-1v8--i2c:13-002c
   regulator:regulator.30--i2c:13-002c
   #

So in the latter case the panel-dsi-lvds--i2c:13-002c link is missing. I think it gets created but later on removed. Here's a snippet of the kernel log when that happens:

[    9.578279] ----- cycle: start -----
[    9.578283] /soc@0/bus@30800000/i2c@30ad0000/i2cmux@70/i2c@3/dsi-lvds-bridge@2c: cycle: depends on /panel-dsi-lvds
[    9.578308] /panel-dsi-lvds: cycle: depends on /soc@0/bus@30800000/i2c@30ad0000/i2cmux@70/i2c@3/dsi-lvds-bridge@2c
[    9.578329] ----- cycle: end -----
[    9.578334] platform panel-dsi-lvds: Fixed dependency cycle(s) with /soc@0/bus@30800000/i2c@30ad0000/i2cmux@70/i2c@3/dsi-lvds-bridge@2c
...
[    9.590620] /panel-dsi-lvds Dropping the fwnode link to /soc@0/bus@30800000/i2c@30ad0000/i2cmux@70/i2c@3/dsi-lvds-bridge@2c
...
[    9.597280] ----- cycle: start -----
[    9.597283] /panel-dsi-lvds: cycle: depends on /soc@0/bus@30800000/i2c@30ad0000/i2cmux@70/i2c@3/dsi-lvds-bridge@2c
[    9.602781] /soc@0/bus@30800000/i2c@30ad0000/i2cmux@70/i2c@3/dsi-lvds-bridge@2c: cycle: depends on /panel-dsi-lvds
[    9.607581] ----- cycle: end -----
[    9.607585] i2c 13-002c: Fixed dependency cycle(s) with /panel-dsi-lvds
[    9.614217] device: 'platform:panel-dsi-lvds--i2c:13-002c': device_add
...
[    9.614277] /soc@0/bus@30800000/i2c@30ad0000/i2cmux@70/i2c@3/dsi-lvds-bridge@2c Dropping the fwnode link to /panel-dsi-lvds
[    9.614369] /soc@0/bus@30800000/i2c@30ad0000/i2cmux@70/i2c@3/dsi-lvds-bridge@2c Dropping the fwnode link to /regulator-dock-sys-1v8
...
[    9.739840] panel-simple panel-dsi-lvds: Dropping the link to 13-002c
[    9.739846] device: 'i2c:13-002c--platform:panel-dsi-lvds': device_unregister
[   10.247037] sn65dsi83 13-002c: Dropping the link to panel-dsi-lvds
[   10.247049] device: 'platform:panel-dsi-lvds--i2c:13-002c': device_unregister

And here's the relevant portion of my device tree overlay:

--------------------8<--------------------

/dts-v1/;
/plugin/;

&{/}

{
        panel_dsi_lvds: panel-dsi-lvds {
                compatible = "auo,g133han01.1";

                ports {
                        #address-cells = <1>;
                        #size-cells = <0>;
                        port@0{
                                reg = <0>;
                                dual-lvds-odd-pixels;
                                panel_dsi_lvds_in0: endpoint {
                                        remote-endpoint = <&sn65dsi84_out0>;
                                };
                        };

                        port@1{
                                reg = <1>;
                                dual-lvds-even-pixels;
                                panel_dsi_lvds_in1: endpoint {
                                        remote-endpoint = <&sn65dsi84_out1>;
                                };
                        };
                };
        };
};

&i2c5_ch3 {
        dsi-lvds-bridge@2c {
                compatible = "ti,sn65dsi84";
                reg = <0x2c>;
                vcc-supply = <&reg_sys_1v8>;

                ports {
                        #address-cells = <1>;
                        #size-cells = <0>;

                        port@0 {
                                reg = <0>;

                                sn65dsi84_from_bridge: endpoint {
                                        remote-endpoint = <&hpbr_source>;
                                        data-lanes = <1 2 3 4>;
                                };
                        };
                        port@2 {
                                reg = <2>;

                                sn65dsi84_out0: endpoint {
                                        remote-endpoint = <&panel_dsi_lvds_in0>;
                                };
                        };
                        port@3 {
                                reg = <3>;

                                sn65dsi84_out1: endpoint {
                                        remote-endpoint = <&panel_dsi_lvds_in1>;
                                };
                        };
                };
        };
};

--------------------8<--------------------

That's all I could get at this point. Any clues for further investigation?

Best regards,
Luca

Signature · 3 lines

Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Saravana Kannan

author

2024-02-24 01:36 UTC #4 lore raw

Re: [PATCH v2 2/3] of: property: Improve finding the supplier of a remote-endpoint property

Quoted 14 lines · Luca Ceresoli

Hello Saravana,
[+cc Hervé Codina]
Quoted 6 lines · On Tue, 6 Feb 2024 17:18:01 -0800 Saravana Kannan <[email protected]> wrote:
After commit 4a032827daa8 ("of: property: Simplify of_link_to_phandle()"), remote-endpoint properties created a fwnode link from the consumer device to the supplier endpoint. This is a tiny bit inefficient (not buggy) when trying to create device links or detecting cycles. So, improve this the same way we improved finding the consumer of a remote-endpoint property.

Quoted 7 lines

Quoted 2 lines
Fixes 4a032827daa8 ("of: property: Simplify of_link_to_phandle()")
Signed-off-by Saravana Kannan <[email protected]>
After rebasing my own branch on v6.8-rc5 from v6.8-rc1 I started getting unexpected warnings during device tree overlay removal. After a somewhat painful bisection I identified this patch as the one that triggers it all.

Thanks for the report.

Quoted 16 lines

---

drivers/of/property.c +0 -1

@@ -1232,7 +1232,6 @@ 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_SIMPLE_PROP(pwms, "pwms", "#pwm-cells")
 DEFINE_SIMPLE_PROP(resets, "resets", "#reset-cells")
 DEFINE_SIMPLE_PROP(leds, "leds", NULL)

@@ -1298,6 +1297,17 @@ static struct device_node *parse_interrupts(struct device_node *np,
      return of_irq_parse_one(np, index, &sup_args) ? NULL : sup_args.np;
 }

Quoted 9 lines

drivers/of/property.c static struct device_node *parse_interrupts(struct device_node *np, · line 1305

Quoted 6 lines

drivers/of/property.c +6 -0

+static struct device_node *parse_remote_endpoint(struct device_node *np,
+                                              const char *prop_name,
+                                              int index)
+{
+     /* Return NULL for index > 0 to signify end of remote-endpoints. */
+     if (!index || strcmp(prop_name, "remote-endpoint"))

There seem to be a bug here: "!index" should be "index > 0", as the comment suggests. Otherwise NULL is always returned.

Ah crap, I think you are right. It should have been "index". Not "!index". But I tested this! Sigh. I probably screwed up my testing.

Please send out a Fix for this.

Geert, we got excited too soon. :(

Quoted 5 lines

I am going to send a quick patch for that, but haven't done so yet because it still won't solve the problem, so I wanted to open the topic here without further delay.
Even with the 'index > 0' fix I'm still getting pretty much the same:

This part is confusing though. If I read your DT correctly, there's a cycle between platform:panel-dsi-lvds and i2c:13-002c. And fw_devlink should not be enforcing any ordering between those devices ever.

I'm surprised that in your "working" case, fw_devlink didn't detect any cycle. It should have. If there's any debugging to do, that's the one we need to debug.

Quoted 23 lines

[   34.836781] ------------[ cut here ]------------
[   34.841401] WARNING: CPU: 2 PID: 204 at drivers/base/devres.c:1064 devm_kfree+0x8c/0xfc
...
[   35.024751] Call trace:
[   35.027199]  devm_kfree+0x8c/0xfc
[   35.030520]  devm_drm_panel_bridge_release+0x54/0x64 [drm_kms_helper]
[   35.036990]  devres_release_group+0xe0/0x164
[   35.041264]  i2c_device_remove+0x38/0x9c
[   35.045196]  device_remove+0x4c/0x80
[   35.048774]  device_release_driver_internal+0x1d4/0x230
[   35.054003]  device_release_driver+0x18/0x24
[   35.058279]  bus_remove_device+0xcc/0x10c
[   35.062292]  device_del+0x15c/0x41c
[   35.065786]  device_unregister+0x18/0x34
[   35.069714]  i2c_unregister_device+0x54/0x88
[   35.073988]  of_i2c_notify+0x98/0x224
[   35.077656]  blocking_notifier_call_chain+0x6c/0xa0
[   35.082543]  __of_changeset_entry_notify+0x100/0x16c
[   35.087515]  __of_changeset_revert_notify+0x44/0x78
[   35.092398]  of_overlay_remove+0x114/0x1c4

...

Quoted 7 lines

By comparing the two versions I found that before removing the overlay:
* in the "working" case (with this patch reverted) I have:
   # ls /sys/class/devlink/ | grep 002c
   platform:hpbr--i2c:13-002c
   platform:panel-dsi-lvds--i2c:13-002c

Can you check the "status" and "sync_state_only" file in this folder and tell me what it says?

Since these devices have a cyclic dependency between them, it should have been something other than "not tracked" and "sync_state_only" should be "1". But my guess is you'll see "active" and "0".

Quoted 16 lines

   platform:regulator-sys-1v8--i2c:13-002c
   regulator:regulator.31--i2c:13-002c
   #
* in the "broken" case (v6.8-rc5 + s/!index/index > 0/ as mentioned):
   # ls /sys/class/devlink/ | grep 002c
   platform:hpbr--i2c:13-002c
   platform:regulator-sys-1v8--i2c:13-002c
   regulator:regulator.30--i2c:13-002c
   #
So in the latter case the panel-dsi-lvds--i2c:13-002c link is missing. I think it gets created but later on removed. Here's a snippet of the kernel log when that happens:

Quoted 6 lines

[    9.578279] ----- cycle: start -----
[    9.578283] /soc@0/bus@30800000/i2c@30ad0000/i2cmux@70/i2c@3/dsi-lvds-bridge@2c: cycle: depends on /panel-dsi-lvds
[    9.578308] /panel-dsi-lvds: cycle: depends on /soc@0/bus@30800000/i2c@30ad0000/i2cmux@70/i2c@3/dsi-lvds-bridge@2c
[    9.578329] ----- cycle: end -----
[    9.578334] platform panel-dsi-lvds: Fixed dependency cycle(s) with /soc@0/bus@30800000/i2c@30ad0000/i2cmux@70/i2c@3/dsi-lvds-bridge@2c

...

Somewhere in this area, I'm thinking you'll also see "device:
'i2c:13-002c--platform:panel-dsi-lvds': device_add" do you not? And if you enabled device link logs, you'll see that it was "sync state only" link.

Quoted 6 lines

[    9.590620] /panel-dsi-lvds Dropping the fwnode link to /soc@0/bus@30800000/i2c@30ad0000/i2cmux@70/i2c@3/dsi-lvds-bridge@2c
...
[    9.597280] ----- cycle: start -----
[    9.597283] /panel-dsi-lvds: cycle: depends on /soc@0/bus@30800000/i2c@30ad0000/i2cmux@70/i2c@3/dsi-lvds-bridge@2c
[    9.602781] /soc@0/bus@30800000/i2c@30ad0000/i2cmux@70/i2c@3/dsi-lvds-bridge@2c: cycle: depends on /panel-dsi-lvds
[    9.607581] ----- cycle: end -----

Quoted 8 lines

[    9.607585] i2c 13-002c: Fixed dependency cycle(s) with /panel-dsi-lvds
[    9.614217] device: 'platform:panel-dsi-lvds--i2c:13-002c': device_add
...
[    9.614277] /soc@0/bus@30800000/i2c@30ad0000/i2cmux@70/i2c@3/dsi-lvds-bridge@2c Dropping the fwnode link to /panel-dsi-lvds
[    9.614369] /soc@0/bus@30800000/i2c@30ad0000/i2cmux@70/i2c@3/dsi-lvds-bridge@2c Dropping the fwnode link to /regulator-dock-sys-1v8
...
[    9.739840] panel-simple panel-dsi-lvds: Dropping the link to 13-002c
[    9.739846] device: 'i2c:13-002c--platform:panel-dsi-lvds': device_unregister

Oh yeah, see. The "device_add" I expected earlier is getting removed here.

Quoted 7 lines

[   10.247037] sn65dsi83 13-002c: Dropping the link to panel-dsi-lvds
[   10.247049] device: 'platform:panel-dsi-lvds--i2c:13-002c': device_unregister
And here's the relevant portion of my device tree overlay:
--------------------8<--------------------

I think the eventual fix would be this series + adding a "post-init-providers" property to the device that's supposed to probe first and point it to the device that's supposed to probe next. Do this at the device node level, not the endpoint level.
https://lore.kernel.org/lkml/[email protected]/

-Saravana

Quoted 78 lines

/dts-v1/;
/plugin/;

&{/}

{
        panel_dsi_lvds: panel-dsi-lvds {
                compatible = "auo,g133han01.1";

                ports {
                        #address-cells = <1>;
                        #size-cells = <0>;
                        port@0{
                                reg = <0>;
                                dual-lvds-odd-pixels;
                                panel_dsi_lvds_in0: endpoint {
                                        remote-endpoint = <&sn65dsi84_out0>;
                                };
                        };

                        port@1{
                                reg = <1>;
                                dual-lvds-even-pixels;
                                panel_dsi_lvds_in1: endpoint {
                                        remote-endpoint = <&sn65dsi84_out1>;
                                };
                        };
                };
        };
};

&i2c5_ch3 {
        dsi-lvds-bridge@2c {
                compatible = "ti,sn65dsi84";
                reg = <0x2c>;
                vcc-supply = <&reg_sys_1v8>;

                ports {
                        #address-cells = <1>;
                        #size-cells = <0>;

                        port@0 {
                                reg = <0>;

                                sn65dsi84_from_bridge: endpoint {
                                        remote-endpoint = <&hpbr_source>;
                                        data-lanes = <1 2 3 4>;
                                };
                        };
                        port@2 {
                                reg = <2>;

                                sn65dsi84_out0: endpoint {
                                        remote-endpoint = <&panel_dsi_lvds_in0>;
                                };
                        };
                        port@3 {
                                reg = <3>;

                                sn65dsi84_out1: endpoint {
                                        remote-endpoint = <&panel_dsi_lvds_in1>;
                                };
                        };
                };
        };
};

--------------------8<--------------------

That's all I could get at this point. Any clues for further investigation?

Best regards,
Luca

--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Luca Ceresoli

2024-02-26 11:52 UTC #5 lore raw

[REGRESSION] Re: [PATCH v2 2/3] of: property: Improve finding the supplier of a remote-endpoint property

Hello Saravana,

Quoted 160 lines · On Fri, 23 Feb 2024 17:35:24 -0800 Saravana Kannan <[email protected]> wrote:

Quoted 14 lines · Luca Ceresoli
Hello Saravana,
[+cc Hervé Codina]
Quoted 6 lines · On Tue, 6 Feb 2024 17:18:01 -0800 Saravana Kannan <[email protected]> wrote:
After commit 4a032827daa8 ("of: property: Simplify of_link_to_phandle()"), remote-endpoint properties created a fwnode link from the consumer device to the supplier endpoint. This is a tiny bit inefficient (not buggy) when trying to create device links or detecting cycles. So, improve this the same way we improved finding the consumer of a remote-endpoint property.
Quoted 7 lines
Quoted 2 lines
Fixes 4a032827daa8 ("of: property: Simplify of_link_to_phandle()")
Signed-off-by Saravana Kannan <[email protected]>
After rebasing my own branch on v6.8-rc5 from v6.8-rc1 I started getting unexpected warnings during device tree overlay removal. After a somewhat painful bisection I identified this patch as the one that triggers it all.
Thanks for the report.
Quoted 16 lines
Quoted 16 lines
---
drivers/of/property.c +0 -1
@@ -1232,7 +1232,6 @@ 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_SIMPLE_PROP(pwms, "pwms", "#pwm-cells")
 DEFINE_SIMPLE_PROP(resets, "resets", "#reset-cells")
 DEFINE_SIMPLE_PROP(leds, "leds", NULL)
@@ -1298,6 +1297,17 @@ static struct device_node *parse_interrupts(struct device_node *np,
      return of_irq_parse_one(np, index, &sup_args) ? NULL : sup_args.np;
 }
 
Quoted 9 lines
drivers/of/property.c static struct device_node *parse_interrupts(struct device_node *np, · line 1305
Quoted 6 lines
drivers/of/property.c +6 -0
+static struct device_node *parse_remote_endpoint(struct device_node *np,
+                                              const char *prop_name,
+                                              int index)
+{
+     /* Return NULL for index > 0 to signify end of remote-endpoints. */
+     if (!index || strcmp(prop_name, "remote-endpoint"))  
There seem to be a bug here: "!index" should be "index > 0", as the comment suggests. Otherwise NULL is always returned.
Ah crap, I think you are right. It should have been "index". Not "!index". But I tested this! Sigh. I probably screwed up my testing.
Please send out a Fix for this.
Geert, we got excited too soon. :(
Quoted 5 lines
I am going to send a quick patch for that, but haven't done so yet because it still won't solve the problem, so I wanted to open the topic here without further delay.
Even with the 'index > 0' fix I'm still getting pretty much the same:
This part is confusing though. If I read your DT correctly, there's a cycle between platform:panel-dsi-lvds and i2c:13-002c. And fw_devlink should not be enforcing any ordering between those devices ever.
I'm surprised that in your "working" case, fw_devlink didn't detect any cycle. It should have. If there's any debugging to do, that's the one we need to debug.
Quoted 23 lines
[   34.836781] ------------[ cut here ]------------
[   34.841401] WARNING: CPU: 2 PID: 204 at drivers/base/devres.c:1064 devm_kfree+0x8c/0xfc
...
[   35.024751] Call trace:
[   35.027199]  devm_kfree+0x8c/0xfc
[   35.030520]  devm_drm_panel_bridge_release+0x54/0x64 [drm_kms_helper]
[   35.036990]  devres_release_group+0xe0/0x164
[   35.041264]  i2c_device_remove+0x38/0x9c
[   35.045196]  device_remove+0x4c/0x80
[   35.048774]  device_release_driver_internal+0x1d4/0x230
[   35.054003]  device_release_driver+0x18/0x24
[   35.058279]  bus_remove_device+0xcc/0x10c
[   35.062292]  device_del+0x15c/0x41c
[   35.065786]  device_unregister+0x18/0x34
[   35.069714]  i2c_unregister_device+0x54/0x88
[   35.073988]  of_i2c_notify+0x98/0x224
[   35.077656]  blocking_notifier_call_chain+0x6c/0xa0
[   35.082543]  __of_changeset_entry_notify+0x100/0x16c
[   35.087515]  __of_changeset_revert_notify+0x44/0x78
[   35.092398]  of_overlay_remove+0x114/0x1c4
...
Quoted 7 lines
By comparing the two versions I found that before removing the overlay:
* in the "working" case (with this patch reverted) I have:
   # ls /sys/class/devlink/ | grep 002c
   platform:hpbr--i2c:13-002c
   platform:panel-dsi-lvds--i2c:13-002c  
Can you check the "status" and "sync_state_only" file in this folder and tell me what it says?
Since these devices have a cyclic dependency between them, it should have been something other than "not tracked" and "sync_state_only" should be "1". But my guess is you'll see "active" and "0".
Quoted 16 lines
   platform:regulator-sys-1v8--i2c:13-002c
   regulator:regulator.31--i2c:13-002c
   #
* in the "broken" case (v6.8-rc5 + s/!index/index > 0/ as mentioned):
   # ls /sys/class/devlink/ | grep 002c
   platform:hpbr--i2c:13-002c
   platform:regulator-sys-1v8--i2c:13-002c
   regulator:regulator.30--i2c:13-002c
   #
So in the latter case the panel-dsi-lvds--i2c:13-002c link is missing. I think it gets created but later on removed. Here's a snippet of the kernel log when that happens:
Quoted 6 lines
[    9.578279] ----- cycle: start -----
[    9.578283] /soc@0/bus@30800000/i2c@30ad0000/i2cmux@70/i2c@3/dsi-lvds-bridge@2c: cycle: depends on /panel-dsi-lvds
[    9.578308] /panel-dsi-lvds: cycle: depends on /soc@0/bus@30800000/i2c@30ad0000/i2cmux@70/i2c@3/dsi-lvds-bridge@2c
[    9.578329] ----- cycle: end -----
[    9.578334] platform panel-dsi-lvds: Fixed dependency cycle(s) with /soc@0/bus@30800000/i2c@30ad0000/i2cmux@70/i2c@3/dsi-lvds-bridge@2c
...
Somewhere in this area, I'm thinking you'll also see "device:
'i2c:13-002c--platform:panel-dsi-lvds': device_add" do you not? And if you enabled device link logs, you'll see that it was "sync state only" link.
Quoted 6 lines
[    9.590620] /panel-dsi-lvds Dropping the fwnode link to /soc@0/bus@30800000/i2c@30ad0000/i2cmux@70/i2c@3/dsi-lvds-bridge@2c
...
[    9.597280] ----- cycle: start -----
[    9.597283] /panel-dsi-lvds: cycle: depends on /soc@0/bus@30800000/i2c@30ad0000/i2cmux@70/i2c@3/dsi-lvds-bridge@2c
[    9.602781] /soc@0/bus@30800000/i2c@30ad0000/i2cmux@70/i2c@3/dsi-lvds-bridge@2c: cycle: depends on /panel-dsi-lvds
[    9.607581] ----- cycle: end -----
Quoted 8 lines
[    9.607585] i2c 13-002c: Fixed dependency cycle(s) with /panel-dsi-lvds
[    9.614217] device: 'platform:panel-dsi-lvds--i2c:13-002c': device_add
...
[    9.614277] /soc@0/bus@30800000/i2c@30ad0000/i2cmux@70/i2c@3/dsi-lvds-bridge@2c Dropping the fwnode link to /panel-dsi-lvds
[    9.614369] /soc@0/bus@30800000/i2c@30ad0000/i2cmux@70/i2c@3/dsi-lvds-bridge@2c Dropping the fwnode link to /regulator-dock-sys-1v8
...
[    9.739840] panel-simple panel-dsi-lvds: Dropping the link to 13-002c
[    9.739846] device: 'i2c:13-002c--platform:panel-dsi-lvds': device_unregister  
Oh yeah, see. The "device_add" I expected earlier is getting removed here.
Quoted 5 lines
[   10.247037] sn65dsi83 13-002c: Dropping the link to panel-dsi-lvds
[   10.247049] device: 'platform:panel-dsi-lvds--i2c:13-002c': device_unregister
And here's the relevant portion of my device tree overlay:

Quoted 8 lines

Quoted 2 lines
--------------------8<--------------------
I think the eventual fix would be this series + adding a "post-init-providers" property to the device that's supposed to probe first and point it to the device that's supposed to probe next. Do this at the device node level, not the endpoint level.
https://lore.kernel.org/lkml/[email protected]/

I'm certainly going to look at this series in more detail and at the debugging you asked for, however I'm afraid I won't have access to the hardware this week and it's not going to be a quick task anyway.

So in this moment I think it's quite clear that this specific patch creates a regression and there is no clear fix that is reasonably likely to get merged before 6.8.

I propose reverting this patch immediately, unless you have a better short-term solution.

Luca

Signature · 3 lines

Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Rob Herring

2024-02-28 21:56 UTC #6 lore raw

Re: [REGRESSION] Re: [PATCH v2 2/3] of: property: Improve finding the supplier of a remote-endpoint property

Quoted 179 lines · Luca Ceresoli

Hello Saravana,
Quoted 160 lines · On Fri, 23 Feb 2024 17:35:24 -0800 Saravana Kannan <[email protected]> wrote:
Quoted 14 lines · Luca Ceresoli
Hello Saravana,
[+cc Hervé Codina]
Quoted 6 lines · On Tue, 6 Feb 2024 17:18:01 -0800 Saravana Kannan <[email protected]> wrote:
After commit 4a032827daa8 ("of: property: Simplify of_link_to_phandle()"), remote-endpoint properties created a fwnode link from the consumer device to the supplier endpoint. This is a tiny bit inefficient (not buggy) when trying to create device links or detecting cycles. So, improve this the same way we improved finding the consumer of a remote-endpoint property.
Quoted 7 lines
Quoted 2 lines
Fixes 4a032827daa8 ("of: property: Simplify of_link_to_phandle()")
Signed-off-by Saravana Kannan <[email protected]>
After rebasing my own branch on v6.8-rc5 from v6.8-rc1 I started getting unexpected warnings during device tree overlay removal. After a somewhat painful bisection I identified this patch as the one that triggers it all.
Thanks for the report.
Quoted 16 lines
Quoted 16 lines
---
drivers/of/property.c +0 -1
@@ -1232,7 +1232,6 @@ 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_SIMPLE_PROP(pwms, "pwms", "#pwm-cells")
 DEFINE_SIMPLE_PROP(resets, "resets", "#reset-cells")
 DEFINE_SIMPLE_PROP(leds, "leds", NULL)
@@ -1298,6 +1297,17 @@ static struct device_node *parse_interrupts(struct device_node *np,
      return of_irq_parse_one(np, index, &sup_args) ? NULL : sup_args.np;
 }
 
Quoted 9 lines
drivers/of/property.c static struct device_node *parse_interrupts(struct device_node *np, · line 1305
Quoted 6 lines
drivers/of/property.c +6 -0
+static struct device_node *parse_remote_endpoint(struct device_node *np,
+                                              const char *prop_name,
+                                              int index)
+{
+     /* Return NULL for index > 0 to signify end of remote-endpoints. */
+     if (!index || strcmp(prop_name, "remote-endpoint"))
There seem to be a bug here: "!index" should be "index > 0", as the comment suggests. Otherwise NULL is always returned.
Ah crap, I think you are right. It should have been "index". Not "!index". But I tested this! Sigh. I probably screwed up my testing.
Please send out a Fix for this.
Geert, we got excited too soon. :(
Quoted 5 lines
I am going to send a quick patch for that, but haven't done so yet because it still won't solve the problem, so I wanted to open the topic here without further delay.
Even with the 'index > 0' fix I'm still getting pretty much the same:
This part is confusing though. If I read your DT correctly, there's a cycle between platform:panel-dsi-lvds and i2c:13-002c. And fw_devlink should not be enforcing any ordering between those devices ever.
I'm surprised that in your "working" case, fw_devlink didn't detect any cycle. It should have. If there's any debugging to do, that's the one we need to debug.
Quoted 23 lines
[   34.836781] ------------[ cut here ]------------
[   34.841401] WARNING: CPU: 2 PID: 204 at drivers/base/devres.c:1064 devm_kfree+0x8c/0xfc
...
[   35.024751] Call trace:
[   35.027199]  devm_kfree+0x8c/0xfc
[   35.030520]  devm_drm_panel_bridge_release+0x54/0x64 [drm_kms_helper]
[   35.036990]  devres_release_group+0xe0/0x164
[   35.041264]  i2c_device_remove+0x38/0x9c
[   35.045196]  device_remove+0x4c/0x80
[   35.048774]  device_release_driver_internal+0x1d4/0x230
[   35.054003]  device_release_driver+0x18/0x24
[   35.058279]  bus_remove_device+0xcc/0x10c
[   35.062292]  device_del+0x15c/0x41c
[   35.065786]  device_unregister+0x18/0x34
[   35.069714]  i2c_unregister_device+0x54/0x88
[   35.073988]  of_i2c_notify+0x98/0x224
[   35.077656]  blocking_notifier_call_chain+0x6c/0xa0
[   35.082543]  __of_changeset_entry_notify+0x100/0x16c
[   35.087515]  __of_changeset_revert_notify+0x44/0x78
[   35.092398]  of_overlay_remove+0x114/0x1c4
...
Quoted 7 lines
By comparing the two versions I found that before removing the overlay:
* in the "working" case (with this patch reverted) I have:
   # ls /sys/class/devlink/ | grep 002c
   platform:hpbr--i2c:13-002c
   platform:panel-dsi-lvds--i2c:13-002c
Can you check the "status" and "sync_state_only" file in this folder and tell me what it says?
Since these devices have a cyclic dependency between them, it should have been something other than "not tracked" and "sync_state_only" should be "1". But my guess is you'll see "active" and "0".
Quoted 16 lines
   platform:regulator-sys-1v8--i2c:13-002c
   regulator:regulator.31--i2c:13-002c
   #
* in the "broken" case (v6.8-rc5 + s/!index/index > 0/ as mentioned):
   # ls /sys/class/devlink/ | grep 002c
   platform:hpbr--i2c:13-002c
   platform:regulator-sys-1v8--i2c:13-002c
   regulator:regulator.30--i2c:13-002c
   #
So in the latter case the panel-dsi-lvds--i2c:13-002c link is missing. I think it gets created but later on removed. Here's a snippet of the kernel log when that happens:
Quoted 6 lines
[    9.578279] ----- cycle: start -----
[    9.578283] /soc@0/bus@30800000/i2c@30ad0000/i2cmux@70/i2c@3/dsi-lvds-bridge@2c: cycle: depends on /panel-dsi-lvds
[    9.578308] /panel-dsi-lvds: cycle: depends on /soc@0/bus@30800000/i2c@30ad0000/i2cmux@70/i2c@3/dsi-lvds-bridge@2c
[    9.578329] ----- cycle: end -----
[    9.578334] platform panel-dsi-lvds: Fixed dependency cycle(s) with /soc@0/bus@30800000/i2c@30ad0000/i2cmux@70/i2c@3/dsi-lvds-bridge@2c
...
Somewhere in this area, I'm thinking you'll also see "device:
'i2c:13-002c--platform:panel-dsi-lvds': device_add" do you not? And if you enabled device link logs, you'll see that it was "sync state only" link.
Quoted 6 lines
[    9.590620] /panel-dsi-lvds Dropping the fwnode link to /soc@0/bus@30800000/i2c@30ad0000/i2cmux@70/i2c@3/dsi-lvds-bridge@2c
...
[    9.597280] ----- cycle: start -----
[    9.597283] /panel-dsi-lvds: cycle: depends on /soc@0/bus@30800000/i2c@30ad0000/i2cmux@70/i2c@3/dsi-lvds-bridge@2c
[    9.602781] /soc@0/bus@30800000/i2c@30ad0000/i2cmux@70/i2c@3/dsi-lvds-bridge@2c: cycle: depends on /panel-dsi-lvds
[    9.607581] ----- cycle: end -----
Quoted 8 lines
[    9.607585] i2c 13-002c: Fixed dependency cycle(s) with /panel-dsi-lvds
[    9.614217] device: 'platform:panel-dsi-lvds--i2c:13-002c': device_add
...
[    9.614277] /soc@0/bus@30800000/i2c@30ad0000/i2cmux@70/i2c@3/dsi-lvds-bridge@2c Dropping the fwnode link to /panel-dsi-lvds
[    9.614369] /soc@0/bus@30800000/i2c@30ad0000/i2cmux@70/i2c@3/dsi-lvds-bridge@2c Dropping the fwnode link to /regulator-dock-sys-1v8
...
[    9.739840] panel-simple panel-dsi-lvds: Dropping the link to 13-002c
[    9.739846] device: 'i2c:13-002c--platform:panel-dsi-lvds': device_unregister
Oh yeah, see. The "device_add" I expected earlier is getting removed here.
Quoted 5 lines
[   10.247037] sn65dsi83 13-002c: Dropping the link to panel-dsi-lvds
[   10.247049] device: 'platform:panel-dsi-lvds--i2c:13-002c': device_unregister
And here's the relevant portion of my device tree overlay:
Quoted 8 lines
Quoted 2 lines
--------------------8<--------------------
I think the eventual fix would be this series + adding a "post-init-providers" property to the device that's supposed to probe first and point it to the device that's supposed to probe next. Do this at the device node level, not the endpoint level.
https://lore.kernel.org/lkml/[email protected]/
I'm certainly going to look at this series in more detail and at the debugging you asked for, however I'm afraid I won't have access to the hardware this week and it's not going to be a quick task anyway.

Quoted 6 lines

So in this moment I think it's quite clear that this specific patch creates a regression and there is no clear fix that is reasonably likely to get merged before 6.8.
I propose reverting this patch immediately, unless you have a better short-term solution.

It's just this one of the 3 patches that needs reverting?

Rob

Saravana Kannan

author

2024-02-28 23:58 UTC #7 lore raw

Re: [REGRESSION] Re: [PATCH v2 2/3] of: property: Improve finding the supplier of a remote-endpoint property

Quoted 181 lines · Rob Herring

Quoted 179 lines · Luca Ceresoli
Hello Saravana,
Quoted 160 lines · On Fri, 23 Feb 2024 17:35:24 -0800 Saravana Kannan <[email protected]> wrote:
Quoted 14 lines · Luca Ceresoli
Hello Saravana,
[+cc Hervé Codina]
Quoted 6 lines · On Tue, 6 Feb 2024 17:18:01 -0800 Saravana Kannan <[email protected]> wrote:
After commit 4a032827daa8 ("of: property: Simplify of_link_to_phandle()"), remote-endpoint properties created a fwnode link from the consumer device to the supplier endpoint. This is a tiny bit inefficient (not buggy) when trying to create device links or detecting cycles. So, improve this the same way we improved finding the consumer of a remote-endpoint property.
Quoted 7 lines
Quoted 2 lines
Fixes 4a032827daa8 ("of: property: Simplify of_link_to_phandle()")
Signed-off-by Saravana Kannan <[email protected]>
After rebasing my own branch on v6.8-rc5 from v6.8-rc1 I started getting unexpected warnings during device tree overlay removal. After a somewhat painful bisection I identified this patch as the one that triggers it all.
Thanks for the report.
Quoted 16 lines
Quoted 16 lines
---
drivers/of/property.c +0 -1
@@ -1232,7 +1232,6 @@ 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_SIMPLE_PROP(pwms, "pwms", "#pwm-cells")
 DEFINE_SIMPLE_PROP(resets, "resets", "#reset-cells")
 DEFINE_SIMPLE_PROP(leds, "leds", NULL)
@@ -1298,6 +1297,17 @@ static struct device_node *parse_interrupts(struct device_node *np,
      return of_irq_parse_one(np, index, &sup_args) ? NULL : sup_args.np;
 }
 
Quoted 9 lines
drivers/of/property.c static struct device_node *parse_interrupts(struct device_node *np, · line 1305
Quoted 6 lines
drivers/of/property.c +6 -0
+static struct device_node *parse_remote_endpoint(struct device_node *np,
+                                              const char *prop_name,
+                                              int index)
+{
+     /* Return NULL for index > 0 to signify end of remote-endpoints. */
+     if (!index || strcmp(prop_name, "remote-endpoint"))
There seem to be a bug here: "!index" should be "index > 0", as the comment suggests. Otherwise NULL is always returned.
Ah crap, I think you are right. It should have been "index". Not "!index". But I tested this! Sigh. I probably screwed up my testing.
Please send out a Fix for this.
Geert, we got excited too soon. :(
Quoted 5 lines
I am going to send a quick patch for that, but haven't done so yet because it still won't solve the problem, so I wanted to open the topic here without further delay.
Even with the 'index > 0' fix I'm still getting pretty much the same:
This part is confusing though. If I read your DT correctly, there's a cycle between platform:panel-dsi-lvds and i2c:13-002c. And fw_devlink should not be enforcing any ordering between those devices ever.
I'm surprised that in your "working" case, fw_devlink didn't detect any cycle. It should have. If there's any debugging to do, that's the one we need to debug.
Quoted 23 lines
[   34.836781] ------------[ cut here ]------------
[   34.841401] WARNING: CPU: 2 PID: 204 at drivers/base/devres.c:1064 devm_kfree+0x8c/0xfc
...
[   35.024751] Call trace:
[   35.027199]  devm_kfree+0x8c/0xfc
[   35.030520]  devm_drm_panel_bridge_release+0x54/0x64 [drm_kms_helper]
[   35.036990]  devres_release_group+0xe0/0x164
[   35.041264]  i2c_device_remove+0x38/0x9c
[   35.045196]  device_remove+0x4c/0x80
[   35.048774]  device_release_driver_internal+0x1d4/0x230
[   35.054003]  device_release_driver+0x18/0x24
[   35.058279]  bus_remove_device+0xcc/0x10c
[   35.062292]  device_del+0x15c/0x41c
[   35.065786]  device_unregister+0x18/0x34
[   35.069714]  i2c_unregister_device+0x54/0x88
[   35.073988]  of_i2c_notify+0x98/0x224
[   35.077656]  blocking_notifier_call_chain+0x6c/0xa0
[   35.082543]  __of_changeset_entry_notify+0x100/0x16c
[   35.087515]  __of_changeset_revert_notify+0x44/0x78
[   35.092398]  of_overlay_remove+0x114/0x1c4
...
Quoted 7 lines
By comparing the two versions I found that before removing the overlay:
* in the "working" case (with this patch reverted) I have:
   # ls /sys/class/devlink/ | grep 002c
   platform:hpbr--i2c:13-002c
   platform:panel-dsi-lvds--i2c:13-002c
Can you check the "status" and "sync_state_only" file in this folder and tell me what it says?
Since these devices have a cyclic dependency between them, it should have been something other than "not tracked" and "sync_state_only" should be "1". But my guess is you'll see "active" and "0".
Quoted 16 lines
   platform:regulator-sys-1v8--i2c:13-002c
   regulator:regulator.31--i2c:13-002c
   #
* in the "broken" case (v6.8-rc5 + s/!index/index > 0/ as mentioned):
   # ls /sys/class/devlink/ | grep 002c
   platform:hpbr--i2c:13-002c
   platform:regulator-sys-1v8--i2c:13-002c
   regulator:regulator.30--i2c:13-002c
   #
So in the latter case the panel-dsi-lvds--i2c:13-002c link is missing. I think it gets created but later on removed. Here's a snippet of the kernel log when that happens:
Quoted 6 lines
[    9.578279] ----- cycle: start -----
[    9.578283] /soc@0/bus@30800000/i2c@30ad0000/i2cmux@70/i2c@3/dsi-lvds-bridge@2c: cycle: depends on /panel-dsi-lvds
[    9.578308] /panel-dsi-lvds: cycle: depends on /soc@0/bus@30800000/i2c@30ad0000/i2cmux@70/i2c@3/dsi-lvds-bridge@2c
[    9.578329] ----- cycle: end -----
[    9.578334] platform panel-dsi-lvds: Fixed dependency cycle(s) with /soc@0/bus@30800000/i2c@30ad0000/i2cmux@70/i2c@3/dsi-lvds-bridge@2c
...
Somewhere in this area, I'm thinking you'll also see "device:
'i2c:13-002c--platform:panel-dsi-lvds': device_add" do you not? And if you enabled device link logs, you'll see that it was "sync state only" link.
Quoted 6 lines
[    9.590620] /panel-dsi-lvds Dropping the fwnode link to /soc@0/bus@30800000/i2c@30ad0000/i2cmux@70/i2c@3/dsi-lvds-bridge@2c
...
[    9.597280] ----- cycle: start -----
[    9.597283] /panel-dsi-lvds: cycle: depends on /soc@0/bus@30800000/i2c@30ad0000/i2cmux@70/i2c@3/dsi-lvds-bridge@2c
[    9.602781] /soc@0/bus@30800000/i2c@30ad0000/i2cmux@70/i2c@3/dsi-lvds-bridge@2c: cycle: depends on /panel-dsi-lvds
[    9.607581] ----- cycle: end -----
Quoted 8 lines
[    9.607585] i2c 13-002c: Fixed dependency cycle(s) with /panel-dsi-lvds
[    9.614217] device: 'platform:panel-dsi-lvds--i2c:13-002c': device_add
...
[    9.614277] /soc@0/bus@30800000/i2c@30ad0000/i2cmux@70/i2c@3/dsi-lvds-bridge@2c Dropping the fwnode link to /panel-dsi-lvds
[    9.614369] /soc@0/bus@30800000/i2c@30ad0000/i2cmux@70/i2c@3/dsi-lvds-bridge@2c Dropping the fwnode link to /regulator-dock-sys-1v8
...
[    9.739840] panel-simple panel-dsi-lvds: Dropping the link to 13-002c
[    9.739846] device: 'i2c:13-002c--platform:panel-dsi-lvds': device_unregister
Oh yeah, see. The "device_add" I expected earlier is getting removed here.
Quoted 5 lines
[   10.247037] sn65dsi83 13-002c: Dropping the link to panel-dsi-lvds
[   10.247049] device: 'platform:panel-dsi-lvds--i2c:13-002c': device_unregister
And here's the relevant portion of my device tree overlay:
Quoted 8 lines
Quoted 2 lines
--------------------8<--------------------
I think the eventual fix would be this series + adding a "post-init-providers" property to the device that's supposed to probe first and point it to the device that's supposed to probe next. Do this at the device node level, not the endpoint level.
https://lore.kernel.org/lkml/[email protected]/
I'm certainly going to look at this series in more detail and at the debugging you asked for, however I'm afraid I won't have access to the hardware this week and it's not going to be a quick task anyway.

Quoted 8 lines

Quoted 6 lines
So in this moment I think it's quite clear that this specific patch creates a regression and there is no clear fix that is reasonably likely to get merged before 6.8.
I propose reverting this patch immediately, unless you have a better short-term solution.
It's just this one of the 3 patches that needs reverting?

I sent a fix. With the fix, it's just exposing a bug elsewhere.

-Saravana

Rob Herring

2024-02-29 00:26 UTC #8 lore raw

Re: [REGRESSION] Re: [PATCH v2 2/3] of: property: Improve finding the supplier of a remote-endpoint property

Quoted 187 lines · Saravana Kannan

Quoted 185 lines · Rob Herring
Quoted 183 lines · Luca Ceresoli
Hello Saravana,
Quoted 160 lines · On Fri, 23 Feb 2024 17:35:24 -0800 Saravana Kannan <[email protected]> wrote:
Quoted 14 lines · Luca Ceresoli
Hello Saravana,
[+cc Hervé Codina]
Quoted 6 lines · On Tue, 6 Feb 2024 17:18:01 -0800 Saravana Kannan <[email protected]> wrote:
After commit 4a032827daa8 ("of: property: Simplify of_link_to_phandle()"), remote-endpoint properties created a fwnode link from the consumer device to the supplier endpoint. This is a tiny bit inefficient (not buggy) when trying to create device links or detecting cycles. So, improve this the same way we improved finding the consumer of a remote-endpoint property.
Quoted 7 lines
Quoted 2 lines
Fixes 4a032827daa8 ("of: property: Simplify of_link_to_phandle()")
Signed-off-by Saravana Kannan <[email protected]>
After rebasing my own branch on v6.8-rc5 from v6.8-rc1 I started getting unexpected warnings during device tree overlay removal. After a somewhat painful bisection I identified this patch as the one that triggers it all.
Thanks for the report.
Quoted 16 lines
Quoted 16 lines
---
drivers/of/property.c +0 -1
@@ -1232,7 +1232,6 @@ 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_SIMPLE_PROP(pwms, "pwms", "#pwm-cells")
 DEFINE_SIMPLE_PROP(resets, "resets", "#reset-cells")
 DEFINE_SIMPLE_PROP(leds, "leds", NULL)
@@ -1298,6 +1297,17 @@ static struct device_node *parse_interrupts(struct device_node *np,
      return of_irq_parse_one(np, index, &sup_args) ? NULL : sup_args.np;
 }
 
Quoted 9 lines
drivers/of/property.c static struct device_node *parse_interrupts(struct device_node *np, · line 1305
Quoted 6 lines
drivers/of/property.c +6 -0
+static struct device_node *parse_remote_endpoint(struct device_node *np,
+                                              const char *prop_name,
+                                              int index)
+{
+     /* Return NULL for index > 0 to signify end of remote-endpoints. */
+     if (!index || strcmp(prop_name, "remote-endpoint"))
There seem to be a bug here: "!index" should be "index > 0", as the comment suggests. Otherwise NULL is always returned.
Ah crap, I think you are right. It should have been "index". Not "!index". But I tested this! Sigh. I probably screwed up my testing.
Please send out a Fix for this.
Geert, we got excited too soon. :(
Quoted 5 lines
I am going to send a quick patch for that, but haven't done so yet because it still won't solve the problem, so I wanted to open the topic here without further delay.
Even with the 'index > 0' fix I'm still getting pretty much the same:
This part is confusing though. If I read your DT correctly, there's a cycle between platform:panel-dsi-lvds and i2c:13-002c. And fw_devlink should not be enforcing any ordering between those devices ever.
I'm surprised that in your "working" case, fw_devlink didn't detect any cycle. It should have. If there's any debugging to do, that's the one we need to debug.
Quoted 23 lines
[   34.836781] ------------[ cut here ]------------
[   34.841401] WARNING: CPU: 2 PID: 204 at drivers/base/devres.c:1064 devm_kfree+0x8c/0xfc
...
[   35.024751] Call trace:
[   35.027199]  devm_kfree+0x8c/0xfc
[   35.030520]  devm_drm_panel_bridge_release+0x54/0x64 [drm_kms_helper]
[   35.036990]  devres_release_group+0xe0/0x164
[   35.041264]  i2c_device_remove+0x38/0x9c
[   35.045196]  device_remove+0x4c/0x80
[   35.048774]  device_release_driver_internal+0x1d4/0x230
[   35.054003]  device_release_driver+0x18/0x24
[   35.058279]  bus_remove_device+0xcc/0x10c
[   35.062292]  device_del+0x15c/0x41c
[   35.065786]  device_unregister+0x18/0x34
[   35.069714]  i2c_unregister_device+0x54/0x88
[   35.073988]  of_i2c_notify+0x98/0x224
[   35.077656]  blocking_notifier_call_chain+0x6c/0xa0
[   35.082543]  __of_changeset_entry_notify+0x100/0x16c
[   35.087515]  __of_changeset_revert_notify+0x44/0x78
[   35.092398]  of_overlay_remove+0x114/0x1c4
...
Quoted 7 lines
By comparing the two versions I found that before removing the overlay:
* in the "working" case (with this patch reverted) I have:
   # ls /sys/class/devlink/ | grep 002c
   platform:hpbr--i2c:13-002c
   platform:panel-dsi-lvds--i2c:13-002c
Can you check the "status" and "sync_state_only" file in this folder and tell me what it says?
Since these devices have a cyclic dependency between them, it should have been something other than "not tracked" and "sync_state_only" should be "1". But my guess is you'll see "active" and "0".
Quoted 16 lines
   platform:regulator-sys-1v8--i2c:13-002c
   regulator:regulator.31--i2c:13-002c
   #
* in the "broken" case (v6.8-rc5 + s/!index/index > 0/ as mentioned):
   # ls /sys/class/devlink/ | grep 002c
   platform:hpbr--i2c:13-002c
   platform:regulator-sys-1v8--i2c:13-002c
   regulator:regulator.30--i2c:13-002c
   #
So in the latter case the panel-dsi-lvds--i2c:13-002c link is missing. I think it gets created but later on removed. Here's a snippet of the kernel log when that happens:
Quoted 6 lines
[    9.578279] ----- cycle: start -----
[    9.578283] /soc@0/bus@30800000/i2c@30ad0000/i2cmux@70/i2c@3/dsi-lvds-bridge@2c: cycle: depends on /panel-dsi-lvds
[    9.578308] /panel-dsi-lvds: cycle: depends on /soc@0/bus@30800000/i2c@30ad0000/i2cmux@70/i2c@3/dsi-lvds-bridge@2c
[    9.578329] ----- cycle: end -----
[    9.578334] platform panel-dsi-lvds: Fixed dependency cycle(s) with /soc@0/bus@30800000/i2c@30ad0000/i2cmux@70/i2c@3/dsi-lvds-bridge@2c
...
Somewhere in this area, I'm thinking you'll also see "device:
'i2c:13-002c--platform:panel-dsi-lvds': device_add" do you not? And if you enabled device link logs, you'll see that it was "sync state only" link.
Quoted 6 lines
[    9.590620] /panel-dsi-lvds Dropping the fwnode link to /soc@0/bus@30800000/i2c@30ad0000/i2cmux@70/i2c@3/dsi-lvds-bridge@2c
...
[    9.597280] ----- cycle: start -----
[    9.597283] /panel-dsi-lvds: cycle: depends on /soc@0/bus@30800000/i2c@30ad0000/i2cmux@70/i2c@3/dsi-lvds-bridge@2c
[    9.602781] /soc@0/bus@30800000/i2c@30ad0000/i2cmux@70/i2c@3/dsi-lvds-bridge@2c: cycle: depends on /panel-dsi-lvds
[    9.607581] ----- cycle: end -----
Quoted 8 lines
[    9.607585] i2c 13-002c: Fixed dependency cycle(s) with /panel-dsi-lvds
[    9.614217] device: 'platform:panel-dsi-lvds--i2c:13-002c': device_add
...
[    9.614277] /soc@0/bus@30800000/i2c@30ad0000/i2cmux@70/i2c@3/dsi-lvds-bridge@2c Dropping the fwnode link to /panel-dsi-lvds
[    9.614369] /soc@0/bus@30800000/i2c@30ad0000/i2cmux@70/i2c@3/dsi-lvds-bridge@2c Dropping the fwnode link to /regulator-dock-sys-1v8
...
[    9.739840] panel-simple panel-dsi-lvds: Dropping the link to 13-002c
[    9.739846] device: 'i2c:13-002c--platform:panel-dsi-lvds': device_unregister
Oh yeah, see. The "device_add" I expected earlier is getting removed here.
Quoted 5 lines
[   10.247037] sn65dsi83 13-002c: Dropping the link to panel-dsi-lvds
[   10.247049] device: 'platform:panel-dsi-lvds--i2c:13-002c': device_unregister
And here's the relevant portion of my device tree overlay:
Quoted 8 lines
Quoted 2 lines
--------------------8<--------------------
I think the eventual fix would be this series + adding a "post-init-providers" property to the device that's supposed to probe first and point it to the device that's supposed to probe next. Do this at the device node level, not the endpoint level.
https://lore.kernel.org/lkml/[email protected]/
I'm certainly going to look at this series in more detail and at the debugging you asked for, however I'm afraid I won't have access to the hardware this week and it's not going to be a quick task anyway.
So in this moment I think it's quite clear that this specific patch creates a regression and there is no clear fix that is reasonably likely to get merged before 6.8.

Quoted 6 lines

Quoted 4 lines
Quoted 2 lines
I propose reverting this patch immediately, unless you have a better short-term solution.
It's just this one of the 3 patches that needs reverting?
I sent a fix. With the fix, it's just exposing a bug elsewhere.

That's not telling me what to do...

You say apply the fix. Luca says revert. I say I wish I made this 6.9 material. Which is it?

If the overlay applying depends on out of tree code (likely as there are limited ways to apply an overlay in mainline), then I don't really care if there is still a regression.

Rob

Luca Ceresoli

2024-02-29 09:34 UTC #9 lore raw

Re: [REGRESSION] Re: [PATCH v2 2/3] of: property: Improve finding the supplier of a remote-endpoint property

Hi Rob, Saravana,

Quoted 184 lines · On Wed, 28 Feb 2024 18:26:36 -0600 Rob Herring <[email protected]> wrote:

Quoted 183 lines · Saravana Kannan
Quoted 181 lines · Rob Herring
Quoted 179 lines · Luca Ceresoli
Hello Saravana,
Quoted 160 lines · On Fri, 23 Feb 2024 17:35:24 -0800 Saravana Kannan <[email protected]> wrote:
Quoted 14 lines · Luca Ceresoli
Hello Saravana,
[+cc Hervé Codina]
Quoted 6 lines · On Tue, 6 Feb 2024 17:18:01 -0800 Saravana Kannan <[email protected]> wrote:
After commit 4a032827daa8 ("of: property: Simplify of_link_to_phandle()"), remote-endpoint properties created a fwnode link from the consumer device to the supplier endpoint. This is a tiny bit inefficient (not buggy) when trying to create device links or detecting cycles. So, improve this the same way we improved finding the consumer of a remote-endpoint property.
Quoted 7 lines
Quoted 2 lines
Fixes 4a032827daa8 ("of: property: Simplify of_link_to_phandle()")
Signed-off-by Saravana Kannan <[email protected]>
After rebasing my own branch on v6.8-rc5 from v6.8-rc1 I started getting unexpected warnings during device tree overlay removal. After a somewhat painful bisection I identified this patch as the one that triggers it all.
Thanks for the report.
Quoted 16 lines
Quoted 16 lines
---
drivers/of/property.c +0 -1
@@ -1232,7 +1232,6 @@ 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_SIMPLE_PROP(pwms, "pwms", "#pwm-cells")
 DEFINE_SIMPLE_PROP(resets, "resets", "#reset-cells")
 DEFINE_SIMPLE_PROP(leds, "leds", NULL)
@@ -1298,6 +1297,17 @@ static struct device_node *parse_interrupts(struct device_node *np,
      return of_irq_parse_one(np, index, &sup_args) ? NULL : sup_args.np;
 }
 
Quoted 9 lines
drivers/of/property.c static struct device_node *parse_interrupts(struct device_node *np, · line 1305
Quoted 6 lines
drivers/of/property.c +6 -0
+static struct device_node *parse_remote_endpoint(struct device_node *np,
+                                              const char *prop_name,
+                                              int index)
+{
+     /* Return NULL for index > 0 to signify end of remote-endpoints. */
+     if (!index || strcmp(prop_name, "remote-endpoint"))  
There seem to be a bug here: "!index" should be "index > 0", as the comment suggests. Otherwise NULL is always returned.
Ah crap, I think you are right. It should have been "index". Not "!index". But I tested this! Sigh. I probably screwed up my testing.
Please send out a Fix for this.
Geert, we got excited too soon. :(
Quoted 5 lines
I am going to send a quick patch for that, but haven't done so yet because it still won't solve the problem, so I wanted to open the topic here without further delay.
Even with the 'index > 0' fix I'm still getting pretty much the same:
This part is confusing though. If I read your DT correctly, there's a cycle between platform:panel-dsi-lvds and i2c:13-002c. And fw_devlink should not be enforcing any ordering between those devices ever.
I'm surprised that in your "working" case, fw_devlink didn't detect any cycle. It should have. If there's any debugging to do, that's the one we need to debug.
Quoted 23 lines
[   34.836781] ------------[ cut here ]------------
[   34.841401] WARNING: CPU: 2 PID: 204 at drivers/base/devres.c:1064 devm_kfree+0x8c/0xfc
...
[   35.024751] Call trace:
[   35.027199]  devm_kfree+0x8c/0xfc
[   35.030520]  devm_drm_panel_bridge_release+0x54/0x64 [drm_kms_helper]
[   35.036990]  devres_release_group+0xe0/0x164
[   35.041264]  i2c_device_remove+0x38/0x9c
[   35.045196]  device_remove+0x4c/0x80
[   35.048774]  device_release_driver_internal+0x1d4/0x230
[   35.054003]  device_release_driver+0x18/0x24
[   35.058279]  bus_remove_device+0xcc/0x10c
[   35.062292]  device_del+0x15c/0x41c
[   35.065786]  device_unregister+0x18/0x34
[   35.069714]  i2c_unregister_device+0x54/0x88
[   35.073988]  of_i2c_notify+0x98/0x224
[   35.077656]  blocking_notifier_call_chain+0x6c/0xa0
[   35.082543]  __of_changeset_entry_notify+0x100/0x16c
[   35.087515]  __of_changeset_revert_notify+0x44/0x78
[   35.092398]  of_overlay_remove+0x114/0x1c4
...
Quoted 7 lines
By comparing the two versions I found that before removing the overlay:
* in the "working" case (with this patch reverted) I have:
   # ls /sys/class/devlink/ | grep 002c
   platform:hpbr--i2c:13-002c
   platform:panel-dsi-lvds--i2c:13-002c  
Can you check the "status" and "sync_state_only" file in this folder and tell me what it says?
Since these devices have a cyclic dependency between them, it should have been something other than "not tracked" and "sync_state_only" should be "1". But my guess is you'll see "active" and "0".
Quoted 16 lines
   platform:regulator-sys-1v8--i2c:13-002c
   regulator:regulator.31--i2c:13-002c
   #
* in the "broken" case (v6.8-rc5 + s/!index/index > 0/ as mentioned):
   # ls /sys/class/devlink/ | grep 002c
   platform:hpbr--i2c:13-002c
   platform:regulator-sys-1v8--i2c:13-002c
   regulator:regulator.30--i2c:13-002c
   #
So in the latter case the panel-dsi-lvds--i2c:13-002c link is missing. I think it gets created but later on removed. Here's a snippet of the kernel log when that happens:
Quoted 6 lines
[    9.578279] ----- cycle: start -----
[    9.578283] /soc@0/bus@30800000/i2c@30ad0000/i2cmux@70/i2c@3/dsi-lvds-bridge@2c: cycle: depends on /panel-dsi-lvds
[    9.578308] /panel-dsi-lvds: cycle: depends on /soc@0/bus@30800000/i2c@30ad0000/i2cmux@70/i2c@3/dsi-lvds-bridge@2c
[    9.578329] ----- cycle: end -----
[    9.578334] platform panel-dsi-lvds: Fixed dependency cycle(s) with /soc@0/bus@30800000/i2c@30ad0000/i2cmux@70/i2c@3/dsi-lvds-bridge@2c
...
Somewhere in this area, I'm thinking you'll also see "device:
'i2c:13-002c--platform:panel-dsi-lvds': device_add" do you not? And if you enabled device link logs, you'll see that it was "sync state only" link.
Quoted 6 lines
[    9.590620] /panel-dsi-lvds Dropping the fwnode link to /soc@0/bus@30800000/i2c@30ad0000/i2cmux@70/i2c@3/dsi-lvds-bridge@2c
...
[    9.597280] ----- cycle: start -----
[    9.597283] /panel-dsi-lvds: cycle: depends on /soc@0/bus@30800000/i2c@30ad0000/i2cmux@70/i2c@3/dsi-lvds-bridge@2c
[    9.602781] /soc@0/bus@30800000/i2c@30ad0000/i2cmux@70/i2c@3/dsi-lvds-bridge@2c: cycle: depends on /panel-dsi-lvds
[    9.607581] ----- cycle: end -----
Quoted 8 lines
[    9.607585] i2c 13-002c: Fixed dependency cycle(s) with /panel-dsi-lvds
[    9.614217] device: 'platform:panel-dsi-lvds--i2c:13-002c': device_add
...
[    9.614277] /soc@0/bus@30800000/i2c@30ad0000/i2cmux@70/i2c@3/dsi-lvds-bridge@2c Dropping the fwnode link to /panel-dsi-lvds
[    9.614369] /soc@0/bus@30800000/i2c@30ad0000/i2cmux@70/i2c@3/dsi-lvds-bridge@2c Dropping the fwnode link to /regulator-dock-sys-1v8
...
[    9.739840] panel-simple panel-dsi-lvds: Dropping the link to 13-002c
[    9.739846] device: 'i2c:13-002c--platform:panel-dsi-lvds': device_unregister  
Oh yeah, see. The "device_add" I expected earlier is getting removed here.
Quoted 5 lines
[   10.247037] sn65dsi83 13-002c: Dropping the link to panel-dsi-lvds
[   10.247049] device: 'platform:panel-dsi-lvds--i2c:13-002c': device_unregister
And here's the relevant portion of my device tree overlay:
Quoted 8 lines
Quoted 2 lines
--------------------8<--------------------
I think the eventual fix would be this series + adding a "post-init-providers" property to the device that's supposed to probe first and point it to the device that's supposed to probe next. Do this at the device node level, not the endpoint level.
https://lore.kernel.org/lkml/[email protected]/
I'm certainly going to look at this series in more detail and at the debugging you asked for, however I'm afraid I won't have access to the hardware this week and it's not going to be a quick task anyway.

Quoted 8 lines

Quoted 8 lines
Quoted 8 lines
Quoted 6 lines
So in this moment I think it's quite clear that this specific patch creates a regression and there is no clear fix that is reasonably likely to get merged before 6.8.
I propose reverting this patch immediately, unless you have a better short-term solution.
It's just this one of the 3 patches that needs reverting?

Just this patch. I reverted only this and the issue disappeared.

Quoted 1 line

Quoted 1 line
I sent a fix. With the fix, it's just exposing a bug elsewhere.

Exactly, this patch has two issues and only the easy one has a fix [0] currently as far as I know.

Quoted 6 lines

You say apply the fix. Luca says revert. I say I wish I made this 6.9 material. Which is it?
If the overlay applying depends on out of tree code (likely as there are limited ways to apply an overlay in mainline), then I don't really care if there is still a regression.

Obviously, to load and unload the overlays I'm using code not yet in mainline. It is using of_overlay_fdt_apply() and of_overlay_remove() via a driver underdevelopment that is similar to the one Hervé and Lizhi Hou are working on [1][2].

I see the point that "we are not breaking existing use cases as no code is (un)loading overlays except unittest", sure.

As I see it, we have a feature in the kernel that is not used, but it will be, eventually: there are use cases, development is progressing and patches are being sent actively. My opinion is that we should not put additional known obstacles that will make it even harder than it already is.

[0]
https://lore.kernel.org/linux-devicetree/[email protected]/
[1]
https://lore.kernel.org/all/[email protected]/
[2]
https://lore.kernel.org/lkml/[email protected]/

Luca

Signature · 3 lines

Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Rob Herring

2024-02-29 22:10 UTC #10 lore raw

Re: [REGRESSION] Re: [PATCH v2 2/3] of: property: Improve finding the supplier of a remote-endpoint property

Quoted 218 lines · Luca Ceresoli

Hi Rob, Saravana,
Quoted 184 lines · On Wed, 28 Feb 2024 18:26:36 -0600 Rob Herring <[email protected]> wrote:
Quoted 183 lines · Saravana Kannan
Quoted 181 lines · Rob Herring
Quoted 179 lines · Luca Ceresoli
Hello Saravana,
Quoted 160 lines · On Fri, 23 Feb 2024 17:35:24 -0800 Saravana Kannan <[email protected]> wrote:
Quoted 14 lines · Luca Ceresoli
Hello Saravana,
[+cc Hervé Codina]
Quoted 6 lines · On Tue, 6 Feb 2024 17:18:01 -0800 Saravana Kannan <[email protected]> wrote:
After commit 4a032827daa8 ("of: property: Simplify of_link_to_phandle()"), remote-endpoint properties created a fwnode link from the consumer device to the supplier endpoint. This is a tiny bit inefficient (not buggy) when trying to create device links or detecting cycles. So, improve this the same way we improved finding the consumer of a remote-endpoint property.
Quoted 7 lines
Quoted 2 lines
Fixes 4a032827daa8 ("of: property: Simplify of_link_to_phandle()")
Signed-off-by Saravana Kannan <[email protected]>
After rebasing my own branch on v6.8-rc5 from v6.8-rc1 I started getting unexpected warnings during device tree overlay removal. After a somewhat painful bisection I identified this patch as the one that triggers it all.
Thanks for the report.
Quoted 16 lines
Quoted 16 lines
---
drivers/of/property.c +0 -1
@@ -1232,7 +1232,6 @@ 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_SIMPLE_PROP(pwms, "pwms", "#pwm-cells")
 DEFINE_SIMPLE_PROP(resets, "resets", "#reset-cells")
 DEFINE_SIMPLE_PROP(leds, "leds", NULL)
@@ -1298,6 +1297,17 @@ static struct device_node *parse_interrupts(struct device_node *np,
      return of_irq_parse_one(np, index, &sup_args) ? NULL : sup_args.np;
 }
 
Quoted 9 lines
drivers/of/property.c static struct device_node *parse_interrupts(struct device_node *np, · line 1305
Quoted 6 lines
drivers/of/property.c +6 -0
+static struct device_node *parse_remote_endpoint(struct device_node *np,
+                                              const char *prop_name,
+                                              int index)
+{
+     /* Return NULL for index > 0 to signify end of remote-endpoints. */
+     if (!index || strcmp(prop_name, "remote-endpoint"))
There seem to be a bug here: "!index" should be "index > 0", as the comment suggests. Otherwise NULL is always returned.
Ah crap, I think you are right. It should have been "index". Not "!index". But I tested this! Sigh. I probably screwed up my testing.
Please send out a Fix for this.
Geert, we got excited too soon. :(
Quoted 5 lines
I am going to send a quick patch for that, but haven't done so yet because it still won't solve the problem, so I wanted to open the topic here without further delay.
Even with the 'index > 0' fix I'm still getting pretty much the same:
This part is confusing though. If I read your DT correctly, there's a cycle between platform:panel-dsi-lvds and i2c:13-002c. And fw_devlink should not be enforcing any ordering between those devices ever.
I'm surprised that in your "working" case, fw_devlink didn't detect any cycle. It should have. If there's any debugging to do, that's the one we need to debug.
Quoted 23 lines
[   34.836781] ------------[ cut here ]------------
[   34.841401] WARNING: CPU: 2 PID: 204 at drivers/base/devres.c:1064 devm_kfree+0x8c/0xfc
...
[   35.024751] Call trace:
[   35.027199]  devm_kfree+0x8c/0xfc
[   35.030520]  devm_drm_panel_bridge_release+0x54/0x64 [drm_kms_helper]
[   35.036990]  devres_release_group+0xe0/0x164
[   35.041264]  i2c_device_remove+0x38/0x9c
[   35.045196]  device_remove+0x4c/0x80
[   35.048774]  device_release_driver_internal+0x1d4/0x230
[   35.054003]  device_release_driver+0x18/0x24
[   35.058279]  bus_remove_device+0xcc/0x10c
[   35.062292]  device_del+0x15c/0x41c
[   35.065786]  device_unregister+0x18/0x34
[   35.069714]  i2c_unregister_device+0x54/0x88
[   35.073988]  of_i2c_notify+0x98/0x224
[   35.077656]  blocking_notifier_call_chain+0x6c/0xa0
[   35.082543]  __of_changeset_entry_notify+0x100/0x16c
[   35.087515]  __of_changeset_revert_notify+0x44/0x78
[   35.092398]  of_overlay_remove+0x114/0x1c4
...
Quoted 7 lines
By comparing the two versions I found that before removing the overlay:
* in the "working" case (with this patch reverted) I have:
   # ls /sys/class/devlink/ | grep 002c
   platform:hpbr--i2c:13-002c
   platform:panel-dsi-lvds--i2c:13-002c
Can you check the "status" and "sync_state_only" file in this folder and tell me what it says?
Since these devices have a cyclic dependency between them, it should have been something other than "not tracked" and "sync_state_only" should be "1". But my guess is you'll see "active" and "0".
Quoted 16 lines
   platform:regulator-sys-1v8--i2c:13-002c
   regulator:regulator.31--i2c:13-002c
   #
* in the "broken" case (v6.8-rc5 + s/!index/index > 0/ as mentioned):
   # ls /sys/class/devlink/ | grep 002c
   platform:hpbr--i2c:13-002c
   platform:regulator-sys-1v8--i2c:13-002c
   regulator:regulator.30--i2c:13-002c
   #
So in the latter case the panel-dsi-lvds--i2c:13-002c link is missing. I think it gets created but later on removed. Here's a snippet of the kernel log when that happens:
Quoted 6 lines
[    9.578279] ----- cycle: start -----
[    9.578283] /soc@0/bus@30800000/i2c@30ad0000/i2cmux@70/i2c@3/dsi-lvds-bridge@2c: cycle: depends on /panel-dsi-lvds
[    9.578308] /panel-dsi-lvds: cycle: depends on /soc@0/bus@30800000/i2c@30ad0000/i2cmux@70/i2c@3/dsi-lvds-bridge@2c
[    9.578329] ----- cycle: end -----
[    9.578334] platform panel-dsi-lvds: Fixed dependency cycle(s) with /soc@0/bus@30800000/i2c@30ad0000/i2cmux@70/i2c@3/dsi-lvds-bridge@2c
...
Somewhere in this area, I'm thinking you'll also see "device:
'i2c:13-002c--platform:panel-dsi-lvds': device_add" do you not? And if you enabled device link logs, you'll see that it was "sync state only" link.
Quoted 6 lines
[    9.590620] /panel-dsi-lvds Dropping the fwnode link to /soc@0/bus@30800000/i2c@30ad0000/i2cmux@70/i2c@3/dsi-lvds-bridge@2c
...
[    9.597280] ----- cycle: start -----
[    9.597283] /panel-dsi-lvds: cycle: depends on /soc@0/bus@30800000/i2c@30ad0000/i2cmux@70/i2c@3/dsi-lvds-bridge@2c
[    9.602781] /soc@0/bus@30800000/i2c@30ad0000/i2cmux@70/i2c@3/dsi-lvds-bridge@2c: cycle: depends on /panel-dsi-lvds
[    9.607581] ----- cycle: end -----
Quoted 8 lines
[    9.607585] i2c 13-002c: Fixed dependency cycle(s) with /panel-dsi-lvds
[    9.614217] device: 'platform:panel-dsi-lvds--i2c:13-002c': device_add
...
[    9.614277] /soc@0/bus@30800000/i2c@30ad0000/i2cmux@70/i2c@3/dsi-lvds-bridge@2c Dropping the fwnode link to /panel-dsi-lvds
[    9.614369] /soc@0/bus@30800000/i2c@30ad0000/i2cmux@70/i2c@3/dsi-lvds-bridge@2c Dropping the fwnode link to /regulator-dock-sys-1v8
...
[    9.739840] panel-simple panel-dsi-lvds: Dropping the link to 13-002c
[    9.739846] device: 'i2c:13-002c--platform:panel-dsi-lvds': device_unregister
Oh yeah, see. The "device_add" I expected earlier is getting removed here.
Quoted 5 lines
[   10.247037] sn65dsi83 13-002c: Dropping the link to panel-dsi-lvds
[   10.247049] device: 'platform:panel-dsi-lvds--i2c:13-002c': device_unregister
And here's the relevant portion of my device tree overlay:
Quoted 8 lines
Quoted 2 lines
--------------------8<--------------------
I think the eventual fix would be this series + adding a "post-init-providers" property to the device that's supposed to probe first and point it to the device that's supposed to probe next. Do this at the device node level, not the endpoint level.
https://lore.kernel.org/lkml/[email protected]/
I'm certainly going to look at this series in more detail and at the debugging you asked for, however I'm afraid I won't have access to the hardware this week and it's not going to be a quick task anyway.
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 6 lines
So in this moment I think it's quite clear that this specific patch creates a regression and there is no clear fix that is reasonably likely to get merged before 6.8.
I propose reverting this patch immediately, unless you have a better short-term solution.
It's just this one of the 3 patches that needs reverting?
Just this patch. I reverted only this and the issue disappeared.
Quoted 1 line
Quoted 1 line
I sent a fix. With the fix, it's just exposing a bug elsewhere.
Exactly, this patch has two issues and only the easy one has a fix [0] currently as far as I know.
Quoted 6 lines
You say apply the fix. Luca says revert. I say I wish I made this 6.9 material. Which is it?
If the overlay applying depends on out of tree code (likely as there are limited ways to apply an overlay in mainline), then I don't really care if there is still a regression.
Obviously, to load and unload the overlays I'm using code not yet in mainline. It is using of_overlay_fdt_apply() and of_overlay_remove() via a driver underdevelopment that is similar to the one Hervé and Lizhi Hou are working on [1][2].

Quoted 8 lines

I see the point that "we are not breaking existing use cases as no code is (un)loading overlays except unittest", sure.
As I see it, we have a feature in the kernel that is not used, but it will be, eventually: there are use cases, development is progressing and patches are being sent actively. My opinion is that we should not put additional known obstacles that will make it even harder than it already is.

Well, I don't care to do extra work of applying things and then have to turn right around fix or revert them. It happens enough as-is with just mainline. And no one wants to step up and fix the problems with overlays, but are fine just carrying their out of tree patches. What's one more. This is the 2nd case of overlay problems with out of tree users *today*! Some days I'm tempted to just remove overlay support altogether given the only way to apply them is unittest.

Given Geert is having issues too, I guess I'm going to revert.

Rob

Saravana Kannan

author

2024-02-29 22:54 UTC #11 lore raw

Re: [REGRESSION] Re: [PATCH v2 2/3] of: property: Improve finding the supplier of a remote-endpoint property

Quoted 229 lines · Rob Herring

Quoted 218 lines · Luca Ceresoli
Hi Rob, Saravana,
Quoted 184 lines · On Wed, 28 Feb 2024 18:26:36 -0600 Rob Herring <[email protected]> wrote:
Quoted 183 lines · Saravana Kannan
Quoted 181 lines · Rob Herring
Quoted 179 lines · Luca Ceresoli
Hello Saravana,
Quoted 160 lines · On Fri, 23 Feb 2024 17:35:24 -0800 Saravana Kannan <[email protected]> wrote:
Quoted 14 lines · Luca Ceresoli
Hello Saravana,
[+cc Hervé Codina]
Quoted 6 lines · On Tue, 6 Feb 2024 17:18:01 -0800 Saravana Kannan <[email protected]> wrote:
After commit 4a032827daa8 ("of: property: Simplify of_link_to_phandle()"), remote-endpoint properties created a fwnode link from the consumer device to the supplier endpoint. This is a tiny bit inefficient (not buggy) when trying to create device links or detecting cycles. So, improve this the same way we improved finding the consumer of a remote-endpoint property.
Quoted 7 lines
Quoted 2 lines
Fixes 4a032827daa8 ("of: property: Simplify of_link_to_phandle()")
Signed-off-by Saravana Kannan <[email protected]>
After rebasing my own branch on v6.8-rc5 from v6.8-rc1 I started getting unexpected warnings during device tree overlay removal. After a somewhat painful bisection I identified this patch as the one that triggers it all.
Thanks for the report.
Quoted 16 lines
Quoted 16 lines
---
drivers/of/property.c +0 -1
@@ -1232,7 +1232,6 @@ 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_SIMPLE_PROP(pwms, "pwms", "#pwm-cells")
 DEFINE_SIMPLE_PROP(resets, "resets", "#reset-cells")
 DEFINE_SIMPLE_PROP(leds, "leds", NULL)
@@ -1298,6 +1297,17 @@ static struct device_node *parse_interrupts(struct device_node *np,
      return of_irq_parse_one(np, index, &sup_args) ? NULL : sup_args.np;
 }
 
Quoted 9 lines
drivers/of/property.c static struct device_node *parse_interrupts(struct device_node *np, · line 1305
Quoted 6 lines
drivers/of/property.c +6 -0
+static struct device_node *parse_remote_endpoint(struct device_node *np,
+                                              const char *prop_name,
+                                              int index)
+{
+     /* Return NULL for index > 0 to signify end of remote-endpoints. */
+     if (!index || strcmp(prop_name, "remote-endpoint"))
There seem to be a bug here: "!index" should be "index > 0", as the comment suggests. Otherwise NULL is always returned.
Ah crap, I think you are right. It should have been "index". Not "!index". But I tested this! Sigh. I probably screwed up my testing.
Please send out a Fix for this.
Geert, we got excited too soon. :(
Quoted 5 lines
I am going to send a quick patch for that, but haven't done so yet because it still won't solve the problem, so I wanted to open the topic here without further delay.
Even with the 'index > 0' fix I'm still getting pretty much the same:
This part is confusing though. If I read your DT correctly, there's a cycle between platform:panel-dsi-lvds and i2c:13-002c. And fw_devlink should not be enforcing any ordering between those devices ever.
I'm surprised that in your "working" case, fw_devlink didn't detect any cycle. It should have. If there's any debugging to do, that's the one we need to debug.
Quoted 23 lines
[   34.836781] ------------[ cut here ]------------
[   34.841401] WARNING: CPU: 2 PID: 204 at drivers/base/devres.c:1064 devm_kfree+0x8c/0xfc
...
[   35.024751] Call trace:
[   35.027199]  devm_kfree+0x8c/0xfc
[   35.030520]  devm_drm_panel_bridge_release+0x54/0x64 [drm_kms_helper]
[   35.036990]  devres_release_group+0xe0/0x164
[   35.041264]  i2c_device_remove+0x38/0x9c
[   35.045196]  device_remove+0x4c/0x80
[   35.048774]  device_release_driver_internal+0x1d4/0x230
[   35.054003]  device_release_driver+0x18/0x24
[   35.058279]  bus_remove_device+0xcc/0x10c
[   35.062292]  device_del+0x15c/0x41c
[   35.065786]  device_unregister+0x18/0x34
[   35.069714]  i2c_unregister_device+0x54/0x88
[   35.073988]  of_i2c_notify+0x98/0x224
[   35.077656]  blocking_notifier_call_chain+0x6c/0xa0
[   35.082543]  __of_changeset_entry_notify+0x100/0x16c
[   35.087515]  __of_changeset_revert_notify+0x44/0x78
[   35.092398]  of_overlay_remove+0x114/0x1c4
...
Quoted 7 lines
By comparing the two versions I found that before removing the overlay:
* in the "working" case (with this patch reverted) I have:
   # ls /sys/class/devlink/ | grep 002c
   platform:hpbr--i2c:13-002c
   platform:panel-dsi-lvds--i2c:13-002c
Can you check the "status" and "sync_state_only" file in this folder and tell me what it says?
Since these devices have a cyclic dependency between them, it should have been something other than "not tracked" and "sync_state_only" should be "1". But my guess is you'll see "active" and "0".
Quoted 16 lines
   platform:regulator-sys-1v8--i2c:13-002c
   regulator:regulator.31--i2c:13-002c
   #
* in the "broken" case (v6.8-rc5 + s/!index/index > 0/ as mentioned):
   # ls /sys/class/devlink/ | grep 002c
   platform:hpbr--i2c:13-002c
   platform:regulator-sys-1v8--i2c:13-002c
   regulator:regulator.30--i2c:13-002c
   #
So in the latter case the panel-dsi-lvds--i2c:13-002c link is missing. I think it gets created but later on removed. Here's a snippet of the kernel log when that happens:
Quoted 6 lines
[    9.578279] ----- cycle: start -----
[    9.578283] /soc@0/bus@30800000/i2c@30ad0000/i2cmux@70/i2c@3/dsi-lvds-bridge@2c: cycle: depends on /panel-dsi-lvds
[    9.578308] /panel-dsi-lvds: cycle: depends on /soc@0/bus@30800000/i2c@30ad0000/i2cmux@70/i2c@3/dsi-lvds-bridge@2c
[    9.578329] ----- cycle: end -----
[    9.578334] platform panel-dsi-lvds: Fixed dependency cycle(s) with /soc@0/bus@30800000/i2c@30ad0000/i2cmux@70/i2c@3/dsi-lvds-bridge@2c
...
Somewhere in this area, I'm thinking you'll also see "device:
'i2c:13-002c--platform:panel-dsi-lvds': device_add" do you not? And if you enabled device link logs, you'll see that it was "sync state only" link.
Quoted 6 lines
[    9.590620] /panel-dsi-lvds Dropping the fwnode link to /soc@0/bus@30800000/i2c@30ad0000/i2cmux@70/i2c@3/dsi-lvds-bridge@2c
...
[    9.597280] ----- cycle: start -----
[    9.597283] /panel-dsi-lvds: cycle: depends on /soc@0/bus@30800000/i2c@30ad0000/i2cmux@70/i2c@3/dsi-lvds-bridge@2c
[    9.602781] /soc@0/bus@30800000/i2c@30ad0000/i2cmux@70/i2c@3/dsi-lvds-bridge@2c: cycle: depends on /panel-dsi-lvds
[    9.607581] ----- cycle: end -----
Quoted 8 lines
[    9.607585] i2c 13-002c: Fixed dependency cycle(s) with /panel-dsi-lvds
[    9.614217] device: 'platform:panel-dsi-lvds--i2c:13-002c': device_add
...
[    9.614277] /soc@0/bus@30800000/i2c@30ad0000/i2cmux@70/i2c@3/dsi-lvds-bridge@2c Dropping the fwnode link to /panel-dsi-lvds
[    9.614369] /soc@0/bus@30800000/i2c@30ad0000/i2cmux@70/i2c@3/dsi-lvds-bridge@2c Dropping the fwnode link to /regulator-dock-sys-1v8
...
[    9.739840] panel-simple panel-dsi-lvds: Dropping the link to 13-002c
[    9.739846] device: 'i2c:13-002c--platform:panel-dsi-lvds': device_unregister
Oh yeah, see. The "device_add" I expected earlier is getting removed here.
Quoted 5 lines
[   10.247037] sn65dsi83 13-002c: Dropping the link to panel-dsi-lvds
[   10.247049] device: 'platform:panel-dsi-lvds--i2c:13-002c': device_unregister
And here's the relevant portion of my device tree overlay:
Quoted 8 lines
Quoted 2 lines
--------------------8<--------------------
I think the eventual fix would be this series + adding a "post-init-providers" property to the device that's supposed to probe first and point it to the device that's supposed to probe next. Do this at the device node level, not the endpoint level.
https://lore.kernel.org/lkml/[email protected]/
I'm certainly going to look at this series in more detail and at the debugging you asked for, however I'm afraid I won't have access to the hardware this week and it's not going to be a quick task anyway.
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 6 lines
So in this moment I think it's quite clear that this specific patch creates a regression and there is no clear fix that is reasonably likely to get merged before 6.8.
I propose reverting this patch immediately, unless you have a better short-term solution.
It's just this one of the 3 patches that needs reverting?
Just this patch. I reverted only this and the issue disappeared.
Quoted 1 line
Quoted 1 line
I sent a fix. With the fix, it's just exposing a bug elsewhere.
Exactly, this patch has two issues and only the easy one has a fix [0] currently as far as I know.
Quoted 6 lines
You say apply the fix. Luca says revert. I say I wish I made this 6.9 material. Which is it?
If the overlay applying depends on out of tree code (likely as there are limited ways to apply an overlay in mainline), then I don't really care if there is still a regression.
Obviously, to load and unload the overlays I'm using code not yet in mainline. It is using of_overlay_fdt_apply() and of_overlay_remove() via a driver underdevelopment that is similar to the one Hervé and Lizhi Hou are working on [1][2].
Quoted 8 lines
I see the point that "we are not breaking existing use cases as no code is (un)loading overlays except unittest", sure.
As I see it, we have a feature in the kernel that is not used, but it will be, eventually: there are use cases, development is progressing and patches are being sent actively. My opinion is that we should not put additional known obstacles that will make it even harder than it already is.

Quoted 7 lines

Well, I don't care to do extra work of applying things and then have to turn right around fix or revert them. It happens enough as-is with just mainline. And no one wants to step up and fix the problems with overlays, but are fine just carrying their out of tree patches. What's one more. This is the 2nd case of overlay problems with out of tree users *today*! Some days I'm tempted to just remove overlay support altogether given the only way to apply them is unittest.

Rob,

Sorry I couldn't reply yesterday. And sorry for getting this into 6.8 and causing headaches for you.

With [1], there are no more bugs to fix in fw_devlink wrt remote-endpoints for sure.
[1] - https://lore.kernel.org/lkml/[email protected]/

It's solely exposing a bug in another driver. If this was upstream code, I might have been okay with reverting things just to make their bug for now. I didn't realize this was downstream stuff until you asked/Luca confirmed. We definitely shouldn't revert anything. Luca can take my pointers and debug their driver and I'm happy to help debug this further.

Also, post-init-providers should definitely help in this case. So, Luca can use that once we land it.

Quoted 1 line

Given Geert is having issues too, I guess I'm going to revert.

It's just extra/explicit logging because as the original series was meant to do, it improves remote-enpoint parsing.

-Saravana

Luca Ceresoli

2024-03-01 09:59 UTC #12 lore raw

Re: [REGRESSION] Re: [PATCH v2 2/3] of: property: Improve finding the supplier of a remote-endpoint property

In reply to Rob Herring

Hello Rob,

On Thu, 29 Feb 2024 16:10:38 -0600
Rob Herring <[email protected]> wrote:
[...]

Quoted 30 lines

Quoted 21 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
It's just this one of the 3 patches that needs reverting?
Just this patch. I reverted only this and the issue disappeared.
Quoted 1 line
Quoted 1 line
I sent a fix. With the fix, it's just exposing a bug elsewhere.
Exactly, this patch has two issues and only the easy one has a fix [0] currently as far as I know.
Quoted 6 lines
You say apply the fix. Luca says revert. I say I wish I made this 6.9 material. Which is it?
If the overlay applying depends on out of tree code (likely as there are limited ways to apply an overlay in mainline), then I don't really care if there is still a regression.
Obviously, to load and unload the overlays I'm using code not yet in mainline. It is using of_overlay_fdt_apply() and of_overlay_remove() via a driver underdevelopment that is similar to the one Hervé and Lizhi Hou are working on [1][2].
Quoted 8 lines
I see the point that "we are not breaking existing use cases as no code is (un)loading overlays except unittest", sure.
As I see it, we have a feature in the kernel that is not used, but it will be, eventually: there are use cases, development is progressing and patches are being sent actively. My opinion is that we should not put additional known obstacles that will make it even harder than it already is.

Quoted 7 lines

Well, I don't care to do extra work of applying things and then have to turn right around fix or revert them. It happens enough as-is with just mainline. And no one wants to step up and fix the problems with overlays, but are fine just carrying their out of tree patches. What's one more. This is the 2nd case of overlay problems with out of tree users *today*! Some days I'm tempted to just remove overlay support altogether given the only way to apply them is unittest.

Thanks for taking time to understand the situation.

Just to clarify my position: together with Hervé we are not just carrying out of tree code, we are actively developing code that uses overlay load/unload at runtime and we will send it to mainline as soon as it is ready.

As part of this process, Hervé has already sent patches to fix various problems that happen when overlays are loaded and especially unloaded:
https://lore.kernel.org/all/[email protected]/
https://lore.kernel.org/all/[email protected]/
https://lore.kernel.org/all/[email protected]/
https://lore.kernel.org/all/[email protected]/

Best regards,
Luca

Signature · 3 lines

Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Rob Herring

2024-02-28 22:01 UTC #13 lore raw

Re: [PATCH v2 2/3] of: property: Improve finding the supplier of a remote-endpoint property

In reply to Luca Ceresoli

Quoted 14 lines · Luca Ceresoli

Hello Saravana,
[+cc Hervé Codina]
Quoted 6 lines · On Tue, 6 Feb 2024 17:18:01 -0800 Saravana Kannan <[email protected]> wrote:
After commit 4a032827daa8 ("of: property: Simplify of_link_to_phandle()"), remote-endpoint properties created a fwnode link from the consumer device to the supplier endpoint. This is a tiny bit inefficient (not buggy) when trying to create device links or detecting cycles. So, improve this the same way we improved finding the consumer of a remote-endpoint property.

Quoted 7 lines

Quoted 2 lines
Fixes 4a032827daa8 ("of: property: Simplify of_link_to_phandle()")
Signed-off-by Saravana Kannan <[email protected]>
After rebasing my own branch on v6.8-rc5 from v6.8-rc1 I started getting unexpected warnings during device tree overlay removal. After a somewhat painful bisection I identified this patch as the one that triggers it all.

How are you applying the overlay?

Rob

Saravana Kannan

author

2024-02-07 01:18 UTC #14 lore raw

PATCH v2 3/3 of: property: Add in-ports/out-ports support to of_graph_get_port_parent()

In reply to Saravana Kannan

Similar to the existing "ports" node name, coresight device tree bindings have added "in-ports" and "out-ports" as standard node names for a collection of ports.

Add support for these name to of_graph_get_port_parent() so that remote-endpoint parsing can find the correct parent node for these coresight ports too.

Signed-off-by Saravana Kannan <[email protected]>

---
 drivers/of/property.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

drivers/of/property.c +3 -1

index 7bb2d8e290de..39a3ee1dfb58 100644
@@ -763,7 +763,9 @@ struct device_node *of_graph_get_port_parent(struct device_node *node)
 	/* Walk 3 levels up only if there is 'ports' node. */
 	for (depth = 3; depth && node; depth--) {
 		node = of_get_next_parent(node);
-		if (depth == 2 && !of_node_name_eq(node, "ports"))
+		if (depth == 2 && !of_node_name_eq(node, "ports") &&
+		    !of_node_name_eq(node, "in-ports") &&
+		    !of_node_name_eq(node, "out-ports"))
 			break;
 	}
 	return node;

Signature · 1 line

2.43.0.594.gd9cf4e227d-goog

Saravana Kannan

author

2024-02-21 00:48 UTC #15 lore raw

Re: [PATCH v2 3/3] of: property: Add in-ports/out-ports support to of_graph_get_port_parent()

Quoted 10 lines · Saravana Kannan

Similar to the existing "ports" node name, coresight device tree bindings have added "in-ports" and "out-ports" as standard node names for a collection of ports.
Add support for these name to of_graph_get_port_parent() so that remote-endpoint parsing can find the correct parent node for these coresight ports too.
Signed-off-by Saravana Kannan <[email protected]>

Greg,

I saw that you pulled the previous 2 patches in this series to 6.1, 6.6 and 6.7 kernel branches. I really should have added both of those Fixes tag to this patch too.

Can you please pull in the patch to those stable branches too?

Thanks,
Saravana

Quoted 22 lines

---
 drivers/of/property.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

drivers/of/property.c +3 -2

index 7bb2d8e290de..39a3ee1dfb58 100644
@@ -763,7 +763,9 @@ struct device_node *of_graph_get_port_parent(struct device_node *node)
        /* Walk 3 levels up only if there is 'ports' node. */
        for (depth = 3; depth && node; depth--) {
                node = of_get_next_parent(node);
-               if (depth == 2 && !of_node_name_eq(node, "ports"))
+               if (depth == 2 && !of_node_name_eq(node, "ports") &&
+                   !of_node_name_eq(node, "in-ports") &&
+                   !of_node_name_eq(node, "out-ports"))
                        break;
        }
        return node;
--

2.43.0.594.gd9cf4e227d-goog

Greg Kroah-Hartman

maintainer

2024-02-21 07:25 UTC #16 lore raw

Re: [PATCH v2 3/3] of: property: Add in-ports/out-ports support to of_graph_get_port_parent()

Quoted 12 lines · Saravana Kannan

Quoted 10 lines · Saravana Kannan
Similar to the existing "ports" node name, coresight device tree bindings have added "in-ports" and "out-ports" as standard node names for a collection of ports.
Add support for these name to of_graph_get_port_parent() so that remote-endpoint parsing can find the correct parent node for these coresight ports too.
Signed-off-by Saravana Kannan <[email protected]>

Quoted 7 lines

Greg,
I saw that you pulled the previous 2 patches in this series to 6.1, 6.6 and 6.7 kernel branches. I really should have added both of those Fixes tag to this patch too.
Can you please pull in the patch to those stable branches too?

Sure, what's the git id?

thanks,

greg k-h

Greg Kroah-Hartman

maintainer

2024-02-21 08:38 UTC #17 lore raw

Re: [PATCH v2 3/3] of: property: Add in-ports/out-ports support to of_graph_get_port_parent()

Quoted 15 lines · Greg Kroah-Hartman

Quoted 14 lines · Saravana Kannan
Quoted 10 lines · Saravana Kannan
Similar to the existing "ports" node name, coresight device tree bindings have added "in-ports" and "out-ports" as standard node names for a collection of ports.
Add support for these name to of_graph_get_port_parent() so that remote-endpoint parsing can find the correct parent node for these coresight ports too.
Signed-off-by Saravana Kannan <[email protected]>
Greg,

Quoted 7 lines

Quoted 5 lines
I saw that you pulled the previous 2 patches in this series to 6.1, 6.6 and 6.7 kernel branches. I really should have added both of those Fixes tag to this patch too.
Can you please pull in the patch to those stable branches too?
Sure, what's the git id?

Nevermind, I found it...

Rob Herring

2024-02-07 08:11 UTC #18 lore raw

In reply to Saravana Kannan

Quoted 8 lines · Saravana Kannan

Some changes to do a more accurate parsing of remote-endpoints. Making fw_devlink a tiny bit more efficient and the debug logs a bit cleaner when trying to debug fw_devlink.
Rob,
Can we get this into 6.8-rcX please?

I'm failing to see how this is 6.8 material?

Rob

Saravana Kannan

author

2024-02-07 21:07 UTC #19 lore raw

Quoted 7 lines · Rob Herring

Quoted 5 lines · Saravana Kannan
Some changes to do a more accurate parsing of remote-endpoints. Making fw_devlink a tiny bit more efficient and the debug logs a bit cleaner when trying to debug fw_devlink.

Quoted 5 lines

Quoted 3 lines
Rob,
Can we get this into 6.8-rcX please?
I'm failing to see how this is 6.8 material?

It's a bug fix that's making the remote-endpoint tracking more accurate. I thought stuff like this would be okay for rcX. But if you want to hold it for later, that's ok too.

-Saravana

Rob Herring

maintainer Applied

2024-02-09 10:31 UTC #20 lore raw

In reply to Saravana Kannan

Quoted 23 lines · Saravana Kannan

Some changes to do a more accurate parsing of remote-endpoints. Making fw_devlink a tiny bit more efficient and the debug logs a bit cleaner when trying to debug fw_devlink.
Rob,
Can we get this into 6.8-rcX please?
Thanks,
Saravana
v1->v2:
- Switched from fwnode_graph_get_port_parent() to of_graph_get_port_parent()
- Added Patch 3
Saravana Kannan (3):
of: property: Improve finding the consumer of a remote-endpoint
property
of: property: Improve finding the supplier of a remote-endpoint
property
of: property: Add in-ports/out-ports support to
of_graph_get_port_parent()

Quoted 8 lines

 drivers/of/property.c | 63 +++++++++++++++++--------------------------
 1 file changed, 24 insertions(+), 39 deletions(-)

--
2.43.0.594.gd9cf4e227d-goog

Applied, thanks!