From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from a7-17.smtp-out.eu-west-1.amazonses.com (a7-17.smtp-out.eu-west-1.amazonses.com [54.240.7.17]) by mx.groups.io with SMTP id smtpd.web10.11931.1673399484336287176 for ; Tue, 10 Jan 2023 17:11:24 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@ipxe.org header.s=cphpx6z2rfcgehlykjjh3gknqe3hsoe2 header.b=d2RBqWbr; spf=pass (domain: eu-west-1.amazonses.com, ip: 54.240.7.17, mailfrom: 010201859e61b891-f355ea1c-08c2-4a5f-8568-f47097af6994-000000@eu-west-1.amazonses.com) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/simple; s=cphpx6z2rfcgehlykjjh3gknqe3hsoe2; d=ipxe.org; t=1673399482; h=Message-ID:Date:MIME-Version:Subject:From:To:References:In-Reply-To:Content-Type:Content-Transfer-Encoding; bh=3h3w06kiv8Asw9FQCJ4sMBSF2pso6BG6WVsNu9VY5vo=; b=d2RBqWbr/kpKNrBQJZyRBqsfLR2QbXUCDVMUOdvfRhRDbpdPVoNDZA8S7iFSzPaN py2MfbrJUFyUpmzYDlHNoMVaPL0KoHIBQXB90iYxWJQ/rItfwq4EEPz/U0fMieJdq3F M3apP3B1CehSsX4YXvLg3FYsTlRZvod1NGo+LbEudOSJuEmZkn3St/SRpiE/1yvosod lgUIcYIPAsDMTJlNGXkWtYDpPIfbHRX4R+i+qOtof3FUQdLosjHMYtJwnQ6pPzqD4yd 7snVRuKg/vObVxl/7pWeuY/UKQh5DINAEXH2TJ7x1O2gGIr8NyD8F6PmIZA9CyVFPdk Ww2H9Ydj4g== DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/simple; s=ihchhvubuqgjsxyuhssfvqohv7z3u4hn; d=amazonses.com; t=1673399482; h=Message-ID:Date:MIME-Version:Subject:From:To:References:In-Reply-To:Content-Type:Content-Transfer-Encoding:Feedback-ID; bh=3h3w06kiv8Asw9FQCJ4sMBSF2pso6BG6WVsNu9VY5vo=; b=3wVtdUemCoiX+Elo6KOly3zOLDSBI1BtzJmnfx55oUobFbE3gGekdM1+0VNa3PG+ yaxqLY/dWUdaooG6b97SjO4EqAnHeu8tSpgLzMeNANKXQB5UkYzk7qLj6OjeVKqhsmW NLNZ2phRlC9ybpbis05EyviomNsidUpdNdnvILJw= Message-ID: <010201859e61b891-f355ea1c-08c2-4a5f-8568-f47097af6994-000000@eu-west-1.amazonses.com> Date: Wed, 11 Jan 2023 01:11:22 +0000 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.6.0 Subject: Re: [edk2-devel] [PATCH 1/3] UsbNetworkPkg/UsbRndis: Add USB RNDIS devices support From: "Michael Brown" To: devel@edk2.groups.io, =?UTF-8?B?UmljaGFyZEhvIFvkvZXmmI7lv6Bd?= References: <1271.1673174497155150644@groups.io> <53d2211e-d8ad-fafe-38ad-814ed77d19cf@ipxe.org> In-Reply-To: <53d2211e-d8ad-fafe-38ad-814ed77d19cf@ipxe.org> X-Spam-Status: No, score=-2.9 required=5.0 tests=ALL_TRUSTED,BAYES_00 autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on blyat.fensystems.co.uk Feedback-ID: 1.eu-west-1.fspj4M/5bzJ9NLRzJP0PaxRwxrpZqiDQJ1IF94CF2TA=:AmazonSES X-SES-Outgoing: 2023.01.11-54.240.7.17 Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit On 09/01/2023 23:50, Michael Brown wrote: > Coincidentally, I have just implemented a fix for this.  It works by > implementing a simple credit-based rate limiter.  Calls to > UsbEthReceive() are limited to 10 per second while the receive datapath > is idle.  No limiting takes place while the receive datapath is active. > > Richard Ho: please consider using this rate limiting approach (or > something very similar). I have updated the patch to run the timer callback at TPL_NOTIFY. This seems to be required to avoid a long delay when the timer is disabled. (I have not investigated the root cause of this delay, but it is 100% reproducible.) With this updated patch, I get almost no noticeable slowdown of the system and I am still able to receive data at around 100Mbps (which is about as good as it's likely to get, given the fundamental design flaws in EFI_USB_IO_PROTOCOL). Updated patch: diff --git a/UsbNetworkPkg/Include/Protocol/EdkIIUsbEthernetProtocol.h b/UsbNetworkPkg/Include/Protocol/EdkIIUsbEthernetProtocol.h index df6ebc64ef..d0e2048114 100644 --- a/UsbNetworkPkg/Include/Protocol/EdkIIUsbEthernetProtocol.h +++ b/UsbNetworkPkg/Include/Protocol/EdkIIUsbEthernetProtocol.h @@ -160,6 +160,8 @@ typedef struct { UINT8 CurrentNodeAddress[PXE_MAC_LENGTH]; UINT8 BroadcastNodeAddress[PXE_MAC_LENGTH]; EFI_USB_DEVICE_REQUEST Request; + EFI_EVENT RateLimiter; + UINTN RateLimitCredit; } NIC_DATA; #define NIC_DATA_SIGNATURE SIGNATURE_32('n', 'i', 'c', 'd') diff --git a/UsbNetworkPkg/NetworkCommon/DriverBinding.h b/UsbNetworkPkg/NetworkCommon/DriverBinding.h index 946727ffc9..ae1d3c201e 100644 --- a/UsbNetworkPkg/NetworkCommon/DriverBinding.h +++ b/UsbNetworkPkg/NetworkCommon/DriverBinding.h @@ -25,6 +25,8 @@ #define RX_BUFFER_COUNT 32 #define TX_BUFFER_COUNT 32 #define MEMORY_REQUIRE 0 +#define RATE_LIMITER_INTERVAL 1000000 // 10Hz +#define RATE_LIMITER_BURST 10 #define UNDI_DEV_SIGNATURE SIGNATURE_32('u','n','d','i') #define UNDI_DEV_FROM_THIS(a) CR(a, NIC_DEVICE, NiiProtocol, UNDI_DEV_SIGNATURE) diff --git a/UsbNetworkPkg/NetworkCommon/PxeFunction.c b/UsbNetworkPkg/NetworkCommon/PxeFunction.c index fd53a215a4..e02402dc57 100644 --- a/UsbNetworkPkg/NetworkCommon/PxeFunction.c +++ b/UsbNetworkPkg/NetworkCommon/PxeFunction.c @@ -29,6 +29,21 @@ API_FUNC gUndiApiTable[] = { UndiReceive }; +STATIC +VOID +EFIAPI +UndiRateLimiterCallback ( + IN EFI_EVENT Event, + IN VOID *Context + ) +{ + NIC_DATA *Nic = Context; + + if (Nic->RateLimitCredit < RATE_LIMITER_BURST) { + Nic->RateLimitCredit++; + } +} + /** This command is used to determine the operational state of the UNDI. @@ -100,9 +115,6 @@ UndiStart ( Cdb->StatFlags = PXE_STATFLAGS_COMMAND_FAILED; Cdb->StatCode = PXE_STATCODE_INVALID_CDB; return; - } else { - Cdb->StatFlags = PXE_STATFLAGS_COMMAND_COMPLETE; - Cdb->StatCode = PXE_STATCODE_SUCCESS; } if (Nic->State != PXE_STATFLAGS_GET_STATE_STOPPED) { @@ -120,14 +132,46 @@ UndiStart ( Nic->PxeStart.UnMap_Mem = 0; Nic->PxeStart.Sync_Mem = Cpb->Sync_Mem; Nic->PxeStart.Unique_ID = Cpb->Unique_ID; - Nic->State = PXE_STATFLAGS_GET_STATE_STARTED; + + Status = gBS->CreateEvent ( + EVT_TIMER | EVT_NOTIFY_SIGNAL, + TPL_NOTIFY, + UndiRateLimiterCallback, + Nic, + &Nic->RateLimiter + ); + if (EFI_ERROR (Status)) { + goto ErrorCreateEvent; + } + + Status = gBS->SetTimer ( + Nic->RateLimiter, + TimerPeriodic, + RATE_LIMITER_INTERVAL + ); + if (EFI_ERROR (Status)) { + goto ErrorSetTimer; + } if (Nic->UsbEth->UsbEthUndi.UsbEthUndiStart != NULL) { Status = Nic->UsbEth->UsbEthUndi.UsbEthUndiStart (Cdb, Nic); if (EFI_ERROR (Status)) { - Cdb->StatFlags = PXE_STATFLAGS_COMMAND_FAILED; + goto ErrorUndiStart; } } + + Nic->State = PXE_STATFLAGS_GET_STATE_STARTED; + Cdb->StatFlags = PXE_STATFLAGS_COMMAND_COMPLETE; + Cdb->StatCode = PXE_STATCODE_SUCCESS; + return; + + ErrorUndiStart: + gBS->SetTimer (&Nic->RateLimiter, TimerCancel, 0); + ErrorSetTimer: + gBS->CloseEvent (&Nic->RateLimiter); + ErrorCreateEvent: + Cdb->StatFlags = PXE_STATFLAGS_COMMAND_FAILED; + Cdb->StatCode = PXE_STATCODE_DEVICE_FAILURE; } /** @@ -183,6 +227,10 @@ UndiStop ( Nic->PxeStart.Sync_Mem = 0; Nic->State = PXE_STATFLAGS_GET_STATE_STOPPED; + gBS->SetTimer (&Nic->RateLimiter, TimerCancel, 0); + + gBS->CloseEvent (&Nic->RateLimiter); + if (Nic->UsbEth->UsbEthUndi.UsbEthUndiStop != NULL) { Status = Nic->UsbEth->UsbEthUndi.UsbEthUndiStop (Cdb, Nic); if (EFI_ERROR (Status)) { @@ -1483,6 +1531,7 @@ Receive ( ) { EFI_STATUS Status; + EFI_TPL OriginalTpl; UINTN Index; UINT16 StatCode; PXE_FRAME_TYPE FrameType; @@ -1506,8 +1555,15 @@ Receive ( } while (1) { + if (Nic->RateLimitCredit == 0) { + break; + } + Status = Nic->UsbEth->UsbEthReceive (Cdb, Nic->UsbEth, (VOID *)BulkInData, &DataLength); if (EFI_ERROR (Status)) { + OriginalTpl = gBS->RaiseTPL (TPL_NOTIFY); + Nic->RateLimitCredit--; + gBS->RestoreTPL (OriginalTpl); break; } Thanks, Michael