[PATCH] MdePkg SmmIoLib: Use NULL pointer check instead of useless Status check

[PATCH] MdePkg SmmIoLib: Use NULL pointer check instead of useless Status check

[Star Zeng]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=587

The Status check in "if (!EFI_ERROR (Status))" condition is useless,
it should be NULL pointer check. And this patch also fixes a typo
"continous" to "continuous".

Cc: Jiewen Yao <jiewen.yao@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Star Zeng <star.zeng@intel.com>
---
 MdePkg/Library/SmmIoLib/SmmIoLib.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/MdePkg/Library/SmmIoLib/SmmIoLib.c b/MdePkg/Library/SmmIoLib/SmmIoLib.c
index 181abb8e25df..f1cb0dace500 100644
--- a/MdePkg/Library/SmmIoLib/SmmIoLib.c
+++ b/MdePkg/Library/SmmIoLib/SmmIoLib.c
@@ -156,7 +156,7 @@ SmmIsMmioValid (
 }
 
 /**
-  Merge continous entries whose type is EfiGcdMemoryTypeMemoryMappedIo.
+  Merge continuous entries whose type is EfiGcdMemoryTypeMemoryMappedIo.
 
   @param[in, out]  GcdMemoryMap           A pointer to the buffer in which firmware places
                                           the current GCD memory map.
@@ -217,7 +217,8 @@ MergeGcdMmioEntry (
   @param[in] Interface  Points to the interface instance.
   @param[in] Handle     The handle on which the interface was installed.
 
-  @retval EFI_SUCCESS   Notification runs successfully.
+  @retval EFI_SUCCESS           Notification runs successfully.
+  @retval EFI_OUT_OF_RESOURCES  No enough resources to save GCD MMIO map.
 **/
 EFI_STATUS
 EFIAPI
