fw_devlink improvements

The OF_POPULATED flag was set to let fw_devlink know that the device tree node will not have a struct device created for it. This information is used by fw_devlink to avoid deferring the probe of consumers of this device tree node.

Let's use fwnode_dev_initialized() instead because it achieves the same effect without using OF specific flags. This allows more generic code to be written in driver core.

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

Reviewed-by Geert Uytterhoeven <[email protected]>

Registering an irqdomain sets the flag for the fwnode. But having the flag set when a device is added is interpreted by fw_devlink to mean the device has already been initialized and will never probe. This prevents fw_devlink from creating device links with the gpio_device as a supplier. So, clear the flag before adding the device.

Quoted 5 lines · Saravana Kannan

Registering an irqdomain sets the flag for the fwnode. But having the flag set when a device is added is interpreted by fw_devlink to mean the device has already been initialized and will never probe. This prevents fw_devlink from creating device links with the gpio_device as a supplier. So, clear the flag before adding the device.

drivers/gpio/gpiolib.c static int gpiochip_setup_dev(struct gpio_device *gdev) · line 586

Quoted 2 lines

drivers/gpio/gpiolib.c +2 -0

+	if (gdev->dev.fwnode && !gdev->dev.fwnode->dev)
+		fwnode_dev_initialized(gdev->dev.fwnode, false);

Please, do not dereference fwnode from struct device. We have dev_fwnode() for that.

The driver core now:
- Has the parent device of a supplier pick up the consumers if the supplier never has a device created for it.
- Ignores a supplier if the supplier has no parent device and will never be probed by a driver

And already prevents creating a device link with the consumer as a supplier of a parent.

So, we no longer need to find the "compatible" node of the supplier or do any other checks in of_link_to_phandle(). We simply need to make sure that the supplier is available in DT.

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

Hi Saravana,

Quoted 6 lines · Saravana Kannan

The driver core now:
- Has the parent device of a supplier pick up the consumers if the supplier never has a device created for it.
- Ignores a supplier if the supplier has no parent device and will never be probed by a driver

Quoted 8 lines

And already prevents creating a device link with the consumer as a supplier of a parent.

So, we no longer need to find the "compatible" node of the supplier or do any other checks in of_link_to_phandle(). We simply need to make sure that the supplier is available in DT.

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

Thanks for your patch!

This patch introduces a regression when dynamically loading DT overlays. Unfortunately this happens when using the out-of-tree OF configfs, which is not supported upstream. Still, there may be (obscure) in-tree users.

When loading a DT overlay[1] to enable an SPI controller, and instantiate a connected SPI EEPROM:

    $ overlay add 25lc040
    OF: overlay: WARNING: memory leak will occur if overlay removed,
property: /keys/status
    OF: overlay: WARNING: memory leak will occur if overlay removed,
property: /soc/spi@e6e90000/pinctrl-0
    OF: overlay: WARNING: memory leak will occur if overlay removed,
property: /soc/spi@e6e90000/pinctrl-names
    OF: overlay: WARNING: memory leak will occur if overlay removed,
property: /soc/spi@e6e90000/cs-gpios
    OF: overlay: WARNING: memory leak will occur if overlay removed,
property: /soc/spi@e6e90000/status
    OF: overlay: WARNING: memory leak will occur if overlay removed,
property: /__symbols__/msiof0_pins

The SPI controller and the SPI EEPROM are no longer instantiated.

    # cat /sys/kernel/debug/devices_deferred
    e6e90000.spi    platform: wait for supplier msiof0

Let's remove the overlay again:

    $ overlay rm 25lc040
    input: keys as /devices/platform/keys/input/input1

And retry:

    $ overlay add 25lc040
    OF: overlay: WARNING: memory leak will occur if overlay removed,
property: /keys/status
    OF: overlay: WARNING: memory leak will occur if overlay removed,
property: /soc/spi@e6e90000/pinctrl-0
    OF: overlay: WARNING: memory leak will occur if overlay removed,
property: /soc/spi@e6e90000/pinctrl-names
    OF: overlay: WARNING: memory leak will occur if overlay removed,
property: /soc/spi@e6e90000/cs-gpios
    OF: overlay: WARNING: memory leak will occur if overlay removed,
property: /soc/spi@e6e90000/status
    OF: overlay: WARNING: memory leak will occur if overlay removed,
property: /__symbols__/msiof0_pins
    spi_sh_msiof e6e90000.spi: DMA available
    spi_sh_msiof e6e90000.spi: registered master spi0
    spi spi0.0: setup mode 0, 8 bits/w, 100000 Hz max --> 0
    at25 spi0.0: 512 Byte at25 eeprom, pagesize 16
    spi_sh_msiof e6e90000.spi: registered child spi0.0

Now it succeeds, and the SPI EEPROM is available, and works.

Without this patch, or with this patch reverted after applying the full series:

    $ overlay add 25lc040
    OF: overlay: WARNING: memory leak will occur if overlay removed,
property: /keys/status
    OF: overlay: WARNING: memory leak will occur if overlay removed,
property: /soc/spi@e6e90000/pinctrl-0
    OF: overlay: WARNING: memory leak will occur if overlay removed,
property: /soc/spi@e6e90000/pinctrl-names
    OF: overlay: WARNING: memory leak will occur if overlay removed,
property: /soc/spi@e6e90000/cs-gpios
    OF: overlay: WARNING: memory leak will occur if overlay removed,
property: /soc/spi@e6e90000/status
    OF: overlay: WARNING: memory leak will occur if overlay removed,
property: /__symbols__/msiof0_pins
    OF: Not linking spi@e6e90000 to interrupt-controller@f1010000 - No
struct device
    spi_sh_msiof e6e90000.spi: DMA available
    spi_sh_msiof e6e90000.spi: registered master spi0
    spi spi0.0: setup mode 0, 8 bits/w, 100000 Hz max --> 0
    at25 spi0.0: 444 bps (2 bytes in 9 ticks)
    at25 spi0.0: 512 Byte at25 eeprom, pagesize 16
    spi_sh_msiof e6e90000.spi: registered child spi0.0

The SPI EEPROM is available on the first try after boot.

Quoted 98 lines · Geert Uytterhoeven

Quoted 6 lines · Saravana Kannan

The driver core now:
- Has the parent device of a supplier pick up the consumers if the supplier never has a device created for it.
- Ignores a supplier if the supplier has no parent device and will never be probed by a driver

Quoted 8 lines

And already prevents creating a device link with the consumer as a supplier of a parent.

So, we no longer need to find the "compatible" node of the supplier or do any other checks in of_link_to_phandle(). We simply need to make sure that the supplier is available in DT.

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

Thanks for your patch!

This patch introduces a regression when dynamically loading DT overlays. Unfortunately this happens when using the out-of-tree OF configfs, which is not supported upstream. Still, there may be (obscure) in-tree users.

When loading a DT overlay[1] to enable an SPI controller, and instantiate a connected SPI EEPROM:

    $ overlay add 25lc040
    OF: overlay: WARNING: memory leak will occur if overlay removed,
property: /keys/status
    OF: overlay: WARNING: memory leak will occur if overlay removed,
property: /soc/spi@e6e90000/pinctrl-0
    OF: overlay: WARNING: memory leak will occur if overlay removed,
property: /soc/spi@e6e90000/pinctrl-names
    OF: overlay: WARNING: memory leak will occur if overlay removed,
property: /soc/spi@e6e90000/cs-gpios
    OF: overlay: WARNING: memory leak will occur if overlay removed,
property: /soc/spi@e6e90000/status
    OF: overlay: WARNING: memory leak will occur if overlay removed,
property: /__symbols__/msiof0_pins

The SPI controller and the SPI EEPROM are no longer instantiated.

    # cat /sys/kernel/debug/devices_deferred
    e6e90000.spi    platform: wait for supplier msiof0

Let's remove the overlay again:

    $ overlay rm 25lc040
    input: keys as /devices/platform/keys/input/input1

And retry:

    $ overlay add 25lc040
    OF: overlay: WARNING: memory leak will occur if overlay removed,
property: /keys/status
    OF: overlay: WARNING: memory leak will occur if overlay removed,
property: /soc/spi@e6e90000/pinctrl-0
    OF: overlay: WARNING: memory leak will occur if overlay removed,
property: /soc/spi@e6e90000/pinctrl-names
    OF: overlay: WARNING: memory leak will occur if overlay removed,
property: /soc/spi@e6e90000/cs-gpios
    OF: overlay: WARNING: memory leak will occur if overlay removed,
property: /soc/spi@e6e90000/status
    OF: overlay: WARNING: memory leak will occur if overlay removed,
property: /__symbols__/msiof0_pins
    spi_sh_msiof e6e90000.spi: DMA available
    spi_sh_msiof e6e90000.spi: registered master spi0
    spi spi0.0: setup mode 0, 8 bits/w, 100000 Hz max --> 0
    at25 spi0.0: 512 Byte at25 eeprom, pagesize 16
    spi_sh_msiof e6e90000.spi: registered child spi0.0

