Re: [edk2-platforms: PATCH 1/4] Marvel/Drivers: Pp2Dxe: Basic support for Adapter Information Protocol

["Ard Biesheuvel" <ard.biesheuvel@linaro.org>]

On Fri, 3 May 2019 at 01:50, Marcin Wojtas <mw@semihalf.com> wrote:
>
> From: Tomasz Michalec <tm@semihalf.com>
>
> Add implementation of Adapter Information Protocol in Armada 7k8k
> PP2 NIC driver. Support retrieving information about media state
> which allows to use NetLibDetectMediaWaitTimeout on network interface.
> This patch fixes possible hangs when attempting to PXE boot on
> unconnected interfaces.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Tomasz Michalec <tm@semihalf.com>

Please put your own signoff here. You can retain the one from Tomasz
if you like.

> ---
>  Silicon/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.inf |   1 +
>  Silicon/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.h   |   3 +
>  Silicon/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.c   | 157 ++++++++++++++++++++
>  3 files changed, 161 insertions(+)
>
> diff --git a/Silicon/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.inf b/Silicon/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.inf
> index be536ab..73aa413 100644
> --- a/Silicon/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.inf
> +++ b/Silicon/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.inf
> @@ -64,6 +64,7 @@
>    CacheMaintenanceLib
>
>  [Protocols]
> +  gEfiAdapterInformationProtocolGuid
>    gEfiSimpleNetworkProtocolGuid
>    gEfiDevicePathProtocolGuid
>    gEfiCpuArchProtocolGuid
> diff --git a/Silicon/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.h b/Silicon/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.h
> index b8a5dae..66bd46a 100644
> --- a/Silicon/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.h
> +++ b/Silicon/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.h
> @@ -35,6 +35,7 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>  #ifndef __PP2_DXE_H__
>  #define __PP2_DXE_H__
>
> +#include <Protocol/AdapterInformation.h>
>  #include <Protocol/Cpu.h>
>  #include <Protocol/DevicePath.h>
>  #include <Protocol/DriverBinding.h>
> @@ -59,6 +60,7 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>  #define MVPP2_MAX_PORT  3
>
>  #define PP2DXE_SIGNATURE                    SIGNATURE_32('P', 'P', '2', 'D')
> +#define INSTANCE_FROM_AIP(a)                CR((a), PP2DXE_CONTEXT, Aip, PP2DXE_SIGNATURE)
>  #define INSTANCE_FROM_SNP(a)                CR((a), PP2DXE_CONTEXT, Snp, PP2DXE_SIGNATURE)
>
>  /* OS API */
> @@ -365,6 +367,7 @@ typedef struct {
>    UINTN                       CompletionQueueTail;
>    EFI_EVENT                   EfiExitBootServicesEvent;
>    PP2_DEVICE_PATH             *DevicePath;
> +  EFI_ADAPTER_INFORMATION_PROTOCOL Aip;
>  } PP2DXE_CONTEXT;
>
>  /* Inline helpers */
> diff --git a/Silicon/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.c b/Silicon/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.c
> index 02b2798..473a2a1 100644
> --- a/Silicon/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.c
> +++ b/Silicon/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.c
> @@ -1129,6 +1129,7 @@ Pp2DxeSnpInstall (
>        &Handle,
>        &gEfiSimpleNetworkProtocolGuid, &Pp2Context->Snp,
>        &gEfiDevicePathProtocolGuid, Pp2DevicePath,
> +      &gEfiAdapterInformationProtocolGuid, &Pp2Context->Aip,
>        NULL
>        );
>
> @@ -1139,6 +1140,157 @@ Pp2DxeSnpInstall (
>    return Status;
>  }
>
> +/**
> +  Returns the current state information for the adapter.
> +
> +  This function returns information of type InformationType from the adapter.
> +  If an adapter does not support the requested informational type, then
> +  EFI_UNSUPPORTED is returned.
> +
> +  @param[in]  This                   A pointer to the EFI_ADAPTER_INFORMATION_PROTOCOL instance.
> +  @param[in]  InformationType        A pointer to an EFI_GUID that defines the contents of InformationBlock.
> +  @param[out] InforamtionBlock       The service returns a pointer to the buffer with the InformationBlock
> +                                     structure which contains details about the data specific to InformationType.
> +  @param[out] InforamtionBlockSize   The driver returns the size of the InformationBlock in bytes.

Please fix these typos. I know they were copy/pasted from the protocol
definition, but I'd still prefer them to be fixed.

> +
> +  @retval EFI_SUCCESS                The InformationType information was retrieved.
> +  @retval EFI_UNSUPPORTED            The InformationType is not known.
> +  @retval EFI_DEVICE_ERROR           The device reported an error.
> +  @retval EFI_OUT_OF_RESOURCES       The request could not be completed due to a lack of resources.
> +  @retval EFI_INVALID_PARAMETER      This is NULL.
> +  @retval EFI_INVALID_PARAMETER      InformationBlock is NULL.
> +  @retval EFI_INVALID_PARAMETER      InformationBlockSize is NULL.
> +
> +**/

