Hi Sami, On Fri, Feb 12, 2021 at 01:11:10PM +0000, Sami Mujawar wrote: > Hi Ilias, > > Apologies for the delay in reviewing this patch. >
No worries, thanks for the comments > Please find my response inline marked [SAMI]. > There are some coding standard issues remaining. I believe these are not > reported by Ecc. Yea Ecc or PatchCheck didn't catch any of these. > Also, InitializeFvAndVariableStoreHeaders() would need error handling for a > memory allocation failure. > > Regards, > > Sami Mujawar > > [...] > + Instance = INSTANCE_FROM_FVB_THIS(FvbProtocol); > [SAMI] Space needed before opening parenthesis. > See > https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/5_source_files/52_spacing#5-2-2-6-always-put-space-before-an-open-parenthesis > Please also check other places in this patch. Ok > [/SAMI] > > + // The Pcd is user defined, so make sure we don't overflow > > + if (Instance->MemBaseAddress > MAX_UINT64 - PcdGet32 > (PcdFlashNvStorageVariableSize)) { > > + return EFI_INVALID_PARAMETER; > > + } > > + > > + if (Instance->MemBaseAddress > MAX_UINT64 - PcdGet32 > (PcdFlashNvStorageVariableSize) - > > + PcdGet32 (PcdFlashNvStorageFtwWorkingSize)) { > > + return EFI_INVALID_PARAMETER; > > + } > > + > > + // Patch PCDs with the the correct values > > + PatchPcdSet64 (PcdFlashNvStorageVariableBase64, Instance->MemBaseAddress); > > + PatchPcdSet64 (PcdFlashNvStorageFtwWorkingBase64, Instance->MemBaseAddress > + > > + PcdGet32 (PcdFlashNvStorageVariableSize)); > [SAMI] Please see alignment guidelines described in EDKII coding standard at: > > https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/5_source_files/52_spacing#5-2-2-4-subsequent-lines-of-multi-line-function-calls-should-line-up-two-spaces-from-the-beginning-of-the-function-name > Please also fix this at other places in this patch. Ok > [/SAMI] > [...] > +[Pcd] > > + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64 > > + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize > > + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase64 > > + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize > > + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase64 > > + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize > [SAMI] Please sort in alphabetical order. > [/SAMI] Ok > [...] > > + > > +[Pcd] > > + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64 > > + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize > > + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase64 > > + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize > > + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase64 > > + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize > [SAMI] Please sort in alphabetical order. > [/SAMI] Ok > > + > > +OpTeeRpmbFvbSetAttributes ( > > + IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This, > > + IN OUT EFI_FVB_ATTRIBUTES_2 *Attributes > > + ) > > +{ > > + return EFI_SUCCESS; // ignore for now > [SAMI] Should this be EFI_UNSUPPORTED? If not, a comment may be helpful. Some of the drivers I looked were returning EFI_SUCCESS. Checking again I see some returning EFI_UNSUPPORTED as well, so I'll switch to that. > [/SAMI] > > +} [...] > > + *NumberOfBlocks = Instance->NBlocks - (UINTN) Lba; > [SAMI] There should be no space between (UINTN) and Lba. > Please see point 2 at > https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/v/release%2F2.20/3_quick_reference#3-2-3-formatting-horizontal-spacing > [/SAMI] Ok > > + *BlockSize = Instance->BlockSize; > > + [...] > +OpTeeRpmbFvbRead ( > > + IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This, > > + IN EFI_LBA Lba, > > + IN UINTN Offset, > > + IN OUT UINTN *NumBytes, > > + IN OUT UINT8 *Buffer > > + ) > > +{ > > + EFI_STATUS Status = EFI_SUCCESS; > > + MEM_INSTANCE *Instance; > > + VOID *Base; > > + > > + Instance = INSTANCE_FROM_FVB_THIS(This); > > + if (!Instance->Initialized) { > > + Instance->Initialize (Instance); > [SAMI] Should the status be checked here and returned? > Or is the assumption that Initialize will always succeed. If so, > - a comment would be helpful. > - the Status variable here is redundant. > Same comment for OpTeeRpmbFvbWrite(). No you are right 'Status =' is missing, I'll add that. > [/SAMI] > + } > > + > > + Base = (VOID *)Instance->MemBaseAddress + Lba * Instance->BlockSize + > Offset; > [SAMI] Use of parentheses is encouraged. See > https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/5_source_files/52_spacing#5-2-2-10-use-extra-parentheses-rather-than-depending-on-in-depth-knowledge-of-the-order-of-precedence-of-c > [/SAMI] ok [...] > > + } > > + NumBytes = NumLba * Instance->BlockSize; > > + Base = (VOID *)Instance->MemBaseAddress + Start * Instance->BlockSize; > > + Buf = AllocatePool(NumLba * Instance->BlockSize); > > + if (!Buf) { > [SAMI] if (Buf == NULL) { > [/SAMI] Ok > > + return EFI_DEVICE_ERROR; > [...] > +EFI_STATUS > > +InitializeFvAndVariableStoreHeaders ( > > + MEM_INSTANCE *Instance > > + ) > > +{ > > + EFI_FIRMWARE_VOLUME_HEADER *FirmwareVolumeHeader; > > + VARIABLE_STORE_HEADER *VariableStoreHeader; > > + EFI_STATUS Status = EFI_SUCCESS; > > + UINTN HeadersLength; > > + VOID* Headers; > > + > > + HeadersLength = sizeof(EFI_FIRMWARE_VOLUME_HEADER) + > > + sizeof(EFI_FV_BLOCK_MAP_ENTRY) + > > + sizeof(VARIABLE_STORE_HEADER); > > + Headers = AllocateZeroPool(HeadersLength); > [SAMI] Error handling for allocation failure is needed here. > [/SAMI] Yes missed that > [...] > + // > [SAMI] ASSERT (Addr != NULL); could be removed from the line above an instead > ASSERT (0); could be added here. > [/SAMI] Ok > + return EFI_OUT_OF_RESOURCES; > + I'll fix those and send a v5. Thanks! /Ilias -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#71643): https://edk2.groups.io/g/devel/message/71643 Mute This Topic: https://groups.io/mt/80350216/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
