Re: [edk2] [Patch] MdeModulePkg: Refine SNP driver's media status check logic.

[Ye, Ting]

Siyuan,

Some minor comments to the patch:
1. +  // Whether UNDI support cable detect for INITIALIZE command,  // i.e. 
+ PXE_STATFLAGS_CABLE_DETECT_NOT_SUPPORTED

Suggest to also include PXE_STATFLAGS_CABLE_DETECT_SUPPORTED if uses "i.e." 
here.

2. In the function header of PxeGetStatus(), there is a redundant \ need be 
removed. (MediaPresent\  field...)
Also since we add the function declaration to header file, I suggest we update 
the style of param to param[in] or param[out].

Others are good to me.

Reviewed-by: Ye Ting 

Thanks,
Ting


-Original Message-
From: Fu, Siyuan 
Sent: Wednesday, May 04, 2016 9:13 AM
To: edk2-devel@lists.01.org
Cc: Ye, Ting ; Wu, Jiaxin 
Subject: [Patch] MdeModulePkg: Refine SNP driver's media status check logic.

Some UNDI drivers may not support the cable detect in UNDI INITIALIZE command, 
but support the media present check in UNDI GET_STATUS command. Current SNP 
driver will set the MediaPresentSupported field to FALSE in 
EFI_SIMPLE_NETWORK_MODE for such case, which forbid the media detect from the 
callers.

This patch updates the SNP driver to support such kind of UNDIs for media 
detect.
MediaPresentSupported will be set to TRUE, and a GET_STATUS command will be 
issued immediately after INITIALIZE command in SNP->Initialize() function, to 
refresh the value of MediaPresent in SNP mode data.

Cc: Ye Ting 
Cc: Wu Jiaxin 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Fu Siyuan 
---
 MdeModulePkg/Universal/Network/SnpDxe/Get_status.c |  3 ++-  
MdeModulePkg/Universal/Network/SnpDxe/Initialize.c | 13 +-
 MdeModulePkg/Universal/Network/SnpDxe/Snp.c|  8 --
 MdeModulePkg/Universal/Network/SnpDxe/Snp.h| 30 ++
 4 files changed, 50 insertions(+), 4 deletions(-)