STATIC?

> +EFI_STATUS
> +EFIAPI
> +Pp2AipGetInformation (
> +  IN  EFI_ADAPTER_INFORMATION_PROTOCOL  *This,
> +  IN  EFI_GUID                          *InformationType,
> +  OUT VOID                              **InformationBlock,
> +  OUT UINTN                             *InformationBlockSize
> +  )
> +{
> +  EFI_ADAPTER_INFO_MEDIA_STATE  *AdapterInfo;
> +  PP2DXE_CONTEXT                *Pp2Context;
> +  EFI_STATUS                     Status;
> +
> +  if (This == NULL || InformationBlock == NULL || InformationBlockSize == NULL) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  if (!CompareGuid (InformationType, &gEfiAdapterInfoMediaStateGuid)) {
> +    return EFI_UNSUPPORTED;
> +  }
> +
> +  AdapterInfo = AllocateZeroPool (sizeof (EFI_ADAPTER_INFO_MEDIA_STATE));
> +  if (AdapterInfo == NULL) {
> +    return EFI_OUT_OF_RESOURCES;
> +  }
> +
> +  *InformationBlock = AdapterInfo;
> +  *InformationBlockSize = sizeof (EFI_ADAPTER_INFO_MEDIA_STATE);
> +
> +  Pp2Context = INSTANCE_FROM_AIP (This);
> +
> +  Status = Pp2Context->Snp.GetStatus (&(Pp2Context->Snp), NULL, NULL);
> +  if (Status == EFI_NOT_READY){
> +    AdapterInfo->MediaState = EFI_NOT_READY;
> +    return EFI_SUCCESS;
> +  }

What happens if GetStatus returns something other than EFI_SUCCESS or
EFI_NOT_READY?

> +
> +  AdapterInfo->MediaState = (Pp2Context->Snp.Mode->MediaPresent ?
> +                             EFI_SUCCESS : EFI_NOT_READY);
> +

Please get rid of the ternary expression, just fold the condition into
the preceding if()


> +  return EFI_SUCCESS;
> +}
> +
> +/**
> +  Sets state information for an adapter.
> +
> +  This function sends information of type InformationType for an adapter.
> +  If an adapter does not support the requested information type, then EFI_UNSUPPORTED
> +  is returned.
> +
> +  @param[in]  This                   A pointer to the EFI_ADAPTER_INFORMATION_PROTOCOL instance.
> +  @param[in]  InformationType        A pointer to an EFI_GUID that defines the contents of InformationBlock.
> +  @param[in]  InforamtionBlock       A pointer to the InformationBlock structure which contains details
> +                                     about the data specific to InformationType.
> +  @param[in]  InforamtionBlockSize   The size of the InformationBlock in bytes.
> +

More typos

> +  @retval EFI_SUCCESS                The information was received and interpreted successfully.
> +  @retval EFI_UNSUPPORTED            The InformationType is not known.
> +  @retval EFI_DEVICE_ERROR           The device reported an error.
> +  @retval EFI_INVALID_PARAMETER      This is NULL.
> +  @retval EFI_INVALID_PARAMETER      InformationBlock is NULL.
> +  @retval EFI_WRITE_PROTECTED        The InformationType cannot be modified using EFI_ADAPTER_INFO_SET_INFO().
> +
> +**/

