From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4001:c0b::242; helo=mail-it0-x242.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-it0-x242.google.com (mail-it0-x242.google.com [IPv6:2607:f8b0:4001:c0b::242]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id D621421BADAB2 for ; Thu, 24 May 2018 07:41:55 -0700 (PDT) Received: by mail-it0-x242.google.com with SMTP id 144-v6so2651893iti.5 for ; Thu, 24 May 2018 07:41:55 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=xslcVmTaWH2/XsHYD5WzItmf37CHhi7evHPE55Rg2lk=; b=Jr5FmVR3gTYWoQadOPPC9B0I1UeWMemyMG8MwPop+8tHjFSpN+CZWV27yDlB5nMft3 z2T9AJQFMA/gbb/oc93V8gPbV2psHwnJ57qSGZSEqmZaULpcZtb9G+kVEc0rN590xyfB rAy0ppbpDm/+8s5Q3J1Svfx9beX6wgxtjToqs= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=xslcVmTaWH2/XsHYD5WzItmf37CHhi7evHPE55Rg2lk=; b=r+k17mGFF9V1rpme/S/cBmQ2E4ZODf6jQhno3ezavCe/HPdibPSBxmVgpjo0/+TRZP TEnMh2IikClklAKd/FCQ4nsjwCPUwm3Jiro+zSfjyp43Hv3OqZUvEbggsVD7jNFRLxDV 12ziTS2YEyM+RT0tH3HNGR1sXcKzMMVFYAbFRfFTu+kEx1SXgcYjZuXTwL5fRxly4+DH jCqis5WrKCliWQoCrJ7BRVZyN560ZC8L8lYHP4Fx+9uT6LYkztbkr3CfwwaHGUE2TDtG piaem2yDHE3UbFcPAhrBCTKiGy3/qtbG+It1bKBH/8UxRIdi5YKeQSTD9Rd3FGzexpOx 4iiA== X-Gm-Message-State: ALKqPweSMwJmgscvtzsChmATG6lbjfTsu0V2ci89QU+KGtvX7Br/OB5n j5JHpzdgx6Nfswn01/BbgWmu1wtOiVC2dAOOEF65aw== X-Google-Smtp-Source: AB8JxZrpOms6liShmLUnFU5yxOvEjg9w0YCmw1Nsgceldq0nHyCXNBd67sVk1PrJOC5JGo7mZP7AUu+jq4o++MHxPXc= X-Received: by 2002:a24:534e:: with SMTP id n75-v6mr9617550itb.138.1527172914938; Thu, 24 May 2018 07:41:54 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a6b:bb86:0:0:0:0:0 with HTTP; Thu, 24 May 2018 07:41:54 -0700 (PDT) In-Reply-To: <79485048-7ee5-c3ee-40b6-25f0eeb46ca6@redhat.com> References: <20180523202121.8125-1-lersek@redhat.com> <20180523202121.8125-2-lersek@redhat.com> <79485048-7ee5-c3ee-40b6-25f0eeb46ca6@redhat.com> From: Ard Biesheuvel Date: Thu, 24 May 2018 16:41:54 +0200 Message-ID: To: Laszlo Ersek Cc: edk2-devel-01 , Jordan Justen 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:41:56 -0000 Content-Type: text/plain; charset="UTF-8" On 24 May 2018 at 16:39, Laszlo Ersek 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