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

["Tinh Nguyen" <tinhnguyen@os.amperecomputing.com>]

On 1/12/2023 3:36 PM, Richard Ho (何明忠) wrote:
> Hi Michael,
>
> We add this patch in my X86 platform and use the NCM device to test IPV4 PXE boot from 1330MB ISO file.
>
> No this patch: 35 sec to download 1330M ISO file
> Add this patch: 181  sec to download 1330M ISO file
>
> The patch will increase boot time in my X86 platform.
>
> Thanks,
> Richard
>
> -----Original Message-----
> From: Michael Brown <mcb30@ipxe.org>
> Sent: 2023年1月10日 7:50 AM
> To: devel@edk2.groups.io; tinhnguyen@os.amperecomputing.com; Richard Ho (何明忠) <RichardHo@ami.com>
> Subject: [EXTERNAL] Re: [edk2-devel] [PATCH 1/3] UsbNetworkPkg/UsbRndis: Add USB RNDIS devices support
>
>
> **CAUTION: The e-mail below is from an external source. Please exercise caution before opening attachments, clicking links, or following guidance.**
>
> 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
>
> -The information contained in this message may be confidential and proprietary to American Megatrends (AMI). This communication is intended to be read only by the individual or entity to whom it is addressed or by their designee. If the reader of this message is not the intended recipient, you are on notice that any distribution of this message, in any form, is strictly prohibited. Please promptly notify the sender by reply e-mail or by telephone at 770-246-8600, and then delete or destroy all copies of the transmission.

Hi,

Sorry for the late response to this thread.  My platform cannot utilize 
this driver without this fix, whether we can support a PCD to 
enable/disable this rate limiting?

Thanks,

- Tinh