Now it succeeds, and the SPI EEPROM is available, and works.

Without this patch, or with this patch reverted after applying the full series:

    $ overlay add 25lc040
    OF: overlay: WARNING: memory leak will occur if overlay removed,
property: /keys/status
    OF: overlay: WARNING: memory leak will occur if overlay removed,
property: /soc/spi@e6e90000/pinctrl-0
    OF: overlay: WARNING: memory leak will occur if overlay removed,
property: /soc/spi@e6e90000/pinctrl-names
    OF: overlay: WARNING: memory leak will occur if overlay removed,
property: /soc/spi@e6e90000/cs-gpios
    OF: overlay: WARNING: memory leak will occur if overlay removed,
property: /soc/spi@e6e90000/status
    OF: overlay: WARNING: memory leak will occur if overlay removed,
property: /__symbols__/msiof0_pins
    OF: Not linking spi@e6e90000 to interrupt-controller@f1010000 - No
struct device
    spi_sh_msiof e6e90000.spi: DMA available
    spi_sh_msiof e6e90000.spi: registered master spi0
    spi spi0.0: setup mode 0, 8 bits/w, 100000 Hz max --> 0
    at25 spi0.0: 444 bps (2 bytes in 9 ticks)
    at25 spi0.0: 512 Byte at25 eeprom, pagesize 16
    spi_sh_msiof e6e90000.spi: registered child spi0.0

Quoted 1 line

The SPI EEPROM is available on the first try after boot.

Sigh... I spent way too long trying to figure out if I caused a memory leak. I should have scrolled down further! Doesn't look like that part is related to anything I did.

There are some flags set to avoid re-parsing fwnodes multiple times. My guess is that the issue you are seeing has to do with how many of the in memory structs are reused vs not when an overlay is applied/removed and some of these flags might not be getting cleared and this is having a bigger impact with this patch (because the fwnode links are no longer anchored on "compatible" nodes).

With/without this patch (let's keep the series) can you look at how the following things change between each step you do above (add, remove, retry):
1) List of directories under /sys/class/devlink
2) Enable the debug logs inside __fwnode_link_add(), __fwnode_link_del(), device_link_add()

Quoted 101 lines · Geert Uytterhoeven

Hi Saravana,

Quoted 6 lines · Saravana Kannan

The driver core now:
- Has the parent device of a supplier pick up the consumers if the supplier never has a device created for it.
- Ignores a supplier if the supplier has no parent device and will never be probed by a driver

Quoted 8 lines

And already prevents creating a device link with the consumer as a supplier of a parent.

So, we no longer need to find the "compatible" node of the supplier or do any other checks in of_link_to_phandle(). We simply need to make sure that the supplier is available in DT.

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

Thanks for your patch!

This patch introduces a regression when dynamically loading DT overlays. Unfortunately this happens when using the out-of-tree OF configfs, which is not supported upstream. Still, there may be (obscure) in-tree users.

When loading a DT overlay[1] to enable an SPI controller, and instantiate a connected SPI EEPROM:

    $ overlay add 25lc040
    OF: overlay: WARNING: memory leak will occur if overlay removed,
property: /keys/status
    OF: overlay: WARNING: memory leak will occur if overlay removed,
property: /soc/spi@e6e90000/pinctrl-0
    OF: overlay: WARNING: memory leak will occur if overlay removed,
property: /soc/spi@e6e90000/pinctrl-names
    OF: overlay: WARNING: memory leak will occur if overlay removed,
property: /soc/spi@e6e90000/cs-gpios
    OF: overlay: WARNING: memory leak will occur if overlay removed,
property: /soc/spi@e6e90000/status
    OF: overlay: WARNING: memory leak will occur if overlay removed,
property: /__symbols__/msiof0_pins

The SPI controller and the SPI EEPROM are no longer instantiated.

    # cat /sys/kernel/debug/devices_deferred
    e6e90000.spi    platform: wait for supplier msiof0

Let's remove the overlay again:

    $ overlay rm 25lc040
    input: keys as /devices/platform/keys/input/input1

And retry:

    $ overlay add 25lc040
    OF: overlay: WARNING: memory leak will occur if overlay removed,
property: /keys/status
    OF: overlay: WARNING: memory leak will occur if overlay removed,
property: /soc/spi@e6e90000/pinctrl-0
    OF: overlay: WARNING: memory leak will occur if overlay removed,
property: /soc/spi@e6e90000/pinctrl-names
    OF: overlay: WARNING: memory leak will occur if overlay removed,
property: /soc/spi@e6e90000/cs-gpios
    OF: overlay: WARNING: memory leak will occur if overlay removed,
property: /soc/spi@e6e90000/status
    OF: overlay: WARNING: memory leak will occur if overlay removed,
property: /__symbols__/msiof0_pins
    spi_sh_msiof e6e90000.spi: DMA available
    spi_sh_msiof e6e90000.spi: registered master spi0
    spi spi0.0: setup mode 0, 8 bits/w, 100000 Hz max --> 0
    at25 spi0.0: 512 Byte at25 eeprom, pagesize 16
    spi_sh_msiof e6e90000.spi: registered child spi0.0

Now it succeeds, and the SPI EEPROM is available, and works.

Without this patch, or with this patch reverted after applying the full series:

    $ overlay add 25lc040
    OF: overlay: WARNING: memory leak will occur if overlay removed,
property: /keys/status
    OF: overlay: WARNING: memory leak will occur if overlay removed,
property: /soc/spi@e6e90000/pinctrl-0
    OF: overlay: WARNING: memory leak will occur if overlay removed,
property: /soc/spi@e6e90000/pinctrl-names
    OF: overlay: WARNING: memory leak will occur if overlay removed,
property: /soc/spi@e6e90000/cs-gpios
    OF: overlay: WARNING: memory leak will occur if overlay removed,
property: /soc/spi@e6e90000/status
    OF: overlay: WARNING: memory leak will occur if overlay removed,
property: /__symbols__/msiof0_pins
    OF: Not linking spi@e6e90000 to interrupt-controller@f1010000 - No
struct device
    spi_sh_msiof e6e90000.spi: DMA available
    spi_sh_msiof e6e90000.spi: registered master spi0
    spi spi0.0: setup mode 0, 8 bits/w, 100000 Hz max --> 0
    at25 spi0.0: 444 bps (2 bytes in 9 ticks)
    at25 spi0.0: 512 Byte at25 eeprom, pagesize 16
    spi_sh_msiof e6e90000.spi: registered child spi0.0

Quoted 1 line

The SPI EEPROM is available on the first try after boot.

Sigh... I spent way too long trying to figure out if I caused a memory leak. I should have scrolled down further! Doesn't look like that part is related to anything I did.

There are some flags set to avoid re-parsing fwnodes multiple times. My guess is that the issue you are seeing has to do with how many of the in memory structs are reused vs not when an overlay is applied/removed and some of these flags might not be getting cleared and this is having a bigger impact with this patch (because the fwnode links are no longer anchored on "compatible" nodes).

With/without this patch (let's keep the series) can you look at how the following things change between each step you do above (add, remove, retry):
1) List of directories under /sys/class/devlink
2) Enable the debug logs inside __fwnode_link_add(), __fwnode_link_del(), device_link_add()

My guess is that the final solution would entail clearing FWNODE_FLAG_LINKS_ADDED for some fwnodes.

Quoted 26 lines · Saravana Kannan

Quoted 25 lines · Geert Uytterhoeven

Quoted 6 lines · Saravana Kannan

The driver core now:
- Has the parent device of a supplier pick up the consumers if the supplier never has a device created for it.
- Ignores a supplier if the supplier has no parent device and will never be probed by a driver

Quoted 8 lines

And already prevents creating a device link with the consumer as a supplier of a parent.

So, we no longer need to find the "compatible" node of the supplier or do any other checks in of_link_to_phandle(). We simply need to make sure that the supplier is available in DT.

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

Thanks for your patch!

This patch introduces a regression when dynamically loading DT overlays. Unfortunately this happens when using the out-of-tree OF configfs, which is not supported upstream. Still, there may be (obscure) in-tree users.

When loading a DT overlay[1] to enable an SPI controller, and instantiate a connected SPI EEPROM:

Quoted 20 lines

Quoted 1 line

The SPI controller and the SPI EEPROM are no longer instantiated.

Sigh... I spent way too long trying to figure out if I caused a memory leak. I should have scrolled down further! Doesn't look like that part is related to anything I did.

There are some flags set to avoid re-parsing fwnodes multiple times. My guess is that the issue you are seeing has to do with how many of the in memory structs are reused vs not when an overlay is applied/removed and some of these flags might not be getting cleared and this is having a bigger impact with this patch (because the fwnode links are no longer anchored on "compatible" nodes).

With/without this patch (let's keep the series) can you look at how the following things change between each step you do above (add, remove, retry):
1) List of directories under /sys/class/devlink
2) Enable the debug logs inside __fwnode_link_add(), __fwnode_link_del(), device_link_add()

