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.muja...@arm.com>
Regards,
Sami Mujawar
On 23/05/2024 11:55 am, Sahil Kaushal wrote:
From: sahil<sa...@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<sa...@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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
