[PATCH 0/2] devlink: Take care of FWNODE_FLAG_NOT_DEVICE in case of DT overlays

[PATCH 0/2] devlink: Take care of FWNODE_FLAG_NOT_DEVICE in case of DT overlays

[Herve Codina]

Since commit 1a50d9403fb9 ("treewide: Fix probing of devices in DT
overlays"), the FWNODE_FLAG_NOT_DEVICE is set on nodes present in the
overlay. Having this flag set leads to some wrong devlink links.
For instance, some links can be created with a supplier set to a parent
device instead of the device itself.

This series clears the FWNODE_FLAG_NOT_DEVICE in some specific location
to fix the wrong links issues.
- device_add()
  When a device is added, the related fwnode (if any) is populated as
  a struct device. It makes sense to clear the flag at that point and so
  avoid differences between DT used with or without overlays.
- of_link_phandle()
  If the supplier device has a compatible string and 'is available', a
  device is going to be created soon to handle this node. In that case,
  to avoid a link created using the supplier parent device instead of
  the device itself, clearing the FWNODE_FLAG_NOT_DEVICE makes sense.

Best regards,
Hervé Codina

Herve Codina (2):
  driver core: Clear FWNODE_FLAG_NOT_DEVICE when a device is added
  of: property: fw_devlink: Fix links to supplier when created from
    phandles

 drivers/base/core.c   |  1 +
 drivers/of/property.c | 16 +++++++++++++++-
 2 files changed, 16 insertions(+), 1 deletion(-)

-- 
2.43.0

[PATCH 1/2] driver core: Clear FWNODE_FLAG_NOT_DEVICE when a device is added

[Herve Codina]

Since commit 1a50d9403fb9 ("treewide: Fix probing of devices in DT
overlays"), when using device-tree overlays, the FWNODE_FLAG_NOT_DEVICE
is set on each overlay nodes.
When an overlay contains a node related to a bus (i2c for instance)
and its children nodes representing i2c devices, the flag is cleared for
the bus node by the OF notifier but the "standard" probe sequence takes
place (the same one is performed without an overlay) for the bus and
children devices are created simply by walking the children DT nodes
without clearing the FWNODE_FLAG_NOT_DEVICE flag for these devices.

Clear the FWNODE_FLAG_NOT_DEVICE when the device is added, no matter if
an overlay is used or not.

Fixes: 1a50d9403fb9 ("treewide: Fix probing of devices in DT overlays")
Cc: <stable@vger.kernel.org>
Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 drivers/base/core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 14d46af40f9a..61d09ac57bfb 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -3619,6 +3619,7 @@ int device_add(struct device *dev)
 	 */
 	if (dev->fwnode && !dev->fwnode->dev) {
 		dev->fwnode->dev = dev;
+		dev->fwnode->flags &= ~FWNODE_FLAG_NOT_DEVICE;
 		fw_devlink_link_device(dev);
 	}
 
-- 
2.43.0

Re: [PATCH 1/2] driver core: Clear FWNODE_FLAG_NOT_DEVICE when a device is added

[Saravana Kannan]

On Tue, Feb 20, 2024 at 3:10 AM Herve Codina <herve.codina@bootlin.com> wrote:
>
> Since commit 1a50d9403fb9 ("treewide: Fix probing of devices in DT
> overlays"), when using device-tree overlays, the FWNODE_FLAG_NOT_DEVICE
> is set on each overlay nodes.
> When an overlay contains a node related to a bus (i2c for instance)
> and its children nodes representing i2c devices, the flag is cleared for
> the bus node by the OF notifier but the "standard" probe sequence takes
> place (the same one is performed without an overlay) for the bus and
> children devices are created simply by walking the children DT nodes
> without clearing the FWNODE_FLAG_NOT_DEVICE flag for these devices.
>
> Clear the FWNODE_FLAG_NOT_DEVICE when the device is added, no matter if
> an overlay is used or not.
>
> Fixes: 1a50d9403fb9 ("treewide: Fix probing of devices in DT overlays")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> ---
>  drivers/base/core.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 14d46af40f9a..61d09ac57bfb 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -3619,6 +3619,7 @@ int device_add(struct device *dev)
>          */
>         if (dev->fwnode && !dev->fwnode->dev) {
>                 dev->fwnode->dev = dev;
> +               dev->fwnode->flags &= ~FWNODE_FLAG_NOT_DEVICE;
>                 fw_devlink_link_device(dev);
>         }

Temporary Nack on this. I think depending on how we address patch 2/2
this patch might not be necessary.

Also, I'd ideally prefer this gets cleared before the device is added,
but it's a position that I'd be willing to change.



-Saravana

Re: [PATCH 1/2] driver core: Clear FWNODE_FLAG_NOT_DEVICE when a device is added

[Herve Codina]

Hi Saravana,

On Tue, 20 Feb 2024 18:41:03 -0800
Saravana Kannan <saravanak@google.com> wrote:

> On Tue, Feb 20, 2024 at 3:10 AM Herve Codina <herve.codina@bootlin.com> wrote:
> >
> > Since commit 1a50d9403fb9 ("treewide: Fix probing of devices in DT
> > overlays"), when using device-tree overlays, the FWNODE_FLAG_NOT_DEVICE
> > is set on each overlay nodes.
> > When an overlay contains a node related to a bus (i2c for instance)
> > and its children nodes representing i2c devices, the flag is cleared for
> > the bus node by the OF notifier but the "standard" probe sequence takes
> > place (the same one is performed without an overlay) for the bus and
> > children devices are created simply by walking the children DT nodes
> > without clearing the FWNODE_FLAG_NOT_DEVICE flag for these devices.
> >
> > Clear the FWNODE_FLAG_NOT_DEVICE when the device is added, no matter if
> > an overlay is used or not.
> >
> > Fixes: 1a50d9403fb9 ("treewide: Fix probing of devices in DT overlays")
> > Cc: <stable@vger.kernel.org>
> > Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> > ---
> >  drivers/base/core.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > index 14d46af40f9a..61d09ac57bfb 100644
> > --- a/drivers/base/core.c
> > +++ b/drivers/base/core.c
> > @@ -3619,6 +3619,7 @@ int device_add(struct device *dev)
> >          */
> >         if (dev->fwnode && !dev->fwnode->dev) {
> >                 dev->fwnode->dev = dev;
> > +               dev->fwnode->flags &= ~FWNODE_FLAG_NOT_DEVICE;
> >                 fw_devlink_link_device(dev);
> >         }  
> 
> Temporary Nack on this. I think depending on how we address patch 2/2
> this patch might not be necessary.
> 
> Also, I'd ideally prefer this gets cleared before the device is added,
> but it's a position that I'd be willing to change.
> 

Some more information about this current patch.

Several month ago, I sent a patch related to a warning raised during driver
unbinding [1]. This warning was raised by __device_links_no_driver() because
we unlink a consumer while its supplier links.status is DL_DEV_UNBINDING.
You suspected an issue with the device removal ordering.

On this system, I applied this current patch clearing FWNODE_FLAG_NOT_DEVICE
in device_add(). This fixes the warning described in [1].

[1]: https://lore.kernel.org/linux-kernel/CAGETcx-Mp0uKBF_BWFFBUm=eVOp8xhxF3+znFB8vTaFwpJWTnw@mail.gmail.com/


The use case on that system, involved DT overlays and the fragment applied is the
following:
--- 8< ---
   pci-ep-bus@0 {
	compatible = "simple-bus";
	#address-cells = <1>;
	#size-cells = <1>;

	/*
	 * map @0xe2000000 (32MB) to BAR0 (CPU)
	 * map @0xe0000000 (16MB) to BAR1 (AMBA)
	 */
	ranges = <0xe2000000 0x00 0x00 0x00 0x2000000
	          0xe0000000 0x01 0x00 0x00 0x1000000>;

	...

	flx0: flexcom@e0040000 {
		compatible = "atmel,sama5d2-flexcom";
		reg = <0xe0040000 0x100>;
		clocks = <&clks GCK_ID_FLEXCOM0>;
		#address-cells = <1>;
		#size-cells = <1>;
		ranges = <0x0 0xe0040000 0x800>;

		atmel,flexcom-mode = <ATMEL_FLEXCOM_MODE_TWI>;

		i2c_lan966x: i2c@600 {
			compatible = "microchip,lan966x-i2c";
			reg = <0x600 0x200>;
			interrupt-parent = <&itc>;
			interrupts = <48>;
			#address-cells = <1>;
			#size-cells = <0>;
			clocks = <&clks GCK_ID_FLEXCOM0>;
			assigned-clocks = <&clks GCK_ID_FLEXCOM0>;
			assigned-clock-rates = <20000000>;
			pinctrl-0 = <&fc0_a_pins>;
			pinctrl-names = "default";
			i2c-analog-filter;
			i2c-digital-filter;
			i2c-digital-filter-width-ns = <35>;
		};
	};
	...
   };
--- 8< ---
This fragment is applied to a PCI device node.

Without clearing the FWNODE_FLAG_NOT_DEVICE, a link is present between the
i2c@600 and the PCI device. With the flag cleared, this link is replaced by
a link between the i2c@600 and the pci-ep-bus. Which looks better.

The flexcom driver is a MFD driver. As a MFD driver, it simply calls
devm_of_platform_populate(). In this path, devices are created and added but
nothing cleared the FWNODE_FLAG_NOT_DEVICE.

Based on your remark "I'd ideally prefer this gets cleared before the device
is added", I have the feeling that all calls to device_add() should clear the
flag before calling device_add(). So having FWNODE_FLAG_NOT_DEVICE cleared
in device_add() itself in that case makes sense.

What is your opinion ?

Also, feel free to ask for some more traces and/or logs if needed.

Best regards,
Hervé
-- 
Hervé Codina, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[PATCH 2/2] of: property: fw_devlink: Fix links to supplier when created from phandles

[Herve Codina]

Since commit 1a50d9403fb9 ("treewide: Fix probing of devices in DT
overlays"), when using device-tree overlays, the FWNODE_FLAG_NOT_DEVICE
is set on each overlay nodes. This flag is cleared when a struct device
is actually created for the DT node.
Also, when a device is created, the device DT node is parsed for known
phandle and devlinks consumer/supplier links are created between the
device (consumer) and the devices referenced by phandles (suppliers).
As these supplier device can have a struct device not already created,
the FWNODE_FLAG_NOT_DEVICE can be set for suppliers and leads the
devlink supplier point to the device's parent instead of the device
itself.

Avoid this situation clearing the supplier FWNODE_FLAG_NOT_DEVICE just
before the devlink creation if a device is supposed to be created and
handled later in the process.

Fixes: 1a50d9403fb9 ("treewide: Fix probing of devices in DT overlays")
Cc: <stable@vger.kernel.org>
Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 drivers/of/property.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/of/property.c b/drivers/of/property.c
index 641a40cf5cf3..ff5cac477dbe 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -1097,6 +1097,7 @@ static void of_link_to_phandle(struct device_node *con_np,
 			      struct device_node *sup_np)
 {
 	struct device_node *tmp_np = of_node_get(sup_np);
+	struct fwnode_handle *sup_fwnode;
 
 	/* Check that sup_np and its ancestors are available. */
 	while (tmp_np) {
@@ -1113,7 +1114,20 @@ static void of_link_to_phandle(struct device_node *con_np,
 		tmp_np = of_get_next_parent(tmp_np);
 	}
 
-	fwnode_link_add(of_fwnode_handle(con_np), of_fwnode_handle(sup_np));
+	/*
+	 * In case of overlays, the fwnode are added with FWNODE_FLAG_NOT_DEVICE
+	 * flag set. A node can have a phandle that references an other node
+	 * added by the overlay.
+	 * Clear the supplier's FWNODE_FLAG_NOT_DEVICE so that fw_devlink links
+	 * to this supplier instead of linking to its parent.
+	 */
+	sup_fwnode = of_fwnode_handle(sup_np);
+	if (sup_fwnode->flags & FWNODE_FLAG_NOT_DEVICE) {
+		if (of_property_present(sup_np, "compatible") &&
+		    of_device_is_available(sup_np))
+			sup_fwnode->flags &= ~FWNODE_FLAG_NOT_DEVICE;
+	}
+	fwnode_link_add(of_fwnode_handle(con_np), sup_fwnode);
 }
 
 /**
-- 
2.43.0

Re: [PATCH 2/2] of: property: fw_devlink: Fix links to supplier when created from phandles

[Saravana Kannan]

On Tue, Feb 20, 2024 at 3:10 AM Herve Codina <herve.codina@bootlin.com> wrote:
>
> Since commit 1a50d9403fb9 ("treewide: Fix probing of devices in DT
> overlays"), when using device-tree overlays, the FWNODE_FLAG_NOT_DEVICE
> is set on each overlay nodes. This flag is cleared when a struct device
> is actually created for the DT node.
> Also, when a device is created, the device DT node is parsed for known
> phandle and devlinks consumer/supplier links are created between the
> device (consumer) and the devices referenced by phandles (suppliers).
> As these supplier device can have a struct device not already created,
> the FWNODE_FLAG_NOT_DEVICE can be set for suppliers and leads the
> devlink supplier point to the device's parent instead of the device
> itself.
>
> Avoid this situation clearing the supplier FWNODE_FLAG_NOT_DEVICE just
> before the devlink creation if a device is supposed to be created and
> handled later in the process.
>
> Fixes: 1a50d9403fb9 ("treewide: Fix probing of devices in DT overlays")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> ---
>  drivers/of/property.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index 641a40cf5cf3..ff5cac477dbe 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -1097,6 +1097,7 @@ static void of_link_to_phandle(struct device_node *con_np,
>                               struct device_node *sup_np)
>  {
>         struct device_node *tmp_np = of_node_get(sup_np);
> +       struct fwnode_handle *sup_fwnode;
>
>         /* Check that sup_np and its ancestors are available. */
>         while (tmp_np) {
> @@ -1113,7 +1114,20 @@ static void of_link_to_phandle(struct device_node *con_np,
>                 tmp_np = of_get_next_parent(tmp_np);
>         }
>
> -       fwnode_link_add(of_fwnode_handle(con_np), of_fwnode_handle(sup_np));
> +       /*
> +        * In case of overlays, the fwnode are added with FWNODE_FLAG_NOT_DEVICE
> +        * flag set. A node can have a phandle that references an other node
> +        * added by the overlay.
> +        * Clear the supplier's FWNODE_FLAG_NOT_DEVICE so that fw_devlink links
> +        * to this supplier instead of linking to its parent.
> +        */
> +       sup_fwnode = of_fwnode_handle(sup_np);
> +       if (sup_fwnode->flags & FWNODE_FLAG_NOT_DEVICE) {
> +               if (of_property_present(sup_np, "compatible") &&
> +                   of_device_is_available(sup_np))
> +                       sup_fwnode->flags &= ~FWNODE_FLAG_NOT_DEVICE;
> +       }
> +       fwnode_link_add(of_fwnode_handle(con_np), sup_fwnode);

Nack.

of_link_to_phandle() doesn't care about any of the fwnode flags. It
just creates links between the consumer and supplier nodes. Don't add
more intelligence into it please. Also, "compatible" doesn't really
guarantee device creation and you can have devices created out of
nodes with no compatible property. I finally managed to get away from
looking for the "compatible" property. So, let's not add back a
dependency on that property please.

Can you please give a real example where you are hitting this? I have
some thoughts on solutions, but I want to understand the issue fully
before I make suggestions.

Thanks,
Saravana

Re: [PATCH 2/2] of: property: fw_devlink: Fix links to supplier when created from phandles

[Herve Codina]

Hi Saravana,

On Tue, 20 Feb 2024 18:40:40 -0800
Saravana Kannan <saravanak@google.com> wrote:

> On Tue, Feb 20, 2024 at 3:10 AM Herve Codina <herve.codina@bootlin.com> wrote:
> >
> > Since commit 1a50d9403fb9 ("treewide: Fix probing of devices in DT
> > overlays"), when using device-tree overlays, the FWNODE_FLAG_NOT_DEVICE
> > is set on each overlay nodes. This flag is cleared when a struct device
> > is actually created for the DT node.
> > Also, when a device is created, the device DT node is parsed for known
> > phandle and devlinks consumer/supplier links are created between the
> > device (consumer) and the devices referenced by phandles (suppliers).
> > As these supplier device can have a struct device not already created,
> > the FWNODE_FLAG_NOT_DEVICE can be set for suppliers and leads the
> > devlink supplier point to the device's parent instead of the device
> > itself.
> >
> > Avoid this situation clearing the supplier FWNODE_FLAG_NOT_DEVICE just
> > before the devlink creation if a device is supposed to be created and
> > handled later in the process.
> >
> > Fixes: 1a50d9403fb9 ("treewide: Fix probing of devices in DT overlays")
> > Cc: <stable@vger.kernel.org>
> > Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> > ---
> >  drivers/of/property.c | 16 +++++++++++++++-
> >  1 file changed, 15 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/of/property.c b/drivers/of/property.c
> > index 641a40cf5cf3..ff5cac477dbe 100644
> > --- a/drivers/of/property.c
> > +++ b/drivers/of/property.c
> > @@ -1097,6 +1097,7 @@ static void of_link_to_phandle(struct device_node *con_np,
> >                               struct device_node *sup_np)
> >  {
> >         struct device_node *tmp_np = of_node_get(sup_np);
> > +       struct fwnode_handle *sup_fwnode;
> >
> >         /* Check that sup_np and its ancestors are available. */
> >         while (tmp_np) {
> > @@ -1113,7 +1114,20 @@ static void of_link_to_phandle(struct device_node *con_np,
> >                 tmp_np = of_get_next_parent(tmp_np);
> >         }
> >
> > -       fwnode_link_add(of_fwnode_handle(con_np), of_fwnode_handle(sup_np));
> > +       /*
> > +        * In case of overlays, the fwnode are added with FWNODE_FLAG_NOT_DEVICE
> > +        * flag set. A node can have a phandle that references an other node
> > +        * added by the overlay.
> > +        * Clear the supplier's FWNODE_FLAG_NOT_DEVICE so that fw_devlink links
> > +        * to this supplier instead of linking to its parent.
> > +        */
> > +       sup_fwnode = of_fwnode_handle(sup_np);
> > +       if (sup_fwnode->flags & FWNODE_FLAG_NOT_DEVICE) {
> > +               if (of_property_present(sup_np, "compatible") &&
> > +                   of_device_is_available(sup_np))
> > +                       sup_fwnode->flags &= ~FWNODE_FLAG_NOT_DEVICE;
> > +       }
> > +       fwnode_link_add(of_fwnode_handle(con_np), sup_fwnode);  
> 
> Nack.
> 
> of_link_to_phandle() doesn't care about any of the fwnode flags. It
> just creates links between the consumer and supplier nodes. Don't add
> more intelligence into it please. Also, "compatible" doesn't really
> guarantee device creation and you can have devices created out of
> nodes with no compatible property. I finally managed to get away from
> looking for the "compatible" property. So, let's not add back a
> dependency on that property please.
> 
> Can you please give a real example where you are hitting this? I have
> some thoughts on solutions, but I want to understand the issue fully
> before I make suggestions.
> 

I detected the issue with this overlay:
--- 8< ---
&{/}
{
	reg_dock_sys_3v3: regulator-dock-sys-3v3 {
		compatible = "regulator-fixed";
		regulator-name = "DOCK_SYS_3V3";
		regulator-min-microvolt = <3300000>;
		regulator-max-microvolt = <3300000>;
		gpios = <&tca6424_dock_1 5 GPIO_ACTIVE_HIGH>; // DOCK_SYS3V3_EN
		enable-active-high;
		regulator-always-on;
	};
};

&i2c5 {
	tca6424_dock_1: gpio@22 {
		compatible = "ti,tca6424";
		reg = <0x22>;
		gpio-controller;
		#gpio-cells = <2>;
		interrupt-parent = <&gpio4>;
		interrupts = <1 IRQ_TYPE_EDGE_FALLING>;
		interrupt-controller;
		#interrupt-cells = <2>;
		vcc-supply = <&reg_dock_ctrl_3v3>;
	};
};
--- 8< ---

The regulator uses a gpio.
The supplier for the regulator was not the gpio chip (gpio@22) but the i2c bus.

I first tried to clear always the flag in of_link_to_phandle() without any check
to a "compatible" string and in that case, I broke pinctrl.

All devices were waiting for the pinctrl they used (child of pinctrl device
node) even if the pinctrl driver was bound to the device.

For pinctrl, the DT structure looks like the following:
--- 8< ---
{
	...
	pinctrl@1234 {
		reg = <1234>;
		compatible = "vendor,chip";

		pinctrl_some_device: grp {
			fsl,pins = < ... >;
		};
	};

	some_device@4567 {
		compablile = "foo,bar";
		reg = <4567>;
		pinctrl-names = "default";
		pinctrl-0 = <&pinctrl_some_device>;
		...
	};
};
--- 8< ---
		
In that case the link related to pinctrl for some_device needs to be to the
'pinctrl_some_device' node parent (i.e. the pinctrl@1234 node).


Best regards,
Hervé

Re: [PATCH 2/2] of: property: fw_devlink: Fix links to supplier when created from phandles

[Saravana Kannan]

On Wed, Feb 21, 2024 at 12:51 AM Herve Codina <herve.codina@bootlin.com> wrote:
>
> Hi Saravana,
>
> On Tue, 20 Feb 2024 18:40:40 -0800
> Saravana Kannan <saravanak@google.com> wrote:
>
> > On Tue, Feb 20, 2024 at 3:10 AM Herve Codina <herve.codina@bootlin.com> wrote:
> > >
> > > Since commit 1a50d9403fb9 ("treewide: Fix probing of devices in DT
> > > overlays"), when using device-tree overlays, the FWNODE_FLAG_NOT_DEVICE
> > > is set on each overlay nodes. This flag is cleared when a struct device
> > > is actually created for the DT node.
> > > Also, when a device is created, the device DT node is parsed for known
> > > phandle and devlinks consumer/supplier links are created between the
> > > device (consumer) and the devices referenced by phandles (suppliers).
> > > As these supplier device can have a struct device not already created,
> > > the FWNODE_FLAG_NOT_DEVICE can be set for suppliers and leads the
> > > devlink supplier point to the device's parent instead of the device
> > > itself.
> > >
> > > Avoid this situation clearing the supplier FWNODE_FLAG_NOT_DEVICE just
> > > before the devlink creation if a device is supposed to be created and
> > > handled later in the process.
> > >
> > > Fixes: 1a50d9403fb9 ("treewide: Fix probing of devices in DT overlays")
> > > Cc: <stable@vger.kernel.org>
> > > Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> > > ---
> > >  drivers/of/property.c | 16 +++++++++++++++-
> > >  1 file changed, 15 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/of/property.c b/drivers/of/property.c
> > > index 641a40cf5cf3..ff5cac477dbe 100644
> > > --- a/drivers/of/property.c
> > > +++ b/drivers/of/property.c
> > > @@ -1097,6 +1097,7 @@ static void of_link_to_phandle(struct device_node *con_np,
> > >                               struct device_node *sup_np)
> > >  {
> > >         struct device_node *tmp_np = of_node_get(sup_np);
> > > +       struct fwnode_handle *sup_fwnode;
> > >
> > >         /* Check that sup_np and its ancestors are available. */
> > >         while (tmp_np) {
> > > @@ -1113,7 +1114,20 @@ static void of_link_to_phandle(struct device_node *con_np,
> > >                 tmp_np = of_get_next_parent(tmp_np);
> > >         }
> > >
> > > -       fwnode_link_add(of_fwnode_handle(con_np), of_fwnode_handle(sup_np));
> > > +       /*
> > > +        * In case of overlays, the fwnode are added with FWNODE_FLAG_NOT_DEVICE
> > > +        * flag set. A node can have a phandle that references an other node
> > > +        * added by the overlay.
> > > +        * Clear the supplier's FWNODE_FLAG_NOT_DEVICE so that fw_devlink links
> > > +        * to this supplier instead of linking to its parent.
> > > +        */
> > > +       sup_fwnode = of_fwnode_handle(sup_np);
> > > +       if (sup_fwnode->flags & FWNODE_FLAG_NOT_DEVICE) {
> > > +               if (of_property_present(sup_np, "compatible") &&
> > > +                   of_device_is_available(sup_np))
> > > +                       sup_fwnode->flags &= ~FWNODE_FLAG_NOT_DEVICE;
> > > +       }
> > > +       fwnode_link_add(of_fwnode_handle(con_np), sup_fwnode);
> >
> > Nack.
> >
> > of_link_to_phandle() doesn't care about any of the fwnode flags. It
> > just creates links between the consumer and supplier nodes. Don't add
> > more intelligence into it please. Also, "compatible" doesn't really
> > guarantee device creation and you can have devices created out of
> > nodes with no compatible property. I finally managed to get away from
> > looking for the "compatible" property. So, let's not add back a
> > dependency on that property please.
> >
> > Can you please give a real example where you are hitting this? I have
> > some thoughts on solutions, but I want to understand the issue fully
> > before I make suggestions.
> >
>
> I detected the issue with this overlay:
> --- 8< ---
> &{/}
> {
>         reg_dock_sys_3v3: regulator-dock-sys-3v3 {
>                 compatible = "regulator-fixed";
>                 regulator-name = "DOCK_SYS_3V3";
>                 regulator-min-microvolt = <3300000>;
>                 regulator-max-microvolt = <3300000>;
>                 gpios = <&tca6424_dock_1 5 GPIO_ACTIVE_HIGH>; // DOCK_SYS3V3_EN
>                 enable-active-high;
>                 regulator-always-on;
>         };
> };
>
> &i2c5 {
>         tca6424_dock_1: gpio@22 {
>                 compatible = "ti,tca6424";
>                 reg = <0x22>;
>                 gpio-controller;
>                 #gpio-cells = <2>;
>                 interrupt-parent = <&gpio4>;
>                 interrupts = <1 IRQ_TYPE_EDGE_FALLING>;
>                 interrupt-controller;
>                 #interrupt-cells = <2>;
>                 vcc-supply = <&reg_dock_ctrl_3v3>;
>         };
> };
> --- 8< ---
>
> The regulator uses a gpio.
> The supplier for the regulator was not the gpio chip (gpio@22) but the i2c bus.

Thanks for the example. Let me think about this a bit on how we could
fix this and get back to you.

Please do ping me if I don't get back in a week or two.

-Saravana

>
> I first tried to clear always the flag in of_link_to_phandle() without any check
> to a "compatible" string and in that case, I broke pinctrl.
>
> All devices were waiting for the pinctrl they used (child of pinctrl device
> node) even if the pinctrl driver was bound to the device.
>
> For pinctrl, the DT structure looks like the following:
> --- 8< ---
> {
>         ...
>         pinctrl@1234 {
>                 reg = <1234>;
>                 compatible = "vendor,chip";
>
>                 pinctrl_some_device: grp {
>                         fsl,pins = < ... >;
>                 };
>         };
>
>         some_device@4567 {
>                 compablile = "foo,bar";
>                 reg = <4567>;
>                 pinctrl-names = "default";
>                 pinctrl-0 = <&pinctrl_some_device>;
>                 ...
>         };
> };
> --- 8< ---
>
> In that case the link related to pinctrl for some_device needs to be to the
> 'pinctrl_some_device' node parent (i.e. the pinctrl@1234 node).
>
>
> Best regards,
> Hervé

Re: [PATCH 2/2] of: property: fw_devlink: Fix links to supplier when created from phandles

[Herve Codina]

Hi Saravana,

On Mon, 4 Mar 2024 23:14:13 -0800
Saravana Kannan <saravanak@google.com> wrote:

...
> 
> Thanks for the example. Let me think about this a bit on how we could
> fix this and get back to you.
> 
> Please do ping me if I don't get back in a week or two.
> 

This is my ping.
Do you move forward ?

Best regards,
Hervé

Re: [PATCH 2/2] of: property: fw_devlink: Fix links to supplier when created from phandles

[Saravana Kannan]

On Thu, Mar 21, 2024 at 4:59 AM Herve Codina <herve.codina@bootlin.com> wrote:
>
> Hi Saravana,
>
> On Mon, 4 Mar 2024 23:14:13 -0800
> Saravana Kannan <saravanak@google.com> wrote:
>
> ...
> >
> > Thanks for the example. Let me think about this a bit on how we could
> > fix this and get back to you.
> >
> > Please do ping me if I don't get back in a week or two.
> >
>
> This is my ping.
> Do you move forward ?

Thanks for the ping. I thought about it a bit. I think the right fix
it to undo the overlay fix I had suggested to Geert and then make the
overlay code call __fw_devlink_pickup_dangling_consumers() on the
parent device of the top level overlay nodes that get added that don't
have a device created for them.

I'll try to wrap up a patch for this on Monday. But if you want to
take a shot at this, that's ok too.

-Saravana


-Saravana

Re: [PATCH 2/2] of: property: fw_devlink: Fix links to supplier when created from phandles

[Herve Codina]

Hi Sarava,

On Fri, 22 Mar 2024 19:00:03 -0700
Saravana Kannan <saravanak@google.com> wrote:

> On Thu, Mar 21, 2024 at 4:59 AM Herve Codina <herve.codina@bootlin.com> wrote:
> >
> > Hi Saravana,
> >
> > On Mon, 4 Mar 2024 23:14:13 -0800
> > Saravana Kannan <saravanak@google.com> wrote:
> >
> > ...  
> > >
> > > Thanks for the example. Let me think about this a bit on how we could
> > > fix this and get back to you.
> > >
> > > Please do ping me if I don't get back in a week or two.
> > >  
> >
> > This is my ping.
> > Do you move forward ?  
> 
> Thanks for the ping. I thought about it a bit. I think the right fix
> it to undo the overlay fix I had suggested to Geert and then make the
> overlay code call __fw_devlink_pickup_dangling_consumers() on the
> parent device of the top level overlay nodes that get added that don't
> have a device created for them.
> 
> I'll try to wrap up a patch for this on Monday. But if you want to
> take a shot at this, that's ok too.
> 

I didn't see anything on this topic. Maybe I missed the related modifications.
Did you move forward on that patch ?

Best regards,
Hervé

Re: [PATCH 2/2] of: property: fw_devlink: Fix links to supplier when created from phandles

[Saravana Kannan]

On Mon, Apr 8, 2024 at 7:40 AM Herve Codina <herve.codina@bootlin.com> wrote:
>
> Hi Sarava,
>
> On Fri, 22 Mar 2024 19:00:03 -0700
> Saravana Kannan <saravanak@google.com> wrote:
>
> > On Thu, Mar 21, 2024 at 4:59 AM Herve Codina <herve.codina@bootlin.com> wrote:
> > >
> > > Hi Saravana,
> > >
> > > On Mon, 4 Mar 2024 23:14:13 -0800
> > > Saravana Kannan <saravanak@google.com> wrote:
> > >
> > > ...
> > > >
> > > > Thanks for the example. Let me think about this a bit on how we could
> > > > fix this and get back to you.
> > > >
> > > > Please do ping me if I don't get back in a week or two.
> > > >
> > >
> > > This is my ping.
> > > Do you move forward ?
> >
> > Thanks for the ping. I thought about it a bit. I think the right fix
> > it to undo the overlay fix I had suggested to Geert and then make the
> > overlay code call __fw_devlink_pickup_dangling_consumers() on the
> > parent device of the top level overlay nodes that get added that don't
> > have a device created for them.
> >
> > I'll try to wrap up a patch for this on Monday. But if you want to
> > take a shot at this, that's ok too.
> >
>
> I didn't see anything on this topic. Maybe I missed the related modifications.
> Did you move forward on that patch ?

Give this a shot and let me know please.
https://lore.kernel.org/lkml/20240408231310.325451-1-saravanak@google.com/T/#m40e641cb2b1c0cf5ad1af1021f2daca63faeb427

-Saravana