devlink: Take care of FWNODE_FLAG_NOT_DEVICE in case of DT overlays

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 <[email protected]>

Signed-off-by Herve Codina <[email protected]>

---
 drivers/base/core.c | 1 +
 1 file changed, 1 insertion(+)

drivers/base/core.c +0 -0

index 14d46af40f9a..61d09ac57bfb 100644

Quoted 26 lines · 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 <[email protected]>

Signed-off-by Herve Codina <[email protected]>

---
 drivers/base/core.c | 1 +
 1 file changed, 1 insertion(+)

drivers/base/core.c +0 -0

index 14d46af40f9a..61d09ac57bfb 100644

drivers/base/core.c int device_add(struct device *dev) · line 3624

Quoted 6 lines

drivers/base/core.c +1 -0

@@ -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.

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 <[email protected]>

Signed-off-by Herve Codina <[email protected]>

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

drivers/of/property.c +5 -1

index 641a40cf5cf3..ff5cac477dbe 100644
@@ -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.

Quoted 45 lines · 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 <[email protected]>

Signed-off-by Herve Codina <[email protected]>

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

drivers/of/property.c +5 -1

index 641a40cf5cf3..ff5cac477dbe 100644
@@ -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.

drivers/of/property.c static void of_link_to_phandle(struct device_node *con_np, · line 1130

Quoted 10 lines

drivers/of/property.c +10 -0

+        * 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.

Hi Saravana,

Quoted 67 lines · On Tue, 20 Feb 2024 18:40:40 -0800 Saravana Kannan <[email protected]> wrote:

Quoted 45 lines · 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 <[email protected]>

Signed-off-by Herve Codina <[email protected]>

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

drivers/of/property.c +5 -1

index 641a40cf5cf3..ff5cac477dbe 100644
@@ -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.

drivers/of/property.c static void of_link_to_phandle(struct device_node *con_np, · line 1130

Quoted 10 lines

drivers/of/property.c +10 -0

+        * 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.

Quoted 4 lines

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é

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.

Hi Saravana,

On Mon, 4 Mar 2024 23:14:13 -0800
Saravana Kannan <[email protected]> wrote:

...

Quoted 1 line

Quoted 5 lines

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 ?

Quoted 8 lines · Herve Codina

Hi Saravana,

On Mon, 4 Mar 2024 23:14:13 -0800
Saravana Kannan <[email protected]> wrote:

...

Quoted 1 line

Quoted 8 lines

Quoted 5 lines

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.

Hi Sarava,

Quoted 24 lines · On Fri, 22 Mar 2024 19:00:03 -0700 Saravana Kannan <[email protected]> wrote:

Quoted 8 lines · Herve Codina

Hi Saravana,

On Mon, 4 Mar 2024 23:14:13 -0800
Saravana Kannan <[email protected]> wrote:

...

Quoted 1 line

Quoted 8 lines

Quoted 5 lines

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.

Quoted 3 lines

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 ?