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.web12.2756.1592344538440063930 for ; Tue, 16 Jun 2020 14:55:38 -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 D77181FB; Tue, 16 Jun 2020 14:55:36 -0700 (PDT) Received: from [192.168.1.69] (unknown [10.37.8.36]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id D1E8A3F71F; Tue, 16 Jun 2020 14:55:35 -0700 (PDT) Subject: Re: [PATCH edk2-platforms] Silicon/Broadcom/BcmGenetDxe: implement media state adapter info protocol To: devel@edk2.groups.io Cc: Pete Batard , Andrei Warkentin , Samer El-Haj-Mahmoud References: <20200616181712.1124353-1-ard.biesheuvel@arm.com> From: "Ard Biesheuvel" Message-ID: <8e2530ba-f356-cd0d-d552-b8e09fe90097@arm.com> Date: Tue, 16 Jun 2020 23:55:26 +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: <20200616181712.1124353-1-ard.biesheuvel@arm.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit On 6/16/20 8:17 PM, Ard Biesheuvel wrote: > 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 > --- Note that this code was build tested only, and turns out not to work as expected. Samer and I are collaborating off-list, I will follow up with a v2 once we have code that actually works. > Silicon/Broadcom/Drivers/Net/BcmGenetDxe/BcmGenetDxe.inf | 3 + > Silicon/Broadcom/Drivers/Net/BcmGenetDxe/BcmGenetDxe.h | 4 + > Silicon/Broadcom/Drivers/Net/BcmGenetDxe/AdapterInfo.c | 109 ++++++++++++++++++++ > Silicon/Broadcom/Drivers/Net/BcmGenetDxe/DriverBinding.c | 12 ++- > 4 files changed, 123 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..48bb8550426f 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,6 +237,7 @@ 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) > diff --git a/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/AdapterInfo.c b/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/AdapterInfo.c > new file mode 100644 > index 000000000000..cb7733bbba76 > --- /dev/null > +++ b/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/AdapterInfo.c > @@ -0,0 +1,109 @@ > +/** @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_SNP_THIS (This); > + if (Genet->Snp.Mode->MediaPresent) { > + AdapterInfo->MediaState = EFI_SUCCESS; > + } else { > + AdapterInfo->MediaState = EFI_NOT_READY; > + } > + 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; > } >