From: Luca Ceresoli <luca.ceresoli@bootlin.com>
To: Saravana Kannan <saravanak@google.com>
Cc: "Rob Herring" <robh+dt@kernel.org>,
"Frank Rowand" <frowand.list@gmail.com>,
"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
"Xu Yang" <xu.yang_2@nxp.com>,
kernel-team@android.com, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org,
"Hervé Codina" <herve.codina@bootlin.com>
Subject: Re: [PATCH v2 2/3] of: property: Improve finding the supplier of a remote-endpoint property
Date: Fri, 23 Feb 2024 17:18:49 +0100 [thread overview]
Message-ID: <20240223171849.10f9901d@booty> (raw)
In-Reply-To: <20240207011803.2637531-3-saravanak@google.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 = <®_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
next prev parent reply other threads:[~2024-02-23 16:18 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-07 1:17 [PATCH v2 0/3] Improve remote-endpoint parsing Saravana Kannan
2024-02-07 1:18 ` [PATCH v2 1/3] of: property: Improve finding the consumer of a remote-endpoint property Saravana Kannan
2024-02-07 1:18 ` [PATCH v2 2/3] of: property: Improve finding the supplier " Saravana Kannan
2024-02-23 16:18 ` Luca Ceresoli [this message]
2024-02-24 1:35 ` Saravana Kannan
2024-02-26 11:52 ` [REGRESSION] " Luca Ceresoli
2024-02-28 21:56 ` Rob Herring
2024-02-28 23:58 ` Saravana Kannan
2024-02-29 0:26 ` Rob Herring
2024-02-29 9:34 ` Luca Ceresoli
2024-02-29 22:10 ` Rob Herring
2024-02-29 22:54 ` Saravana Kannan
2024-03-01 9:59 ` Luca Ceresoli
2024-02-28 22:01 ` Rob Herring
2024-02-07 1:18 ` [PATCH v2 3/3] of: property: Add in-ports/out-ports support to of_graph_get_port_parent() Saravana Kannan
2024-02-21 0:47 ` Saravana Kannan
2024-02-21 7:00 ` Greg Kroah-Hartman
2024-02-21 8:38 ` Greg Kroah-Hartman
2024-02-07 8:10 ` [PATCH v2 0/3] Improve remote-endpoint parsing Rob Herring
2024-02-07 21:07 ` Saravana Kannan
2024-02-09 10:31 ` Rob Herring
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240223171849.10f9901d@booty \
--to=luca.ceresoli@bootlin.com \
--cc=devicetree@vger.kernel.org \
--cc=frowand.list@gmail.com \
--cc=gregkh@linuxfoundation.org \
--cc=herve.codina@bootlin.com \
--cc=kernel-team@android.com \
--cc=linux-kernel@vger.kernel.org \
--cc=robh+dt@kernel.org \
--cc=saravanak@google.com \
--cc=xu.yang_2@nxp.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).