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

[Luca Ceresoli <luca.ceresoli@bootlin.com>]

Hello Saravana,

[+cc Hervé Codina]

On Tue,  6 Feb 2024 17:18:01 -0800
Saravana Kannan <saravanak@google.com> 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 <saravanak@google.com>

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.

> ---
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -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"))

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

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