Message-ID: <e6828dce-3e10-779e-4d12-67e7bdfd0f73@gmail.com>
Date:   Mon, 9 May 2022 15:07:18 -0500
From:   Frank Rowand <frowand.list@...il.com>
To:     Clément Léger <clement.leger@...tlin.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

On 5/9/22 11:09, Clément Léger wrote:
> 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.
> 

Apologies if my following questions have already been answered in the
other threads...

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

Are you trying to make your card work on any ACPI based system (x86,
x86-64, etc)?  Or do you have a specific model of computer that you
want to make this work on for a specific customer or appliance?

If for many arbitrary systems, can you limit it to one architecture
or sub-architecture?

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

Yes.  I was envisioning the pre-bootloader, bootloader, or Linux pre-boot
shim dynamically determining the slot containing the card, and applying
the overlay devicetree to the base devicetree, retargeting the overlay
to the proper location, before the Linux boot.

The base devicetree would be for a specific type of machine or family
of machines, just as is the case for all devicetree based systems.

> 
>>
>> 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 !

Your welcome.  I'll keep looking deeper into the previous threads.

-Frank

> 
> Clément
> 
>>
>> -Frank
> 
> .

Powered by blists - more mailing lists