Re: [PATCH v3 09/12] of: property: Simplify of_link_to_phandle()

[Saravana Kannan <saravanak@google.com>]

On Tue, Feb 7, 2023 at 12:57 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Saravana,
>
> On Tue, Feb 7, 2023 at 2:42 AM Saravana Kannan <saravanak@google.com> wrote:
> > 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 <saravanak@google.com>
>
> 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.

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.

Thanks,
Saravana