Re: [edk2-devel] [PATCH RESEND edk2-platforms][PATCH V2 09/14] Platform/ARM: Add optional provision to fetch and print NOR Flash info

["Sami Mujawar" <sami.mujawar@arm.com>]

[-- Attachment #1: Type: text/plain, Size: 13814 bytes --]

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<sahil@arm.com>
>
> 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<sahil@arm.com>
> ---
>   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 <Library/UefiLib.h>
>
>   #include <Library/BaseMemoryLib.h>
>
>   #include <Library/MemoryAllocationLib.h>
>
> +#include <Library/NorFlashInfoLib.h>
>
>   #include <Library/UefiBootServicesTableLib.h>
>
>   #include <Library/PcdLib.h>
>
>   #include <Library/HobLib.h>
>
> @@ -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.

<SNIP>

   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);
   }

</SNIP>



>     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 <Library/BaseMemoryLib.h>
>
>   #include <Library/MemoryAllocationLib.h>
>
>   #include <Library/MmServicesTableLib.h>
>
> +#include <Library/NorFlashInfoLib.h>
>
>   
>
>   #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]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 16425 bytes --]