Hi Sahil, Thank you for this patch. Please find my feedback inline marked [SAMI]. Regards, Sami Mujawarnd On 23/04/2024 06:56 am, Sahil Kaushal wrote: > From: sahil > > This patch adds an optional functionality in NorFlashDxe to fetch and > print NOR Flash information from NorFlashInfoLib using its JEDEC ID. > > NOR Flash libraries will implement a function "NorFlashReadID" which > will fetch and return JEDEC ID. This JEDEC ID can be then used to > print NOR Flash info using NorFlashInfoLib. If this functionality is [SAMI] Can you explain how this information is useful, please? Is it just for debugging or it can be used for some other purpose. If it is just for printing the information, then maybe the above sentence could be silghtly modified, e.g. This JEDEC ID can be then printed along with the NOR Flash info by NorFlashInfoLib. [/SAMI] > not needed then the function can just return EFI_UNSUPPORTED. > > Signed-off-by: sahil > --- > Platform/ARM/SgiPkg/SgiPlatform.dsc.inc | 2 ++ > Platform/ARM/SgiPkg/SgiPlatformMm.dsc.inc | 2 ++ > Platform/ARM/VExpressPkg/ArmVExpress.dsc.inc | 2 ++ > Platform/ARM/JunoPkg/ArmJuno.dsc | 2 ++ > Platform/ARM/VExpressPkg/PlatformStandaloneMm.dsc | 2 ++ > Platform/ARM/Drivers/NorFlashDxe/NorFlashDxe.inf | 1 + > Platform/ARM/Drivers/NorFlashDxe/NorFlashStandaloneMm.inf | 1 + > Platform/ARM/Include/Library/NorFlashDeviceLib.h | 6 ++++++ > Platform/ARM/Drivers/NorFlashDxe/NorFlashDxe.c | 19 +++++++++++++++++++ > Platform/ARM/Drivers/NorFlashDxe/NorFlashStandaloneMm.c | 19 +++++++++++++++++++ > Platform/ARM/Library/P30NorFlashDeviceLib/P30NorFlashDeviceLib.c | 18 ++++++++++++++++++ > 11 files changed, 74 insertions(+) > > diff --git a/Platform/ARM/SgiPkg/SgiPlatform.dsc.inc b/Platform/ARM/SgiPkg/SgiPlatform.dsc.inc > index 3dcf422eab4b..aef7cba5449e 100644 > --- a/Platform/ARM/SgiPkg/SgiPlatform.dsc.inc > +++ b/Platform/ARM/SgiPkg/SgiPlatform.dsc.inc > @@ -36,6 +36,8 @@ > LcdPlatformLib|Platform/ARM/SgiPkg/Library/HdLcdArmSgiLib/HdLcdArmSgiLib.inf > > NorFlashDeviceLib|Platform/ARM/Library/P30NorFlashDeviceLib/P30NorFlashDeviceLib.inf > > NorFlashPlatformLib|Platform/ARM/SgiPkg/Library/NorFlashLib/NorFlashLib.inf > > + # NOR flash support > > + NorFlashInfoLib|EmbeddedPkg/Library/NorFlashInfoLib/NorFlashInfoLib.inf > > HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf > > ResetSystemLib|ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.inf > > TimerLib|ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.inf > > diff --git a/Platform/ARM/SgiPkg/SgiPlatformMm.dsc.inc b/Platform/ARM/SgiPkg/SgiPlatformMm.dsc.inc > index ab0e2a957a1b..02d684adaebd 100644 > --- a/Platform/ARM/SgiPkg/SgiPlatformMm.dsc.inc > +++ b/Platform/ARM/SgiPkg/SgiPlatformMm.dsc.inc > @@ -65,6 +65,8 @@ > IntrinsicLib|CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf > > NorFlashDeviceLib|Platform/ARM/Library/P30NorFlashDeviceLib/P30NorFlashDeviceLib.inf > > NorFlashPlatformLib|Platform/ARM/SgiPkg/Library/NorFlashLib/StandaloneMmNorFlashLib.inf > > + # NOR flash support > > + NorFlashInfoLib|EmbeddedPkg/Library/NorFlashInfoLib/NorFlashInfoLib.inf > > OpensslLib|CryptoPkg/Library/OpensslLib/OpensslLib.inf > > RngLib|MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.inf > > PlatformSecureLib|SecurityPkg/Library/PlatformSecureLibNull/PlatformSecureLibNull.inf > > diff --git a/Platform/ARM/VExpressPkg/ArmVExpress.dsc.inc b/Platform/ARM/VExpressPkg/ArmVExpress.dsc.inc > index 70ff049d3248..4e208c539a88 100644 > --- a/Platform/ARM/VExpressPkg/ArmVExpress.dsc.inc > +++ b/Platform/ARM/VExpressPkg/ArmVExpress.dsc.inc > @@ -95,6 +95,8 @@ > ArmPlatformSysConfigLib|Platform/ARM/VExpressPkg/Library/ArmVExpressSysConfigLib/ArmVExpressSysConfigLib.inf > > NorFlashDeviceLib|Platform/ARM/Library/P30NorFlashDeviceLib/P30NorFlashDeviceLib.inf > > NorFlashPlatformLib|Platform/ARM/VExpressPkg/Library/NorFlashArmVExpressLib/NorFlashArmVExpressLib.inf > > + # NOR flash support > > + NorFlashInfoLib|EmbeddedPkg/Library/NorFlashInfoLib/NorFlashInfoLib.inf > > ResetSystemLib|ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.inf > > > > # ARM PL031 RTC Driver > > diff --git a/Platform/ARM/JunoPkg/ArmJuno.dsc b/Platform/ARM/JunoPkg/ArmJuno.dsc > index 81d2cbe4359f..946b8680c8c2 100644 > --- a/Platform/ARM/JunoPkg/ArmJuno.dsc > +++ b/Platform/ARM/JunoPkg/ArmJuno.dsc > @@ -42,6 +42,8 @@ > > > NorFlashDeviceLib|Platform/ARM/Library/P30NorFlashDeviceLib/P30NorFlashDeviceLib.inf > > NorFlashPlatformLib|Platform/ARM/JunoPkg/Library/NorFlashJunoLib/NorFlashJunoLib.inf > > + # NOR flash support > > + NorFlashInfoLib|EmbeddedPkg/Library/NorFlashInfoLib/NorFlashInfoLib.inf > > > > CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibNull/DxeCapsuleLibNull.inf > > CustomizedDisplayLib|MdeModulePkg/Library/CustomizedDisplayLib/CustomizedDisplayLib.inf > > diff --git a/Platform/ARM/VExpressPkg/PlatformStandaloneMm.dsc b/Platform/ARM/VExpressPkg/PlatformStandaloneMm.dsc > index a5805da49c92..ee71bbb1fc09 100644 > --- a/Platform/ARM/VExpressPkg/PlatformStandaloneMm.dsc > +++ b/Platform/ARM/VExpressPkg/PlatformStandaloneMm.dsc > @@ -102,6 +102,8 @@ > !if $(ENABLE_UEFI_SECURE_VARIABLE) == TRUE > > NorFlashDeviceLib|Platform/ARM/Library/P30NorFlashDeviceLib/P30NorFlashDeviceLib.inf > > NorFlashPlatformLib|Platform/ARM/VExpressPkg/Library/NorFlashArmVExpressLib/NorFlashStMmLib.inf > > + # NOR flash support [SAMI] Nit. Should the above comment be 'NOR flash identification support'? Same comment for other places in this patch. > > + NorFlashInfoLib|EmbeddedPkg/Library/NorFlashInfoLib/NorFlashInfoLib.inf > > VarCheckLib|MdeModulePkg/Library/VarCheckLib/VarCheckLib.inf > > VariableFlashInfoLib|MdeModulePkg/Library/BaseVariableFlashInfoLib/BaseVariableFlashInfoLib.inf > > AuthVariableLib|SecurityPkg/Library/AuthVariableLib/AuthVariableLib.inf > > diff --git a/Platform/ARM/Drivers/NorFlashDxe/NorFlashDxe.inf b/Platform/ARM/Drivers/NorFlashDxe/NorFlashDxe.inf > index 6522968d6c5a..4ab4d6a26926 100644 > --- a/Platform/ARM/Drivers/NorFlashDxe/NorFlashDxe.inf > +++ b/Platform/ARM/Drivers/NorFlashDxe/NorFlashDxe.inf > @@ -35,6 +35,7 @@ > HobLib > > IoLib > > NorFlashDeviceLib > > + NorFlashInfoLib > > NorFlashPlatformLib > > UefiBootServicesTableLib > > UefiDriverEntryPoint > > diff --git a/Platform/ARM/Drivers/NorFlashDxe/NorFlashStandaloneMm.inf b/Platform/ARM/Drivers/NorFlashDxe/NorFlashStandaloneMm.inf > index eb86d423f106..8b583f77d927 100644 > --- a/Platform/ARM/Drivers/NorFlashDxe/NorFlashStandaloneMm.inf > +++ b/Platform/ARM/Drivers/NorFlashDxe/NorFlashStandaloneMm.inf > @@ -37,6 +37,7 @@ > MemoryAllocationLib > > MmServicesTableLib > > NorFlashDeviceLib > > + NorFlashInfoLib > > NorFlashPlatformLib > > StandaloneMmDriverEntryPoint > > > > diff --git a/Platform/ARM/Include/Library/NorFlashDeviceLib.h b/Platform/ARM/Include/Library/NorFlashDeviceLib.h > index 29b8b8901525..1e61f7c935ae 100644 > --- a/Platform/ARM/Include/Library/NorFlashDeviceLib.h > +++ b/Platform/ARM/Include/Library/NorFlashDeviceLib.h > @@ -154,4 +154,10 @@ NorFlashUnlock ( > IN EFI_TPL OriginalTPL > > ); > > > > +EFI_STATUS > > +NorFlashReadID ( > > + IN NOR_FLASH_INSTANCE *Instance, > > + OUT UINT8 *JedecId // Maximum length of JedecId can be upto 6 bytes. > > + ); [SAMI] You have the documentation header already in P30NorFlashDeviceLib.. Can you put that in the header file here as well, please? > > + > > #endif /* NOR_FLASH_DEVICE_LIB_H_ */ > > diff --git a/Platform/ARM/Drivers/NorFlashDxe/NorFlashDxe.c b/Platform/ARM/Drivers/NorFlashDxe/NorFlashDxe.c > index f5c0dadf84e0..1084f4731ce4 100644 > --- a/Platform/ARM/Drivers/NorFlashDxe/NorFlashDxe.c > +++ b/Platform/ARM/Drivers/NorFlashDxe/NorFlashDxe.c > @@ -9,6 +9,7 @@ > #include > > #include > > #include > > +#include > > #include > > #include > > #include > > @@ -112,6 +113,8 @@ NorFlashCreateInstance ( > { > > EFI_STATUS Status; > > NOR_FLASH_INSTANCE *Instance; > > + NOR_FLASH_INFO *FlashInfo; > > + UINT8 JedecId[6]; > > > > ASSERT (NorFlashInstance != NULL); > > > > @@ -138,6 +141,22 @@ NorFlashCreateInstance ( > return EFI_OUT_OF_RESOURCES; > > } > > > > + Status = NorFlashReadID (Instance, JedecId); > > + if (EFI_ERROR (Status) && (Status != EFI_UNSUPPORTED)) { > > + FreePool (Instance); [SAMI] What about freeing the Instance->ShadowBuffer buffer? This issue appears to be present even before your changes. Can you add another patch before patch 9/14 to fix the memory leak in this funciton, please? I think it may be better to handle the error at the end of the function and cleanup the resources allocated, i.e. implement an exit_handler<1,2> label  as needed that frees up the resources and add goto exit_handler<1,2> from whereever the error occurs, setting up the Status appropriately to be returned at the end of the function. > > + return Status; > > + } > > + > > + if (Status == EFI_SUCCESS) { > > + Status = NorFlashGetInfo (JedecId, &FlashInfo, TRUE); [SAMI] It looks like the memory allocated for FlashInfo is leaked. Also do you need the memory allocation to be rom the runtime pool? Can you check, please? > > + if (EFI_ERROR (Status)) { > > + FreePool (Instance); [SAMI] Same comment as above. > > + return Status; > > + } > > + > > + NorFlashPrintInfo (FlashInfo); > > + } > > + [SAMI] I think the above code can be slightly optimised, see snippet below.   Status = NorFlashReadID (Instance, JedecId);   if (EFI_ERROR (Status)) {     if (Status != EFI_UNSUPPORTED) {          goto error_handler1;      }   } else {     Status = NorFlashGetInfo (JedecId, &FlashInfo, TRUE);     if (EFI_ERROR (Status)) { [SAMI] Question? How critical is this error? Can it not be treated as JedecID not supported, and the flash driver can proceed without printing the flash info? In other words, is it worth adding a warning message that the flash info cannot be read and proceed with loading the driver?         goto error_handler1;     }     NorFlashPrintInfo (FlashInfo);   } > if (SupportFvb) { > > NorFlashFvbInitialize (Instance); > > > > diff --git a/Platform/ARM/Drivers/NorFlashDxe/NorFlashStandaloneMm.c b/Platform/ARM/Drivers/NorFlashDxe/NorFlashStandaloneMm.c > index ef9bed37d140..a621b29f34e2 100644 > --- a/Platform/ARM/Drivers/NorFlashDxe/NorFlashStandaloneMm.c > +++ b/Platform/ARM/Drivers/NorFlashDxe/NorFlashStandaloneMm.c > @@ -10,6 +10,7 @@ > #include > > #include > > #include > > +#include > > > > #include "NorFlashCommon.h" > > > > @@ -106,6 +107,8 @@ NorFlashCreateInstance ( > { > > EFI_STATUS Status; > > NOR_FLASH_INSTANCE *Instance; > > + NOR_FLASH_INFO *FlashInfo; > > + UINT8 JedecId[6]; > > > > ASSERT (NorFlashInstance != NULL); > > > > @@ -132,6 +135,22 @@ NorFlashCreateInstance ( > return EFI_OUT_OF_RESOURCES; > > } > > > > + Status = NorFlashReadID (Instance, JedecId); > > + if (EFI_ERROR (Status) && (Status != EFI_UNSUPPORTED)) { > > + FreePool (Instance); > > + return Status; > > + } > > + [SAMI] Same comments as in NorFlashDxe. > > + if (Status == EFI_SUCCESS) { > > + Status = NorFlashGetInfo (JedecId, &FlashInfo, TRUE); > > + if (EFI_ERROR (Status)) { > > + FreePool (Instance); > > + return Status; > > + } > > + > > + NorFlashPrintInfo (FlashInfo); > > + } > > + > > if (SupportFvb) { > > NorFlashFvbInitialize (Instance); > > > > diff --git a/Platform/ARM/Library/P30NorFlashDeviceLib/P30NorFlashDeviceLib.c b/Platform/ARM/Library/P30NorFlashDeviceLib/P30NorFlashDeviceLib.c > index d68e237e2e26..5f8f137482ee 100644 > --- a/Platform/ARM/Library/P30NorFlashDeviceLib/P30NorFlashDeviceLib.c > +++ b/Platform/ARM/Library/P30NorFlashDeviceLib/P30NorFlashDeviceLib.c > @@ -947,3 +947,21 @@ NorFlashReset ( > SEND_NOR_COMMAND (Instance->DeviceBaseAddress, 0, P30_CMD_READ_ARRAY); > > return EFI_SUCCESS; > > } > > + > > +/** > > + Read JEDEC ID of NOR flash device. > > + > > + @param[in] Instance NOR flash Instance of variable store region. > > + @param[out] JedecId JEDEC ID of NOR flash device. > > + Maximum length of JedecId can be upto 6 bytes. > > + > > + @retval EFI_UNSUPPORTED JEDEC ID retrievel not implemented. > > +**/ > > +EFI_STATUS > > +NorFlashReadID ( [SAMI] Nit.  Rename NorFlashReadID() => NorFlashReadId(). > > + IN NOR_FLASH_INSTANCE *Instance, > > + OUT UINT8 *JedecId > > + ) > > +{ > > + return EFI_UNSUPPORTED; > > +} > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#118965): https://edk2.groups.io/g/devel/message/118965 Mute This Topic: https://groups.io/mt/105690944/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=-