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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#98206): https://edk2.groups.io/g/devel/message/98206
Mute This Topic: https://groups.io/mt/95531719/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
