From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=pass header.i=@semihalf-com.20150623.gappssmtp.com header.s=20150623 header.b=yiISDjkd; spf=none, err=SPF record not found (domain: semihalf.com, ip: 209.85.160.196, mailfrom: mw@semihalf.com) Received: from mail-qt1-f196.google.com (mail-qt1-f196.google.com [209.85.160.196]) by groups.io with SMTP; Fri, 03 May 2019 00:49:45 -0700 Received: by mail-qt1-f196.google.com with SMTP id c35so5702574qtk.3 for ; Fri, 03 May 2019 00:49:45 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=semihalf-com.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=BwQwEJLpT43xq/Fvzee5dU07H0aBv3kpOKTrwzZ/JFo=; b=yiISDjkdAOEaQcAdfy/+27BqMaiPr5Zdz0FhusD4XhvUwvPXTSBitpkfgu2VnUqHOY rdntK6+bKgtk7MN/foSqOxNHEfvMr20J8tMCWAOiN/TAvXUQNLBC/DdG7OtoFxvRN53R PLxMvQa+1l/1boCzfiDGjVozPQPZOJli6H9AapNC7aTiUmA93mlZsF8yhZ8ES+H0vBq6 vywsGr7XGkCHTmkvzVqNgrk1vutodPTd2TP9IdW1ivwMG3u8h5hHhvOeYNV0As0If9jK iwsH9HBZEukfNm3QAOaqOOe87RBlplg1DtAQula1dLBx3usMmF7nCIVqF0hHUvaFmj9h BdVg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=BwQwEJLpT43xq/Fvzee5dU07H0aBv3kpOKTrwzZ/JFo=; b=dp4L+gvN97lr6qbymSqTtPUSCoyd/YVNfCASeeWlOaR9KwyWYjq+9bWEzjB6cOHtlv +PaR7CQiFVDZMHM2Ek3oHMibQBjitfUffN6Iw48tnsXJ4cjdBhGdXMnBZ9h1dDzUFxCc RzAol5rKKVITH2Hauuz/3xPZ42L6rKn/d+hxXenCfzYPv1BTOgqef1QgfMhhrJek9+55 a0McZpBn4kAmKc0qt9vgM6iK/smK2IY7MD7kfQzR/N/gMnbRYXssV1u0d+xZnFxcAyEa jEyBjz1wJhezkKjMnElZSm7wUBT+sMmU7aIZFcPu1WLiKy8s8cO8SMtpbiTW3QHvhpD7 S3HA== X-Gm-Message-State: APjAAAWVt4dEEAUcCg1NRDPTHadH/y37ARbYVRh8aTqqhvQtm20viLB+ LI3HzqOsLDSeKJDqkH5jdGXm9end/JBNM57Rdsvfbw== X-Google-Smtp-Source: APXvYqyMpuab5YB/B3Yhu/T3vcm8n/Na9eH4wUOmr/Sc7ww1zvN6/HqnE61IxaM1IJjPBj69HRnqxLfxwMzrvflPrcI= X-Received: by 2002:a0c:ba99:: with SMTP id x25mr7072216qvf.212.1556869784213; Fri, 03 May 2019 00:49:44 -0700 (PDT) MIME-Version: 1.0 References: <1556841016-10342-1-git-send-email-mw@semihalf.com> <1556841016-10342-2-git-send-email-mw@semihalf.com> In-Reply-To: From: "Marcin Wojtas" Date: Fri, 3 May 2019 09:49:30 +0200 Message-ID: Subject: Re: [edk2-platforms: PATCH 1/4] Marvel/Drivers: Pp2Dxe: Basic support for Adapter Information Protocol To: Ard Biesheuvel Cc: edk2-devel-groups-io , Leif Lindholm , =?UTF-8?B?SmFuIETEhWJyb8Wb?= , Grzegorz Jaszczyk , Kostya Porotchkin , Jici Gao , Tomasz Michalec Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hi Ard, pt., 3 maj 2019 o 08:33 Ard Biesheuvel napisa= =C5=82(a): > > On Fri, 3 May 2019 at 01:50, Marcin Wojtas wrote: > > > > From: Tomasz Michalec > > > > 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 > > 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/Ma= rvell/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/Marv= ell/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 > > #include > > #include > > #include > > @@ -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, Ai= p, PP2DXE_SIGNATURE) > > #define INSTANCE_FROM_SNP(a) CR((a), PP2DXE_CONTEXT, Sn= p, 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/Marv= ell/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 a= dapter. > > + If an adapter does not support the requested informational type, the= n > > + EFI_UNSUPPORTED is returned. > > + > > + @param[in] This A pointer to the EFI_ADAPTER_INFO= RMATION_PROTOCOL instance. > > + @param[in] InformationType A pointer to an EFI_GUID that def= ines 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 th= e 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 w= as 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 complete= d 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 =3D=3D NULL || InformationBlock =3D=3D NULL || InformationB= lockSize =3D=3D NULL) { > > + return EFI_INVALID_PARAMETER; > > + } > > + > > + if (!CompareGuid (InformationType, &gEfiAdapterInfoMediaStateGuid)) = { > > + return EFI_UNSUPPORTED; > > + } > > + > > + AdapterInfo =3D AllocateZeroPool (sizeof (EFI_ADAPTER_INFO_MEDIA_STA= TE)); > > + if (AdapterInfo =3D=3D NULL) { > > + return EFI_OUT_OF_RESOURCES; > > + } > > + > > + *InformationBlock =3D AdapterInfo; > > + *InformationBlockSize =3D sizeof (EFI_ADAPTER_INFO_MEDIA_STATE); > > + > > + Pp2Context =3D INSTANCE_FROM_AIP (This); > > + > > + Status =3D Pp2Context->Snp.GetStatus (&(Pp2Context->Snp), NULL, NULL= ); > > + if (Status =3D=3D EFI_NOT_READY){ > > + AdapterInfo->MediaState =3D 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 =3D (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 adapt= er. > > + If an adapter does not support the requested information type, then = EFI_UNSUPPORTED > > + is returned. > > + > > + @param[in] This A pointer to the EFI_ADAPTER_INFO= RMATION_PROTOCOL instance. > > + @param[in] InformationType A pointer to an EFI_GUID that def= ines the contents of InformationBlock. > > + @param[in] InforamtionBlock A pointer to the InformationBlock= structure which contains details > > + about the data specific to Inform= ationType. > > + @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 mod= ified 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 =3D=3D NULL || InformationBlock =3D=3D 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 p= rotocol. > > + > > + This function returns a list of InformationType GUIDs that are suppo= rted on an > > + adapter with this instance of EFI_ADAPTER_INFORMATION_PROTOCOL. The = list is returned > > + in InfoTypesBuffer, and the number of GUID pointers in InfoTypesBuff= er is returned in > > + InfoTypesBufferCount. > > + > > + @param[in] This A pointer to the EFI_ADAPTER_INFOR= MATION_PROTOCOL instance. > > + @param[out] InfoTypesBuffer A pointer to the array of Informat= ionType GUIDs that are supported > > + by This. > > + @param[out] InfoTypesBufferCount A pointer to the number of GUIDs p= resent in InfoTypesBuffer. > > + > > + @retval EFI_SUCCESS The list of information type GUIDs= that are supported on this adapter was > > + returned in InfoTypesBuffer. The n= umber 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 =3D=3D NULL || InfoTypesBuffer =3D=3D NULL || InfoTypesBuff= erCount =3D=3D NULL) { > > + return EFI_INVALID_PARAMETER; > > + } > > + > > + *InfoTypesBuffer =3D AllocateZeroPool (sizeof (EFI_GUID)); > > No need to zero if you assign the whole thing immediately > > > + if (*InfoTypesBuffer =3D=3D NULL) { > > + return EFI_OUT_OF_RESOURCES; > > + } > > + > > + *InfoTypesBufferCount =3D 1; > > + (*InfoTypesBuffer)[0] =3D gEfiAdapterInfoMediaStateGuid; > > + > > Use CopyGuid() not struct assignment > > > + return EFI_SUCCESS; > > +} > > + > > STATIC > > VOID > > Pp2DxeParsePortPcd ( > > @@ -1290,6 +1442,11 @@ Pp2DxeInitialiseController ( > > Pp2Context->Instance =3D DeviceInstance; > > DeviceInstance++; > > > > + /* Prepare AIP Protocol */ > > + Pp2Context->Aip.GetInformation =3D Pp2AipGetInformation; > > + Pp2Context->Aip.SetInformation =3D Pp2AipSetInformation; > > + Pp2Context->Aip.GetSupportedTypes =3D Pp2AipGetSupportedTypes; > > + > > /* Install SNP protocol */ > > Status =3D Pp2DxeSnpInstall(Pp2Context); > > if (EFI_ERROR(Status)) { > > -- > > 2.7.4 > >