Fix simple-bus issues with fw_devlink

fw_devlink could end up creating device links for bus only devices. However, bus only devices don't get probed and can block probe() or sync_state() [1] call backs of other devices. To avoid this, probe these devices using the simple-pm-bus driver.

However, there are instances of devices that are not simple buses (they get probed by their specific drivers) that also list the "simple-bus" (or other bus only compatible strings) in their compatible property to automatically populate their child devices. We still want these devices to get probed by their specific drivers. So, we make sure this driver only probes devices that are only buses.

[1] - https://lore.kernel.org/lkml/CAPDyKFo9Bxremkb1dDrr4OcXSpE0keVze94Cm=zrkOVxHHxBmQ@mail.gmail.com/

Fixes c442a0d18744 ("driver core: Set fw_devlink to "permissive" behavior by default")

Signed-off-by Saravana Kannan <[email protected]>

Tested-by Saravana Kannan <[email protected]>

Tested-by Ulf Hansson <[email protected]>

fw_devlink could end up creating device links for bus only devices. However, bus only devices don't get probed and can block probe() or sync_state() [1] call backs of other devices. To avoid this, probe these devices using the simple-pm-bus driver.

However, there are instances of devices that are not simple buses (they get probed by their specific drivers) that also list the "simple-bus" (or other bus only compatible strings) in their compatible property to automatically populate their child devices. We still want these devices to get probed by their specific drivers. So, we make sure this driver only probes devices that are only buses.

[1] - https://lore.kernel.org/lkml/CAPDyKFo9Bxremkb1dDrr4OcXSpE0keVze94Cm=zrkOVxHHxBmQ@mail.gmail.com/

Fixes c442a0d18744 ("driver core: Set fw_devlink to "permissive" behavior by default")

Signed-off-by Saravana Kannan <[email protected]>

Tested-by Saravana Kannan <[email protected]>

Tested-by Ulf Hansson <[email protected]>

---
 drivers/bus/simple-pm-bus.c | 42 ++++++++++++++++++++++++++++++++++---
 1 file changed, 39 insertions(+), 3 deletions(-)

drivers/bus/simple-pm-bus.c +18 -3

index 01a3d0cd08ed..6b8d6257ed8a 100644
@@ -13,11 +13,36 @@
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
 
