Message-ID: <20220509180917.0f0ae851@xps-bootlin>
Date:   Mon, 9 May 2022 18:09:17 +0200
From:   Clément Léger <clement.leger@...tlin.com>
To:     Frank Rowand <frowand.list@...il.com>
Cc:     Rob Herring <robh+dt@...nel.org>,
        Pantelis Antoniou <pantelis.antoniou@...sulko.com>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        Allan Nielsen <allan.nielsen@...rochip.com>,
        Horatiu Vultur <horatiu.vultur@...rochip.com>,
        Steen Hegelund <steen.hegelund@...rochip.com>,
        Thomas Petazzoni <thomas.petazonni@...tlin.com>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Mark Brown <broonie@...nel.org>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Hans de Goede <hdegoede@...hat.com>,
        Andrew Lunn <andrew@...n.ch>, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org
Subject: Re: [PATCH 0/3] add dynamic PCI device of_node creation for overlay

Le Mon, 9 May 2022 10:56:36 -0500,
Frank Rowand <frowand.list@...il.com> a écrit :

> > Hi Frank,
> > 
> > This work uses the kernel space interface (of_overlay_fdt_apply())
> > and the device tree overlay is builtin the driver. This interface
> > was used until recently by rcu-dcar driver. While the only user
> > (sic), this seems to work pretty well and I was able to use it
> > successfully.  
> 
> Yes, of_overlay_fdt_apply() was used by one driver.  But that driver
> was explicitly recognized as a grandfathered exception, and not an
> example for other users.  It was finally removed in 5.18-rc1.

I noticed that unfortunately.

> 
> You may have used of_overlay_fdt_apply() in a specific use case at
> a specific kernel version, but if you read through the references
> I provided you will find that applying overlays after the kernel
> boots is a fragile endeavor, with expectations of bugs and problems
> being exposed as usage is changed (simple example is that my adding
> some overlay notifier unittests exposed yet another memory leak).
> 
> The reference that I provided also shows how the overlay code is
> being improved over time.  Even with improvements, it will remain
> fragile.

Acked.

> 
> > 
> > Moreover, this support targets at using this with PCI devices. This
> > devices are really well contained and do not interfere with other
> > devices. This actually consists in adding a complete subtree into
> > the existing device-tree and thus it limits the interactions between
> > potentially platform provided devices and PCI ones.  
> 
> Yes, that it is very important that you have described this fact, both
> here and in other emails.  Thank you for that information, it does
> help understanding the alternatives.
> 
> I've hesitated in recommending a specific solution before better
> understanding the architecture of your pcie board and drivers, but
> I've delayed too long, so I am going to go ahead and mention one
> possibility at the risk of not yet fully understanding the situation.
> 
> On the surface, it appears that your need might be well met by having
> a base devicetree that describes all of the pcie nodes, but with each
> node having a status of "disabled" so that they will not be used.
> Have a devicetree overlay describing the pcie card (as you proposed),
> where the overlay also includes a status of "ok" for the pcie node.
> Applying the overlay, with a method of redirecting the target to a
> specific pcie node would change the status of the pcie node to enable
> its use.  (You have already proposed a patch to modify
> of_overlay_fdt_apply() to allow a modified target, so not a new
> concept from me.)  My suggestion is to apply the overlay devicetree
> to the base devicetree before the combined FDT devicetree is passed
> to the kernel at boot.  The overlay apply could be done by several
> different entities.  It could be before the bootloader executes, it
> could be done by the bootloader, it could be done by a shim between
> the bootloader and the kernel.  This method avoids all of the issues
> of applying an overlay to a running system that I find problematic.
> It is also a method used by the U-boot bootloader, as an example.

Ok, that is actually possible on a system that is given a device-tree
by the bootloader. But on a system that is desrcibed using ACPI (such
as the x86), this is much more difficult (at least to my knowledge)...
We want this feature to be easy to use for the end user. Adding such
configuration which also differs between various architecture is
clearly not so easy to setup.

Moreover, since the PCI is meant to be "Plug and Play", such
configuration would completely break that. If the user switches the
PCIe card from one slot to another, the bootloader configuration will
need to be modified. This seems a big no way for me (and for the user).

> 
> The other big issue is mixing ACPI and devicetree on a single system.
> Historically, the Linux devicetree community has not been receptive
> to the ides of that mixture.  Your example might be a specific case
> where the two can be isolated from each other, or maybe not.  (For
> disclosure, I am essentially ACPI ignorant.)  I suspect that mixing
> ACPI and devicetree is a recipe for disaster in the general case.

Agreed, on that fact, it did raised some eyebrows, and it was for that
specific concern that initially, I proposed the fwnode solution.
Honestly, the fwnode conversion represent a lot of work (hundreds of
lines easily) + requires a conversion of all the subsystem that are not
fwnode ready (spoiler: almost all of them are not ready). 

After implementing Rob's solution, the device-tree overlay really seems
the cleaner to me and requires much less modifications.

> 
> More to come later as I finish reading through the various threads.

Ok, thanks for your time !

Clément

> 
> -Frank

Powered by blists - more mailing lists