diff mbox

[edk2,01/10] MdeModulePkg/MnpDxe: Checked returned value of Snp->GetStatus()

Message ID B1FF2E9001CE9041BD10B825821D5BC504A0A75B@SHSMSX101.ccr.corp.intel.com
State New
Headers show

Commit Message

Fu, Siyuan Oct. 8, 2013, 8:51 a.m. UTC 
Hi, Pollack and Martin

Here calling Snp->GetStatus() without state code check is intend to update the MediaPresent field of EFI_SIMPLE_NETWORK_MODE to reflect the latest media status. Even though this call meet some errors we still need to return SnpModeData to the caller since SnpModeData contains many other fields other than MediaPresent. So I think move CopyMem into the conditional statements is not a good idea.

Siyuan
Best regards


-----Original Message-----
From: Reece R. Pollack [mailto:reece.pollack@linaro.org] 
Sent: Tuesday, October 08, 2013 3:40 AM
To: edk2-devel@lists.sourceforge.net
Cc: patches@linaro.org
Subject: [edk2] [PATCH 01/10] MdeModulePkg/MnpDxe: Checked returned value of Snp->GetStatus()

From: Olivier Martin <olivier.martin@arm.com>

... as explicitly said by the comment.

Signed-off-by: Olivier Martin <olivier.martin@arm.com>

Change-Id: Id9fd51dd5510d6acd04fe2c323a901248c4b85c3
---
 MdeModulePkg/Universal/Network/MnpDxe/MnpMain.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Ryan Harkin Oct. 9, 2013, 3:27 p.m. UTC | #1 
On 8 October 2013 09:51, Fu, Siyuan <siyuan.fu@intel.com> wrote:

> Hi, Pollack and Martin
>
> Here calling Snp->GetStatus() without state code check is intend to update
> the MediaPresent field of EFI_SIMPLE_NETWORK_MODE to reflect the latest
> media status. Even though this call meet some errors we still need to
> return SnpModeData to the caller since SnpModeData contains many other
> fields other than MediaPresent. So I think move CopyMem into the
> conditional statements is not a good idea.
>
>
Yes, this was already discussed when Olivier originally posted this patch.
I personally missed his re-posting of the patch with a new version, but I
see that it has not had a reply on the list anyway.

I think we should "withdraw" this patch submission and continue discussion
on Olivier's post.


Siyuan
> Best regards
>
>
> -----Original Message-----
> From: Reece R. Pollack [mailto:reece.pollack@linaro.org]
> Sent: Tuesday, October 08, 2013 3:40 AM
> To: edk2-devel@lists.sourceforge.net
> Cc: patches@linaro.org
> Subject: [edk2] [PATCH 01/10] MdeModulePkg/MnpDxe: Checked returned value
> of Snp->GetStatus()
>
> From: Olivier Martin <olivier.martin@arm.com>
>
> ... as explicitly said by the comment.
>
> Signed-off-by: Olivier Martin <olivier.martin@arm.com>
>
> Change-Id: Id9fd51dd5510d6acd04fe2c323a901248c4b85c3
> ---
>  MdeModulePkg/Universal/Network/MnpDxe/MnpMain.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/MdeModulePkg/Universal/Network/MnpDxe/MnpMain.c
> b/MdeModulePkg/Universal/Network/MnpDxe/MnpMain.c
> index 4c0f3dd..9cdbb43 100644
> --- a/MdeModulePkg/Universal/Network/MnpDxe/MnpMain.c
> +++ b/MdeModulePkg/Universal/Network/MnpDxe/MnpMain.c
> @@ -79,8 +79,10 @@ MnpGetModeData (
>      // Upon successful return of GetStatus(), the Snp->Mode->MediaPresent
>      // will be updated to reflect any change of media status
>      //
> -    Snp->GetStatus (Snp, &InterruptStatus, NULL);
> -    CopyMem (SnpModeData, Snp->Mode, sizeof (*SnpModeData));
> +    Status = Snp->GetStatus (Snp, &InterruptStatus, NULL);
> +    if (!EFI_ERROR (Status)) {
> +      CopyMem (SnpModeData, Snp->Mode, sizeof (*SnpModeData));
> +    }
>    }
>
>    if (!Instance->Configured) {
> --
> 1.7.10.4
>
>
>
> ------------------------------------------------------------------------------
> October Webinars: Code for Performance
> Free Intel webinars can help you accelerate application performance.
> Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most
> from
> the latest Intel processors and coprocessors. See abstracts and register >
> http://pubads.g.doubleclick.net/gampad/clk?id=60134071&iu=/4140/ostg.clktrk
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/edk2-devel
>
>
> ------------------------------------------------------------------------------
> October Webinars: Code for Performance
> Free Intel webinars can help you accelerate application performance.
> Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most
> from
> the latest Intel processors and coprocessors. See abstracts and register >
> http://pubads.g.doubleclick.net/gampad/clk?id=60134071&iu=/4140/ostg.clktrk
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/edk2-devel
>
diff mbox

Patch

diff --git a/MdeModulePkg/Universal/Network/MnpDxe/MnpMain.c b/MdeModulePkg/Universal/Network/MnpDxe/MnpMain.c
index 4c0f3dd..9cdbb43 100644
--- a/MdeModulePkg/Universal/Network/MnpDxe/MnpMain.c
+++ b/MdeModulePkg/Universal/Network/MnpDxe/MnpMain.c
@@ -79,8 +79,10 @@  MnpGetModeData (
     // Upon successful return of GetStatus(), the Snp->Mode->MediaPresent
     // will be updated to reflect any change of media status
     //
-    Snp->GetStatus (Snp, &InterruptStatus, NULL);
-    CopyMem (SnpModeData, Snp->Mode, sizeof (*SnpModeData));
+    Status = Snp->GetStatus (Snp, &InterruptStatus, NULL);
+    if (!EFI_ERROR (Status)) {
+      CopyMem (SnpModeData, Snp->Mode, sizeof (*SnpModeData));
+    }
   }
 
   if (!Instance->Configured) {