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

[Laszlo Ersek <lersek@redhat.com>]

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? :)

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

Thanks,
Laszlo