Re: [edk2-devel] [PATCH 1/3] UsbNetworkPkg/UsbRndis: Add USB RNDIS devices support

["Michael Brown" <mcb30@ipxe.org>]

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