-
 static int simple_pm_bus_probe(struct platform_device *pdev)
 {
-	const struct of_dev_auxdata *lookup = dev_get_platdata(&pdev->dev);
-	struct device_node *np = pdev->dev.of_node;
+	const struct device *dev = &pdev->dev;
+	const struct of_dev_auxdata *lookup = dev_get_platdata(dev);
+	struct device_node *np = dev->of_node;
+	const struct of_device_id *match;
+
+	/*
+	 * Allow user to use driver_override to bind this driver to a
+	 * transparent bus device which has a different compatible string
+	 * that's not listed in simple_pm_bus_of_match. We don't want to do any
+	 * of the simple-pm-bus tasks for these devices, so return early.
+	 */
+	if (pdev->driver_override)
+		return 0;
+
+	match = of_match_device(dev->driver->of_match_table, dev);
+	/*
+	 * These are transparent bus devices (not simple-pm-bus matches) that
+	 * have their child nodes populated automatically.  So, don't need to

drivers/bus/simple-pm-bus.c +11 -0

 	dev_dbg(&pdev->dev, "%s\n", __func__);
 

@@ -31,14 +56,25 @@ static int simple_pm_bus_probe(struct platform_device *pdev)
 
 static int simple_pm_bus_remove(struct platform_device *pdev)
 {
+	const void *data = of_device_get_match_data(&pdev->dev);
+
+	if (pdev->driver_override || data)
+		return 0;
+
 	dev_dbg(&pdev->dev, "%s\n", __func__);
 
 	pm_runtime_disable(&pdev->dev);
 	return 0;
 }
 
+#define ONLY_BUS	((void *) 1) /* Match if the device is only a bus. */
+
 static const struct of_device_id simple_pm_bus_of_match[] = {
 	{ .compatible = "simple-pm-bus", },
+	{ .compatible = "simple-bus",	.data = ONLY_BUS },
+	{ .compatible = "simple-mfd",	.data = ONLY_BUS },
+	{ .compatible = "isa",		.data = ONLY_BUS },
+	{ .compatible = "arm,amba-bus",	.data = ONLY_BUS },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, simple_pm_bus_of_match);

Signature · 1 line

2.33.0.685.g46640cef36-goog

Quoted 52 lines · Saravana Kannan

fw_devlink could end up creating device links for bus only devices. However, bus only devices don't get probed and can block probe() or sync_state() [1] call backs of other devices. To avoid this, probe these devices using the simple-pm-bus driver.

However, there are instances of devices that are not simple buses (they get probed by their specific drivers) that also list the "simple-bus" (or other bus only compatible strings) in their compatible property to automatically populate their child devices. We still want these devices to get probed by their specific drivers. So, we make sure this driver only probes devices that are only buses.

[1] - https://lore.kernel.org/lkml/CAPDyKFo9Bxremkb1dDrr4OcXSpE0keVze94Cm=zrkOVxHHxBmQ@mail.gmail.com/

Fixes c442a0d18744 ("driver core: Set fw_devlink to "permissive" behavior by default")

Signed-off-by Saravana Kannan <[email protected]>

Tested-by Saravana Kannan <[email protected]>

Tested-by Ulf Hansson <[email protected]>

---
 drivers/bus/simple-pm-bus.c | 42 ++++++++++++++++++++++++++++++++++---
 1 file changed, 39 insertions(+), 3 deletions(-)

drivers/bus/simple-pm-bus.c +18 -3

index 01a3d0cd08ed..6b8d6257ed8a 100644
@@ -13,11 +13,36 @@
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
 
-
 static int simple_pm_bus_probe(struct platform_device *pdev)
 {
-     const struct of_dev_auxdata *lookup = dev_get_platdata(&pdev->dev);
-     struct device_node *np = pdev->dev.of_node;
+     const struct device *dev = &pdev->dev;
+     const struct of_dev_auxdata *lookup = dev_get_platdata(dev);
+     struct device_node *np = dev->of_node;
+     const struct of_device_id *match;
+
+     /*
+      * Allow user to use driver_override to bind this driver to a
+      * transparent bus device which has a different compatible string
+      * that's not listed in simple_pm_bus_of_match. We don't want to do any
+      * of the simple-pm-bus tasks for these devices, so return early.
+      */
+     if (pdev->driver_override)
+             return 0;
+
+     match = of_match_device(dev->driver->of_match_table, dev);
+     /*
+      * These are transparent bus devices (not simple-pm-bus matches) that
+      * have their child nodes populated automatically.  So, don't need to

drivers/bus/simple-pm-bus.c line 45

Quoted 10 lines

drivers/bus/simple-pm-bus.c +10 -0

+      * do anything more. We only match with the device if this driver is
+      * the most specific match because we don't want to incorrectly bind to
+      * a device that has a more specific driver.
+      */
+     if (match && match->data) {
+             if (of_property_match_string(np, "compatible", match->compatible) == 0)
+                     return 0;
+             else
+                     return -ENODEV;
+     }

This change is breaking the expected behavior for the already existent simple-bus nodes. All the simple-bus compatibles should be replaced now to simple-pm-bus. In my case, on some i.MX8 platforms, without the devlink, the devices suspend sequence changes (and even breaks).

To avoid breaking the already existent simple-bus nodes, maybe the logic should've been reversed: keep the simple-bus as is and add another compatible, IDK, something like simple-trasnparent-bus, or something.

Quoted 33 lines

drivers/bus/simple-pm-bus.c +11 -1

      dev_dbg(&pdev->dev, "%s\n", __func__);
 

@@ -31,14 +56,25 @@ static int simple_pm_bus_probe(struct platform_device *pdev)
 
 static int simple_pm_bus_remove(struct platform_device *pdev)
 {
+     const void *data = of_device_get_match_data(&pdev->dev);
+
+     if (pdev->driver_override || data)
+             return 0;
+
      dev_dbg(&pdev->dev, "%s\n", __func__);
 
      pm_runtime_disable(&pdev->dev);
      return 0;
 }
 
+#define ONLY_BUS     ((void *) 1) /* Match if the device is only a bus. */
+
 static const struct of_device_id simple_pm_bus_of_match[] = {
      { .compatible = "simple-pm-bus", },
+     { .compatible = "simple-bus",   .data = ONLY_BUS },
+     { .compatible = "simple-mfd",   .data = ONLY_BUS },
+     { .compatible = "isa",          .data = ONLY_BUS },
+     { .compatible = "arm,amba-bus", .data = ONLY_BUS },
      { /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, simple_pm_bus_of_match);
--

2.33.0.685.g46640cef36-goog

Quoted 70 lines · Abel Vesa

Quoted 52 lines · Saravana Kannan

fw_devlink could end up creating device links for bus only devices. However, bus only devices don't get probed and can block probe() or sync_state() [1] call backs of other devices. To avoid this, probe these devices using the simple-pm-bus driver.

However, there are instances of devices that are not simple buses (they get probed by their specific drivers) that also list the "simple-bus" (or other bus only compatible strings) in their compatible property to automatically populate their child devices. We still want these devices to get probed by their specific drivers. So, we make sure this driver only probes devices that are only buses.

[1] - https://lore.kernel.org/lkml/CAPDyKFo9Bxremkb1dDrr4OcXSpE0keVze94Cm=zrkOVxHHxBmQ@mail.gmail.com/

Fixes c442a0d18744 ("driver core: Set fw_devlink to "permissive" behavior by default")

Signed-off-by Saravana Kannan <[email protected]>

Tested-by Saravana Kannan <[email protected]>

Tested-by Ulf Hansson <[email protected]>

---
 drivers/bus/simple-pm-bus.c | 42 ++++++++++++++++++++++++++++++++++---
 1 file changed, 39 insertions(+), 3 deletions(-)

drivers/bus/simple-pm-bus.c +18 -3

index 01a3d0cd08ed..6b8d6257ed8a 100644
@@ -13,11 +13,36 @@
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
 
-
 static int simple_pm_bus_probe(struct platform_device *pdev)
 {
-     const struct of_dev_auxdata *lookup = dev_get_platdata(&pdev->dev);
-     struct device_node *np = pdev->dev.of_node;
+     const struct device *dev = &pdev->dev;
+     const struct of_dev_auxdata *lookup = dev_get_platdata(dev);
+     struct device_node *np = dev->of_node;
+     const struct of_device_id *match;
+
+     /*
+      * Allow user to use driver_override to bind this driver to a
+      * transparent bus device which has a different compatible string
+      * that's not listed in simple_pm_bus_of_match. We don't want to do any
+      * of the simple-pm-bus tasks for these devices, so return early.
+      */
+     if (pdev->driver_override)
+             return 0;
+
+     match = of_match_device(dev->driver->of_match_table, dev);
+     /*
+      * These are transparent bus devices (not simple-pm-bus matches) that
+      * have their child nodes populated automatically.  So, don't need to

drivers/bus/simple-pm-bus.c line 45

Quoted 10 lines

drivers/bus/simple-pm-bus.c +10 -0

+      * do anything more. We only match with the device if this driver is
+      * the most specific match because we don't want to incorrectly bind to
+      * a device that has a more specific driver.
+      */
+     if (match && match->data) {
+             if (of_property_match_string(np, "compatible", match->compatible) == 0)
+                     return 0;
+             else
+                     return -ENODEV;
+     }

This change is breaking the expected behavior for the already existent simple-bus nodes. All the simple-bus compatibles should be replaced now to simple-pm-bus. In my case, on some i.MX8 platforms, without the devlink, the devices suspend sequence changes (and even breaks).

Quoted 3 lines

To avoid breaking the already existent simple-bus nodes, maybe the logic should've been reversed: keep the simple-bus as is and add another compatible, IDK, something like simple-trasnparent-bus, or something.

The intent of this change IS to affect existing simple-bus nodes (but not in the way it's affecting you). But if it's breaking stuff, we obviously need to fix it.

I have a hunch on what's going on in your case, but can you point me to the specific simple-bus node that's getting affected? I'm expecting it to be a simple-bus node that gets added AFTER this driver is registered at device_initcall (module_init gets converted to device_initcall).

Also, can you try this hack patch to see if it helps your case?
https://lore.kernel.org/lkml/CAGETcx9U130Oq-umrvXME4JhEpO0Wadoki3kNxx=0-YvTR6PtQ@mail.gmail.com/

I have some thoughts on how I could fix this, but I need to think about a few cases.

Quoted 15 lines · Saravana Kannan

Quoted 13 lines · Abel Vesa

Quoted 11 lines · Saravana Kannan

fw_devlink could end up creating device links for bus only devices. However, bus only devices don't get probed and can block probe() or sync_state() [1] call backs of other devices. To avoid this, probe these devices using the simple-pm-bus driver.

However, there are instances of devices that are not simple buses (they get probed by their specific drivers) that also list the "simple-bus" (or other bus only compatible strings) in their compatible property to automatically populate their child devices. We still want these devices to get probed by their specific drivers. So, we make sure this driver only probes devices that are only buses.

Quoted 20 lines

Quoted 9 lines

This change is breaking the expected behavior for the already existent simple-bus nodes. All the simple-bus compatibles should be replaced now to simple-pm-bus. In my case, on some i.MX8 platforms, without the devlink, the devices suspend sequence changes (and even breaks).

To avoid breaking the already existent simple-bus nodes, maybe the logic should've been reversed: keep the simple-bus as is and add another compatible, IDK, something like simple-trasnparent-bus, or something.

The intent of this change IS to affect existing simple-bus nodes (but not in the way it's affecting you). But if it's breaking stuff, we obviously need to fix it.

I have a hunch on what's going on in your case, but can you point me to the specific simple-bus node that's getting affected? I'm expecting it to be a simple-bus node that gets added AFTER this driver is registered at device_initcall (module_init gets converted to device_initcall).

Quoted 5 lines

Also, can you try this hack patch to see if it helps your case?
https://lore.kernel.org/lkml/CAGETcx9U130Oq-umrvXME4JhEpO0Wadoki3kNxx=0-YvTR6PtQ@mail.gmail.com/

I have some thoughts on how I could fix this, but I need to think about a few cases.

Quoted 80 lines · Saravana Kannan

Quoted 70 lines · Abel Vesa

Quoted 52 lines · Saravana Kannan

fw_devlink could end up creating device links for bus only devices. However, bus only devices don't get probed and can block probe() or sync_state() [1] call backs of other devices. To avoid this, probe these devices using the simple-pm-bus driver.

However, there are instances of devices that are not simple buses (they get probed by their specific drivers) that also list the "simple-bus" (or other bus only compatible strings) in their compatible property to automatically populate their child devices. We still want these devices to get probed by their specific drivers. So, we make sure this driver only probes devices that are only buses.

[1] - https://lore.kernel.org/lkml/CAPDyKFo9Bxremkb1dDrr4OcXSpE0keVze94Cm=zrkOVxHHxBmQ@mail.gmail.com/

Fixes c442a0d18744 ("driver core: Set fw_devlink to "permissive" behavior by default")

Signed-off-by Saravana Kannan <[email protected]>

Tested-by Saravana Kannan <[email protected]>

Tested-by Ulf Hansson <[email protected]>

---
 drivers/bus/simple-pm-bus.c | 42 ++++++++++++++++++++++++++++++++++---
 1 file changed, 39 insertions(+), 3 deletions(-)

drivers/bus/simple-pm-bus.c +18 -3

index 01a3d0cd08ed..6b8d6257ed8a 100644
@@ -13,11 +13,36 @@
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
 
-
 static int simple_pm_bus_probe(struct platform_device *pdev)
 {
-     const struct of_dev_auxdata *lookup = dev_get_platdata(&pdev->dev);
-     struct device_node *np = pdev->dev.of_node;
+     const struct device *dev = &pdev->dev;
+     const struct of_dev_auxdata *lookup = dev_get_platdata(dev);
+     struct device_node *np = dev->of_node;
+     const struct of_device_id *match;
+
+     /*
+      * Allow user to use driver_override to bind this driver to a
+      * transparent bus device which has a different compatible string
+      * that's not listed in simple_pm_bus_of_match. We don't want to do any
+      * of the simple-pm-bus tasks for these devices, so return early.
+      */
+     if (pdev->driver_override)
+             return 0;
+
+     match = of_match_device(dev->driver->of_match_table, dev);
+     /*
+      * These are transparent bus devices (not simple-pm-bus matches) that
+      * have their child nodes populated automatically.  So, don't need to

drivers/bus/simple-pm-bus.c line 45

Quoted 10 lines

drivers/bus/simple-pm-bus.c +10 -0

+      * do anything more. We only match with the device if this driver is
+      * the most specific match because we don't want to incorrectly bind to
+      * a device that has a more specific driver.
+      */
+     if (match && match->data) {
+             if (of_property_match_string(np, "compatible", match->compatible) == 0)
+                     return 0;
+             else
+                     return -ENODEV;
+     }

This change is breaking the expected behavior for the already existent simple-bus nodes. All the simple-bus compatibles should be replaced now to simple-pm-bus. In my case, on some i.MX8 platforms, without the devlink, the devices suspend sequence changes (and even breaks).

Quoted 3 lines

To avoid breaking the already existent simple-bus nodes, maybe the logic should've been reversed: keep the simple-bus as is and add another compatible, IDK, something like simple-trasnparent-bus, or something.

The intent of this change IS to affect existing simple-bus nodes (but not in the way it's affecting you). But if it's breaking stuff, we obviously need to fix it.

Quoted 6 lines

I have a hunch on what's going on in your case, but can you point me to the specific simple-bus node that's getting affected? I'm expecting it to be a simple-bus node that gets added AFTER this driver is registered at device_initcall (module_init gets converted to device_initcall).

Quoted 3 lines

Also, can you try this hack patch to see if it helps your case?
https://lore.kernel.org/lkml/CAGETcx9U130Oq-umrvXME4JhEpO0Wadoki3kNxx=0-YvTR6PtQ@mail.gmail.com/

Quoted 2 lines

I have some thoughts on how I could fix this, but I need to think about a few cases.

Hi Abel,

Friendly reminder.

-Saravana

Quoted 81 lines · Saravana Kannan

Quoted 80 lines · Saravana Kannan

Quoted 70 lines · Abel Vesa

Quoted 52 lines · Saravana Kannan

fw_devlink could end up creating device links for bus only devices. However, bus only devices don't get probed and can block probe() or sync_state() [1] call backs of other devices. To avoid this, probe these devices using the simple-pm-bus driver.

However, there are instances of devices that are not simple buses (they get probed by their specific drivers) that also list the "simple-bus" (or other bus only compatible strings) in their compatible property to automatically populate their child devices. We still want these devices to get probed by their specific drivers. So, we make sure this driver only probes devices that are only buses.

[1] - https://lore.kernel.org/lkml/CAPDyKFo9Bxremkb1dDrr4OcXSpE0keVze94Cm=zrkOVxHHxBmQ@mail.gmail.com/

Fixes c442a0d18744 ("driver core: Set fw_devlink to "permissive" behavior by default")

Signed-off-by Saravana Kannan <[email protected]>

Tested-by Saravana Kannan <[email protected]>

Tested-by Ulf Hansson <[email protected]>

---
 drivers/bus/simple-pm-bus.c | 42 ++++++++++++++++++++++++++++++++++---
 1 file changed, 39 insertions(+), 3 deletions(-)

drivers/bus/simple-pm-bus.c +18 -3

index 01a3d0cd08ed..6b8d6257ed8a 100644
@@ -13,11 +13,36 @@
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
 
-
 static int simple_pm_bus_probe(struct platform_device *pdev)
 {
-     const struct of_dev_auxdata *lookup = dev_get_platdata(&pdev->dev);
-     struct device_node *np = pdev->dev.of_node;
+     const struct device *dev = &pdev->dev;
+     const struct of_dev_auxdata *lookup = dev_get_platdata(dev);
+     struct device_node *np = dev->of_node;
+     const struct of_device_id *match;
+
+     /*
+      * Allow user to use driver_override to bind this driver to a
+      * transparent bus device which has a different compatible string
+      * that's not listed in simple_pm_bus_of_match. We don't want to do any
+      * of the simple-pm-bus tasks for these devices, so return early.
+      */
+     if (pdev->driver_override)
+             return 0;
+
+     match = of_match_device(dev->driver->of_match_table, dev);
+     /*
+      * These are transparent bus devices (not simple-pm-bus matches) that
+      * have their child nodes populated automatically.  So, don't need to

drivers/bus/simple-pm-bus.c line 45

Quoted 10 lines

drivers/bus/simple-pm-bus.c +10 -0

+      * do anything more. We only match with the device if this driver is
+      * the most specific match because we don't want to incorrectly bind to
+      * a device that has a more specific driver.
+      */
+     if (match && match->data) {
+             if (of_property_match_string(np, "compatible", match->compatible) == 0)
+                     return 0;
+             else
+                     return -ENODEV;
+     }

This change is breaking the expected behavior for the already existent simple-bus nodes. All the simple-bus compatibles should be replaced now to simple-pm-bus. In my case, on some i.MX8 platforms, without the devlink, the devices suspend sequence changes (and even breaks).

Quoted 3 lines

To avoid breaking the already existent simple-bus nodes, maybe the logic should've been reversed: keep the simple-bus as is and add another compatible, IDK, something like simple-trasnparent-bus, or something.

The intent of this change IS to affect existing simple-bus nodes (but not in the way it's affecting you). But if it's breaking stuff, we obviously need to fix it.

Quoted 6 lines

Quoted 6 lines

I have a hunch on what's going on in your case, but can you point me to the specific simple-bus node that's getting affected? I'm expecting it to be a simple-bus node that gets added AFTER this driver is registered at device_initcall (module_init gets converted to device_initcall).

Well, for now, I can't pinpoint exactly one specific dts node. My usecase is a little bit more complicated since it's only happening in conjunction with a local change in the imx-mu-irq driver
in our local tree. Basically, the local change is forcing a suspend abort by calling pm_system_wake if there is an irq from SCU triggered after the suspend noirq imx-mu-irq driver callback has been called.
If we replace all the simple-bus with simple-pm-bus, everything works fine.

Quoted 3 lines

Quoted 3 lines

Also, can you try this hack patch to see if it helps your case?
https://lore.kernel.org/lkml/CAGETcx9U130Oq-umrvXME4JhEpO0Wadoki3kNxx=0-YvTR6PtQ@mail.gmail.com/

Nope, it doesn't work with this patch either.

And the funny thing is, even if I add the pm_runtime_enable to your patch like this:

drivers/bus/simple-pm-bus.c +4 -2

index 6b8d6257ed8a..a13324fd700b 100644
@@ -38,9 +38,11 @@ static int simple_pm_bus_probe(struct platform_device *pdev)
         * a device that has a more specific driver.
         */
        if (match && match->data) {
-               if (of_property_match_string(np, "compatible", match->compatible) == 0)
+               if (of_property_match_string(np, "compatible", match->compatible) == 0) {
+                       pm_runtime_enable(&pdev->dev);
+                       of_platform_populate(np, NULL, lookup, &pdev->dev);
                        return 0;
-               else
+               } else
                        return -ENODEV;
        }
 

I get the following warning:

[    1.009392] ------------[ cut here ]------------
[    1.013681] Enabling runtime PM for inactive device (bus@5d000000) with active children
[    1.021721] WARNING: CPU: 3 PID: 1 at drivers/base/power/runtime.c:1439 pm_runtime_enable+0xc0/0x100
[    1.030794] Modules linked in:                                              
[    1.033837] CPU: 3 PID: 1 Comm: swapper/0 Not tainted 5.15.0-04906-gc0a8c67532b6-dirty #226
[    1.042175] Hardware name: Freescale i.MX8QXP MEK (DT)
[    1.047302] pstate: 600000c5 (nZCv daIF -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[    1.054248] pc : pm_runtime_enable+0xc0/0x100
[    1.058594] lr : pm_runtime_enable+0xc0/0x100
[    1.062938] sp : ffff800011e4bb60                                           
[    1.066237] x29: ffff800011e4bb60 x28: 0000000000000000 x27: ffff8000119204d0
[    1.073361] x26: ffff8000119c1060 x25: ffff8000119c1078 x24: ffff80001190f0a0
[    1.080484] x23: 0000000000000000 x22: 0000000000000000 x21: ffff0008006a00f4
[    1.087608] x20: 0000000000000000 x19: ffff0008006a0010 x18: 0000000000000030
[    1.094732] x17: 6974636120687469 x16: 7720293030303030 x15: 3064354073756228
[    1.101856] x14: 2065636976656420 x13: ffff800011c21dc0 x12: 0000000000000246
[    1.108981] x11: 00000000000000c2 x10: ffff800011c79dc0 x9 : 00000000fffff000
[    1.116103] x8 : ffff800011c21dc0 x7 : ffff800011c79dc0 x6 : 0000000000000000
[    1.123227] x5 : 000000000000bff4 x4 : 0000000000000000 x3 : 00000000ffffefff
[    1.130351] x2 : ffff800011c21d30 x1 : 6bf7c63081c63600 x0 : 0000000000000000
[    1.137478] Call trace:                                                     
[    1.139910]  pm_runtime_enable+0xc0/0x100
[    1.143903]  simple_pm_bus_probe+0x80/0xf0
[    1.147986]  platform_probe+0x6c/0xe0
[    1.151637]  really_probe.part.0+0x9c/0x310
[    1.155805]  __driver_probe_device+0x98/0x144
[    1.160149]  driver_probe_device+0xc8/0x15c
[    1.164319]  __driver_attach+0xfc/0x190
[    1.168141]  bus_for_each_dev+0x70/0xc0
[    1.171964]  driver_attach+0x28/0x3c                                        
[    1.175526]  bus_add_driver+0x108/0x1f0
[    1.179348]  driver_register+0x7c/0x130
[    1.183173]  __platform_driver_register+0x2c/0x40
[    1.187862]  simple_pm_bus_driver_init+0x20/0x2c
[    1.192466]  do_one_initcall+0x54/0x1bc
[    1.196289]  kernel_init_freeable+0x210/0x294
[    1.200633]  kernel_init+0x28/0x130                                         
[    1.204110]  ret_from_fork+0x10/0x20                                        
[    1.207678] ---[ end trace 719a02e69bfeb03f ]---

though it should be behaving exactly like in the case of simple-pm-bus.

Maybe I'm missing something here.

I'll keep digging.

Quoted 7 lines

Quoted 2 lines

I have some thoughts on how I could fix this, but I need to think about a few cases.

Hi Abel,

Friendly reminder.

Sorry for the late reply.

Quoted 1 line

-Saravana

Quoted 167 lines · Abel Vesa

Quoted 81 lines · Saravana Kannan

Quoted 80 lines · Saravana Kannan

Quoted 70 lines · Abel Vesa

Quoted 52 lines · Saravana Kannan

fw_devlink could end up creating device links for bus only devices. However, bus only devices don't get probed and can block probe() or sync_state() [1] call backs of other devices. To avoid this, probe these devices using the simple-pm-bus driver.

However, there are instances of devices that are not simple buses (they get probed by their specific drivers) that also list the "simple-bus" (or other bus only compatible strings) in their compatible property to automatically populate their child devices. We still want these devices to get probed by their specific drivers. So, we make sure this driver only probes devices that are only buses.

[1] - https://lore.kernel.org/lkml/CAPDyKFo9Bxremkb1dDrr4OcXSpE0keVze94Cm=zrkOVxHHxBmQ@mail.gmail.com/

Fixes c442a0d18744 ("driver core: Set fw_devlink to "permissive" behavior by default")

Signed-off-by Saravana Kannan <[email protected]>

Tested-by Saravana Kannan <[email protected]>

Tested-by Ulf Hansson <[email protected]>

---
 drivers/bus/simple-pm-bus.c | 42 ++++++++++++++++++++++++++++++++++---
 1 file changed, 39 insertions(+), 3 deletions(-)

drivers/bus/simple-pm-bus.c +18 -3

index 01a3d0cd08ed..6b8d6257ed8a 100644
@@ -13,11 +13,36 @@
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
 
-
 static int simple_pm_bus_probe(struct platform_device *pdev)
 {
-     const struct of_dev_auxdata *lookup = dev_get_platdata(&pdev->dev);
-     struct device_node *np = pdev->dev.of_node;
+     const struct device *dev = &pdev->dev;
+     const struct of_dev_auxdata *lookup = dev_get_platdata(dev);
+     struct device_node *np = dev->of_node;
+     const struct of_device_id *match;
+
+     /*
+      * Allow user to use driver_override to bind this driver to a
+      * transparent bus device which has a different compatible string
+      * that's not listed in simple_pm_bus_of_match. We don't want to do any
+      * of the simple-pm-bus tasks for these devices, so return early.
+      */
+     if (pdev->driver_override)
+             return 0;
+
+     match = of_match_device(dev->driver->of_match_table, dev);
+     /*
+      * These are transparent bus devices (not simple-pm-bus matches) that
+      * have their child nodes populated automatically.  So, don't need to

drivers/bus/simple-pm-bus.c line 45

Quoted 10 lines

drivers/bus/simple-pm-bus.c +10 -0

+      * do anything more. We only match with the device if this driver is
+      * the most specific match because we don't want to incorrectly bind to
+      * a device that has a more specific driver.
+      */
+     if (match && match->data) {
+             if (of_property_match_string(np, "compatible", match->compatible) == 0)
+                     return 0;
+             else
+                     return -ENODEV;
+     }

This change is breaking the expected behavior for the already existent simple-bus nodes. All the simple-bus compatibles should be replaced now to simple-pm-bus. In my case, on some i.MX8 platforms, without the devlink, the devices suspend sequence changes (and even breaks).

Quoted 3 lines

To avoid breaking the already existent simple-bus nodes, maybe the logic should've been reversed: keep the simple-bus as is and add another compatible, IDK, something like simple-trasnparent-bus, or something.

The intent of this change IS to affect existing simple-bus nodes (but not in the way it's affecting you). But if it's breaking stuff, we obviously need to fix it.

Quoted 6 lines

Quoted 6 lines

I have a hunch on what's going on in your case, but can you point me to the specific simple-bus node that's getting affected? I'm expecting it to be a simple-bus node that gets added AFTER this driver is registered at device_initcall (module_init gets converted to device_initcall).

Well, for now, I can't pinpoint exactly one specific dts node. My usecase is a little bit more complicated since it's only happening in conjunction with a local change in the imx-mu-irq driver
in our local tree. Basically, the local change is forcing a suspend abort by calling pm_system_wake if there is an irq from SCU triggered after the suspend noirq imx-mu-irq driver callback has been called.
If we replace all the simple-bus with simple-pm-bus, everything works fine.

Quoted 3 lines

Quoted 3 lines

Also, can you try this hack patch to see if it helps your case?
https://lore.kernel.org/lkml/CAGETcx9U130Oq-umrvXME4JhEpO0Wadoki3kNxx=0-YvTR6PtQ@mail.gmail.com/

Nope, it doesn't work with this patch either.

And the funny thing is, even if I add the pm_runtime_enable to your patch like this:

drivers/bus/simple-pm-bus.c +4 -2

index 6b8d6257ed8a..a13324fd700b 100644
@@ -38,9 +38,11 @@ static int simple_pm_bus_probe(struct platform_device *pdev)
         * a device that has a more specific driver.
         */
        if (match && match->data) {
-               if (of_property_match_string(np, "compatible", match->compatible) == 0)
+               if (of_property_match_string(np, "compatible", match->compatible) == 0) {
+                       pm_runtime_enable(&pdev->dev);
+                       of_platform_populate(np, NULL, lookup, &pdev->dev);
                        return 0;
-               else
+               } else
                        return -ENODEV;
        }
 

I get the following warning:

[    1.009392] ------------[ cut here ]------------
[    1.013681] Enabling runtime PM for inactive device (bus@5d000000) with active children
[    1.021721] WARNING: CPU: 3 PID: 1 at drivers/base/power/runtime.c:1439 pm_runtime_enable+0xc0/0x100
[    1.030794] Modules linked in:                                              
[    1.033837] CPU: 3 PID: 1 Comm: swapper/0 Not tainted 5.15.0-04906-gc0a8c67532b6-dirty #226
[    1.042175] Hardware name: Freescale i.MX8QXP MEK (DT)
[    1.047302] pstate: 600000c5 (nZCv daIF -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[    1.054248] pc : pm_runtime_enable+0xc0/0x100
[    1.058594] lr : pm_runtime_enable+0xc0/0x100
[    1.062938] sp : ffff800011e4bb60                                           
[    1.066237] x29: ffff800011e4bb60 x28: 0000000000000000 x27: ffff8000119204d0
[    1.073361] x26: ffff8000119c1060 x25: ffff8000119c1078 x24: ffff80001190f0a0
[    1.080484] x23: 0000000000000000 x22: 0000000000000000 x21: ffff0008006a00f4
[    1.087608] x20: 0000000000000000 x19: ffff0008006a0010 x18: 0000000000000030
[    1.094732] x17: 6974636120687469 x16: 7720293030303030 x15: 3064354073756228
[    1.101856] x14: 2065636976656420 x13: ffff800011c21dc0 x12: 0000000000000246
[    1.108981] x11: 00000000000000c2 x10: ffff800011c79dc0 x9 : 00000000fffff000
[    1.116103] x8 : ffff800011c21dc0 x7 : ffff800011c79dc0 x6 : 0000000000000000
[    1.123227] x5 : 000000000000bff4 x4 : 0000000000000000 x3 : 00000000ffffefff
[    1.130351] x2 : ffff800011c21d30 x1 : 6bf7c63081c63600 x0 : 0000000000000000
[    1.137478] Call trace:                                                     
[    1.139910]  pm_runtime_enable+0xc0/0x100
[    1.143903]  simple_pm_bus_probe+0x80/0xf0
[    1.147986]  platform_probe+0x6c/0xe0
[    1.151637]  really_probe.part.0+0x9c/0x310
[    1.155805]  __driver_probe_device+0x98/0x144
[    1.160149]  driver_probe_device+0xc8/0x15c
[    1.164319]  __driver_attach+0xfc/0x190
[    1.168141]  bus_for_each_dev+0x70/0xc0
[    1.171964]  driver_attach+0x28/0x3c                                        
[    1.175526]  bus_add_driver+0x108/0x1f0
[    1.179348]  driver_register+0x7c/0x130
[    1.183173]  __platform_driver_register+0x2c/0x40
[    1.187862]  simple_pm_bus_driver_init+0x20/0x2c
[    1.192466]  do_one_initcall+0x54/0x1bc
[    1.196289]  kernel_init_freeable+0x210/0x294
[    1.200633]  kernel_init+0x28/0x130                                         
[    1.204110]  ret_from_fork+0x10/0x20                                        
[    1.207678] ---[ end trace 719a02e69bfeb03f ]---

Quoted 6 lines

though it should be behaving exactly like in the case of simple-pm-bus.

Maybe I'm missing something here.

I'll keep digging.

One more thing.

So basically, your already upstream patch removes the devlinks for all the simple-bus nodes.

By adding your hack patch, it also removes devlinks between subnodes of the simple-bus node.

Quoted 11 lines

Quoted 7 lines

Quoted 2 lines

I have some thoughts on how I could fix this, but I need to think about a few cases.

Hi Abel,

Friendly reminder.

Sorry for the late reply.

Quoted 1 line

-Saravana

Quoted 90 lines · Abel Vesa

Quoted 89 lines · Abel Vesa

Quoted 87 lines · Saravana Kannan

Quoted 86 lines · Saravana Kannan

Quoted 70 lines · Abel Vesa

Quoted 52 lines · Saravana Kannan

fw_devlink could end up creating device links for bus only devices. However, bus only devices don't get probed and can block probe() or sync_state() [1] call backs of other devices. To avoid this, probe these devices using the simple-pm-bus driver.

However, there are instances of devices that are not simple buses (they get probed by their specific drivers) that also list the "simple-bus" (or other bus only compatible strings) in their compatible property to automatically populate their child devices. We still want these devices to get probed by their specific drivers. So, we make sure this driver only probes devices that are only buses.

[1] - https://lore.kernel.org/lkml/CAPDyKFo9Bxremkb1dDrr4OcXSpE0keVze94Cm=zrkOVxHHxBmQ@mail.gmail.com/

Fixes c442a0d18744 ("driver core: Set fw_devlink to "permissive" behavior by default")

Signed-off-by Saravana Kannan <[email protected]>

Tested-by Saravana Kannan <[email protected]>

Tested-by Ulf Hansson <[email protected]>

---
 drivers/bus/simple-pm-bus.c | 42 ++++++++++++++++++++++++++++++++++---
 1 file changed, 39 insertions(+), 3 deletions(-)

drivers/bus/simple-pm-bus.c +18 -3

index 01a3d0cd08ed..6b8d6257ed8a 100644
@@ -13,11 +13,36 @@
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
 
-
 static int simple_pm_bus_probe(struct platform_device *pdev)
 {
-     const struct of_dev_auxdata *lookup = dev_get_platdata(&pdev->dev);
-     struct device_node *np = pdev->dev.of_node;
+     const struct device *dev = &pdev->dev;
+     const struct of_dev_auxdata *lookup = dev_get_platdata(dev);
+     struct device_node *np = dev->of_node;
+     const struct of_device_id *match;
+
+     /*
+      * Allow user to use driver_override to bind this driver to a
+      * transparent bus device which has a different compatible string
+      * that's not listed in simple_pm_bus_of_match. We don't want to do any
+      * of the simple-pm-bus tasks for these devices, so return early.
+      */
+     if (pdev->driver_override)
+             return 0;
+
+     match = of_match_device(dev->driver->of_match_table, dev);
+     /*
+      * These are transparent bus devices (not simple-pm-bus matches) that
+      * have their child nodes populated automatically.  So, don't need to

drivers/bus/simple-pm-bus.c line 45

Quoted 10 lines

drivers/bus/simple-pm-bus.c +10 -0

+      * do anything more. We only match with the device if this driver is
+      * the most specific match because we don't want to incorrectly bind to
+      * a device that has a more specific driver.
+      */
+     if (match && match->data) {
+             if (of_property_match_string(np, "compatible", match->compatible) == 0)
+                     return 0;
+             else
+                     return -ENODEV;
+     }

This change is breaking the expected behavior for the already existent simple-bus nodes. All the simple-bus compatibles should be replaced now to simple-pm-bus. In my case, on some i.MX8 platforms, without the devlink, the devices suspend sequence changes (and even breaks).

Quoted 3 lines

To avoid breaking the already existent simple-bus nodes, maybe the logic should've been reversed: keep the simple-bus as is and add another compatible, IDK, something like simple-trasnparent-bus, or something.

The intent of this change IS to affect existing simple-bus nodes (but not in the way it's affecting you). But if it's breaking stuff, we obviously need to fix it.

I have a hunch on what's going on in your case, but can you point me to the specific simple-bus node that's getting affected? I'm expecting it to be a simple-bus node that gets added AFTER this driver is registered at device_initcall (module_init gets converted to device_initcall).

Quoted 8 lines

Quoted 8 lines

Well, for now, I can't pinpoint exactly one specific dts node. My usecase is a little bit more complicated since it's only happening in conjunction with a local change in the imx-mu-irq driver
in our local tree. Basically, the local change is forcing a suspend abort by calling pm_system_wake if there is an irq from SCU triggered after the suspend noirq imx-mu-irq driver callback has been called.
If we replace all the simple-bus with simple-pm-bus, everything works fine.

Quoted 70 lines

Quoted 70 lines

Quoted 4 lines

Quoted 4 lines

Also, can you try this hack patch to see if it helps your case?
https://lore.kernel.org/lkml/CAGETcx9U130Oq-umrvXME4JhEpO0Wadoki3kNxx=0-YvTR6PtQ@mail.gmail.com/

Nope, it doesn't work with this patch either.

And the funny thing is, even if I add the pm_runtime_enable to your patch like this:

drivers/bus/simple-pm-bus.c +4 -2

index 6b8d6257ed8a..a13324fd700b 100644
@@ -38,9 +38,11 @@ static int simple_pm_bus_probe(struct platform_device *pdev)
         * a device that has a more specific driver.
         */
        if (match && match->data) {
-               if (of_property_match_string(np, "compatible", match->compatible) == 0)
+               if (of_property_match_string(np, "compatible", match->compatible) == 0) {
+                       pm_runtime_enable(&pdev->dev);
+                       of_platform_populate(np, NULL, lookup, &pdev->dev);
                        return 0;
-               else
+               } else
                        return -ENODEV;
        }
 

I get the following warning:

[    1.009392] ------------[ cut here ]------------
[    1.013681] Enabling runtime PM for inactive device (bus@5d000000) with active children
[    1.021721] WARNING: CPU: 3 PID: 1 at drivers/base/power/runtime.c:1439 pm_runtime_enable+0xc0/0x100
[    1.030794] Modules linked in:
[    1.033837] CPU: 3 PID: 1 Comm: swapper/0 Not tainted 5.15.0-04906-gc0a8c67532b6-dirty #226
[    1.042175] Hardware name: Freescale i.MX8QXP MEK (DT)
[    1.047302] pstate: 600000c5 (nZCv daIF -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[    1.054248] pc : pm_runtime_enable+0xc0/0x100
[    1.058594] lr : pm_runtime_enable+0xc0/0x100
[    1.062938] sp : ffff800011e4bb60
[    1.066237] x29: ffff800011e4bb60 x28: 0000000000000000 x27: ffff8000119204d0
[    1.073361] x26: ffff8000119c1060 x25: ffff8000119c1078 x24: ffff80001190f0a0
[    1.080484] x23: 0000000000000000 x22: 0000000000000000 x21: ffff0008006a00f4
[    1.087608] x20: 0000000000000000 x19: ffff0008006a0010 x18: 0000000000000030
[    1.094732] x17: 6974636120687469 x16: 7720293030303030 x15: 3064354073756228
[    1.101856] x14: 2065636976656420 x13: ffff800011c21dc0 x12: 0000000000000246
[    1.108981] x11: 00000000000000c2 x10: ffff800011c79dc0 x9 : 00000000fffff000
[    1.116103] x8 : ffff800011c21dc0 x7 : ffff800011c79dc0 x6 : 0000000000000000
[    1.123227] x5 : 000000000000bff4 x4 : 0000000000000000 x3 : 00000000ffffefff
[    1.130351] x2 : ffff800011c21d30 x1 : 6bf7c63081c63600 x0 : 0000000000000000
[    1.137478] Call trace:
[    1.139910]  pm_runtime_enable+0xc0/0x100
[    1.143903]  simple_pm_bus_probe+0x80/0xf0
[    1.147986]  platform_probe+0x6c/0xe0
[    1.151637]  really_probe.part.0+0x9c/0x310
[    1.155805]  __driver_probe_device+0x98/0x144
[    1.160149]  driver_probe_device+0xc8/0x15c
[    1.164319]  __driver_attach+0xfc/0x190
[    1.168141]  bus_for_each_dev+0x70/0xc0
[    1.171964]  driver_attach+0x28/0x3c
[    1.175526]  bus_add_driver+0x108/0x1f0
[    1.179348]  driver_register+0x7c/0x130
[    1.183173]  __platform_driver_register+0x2c/0x40
[    1.187862]  simple_pm_bus_driver_init+0x20/0x2c
[    1.192466]  do_one_initcall+0x54/0x1bc
[    1.196289]  kernel_init_freeable+0x210/0x294
[    1.200633]  kernel_init+0x28/0x130
[    1.204110]  ret_from_fork+0x10/0x20
[    1.207678] ---[ end trace 719a02e69bfeb03f ]---

Quoted 1 line

Quoted 1 line

though it should be behaving exactly like in the case of simple-pm-bus.

Quoted 5 lines

Quoted 5 lines

Maybe I'm missing something here.

I'll keep digging.

Looking at this, it seems the devlinks are actually dropped after being created:

[   36.993371] Hardware name: Freescale i.MX8QXP MEK (DT)
[   36.998498] Call trace:
[   37.000928]  dump_backtrace+0x0/0x19c
[   37.004578]  show_stack+0x1c/0x70
[   37.007878]  dump_stack_lvl+0x68/0x84
[   37.011527]  dump_stack+0x1c/0x38
[   37.014826]  __device_link_del+0x40/0xbc
[   37.018738]  device_link_drop_managed+0x74/0x90
[   37.023253]  device_links_driver_bound+0x1b4/0x280
[   37.028031]  driver_bound+0x68/0xd0
[   37.031509]  really_probe.part.0+0x258/0x310
[   37.035763]  __driver_probe_device+0x98/0x144
[   37.040107]  driver_probe_device+0xc8/0x15c
[   37.044279]  __driver_attach+0xfc/0x190
[   37.048100]  bus_for_each_dev+0x70/0xc0
[   37.051924]  driver_attach+0x28/0x3c
[   37.055484]  bus_add_driver+0x108/0x1f0
[   37.059306]  driver_register+0x7c/0x130
[   37.063131]  __platform_driver_register+0x2c/0x40
[   37.067820]  simple_pm_bus_driver_init+0x20/0x2c
[   37.072424]  do_one_initcall+0x54/0x1bc
[   37.076249]  kernel_init_freeable+0x210/0x294
[   37.080591]  kernel_init+0x28/0x130
[   37.084068]  ret_from_fork+0x10/0x20
[   37.087688] simple-pm-bus bus@56220000: Dropping the link to 5d090000.gpio
[   37.094501] device: 'platform:5d090000.gpio--platform:bus@56220000': device_unregister

So, before your patch, the devlinks were not dropped because there was no driver to probe for the "simple-bus" compatible.

And I think this tells us why your hack patch didn't solve the problem either.

fw_devlink could end up creating device links for bus only devices. However, bus only devices don't get probed and can block probe() or sync_state() [1] call backs of other devices. To avoid this, probe these devices using the simple-pm-bus driver.

However, there are instances of devices that are not simple buses (they get probed by their specific drivers) that also list the "simple-bus" (or other bus only compatible strings) in their compatible property to automatically populate their child devices. We still want these devices to get probed by their specific drivers. So, we make sure this driver only probes devices that are only buses.

[1] - https://lore.kernel.org/lkml/CAPDyKFo9Bxremkb1dDrr4OcXSpE0keVze94Cm=zrkOVxHHxBmQ@mail.gmail.com/

Fixes c442a0d18744 ("driver core: Set fw_devlink to "permissive" behavior by default")

Signed-off-by Saravana Kannan <[email protected]>

Tested-by Saravana Kannan <[email protected]>

Tested-by Ulf Hansson <[email protected]>

Hi Saravana,

Quoted 12 lines · Saravana Kannan

fw_devlink could end up creating device links for bus only devices. However, bus only devices don't get probed and can block probe() or sync_state() [1] call backs of other devices. To avoid this, probe these devices using the simple-pm-bus driver.

However, there are instances of devices that are not simple buses (they get probed by their specific drivers) that also list the "simple-bus" (or other bus only compatible strings) in their compatible property to automatically populate their child devices. We still want these devices to get probed by their specific drivers. So, we make sure this driver only probes devices that are only buses.

Quoted 5 lines

[1] - https://lore.kernel.org/lkml/CAPDyKFo9Bxremkb1dDrr4OcXSpE0keVze94Cm=zrkOVxHHxBmQ@mail.gmail.com/

Fixes c442a0d18744 ("driver core: Set fw_devlink to "permissive" behavior by default")

Signed-off-by Saravana Kannan <[email protected]>

Tested-by Saravana Kannan <[email protected]>

Tested-by Ulf Hansson <[email protected]>

This patch landed in stable/linux-5.10.y as commit d5f13bbb5104 and it broke suspend/resume on at least one TI AM335x board I'm testing on:
upstream dts: arch/arm/boot/dts/am335x-icev2.dts, upstream defconfig:
arch/arm/configs/omap2plus_defconfig.

Bisecting between vanilla v5.10 (good) and stable/linux-5.10.y (bad) pointed me to this patch, and I confirmed that reverting just this patch on top of stable/linux-5.10.y makes it work again.

Also interesting, this same platform works fine on vanilla v5.15, which also includes this patch. That suggests that either 1) this patch should not have been backported to v5.10 stable or 2) there are some other dependencies that are missing in v5.10.

Since vanilla v5.10 works fine, I'm leaning towards (1), but if you have any ideas for deps that need backporting, I'm happy to try.

I haven't debugged exactly where it's hanging yet, but, enabling CONFIG_DEBUG_DRIVER=y, and suspending with "no_console_suspend" on the command line, the last line before it hangs is:

   [   28.129966] simple-pm-bus ocp: noirq power domain suspend

Any ideas?

Quoted 32 lines · Kevin Hilman

Hi Saravana,

Quoted 12 lines · Saravana Kannan

fw_devlink could end up creating device links for bus only devices. However, bus only devices don't get probed and can block probe() or sync_state() [1] call backs of other devices. To avoid this, probe these devices using the simple-pm-bus driver.

However, there are instances of devices that are not simple buses (they get probed by their specific drivers) that also list the "simple-bus" (or other bus only compatible strings) in their compatible property to automatically populate their child devices. We still want these devices to get probed by their specific drivers. So, we make sure this driver only probes devices that are only buses.

Quoted 5 lines

[1] - https://lore.kernel.org/lkml/CAPDyKFo9Bxremkb1dDrr4OcXSpE0keVze94Cm=zrkOVxHHxBmQ@mail.gmail.com/

Fixes c442a0d18744 ("driver core: Set fw_devlink to "permissive" behavior by default")

Signed-off-by Saravana Kannan <[email protected]>

Tested-by Saravana Kannan <[email protected]>

Tested-by Ulf Hansson <[email protected]>

This patch landed in stable/linux-5.10.y as commit d5f13bbb5104 and it broke suspend/resume on at least one TI AM335x board I'm testing on:
upstream dts: arch/arm/boot/dts/am335x-icev2.dts, upstream defconfig:
arch/arm/configs/omap2plus_defconfig.

Bisecting between vanilla v5.10 (good) and stable/linux-5.10.y (bad) pointed me to this patch, and I confirmed that reverting just this patch on top of stable/linux-5.10.y makes it work again.

Quoted 7 lines

Also interesting, this same platform works fine on vanilla v5.15, which also includes this patch. That suggests that either 1) this patch should not have been backported to v5.10 stable or 2) there are some other dependencies that are missing in v5.10.

Since vanilla v5.10 works fine, I'm leaning towards (1), but if you have any ideas for deps that need backporting, I'm happy to try.

Oh wow! I didn't realize I made so many changes AFTER 5.10! Unless I'm doing something wrong with my git commands.

$ git log v5.10..v5.15 --oneline -- drivers/of/property.c
$ git log v5.10..v5.15 --oneline --author=saravanak -- drivers/base/

If you don't think I got my git command completely wrong, yeah, way too many patches are missing on 5.10. I'd go with the option of dropping this patch on 5.10.

Quoted 4 lines

I haven't debugged exactly where it's hanging yet, but, enabling CONFIG_DEBUG_DRIVER=y, and suspending with "no_console_suspend" on the command line, the last line before it hangs is:

Quoted 3 lines

   [   28.129966] simple-pm-bus ocp: noirq power domain suspend

Any ideas?

I'd guess it's either a sync_state() happening too soon since some of the dependencies aren't tracked. Or some dependency cycle that'd be handled correctly if the rest of the patches were picked up. Yeah, a pretty broad/vague answer.

Quoted 46 lines · Saravana Kannan

Quoted 32 lines · Kevin Hilman

Hi Saravana,

Quoted 12 lines · Saravana Kannan

fw_devlink could end up creating device links for bus only devices. However, bus only devices don't get probed and can block probe() or sync_state() [1] call backs of other devices. To avoid this, probe these devices using the simple-pm-bus driver.

However, there are instances of devices that are not simple buses (they get probed by their specific drivers) that also list the "simple-bus" (or other bus only compatible strings) in their compatible property to automatically populate their child devices. We still want these devices to get probed by their specific drivers. So, we make sure this driver only probes devices that are only buses.

Quoted 5 lines

[1] - https://lore.kernel.org/lkml/CAPDyKFo9Bxremkb1dDrr4OcXSpE0keVze94Cm=zrkOVxHHxBmQ@mail.gmail.com/

Fixes c442a0d18744 ("driver core: Set fw_devlink to "permissive" behavior by default")

Signed-off-by Saravana Kannan <[email protected]>

Tested-by Saravana Kannan <[email protected]>

Tested-by Ulf Hansson <[email protected]>

This patch landed in stable/linux-5.10.y as commit d5f13bbb5104 and it broke suspend/resume on at least one TI AM335x board I'm testing on:
upstream dts: arch/arm/boot/dts/am335x-icev2.dts, upstream defconfig:
arch/arm/configs/omap2plus_defconfig.

Bisecting between vanilla v5.10 (good) and stable/linux-5.10.y (bad) pointed me to this patch, and I confirmed that reverting just this patch on top of stable/linux-5.10.y makes it work again.

Quoted 7 lines

Also interesting, this same platform works fine on vanilla v5.15, which also includes this patch. That suggests that either 1) this patch should not have been backported to v5.10 stable or 2) there are some other dependencies that are missing in v5.10.

Since vanilla v5.10 works fine, I'm leaning towards (1), but if you have any ideas for deps that need backporting, I'm happy to try.

Oh wow! I didn't realize I made so many changes AFTER 5.10! Unless I'm doing something wrong with my git commands.

$ git log v5.10..v5.15 --oneline -- drivers/of/property.c
$ git log v5.10..v5.15 --oneline --author=saravanak -- drivers/base/

Quoted 3 lines

If you don't think I got my git command completely wrong, yeah, way too many patches are missing on 5.10. I'd go with the option of dropping this patch on 5.10.

I agree. Could you submit a revert for v5.10 stable? As the patch author, it's probably better if it comes from you.

The simple-pm-bus driver is mandatory for CONFIG_OF based platforms to work with fw_devlink. So, always compile it in for CONFIG_OF and delete the config since it's no longer necessary.

Signed-off-by Saravana Kannan <[email protected]>

Tested-by Ulf Hansson <[email protected]>

The simple-pm-bus driver is mandatory for CONFIG_OF based platforms to work with fw_devlink. So, always compile it in for CONFIG_OF and delete the config since it's no longer necessary.

Signed-off-by Saravana Kannan <[email protected]>

Tested-by Ulf Hansson <[email protected]>

---
 arch/arm/configs/multi_v7_defconfig |  1 -
 arch/arm/configs/oxnas_v6_defconfig |  1 -
 arch/arm/configs/shmobile_defconfig |  1 -
 arch/arm/mach-omap2/Kconfig         |  1 -
 arch/arm64/configs/defconfig        |  1 -
 drivers/bus/Kconfig                 | 12 ------------
 drivers/bus/Makefile                |  2 +-
 drivers/soc/canaan/Kconfig          |  1 -
 8 files changed, 1 insertion(+), 19 deletions(-)

arch/arm/configs/multi_v7_defconfig +0 -1

index d9abaae118dd..362720ae8d65 100644
@@ -196,7 +196,6 @@ CONFIG_PCI_EPF_TEST=m
 CONFIG_DEVTMPFS=y
 CONFIG_DEVTMPFS_MOUNT=y
 CONFIG_OMAP_OCP2SCP=y
-CONFIG_SIMPLE_PM_BUS=y
 CONFIG_MTD=y
 CONFIG_MTD_CMDLINE_PARTS=y
 CONFIG_MTD_BLOCK=y

arch/arm/configs/oxnas_v6_defconfig +0 -1

index cae0db6b4eaf..de37f7e90999 100644
@@ -46,7 +46,6 @@ CONFIG_DEVTMPFS=y
 CONFIG_DEVTMPFS_MOUNT=y
 CONFIG_DMA_CMA=y
 CONFIG_CMA_SIZE_MBYTES=64
-CONFIG_SIMPLE_PM_BUS=y
 CONFIG_MTD=y
 CONFIG_MTD_CMDLINE_PARTS=y
 CONFIG_MTD_BLOCK=y

arch/arm/configs/shmobile_defconfig +0 -1

index d9a27e4e0914..18d2a960b2d2 100644
@@ -40,7 +40,6 @@ CONFIG_PCI_RCAR_GEN2=y
 CONFIG_PCIE_RCAR_HOST=y
 CONFIG_DEVTMPFS=y
 CONFIG_DEVTMPFS_MOUNT=y
-CONFIG_SIMPLE_PM_BUS=y
 CONFIG_MTD=y
 CONFIG_MTD_BLOCK=y
 CONFIG_MTD_CFI=y

arch/arm/mach-omap2/Kconfig +0 -1

index 7df8f5276ddf..02f2f3157f07 100644
@@ -112,7 +112,6 @@ config ARCH_OMAP2PLUS
        select PM_GENERIC_DOMAINS
        select PM_GENERIC_DOMAINS_OF
        select RESET_CONTROLLER
-       select SIMPLE_PM_BUS
        select SOC_BUS
        select TI_SYSC
        select OMAP_IRQCHIP

arch/arm64/configs/defconfig +0 -1

index f423d08b9a71..474b1f2e3f06 100644
@@ -245,7 +245,6 @@ CONFIG_DEVTMPFS_MOUNT=y
 CONFIG_FW_LOADER_USER_HELPER=y
 CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y
 CONFIG_HISILICON_LPC=y
-CONFIG_SIMPLE_PM_BUS=y
 CONFIG_FSL_MC_BUS=y
 CONFIG_TEGRA_ACONNECT=m
 CONFIG_GNSS=m

drivers/bus/Kconfig +0 -12

index e7f7eee6ee9a..dc3801369488 100644
@@ -141,18 +141,6 @@ config QCOM_EBI2
          Interface 2, which can be used to connect things like NAND Flash,
          SRAM, ethernet adapters, FPGAs and LCD displays.
 
-config SIMPLE_PM_BUS
-       tristate "Simple Power-Managed Bus Driver"
-       depends on OF && PM
-       help
-         Driver for transparent busses that don't need a real driver, but
-         where the bus controller is part of a PM domain, or under the control
-         of a functional clock, and thus relies on runtime PM for managing
-         this PM domain and/or clock.
-         An example of such a bus controller is the Renesas Bus State
-         Controller (BSC, sometimes called "LBSC within Bus Bridge", or
-         "External Bus Interface") as found on several Renesas ARM SoCs.
-
 config SUN50I_DE2_BUS
        bool "Allwinner A64 DE2 Bus Driver"
          default ARM64

drivers/bus/Makefile +1 -1

index 397e35392bff..86aacd36a56d 100644
@@ -26,7 +26,7 @@ obj-$(CONFIG_OMAP_OCP2SCP)    += omap-ocp2scp.o
 obj-$(CONFIG_QCOM_EBI2)                += qcom-ebi2.o
 obj-$(CONFIG_SUN50I_DE2_BUS)   += sun50i-de2.o
 obj-$(CONFIG_SUNXI_RSB)                += sunxi-rsb.o
-obj-$(CONFIG_SIMPLE_PM_BUS)    += simple-pm-bus.o
+obj-$(CONFIG_OF)               += simple-pm-bus.o
 obj-$(CONFIG_TEGRA_ACONNECT)   += tegra-aconnect.o
 obj-$(CONFIG_TEGRA_GMI)                += tegra-gmi.o
 obj-$(CONFIG_TI_PWMSS)         += ti-pwmss.o

drivers/soc/canaan/Kconfig +0 -2

index 8179b69518b4..853096b7e84c 100644
@@ -5,7 +5,6 @@ config SOC_K210_SYSCTL
        depends on RISCV && SOC_CANAAN && OF
        default SOC_CANAAN
         select PM
-        select SIMPLE_PM_BUS
         select SYSCON
         select MFD_SYSCON
        help
--

2.33.0.685.g46640cef36-goog

Hi Saravana,

This patch broke v5.15-rc6 on RB5 (sm8250 | qcom/qrb5165-rb5.dts). I can't boot past this point https://www.irccloud.com/pastebin/raw/Nv6ZwHmW.

Hi Saravana,

This patch broke v5.15-rc6 on RB5 (sm8250 | qcom/qrb5165-rb5.dts). I can't boot past this point https://www.irccloud.com/pastebin/raw/Nv6ZwHmW.

Quoted 5 lines · Amit Pundir

Hi Saravana,

This patch broke v5.15-rc6 on RB5 (sm8250 | qcom/qrb5165-rb5.dts). I can't boot past this point https://www.irccloud.com/pastebin/raw/Nv6ZwHmW.

Amit top posting? How did that happen? :)

The fact you are seeing this issue is super strange though. The driver literally does nothing other than allowing some sync_state() callbacks to happen. I also grepped for the occurence of "simple-bus" in arch/arm64/boot/dts/qcom/ and the only instance for 8250 is for the soc node.

The only thing I can think of is that without my patch some sync_state() callbacks weren't getting called and maybe it was masking some other issue.

Can you try to boot with this log (see log patch below) and see if the device hangs right after a sync_state() callback? Also, looking at the different sync_state() implementations in upstream, I'm guessing one of the devices isn't voting for interconnect bandwidth when it should have.

Another thing you could do is boot without the simple-bus changes and then look for all instances of "state_synced" in /sys/devices and then see if any of them has the value "0" after boot up is complete.

(quoted diff) +2 -2

-Saravana
 
-- a/drivers/base/core.c
+++ b/drivers/base/core.c

@@ -1099,6 +1099,7 @@ static void device_links_flush_sync_list(struct
 list_head *list,
                if (dev != dont_lock_dev)
                        device_lock(dev);
 
+               dev_info(dev, "Calling sync_state()\n");
                if (dev->bus->sync_state)
                        dev->bus->sync_state(dev);

                else if (dev->driver && dev->driver->sync_state)

Quoted 26 lines · Saravana Kannan

Quoted 5 lines · Amit Pundir

Hi Saravana,

This patch broke v5.15-rc6 on RB5 (sm8250 | qcom/qrb5165-rb5.dts). I can't boot past this point https://www.irccloud.com/pastebin/raw/Nv6ZwHmW.

Amit top posting? How did that happen? :)

The fact you are seeing this issue is super strange though. The driver literally does nothing other than allowing some sync_state() callbacks to happen. I also grepped for the occurence of "simple-bus" in arch/arm64/boot/dts/qcom/ and the only instance for 8250 is for the soc node.

The only thing I can think of is that without my patch some sync_state() callbacks weren't getting called and maybe it was masking some other issue.

Can you try to boot with this log (see log patch below) and see if the device hangs right after a sync_state() callback? Also, looking at the different sync_state() implementations in upstream, I'm guessing one of the devices isn't voting for interconnect bandwidth when it should have.

Quoted 3 lines

Another thing you could do is boot without the simple-bus changes and then look for all instances of "state_synced" in /sys/devices and then see if any of them has the value "0" after boot up is complete.

Turned out RB5 is not even reaching up to device_links_flush_sync_list() and seem to be stuck somewhere in device_links_driver_bound(). So I added more print logs to narrow down to any specific lock state but those additional prints seem to have added enough delay to unblock that particular driver (Serial:
8250/16550 driver if I understood the logs correctly) and I eventually booted to UI.

Quoted 32 lines · Amit Pundir

Quoted 26 lines · Saravana Kannan

Quoted 5 lines · Amit Pundir

Hi Saravana,

This patch broke v5.15-rc6 on RB5 (sm8250 | qcom/qrb5165-rb5.dts). I can't boot past this point https://www.irccloud.com/pastebin/raw/Nv6ZwHmW.

Amit top posting? How did that happen? :)

The fact you are seeing this issue is super strange though. The driver literally does nothing other than allowing some sync_state() callbacks to happen. I also grepped for the occurence of "simple-bus" in arch/arm64/boot/dts/qcom/ and the only instance for 8250 is for the soc node.

The only thing I can think of is that without my patch some sync_state() callbacks weren't getting called and maybe it was masking some other issue.

Can you try to boot with this log (see log patch below) and see if the device hangs right after a sync_state() callback? Also, looking at the different sync_state() implementations in upstream, I'm guessing one of the devices isn't voting for interconnect bandwidth when it should have.

Quoted 3 lines

Another thing you could do is boot without the simple-bus changes and then look for all instances of "state_synced" in /sys/devices and then see if any of them has the value "0" after boot up is complete.

Quoted 7 lines

Turned out RB5 is not even reaching up to device_links_flush_sync_list() and seem to be stuck somewhere in device_links_driver_bound(). So I added more print logs to narrow down to any specific lock state but those additional prints seem to have added enough delay to unblock that particular driver (Serial:
8250/16550 driver if I understood the logs correctly) and I eventually booted to UI.

Ugh... I think I know what's going on. It popped into my head over the weekend.

Couple of ways to confirm my theory:
1. After it finishes booting in both cases, can you compare the output of the command below? I'm expecting to see a significant drop in the number of device links.
ls -l /sys/class/devlink | wc -l

2. Can you try out this terrible hack patch (not final fix, no code reviews please) on top of Tot to see if it fixes your issue without having to add hacky logs?

Thanks,
Saravana

drivers/bus/simple-pm-bus.c +4 -2

@@ -38,10 +38,12 @@ static int simple_pm_bus_probe(struct platform_device *pdev)
         * a device that has a more specific driver.
         */
        if (match && match->data) {
-               if (of_property_match_string(np, "compatible",
 match->compatible) == 0)
+               if (of_property_match_string(np, "compatible",
 match->compatible) == 0) {
+                       of_platform_populate(np, NULL, lookup, &pdev->dev);
                        return 0;
-               else
+               } else {
                        return -ENODEV;
+               }
        }

Ulf reported an issue[1] with fw_devlink. This series tries to fix that issue.

I replicated a similar set up on my end and I confirmed:
- A simple-bus only device is probed.
- Another device listing simple-bus as a 2nd compatible string isn't probed.

v1->v2:
- Switched to probing the simple-bus device instead of marking it as NEVER_PROBES.

v2->v3:
- Moved all the code into the simple-pm-bus driver
- Addressed Ulf's comment about the remove() code missing a check.

v3->v4:
- Added support for driver_override to accommodate the case where new transparent bus compatible strings are added to DT, but the kernel hasn't been updated yet.
- Added/updated comments to give more details.
- Adding Ulf's Tested-by that he have for v3 (v4 didn't change by much).

Thanks,
Saravana
[1] - https://lore.kernel.org/lkml/CAPDyKFo9Bxremkb1dDrr4OcXSpE0keVze94Cm=zrkOVxHHxBmQ@mail.gmail.com/

Cc Ulf Hansson <[email protected]>

Cc Geert Uytterhoeven <[email protected]>

Cc Rob Herring <[email protected]>

Saravana Kannan (2):
drivers: bus: simple-pm-bus: Add support for probing simple bus only
devices
drivers: bus: Delete CONFIG_SIMPLE_PM_BUS

 arch/arm/configs/multi_v7_defconfig |  1 -
 arch/arm/configs/oxnas_v6_defconfig |  1 -
 arch/arm/configs/shmobile_defconfig |  1 -
 arch/arm/mach-omap2/Kconfig         |  1 -
 arch/arm64/configs/defconfig        |  1 -
 drivers/bus/Kconfig                 | 12 ---------
 drivers/bus/Makefile                |  2 +-
 drivers/bus/simple-pm-bus.c         | 42 ++++++++++++++++++++++++++---
 drivers/soc/canaan/Kconfig          |  1 -
 9 files changed, 40 insertions(+), 22 deletions(-)

Ulf reported an issue[1] with fw_devlink. This series tries to fix that issue.

I replicated a similar set up on my end and I confirmed:
- A simple-bus only device is probed.
- Another device listing simple-bus as a 2nd compatible string isn't probed.

v1->v2:
- Switched to probing the simple-bus device instead of marking it as NEVER_PROBES.

v2->v3:
- Moved all the code into the simple-pm-bus driver
- Addressed Ulf's comment about the remove() code missing a check.

v3->v4:
- Added support for driver_override to accommodate the case where new transparent bus compatible strings are added to DT, but the kernel hasn't been updated yet.
- Added/updated comments to give more details.
- Adding Ulf's Tested-by that he have for v3 (v4 didn't change by much).