Re: [PATCH v2 1/7] OvmfPkg: introduce PciCapLib

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 24 May 2018 at 16:39, Laszlo Ersek <lersek@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.biesheuvel@linaro.org>