* [PATCH v2 0/3] Improve remote-endpoint parsing
@ 2024-02-07 1:17 Saravana Kannan
2024-02-07 1:18 ` [PATCH v2 1/3] of: property: Improve finding the consumer of a remote-endpoint property Saravana Kannan
` (4 more replies)
0 siblings, 5 replies; 21+ messages in thread
From: Saravana Kannan @ 2024-02-07 1:17 UTC (permalink / raw)
To: Rob Herring, Frank Rowand, Greg Kroah-Hartman, Saravana Kannan
Cc: Xu Yang, kernel-team, devicetree, linux-kernel
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(-)
--
2.43.0.594.gd9cf4e227d-goog
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2 1/3] of: property: Improve finding the consumer of a remote-endpoint property
2024-02-07 1:17 [PATCH v2 0/3] Improve remote-endpoint parsing Saravana Kannan
@ 2024-02-07 1:18 ` Saravana Kannan
2024-02-07 1:18 ` [PATCH v2 2/3] of: property: Improve finding the supplier " Saravana Kannan
` (3 subsequent siblings)
4 siblings, 0 replies; 21+ messages in thread
From: Saravana Kannan @ 2024-02-07 1:18 UTC (permalink / raw)
To: Rob Herring, Frank Rowand, Greg Kroah-Hartman, Saravana Kannan
Cc: Xu Yang, kernel-team, devicetree, linux-kernel
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 <saravanak@google.com>
---
drivers/of/property.c | 47 +++++++++----------------------------------
1 file changed, 10 insertions(+), 37 deletions(-)
diff --git a/drivers/of/property.c b/drivers/of/property.c
index 641a40cf5cf3..da70aaa62ca3 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -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++;
--
2.43.0.594.gd9cf4e227d-goog
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v2 2/3] of: property: Improve finding the supplier of a remote-endpoint property
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 ` Saravana Kannan
2024-02-23 16:18 ` Luca Ceresoli
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
` (2 subsequent siblings)
4 siblings, 1 reply; 21+ messages in thread
From: Saravana Kannan @ 2024-02-07 1:18 UTC (permalink / raw)
To: Rob Herring, Frank Rowand, Greg Kroah-Hartman, Saravana Kannan
Cc: Xu Yang, kernel-team, devicetree, linux-kernel
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>
---
drivers/of/property.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/drivers/of/property.c b/drivers/of/property.c
index da70aaa62ca3..7bb2d8e290de 100644
--- 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"))
+ 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, },
--
2.43.0.594.gd9cf4e227d-goog
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v2 3/3] of: property: Add in-ports/out-ports support to of_graph_get_port_parent()
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-07 1:18 ` Saravana Kannan
2024-02-21 0:47 ` Saravana Kannan
2024-02-07 8:10 ` [PATCH v2 0/3] Improve remote-endpoint parsing Rob Herring
2024-02-09 10:31 ` Rob Herring
4 siblings, 1 reply; 21+ messages in thread
From: Saravana Kannan @ 2024-02-07 1:18 UTC (permalink / raw)
To: Rob Herring, Frank Rowand, Greg Kroah-Hartman, Saravana Kannan
Cc: Xu Yang, kernel-team, devicetree, linux-kernel
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 <saravanak@google.com>
---
drivers/of/property.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/of/property.c b/drivers/of/property.c
index 7bb2d8e290de..39a3ee1dfb58 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -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
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v2 0/3] Improve remote-endpoint parsing
2024-02-07 1:17 [PATCH v2 0/3] Improve remote-endpoint parsing Saravana Kannan
` (2 preceding siblings ...)
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-07 8:10 ` Rob Herring
2024-02-07 21:07 ` Saravana Kannan
2024-02-09 10:31 ` Rob Herring
4 siblings, 1 reply; 21+ messages in thread
From: Rob Herring @ 2024-02-07 8:10 UTC (permalink / raw)
To: Saravana Kannan
Cc: Frank Rowand, Greg Kroah-Hartman, Xu Yang, kernel-team,
devicetree, linux-kernel
On Wed, Feb 7, 2024 at 1:18 AM Saravana Kannan <saravanak@google.com> wrote:
>
> 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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 0/3] Improve remote-endpoint parsing
2024-02-07 8:10 ` [PATCH v2 0/3] Improve remote-endpoint parsing Rob Herring
@ 2024-02-07 21:07 ` Saravana Kannan
0 siblings, 0 replies; 21+ messages in thread
From: Saravana Kannan @ 2024-02-07 21:07 UTC (permalink / raw)
To: Rob Herring
Cc: Frank Rowand, Greg Kroah-Hartman, Xu Yang, kernel-team,
devicetree, linux-kernel
On Wed, Feb 7, 2024 at 12:11 AM Rob Herring <robh+dt@kernel.org> wrote:
>
> On Wed, Feb 7, 2024 at 1:18 AM Saravana Kannan <saravanak@google.com> wrote:
> >
> > 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?
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 0/3] Improve remote-endpoint parsing
2024-02-07 1:17 [PATCH v2 0/3] Improve remote-endpoint parsing Saravana Kannan
` (3 preceding siblings ...)
2024-02-07 8:10 ` [PATCH v2 0/3] Improve remote-endpoint parsing Rob Herring
@ 2024-02-09 10:31 ` Rob Herring
4 siblings, 0 replies; 21+ messages in thread
From: Rob Herring @ 2024-02-09 10:31 UTC (permalink / raw)
To: Saravana Kannan
Cc: Rob Herring, Frank Rowand, devicetree, linux-kernel, kernel-team,
Greg Kroah-Hartman, Xu Yang
On Tue, 06 Feb 2024 17:17:59 -0800, Saravana Kannan wrote:
> 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(-)
>
> --
> 2.43.0.594.gd9cf4e227d-goog
>
>
>
Applied, thanks!
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 3/3] of: property: Add in-ports/out-ports support to of_graph_get_port_parent()
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
0 siblings, 1 reply; 21+ messages in thread
From: Saravana Kannan @ 2024-02-21 0:47 UTC (permalink / raw)
To: Rob Herring, Frank Rowand, Greg Kroah-Hartman, Saravana Kannan, stable
Cc: Xu Yang, kernel-team, devicetree, linux-kernel
On Tue, Feb 6, 2024 at 5:18 PM Saravana Kannan <saravanak@google.com> wrote:
>
> 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 <saravanak@google.com>
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
> ---
> drivers/of/property.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index 7bb2d8e290de..39a3ee1dfb58 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -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
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 3/3] of: property: Add in-ports/out-ports support to of_graph_get_port_parent()
2024-02-21 0:47 ` Saravana Kannan
@ 2024-02-21 7:00 ` Greg Kroah-Hartman
2024-02-21 8:38 ` Greg Kroah-Hartman
0 siblings, 1 reply; 21+ messages in thread
From: Greg Kroah-Hartman @ 2024-02-21 7:00 UTC (permalink / raw)
To: Saravana Kannan
Cc: Rob Herring, Frank Rowand, stable, Xu Yang, kernel-team,
devicetree, linux-kernel
On Tue, Feb 20, 2024 at 04:47:35PM -0800, Saravana Kannan wrote:
> On Tue, Feb 6, 2024 at 5:18 PM Saravana Kannan <saravanak@google.com> wrote:
> >
> > 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 <saravanak@google.com>
>
> 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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 3/3] of: property: Add in-ports/out-ports support to of_graph_get_port_parent()
2024-02-21 7:00 ` Greg Kroah-Hartman
@ 2024-02-21 8:38 ` Greg Kroah-Hartman
0 siblings, 0 replies; 21+ messages in thread
From: Greg Kroah-Hartman @ 2024-02-21 8:38 UTC (permalink / raw)
To: Saravana Kannan
Cc: Rob Herring, Frank Rowand, stable, Xu Yang, kernel-team,
devicetree, linux-kernel
On Wed, Feb 21, 2024 at 08:00:00AM +0100, Greg Kroah-Hartman wrote:
> On Tue, Feb 20, 2024 at 04:47:35PM -0800, Saravana Kannan wrote:
> > On Tue, Feb 6, 2024 at 5:18 PM Saravana Kannan <saravanak@google.com> wrote:
> > >
> > > 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 <saravanak@google.com>
> >
> > 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?
Nevermind, I found it...
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 2/3] of: property: Improve finding the supplier of a remote-endpoint property
2024-02-07 1:18 ` [PATCH v2 2/3] of: property: Improve finding the supplier " Saravana Kannan
@ 2024-02-23 16:18 ` Luca Ceresoli
2024-02-24 1:35 ` Saravana Kannan
2024-02-28 22:01 ` Rob Herring
0 siblings, 2 replies; 21+ messages in thread
From: Luca Ceresoli @ 2024-02-23 16:18 UTC (permalink / raw)
To: Saravana Kannan
Cc: Rob Herring, Frank Rowand, Greg Kroah-Hartman, Xu Yang,
kernel-team, devicetree, linux-kernel, Hervé Codina
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 2/3] of: property: Improve finding the supplier of a remote-endpoint property
2024-02-23 16:18 ` Luca Ceresoli
@ 2024-02-24 1:35 ` Saravana Kannan
2024-02-26 11:52 ` [REGRESSION] " Luca Ceresoli
2024-02-28 22:01 ` Rob Herring
1 sibling, 1 reply; 21+ messages in thread
From: Saravana Kannan @ 2024-02-24 1:35 UTC (permalink / raw)
To: Luca Ceresoli
Cc: Rob Herring, Frank Rowand, Greg Kroah-Hartman, Xu Yang,
kernel-team, devicetree, linux-kernel, Hervé Codina,
Geert Uytterhoeven
On Fri, Feb 23, 2024 at 8:18 AM Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:
>
> 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.
Thanks for the report.
>
> > ---
> > --- 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.
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. :(
> 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.
>
> [ 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
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".
> 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
> ...
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.
> [ 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
Oh yeah, see. The "device_add" I expected earlier is getting removed here.
> [ 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/20240221233026.2915061-1-saravanak@google.com/
-Saravana
> /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
^ permalink raw reply [flat|nested] 21+ messages in thread
* [REGRESSION] Re: [PATCH v2 2/3] of: property: Improve finding the supplier of a remote-endpoint property
2024-02-24 1:35 ` Saravana Kannan
@ 2024-02-26 11:52 ` Luca Ceresoli
2024-02-28 21:56 ` Rob Herring
0 siblings, 1 reply; 21+ messages in thread
From: Luca Ceresoli @ 2024-02-26 11:52 UTC (permalink / raw)
To: Saravana Kannan
Cc: Rob Herring, Frank Rowand, Greg Kroah-Hartman, Xu Yang,
kernel-team, devicetree, linux-kernel, Hervé Codina,
Geert Uytterhoeven
Hello Saravana,
On Fri, 23 Feb 2024 17:35:24 -0800
Saravana Kannan <saravanak@google.com> wrote:
> On Fri, Feb 23, 2024 at 8:18 AM Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:
> >
> > 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.
>
> Thanks for the report.
>
> >
> > > ---
> > > --- 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.
>
> 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. :(
>
> > 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.
>
> >
> > [ 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
>
> 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".
>
> > 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
> > ...
>
> 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.
>
> > [ 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
>
> Oh yeah, see. The "device_add" I expected earlier is getting removed here.
>
> > [ 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/20240221233026.2915061-1-saravanak@google.com/
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
--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [REGRESSION] Re: [PATCH v2 2/3] of: property: Improve finding the supplier of a remote-endpoint property
2024-02-26 11:52 ` [REGRESSION] " Luca Ceresoli
@ 2024-02-28 21:56 ` Rob Herring
2024-02-28 23:58 ` Saravana Kannan
0 siblings, 1 reply; 21+ messages in thread
From: Rob Herring @ 2024-02-28 21:56 UTC (permalink / raw)
To: Luca Ceresoli
Cc: Saravana Kannan, Frank Rowand, Greg Kroah-Hartman, Xu Yang,
kernel-team, devicetree, linux-kernel, Hervé Codina,
Geert Uytterhoeven
On Mon, Feb 26, 2024 at 5:52 AM Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:
>
> Hello Saravana,
>
> On Fri, 23 Feb 2024 17:35:24 -0800
> Saravana Kannan <saravanak@google.com> wrote:
>
> > On Fri, Feb 23, 2024 at 8:18 AM Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:
> > >
> > > 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.
> >
> > Thanks for the report.
> >
> > >
> > > > ---
> > > > --- 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.
> >
> > 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. :(
> >
> > > 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.
> >
> > >
> > > [ 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
> >
> > 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".
> >
> > > 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
> > > ...
> >
> > 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.
> >
> > > [ 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
> >
> > Oh yeah, see. The "device_add" I expected earlier is getting removed here.
> >
> > > [ 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/20240221233026.2915061-1-saravanak@google.com/
>
> 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.
It's just this one of the 3 patches that needs reverting?
Rob
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 2/3] of: property: Improve finding the supplier of a remote-endpoint property
2024-02-23 16:18 ` Luca Ceresoli
2024-02-24 1:35 ` Saravana Kannan
@ 2024-02-28 22:01 ` Rob Herring
1 sibling, 0 replies; 21+ messages in thread
From: Rob Herring @ 2024-02-28 22:01 UTC (permalink / raw)
To: Luca Ceresoli
Cc: Saravana Kannan, Frank Rowand, Greg Kroah-Hartman, Xu Yang,
kernel-team, devicetree, linux-kernel, Hervé Codina
On Fri, Feb 23, 2024 at 10:18 AM Luca Ceresoli
<luca.ceresoli@bootlin.com> wrote:
>
> 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.
How are you applying the overlay?
Rob
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [REGRESSION] Re: [PATCH v2 2/3] of: property: Improve finding the supplier of a remote-endpoint property
2024-02-28 21:56 ` Rob Herring
@ 2024-02-28 23:58 ` Saravana Kannan
2024-02-29 0:26 ` Rob Herring
0 siblings, 1 reply; 21+ messages in thread
From: Saravana Kannan @ 2024-02-28 23:58 UTC (permalink / raw)
To: Rob Herring
Cc: Luca Ceresoli, Frank Rowand, Greg Kroah-Hartman, Xu Yang,
kernel-team, devicetree, linux-kernel, Hervé Codina,
Geert Uytterhoeven
On Wed, Feb 28, 2024 at 1:56 PM Rob Herring <robh+dt@kernel.org> wrote:
>
> On Mon, Feb 26, 2024 at 5:52 AM Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:
> >
> > Hello Saravana,
> >
> > On Fri, 23 Feb 2024 17:35:24 -0800
> > Saravana Kannan <saravanak@google.com> wrote:
> >
> > > On Fri, Feb 23, 2024 at 8:18 AM Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:
> > > >
> > > > 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.
> > >
> > > Thanks for the report.
> > >
> > > >
> > > > > ---
> > > > > --- 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.
> > >
> > > 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. :(
> > >
> > > > 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.
> > >
> > > >
> > > > [ 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
> > >
> > > 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".
> > >
> > > > 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
> > > > ...
> > >
> > > 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.
> > >
> > > > [ 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
> > >
> > > Oh yeah, see. The "device_add" I expected earlier is getting removed here.
> > >
> > > > [ 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/20240221233026.2915061-1-saravanak@google.com/
> >
> > 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.
>
> 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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [REGRESSION] Re: [PATCH v2 2/3] of: property: Improve finding the supplier of a remote-endpoint property
2024-02-28 23:58 ` Saravana Kannan
@ 2024-02-29 0:26 ` Rob Herring
2024-02-29 9:34 ` Luca Ceresoli
0 siblings, 1 reply; 21+ messages in thread
From: Rob Herring @ 2024-02-29 0:26 UTC (permalink / raw)
To: Saravana Kannan
Cc: Luca Ceresoli, Frank Rowand, Greg Kroah-Hartman, Xu Yang,
kernel-team, devicetree, linux-kernel, Hervé Codina,
Geert Uytterhoeven
On Wed, Feb 28, 2024 at 5:58 PM Saravana Kannan <saravanak@google.com> wrote:
>
> On Wed, Feb 28, 2024 at 1:56 PM Rob Herring <robh+dt@kernel.org> wrote:
> >
> > On Mon, Feb 26, 2024 at 5:52 AM Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:
> > >
> > > Hello Saravana,
> > >
> > > On Fri, 23 Feb 2024 17:35:24 -0800
> > > Saravana Kannan <saravanak@google.com> wrote:
> > >
> > > > On Fri, Feb 23, 2024 at 8:18 AM Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:
> > > > >
> > > > > 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.
> > > >
> > > > Thanks for the report.
> > > >
> > > > >
> > > > > > ---
> > > > > > --- 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.
> > > >
> > > > 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. :(
> > > >
> > > > > 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.
> > > >
> > > > >
> > > > > [ 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
> > > >
> > > > 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".
> > > >
> > > > > 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
> > > > > ...
> > > >
> > > > 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.
> > > >
> > > > > [ 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
> > > >
> > > > Oh yeah, see. The "device_add" I expected earlier is getting removed here.
> > > >
> > > > > [ 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/20240221233026.2915061-1-saravanak@google.com/
> > >
> > > 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.
> >
> > 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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [REGRESSION] Re: [PATCH v2 2/3] of: property: Improve finding the supplier of a remote-endpoint property
2024-02-29 0:26 ` Rob Herring
@ 2024-02-29 9:34 ` Luca Ceresoli
2024-02-29 22:10 ` Rob Herring
0 siblings, 1 reply; 21+ messages in thread
From: Luca Ceresoli @ 2024-02-29 9:34 UTC (permalink / raw)
To: Rob Herring
Cc: Saravana Kannan, Frank Rowand, Greg Kroah-Hartman, Xu Yang,
kernel-team, devicetree, linux-kernel, Hervé Codina,
Geert Uytterhoeven
Hi Rob, Saravana,
On Wed, 28 Feb 2024 18:26:36 -0600
Rob Herring <robh+dt@kernel.org> wrote:
> On Wed, Feb 28, 2024 at 5:58 PM Saravana Kannan <saravanak@google.com> wrote:
> >
> > On Wed, Feb 28, 2024 at 1:56 PM Rob Herring <robh+dt@kernel.org> wrote:
> > >
> > > On Mon, Feb 26, 2024 at 5:52 AM Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:
> > > >
> > > > Hello Saravana,
> > > >
> > > > On Fri, 23 Feb 2024 17:35:24 -0800
> > > > Saravana Kannan <saravanak@google.com> wrote:
> > > >
> > > > > On Fri, Feb 23, 2024 at 8:18 AM Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:
> > > > > >
> > > > > > 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.
> > > > >
> > > > > Thanks for the report.
> > > > >
> > > > > >
> > > > > > > ---
> > > > > > > --- 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.
> > > > >
> > > > > 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. :(
> > > > >
> > > > > > 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.
> > > > >
> > > > > >
> > > > > > [ 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
> > > > >
> > > > > 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".
> > > > >
> > > > > > 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
> > > > > > ...
> > > > >
> > > > > 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.
> > > > >
> > > > > > [ 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
> > > > >
> > > > > Oh yeah, see. The "device_add" I expected earlier is getting removed here.
> > > > >
> > > > > > [ 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/20240221233026.2915061-1-saravanak@google.com/
> > > >
> > > > 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.
> > >
> > > It's just this one of the 3 patches that needs reverting?
Just this patch. I reverted only this and the issue disappeared.
> > 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.
> 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/20240224052436.3552333-1-saravanak@google.com/
[1]
https://lore.kernel.org/all/1692120000-46900-1-git-send-email-lizhi.hou@amd.com/
[2]
https://lore.kernel.org/lkml/20220427094502.456111-1-clement.leger@bootlin.com/
Luca
--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [REGRESSION] Re: [PATCH v2 2/3] of: property: Improve finding the supplier of a remote-endpoint property
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
0 siblings, 2 replies; 21+ messages in thread
From: Rob Herring @ 2024-02-29 22:10 UTC (permalink / raw)
To: Luca Ceresoli
Cc: Saravana Kannan, Frank Rowand, Greg Kroah-Hartman, Xu Yang,
kernel-team, devicetree, linux-kernel, Hervé Codina,
Geert Uytterhoeven
On Thu, Feb 29, 2024 at 3:34 AM Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:
>
> Hi Rob, Saravana,
>
> On Wed, 28 Feb 2024 18:26:36 -0600
> Rob Herring <robh+dt@kernel.org> wrote:
>
> > On Wed, Feb 28, 2024 at 5:58 PM Saravana Kannan <saravanak@google.com> wrote:
> > >
> > > On Wed, Feb 28, 2024 at 1:56 PM Rob Herring <robh+dt@kernel.org> wrote:
> > > >
> > > > On Mon, Feb 26, 2024 at 5:52 AM Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:
> > > > >
> > > > > Hello Saravana,
> > > > >
> > > > > On Fri, 23 Feb 2024 17:35:24 -0800
> > > > > Saravana Kannan <saravanak@google.com> wrote:
> > > > >
> > > > > > On Fri, Feb 23, 2024 at 8:18 AM Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:
> > > > > > >
> > > > > > > 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.
> > > > > >
> > > > > > Thanks for the report.
> > > > > >
> > > > > > >
> > > > > > > > ---
> > > > > > > > --- 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.
> > > > > >
> > > > > > 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. :(
> > > > > >
> > > > > > > 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.
> > > > > >
> > > > > > >
> > > > > > > [ 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
> > > > > >
> > > > > > 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".
> > > > > >
> > > > > > > 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
> > > > > > > ...
> > > > > >
> > > > > > 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.
> > > > > >
> > > > > > > [ 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
> > > > > >
> > > > > > Oh yeah, see. The "device_add" I expected earlier is getting removed here.
> > > > > >
> > > > > > > [ 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/20240221233026.2915061-1-saravanak@google.com/
> > > > >
> > > > > 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.
> > > >
> > > > It's just this one of the 3 patches that needs reverting?
>
> Just this patch. I reverted only this and the issue disappeared.
>
> > > 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.
>
> > 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.
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [REGRESSION] Re: [PATCH v2 2/3] of: property: Improve finding the supplier of a remote-endpoint property
2024-02-29 22:10 ` Rob Herring
@ 2024-02-29 22:54 ` Saravana Kannan
2024-03-01 9:59 ` Luca Ceresoli
1 sibling, 0 replies; 21+ messages in thread
From: Saravana Kannan @ 2024-02-29 22:54 UTC (permalink / raw)
To: Rob Herring
Cc: Luca Ceresoli, Frank Rowand, Greg Kroah-Hartman, Xu Yang,
kernel-team, devicetree, linux-kernel, Hervé Codina,
Geert Uytterhoeven
On Thu, Feb 29, 2024 at 2:10 PM Rob Herring <robh+dt@kernel.org> wrote:
>
> On Thu, Feb 29, 2024 at 3:34 AM Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:
> >
> > Hi Rob, Saravana,
> >
> > On Wed, 28 Feb 2024 18:26:36 -0600
> > Rob Herring <robh+dt@kernel.org> wrote:
> >
> > > On Wed, Feb 28, 2024 at 5:58 PM Saravana Kannan <saravanak@google.com> wrote:
> > > >
> > > > On Wed, Feb 28, 2024 at 1:56 PM Rob Herring <robh+dt@kernel.org> wrote:
> > > > >
> > > > > On Mon, Feb 26, 2024 at 5:52 AM Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:
> > > > > >
> > > > > > Hello Saravana,
> > > > > >
> > > > > > On Fri, 23 Feb 2024 17:35:24 -0800
> > > > > > Saravana Kannan <saravanak@google.com> wrote:
> > > > > >
> > > > > > > On Fri, Feb 23, 2024 at 8:18 AM Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:
> > > > > > > >
> > > > > > > > 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.
> > > > > > >
> > > > > > > Thanks for the report.
> > > > > > >
> > > > > > > >
> > > > > > > > > ---
> > > > > > > > > --- 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.
> > > > > > >
> > > > > > > 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. :(
> > > > > > >
> > > > > > > > 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.
> > > > > > >
> > > > > > > >
> > > > > > > > [ 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
> > > > > > >
> > > > > > > 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".
> > > > > > >
> > > > > > > > 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
> > > > > > > > ...
> > > > > > >
> > > > > > > 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.
> > > > > > >
> > > > > > > > [ 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
> > > > > > >
> > > > > > > Oh yeah, see. The "device_add" I expected earlier is getting removed here.
> > > > > > >
> > > > > > > > [ 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/20240221233026.2915061-1-saravanak@google.com/
> > > > > >
> > > > > > 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.
> > > > >
> > > > > It's just this one of the 3 patches that needs reverting?
> >
> > Just this patch. I reverted only this and the issue disappeared.
> >
> > > > 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.
> >
> > > 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.
>
> 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/20240224052436.3552333-1-saravanak@google.com/
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.
> 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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [REGRESSION] Re: [PATCH v2 2/3] of: property: Improve finding the supplier of a remote-endpoint property
2024-02-29 22:10 ` Rob Herring
2024-02-29 22:54 ` Saravana Kannan
@ 2024-03-01 9:59 ` Luca Ceresoli
1 sibling, 0 replies; 21+ messages in thread
From: Luca Ceresoli @ 2024-03-01 9:59 UTC (permalink / raw)
To: Rob Herring
Cc: Saravana Kannan, Frank Rowand, Greg Kroah-Hartman, Xu Yang,
kernel-team, devicetree, linux-kernel, Hervé Codina,
Geert Uytterhoeven
Hello Rob,
On Thu, 29 Feb 2024 16:10:38 -0600
Rob Herring <robh+dt@kernel.org> wrote:
[...]
> > > > > It's just this one of the 3 patches that needs reverting?
> >
> > Just this patch. I reverted only this and the issue disappeared.
> >
> > > > 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.
> >
> > > 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.
>
> 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/20240229105204.720717-1-herve.codina@bootlin.com/
https://lore.kernel.org/all/20240227113426.253232-1-herve.codina@bootlin.com/
https://lore.kernel.org/all/20240220133950.138452-1-herve.codina@bootlin.com/
https://lore.kernel.org/all/20240220111044.133776-1-herve.codina@bootlin.com/
Best regards,
Luca
--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2024-03-01 9:59 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
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
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).