Quoted 2 lines

My guess is that the final solution would entail clearing FWNODE_FLAG_LINKS_ADDED for some fwnodes.

You replied just as I was about to hit send. So sending this anyway...

Ok, I took a closer look and I think it's a bit of a mess. The fact that it even worked for you without this patch is a bit of a coincidence.

Let's just take platform devices that are created by driver/of/platform.c as an example.

The main problem is that when you add/remove properties to a DT node of an existing platform device, nothing is really done about it at the device level. We don't even unbind and rebind the driver so the driver could make use of the new properties. We don't remove and add back the device so whoever might use the new property will use it. And if you are adding a new node, it'll only trigger any platform device level impact if it's a new node of a "simple-bus" (or similar bus) device.

Problem 1:
So if you add a new child node to an existing probed device that adds its children explicitly (as in, the parent is not a "simple-bus" like device), nothing will happen. The newly added child device node will get converted into a platform device, not will the parent device notice it. So in your case of adding msiof0_pins, it's just that when the consumer gets the pins, the driver doesn't get involved much and it's the pinctrl framework that reads the DT and figures it out.

With this patch, the fwnode links point to the actual resource and the actual parent device inherits them if they don't get converted to a struct device. But since we are adding this msiof0_pins after the parent device has probed, the fwnode link isn't inherited by the parent pinctrl device.

Problem 2:
So if you add a property to an already bound device, nothing is done by the driver. In your overlay example, if you move the status="okay" line to be the first property you change in the msiof0 spi device, you'll probably see that fw_devlink is no longer the one blocking the probe. This is because the platform device will get added as soon as the status flips from disabled to enabled and at that point fw_devlink will think it has no suppliers and won't do any probe deferring. And then as the new properties get added nothing will happen at the device or fw_devlink level. If the msiof0's spi driver fails immediately with NOT -EPROBE_DEFER when platform device is added because it couldn't find any pinctrl property, then msiof0 will never probe (unless you remove and add the driver). If it had failed with -EPROBE_DEFER, then it might probe again if something else triggers a deferred probe attempt. Clearly, things working/not working based on the order of properties in DT is not a good implementation.

Problem 3:
What if you enable a previously disabled supplier. There's no way to handle that from a fw_devlink level without re-parsing the entire device tree because existing devices might be consumers now.

Anyway, long story short, it's sorta worked due to coincidence and it's quite messy to get it to work correctly.

Another way to get this to work is to:
1) unload pinctrl driver, unload spi driver.
2) apply overlay
3) reload pinctrl driver, reload spi driver.

This is assuming unloading those 2 drivers doesn't crash your system.

Hi Saravana,

Quoted 27 lines · Saravana Kannan

Quoted 26 lines · Saravana Kannan

Quoted 25 lines · Geert Uytterhoeven

Quoted 6 lines · Saravana Kannan

The driver core now:
- Has the parent device of a supplier pick up the consumers if the supplier never has a device created for it.
- Ignores a supplier if the supplier has no parent device and will never be probed by a driver

Quoted 8 lines

And already prevents creating a device link with the consumer as a supplier of a parent.

So, we no longer need to find the "compatible" node of the supplier or do any other checks in of_link_to_phandle(). We simply need to make sure that the supplier is available in DT.

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

Thanks for your patch!

This patch introduces a regression when dynamically loading DT overlays. Unfortunately this happens when using the out-of-tree OF configfs, which is not supported upstream. Still, there may be (obscure) in-tree users.

When loading a DT overlay[1] to enable an SPI controller, and instantiate a connected SPI EEPROM:

[...]

Quoted 72 lines

Quoted 20 lines

Quoted 1 line

The SPI controller and the SPI EEPROM are no longer instantiated.

Sigh... I spent way too long trying to figure out if I caused a memory leak. I should have scrolled down further! Doesn't look like that part is related to anything I did.

There are some flags set to avoid re-parsing fwnodes multiple times. My guess is that the issue you are seeing has to do with how many of the in memory structs are reused vs not when an overlay is applied/removed and some of these flags might not be getting cleared and this is having a bigger impact with this patch (because the fwnode links are no longer anchored on "compatible" nodes).

With/without this patch (let's keep the series) can you look at how the following things change between each step you do above (add, remove, retry):
1) List of directories under /sys/class/devlink
2) Enable the debug logs inside __fwnode_link_add(), __fwnode_link_del(), device_link_add()

Quoted 2 lines

My guess is that the final solution would entail clearing FWNODE_FLAG_LINKS_ADDED for some fwnodes.

You replied just as I was about to hit send. So sending this anyway...

Ok, I took a closer look and I think it's a bit of a mess. The fact that it even worked for you without this patch is a bit of a coincidence.

Let's just take platform devices that are created by driver/of/platform.c as an example.

The main problem is that when you add/remove properties to a DT node of an existing platform device, nothing is really done about it at the device level. We don't even unbind and rebind the driver so the driver could make use of the new properties. We don't remove and add back the device so whoever might use the new property will use it. And if you are adding a new node, it'll only trigger any platform device level impact if it's a new node of a "simple-bus" (or similar bus) device.

Problem 1:
So if you add a new child node to an existing probed device that adds its children explicitly (as in, the parent is not a "simple-bus" like device), nothing will happen. The newly added child device node will get converted into a platform device, not will the parent device notice it. So in your case of adding msiof0_pins, it's just that when the consumer gets the pins, the driver doesn't get involved much and it's the pinctrl framework that reads the DT and figures it out.

With this patch, the fwnode links point to the actual resource and the actual parent device inherits them if they don't get converted to a struct device. But since we are adding this msiof0_pins after the parent device has probed, the fwnode link isn't inherited by the parent pinctrl device.

Problem 2:
So if you add a property to an already bound device, nothing is done by the driver. In your overlay example, if you move the status="okay" line to be the first property you change in the msiof0 spi device, you'll probably see that fw_devlink is no longer the one blocking the probe. This is because the platform device will get added as soon as the status flips from disabled to enabled and at that point fw_devlink will think it has no suppliers and won't do any probe deferring. And then as the new properties get added nothing will happen at the device or fw_devlink level. If the msiof0's spi driver fails immediately with NOT -EPROBE_DEFER when platform device is added because it couldn't find any pinctrl property, then msiof0 will never probe (unless you remove and add the driver). If it had failed with -EPROBE_DEFER, then it might probe again if something else triggers a deferred probe attempt. Clearly, things working/not working based on the order of properties in DT is not a good implementation.

Quoted 7 lines

Problem 3:
What if you enable a previously disabled supplier. There's no way to handle that from a fw_devlink level without re-parsing the entire device tree because existing devices might be consumers now.

Anyway, long story short, it's sorta worked due to coincidence and it's quite messy to get it to work correctly.

Several subsystems register notifiers to be informed of the events above. E.g. drivers/spi/spi.c:

        if (IS_ENABLED(CONFIG_OF_DYNAMIC))
                WARN_ON(of_reconfig_notifier_register(&spi_of_notifier));
        if (IS_ENABLED(CONFIG_ACPI))
                WARN_ON(acpi_reconfig_notifier_register(&spi_acpi_notifier));

So my issue might be triggered using ACPI, too.

Quoted 114 lines · Geert Uytterhoeven

Quoted 27 lines · Saravana Kannan

Quoted 26 lines · Saravana Kannan

Quoted 25 lines · Geert Uytterhoeven

Quoted 6 lines · Saravana Kannan

The driver core now:
- Has the parent device of a supplier pick up the consumers if the supplier never has a device created for it.
- Ignores a supplier if the supplier has no parent device and will never be probed by a driver

Quoted 8 lines

And already prevents creating a device link with the consumer as a supplier of a parent.

So, we no longer need to find the "compatible" node of the supplier or do any other checks in of_link_to_phandle(). We simply need to make sure that the supplier is available in DT.

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

Thanks for your patch!

This patch introduces a regression when dynamically loading DT overlays. Unfortunately this happens when using the out-of-tree OF configfs, which is not supported upstream. Still, there may be (obscure) in-tree users.

When loading a DT overlay[1] to enable an SPI controller, and instantiate a connected SPI EEPROM:

[...]

Quoted 72 lines

Quoted 20 lines

Quoted 1 line

The SPI controller and the SPI EEPROM are no longer instantiated.

Sigh... I spent way too long trying to figure out if I caused a memory leak. I should have scrolled down further! Doesn't look like that part is related to anything I did.

There are some flags set to avoid re-parsing fwnodes multiple times. My guess is that the issue you are seeing has to do with how many of the in memory structs are reused vs not when an overlay is applied/removed and some of these flags might not be getting cleared and this is having a bigger impact with this patch (because the fwnode links are no longer anchored on "compatible" nodes).

With/without this patch (let's keep the series) can you look at how the following things change between each step you do above (add, remove, retry):
1) List of directories under /sys/class/devlink
2) Enable the debug logs inside __fwnode_link_add(), __fwnode_link_del(), device_link_add()