STATIC?

> +EFI_STATUS
> +EFIAPI
> +Pp2AipSetInformation (
> +  IN  EFI_ADAPTER_INFORMATION_PROTOCOL  *This,
> +  IN  EFI_GUID                          *InformationType,
> +  IN  VOID                              *InformationBlock,
> +  IN  UINTN                             InformationBlockSize
> +  )
> +{
> +  if (This == NULL || InformationBlock == NULL) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  if (CompareGuid (InformationType, &gEfiAdapterInfoMediaStateGuid)) {
> +    return EFI_WRITE_PROTECTED;
> +  }
> +
> +  return EFI_UNSUPPORTED;
> +}
> +
> +/**
> +  Get a list of supported information types for this instance of the protocol.
> +
> +  This function returns a list of InformationType GUIDs that are supported on an
> +  adapter with this instance of EFI_ADAPTER_INFORMATION_PROTOCOL. The list is returned
> +  in InfoTypesBuffer, and the number of GUID pointers in InfoTypesBuffer is returned in
> +  InfoTypesBufferCount.
> +
> +  @param[in]  This                  A pointer to the EFI_ADAPTER_INFORMATION_PROTOCOL instance.
> +  @param[out] InfoTypesBuffer       A pointer to the array of InformationType GUIDs that are supported
> +                                    by This.
> +  @param[out] InfoTypesBufferCount  A pointer to the number of GUIDs present in InfoTypesBuffer.
> +
> +  @retval EFI_SUCCESS               The list of information type GUIDs that are supported on this adapter was
> +                                    returned in InfoTypesBuffer. The number of information type GUIDs was
> +                                    returned in InfoTypesBufferCount.
> +  @retval EFI_INVALID_PARAMETER     This is NULL.
> +  @retval EFI_INVALID_PARAMETER     InfoTypesBuffer is NULL.
> +  @retval EFI_INVALID_PARAMETER     InfoTypesBufferCount is NULL.
> +  @retval EFI_OUT_OF_RESOURCES      There is not enough pool memory to store the results.
> +
> +**/

STATIC

> +EFI_STATUS
> +EFIAPI
> +Pp2AipGetSupportedTypes (
> +  IN  EFI_ADAPTER_INFORMATION_PROTOCOL  *This,
> +  OUT EFI_GUID                          **InfoTypesBuffer,
> +  OUT UINTN                             *InfoTypesBufferCount
> +  )
> +{
> +  if (This == NULL || InfoTypesBuffer == NULL || InfoTypesBufferCount == NULL) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  *InfoTypesBuffer = AllocateZeroPool (sizeof (EFI_GUID));

No need to zero if you assign the whole thing immediately

> +  if (*InfoTypesBuffer == NULL) {
> +    return EFI_OUT_OF_RESOURCES;
> +  }
> +
> +  *InfoTypesBufferCount = 1;
> +  (*InfoTypesBuffer)[0] = gEfiAdapterInfoMediaStateGuid;
> +

Use CopyGuid() not struct assignment

> +  return EFI_SUCCESS;
> +}
> +
>  STATIC
>  VOID
>  Pp2DxeParsePortPcd (
> @@ -1290,6 +1442,11 @@ Pp2DxeInitialiseController (
>      Pp2Context->Instance = DeviceInstance;
>      DeviceInstance++;
>
> +    /* Prepare AIP Protocol */
> +    Pp2Context->Aip.GetInformation    = Pp2AipGetInformation;
> +    Pp2Context->Aip.SetInformation    = Pp2AipSetInformation;
> +    Pp2Context->Aip.GetSupportedTypes = Pp2AipGetSupportedTypes;
> +
>      /* Install SNP protocol */
>      Status = Pp2DxeSnpInstall(Pp2Context);
>      if (EFI_ERROR(Status)) {
> --
> 2.7.4
>