Re: [Patch] MdeModulePkg/NetLib: Add NetLibDetectMediaWaitTimeout() API to support EFI_NOT_READY media state detection

["Wu, Jiaxin" <jiaxin.wu@intel.com>]

My comments as below:

1.  "ASSERT (MediaState != NULL)" should be removed since it will be checked later. The same issue is found in NetLibDetectMedia().

2. The Timer event should be created in the first loop for the second loop's check, otherwise, it will be invalid for the next CheckEvent().

	do {
 		 //
  		 // Create new Timer
 		 //
		CreateEvent ()
		SetTimer ();
		do {
			CheckEvent();
		}
		CloseEvent (Timer);
		Timer = NULL;	
	}

3. How about changing the default value of *MediaState to EFI_SUCCESS? You know the error status may be returned from API, then the MediaState can't be updated correctly. In such a case, the MediaState should be treated as EFI_SUCCESS.

Or 

We might need add the description to comments that the MediaState is only valid when EFI_SUCCESS returned from API.

Thanks,
Jiaxin


> -----Original Message-----
> From: Wang, Fan
> Sent: Monday, November 27, 2017 4:12 PM
> To: edk2-devel@lists.01.org
> Cc: Fu, Siyuan <siyuan.fu@intel.com>; Ye, Ting <ting.ye@intel.com>; Wu,
> Jiaxin <jiaxin.wu@intel.com>; Wang, Fan <fan.wang@intel.com>
> Subject: [Patch] MdeModulePkg/NetLib: Add
> NetLibDetectMediaWaitTimeout() API to support EFI_NOT_READY media
> state detection
> 
> In wireless connection, connecting state needs to be cared more
> about. ECR 1772 redefined the state EFI_NOT_READY to represent
> connecting state and can be retrieved from Aip protocol. This
> patch adds a new API to check media state at a specified time
> interval when network is connecting until the connection process
> finishes or timeout.
> 
> Cc: Fu Siyuan <siyuan.fu@intel.com>
> Cc: Ye Ting <ting.ye@intel.com>
> Cc: Jiaxin Wu <jiaxin.wu@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Wang Fan <fan.wang@intel.com>
> ---
>  MdeModulePkg/Include/Library/NetLib.h        |  40 +++++++
>  MdeModulePkg/Library/DxeNetLib/DxeNetLib.c   | 160
> +++++++++++++++++++++++++++
>  MdeModulePkg/Library/DxeNetLib/DxeNetLib.inf |   2 +
>  3 files changed, 202 insertions(+)
> 
> diff --git a/MdeModulePkg/Include/Library/NetLib.h
> b/MdeModulePkg/Include/Library/NetLib.h
> index b9df46c..7862df9 100644
> --- a/MdeModulePkg/Include/Library/NetLib.h
> +++ b/MdeModulePkg/Include/Library/NetLib.h
> @@ -93,10 +93,16 @@ typedef UINT16          TCP_PORTNO;
>  #define  DNS_CLASS_INET        1
>  #define  DNS_CLASS_CH          3
>  #define  DNS_CLASS_HS          4
>  #define  DNS_CLASS_ANY         255
> 
> +//
> +// Number of 100ns units time Interval for network media state detect
> +//
> +#define MEDIA_STATE_DETECT_TIME_INTERVAL  1000000U
> +
> +
>  #pragma pack(1)
> 
>  //
>  // Ethernet head definition
>  //
> @@ -1246,10 +1252,44 @@ NetLibDetectMedia (
>    IN  EFI_HANDLE            ServiceHandle,
>    OUT BOOLEAN               *MediaPresent
>    );
> 
>  /**
> +
> +  Detect media state for a network device. This routine will wait for a period
> of time at
> +  a specified checking interval when a certain network is under connecting
> until connection
> +  process finishes or timeout. If Aip protocol is supported by low layer
> drivers, three kinds
> +  of media states can be detected: EFI_SUCCESS, EFI_NOT_READY and
> EFI_NO_MEDIA, represents
> +  connected state, connecting state and no media state respectively. When
> function detects
> +  the current state is EFI_NOT_READY, it will loop to wait for next time's
> check until state
> +  turns to be EFI_SUCCESS or EFI_NO_MEDIA. If Aip protocol is not
> supported, function will
> +  call NetLibDetectMedia() and return state directly.
> +
> +  @param[in]   ServiceHandle    The handle where network service binding
> protocols are
> +                                installed on.
> +  @param[in]   Timeout          The maximum number of 100ns units to wait
> when network
> +                                is connecting. Zero value means detect once and return
> +                                immediately.
> +  @param[out]  MediaState       The pointer to the detected media state.
> +
> +  @retval EFI_SUCCESS           Media detection success.
> +  @retval EFI_INVALID_PARAMETER ServiceHandle is not a valid network
> device handle or
> +                                MediaState pointer is NULL.
> +  @retval EFI_DEVICE_ERROR      A device error occurred.
> +  @retval EFI_TIMEOUT           Network is connecting but timeout.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +NetLibDetectMediaWaitTimeout (
> +  IN  EFI_HANDLE            ServiceHandle,
> +  IN  UINT64                Timeout,
> +  OUT EFI_STATUS            *MediaState
> +  );
> +
> +
> +/**
>    Create an IPv4 device path node.
> 
>    The header type of IPv4 device path node is MESSAGING_DEVICE_PATH.
>    The header subtype of IPv4 device path node is MSG_IPv4_DP.
>    The length of the IPv4 device path node in bytes is 19.
> diff --git a/MdeModulePkg/Library/DxeNetLib/DxeNetLib.c
> b/MdeModulePkg/Library/DxeNetLib/DxeNetLib.c
> index b8544b8..3030147 100644
> --- a/MdeModulePkg/Library/DxeNetLib/DxeNetLib.c
> +++ b/MdeModulePkg/Library/DxeNetLib/DxeNetLib.c
> @@ -17,10 +17,11 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF
> ANY KIND, EITHER EXPRESS OR IMPLIED.
>  #include <IndustryStandard/SmBios.h>
> 
>  #include <Protocol/DriverBinding.h>
>  #include <Protocol/ServiceBinding.h>
>  #include <Protocol/SimpleNetwork.h>
> +#include <Protocol/AdapterInformation.h>
>  #include <Protocol/ManagedNetwork.h>
>  #include <Protocol/Ip4Config2.h>
>  #include <Protocol/ComponentName.h>
>  #include <Protocol/ComponentName2.h>
> 
> @@ -2501,10 +2502,169 @@ Exit:
> 
>    return Status;
>  }
> 
>  /**
> +
> +  Detect media state for a network device. This routine will wait for a period
> of time at
> +  a specified checking interval when a certain network is under connecting
> until connection
> +  process finishes or timeout. If Aip protocol is supported by low layer
> drivers, three kinds
> +  of media states can be detected: EFI_SUCCESS, EFI_NOT_READY and
> EFI_NO_MEDIA, represents
> +  connected state, connecting state and no media state respectively. When
> function detects
> +  the current state is EFI_NOT_READY, it will loop to wait for next time's
> check until state
> +  turns to be EFI_SUCCESS or EFI_NO_MEDIA. If Aip protocol is not
> supported, function will
> +  call NetLibDetectMedia() and return state directly.
> +
> +  @param[in]   ServiceHandle    The handle where network service binding
> protocols are
> +                                installed on.
> +  @param[in]   Timeout          The maximum number of 100ns units to wait
> when network
> +                                is connecting. Zero value means detect once and return
> +                                immediately.
> +  @param[out]  MediaState       The pointer to the detected media state.
> +
> +  @retval EFI_SUCCESS           Media detection success.
> +  @retval EFI_INVALID_PARAMETER ServiceHandle is not a valid network
> device handle or
> +                                MediaState pointer is NULL.
> +  @retval EFI_DEVICE_ERROR      A device error occurred.
> +  @retval EFI_TIMEOUT           Network is connecting but timeout.
> +
> +**/
> +
> +EFI_STATUS
> +EFIAPI
> +NetLibDetectMediaWaitTimeout (
> +  IN  EFI_HANDLE            ServiceHandle,
> +  IN  UINT64                Timeout,
> +  OUT EFI_STATUS            *MediaState
> +  )
> +{
> +  EFI_STATUS                        Status;
> +  EFI_HANDLE                        SnpHandle;
> +  EFI_SIMPLE_NETWORK_PROTOCOL       *Snp;
> +  EFI_ADAPTER_INFORMATION_PROTOCOL  *Aip;
> +  EFI_ADAPTER_INFO_MEDIA_STATE      *MediaInfo;
> +  BOOLEAN                           MediaPresent;
> +  UINTN                             DataSize;
> +  EFI_STATUS                        TimerStatus;
> +  EFI_EVENT                         Timer;
> +  UINT64                            TimeRemained;
> +
> +  ASSERT (MediaState != NULL);
> +  if (MediaState == NULL) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +  *MediaState = EFI_NO_MEDIA;
> +
> +  //
> +  // Get SNP handle
> +  //
> +  Snp = NULL;
> +  SnpHandle = NetLibGetSnpHandle (ServiceHandle, &Snp);
> +  if (SnpHandle == NULL) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  Status = gBS->HandleProtocol (
> +                  SnpHandle,
> +                  &gEfiAdapterInformationProtocolGuid,
> +                  (VOID *) &Aip
> +                  );
> +  if (EFI_ERROR (Status)) {
> +
> +    MediaPresent = TRUE;
> +    Status = NetLibDetectMedia (ServiceHandle, &MediaPresent);
> +    if (!EFI_ERROR (Status)) {
> +      if (MediaPresent == TRUE) {
> +        *MediaState = EFI_SUCCESS;
> +      } else {
> +        *MediaState = EFI_NO_MEDIA;
> +      }
> +    }
> +
> +    //
> +    // NetLibDetectMedia doesn't support EFI_NOT_READY status, return
> now!
> +    //
> +    return Status;
> +  }
> +
> +  Status = Aip->GetInformation (
> +                  Aip,
> +                  &gEfiAdapterInfoMediaStateGuid,
> +                  (VOID **) &MediaInfo,
> +                  &DataSize
> +                  );
> +  if (!EFI_ERROR (Status)) {
> +
> +    *MediaState = MediaInfo->MediaState;
> +    if (*MediaState != EFI_NOT_READY) {
> +
> +      FreePool (MediaInfo);
> +      return EFI_SUCCESS;
> +    }
> +  }
> +
> +  if (MediaInfo != NULL) {
> +    FreePool (MediaInfo);
> +  }
> +
> +  if (Timeout < MEDIA_STATE_DETECT_TIME_INTERVAL) {
> +    return Status;
> +  }
> +
> +  //
> +  // Loop to check media state
> +  //
> +
> +  Timer        = NULL;
> +  TimeRemained = Timeout;
> +  Status = gBS->CreateEvent (EVT_TIMER, TPL_CALLBACK, NULL, NULL,
> &Timer);
> +  if (EFI_ERROR (Status)) {
> +    return EFI_DEVICE_ERROR;
> +  }
> +
> +  do {
> +    Status = gBS->SetTimer (
> +                    Timer,
> +                    TimerRelative,
> +                    MEDIA_STATE_DETECT_TIME_INTERVAL
> +                    );
> +    if (EFI_ERROR (Status)) {
> +      gBS->CloseEvent(Timer);
> +      return EFI_DEVICE_ERROR;
> +    }
> +
> +    do {
> +      TimerStatus = gBS->CheckEvent (Timer);
> +      if (!EFI_ERROR (TimerStatus)) {
> +
> +        TimeRemained -= MEDIA_STATE_DETECT_TIME_INTERVAL;
> +        Status = Aip->GetInformation (
> +                        Aip,
> +                        &gEfiAdapterInfoMediaStateGuid,
> +                        (VOID **) &MediaInfo,
> +                        &DataSize
> +                        );
> +        if (!EFI_ERROR (Status)) {
> +          *MediaState = MediaInfo->MediaState;
> +        }
> +
> +        if (MediaInfo != NULL) {
> +          FreePool (MediaInfo);
> +        }
> +      }
> +    } while (TimerStatus == EFI_NOT_READY);
> +  } while (*MediaState == EFI_NOT_READY && TimeRemained >=
> MEDIA_STATE_DETECT_TIME_INTERVAL);
> +
> +  gBS->CloseEvent(Timer);
> +  if (*MediaState == EFI_NOT_READY && TimeRemained <
> MEDIA_STATE_DETECT_TIME_INTERVAL) {
> +    return EFI_TIMEOUT;
> +  } else {
> +    return EFI_SUCCESS;
> +  }
> +}
> +
> +/**
>    Check the default address used by the IPv4 driver is static or dynamic
> (acquired
>    from DHCP).
> 
>    If the controller handle does not have the EFI_IP4_CONFIG2_PROTOCOL
> installed, the
>    default address is static. If failed to get the policy from Ip4 Config2 Protocol,
> diff --git a/MdeModulePkg/Library/DxeNetLib/DxeNetLib.inf
> b/MdeModulePkg/Library/DxeNetLib/DxeNetLib.inf
> index 1ff3a4f..ad0727c 100644
> --- a/MdeModulePkg/Library/DxeNetLib/DxeNetLib.inf
> +++ b/MdeModulePkg/Library/DxeNetLib/DxeNetLib.inf
> @@ -52,14 +52,16 @@
> 
> 
>  [Guids]
>    gEfiSmbiosTableGuid                           ## SOMETIMES_CONSUMES  ##
> SystemTable
>    gEfiSmbios3TableGuid                          ## SOMETIMES_CONSUMES  ##
> SystemTable
> +  gEfiAdapterInfoMediaStateGuid                 ## SOMETIMES_CONSUMES
> 
> 
>  [Protocols]
>    gEfiSimpleNetworkProtocolGuid                 ## SOMETIMES_CONSUMES
>    gEfiManagedNetworkProtocolGuid                ## SOMETIMES_CONSUMES
>    gEfiManagedNetworkServiceBindingProtocolGuid  ##
> SOMETIMES_CONSUMES
>    gEfiIp4Config2ProtocolGuid                    ## SOMETIMES_CONSUMES
>    gEfiComponentNameProtocolGuid                 ## SOMETIMES_CONSUMES
>    gEfiComponentName2ProtocolGuid                ## SOMETIMES_CONSUMES
> +  gEfiAdapterInformationProtocolGuid            ## SOMETIMES_CONSUMES
> \ No newline at end of file
> --
> 1.9.5.msysgit.1