On 24 May 2018 at 16:39, Laszlo Ersek <ler...@redhat.com> wrote:
> On 05/24/18 09:53, Ard Biesheuvel wrote:
>
>>> +STATIC
>>> +VOID
>>> +EFIAPI
>>> +DebugDumpPciCapList (
>>> + IN PCI_CAP_LIST *CapList
>>> + )
>>> +{
>>> + DEBUG_CODE_BEGIN ();
>>> + ORDERED_COLLECTION_ENTRY *PciCapEntry;
>>> +
>>> + for (PciCapEntry = OrderedCollectionMin (CapList->Capabilities);
>>> + PciCapEntry != NULL;
>>> + PciCapEntry = OrderedCollectionNext (PciCapEntry)) {
>>> + PCI_CAP *PciCap;
>>> + RETURN_STATUS Status;
>>> + PCI_CAP_INFO Info;
>>> +
>>
>> Move these out of the loop?
>
>>> +STATIC
>>> +VOID
>>> +EmptyAndUninitPciCapCollection (
>>> + IN OUT ORDERED_COLLECTION *PciCapCollection,
>>> + IN BOOLEAN FreePciCap
>>> + )
>>> +{
>>> + ORDERED_COLLECTION_ENTRY *PciCapEntry;
>>> + ORDERED_COLLECTION_ENTRY *NextEntry;
>>> +
>>> + for (PciCapEntry = OrderedCollectionMin (PciCapCollection);
>>> + PciCapEntry != NULL;
>>> + PciCapEntry = NextEntry) {
>>> + PCI_CAP *PciCap;
>>> +
>>
>> and this one
>
>>> +RETURN_STATUS
>>> +EFIAPI
>>> +PciCapListInit (
>>> + IN PCI_CAP_DEV *PciDevice,
>>> + OUT PCI_CAP_LIST **CapList
>>> + )
>>> +{
>>> + PCI_CAP_LIST *OutCapList;
>>> + RETURN_STATUS Status;
>>> + ORDERED_COLLECTION *CapHdrOffsets;
>>> + UINT16 PciStatusReg;
>>> + BOOLEAN DeviceIsExpress;
>>> + ORDERED_COLLECTION_ENTRY *OffsetEntry;
>>> +
>>> + //
>>> + // Allocate the output structure.
>>> + //
>>> + OutCapList = AllocatePool (sizeof *OutCapList);
>>> + if (OutCapList == NULL) {
>>> + return RETURN_OUT_OF_RESOURCES;
>>> + }
>>> + //
>>> + // The OutCapList->Capabilities collection owns the PCI_CAP structures
>>> and
>>> + // orders them based on PCI_CAP.Key.
>>> + //
>>> + OutCapList->Capabilities = OrderedCollectionInit (ComparePciCap,
>>> + ComparePciCapKey);
>>> + if (OutCapList->Capabilities == NULL) {
>>> + Status = RETURN_OUT_OF_RESOURCES;
>>> + goto FreeOutCapList;
>>> + }
>>> +
>>> + //
>>> + // The (temporary) CapHdrOffsets collection only references PCI_CAP
>>> + // structures, and orders them based on PCI_CAP.Offset.
>>> + //
>>> + CapHdrOffsets = OrderedCollectionInit (ComparePciCapOffset,
>>> + ComparePciCapOffsetKey);
>>> + if (CapHdrOffsets == NULL) {
>>> + Status = RETURN_OUT_OF_RESOURCES;
>>> + goto FreeCapabilities;
>>> + }
>>> +
>>> + //
>>> + // Whether the device is PCI Express depends on the normal capability
>>> with
>>> + // identifier EFI_PCI_CAPABILITY_ID_PCIEXP.
>>> + //
>>> + DeviceIsExpress = FALSE;
>>> +
>>> + //
>>> + // Check whether a normal capabilities list is present. If there's none,
>>> + // that's not an error; we'll just return OutCapList->Capabilities empty.
>>> + //
>>> + Status = PciDevice->ReadConfig (PciDevice, PCI_PRIMARY_STATUS_OFFSET,
>>> + &PciStatusReg, sizeof PciStatusReg);
>>> + if (RETURN_ERROR (Status)) {
>>> + goto FreeCapHdrOffsets;
>>> + }
>>> + if ((PciStatusReg & EFI_PCI_STATUS_CAPABILITY) != 0) {
>>> + UINT8 NormalCapHdrOffset;
>>> +
>>
>> and this one
>>
>>> + //
>>> + // Fetch the start offset of the normal capabilities list.
>>> + //
>>> + Status = PciDevice->ReadConfig (PciDevice,
>>> PCI_CAPBILITY_POINTER_OFFSET,
>>> + &NormalCapHdrOffset, sizeof NormalCapHdrOffset);
>>> + if (RETURN_ERROR (Status)) {
>>> + goto FreeCapHdrOffsets;
>>> + }
>>> +
>>> + //
>>> + // Traverse the normal capabilities list.
>>> + //
>>> + NormalCapHdrOffset &= 0xFC;
>>> + while (NormalCapHdrOffset > 0) {
>>> + EFI_PCI_CAPABILITY_HDR NormalCapHdr;
>>> +
>>
>> and this one.
>>
>> Perhaps I am missing something? After four instances, it seems
>> deliberate rather than accidental :-)
>
> It's totally deliberate. C89 supports scoping auto variables more
> tightly than at the function scope, and I liberally use that feature --
> scoping local variables as tightly as possible helps humans reason about
> data flow. This is characteristic of all C code I write.
>
> The coding style writes,
>
> https://edk2-docs.gitbooks.io/edk-ii-c-coding-standards-specification/content/5_source_files/54_code_file_structure.html#54-code-file-structure
>
> "Data declarations may follow the opening brace of a compound statement,
> regardless of nesting depth, and before any code generating statements
> have been entered. Other than at the outermost block of a function body,
> this type of declaration is strongly discouraged."
>
> It's discouraged, but not outright forbidden, and personally I find that
> lumping all local variables together at the top decreases readability
> and harms my ability to reason about data flow.
>
> If you'd really like me to move these variable definitions to the tops
> of their respective functions, knowing my motive, I can do it, of
> course. Do you want me to? :)
>
Please don't. And thanks for educating me. I fully agree that
declaring all variables at function scope is silly, and I always
assumed it was mandated by the coding style.
>>> +RETURN_STATUS
>>> +EFIAPI
>>> +PciCapGetInfo (
>>> + IN PCI_CAP *Cap,
>>> + OUT PCI_CAP_INFO *Info
>>> + )
>>> +{
>>> + PCI_CAP *InstanceZero;
>>> +
>>
>> Nit: add
>>
>> ASSERT (Info != NULL);
>>
>> here?
>>
>> I know it seems rather arbitrary to add it here and not anywhere else,
>> but PciCapGetInfo() is part of the API, and dereferencing Info [which
>> may be the result of e.g., a pool allocation] for writing is
>> particularly bad.
>
>
> I will add the ASSERT().
>
> (I hope I didn't miss any of your comments!)
>
It's just a nit, feel free to ignore.
In any case,
Reviewed-by: Ard Biesheuvel <ard.biesheu...@linaro.org>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
