On 7 April 2016 at 09:57, Laszlo Ersek <[email protected]> wrote:
> On 04/07/16 09:32, Ard Biesheuvel wrote:
>> On 6 April 2016 at 22:25, Laszlo Ersek <[email protected]> wrote:
>
>>> (5) For new code, please strive to keep the Packages / LibraryClasses /
>>> etc sections in INF files sorted. First, it looks nice; second (more
>>> importantly), it helps you verify that you list all the library classes
>>> explicitly that you depend on.
>>>
>>> For example, DebugLib.h is included from the source code (correctly),
>>> but DebugLib is not present here (it should be). The linker succeeds
>>> because one of the library instances we depend on pulls in DebugLib
>>> anyway, but it's not nice. Concord can be reached if the C source
>>> includes Library/*.h headers until the compiler stops whining
>>
>> I don't think minimising the set of #includes should be a goal in
>> itself, especially since you may end up relying on transitive
>> includes. A source file should include the header files for all
>> functionality it pulls in from other objects.
>> I know we still end up relying on transitive includes for things like
>> types, and EDK2 with its AutoGen.? may supply some things implicitly
>> as well. But in general, I don't think removing header #includes
>> because you can is a good thing.
>
> Okay.
>
>> Other than that, I agree that this should be cleaned up
>
> [snip]
>
>>>> +STATIC
>>>> +EFI_STATUS
>>>> +EFIAPI
>>>> +FindCompatibleNodeProperty (
>>>> + IN FDT_CLIENT_PROTOCOL *This,
>>>> + IN CONST CHAR8 *CompatibleString,
>>>> + IN CONST CHAR8 *PropertyName,
>>>> + OUT CONST VOID **Prop,
>>>> + OUT UINTN *PropSize OPTIONAL
>>>> + )
>>>> +{
>>>> + EFI_STATUS Status;
>>>> + INT32 Node;
>>>> +
>>>> + Status = FindCompatibleNode (This, CompatibleString, &Node);
>>>> + if (EFI_ERROR (Status)) {
>>>> + return Status;
>>>> + }
>>>> +
>>>> + return GetNodeProperty (This, Node, PropertyName, Prop, PropSize);
>>>> +}
>>>
>>> (13) Are we going to use this interface frequently? Hm... grepping the
>>> tree in your "virt-fdt-refactor" branch, I can find four call sites. One
>>> is just below, another one in ArmVirtPsciResetSystemLib, and the last
>>> two in ArmVirtTimerFdtClientLib.
>>>
>>> I'm torn. This should be a helper function in a library.
>>>
>>> Meh, don't bother; keep it as-is. We can always rework this protocol if
>>> necessary. :)
>>>
>>
>> I was torn myself between putting the DT base address in a protocol
>> and nothing else,
>
> Yup, it crossed my mind.
>
>> and providing some access functions around it. I
>> have tried to find a sweet spot where we don't duplicate too much code
>> in the caller, and expose an abstract interface which does not allow
>> the DT to be arbitrarily mangled.
>
> I think there are certainly arguments in favor of thinking about a
> protocol like a shared library (dynamic linking). If we consider it a
> service / singleton protocol, and simply want to avoid linking the code
> statically into a bunch of independent EFI binaries, that's a good
> enough reason for protocol-izing it. I don't think it's necessary for a
> protocol to have several instances, and for each instance to have a
> bunch of private data to it.
>
Agreed. Let's be pragmatic here: all we want to reuse PCD based
drivers that produce architectural protocols, and that are normally
fixed for a platform but now need to be discovered from DT, which
imposed some kind of ordering. I am happy with almost anything that
achieves that in a more maintainable way than VirtFdtDxe, even if it
does not rhyme :-)
>>
>>> (14) How about renaming this function (and the protocol member) to
>>> GetCompatibleNodeProperty()? Just a suggestion, feel free to disagree.
>>>
>>
>> I used Find vs Get since the former suggests that you look for
>> something that may be anywhere, or not present at all, whereas Get
>> suggests 'just get me the node, even if you have to create it'
>
> I think GetNodeProperty() doesn't comply with that already.
>
True.
>> But as I said, I am mostly interested in the protocol dependency that
>> this provides, and the actual interface could look wildly different
>> for all I care.
>
> I think the interface looks good thus far, I'd just like it to be a bit
> more intuitive. That's why I suggested docs, or name(s) that I felt
> would be improvement.
>
> [snip]
>
>>>> +STATIC
>>>> +EFI_STATUS
>>>> +GetChosenNode (
>>>> + IN FDT_CLIENT_PROTOCOL *This,
>>>> + OUT INT32 *Node
>>>> + )
>>>> +{
>>>> + INT32 NewNode;
>>>> +
>>>> + ASSERT (mDeviceTreeBase != NULL);
>>>> +
>>>> + NewNode = fdt_path_offset (mDeviceTreeBase, "/chosen");
>>>> + if (NewNode < 0) {
>>>> + NewNode = fdt_add_subnode (mDeviceTreeBase, 0, "/chosen");
>>>> + }
>>>> +
>>>> + if (NewNode < 0) {
>>>> + return EFI_OUT_OF_RESOURCES;
>>>> + }
>>>> +
>>>> + return EFI_SUCCESS;
>>>> +}
>>>
>>> (17) Please rename this function (and the protocol member) to
>>> GetOrInsertChosenNode().
>>>
>>> In general, it wouldn't be bad if you could document all the protocol
>>> member functions (according to the edk2 coding style, with leading
>>> comment blocks). I don't insist (the functions are quite small), but
>>> without such documentation, the function names become way more
>>> important, and should be very clear.
>>>
>>> (Also, in my experience, writing such function comments leads to
>>> "enlightenment" occasionally :) Opportunities for unification etc. I
>>> can't point at any candidates in this patch; it's just a generic
>>> remark.)
>>>
>>
>> Happy to do that, but I deliberately omitted it for v1 considering the
>> high likelihood that we ultimately end up with something completely
>> different.
>
> Good point. It does make sense to delay the docs until we get to the end
> of the series and the interface proves good (for what we need it).
>
> [snip]
>
>> We haven't discussed the general goal of this series, but I assume
>> you're on board with that, given that you are reviewing in detail
>> already.
>
> Yes, I read the blurb. On one hand, we can have one iteration over the
> FDT (+), a bunch of PCDs (which are not flexible enough for exposing
> repeating data, BTW) (-), a driver that is tied to everything (-), and
> an APRIORI section (-).
>
> On the other hand, each interested driver might have to iterate over the
> FDT (-) albeit through the protocol, the data fished out of the DTB
> could be arbitrary (+), the FDT accesses would be separated from each
> other functionally (+), and DepExes could replace the APRIORI section (+).
>
> I think the conversion is valuable. I do expect some snags (I skimmed a
> bit ahead in the series) with PCDs that we'll have to set anyway,
> because core drivers depend on them. Plugin libraries can hopefully help
> with that (or else we might be able to prove the necessary ordering by
> different means). I'll comment on this in more detail when I look at the
> affected patches.
>
Yes, please. And since only virtio/mmio and xenio/mmio remain in
VirtFdtDxe, I could actually handle that as well in a v2, and get rid
of VirtFdtDxe completely.
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