Quoted 2 lines

My guess is that the final solution would entail clearing FWNODE_FLAG_LINKS_ADDED for some fwnodes.

You replied just as I was about to hit send. So sending this anyway...

Ok, I took a closer look and I think it's a bit of a mess. The fact that it even worked for you without this patch is a bit of a coincidence.

Let's just take platform devices that are created by driver/of/platform.c as an example.

The main problem is that when you add/remove properties to a DT node of an existing platform device, nothing is really done about it at the device level. We don't even unbind and rebind the driver so the driver could make use of the new properties. We don't remove and add back the device so whoever might use the new property will use it. And if you are adding a new node, it'll only trigger any platform device level impact if it's a new node of a "simple-bus" (or similar bus) device.

Problem 1:
So if you add a new child node to an existing probed device that adds its children explicitly (as in, the parent is not a "simple-bus" like device), nothing will happen. The newly added child device node will get converted into a platform device, not will the parent device notice it. So in your case of adding msiof0_pins, it's just that when the consumer gets the pins, the driver doesn't get involved much and it's the pinctrl framework that reads the DT and figures it out.

With this patch, the fwnode links point to the actual resource and the actual parent device inherits them if they don't get converted to a struct device. But since we are adding this msiof0_pins after the parent device has probed, the fwnode link isn't inherited by the parent pinctrl device.

Problem 2:
So if you add a property to an already bound device, nothing is done by the driver. In your overlay example, if you move the status="okay" line to be the first property you change in the msiof0 spi device, you'll probably see that fw_devlink is no longer the one blocking the probe. This is because the platform device will get added as soon as the status flips from disabled to enabled and at that point fw_devlink will think it has no suppliers and won't do any probe deferring. And then as the new properties get added nothing will happen at the device or fw_devlink level. If the msiof0's spi driver fails immediately with NOT -EPROBE_DEFER when platform device is added because it couldn't find any pinctrl property, then msiof0 will never probe (unless you remove and add the driver). If it had failed with -EPROBE_DEFER, then it might probe again if something else triggers a deferred probe attempt. Clearly, things working/not working based on the order of properties in DT is not a good implementation.

Quoted 7 lines

Problem 3:
What if you enable a previously disabled supplier. There's no way to handle that from a fw_devlink level without re-parsing the entire device tree because existing devices might be consumers now.

Anyway, long story short, it's sorta worked due to coincidence and it's quite messy to get it to work correctly.

Several subsystems register notifiers to be informed of the events above. E.g. drivers/spi/spi.c:

Quoted 6 lines

        if (IS_ENABLED(CONFIG_OF_DYNAMIC))
                WARN_ON(of_reconfig_notifier_register(&spi_of_notifier));
        if (IS_ENABLED(CONFIG_ACPI))
                WARN_ON(acpi_reconfig_notifier_register(&spi_acpi_notifier));

So my issue might be triggered using ACPI, too.

Yeah, I did notice this before my email. Here's an ugly hack (at end of email) to test my theory about Problem 1. I didn't compile test it (because I should go to bed now), but you get the idea. Can you give this a shot? It should fix your specific case. Basically for all overlays (I hope the function is only used for overlays) we assume all nodes are NOT devices until they actually get added as a device. Don't review the code, it's not meant to be :)

(quoted diff) +0 -1

-Saravana

drivers/of/dynamic.c +1 -0

@@ -226,6 +226,7 @@ static void __of_attach_node(struct device_node *np)
        np->sibling = np->parent->child;
        np->parent->child = np;
        of_node_clear_flag(np, OF_DETACHED);
+       np->fwnode.flags |= FWNODE_FLAG_NOT_DEVICE;
 }
 
 /**

drivers/of/platform.c +1 -0

index 81c8c227ab6b..7299cd668e51 100644
@@ -732,6 +732,7 @@ static int of_platform_notify(struct notifier_block *nb,
                if (of_node_check_flag(rd->dn, OF_POPULATED))
                        return NOTIFY_OK;
 
+               rd->dn->fwnode.flags &= ~FWNODE_FLAG_NOT_DEVICE;
                /* pdev_parent may be NULL when no bus platform device */
                pdev_parent = of_find_device_by_node(rd->dn->parent);
                pdev = of_platform_device_create(rd->dn, NULL,

drivers/spi/spi.c +0 -0

index 15f174f4e056..1de55561b25d 100644

On Tue, Feb 7, 2023 at 6:08 PM Saravana Kannan <[email protected]> wrote:

Another way to get this to work is to:
1) unload pinctrl driver, unload spi driver.
2) apply overlay
3) reload pinctrl driver, reload spi driver.

This is assuming unloading those 2 drivers doesn't crash your system.

Quoted 98 lines · Geert Uytterhoeven

Quoted 6 lines · Saravana Kannan

The driver core now:
- Has the parent device of a supplier pick up the consumers if the supplier never has a device created for it.
- Ignores a supplier if the supplier has no parent device and will never be probed by a driver

Quoted 8 lines

And already prevents creating a device link with the consumer as a supplier of a parent.

So, we no longer need to find the "compatible" node of the supplier or do any other checks in of_link_to_phandle(). We simply need to make sure that the supplier is available in DT.

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

Thanks for your patch!

This patch introduces a regression when dynamically loading DT overlays. Unfortunately this happens when using the out-of-tree OF configfs, which is not supported upstream. Still, there may be (obscure) in-tree users.

When loading a DT overlay[1] to enable an SPI controller, and instantiate a connected SPI EEPROM:

    $ overlay add 25lc040
    OF: overlay: WARNING: memory leak will occur if overlay removed,
property: /keys/status
    OF: overlay: WARNING: memory leak will occur if overlay removed,
property: /soc/spi@e6e90000/pinctrl-0
    OF: overlay: WARNING: memory leak will occur if overlay removed,
property: /soc/spi@e6e90000/pinctrl-names
    OF: overlay: WARNING: memory leak will occur if overlay removed,
property: /soc/spi@e6e90000/cs-gpios
    OF: overlay: WARNING: memory leak will occur if overlay removed,
property: /soc/spi@e6e90000/status
    OF: overlay: WARNING: memory leak will occur if overlay removed,
property: /__symbols__/msiof0_pins

The SPI controller and the SPI EEPROM are no longer instantiated.

    # cat /sys/kernel/debug/devices_deferred
    e6e90000.spi    platform: wait for supplier msiof0

Let's remove the overlay again:

    $ overlay rm 25lc040
    input: keys as /devices/platform/keys/input/input1

And retry:

    $ overlay add 25lc040
    OF: overlay: WARNING: memory leak will occur if overlay removed,
property: /keys/status
    OF: overlay: WARNING: memory leak will occur if overlay removed,
property: /soc/spi@e6e90000/pinctrl-0
    OF: overlay: WARNING: memory leak will occur if overlay removed,
property: /soc/spi@e6e90000/pinctrl-names
    OF: overlay: WARNING: memory leak will occur if overlay removed,
property: /soc/spi@e6e90000/cs-gpios
    OF: overlay: WARNING: memory leak will occur if overlay removed,
property: /soc/spi@e6e90000/status
    OF: overlay: WARNING: memory leak will occur if overlay removed,
property: /__symbols__/msiof0_pins
    spi_sh_msiof e6e90000.spi: DMA available
    spi_sh_msiof e6e90000.spi: registered master spi0
    spi spi0.0: setup mode 0, 8 bits/w, 100000 Hz max --> 0
    at25 spi0.0: 512 Byte at25 eeprom, pagesize 16
    spi_sh_msiof e6e90000.spi: registered child spi0.0

Now it succeeds, and the SPI EEPROM is available, and works.

Without this patch, or with this patch reverted after applying the full series:

    $ overlay add 25lc040
    OF: overlay: WARNING: memory leak will occur if overlay removed,
property: /keys/status
    OF: overlay: WARNING: memory leak will occur if overlay removed,
property: /soc/spi@e6e90000/pinctrl-0
    OF: overlay: WARNING: memory leak will occur if overlay removed,
property: /soc/spi@e6e90000/pinctrl-names
    OF: overlay: WARNING: memory leak will occur if overlay removed,
property: /soc/spi@e6e90000/cs-gpios
    OF: overlay: WARNING: memory leak will occur if overlay removed,
property: /soc/spi@e6e90000/status
    OF: overlay: WARNING: memory leak will occur if overlay removed,
property: /__symbols__/msiof0_pins
    OF: Not linking spi@e6e90000 to interrupt-controller@f1010000 - No
struct device
    spi_sh_msiof e6e90000.spi: DMA available
    spi_sh_msiof e6e90000.spi: registered master spi0
    spi spi0.0: setup mode 0, 8 bits/w, 100000 Hz max --> 0
    at25 spi0.0: 444 bps (2 bytes in 9 ticks)
    at25 spi0.0: 512 Byte at25 eeprom, pagesize 16
    spi_sh_msiof e6e90000.spi: registered child spi0.0

