From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from a7-18.smtp-out.eu-west-1.amazonses.com (a7-18.smtp-out.eu-west-1.amazonses.com [54.240.7.18]) by mx.groups.io with SMTP id smtpd.web11.90565.1673308219915042905 for ; Mon, 09 Jan 2023 15:50:20 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@ipxe.org header.s=cphpx6z2rfcgehlykjjh3gknqe3hsoe2 header.b=fKhDWjuF; spf=pass (domain: eu-west-1.amazonses.com, ip: 54.240.7.18, mailfrom: 0102018598f12261-59c6c0fd-55a7-4b2d-9517-0470d71bb36a-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=1673308218; h=Message-ID:Date:MIME-Version:Subject:To:References:From:In-Reply-To:Content-Type:Content-Transfer-Encoding; bh=LnhvT2fgpmnp2kJviGxK4LkUPkolM2Ks2Z7qByMczek=; b=fKhDWjuFp1CIcED/rKU19MgQE8j1jxbX9XzzCfUhEB+Sh3YzLuDFebq14F7/yMbI ex73NPRTZWahwjlPLzcWWrL6gRJ8CmJFQdXDN5FDOiIMyq8pN5cJoAV0p4SDWTqzgjh FmP9S5nNaWknTgNTzgHK6v/2HWYOklrskS657ZgO6SkzEsaeSCBRARpq+qZmFjtfAfv Fx6Ryh3aR8yEn1BY4/bzNjw6FX+w69isVRRaAyAuY2zkFjmnm66ZdN7pqV+BrKOBF5u AMUGL+hSOVjt8tl5IQDNmCTOu9kG/8in2N4/+8tDgn5oUlaeEMrQe6xNqTFqWLE9DXz 3e2CBDAYcA== DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/simple; s=ihchhvubuqgjsxyuhssfvqohv7z3u4hn; d=amazonses.com; t=1673308218; h=Message-ID:Date:MIME-Version:Subject:To:References:From:In-Reply-To:Content-Type:Content-Transfer-Encoding:Feedback-ID; bh=LnhvT2fgpmnp2kJviGxK4LkUPkolM2Ks2Z7qByMczek=; b=VrT0Efl23lHSELWlMpg4KOxf7TuJAksjon6mWR/S1GWzEoEsChnzyUxPuL6M5QgK oOb+c/76921yzAP0RFkEVbWokSVXTkjzZ8maciDSQQFP2pL0JHdTDNRAWvqLcIv2hWk bD+opgD+z0MLkyRlV2tdCw4HRg7Wcvivw3Ot/P3c= Message-ID: <0102018598f12261-59c6c0fd-55a7-4b2d-9517-0470d71bb36a-000000@eu-west-1.amazonses.com> Date: Mon, 9 Jan 2023 23:50:17 +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 To: devel@edk2.groups.io, tinhnguyen@os.amperecomputing.com, =?UTF-8?B?UmljaGFyZEhvIFvkvZXmmI7lv6Bd?= References: <1271.1673174497155150644@groups.io> From: "Michael Brown" In-Reply-To: <1271.1673174497155150644@groups.io> X-Spam-Status: No, score=-2.9 required=5.0 tests=ALL_TRUSTED,BAYES_00, URIBL_DBL_BLOCKED_OPENDNS,URIBL_ZEN_BLOCKED_OPENDNS 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.09-54.240.7.18 Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit On 08/01/2023 10:41, tinhnguyen via groups.io wrote: > The root cause is that we spend a long time waiting for USB > UsbBulkTransfer to complete, but if there is no data to communicate > -> it will always time out. 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. I have tried to match the existing code style (i.e. zero explanatory comments). Richard Ho: please consider using this rate limiting approach (or something very similar). Patch inline below: 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..2a9b4f6111 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_CALLBACK, + 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)) { @@ -1506,8 +1554,13 @@ Receive ( } while (1) { + if (Nic->RateLimitCredit == 0) { + break; + } + Status = Nic->UsbEth->UsbEthReceive (Cdb, Nic->UsbEth, (VOID *)BulkInData, &DataLength); if (EFI_ERROR (Status)) { + Nic->RateLimitCredit--; break; } Thanks, Michael