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.4623.1589056780269730971 for ; Sat, 09 May 2020 13:39:40 -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 DEE8D1FB; Sat, 9 May 2020 13:39:38 -0700 (PDT) Received: from [192.168.1.81] (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 57A363F305; Sat, 9 May 2020 13:39:37 -0700 (PDT) Subject: Re: [PATCH edk2-platforms 1/1] Silicon/Broadcom/BcmGenetDxe: fix multicast handling To: Andrei Warkentin Cc: "devel@edk2.groups.io" , Pete Batard , Jared McNeill , Samer El-Haj-Mahmoud References: <20200509110609.21104-1-ard.biesheuvel@arm.com> <3008B116-5369-4FF8-B829-B5A8B030D054@vmware.com> From: "Ard Biesheuvel" Message-ID: <7ccfd32c-bb1d-37da-dd3a-60a9bbcefbf9@arm.com> Date: Sat, 9 May 2020 22:39:34 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.7.0 MIME-Version: 1.0 In-Reply-To: <3008B116-5369-4FF8-B829-B5A8B030D054@vmware.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: quoted-printable On 5/9/20 9:34 PM, Andrei Warkentin wrote: > Hi Ard, >=20 > Could we upgrade broadcast to promisc? The spec allows using a =E2=80=9C= wider=E2=80=9D filter. >=20 Eh, are you sure the hardware needs the promiscuous mode enabled to=20 receive broadcast frames? > Could we treat multicast the same way? So any non-unicast filter means = promisc? >=20 The MNP layer already takes care of that. >> 9 =D0=BC=D0=B0=D1=8F 2020 =D0=B3., =D0=B2 6:06 AM, Ard Biesheuvel =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB(=D0=B0): >> >> =EF=BB=BFMove the handling of the promiscuous receive control to the S= NP >> ReceiveFilters() method where it belongs. Given that we do not >> support multicast filtering, only minimal handling is required to >> test the promiscuous bit and program the MAC accordingly. Any >> multicast handling will be done by the MNP layer above it. >> >> Cc: Pete Batard >> Cc: Jared McNeill >> Cc: Andrei Warkentin >> Cc: Samer El-Haj-Mahmoud >> Signed-off-by: Ard Biesheuvel >> --- >> Silicon/Broadcom/Drivers/Net/BcmGenetDxe/DriverBinding.c | 7 +++---- >> Silicon/Broadcom/Drivers/Net/BcmGenetDxe/SimpleNetwork.c | 17 ++++++--= --------- >> 2 files changed, 9 insertions(+), 15 deletions(-) >> >> diff --git a/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/DriverBinding.c = b/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/DriverBinding.c >> index e3d015dd0820..a6102421cc26 100644 >> --- a/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/DriverBinding.c >> +++ b/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/DriverBinding.c >> @@ -154,11 +154,10 @@ GenetDriverBindingStart ( >> Genet->SnpMode.NvRamSize =3D 0; >> Genet->SnpMode.NvRamAccessSize =3D 0; >> Genet->SnpMode.ReceiveFilterMask =3D EFI_SIMPLE_NETWORK_RECEIV= E_UNICAST | >> - EFI_SIMPLE_NETWORK_RECEIVE_= MULTICAST | >> EFI_SIMPLE_NETWORK_RECEIVE_= BROADCAST | >> - EFI_SIMPLE_NETWORK_RECEIVE_= PROMISCUOUS | >> - EFI_SIMPLE_NETWORK_RECEIVE_= PROMISCUOUS_MULTICAST; >> - Genet->SnpMode.ReceiveFilterSetting =3D Genet->SnpMode.ReceiveFil= terMask; >> + EFI_SIMPLE_NETWORK_RECEIVE_= PROMISCUOUS; >> + Genet->SnpMode.ReceiveFilterSetting =3D EFI_SIMPLE_NETWORK_RECEIV= E_UNICAST | >> + EFI_SIMPLE_NETWORK_RECEIVE_= BROADCAST; >> Genet->SnpMode.MaxMCastFilterCount =3D 0; >> Genet->SnpMode.MCastFilterCount =3D 0; >> Genet->SnpMode.IfType =3D NET_IFTYPE_ETHERNET; >> diff --git a/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/SimpleNetwork.c = b/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/SimpleNetwork.c >> index bf28448445d1..9ea153780538 100644 >> --- a/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/SimpleNetwork.c >> +++ b/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/SimpleNetwork.c >> @@ -148,10 +148,6 @@ GenetSimpleNetworkInitialize ( >> } >> >> GenetSetMacAddress (Genet, &Genet->SnpMode.CurrentAddress); >> - /* >> - * TODO: this belongs in GenetSimpleNetworkReceiveFilters, not here= . >> - */ >> - GenetSetPromisc (Genet, TRUE); >> >> GenetDmaInitRings (Genet); >> >> @@ -306,6 +302,10 @@ GenetSimpleNetworkReceiveFilters ( >> } >> >> Genet =3D GENET_PRIVATE_DATA_FROM_SNP_THIS (This); >> + if (((Enable | Disable) & ~Genet->SnpMode.ReceiveFilterMask) !=3D 0= || >> + (!ResetMCastFilter && MCastFilterCnt > Genet->SnpMode.MaxMCastF= ilterCount)) { >> + return EFI_INVALID_PARAMETER; >> + } >> if (Genet->SnpMode.State =3D=3D EfiSimpleNetworkStopped) { >> return EFI_NOT_STARTED; >> } >> @@ -313,13 +313,8 @@ GenetSimpleNetworkReceiveFilters ( >> return EFI_DEVICE_ERROR; >> } >> >> - // >> - // Can't actually return EFI_UNSUPPORTED, so just ignore the filter= s >> - // (we set promiscuous mode ON inside GenetSimpleNetworkInitialize)= . >> - // >> - // TODO: move promisc handling here. >> - // TODO 2: support multicast/broadcast. >> - // >> + GenetSetPromisc (Genet, >> + (Enable & ~Disable & EFI_SIMPLE_NETWORK_RECEIVE_PROMISCUOUS) !=3D= 0); >> >> return EFI_SUCCESS; >> } >> --=20 >> 2.17.1 >>