Quoted 1 line

The SPI EEPROM is available on the first try after boot.

Sigh... I spent way too long trying to figure out if I caused a memory leak. I should have scrolled down further! Doesn't look like that part is related to anything I did.

There are some flags set to avoid re-parsing fwnodes multiple times. My guess is that the issue you are seeing has to do with how many of the in memory structs are reused vs not when an overlay is applied/removed and some of these flags might not be getting cleared and this is having a bigger impact with this patch (because the fwnode links are no longer anchored on "compatible" nodes).

With/without this patch (let's keep the series) can you look at how the following things change between each step you do above (add, remove, retry):
1) List of directories under /sys/class/devlink
2) Enable the debug logs inside __fwnode_link_add(), __fwnode_link_del(), device_link_add()

Hi Saravana,

Quoted 13 lines · Saravana Kannan

The driver core now:
- Has the parent device of a supplier pick up the consumers if the supplier never has a device created for it.
- Ignores a supplier if the supplier has no parent device and will never be probed by a driver

And already prevents creating a device link with the consumer as a supplier of a parent.

So, we no longer need to find the "compatible" node of the supplier or do any other checks in of_link_to_phandle(). We simply need to make sure that the supplier is available in DT.

Quoted 1 line

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

Thanks for your patch!

This patch introduces a regression when dynamically loading DT overlays. Unfortunately this happens when using the out-of-tree OF configfs, which is not supported upstream. Still, there may be (obscure) in-tree users.

Quoted 16 lines · Geert Uytterhoeven

Quoted 14 lines · Saravana Kannan

The driver core now:
- Has the parent device of a supplier pick up the consumers if the supplier never has a device created for it.
- Ignores a supplier if the supplier has no parent device and will never be probed by a driver

And already prevents creating a device link with the consumer as a supplier of a parent.

So, we no longer need to find the "compatible" node of the supplier or do any other checks in of_link_to_phandle(). We simply need to make sure that the supplier is available in DT.

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

Quoted 6 lines

Thanks for your patch!

This patch introduces a regression when dynamically loading DT overlays. Unfortunately this happens when using the out-of-tree OF configfs, which is not supported upstream. Still, there may be (obscure) in-tree users.

As we can't do anything about out-of-tree code, why does this matter?

These "nvmem-cells" platform devices never get probed because there's no platform driver for it and it's never used anywhere else. So it's a waste of memory. These devices also cause fw_devlink to block nvmem consumers of "nvmem-cells" partition from probing because the supplier device never probes.

So stop creating platform devices for nvmem-cells partitions to avoid wasting memory and to avoid blocking probing of consumers.

Reported-by Maxim Kiselev <[email protected]>

Fixes bcdf0315a61a ("mtd: call of_platform_populate() for MTD partitions")

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

---
 drivers/mtd/mtdpart.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

drivers/mtd/mtdpart.c +10 -1

