Hi Sahil,

Thank you for this patch.

I have a minor suggession marked inline as [SAMI].

Otherwise this patch looks good to me.

Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>

Regards,

Sami Mujawar

On 23/05/2024 11:55 am, Sahil Kaushal wrote:
> From: sahil<sahil@arm.com>
>
> This patch adds error_handler1 and error_handler2 labels in
> NorFlashCreateInstance() function to handle the cleanup.
>
> error_handler1: Frees just the Instance structure as the
> ShadowBuffer is not allocated yet.
>
> error_handler2: Frees both Instance and Instance->ShadowBuffer.
>
> Signed-off-by: sahil<sahil@arm.com>
> ---
>   Platform/ARM/Drivers/NorFlashDxe/NorFlashDxe.c          | 18 +++++++++++++-----
>   Platform/ARM/Drivers/NorFlashDxe/NorFlashStandaloneMm.c | 19 ++++++++++++++-----
>   2 files changed, 27 insertions(+), 10 deletions(-)
>
> diff --git a/Platform/ARM/Drivers/NorFlashDxe/NorFlashDxe.c b/Platform/ARM/Drivers/NorFlashDxe/NorFlashDxe.c
> index e01b05d91978..fd47bd9e4c63 100644
> --- a/Platform/ARM/Drivers/NorFlashDxe/NorFlashDxe.c
> +++ b/Platform/ARM/Drivers/NorFlashDxe/NorFlashDxe.c
> @@ -135,7 +135,8 @@ NorFlashCreateInstance (
>   
>
>     Instance->ShadowBuffer = AllocateRuntimePool (BlockSize);
>
>     if (Instance->ShadowBuffer == NULL) {
>
> -    return EFI_OUT_OF_RESOURCES;
>
> +    Status = EFI_OUT_OF_RESOURCES;
>
> +    goto error_handler1;
>
>     }
>
>   
>
>     if (SupportFvb) {
>
> @@ -152,8 +153,7 @@ NorFlashCreateInstance (
>                       NULL
>
>                       );
>
>       if (EFI_ERROR (Status)) {
>
> -      FreePool (Instance);
>
> -      return Status;
>
> +      goto error_handler2;
>
>       }
>
>     } else {
>
>       Status = gBS->InstallMultipleProtocolInterfaces (
>
> @@ -167,12 +167,20 @@ NorFlashCreateInstance (
>                       NULL
>
>                       );
>
>       if (EFI_ERROR (Status)) {
>
> -      FreePool (Instance);
>
> -      return Status;
>
> +      goto error_handler2;
>
>       }
>
>     }
>
>   
>
>     *NorFlashInstance = Instance;
[SNIP]
> +  return EFI_SUCCESS;
>
> +
> +error_handler1:
>
> +  FreePool (Instance);
>
> +  return Status;
>
> +
>
> +error_handler2:
>
> +  FreePool (Instance->ShadowBuffer);
>
> +  FreePool (Instance);
>
>     return Status;

[/SNIP]

[SAMI] I think the above code can be simplified as below:

---

+ return Status;
+

+error_handler2:

+  FreePool (Instance->ShadowBuffer);

+error_handler2:

+  FreePool (Instance);

    return Status;
---

A similar change is reuired later in this patch below.

If you agree, I will fix this up before merging the patch.

[/SAMI]


>   }
>
>   
>
> diff --git a/Platform/ARM/Drivers/NorFlashDxe/NorFlashStandaloneMm.c b/Platform/ARM/Drivers/NorFlashDxe/NorFlashStandaloneMm.c
> index 16fe3762e125..17dfe26627dd 100644
> --- a/Platform/ARM/Drivers/NorFlashDxe/NorFlashStandaloneMm.c
> +++ b/Platform/ARM/Drivers/NorFlashDxe/NorFlashStandaloneMm.c
> @@ -129,7 +129,8 @@ NorFlashCreateInstance (
>   
>
>     Instance->ShadowBuffer = AllocateRuntimePool (BlockSize);
>
>     if (Instance->ShadowBuffer == NULL) {
>
> -    return EFI_OUT_OF_RESOURCES;
>
> +    Status = EFI_OUT_OF_RESOURCES;
>
> +    goto error_handler1;
>
>     }
>
>   
>
>     if (SupportFvb) {
>
> @@ -142,16 +143,24 @@ NorFlashCreateInstance (
>                         &Instance->FvbProtocol
>
>                         );
>
>       if (EFI_ERROR (Status)) {
>
> -      FreePool (Instance);
>
> -      return Status;
>
> +      goto error_handler2;
>
>       }
>
>     } else {
>
>       DEBUG ((DEBUG_ERROR, "standalone MM NOR Flash driver only support FVB.\n"));
>
> -    FreePool (Instance);
>
> -    return EFI_UNSUPPORTED;
>
> +    Status = EFI_UNSUPPORTED;
>
> +    goto error_handler2;
>
>     }
>
>   
>
>     *NorFlashInstance = Instance;
>
> +  return EFI_SUCCESS;
>
> +
>
> +error_handler1:
>
> +  FreePool (Instance);
>
> +  return Status;
>
> +
>
> +error_handler2:
>
> +  FreePool (Instance->ShadowBuffer);
>
> +  FreePool (Instance);
>
>     return Status;
>
>   }
>
>   
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119164): https://edk2.groups.io/g/devel/message/119164
Mute This Topic: https://groups.io/mt/106260149/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-