@@ -237,10 +238,10 @@ SmmIoLibInternalEndOfDxeNotify (
     MergeGcdMmioEntry (MemSpaceMap, &NumberOfDescriptors);
 
     mSmmIoLibGcdMemSpace = AllocateCopyPool (NumberOfDescriptors * sizeof (EFI_GCD_MEMORY_SPACE_DESCRIPTOR), MemSpaceMap);
-    ASSERT_EFI_ERROR (Status);
-    if (EFI_ERROR (Status)) {
+    ASSERT (mSmmIoLibGcdMemSpace != NULL);
+    if (mSmmIoLibGcdMemSpace == NULL) {
       gBS->FreePool (MemSpaceMap);
-      return Status;
+      return EFI_OUT_OF_RESOURCES;
     }
 
     mSmmIoLibGcdMemNumberOfDesc = NumberOfDescriptors;
-- 
2.7.0.windows.1

Re: [PATCH] MdePkg SmmIoLib: Use NULL pointer check instead of useless Status check

[Yao, Jiewen]

Reviewed-by: Jiewen.yao@intel.com

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Star
> Zeng
> Sent: Monday, June 5, 2017 2:41 PM
> To: edk2-devel@lists.01.org
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: [edk2] [PATCH] MdePkg SmmIoLib: Use NULL pointer check instead of
> useless Status check
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=587
>
> The Status check in "if (!EFI_ERROR (Status))" condition is useless,
> it should be NULL pointer check. And this patch also fixes a typo
> "continous" to "continuous".
>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Star Zeng <star.zeng@intel.com>
> ---
>  MdePkg/Library/SmmIoLib/SmmIoLib.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/MdePkg/Library/SmmIoLib/SmmIoLib.c
> b/MdePkg/Library/SmmIoLib/SmmIoLib.c
> index 181abb8e25df..f1cb0dace500 100644
> --- a/MdePkg/Library/SmmIoLib/SmmIoLib.c
> +++ b/MdePkg/Library/SmmIoLib/SmmIoLib.c
> @@ -156,7 +156,7 @@ SmmIsMmioValid (
>  }
>
>  /**
> -  Merge continous entries whose type is
> EfiGcdMemoryTypeMemoryMappedIo.
> +  Merge continuous entries whose type is
> EfiGcdMemoryTypeMemoryMappedIo.
>
>    @param[in, out]  GcdMemoryMap           A pointer to the buffer in
> which firmware places
>                                            the current GCD memory map.
> @@ -217,7 +217,8 @@ MergeGcdMmioEntry (
>    @param[in] Interface  Points to the interface instance.
>    @param[in] Handle     The handle on which the interface was installed.
>
> -  @retval EFI_SUCCESS   Notification runs successfully.
> +  @retval EFI_SUCCESS           Notification runs successfully.
> +  @retval EFI_OUT_OF_RESOURCES  No enough resources to save GCD
> MMIO map.
>  **/
>  EFI_STATUS
>  EFIAPI
> @@ -237,10 +238,10 @@ SmmIoLibInternalEndOfDxeNotify (
>      MergeGcdMmioEntry (MemSpaceMap, &NumberOfDescriptors);
>
>      mSmmIoLibGcdMemSpace = AllocateCopyPool (NumberOfDescriptors *
> sizeof (EFI_GCD_MEMORY_SPACE_DESCRIPTOR), MemSpaceMap);
> -    ASSERT_EFI_ERROR (Status);
> -    if (EFI_ERROR (Status)) {
> +    ASSERT (mSmmIoLibGcdMemSpace != NULL);
> +    if (mSmmIoLibGcdMemSpace == NULL) {
>        gBS->FreePool (MemSpaceMap);
> -      return Status;
> +      return EFI_OUT_OF_RESOURCES;
>      }
>
>      mSmmIoLibGcdMemNumberOfDesc = NumberOfDescriptors;
> --
> 2.7.0.windows.1
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH] MdePkg SmmIoLib: Use NULL pointer check instead of useless Status check

[Udit Kumar]

Hi Star 

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Star
> Zeng
> Sent: Monday, June 05, 2017 12:11 PM
> To: edk2-devel@lists.01.org
> Cc: Jiewen Yao <jiewen.yao@intel.com>; Star Zeng <star.zeng@intel.com>
> Subject: [edk2] [PATCH] MdePkg SmmIoLib: Use NULL pointer check instead of
> useless Status check
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=587
> 
> The Status check in "if (!EFI_ERROR (Status))" condition is useless, it should be
> NULL pointer check. And this patch also fixes a typo "continous" to
> "continuous".
> 
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Star Zeng <star.zeng@intel.com>
> ---
>  MdePkg/Library/SmmIoLib/SmmIoLib.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/MdePkg/Library/SmmIoLib/SmmIoLib.c
> b/MdePkg/Library/SmmIoLib/SmmIoLib.c
> index 181abb8e25df..f1cb0dace500 100644
> --- a/MdePkg/Library/SmmIoLib/SmmIoLib.c
> +++ b/MdePkg/Library/SmmIoLib/SmmIoLib.c
> @@ -156,7 +156,7 @@ SmmIsMmioValid (
>  }
> 
>  /**
> -  Merge continous entries whose type is EfiGcdMemoryTypeMemoryMappedIo.
> +  Merge continuous entries whose type is
> EfiGcdMemoryTypeMemoryMappedIo.
> 
>    @param[in, out]  GcdMemoryMap           A pointer to the buffer in which
> firmware places
>                                            the current GCD memory map.
> @@ -217,7 +217,8 @@ MergeGcdMmioEntry (
>    @param[in] Interface  Points to the interface instance.
>    @param[in] Handle     The handle on which the interface was installed.
> 
> -  @retval EFI_SUCCESS   Notification runs successfully.
> +  @retval EFI_SUCCESS           Notification runs successfully.
> +  @retval EFI_OUT_OF_RESOURCES  No enough resources to save GCD MMIO
> map.
>  **/
>  EFI_STATUS
>  EFIAPI
> @@ -237,10 +238,10 @@ SmmIoLibInternalEndOfDxeNotify (
>      MergeGcdMmioEntry (MemSpaceMap, &NumberOfDescriptors);
> 
>      mSmmIoLibGcdMemSpace = AllocateCopyPool (NumberOfDescriptors *
> sizeof (EFI_GCD_MEMORY_SPACE_DESCRIPTOR), MemSpaceMap);
> -    ASSERT_EFI_ERROR (Status);
> -    if (EFI_ERROR (Status)) {
> +    ASSERT (mSmmIoLibGcdMemSpace != NULL);
> +    if (mSmmIoLibGcdMemSpace == NULL) {


If mSmmIoLibGcdMemSpace is NULL then if condition is not reachable. 
If not NULL then if condition will be false always, 
I think if condition is not needed.  


>        gBS->FreePool (MemSpaceMap);
> -      return Status;
> +      return EFI_OUT_OF_RESOURCES;
>      }
> 
>      mSmmIoLibGcdMemNumberOfDesc = NumberOfDescriptors;
> --
> 2.7.0.windows.1


Regards
Udit

> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH] MdePkg SmmIoLib: Use NULL pointer check instead of useless Status check

[Zeng, Star]

Hi Udit,

What does "If mSmmIoLibGcdMemSpace is NULL then if condition is not reachable." mean?

ASSERT_EFI_ERROR macro only effects at DEBUG mode, and the if condition is for error handling.


Thanks,
Star
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Udit Kumar
Sent: Tuesday, June 6, 2017 7:11 PM
To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
Cc: Yao, Jiewen <jiewen.yao@intel.com>
Subject: Re: [edk2] [PATCH] MdePkg SmmIoLib: Use NULL pointer check instead of useless Status check

Hi Star 

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of 
> Star Zeng
> Sent: Monday, June 05, 2017 12:11 PM
> To: edk2-devel@lists.01.org
> Cc: Jiewen Yao <jiewen.yao@intel.com>; Star Zeng <star.zeng@intel.com>
> Subject: [edk2] [PATCH] MdePkg SmmIoLib: Use NULL pointer check 
> instead of useless Status check
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=587
> 
> The Status check in "if (!EFI_ERROR (Status))" condition is useless, 
> it should be NULL pointer check. And this patch also fixes a typo 
> "continous" to "continuous".
> 
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Star Zeng <star.zeng@intel.com>
> ---
>  MdePkg/Library/SmmIoLib/SmmIoLib.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/MdePkg/Library/SmmIoLib/SmmIoLib.c
> b/MdePkg/Library/SmmIoLib/SmmIoLib.c
> index 181abb8e25df..f1cb0dace500 100644
> --- a/MdePkg/Library/SmmIoLib/SmmIoLib.c
> +++ b/MdePkg/Library/SmmIoLib/SmmIoLib.c
> @@ -156,7 +156,7 @@ SmmIsMmioValid (
>  }
> 
>  /**
> -  Merge continous entries whose type is EfiGcdMemoryTypeMemoryMappedIo.
> +  Merge continuous entries whose type is
> EfiGcdMemoryTypeMemoryMappedIo.
> 
>    @param[in, out]  GcdMemoryMap           A pointer to the buffer in which
> firmware places
>                                            the current GCD memory map.
> @@ -217,7 +217,8 @@ MergeGcdMmioEntry (
>    @param[in] Interface  Points to the interface instance.
>    @param[in] Handle     The handle on which the interface was installed.
> 
> -  @retval EFI_SUCCESS   Notification runs successfully.
> +  @retval EFI_SUCCESS           Notification runs successfully.
> +  @retval EFI_OUT_OF_RESOURCES  No enough resources to save GCD MMIO
> map.
>  **/
>  EFI_STATUS
>  EFIAPI
> @@ -237,10 +238,10 @@ SmmIoLibInternalEndOfDxeNotify (
>      MergeGcdMmioEntry (MemSpaceMap, &NumberOfDescriptors);
> 
>      mSmmIoLibGcdMemSpace = AllocateCopyPool (NumberOfDescriptors * 
> sizeof (EFI_GCD_MEMORY_SPACE_DESCRIPTOR), MemSpaceMap);
> -    ASSERT_EFI_ERROR (Status);
> -    if (EFI_ERROR (Status)) {
> +    ASSERT (mSmmIoLibGcdMemSpace != NULL);
> +    if (mSmmIoLibGcdMemSpace == NULL) {


If mSmmIoLibGcdMemSpace is NULL then if condition is not reachable. 
If not NULL then if condition will be false always, I think if condition is not needed.  


>        gBS->FreePool (MemSpaceMap);
> -      return Status;
> +      return EFI_OUT_OF_RESOURCES;
>      }
> 
>      mSmmIoLibGcdMemNumberOfDesc = NumberOfDescriptors;
> --
> 2.7.0.windows.1


Regards
Udit

> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

UEFI SCT2.5A cannot run with exception

[wang xiaofeng]

Hi ,
     Thanks all for your kind help.
I just update SCT from 2.4B to 2.5A,
WIth the same platform, 2.4B cannot be installed on EDK2 shell , we have to switch to EDK1 shell and then switch back. 2.4B can run on edk2shell.
2.5A aslo cannot be installed on EDK2 shell , we have to switch to EDK1 shell and then switch back. Unfortunatelly , the tool cannot run with an CPU exception. The attached file shows the serial log we see when the CPU exception occurs.
Any suggestion? Where is the SCT2.5 source code link ? I can try to debug the issue to see which code lead to the exception.
     \x16�&

Re: UEFI SCT2.5A cannot run with exception

[Jin, Eric]

Hi Xiaofeng,

Could you please resend the email to utwg@uefi.org for this question? I mean UEFI SCT is not the scope of the EDK2 community. 
Please don't forget to attach the log file. I don't see it or filtered by system. Thanks.

Best Regards
Eric

-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of wang xiaofeng
Sent: Wednesday, June 7, 2017 9:41 AM
To: edk2-devel@lists.01.org
Subject: [edk2] UEFI SCT2.5A cannot run with exception

Hi ,
     Thanks all for your kind help.
I just update SCT from 2.4B to 2.5A,
WIth the same platform, 2.4B cannot be installed on EDK2 shell , we have to switch to EDK1 shell and then switch back. 2.4B can run on edk2shell.
2.5A aslo cannot be installed on EDK2 shell , we have to switch to EDK1 shell and then switch back. Unfortunatelly , the tool cannot run with an CPU exception. The attached file shows the serial log we see when the CPU exception occurs.
Any suggestion? Where is the SCT2.5 source code link ? I can try to debug the issue to see which code lead to the exception.
     
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH] MdePkg SmmIoLib: Use NULL pointer check instead of useless Status check

[Udit Kumar]

Hi Star

I restricted myself into debug mode only , while looking at patch .

Regards
Udit

> -----Original Message-----
> From: Zeng, Star [mailto:star.zeng@intel.com]
> Sent: Wednesday, June 07, 2017 6:41 AM
> To: Udit Kumar <udit.kumar@nxp.com>; edk2-devel@lists.01.org
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: RE: [edk2] [PATCH] MdePkg SmmIoLib: Use NULL pointer check instead
> of useless Status check
> 
> Hi Udit,
> 
> What does "If mSmmIoLibGcdMemSpace is NULL then if condition is not
> reachable." mean?
> 
> ASSERT_EFI_ERROR macro only effects at DEBUG mode, and the if condition is
> for error handling.
> 
> 
> Thanks,
> Star
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Udit
> Kumar
> Sent: Tuesday, June 6, 2017 7:11 PM
> To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
> Cc: Yao, Jiewen <jiewen.yao@intel.com>
> Subject: Re: [edk2] [PATCH] MdePkg SmmIoLib: Use NULL pointer check instead
> of useless Status check
> 
> Hi Star
> 
> > -----Original Message-----
> > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> > Star Zeng
> > Sent: Monday, June 05, 2017 12:11 PM
> > To: edk2-devel@lists.01.org
> > Cc: Jiewen Yao <jiewen.yao@intel.com>; Star Zeng <star.zeng@intel.com>
> > Subject: [edk2] [PATCH] MdePkg SmmIoLib: Use NULL pointer check
> > instead of useless Status check
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=587
> >
> > The Status check in "if (!EFI_ERROR (Status))" condition is useless,
> > it should be NULL pointer check. And this patch also fixes a typo
> > "continous" to "continuous".
> >
> > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Star Zeng <star.zeng@intel.com>
> > ---
> >  MdePkg/Library/SmmIoLib/SmmIoLib.c | 11 ++++++-----
> >  1 file changed, 6 insertions(+), 5 deletions(-)
> >
> > diff --git a/MdePkg/Library/SmmIoLib/SmmIoLib.c
> > b/MdePkg/Library/SmmIoLib/SmmIoLib.c
> > index 181abb8e25df..f1cb0dace500 100644
> > --- a/MdePkg/Library/SmmIoLib/SmmIoLib.c
> > +++ b/MdePkg/Library/SmmIoLib/SmmIoLib.c
> > @@ -156,7 +156,7 @@ SmmIsMmioValid (
> >  }
> >
> >  /**
> > -  Merge continous entries whose type is
> EfiGcdMemoryTypeMemoryMappedIo.
> > +  Merge continuous entries whose type is
> > EfiGcdMemoryTypeMemoryMappedIo.
> >
> >    @param[in, out]  GcdMemoryMap           A pointer to the buffer in which
> > firmware places
> >                                            the current GCD memory map.
> > @@ -217,7 +217,8 @@ MergeGcdMmioEntry (
> >    @param[in] Interface  Points to the interface instance.
> >    @param[in] Handle     The handle on which the interface was installed.
> >
> > -  @retval EFI_SUCCESS   Notification runs successfully.
> > +  @retval EFI_SUCCESS           Notification runs successfully.
> > +  @retval EFI_OUT_OF_RESOURCES  No enough resources to save GCD
> MMIO
> > map.
> >  **/
> >  EFI_STATUS
> >  EFIAPI
> > @@ -237,10 +238,10 @@ SmmIoLibInternalEndOfDxeNotify (
> >      MergeGcdMmioEntry (MemSpaceMap, &NumberOfDescriptors);
> >
> >      mSmmIoLibGcdMemSpace = AllocateCopyPool (NumberOfDescriptors *
> > sizeof (EFI_GCD_MEMORY_SPACE_DESCRIPTOR), MemSpaceMap);
> > -    ASSERT_EFI_ERROR (Status);
> > -    if (EFI_ERROR (Status)) {
> > +    ASSERT (mSmmIoLibGcdMemSpace != NULL);
> > +    if (mSmmIoLibGcdMemSpace == NULL) {
> 
> 
> If mSmmIoLibGcdMemSpace is NULL then if condition is not reachable.
> If not NULL then if condition will be false always, I think if condition is not
> needed.
> 
> 
> >        gBS->FreePool (MemSpaceMap);
> > -      return Status;
> > +      return EFI_OUT_OF_RESOURCES;
> >      }
> >
> >      mSmmIoLibGcdMemNumberOfDesc = NumberOfDescriptors;
> > --
> > 2.7.0.windows.1
> 
> 
> Regards
> Udit
> 
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel