Thanks Sami, I managed to run Ecc properly now, so it already reported that to me
On Fri, 12 Feb 2021 at 17:29, Sami Mujawar <sami.muja...@arm.com> wrote: > > Hi Ilias, > > > +#ifndef OPTEE_RPMB_FVB_H > > +#define OPTEE_RPMB_FVB_H > Just remembered the include file guard should be 'OPTEE_RPMB_FVB_H_' in > Drivers/OpTeeRpmb/OpTeeRpmbFvb.h > > Regards, > > Sami Mujawar > > -----Original Message----- > From: Ilias Apalodimas <ilias.apalodi...@linaro.org> > Sent: 12 February 2021 01:38 PM > To: Sami Mujawar <sami.muja...@arm.com> > Cc: devel@edk2.groups.io; ard+tianoc...@kernel.org; sughosh.g...@linaro.org; > l...@nuviainc.com; nd <n...@arm.com> > Subject: Re: [PATCH 1/2 v4] Drivers/OpTeeRpmb: Add an OP-TEE backed RPMB > driver > > 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 (#71646): https://edk2.groups.io/g/devel/message/71646 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] -=-=-=-=-=-=-=-=-=-=-=-