index d442fa94c872..85f5ee6f06fc 100644
@@ -577,6 +577,7 @@ static int mtd_part_of_parse(struct mtd_info *master,
 {
        struct mtd_part_parser *parser;
        struct device_node *np;
+       struct device_node *child;
        struct property *prop;
        struct device *dev;
        const char *compat;

@@ -594,6 +595,15 @@ static int mtd_part_of_parse(struct mtd_info *master,
        else
                np = of_get_child_by_name(np, "partitions");
 
+       /*
+        * Don't create devices that are added to a bus but will never get
+        * probed. That'll cause fw_devlink to block probing of consumers of
+        * this partition until the partition device is probed.
+        */
+       for_each_child_of_node(np, child)
+               if (of_device_is_compatible(child, "nvmem-cells"))
+                       of_node_set_flag(child, OF_POPULATED);
+
        of_property_for_each_string(np, "compatible", prop, compat) {
                parser = mtd_part_get_compatible_parser(compat);
                if (!parser)
--

2.39.1.519.gcb327c4b5f-goog

Reported-by Maxim Kiselev <[email protected]>

Fixes bcdf0315a61a ("mtd: call of_platform_populate() for MTD partitions")

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

Naresh, Tony, Abel, Geert, Dmitry, Maxim(s), Miquel, Luca, Doug, Martin, Jean-Philippe,

Can I get your Tested-by's for this v3 series please?

Vladimir,

Ccing you because DSA's and fw_devlink have known/existing problems (still in my TODOs to fix). But I want to make sure this series doesn't cause additional problems for DSA.

All,

This patch series improves fw_devlink in the following ways:

1. It no longer cares about a fwnode having a "compatible" property. It figures this out more dynamically. The only expectation is that fwnodes that are converted to devices actually get probed by a driver for the dependencies to be enforced correctly.

2. Finer grained dependency tracking. fw_devlink will now create device links from the consumer to the actual resource's device (if it has one,
Eg: gpio_device) instead of the parent supplier device. This improves things like async suspend/resume ordering, potentially remove the need for frameworks to create device links, more parallelized async probing, and better sync_state() tracking.

3. Handle hardware/software quirks where a child firmware node gets populated as a device before its parent firmware node AND actually supplies a non-optional resource to the parent firmware node's device.

4. Way more robust at cycle handling (see patch for the insane cases).

5. Stops depending on OF_POPULATED to figure out some corner cases.

6. Simplifies the work that needs to be done by the firmware specific code.

The v3 series has gone through my usual testing on my end and looks good to me.

Thanks,
Saravana

[1] - https://lore.kernel.org/lkml/[email protected]/
[2] - https://lore.kernel.org/lkml/CAGETcx-JUV1nj8wBJrTPfyvM7=Mre5j_vkVmZojeiumUGG6QZQ@mail.gmail.com/

v1 -> v2:
- Fixed Patch 1 to handle a corner case discussed in [2].
- New patch 10 to handle "fsl,imx8mq-gpc" being initialized by 2 drivers.
- New patch 11 to add fw_devlink support for SCMI devices.

v2 -> v3:
- Addressed most of Andy's comments in v2
- Added Colin and Sudeep's Tested-by for the series (except the imx and renesas patches)
- Added Sudeep's Acked-by for the scmi patch.
- Added Geert's Reviewed-by for the renesas patch.
- Fixed gpiolib crash reported by Naresh.
- Patch 6: Fix __fwnode_links_move_consumers() to preserve fwnode link flags.
- New Patch 12 to fix nvmem-cells issue reported by Maxim(s)/Miquel.
- Deleted some stale function doc in Patch 8

Cc Abel Vesa <[email protected]>

Cc Alexander Stein <[email protected]>

Cc Tony Lindgren <[email protected]>

Cc Sudeep Holla <[email protected]>

Cc Geert Uytterhoeven <[email protected]>

Cc John Stultz <[email protected]>

Cc Doug Anderson <[email protected]>

Cc Guenter Roeck <[email protected]>

Cc Dmitry Baryshkov <[email protected]>

Cc Maxim Kiselev <[email protected]>

Cc Maxim Kochetkov <[email protected]>

Cc Miquel Raynal <[email protected]>

Cc Luca Weiss <[email protected]>

Cc Colin Foster <[email protected]>

Cc Martin Kepplinger <[email protected]>

Cc Jean-Philippe Brucker <[email protected]>

Cc Vladimir Oltean <[email protected]>

Saravana Kannan (12):
driver core: fw_devlink: Don't purge child fwnode's consumer links driver core: fw_devlink: Improve check for fwnode with no
device/driver
soc: renesas: Move away from using OF_POPULATED for fw_devlink
gpiolib: Clear the gpio_device's fwnode initialized flag before adding driver core: fw_devlink: Add DL_FLAG_CYCLE support to device links driver core: fw_devlink: Allow marking a fwnode link as being part of
a cycle
driver core: fw_devlink: Consolidate device link flag computation driver core: fw_devlink: Make cycle detection more robust
of: property: Simplify of_link_to_phandle() irqchip/irq-imx-gpcv2: Mark fwnode device as not initialized
firmware: arm_scmi: Set fwnode for the scmi_device
mtd: mtdpart: Don't create platform device that'll never probe

 drivers/base/core.c             | 449 +++++++++++++++++++++-----------
 drivers/firmware/arm_scmi/bus.c |   3 +-
 drivers/gpio/gpiolib.c          |   7 +
 drivers/irqchip/irq-imx-gpcv2.c |   1 +
 drivers/mtd/mtdpart.c           |  10 +
 drivers/of/property.c           |  84 +-----
 drivers/soc/imx/gpcv2.c         |   2 +-
 drivers/soc/renesas/rcar-sysc.c |   2 +-
 include/linux/device.h          |   1 +
 include/linux/fwnode.h          |  12 +-
 10 files changed, 344 insertions(+), 227 deletions(-)

Signature · 1 line

2.39.1.519.gcb327c4b5f-goog

Naresh, Tony, Abel, Geert, Dmitry, Maxim(s), Miquel, Luca, Doug, Martin, Jean-Philippe,

Can I get your Tested-by's for this v3 series please?

Vladimir,

Ccing you because DSA's and fw_devlink have known/existing problems (still in my TODOs to fix). But I want to make sure this series doesn't cause additional problems for DSA.

All,

This patch series improves fw_devlink in the following ways:

1. It no longer cares about a fwnode having a "compatible" property. It figures this out more dynamically. The only expectation is that fwnodes that are converted to devices actually get probed by a driver for the dependencies to be enforced correctly.

2. Finer grained dependency tracking. fw_devlink will now create device links from the consumer to the actual resource's device (if it has one,
Eg: gpio_device) instead of the parent supplier device. This improves things like async suspend/resume ordering, potentially remove the need for frameworks to create device links, more parallelized async probing, and better sync_state() tracking.

3. Handle hardware/software quirks where a child firmware node gets populated as a device before its parent firmware node AND actually supplies a non-optional resource to the parent firmware node's device.

4. Way more robust at cycle handling (see patch for the insane cases).

5. Stops depending on OF_POPULATED to figure out some corner cases.

6. Simplifies the work that needs to be done by the firmware specific code.

The v3 series has gone through my usual testing on my end and looks good to me.

Thanks,
Saravana

[1] - https://lore.kernel.org/lkml/[email protected]/
[2] - https://lore.kernel.org/lkml/CAGETcx-JUV1nj8wBJrTPfyvM7=Mre5j_vkVmZojeiumUGG6QZQ@mail.gmail.com/

v1 -> v2:
- Fixed Patch 1 to handle a corner case discussed in [2].
- New patch 10 to handle "fsl,imx8mq-gpc" being initialized by 2 drivers.
- New patch 11 to add fw_devlink support for SCMI devices.

v2 -> v3:
- Addressed most of Andy's comments in v2
- Added Colin and Sudeep's Tested-by for the series (except the imx and renesas patches)
- Added Sudeep's Acked-by for the scmi patch.
- Added Geert's Reviewed-by for the renesas patch.
- Fixed gpiolib crash reported by Naresh.
- Patch 6: Fix __fwnode_links_move_consumers() to preserve fwnode link flags.
- New Patch 12 to fix nvmem-cells issue reported by Maxim(s)/Miquel.
- Deleted some stale function doc in Patch 8

Cc Abel Vesa <[email protected]>

Cc Alexander Stein <[email protected]>

Cc Tony Lindgren <[email protected]>

Cc Sudeep Holla <[email protected]>

Cc Geert Uytterhoeven <[email protected]>

Cc John Stultz <[email protected]>

Cc Doug Anderson <[email protected]>

Cc Guenter Roeck <[email protected]>

Cc Dmitry Baryshkov <[email protected]>

Cc Maxim Kiselev <[email protected]>

Cc Maxim Kochetkov <[email protected]>

Cc Miquel Raynal <[email protected]>

Cc Luca Weiss <[email protected]>

Cc Colin Foster <[email protected]>

Cc Martin Kepplinger <[email protected]>

Cc Jean-Philippe Brucker <[email protected]>

Cc Vladimir Oltean <[email protected]>

Saravana Kannan (12):
driver core: fw_devlink: Don't purge child fwnode's consumer links driver core: fw_devlink: Improve check for fwnode with no
device/driver
soc: renesas: Move away from using OF_POPULATED for fw_devlink
gpiolib: Clear the gpio_device's fwnode initialized flag before adding driver core: fw_devlink: Add DL_FLAG_CYCLE support to device links driver core: fw_devlink: Allow marking a fwnode link as being part of
a cycle
driver core: fw_devlink: Consolidate device link flag computation driver core: fw_devlink: Make cycle detection more robust
of: property: Simplify of_link_to_phandle() irqchip/irq-imx-gpcv2: Mark fwnode device as not initialized
firmware: arm_scmi: Set fwnode for the scmi_device
mtd: mtdpart: Don't create platform device that'll never probe

Hi,

Quoted 83 lines · Saravana Kannan

Naresh, Tony, Abel, Geert, Dmitry, Maxim(s), Miquel, Luca, Doug, Martin, Jean-Philippe,

Can I get your Tested-by's for this v3 series please?

Vladimir,

Ccing you because DSA's and fw_devlink have known/existing problems (still in my TODOs to fix). But I want to make sure this series doesn't cause additional problems for DSA.

All,

This patch series improves fw_devlink in the following ways:

1. It no longer cares about a fwnode having a "compatible" property. It figures this out more dynamically. The only expectation is that fwnodes that are converted to devices actually get probed by a driver for the dependencies to be enforced correctly.

2. Finer grained dependency tracking. fw_devlink will now create device links from the consumer to the actual resource's device (if it has one,
Eg: gpio_device) instead of the parent supplier device. This improves things like async suspend/resume ordering, potentially remove the need for frameworks to create device links, more parallelized async probing, and better sync_state() tracking.

3. Handle hardware/software quirks where a child firmware node gets populated as a device before its parent firmware node AND actually supplies a non-optional resource to the parent firmware node's device.

4. Way more robust at cycle handling (see patch for the insane cases).

5. Stops depending on OF_POPULATED to figure out some corner cases.

6. Simplifies the work that needs to be done by the firmware specific code.

The v3 series has gone through my usual testing on my end and looks good to me.

Thanks,
Saravana

[1] - https://lore.kernel.org/lkml/[email protected]/
[2] - https://lore.kernel.org/lkml/CAGETcx-JUV1nj8wBJrTPfyvM7=Mre5j_vkVmZojeiumUGG6QZQ@mail.gmail.com/

v1 -> v2:
- Fixed Patch 1 to handle a corner case discussed in [2].
- New patch 10 to handle "fsl,imx8mq-gpc" being initialized by 2 drivers.
- New patch 11 to add fw_devlink support for SCMI devices.

v2 -> v3:
- Addressed most of Andy's comments in v2
- Added Colin and Sudeep's Tested-by for the series (except the imx and renesas patches)
- Added Sudeep's Acked-by for the scmi patch.
- Added Geert's Reviewed-by for the renesas patch.
- Fixed gpiolib crash reported by Naresh.
- Patch 6: Fix __fwnode_links_move_consumers() to preserve fwnode link flags.
- New Patch 12 to fix nvmem-cells issue reported by Maxim(s)/Miquel.
- Deleted some stale function doc in Patch 8

Cc Abel Vesa <[email protected]>

Cc Alexander Stein <[email protected]>

Cc Tony Lindgren <[email protected]>

Cc Sudeep Holla <[email protected]>

Cc Geert Uytterhoeven <[email protected]>

Cc John Stultz <[email protected]>

Cc Doug Anderson <[email protected]>

Cc Guenter Roeck <[email protected]>

Cc Dmitry Baryshkov <[email protected]>

Cc Maxim Kiselev <[email protected]>

Cc Maxim Kochetkov <[email protected]>

Cc Miquel Raynal <[email protected]>

Cc Luca Weiss <[email protected]>

Cc Colin Foster <[email protected]>

Cc Martin Kepplinger <[email protected]>

Cc Jean-Philippe Brucker <[email protected]>

Cc Vladimir Oltean <[email protected]>

Quoted 15 lines

Saravana Kannan (12):
driver core: fw_devlink: Don't purge child fwnode's consumer links driver core: fw_devlink: Improve check for fwnode with no
device/driver
soc: renesas: Move away from using OF_POPULATED for fw_devlink
gpiolib: Clear the gpio_device's fwnode initialized flag before adding driver core: fw_devlink: Add DL_FLAG_CYCLE support to device links driver core: fw_devlink: Allow marking a fwnode link as being part of
a cycle
driver core: fw_devlink: Consolidate device link flag computation driver core: fw_devlink: Make cycle detection more robust
of: property: Simplify of_link_to_phandle() irqchip/irq-imx-gpcv2: Mark fwnode device as not initialized
firmware: arm_scmi: Set fwnode for the scmi_device
mtd: mtdpart: Don't create platform device that'll never probe

I tested the whole series together on several devices. I tried to test on a wide variety since previous versions had broken due to all the dependency cycles in the display and some of these devices used different components in their display pipeline. I didn't do massive testing but did confirm that basic devices came up, including display.

Devices tested with your v3 applied atop v6.2-rc7-11-g05ecb680708a:

* sc7180-trogdor-lazor (with ps8640 bridge), which had failed to bring up the display on v2.

* sc7180-trogdor-lazor (with sn65dsi86 bridge)

* sc7180-trogdor-pazquel (with sn65dsi86 bridge)

* sc7180-trogdor-homestar (with ps8640 bridge)

* sc7180-trogdor-wormdingler

* sc7280-herobrine-villager

Tested-by Douglas Anderson <[email protected]>

Naresh, Tony, Abel, Geert, Dmitry, Maxim(s), Miquel, Luca, Doug, Martin, Jean-Philippe,

Can I get your Tested-by's for this v3 series please?

Hi Saravana,

Quoted 3 lines · Saravana Kannan

Naresh, Tony, Abel, Geert, Dmitry, Maxim(s), Miquel, Luca, Doug, Martin, Jean-Philippe,

Quoted 1 line

Can I get your Tested-by's for this v3 series please?

I have tested this on a variety of Renesas arm32/arm64 platforms, and on several RISC-V platforms.
Apart from the regression related to dynamic overlays caused by "[PATCH v3 09/12] of: property: Simplify of_link_to_phandle()" (which you may decide to ignore for now ;-)

Tested-by Geert Uytterhoeven <[email protected]>

Vladimir,

Ccing you because DSA's and fw_devlink have known/existing problems (still in my TODOs to fix). But I want to make sure this series doesn't cause additional problems for DSA.

All,

This patch series improves fw_devlink in the following ways:

1. It no longer cares about a fwnode having a "compatible" property. It figures this out more dynamically. The only expectation is that fwnodes that are converted to devices actually get probed by a driver for the dependencies to be enforced correctly.

2. Finer grained dependency tracking. fw_devlink will now create device links from the consumer to the actual resource's device (if it has one,
Eg: gpio_device) instead of the parent supplier device. This improves things like async suspend/resume ordering, potentially remove the need for frameworks to create device links, more parallelized async probing, and better sync_state() tracking.

