From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=66.187.233.73; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 75D1D203BBB80 for ; Thu, 24 May 2018 07:39:44 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id CA6754022407; Thu, 24 May 2018 14:39:43 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-121-84.rdu2.redhat.com [10.10.121.84]) by smtp.corp.redhat.com (Postfix) with ESMTP id 1833F2026609; Thu, 24 May 2018 14:39:42 +0000 (UTC) To: Ard Biesheuvel Cc: edk2-devel-01 , Jordan Justen References: <20180523202121.8125-1-lersek@redhat.com> <20180523202121.8125-2-lersek@redhat.com> From: Laszlo Ersek Message-ID: <79485048-7ee5-c3ee-40b6-25f0eeb46ca6@redhat.com> Date: Thu, 24 May 2018 16:39:42 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.78 on 10.11.54.4 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.6]); Thu, 24 May 2018 14:39:43 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.6]); Thu, 24 May 2018 14:39:43 +0000 (UTC) for IP:'10.11.54.4' DOMAIN:'int-mx04.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' Subject: Re: [PATCH v2 1/7] OvmfPkg: introduce PciCapLib X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 24 May 2018 14:39:45 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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