diff --git a/MdeModulePkg/Universal/Network/SnpDxe/Get_status.c 
b/MdeModulePkg/Universal/Network/SnpDxe/Get_status.c
index edbc0f2..3c6bf39 100644
--- a/MdeModulePkg/Universal/Network/SnpDxe/Get_status.c
+++ b/MdeModulePkg/Universal/Network/SnpDxe/Get_status.c
@@ -18,7 +18,8 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER 
EXPRESS OR IMPLIED.
 /**
   Call undi to get the status of the interrupts, get the list of recycled 
transmit
   buffers that completed transmitting. The recycled transmit buffer address 
will
-  be saved into Snp->RecycledTxBuf.
+  be saved into Snp->RecycledTxBuf. This function will also update the 
+ MediaPresent  field of EFI_SIMPLE_NETWORK_MODE if UNDI support it.
 
   @param  Snp Pointer to snp driver structure.
   @param  InterruptStatusPtr  A non null pointer to contain the interrupt
diff --git a/MdeModulePkg/Universal/Network/SnpDxe/Initialize.c 
b/MdeModulePkg/Universal/Network/SnpDxe/Initialize.c
index 3f40ef3..2151375 100644
--- a/MdeModulePkg/Universal/Network/SnpDxe/Initialize.c
+++ b/MdeModulePkg/Universal/Network/SnpDxe/Initialize.c
@@ -229,7 +229,10 @@ SnpUndi32Initialize (
   //
   Snp->TxRxBufferSize = (UINT32) (Snp->InitInfo.MemoryRequired + 
ExtraRxBufferSize + ExtraTxBufferSize);
 
-  if (Snp->Mode.MediaPresentSupported) {
+  //
+  // If UNDI support cable detect for INITIALIZE command, try it first.
+  //
+  if (Snp->CableDetectSupported) {
 if (PxeInit (Snp, PXE_OPFLAGS_INITIALIZE_DETECT_CABLE) == EFI_SUCCESS) {
   Snp->Mode.MediaPresent = TRUE;
   goto ON_EXIT;
@@ -242,6 +245,14 @@ SnpUndi32Initialize (
 
   if (EFI_ERROR (EfiStatus)) {
 gBS->CloseEvent (Snp->Snp.WaitForPacket);
+goto ON_EXIT;
+  }
+
+  //
+  // Try to update the MediaPresent field of EFI_SIMPLE_NETWORK_MODE if the 
UNDI support it.
+  //
+  if (Snp->MediaStatusSupported) {
+PxeGetStatus (Snp, NULL, FALSE);
   }
 
 ON_EXIT:
diff --git a/MdeModulePkg/Universal/Network/SnpDxe/Snp.c 
b/MdeModulePkg/Universal/Network/SnpDxe/Snp.c
index 5ff294f..9f61aee 100644
--- a/MdeModulePkg/Universal/Network/SnpDxe/Snp.c
+++ b/MdeModulePkg/Universal/Network/SnpDxe/Snp.c
@@ -554,12 +554,12 @@ SimpleNetworkDriverStart (
 
   switch (InitStatFlags & PXE_STATFLAGS_CABLE_DETECT_MASK) {
   case PXE_STATFLAGS_CABLE_DETECT_SUPPORTED:
-Snp->Mode.MediaPresentSupported = TRUE;
+Snp->CableDetectSupported = TRUE;
 break;
 
   case PXE_STATFLAGS_CABLE_DETECT_NOT_SUPPORTED:
   default:
-Snp->Mode.MediaPresentSupported = FALSE;
+Snp->CableDetectSupported = FALSE;
   }
 
   switch (InitStatFlags & PXE_STATFLAGS_GET_STATUS_NO_MEDIA_MASK) { @@ -572,6 
+572,10 @@ SimpleNetworkDriverStart (
 Snp->MediaStatusSupported = FALSE;
   }
 
+  if (Snp->CableDetectSupported || Snp->MediaStatusSupported) {
+Snp->Mode.MediaPresentSupported = TRUE;  }
+
   if ((Pxe->hw.Implementation & PXE_ROMID_IMP_STATION_ADDR_SETTABLE) != 0) {
 Snp->Mode.MacAddressChangeable = TRUE;
   } else {
diff --git a/MdeModulePkg/Universal/Network/SnpDxe/Snp.h 

[edk2] [Patch] MdeModulePkg: Refine SNP driver's media status check logic.

[Fu Siyuan]

Some UNDI drivers may not support the cable detect in UNDI INITIALIZE command,
but support the media present check in UNDI GET_STATUS command. Current SNP
driver will set the MediaPresentSupported field to FALSE in 
EFI_SIMPLE_NETWORK_MODE
for such case, which forbid the media detect from the callers.

This patch updates the SNP driver to support such kind of UNDIs for media 
detect.
MediaPresentSupported will be set to TRUE, and a GET_STATUS command will be 
issued
immediately after INITIALIZE command in SNP->Initialize() function, to refresh
the value of MediaPresent in SNP mode data.

Cc: Ye Ting 
Cc: Wu Jiaxin 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Fu Siyuan 
---
 MdeModulePkg/Universal/Network/SnpDxe/Get_status.c |  3 ++-
 MdeModulePkg/Universal/Network/SnpDxe/Initialize.c | 13 +-
 MdeModulePkg/Universal/Network/SnpDxe/Snp.c|  8 --
 MdeModulePkg/Universal/Network/SnpDxe/Snp.h| 30 ++
 4 files changed, 50 insertions(+), 4 deletions(-)

diff --git a/MdeModulePkg/Universal/Network/SnpDxe/Get_status.c 
b/MdeModulePkg/Universal/Network/SnpDxe/Get_status.c
index edbc0f2..3c6bf39 100644
--- a/MdeModulePkg/Universal/Network/SnpDxe/Get_status.c
+++ b/MdeModulePkg/Universal/Network/SnpDxe/Get_status.c
@@ -18,7 +18,8 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER 
EXPRESS OR IMPLIED.
 /**
   Call undi to get the status of the interrupts, get the list of recycled 
transmit
   buffers that completed transmitting. The recycled transmit buffer address 
will
-  be saved into Snp->RecycledTxBuf.
+  be saved into Snp->RecycledTxBuf. This function will also update the 
MediaPresent
+  field of EFI_SIMPLE_NETWORK_MODE if UNDI support it.
 
   @param  Snp Pointer to snp driver structure.
   @param  InterruptStatusPtr  A non null pointer to contain the interrupt
diff --git a/MdeModulePkg/Universal/Network/SnpDxe/Initialize.c 
b/MdeModulePkg/Universal/Network/SnpDxe/Initialize.c
index 3f40ef3..2151375 100644
--- a/MdeModulePkg/Universal/Network/SnpDxe/Initialize.c
+++ b/MdeModulePkg/Universal/Network/SnpDxe/Initialize.c
@@ -229,7 +229,10 @@ SnpUndi32Initialize (
   //
   Snp->TxRxBufferSize = (UINT32) (Snp->InitInfo.MemoryRequired + 
ExtraRxBufferSize + ExtraTxBufferSize);
 
-  if (Snp->Mode.MediaPresentSupported) {
+  //
+  // If UNDI support cable detect for INITIALIZE command, try it first.
+  //
+  if (Snp->CableDetectSupported) {
 if (PxeInit (Snp, PXE_OPFLAGS_INITIALIZE_DETECT_CABLE) == EFI_SUCCESS) {
   Snp->Mode.MediaPresent = TRUE;
   goto ON_EXIT;
@@ -242,6 +245,14 @@ SnpUndi32Initialize (
 
   if (EFI_ERROR (EfiStatus)) {
 gBS->CloseEvent (Snp->Snp.WaitForPacket);
+goto ON_EXIT;
+  }
+
+  //
+  // Try to update the MediaPresent field of EFI_SIMPLE_NETWORK_MODE if the 
UNDI support it.
+  //
+  if (Snp->MediaStatusSupported) {
+PxeGetStatus (Snp, NULL, FALSE);
   }
 
 ON_EXIT:
diff --git a/MdeModulePkg/Universal/Network/SnpDxe/Snp.c 
b/MdeModulePkg/Universal/Network/SnpDxe/Snp.c
index 5ff294f..9f61aee 100644
--- a/MdeModulePkg/Universal/Network/SnpDxe/Snp.c
+++ b/MdeModulePkg/Universal/Network/SnpDxe/Snp.c
@@ -554,12 +554,12 @@ SimpleNetworkDriverStart (
 
   switch (InitStatFlags & PXE_STATFLAGS_CABLE_DETECT_MASK) {
   case PXE_STATFLAGS_CABLE_DETECT_SUPPORTED:
-Snp->Mode.MediaPresentSupported = TRUE;
+Snp->CableDetectSupported = TRUE;
 break;
 
   case PXE_STATFLAGS_CABLE_DETECT_NOT_SUPPORTED:
   default:
-Snp->Mode.MediaPresentSupported = FALSE;
+Snp->CableDetectSupported = FALSE;
   }
 
   switch (InitStatFlags & PXE_STATFLAGS_GET_STATUS_NO_MEDIA_MASK) {
@@ -572,6 +572,10 @@ SimpleNetworkDriverStart (
 Snp->MediaStatusSupported = FALSE;
   }
 
+  if (Snp->CableDetectSupported || Snp->MediaStatusSupported) {
+Snp->Mode.MediaPresentSupported = TRUE;
+  }
+
   if ((Pxe->hw.Implementation & PXE_ROMID_IMP_STATION_ADDR_SETTABLE) != 0) {
 Snp->Mode.MacAddressChangeable = TRUE;
   } else {
diff --git a/MdeModulePkg/Universal/Network/SnpDxe/Snp.h 
b/MdeModulePkg/Universal/Network/SnpDxe/Snp.h
index 67f65ff..f802dfb 100644
--- a/MdeModulePkg/Universal/Network/SnpDxe/Snp.h
+++ b/MdeModulePkg/Universal/Network/SnpDxe/Snp.h
@@ -135,6 +135,12 @@ typedef struct {
   BOOLEANMediaStatusSupported;
 
   //
+  // Whether UNDI support cable detect for INITIALIZE command,
+  // i.e. PXE_STATFLAGS_CABLE_DETECT_NOT_SUPPORTED
+  //
+  BOOLEANCableDetectSupported;
+
+  //
   // Array of the recycled transmit buffer address from UNDI.
   //
   UINT64 *RecycledTxBuf;
@@ -234,6 +240,30 @@ PxeGetStnAddr (
   );
 
 /**
+  Call undi to get the status of the interrupts, get the list of recycled 
transmit
+  buffers that completed transmitting. The recycled transmit buffer address 
will
+  be saved into Snp->RecycledTxBuf. This function will also update the