3. Handle hardware/software quirks where a child firmware node gets populated as a device before its parent firmware node AND actually supplies a non-optional resource to the parent firmware node's device.

4. Way more robust at cycle handling (see patch for the insane cases).

5. Stops depending on OF_POPULATED to figure out some corner cases.

6. Simplifies the work that needs to be done by the firmware specific code.

The v3 series has gone through my usual testing on my end and looks good to me.

Hi Saravana,

Quoted 34 lines · Saravana Kannan

Vladimir,

Ccing you because DSA's and fw_devlink have known/existing problems (still in my TODOs to fix). But I want to make sure this series doesn't cause additional problems for DSA.

All,

This patch series improves fw_devlink in the following ways:

1. It no longer cares about a fwnode having a "compatible" property. It figures this out more dynamically. The only expectation is that fwnodes that are converted to devices actually get probed by a driver for the dependencies to be enforced correctly.

2. Finer grained dependency tracking. fw_devlink will now create device links from the consumer to the actual resource's device (if it has one,
Eg: gpio_device) instead of the parent supplier device. This improves things like async suspend/resume ordering, potentially remove the need for frameworks to create device links, more parallelized async probing, and better sync_state() tracking.

3. Handle hardware/software quirks where a child firmware node gets populated as a device before its parent firmware node AND actually supplies a non-optional resource to the parent firmware node's device.

4. Way more robust at cycle handling (see patch for the insane cases).

5. Stops depending on OF_POPULATED to figure out some corner cases.

6. Simplifies the work that needs to be done by the firmware specific code.

Quoted 2 lines

The v3 series has gone through my usual testing on my end and looks good to me.

Booted on an NXP LS1028A (arch/arm64/boot/dts/freescale/fsl-ls1028a-rdb.dts) and a Turris MOX (arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts) with no observed regressions.

Is there something specific you would like
me to test?

Quoted 38 lines · Vladimir Oltean

Hi Saravana,

Quoted 34 lines · Saravana Kannan

Vladimir,

Ccing you because DSA's and fw_devlink have known/existing problems (still in my TODOs to fix). But I want to make sure this series doesn't cause additional problems for DSA.

All,

This patch series improves fw_devlink in the following ways:

1. It no longer cares about a fwnode having a "compatible" property. It figures this out more dynamically. The only expectation is that fwnodes that are converted to devices actually get probed by a driver for the dependencies to be enforced correctly.

2. Finer grained dependency tracking. fw_devlink will now create device links from the consumer to the actual resource's device (if it has one,
Eg: gpio_device) instead of the parent supplier device. This improves things like async suspend/resume ordering, potentially remove the need for frameworks to create device links, more parallelized async probing, and better sync_state() tracking.

3. Handle hardware/software quirks where a child firmware node gets populated as a device before its parent firmware node AND actually supplies a non-optional resource to the parent firmware node's device.

4. Way more robust at cycle handling (see patch for the insane cases).

5. Stops depending on OF_POPULATED to figure out some corner cases.

6. Simplifies the work that needs to be done by the firmware specific code.

Quoted 6 lines

Quoted 2 lines

The v3 series has gone through my usual testing on my end and looks good to me.

Booted on an NXP LS1028A (arch/arm64/boot/dts/freescale/fsl-ls1028a-rdb.dts) and a Turris MOX (arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts) with no observed regressions.

Thanks for testing Vladimir!

Quoted 2 lines

Is there something specific you would like
me to test?

Not really, I just want to make sure the common DSA architectures don't hit any regression. In the hardware you tested, are there cases of PHYs where the supplier is the parent MDIO? I remember that being the only case where I needed special casing (FWNODE_FLAG_NEEDS_CHILD_BOUND_ON_ADD) in fw_devlink -- so it'll be good to make sure I didn't accidentally break anything there.

-Saravana

However, as I had explained in one of the first discussions here:
https://lore.kernel.org/netdev/20210901012826.iuy2bhvkrgahhrl7@skbuf/

it was always hit-or-miss whether the above device tree had an issue with fw_devlink or not: it depended on how the driver was written (and the mv88e6xxx switch driver was tricking the fw_devlink logic from that time to drop the device links because of an unrelated -EPROBE_DEFER).

After applying both of the above changes on top of yours, I confirm that the PHYs on the mv88e6xxx on Turris MOX still probe with their specific PHY driver rather than the generic one, and with interrupts (not poll mode):

even though I am seeing these error messages earlier in the boot process (maybe this is something to look into):

[    0.910219] mdio_bus d0032004.mdio-mii:10: Failed to create device link with d0032004.mdio-mii:10

[    0.943879] mv88e6085 d0032004.mdio-mii:12: switch 0x1900 detected: Marvell 88E6190, revision 1

If _on top_ of all the above, I also remove the logic that sets FWNODE_FLAG_NEEDS_CHILD_BOUND_ON_ADD:

then *finally* I get something approximating Alvin's reported issue. In my case, one switch out of 3 gets its PHYs bound to the Generic PHY driver (why not all is a story for another time):

So I guess that FWNODE_FLAG_NEEDS_CHILD_BOUND_ON_ADD does something.

Naresh, Tony, Abel, Geert, Dmitry, Maxim(s), Miquel, Luca, Doug, Martin, Jean-Philippe,

Can I get your Tested-by's for this v3 series please?

Naresh, Tony, Abel, Geert, Dmitry, Maxim(s), Miquel, Luca, Doug, Martin, Jean-Philippe,

Can I get your Tested-by's for this v3 series please?

Naresh, Tony, Abel, Geert, Dmitry, Maxim(s), Miquel, Luca, Doug, Martin, Jean-Philippe,

Can I get your Tested-by's for this v3 series please?

Vladimir,

Ccing you because DSA's and fw_devlink have known/existing problems (still in my TODOs to fix). But I want to make sure this series doesn't cause additional problems for DSA.

All,

This patch series improves fw_devlink in the following ways:

1. It no longer cares about a fwnode having a "compatible" property. It figures this out more dynamically. The only expectation is that fwnodes that are converted to devices actually get probed by a driver for the dependencies to be enforced correctly.

2. Finer grained dependency tracking. fw_devlink will now create device links from the consumer to the actual resource's device (if it has one,
Eg: gpio_device) instead of the parent supplier device. This improves things like async suspend/resume ordering, potentially remove the need for frameworks to create device links, more parallelized async probing, and better sync_state() tracking.

3. Handle hardware/software quirks where a child firmware node gets populated as a device before its parent firmware node AND actually supplies a non-optional resource to the parent firmware node's device.

4. Way more robust at cycle handling (see patch for the insane cases).

5. Stops depending on OF_POPULATED to figure out some corner cases.

6. Simplifies the work that needs to be done by the firmware specific code.

The v3 series has gone through my usual testing on my end and looks good to me.

Thanks,
Saravana

[1] - https://lore.kernel.org/lkml/[email protected]/
[2] - https://lore.kernel.org/lkml/CAGETcx-JUV1nj8wBJrTPfyvM7=Mre5j_vkVmZojeiumUGG6QZQ@mail.gmail.com/

v1 -> v2:
- Fixed Patch 1 to handle a corner case discussed in [2].
- New patch 10 to handle "fsl,imx8mq-gpc" being initialized by 2 drivers.
- New patch 11 to add fw_devlink support for SCMI devices.

v2 -> v3:
- Addressed most of Andy's comments in v2
- Added Colin and Sudeep's Tested-by for the series (except the imx and renesas patches)
- Added Sudeep's Acked-by for the scmi patch.
- Added Geert's Reviewed-by for the renesas patch.
- Fixed gpiolib crash reported by Naresh.
- Patch 6: Fix __fwnode_links_move_consumers() to preserve fwnode link flags.
- New Patch 12 to fix nvmem-cells issue reported by Maxim(s)/Miquel.
- Deleted some stale function doc in Patch 8

