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