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

["Marcin Wojtas" <mw@semihalf.com>]

Hi Ard,

pt., 3 maj 2019 o 08:33 Ard Biesheuvel <ard.biesheuvel@linaro.org> napisał(a):
>
> 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?
>

In Pp2Dxe those are the only 2 possible return values, but despite
that I'll add handling for:
} else if (EFI_ERROR(Status) {

> > +
> > +  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
> >