Cc Abel Vesa <[email protected]>

Cc Alexander Stein <[email protected]>

Cc Tony Lindgren <[email protected]>

Cc Sudeep Holla <[email protected]>

Cc Geert Uytterhoeven <[email protected]>

Cc John Stultz <[email protected]>

Cc Doug Anderson <[email protected]>

Cc Guenter Roeck <[email protected]>

Cc Dmitry Baryshkov <[email protected]>

Cc Maxim Kiselev <[email protected]>

Cc Maxim Kochetkov <[email protected]>

Cc Miquel Raynal <[email protected]>

Cc Luca Weiss <[email protected]>

Cc Colin Foster <[email protected]>

Cc Martin Kepplinger <[email protected]>

Cc Jean-Philippe Brucker <[email protected]>

Cc Vladimir Oltean <[email protected]>

Saravana Kannan (12):
driver core: fw_devlink: Don't purge child fwnode's consumer links driver core: fw_devlink: Improve check for fwnode with no
device/driver
soc: renesas: Move away from using OF_POPULATED for fw_devlink
gpiolib: Clear the gpio_device's fwnode initialized flag before adding driver core: fw_devlink: Add DL_FLAG_CYCLE support to device links driver core: fw_devlink: Allow marking a fwnode link as being part of
a cycle
driver core: fw_devlink: Consolidate device link flag computation driver core: fw_devlink: Make cycle detection more robust
of: property: Simplify of_link_to_phandle() irqchip/irq-imx-gpcv2: Mark fwnode device as not initialized
firmware: arm_scmi: Set fwnode for the scmi_device
mtd: mtdpart: Don't create platform device that'll never probe

  drivers/base/core.c             | 449 +++++++++++++++++++++-----------
  drivers/firmware/arm_scmi/bus.c |   3 +-
  drivers/gpio/gpiolib.c          |   7 +
  drivers/irqchip/irq-imx-gpcv2.c |   1 +
  drivers/mtd/mtdpart.c           |  10 +
  drivers/of/property.c           |  84 +-----
  drivers/soc/imx/gpcv2.c         |   2 +-
  drivers/soc/renesas/rcar-sysc.c |   2 +-
  include/linux/device.h          |   1 +
  include/linux/fwnode.h          |  12 +-
  10 files changed, 344 insertions(+), 227 deletions(-)

Quoted 34 lines · Saravana Kannan

Naresh, Tony, Abel, Geert, Dmitry, Maxim(s), Miquel, Luca, Doug, Martin, Jean-Philippe,

Can I get your Tested-by's for this v3 series please?

Vladimir,

Ccing you because DSA's and fw_devlink have known/existing problems (still in my TODOs to fix). But I want to make sure this series doesn't cause additional problems for DSA.

All,

This patch series improves fw_devlink in the following ways:

1. It no longer cares about a fwnode having a "compatible" property. It figures this out more dynamically. The only expectation is that fwnodes that are converted to devices actually get probed by a driver for the dependencies to be enforced correctly.

2. Finer grained dependency tracking. fw_devlink will now create device links from the consumer to the actual resource's device (if it has one,
Eg: gpio_device) instead of the parent supplier device. This improves things like async suspend/resume ordering, potentially remove the need for frameworks to create device links, more parallelized async probing, and better sync_state() tracking.

3. Handle hardware/software quirks where a child firmware node gets populated as a device before its parent firmware node AND actually supplies a non-optional resource to the parent firmware node's device.

4. Way more robust at cycle handling (see patch for the insane cases).

Quoted 7 lines

5. Stops depending on OF_POPULATED to figure out some corner cases.

6. Simplifies the work that needs to be done by the firmware specific code.

The v3 series has gone through my usual testing on my end and looks good to me.

Saravana,

Please excuse me, I was completely overwhelmed with my regular work and had no time to properly test the series, while doing just the light test would defeat the purpose of testing.

Tested-by Dmitry Baryshkov <[email protected]> # Qualcomm RB3

Thanks a lot for going through all the troubles and hunting all the issues!

Just a note: on an RB3 device (arm64 qcom/sdm845-db845c.dtsi) extended with the patch at [3] I got the following messages in dmesg:

[    1.051325] platform ae00000.mdss: Failed to create device link
with ae00000.mdss
[    1.059368] platform ae00000.mdss: Failed to create device link
with ae00000.mdss
[    1.067174] platform ae00000.mdss: Failed to create device link
with ae00000.mdss
[    1.088322] platform c440000.spmi: Failed to create device link
with c440000.spmi
[    1.096019] platform c440000.spmi: Failed to create device link
with c440000.spmi
[    1.103707] platform c440000.spmi: Failed to create device link
with c440000.spmi
[    1.111400] platform c440000.spmi: Failed to create device link
with c440000.spmi
[    1.119141] platform c440000.spmi: Failed to create device link
with c440000.spmi
[    1.126825] platform c440000.spmi: Failed to create device link
with c440000.spmi
[    2.024763] qcom-lab-ibb-regulator c440000.spmi:pmic@3:labibb:
Failed to create device link with c440000.spmi
[    2.035026] qcom-lab-ibb-regulator c440000.spmi:pmic@3:labibb:

Failed to create device link with c440000.spmi

They look to be harmless, but it might be good to filter some of them out? Especially the ones which tell about creating a device link pointing back to the same device.

[3] https://lore.kernel.org/linux-arm-msm/[email protected]/

Quoted 57 lines

Thanks,
Saravana

[1] - https://lore.kernel.org/lkml/[email protected]/
[2] - https://lore.kernel.org/lkml/CAGETcx-JUV1nj8wBJrTPfyvM7=Mre5j_vkVmZojeiumUGG6QZQ@mail.gmail.com/

v1 -> v2:
- Fixed Patch 1 to handle a corner case discussed in [2].
- New patch 10 to handle "fsl,imx8mq-gpc" being initialized by 2 drivers.
- New patch 11 to add fw_devlink support for SCMI devices.

v2 -> v3:
- Addressed most of Andy's comments in v2
- Added Colin and Sudeep's Tested-by for the series (except the imx and renesas patches)
- Added Sudeep's Acked-by for the scmi patch.
- Added Geert's Reviewed-by for the renesas patch.
- Fixed gpiolib crash reported by Naresh.
- Patch 6: Fix __fwnode_links_move_consumers() to preserve fwnode link flags.
- New Patch 12 to fix nvmem-cells issue reported by Maxim(s)/Miquel.
- Deleted some stale function doc in Patch 8

Cc Abel Vesa <[email protected]>

Cc Alexander Stein <[email protected]>

Cc Tony Lindgren <[email protected]>

Cc Sudeep Holla <[email protected]>

Cc Geert Uytterhoeven <[email protected]>

Cc John Stultz <[email protected]>

Cc Doug Anderson <[email protected]>

Cc Guenter Roeck <[email protected]>

Cc Dmitry Baryshkov <[email protected]>

Cc Maxim Kiselev <[email protected]>

Cc Maxim Kochetkov <[email protected]>

Cc Miquel Raynal <[email protected]>

Cc Luca Weiss <[email protected]>

Cc Colin Foster <[email protected]>

Cc Martin Kepplinger <[email protected]>

Cc Jean-Philippe Brucker <[email protected]>

Cc Vladimir Oltean <[email protected]>

Saravana Kannan (12):
driver core: fw_devlink: Don't purge child fwnode's consumer links driver core: fw_devlink: Improve check for fwnode with no
device/driver
soc: renesas: Move away from using OF_POPULATED for fw_devlink
gpiolib: Clear the gpio_device's fwnode initialized flag before adding driver core: fw_devlink: Add DL_FLAG_CYCLE support to device links driver core: fw_devlink: Allow marking a fwnode link as being part of
a cycle
driver core: fw_devlink: Consolidate device link flag computation driver core: fw_devlink: Make cycle detection more robust
of: property: Simplify of_link_to_phandle() irqchip/irq-imx-gpcv2: Mark fwnode device as not initialized
firmware: arm_scmi: Set fwnode for the scmi_device
mtd: mtdpart: Don't create platform device that'll never probe

Quoted 12 lines

  drivers/base/core.c             | 449 +++++++++++++++++++++-----------
  drivers/firmware/arm_scmi/bus.c |   3 +-
  drivers/gpio/gpiolib.c          |   7 +
  drivers/irqchip/irq-imx-gpcv2.c |   1 +
  drivers/mtd/mtdpart.c           |  10 +
  drivers/of/property.c           |  84 +-----
  drivers/soc/imx/gpcv2.c         |   2 +-
  drivers/soc/renesas/rcar-sysc.c |   2 +-
  include/linux/device.h          |   1 +
  include/linux/fwnode.h          |  12 +-
  10 files changed, 344 insertions(+), 227 deletions(-)

--
With best wishes

Dmitry