diff mbox

[edk2,02/10] MdeModulePkg//ArpDxe: Retrieved SnpMode only after configuring Snp

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

Commit Message

Fu, Siyuan Oct. 9, 2013, 1:53 a.m. UTC
Hi, Pollack

I don't agree with the opinion that the Snp.Mode can't be accessed before Snp->Initialize() is called. According to UEFI spec, all the fields in Snp.Mode structure must be discovered during driver initialization, per my understanding the words "driver initialization" mean the driver binding start function, not Snp->Initialize() interface. And in EDKII implementation, all these parameters in Snp.Mode are prepared by calling PXE_OPCODE_GET_INIT_INFO in SimpleNetworkDriverStart() function before SNP protocol is installed into the protocol database, so they are valid to use once SNP protocol is produced.

Fu, 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 02/10] MdeModulePkg//ArpDxe: Retrieved SnpMode only after configuring Snp

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

When Arp driver starts (with ArpDriverBindingStart()), its service will be created and the Mnp child configured (ArpService->Mnp->Configure() called in ArpCreateService()).

It is only at this time the Snp protocol will be initialized (at the end of MnpStart()).
So, a valid SnpMode could not be expected prior to ArpService->Mnp->Configure().

Signed-off-by: Olivier Martin <olivier.martin@arm.com>
---
 MdeModulePkg/Universal/Network/ArpDxe/ArpDriver.c |   33 +++++++++++----------
 1 file changed, 17 insertions(+), 16 deletions(-)  mode change 100644 => 100755 MdeModulePkg/Universal/Network/ArpDxe/ArpDriver.c

   }
 
   //
+  // Get the underlayer Snp mode data. Must do this after MNP 
+ configuration else some parameters  // (e.g. current address) may not 
+ be set  //  Status = ArpService->Mnp->GetModeData (ArpService->Mnp, 
+ NULL, &ArpService->SnpMode);  if ((Status != EFI_NOT_STARTED) && 
+ EFI_ERROR (Status)) {
+    goto ERROR_EXIT;
+  }
+
+  if (ArpService->SnpMode.IfType != NET_IFTYPE_ETHERNET) {
+    //
+    // Only support the ethernet.
+    //
+    Status = EFI_UNSUPPORTED;
+    goto ERROR_EXIT;
+  }
+
+  //
   // Create the event used in the RxToken.
   //
   Status = gBS->CreateEvent (
--
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

Comments

Ryan Harkin Oct. 9, 2013, 3:29 p.m. UTC | #1
On 9 October 2013 02:53, Fu, Siyuan <siyuan.fu@intel.com> wrote:

> Hi, Pollack
>
> I don't agree with the opinion that the Snp.Mode can't be accessed before
> Snp->Initialize() is called. According to UEFI spec, all the fields in
> Snp.Mode structure must be discovered during driver initialization, per my
> understanding the words "driver initialization" mean the driver binding
> start function, not Snp->Initialize() interface. And in EDKII
> implementation, all these parameters in Snp.Mode are prepared by calling
> PXE_OPCODE_GET_INIT_INFO in SimpleNetworkDriverStart() function before SNP
> protocol is installed into the protocol database, so they are valid to use
> once SNP protocol is produced.
>
>
As with the other patch, 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.



> Fu, 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 02/10] MdeModulePkg//ArpDxe: Retrieved SnpMode only
> after configuring Snp
>
> From: Olivier Martin <olivier.martin@arm.com>
>
> When Arp driver starts (with ArpDriverBindingStart()), its service will be
> created and the Mnp child configured (ArpService->Mnp->Configure() called
> in ArpCreateService()).
>
> It is only at this time the Snp protocol will be initialized (at the end
> of MnpStart()).
> So, a valid SnpMode could not be expected prior to
> ArpService->Mnp->Configure().
>
> Signed-off-by: Olivier Martin <olivier.martin@arm.com>
> ---
>  MdeModulePkg/Universal/Network/ArpDxe/ArpDriver.c |   33
> +++++++++++----------
>  1 file changed, 17 insertions(+), 16 deletions(-)  mode change 100644 =>
> 100755 MdeModulePkg/Universal/Network/ArpDxe/ArpDriver.c
>
> diff --git a/MdeModulePkg/Universal/Network/ArpDxe/ArpDriver.c
> b/MdeModulePkg/Universal/Network/ArpDxe/ArpDriver.c
> old mode 100644
> new mode 100755
> index 81ddd62..5cf717f
> --- a/MdeModulePkg/Universal/Network/ArpDxe/ArpDriver.c
> +++ b/MdeModulePkg/Universal/Network/ArpDxe/ArpDriver.c
> @@ -103,22 +103,6 @@ ArpCreateService (
>    }
>
>    //
> -  // Get the underlayer Snp mode data.
> -  //
> -  Status = ArpService->Mnp->GetModeData (ArpService->Mnp, NULL,
> &ArpService->SnpMode);
> -  if ((Status != EFI_NOT_STARTED) && EFI_ERROR (Status)) {
> -    goto ERROR_EXIT;
> -  }
> -
> -  if (ArpService->SnpMode.IfType != NET_IFTYPE_ETHERNET) {
> -    //
> -    // Only support the ethernet.
> -    //
> -    Status = EFI_UNSUPPORTED;
> -    goto ERROR_EXIT;
> -  }
> -
> -  //
>    // Set the Mnp config parameters.
>    //
>    ArpService->MnpConfigData.ReceivedQueueTimeoutValue = 0; @@ -141,6
> +125,23 @@ ArpCreateService (
>    }
>
>    //
> +  // Get the underlayer Snp mode data. Must do this after MNP
> + configuration else some parameters  // (e.g. current address) may not
> + be set  //  Status = ArpService->Mnp->GetModeData (ArpService->Mnp,
> + NULL, &ArpService->SnpMode);  if ((Status != EFI_NOT_STARTED) &&
> + EFI_ERROR (Status)) {
> +    goto ERROR_EXIT;
> +  }
> +
> +  if (ArpService->SnpMode.IfType != NET_IFTYPE_ETHERNET) {
> +    //
> +    // Only support the ethernet.
> +    //
> +    Status = EFI_UNSUPPORTED;
> +    goto ERROR_EXIT;
> +  }
> +
> +  //
>    // Create the event used in the RxToken.
>    //
>    Status = gBS->CreateEvent (
> --
> 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/ArpDxe/ArpDriver.c b/MdeModulePkg/Universal/Network/ArpDxe/ArpDriver.c
old mode 100644
new mode 100755
index 81ddd62..5cf717f
--- a/MdeModulePkg/Universal/Network/ArpDxe/ArpDriver.c
+++ b/MdeModulePkg/Universal/Network/ArpDxe/ArpDriver.c
@@ -103,22 +103,6 @@  ArpCreateService (
   }
 
   //
-  // Get the underlayer Snp mode data.
-  //
-  Status = ArpService->Mnp->GetModeData (ArpService->Mnp, NULL, &ArpService->SnpMode);
-  if ((Status != EFI_NOT_STARTED) && EFI_ERROR (Status)) {
-    goto ERROR_EXIT;
-  }
-
-  if (ArpService->SnpMode.IfType != NET_IFTYPE_ETHERNET) {
-    //
-    // Only support the ethernet.
-    //
-    Status = EFI_UNSUPPORTED;
-    goto ERROR_EXIT;
-  }
-
-  //
   // Set the Mnp config parameters.
   //
   ArpService->MnpConfigData.ReceivedQueueTimeoutValue = 0; @@ -141,6 +125,23 @@ ArpCreateService (