From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mx.groups.io with SMTP id smtpd.web10.4606.1592476498563018564 for ; Thu, 18 Jun 2020 03:34:59 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: arm.com, ip: 217.140.110.172, mailfrom: ard.biesheuvel@arm.com) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 2C52831B; Thu, 18 Jun 2020 03:34:57 -0700 (PDT) Received: from [192.168.1.69] (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 50FCE3F6CF; Thu, 18 Jun 2020 03:34:54 -0700 (PDT) Subject: Re: [PATCH edk2-platforms v2] Silicon/Broadcom/BcmGenetDxe: implement media state adapter info protocol To: Samer El-Haj-Mahmoud , "devel@edk2.groups.io" Cc: Pete Batard , "Andrei Warkentin (awarkentin@vmware.com)" References: <20200617181501.1184090-1-ard.biesheuvel@arm.com> From: "Ard Biesheuvel" Message-ID: <2f7dd937-82e5-3f6c-c41e-ef474acddac5@arm.com> Date: Thu, 18 Jun 2020 12:34:42 +0200 User-Agent: Mozilla/5.0 (X11; Linux aarch64; rv:68.0) Gecko/20100101 Thunderbird/68.9.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit On 6/17/20 8:22 PM, Samer El-Haj-Mahmoud wrote: > Tested on RPi4 w/ 4GB > > Reviewed-by: Samer El-Haj-Mahmoud > Thanks Pushed as 743f501b66db712d486347ddca781ffc705c1766 >> -----Original Message----- >> From: Ard Biesheuvel >> Sent: Wednesday, June 17, 2020 2:15 PM >> To: devel@edk2.groups.io >> Cc: Ard Biesheuvel ; Pete Batard >> ; Andrei Warkentin (awarkentin@vmware.com) >> ; Samer El-Haj-Mahmoud > Mahmoud@arm.com> >> Subject: [PATCH edk2-platforms v2] Silicon/Broadcom/BcmGenetDxe: >> implement media state adapter info protocol >> >> NetLibDetectMedia () in DxeNetLib is used as a fallback on implementations >> of the SNP protocol that do not also carry an implementation of the adapter >> info protocol to provide media state information, and it does all kinds of >> terrible things to the network interface (stopping and restarting multiple >> times, reprogramming the multicast filters etc etc) to workaround some >> alleged UNDI shortcoming. >> >> Although our GENET code should be bullet proof and therefore able to take >> this kind of abuse, it is better to avoid it, and provide an implementation >> of the adapter info protocol that returns the media state directly, without >> the need to mistreat the SNP layer. >> >> >> Cc: Pete Batard >> >> Cc: Andrei Warkentin (awarkentin@vmware.com) >> >> >> Cc: Samer El-Haj-Mahmoud >> >> Signed-off-by: Ard Biesheuvel >> --- >> >> v2: >> >> - use the correct accessor to retrieve the GENET_PRIVATE_DATA pointer >> >> - call into the PHY routines to get the state, to ensure it is not stale >> >> >> Silicon/Broadcom/Drivers/Net/BcmGenetDxe/BcmGenetDxe.inf | 3 + >> Silicon/Broadcom/Drivers/Net/BcmGenetDxe/BcmGenetDxe.h | 5 + >> Silicon/Broadcom/Drivers/Net/BcmGenetDxe/AdapterInfo.c | 106 >> ++++++++++++++++++++ >> Silicon/Broadcom/Drivers/Net/BcmGenetDxe/DriverBinding.c | 12 ++- >> 4 files changed, 121 insertions(+), 5 deletions(-) >> >> diff --git a/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/BcmGenetDxe.inf >> b/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/BcmGenetDxe.inf >> index 28f3e66ebaf0..89dee9f10c83 100644 >> --- a/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/BcmGenetDxe.inf >> +++ b/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/BcmGenetDxe.inf >> @@ -19,6 +19,7 @@ [Defines] >> UNLOAD_IMAGE = GenetUnload >> >> >> >> [Sources] >> >> + AdapterInfo.c >> >> BcmGenetDxe.h >> >> ComponentName.c >> >> DriverBinding.c >> >> @@ -49,10 +50,12 @@ [LibraryClasses] >> >> >> [Protocols] >> >> gBcmGenetPlatformDeviceProtocolGuid ## TO_START >> >> + gEfiAdapterInformationProtocolGuid ## BY_START >> >> gEfiDevicePathProtocolGuid ## BY_START >> >> gEfiSimpleNetworkProtocolGuid ## BY_START >> >> >> >> [Guids] >> >> + gEfiAdapterInfoMediaStateGuid >> >> gEfiEventExitBootServicesGuid >> >> >> >> [FixedPcd] >> >> diff --git a/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/BcmGenetDxe.h >> b/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/BcmGenetDxe.h >> index 0af9d5209cf2..1a117b25f87f 100644 >> --- a/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/BcmGenetDxe.h >> +++ b/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/BcmGenetDxe.h >> @@ -14,6 +14,7 @@ >> #include >> >> #include >> >> #include >> >> +#include >> >> #include >> >> #include >> >> #include >> >> @@ -209,6 +210,8 @@ typedef struct { >> EFI_SIMPLE_NETWORK_PROTOCOL Snp; >> >> EFI_SIMPLE_NETWORK_MODE SnpMode; >> >> >> >> + EFI_ADAPTER_INFORMATION_PROTOCOL Aip; >> >> + >> >> BCM_GENET_PLATFORM_DEVICE_PROTOCOL *Dev; >> >> >> >> GENERIC_PHY_PRIVATE_DATA Phy; >> >> @@ -234,9 +237,11 @@ extern EFI_COMPONENT_NAME_PROTOCOL >> gGenetComponentName; >> extern EFI_COMPONENT_NAME2_PROTOCOL >> gGenetComponentName2; >> >> >> >> extern CONST EFI_SIMPLE_NETWORK_PROTOCOL >> gGenetSimpleNetworkTemplate; >> >> +extern CONST EFI_ADAPTER_INFORMATION_PROTOCOL >> gGenetAdapterInfoTemplate; >> >> >> >> #define GENET_DRIVER_SIGNATURE SIGNATURE_32('G', 'N', 'E', 'T') >> >> #define GENET_PRIVATE_DATA_FROM_SNP_THIS(a) CR(a, >> GENET_PRIVATE_DATA, Snp, GENET_DRIVER_SIGNATURE) >> >> +#define GENET_PRIVATE_DATA_FROM_AIP_THIS(a) CR(a, >> GENET_PRIVATE_DATA, Aip, GENET_DRIVER_SIGNATURE) >> >> >> >> #define GENET_RX_BUFFER(g, idx) ((UINT8 *)(UINTN)(g)->RxBuffer + >> GENET_MAX_PACKET_SIZE * (idx)) >> >> >> >> diff --git a/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/AdapterInfo.c >> b/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/AdapterInfo.c >> new file mode 100644 >> index 000000000000..4b42b2bba67a >> --- /dev/null >> +++ b/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/AdapterInfo.c >> @@ -0,0 +1,106 @@ >> +/** @file >> >> + >> >> + Copyright (c) 2020 Arm, Limited. All rights reserved. >> >> + >> >> + SPDX-License-Identifier: BSD-2-Clause-Patent >> >> + >> >> +**/ >> >> + >> >> +#include >> >> +#include >> >> +#include >> >> +#include >> >> + >> >> +#include "BcmGenetDxe.h" >> >> + >> >> +STATIC >> >> +EFI_STATUS >> >> +EFIAPI >> >> +GenetAipGetInformation ( >> >> + IN EFI_ADAPTER_INFORMATION_PROTOCOL *This, >> >> + IN EFI_GUID *InformationType, >> >> + OUT VOID **InformationBlock, >> >> + OUT UINTN *InformationBlockSize >> >> + ) >> >> +{ >> >> + EFI_ADAPTER_INFO_MEDIA_STATE *AdapterInfo; >> >> + GENET_PRIVATE_DATA *Genet; >> >> + >> >> + 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); >> >> + >> >> + Genet = GENET_PRIVATE_DATA_FROM_AIP_THIS (This); >> >> + AdapterInfo->MediaState = GenericPhyUpdateConfig (&Genet->Phy); >> >> + >> >> + return EFI_SUCCESS; >> >> +} >> >> + >> >> +STATIC >> >> +EFI_STATUS >> >> +EFIAPI >> >> +GenetAipSetInformation ( >> >> + 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; >> >> +} >> >> + >> >> +STATIC >> >> +EFI_STATUS >> >> +EFIAPI >> >> +GenetAipGetSupportedTypes ( >> >> + IN EFI_ADAPTER_INFORMATION_PROTOCOL *This, >> >> + OUT EFI_GUID **InfoTypesBuffer, >> >> + OUT UINTN *InfoTypesBufferCount >> >> + ) >> >> +{ >> >> + EFI_GUID *Guid; >> >> + >> >> + if (This == NULL || InfoTypesBuffer == NULL || >> >> + InfoTypesBufferCount == NULL) { >> >> + return EFI_INVALID_PARAMETER; >> >> + } >> >> + >> >> + Guid = AllocatePool (sizeof *Guid); >> >> + if (Guid == NULL) { >> >> + return EFI_OUT_OF_RESOURCES; >> >> + } >> >> + >> >> + CopyGuid (Guid, &gEfiAdapterInfoMediaStateGuid); >> >> + >> >> + *InfoTypesBuffer = Guid; >> >> + *InfoTypesBufferCount = 1; >> >> + >> >> + return EFI_SUCCESS; >> >> +} >> >> + >> >> +CONST EFI_ADAPTER_INFORMATION_PROTOCOL >> gGenetAdapterInfoTemplate = { >> >> + GenetAipGetInformation, >> >> + GenetAipSetInformation, >> >> + GenetAipGetSupportedTypes, >> >> +}; >> >> diff --git a/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/DriverBinding.c >> b/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/DriverBinding.c >> index 7f93c68cd608..f9aa006dc799 100644 >> --- a/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/DriverBinding.c >> +++ b/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/DriverBinding.c >> @@ -163,6 +163,7 @@ GenetDriverBindingStart ( >> >> >> EfiInitializeLock (&Genet->Lock, TPL_CALLBACK); >> >> CopyMem (&Genet->Snp, &gGenetSimpleNetworkTemplate, sizeof Genet- >>> Snp); >> >> + CopyMem (&Genet->Aip, &gGenetAdapterInfoTemplate, sizeof Genet- >>> Aip); >> >> >> >> Genet->Snp.Mode = &Genet->SnpMode; >> >> Genet->SnpMode.State = EfiSimpleNetworkStopped; >> >> @@ -203,7 +204,8 @@ GenetDriverBindingStart ( >> } >> >> >> >> Status = gBS->InstallMultipleProtocolInterfaces (&ControllerHandle, >> >> - &gEfiSimpleNetworkProtocolGuid, &Genet->Snp, >> >> + &gEfiSimpleNetworkProtocolGuid, &Genet->Snp, >> >> + &gEfiAdapterInformationProtocolGuid, &Genet->Aip, >> >> NULL); >> >> >> >> if (EFI_ERROR (Status)) { >> >> @@ -273,10 +275,10 @@ GenetDriverBindingStop ( >> >> >> ASSERT (Genet->ControllerHandle == ControllerHandle); >> >> >> >> - Status = gBS->UninstallProtocolInterface (ControllerHandle, >> >> - &gEfiSimpleNetworkProtocolGuid, >> >> - &Genet->Snp >> >> - ); >> >> + Status = gBS->UninstallMultipleProtocolInterfaces (ControllerHandle, >> >> + &gEfiSimpleNetworkProtocolGuid, &Genet->Snp, >> >> + &gEfiAdapterInformationProtocolGuid, &Genet->Aip, >> >> + NULL); >> >> if (EFI_ERROR (Status)) { >> >> return Status; >> >> } >> >> -- >> 2.27.0 >