From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=pass header.i=@linaro.org header.s=google header.b=XXSfSu23; spf=pass (domain: linaro.org, ip: 209.85.166.66, mailfrom: ard.biesheuvel@linaro.org) Received: from mail-io1-f66.google.com (mail-io1-f66.google.com [209.85.166.66]) by groups.io with SMTP; Thu, 02 May 2019 23:33:35 -0700 Received: by mail-io1-f66.google.com with SMTP id h26so4369744ioj.1 for ; Thu, 02 May 2019 23:33:34 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=pSUL4X7Xhoi0WHSoHoA5iKLJ46mhTLsBfAAU0DtDoJY=; b=XXSfSu23I+s7vHSG35M1gEbkasJlDb9BNWrAGqew9KAImFLxHtpB/e3+j+UMpTPmls 9JQRpRWOJu6KgvdlZSlzE4xarGli+qDbY7GivTI/2JbepE6whXsnt4+t5KpqGzfS11V4 S9hgD8ezWUt+Zh44bwrHmcnnx907GgvqzmMXsdkYvTzLeamvQWWmq++V932/OxpgPJcs sVEyTRhhALKGdc4UeYIFMVELP09278jIQQXpmFOQBXx+/BdB9pSqQmevIHk3CBRKIBWF 0x4GYJ84+vfu/13se5KRrLIJ52mYEpYYrVvi3+WCR1SYDEoSKITkGRAlA2i7xJjuGw5Q 4RTg== 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; bh=pSUL4X7Xhoi0WHSoHoA5iKLJ46mhTLsBfAAU0DtDoJY=; b=jv1gRPX80SjJHgTJDGiXwR+RwOdt/2IGOahiqrQI7uv6vEcU0BJAvrQ91capXzq2rk yNuRiO6KcTtDfveRQWVScy0IsDtOBpZcyVENz2ObEWI58q9TDjOi6S7f6VRU1HGf9QvU cNzTf3e0qsQr9c6rk5kY8qQxOZTJQTl2KCIr0NlrRTF05ji/54APgegP/hR1r1hNOnO4 +ZU9xmEOy6QFKHhcGRKMF8nG5iaCvOTi0GeiC4CyRnJCSp55F9Lr41Uk5oHaczp4vvAH dZjWl8K8txeDTras9JfVrkrki1+ttZVookrwbJaF0IjCXOGkc1vuauHw8+xF7oYKOQcX EFTA== X-Gm-Message-State: APjAAAWZQgbtFUYw2SrX2BTBnGpRojuezGj8uqGHT+mTQ8qZ4xpMFapA 87QaAT2afIC4sjWygeUgJrjkPdPFr4UF35TiI1WQYg== X-Google-Smtp-Source: APXvYqx9F9PVbL8Z/IKzLB3UfCXjX6Kn5prnn8KWUtFjZ26vJyocDlFq9btV067W0XicGnSP/lIBex8zv4nE5k49Nu4= X-Received: by 2002:a5e:9b17:: with SMTP id j23mr6006195iok.60.1556865214201; Thu, 02 May 2019 23:33:34 -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: <1556841016-10342-2-git-send-email-mw@semihalf.com> From: "Ard Biesheuvel" Date: Fri, 3 May 2019 08:33:22 +0200 Message-ID: Subject: Re: [edk2-platforms: PATCH 1/4] Marvel/Drivers: Pp2Dxe: Basic support for Adapter Information Protocol To: Marcin Wojtas 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" 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/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 > #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, 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 >