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

["RichardHo [何明忠]" <richardho@ami.com>]

Hi Michael,

If select EnableRateLimiting  to TRUE and use the X86 system. It will be hang when PXE boot.
Could you help to check?

Thanks,
Richard

-----Original Message-----
From: Michael Brown <mcb30@ipxe.org>
Sent: 2023年2月15日 6:24 PM
To: devel@edk2.groups.io; Richard Ho (何明忠) <RichardHo@ami.com>
Cc: Andrew Fish <afish@apple.com>; Leif Lindholm <quic_llindhol@quicinc.com>; Michael D Kinney <michael.d.kinney@intel.com>; Michael Kubacki <michael.kubacki@microsoft.com>; Zhiguang Liu <zhiguang.liu@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>; Tony Lo (羅金松) <TonyLo@ami.com>
Subject: [EXTERNAL] Re: [edk2-devel] [PATCH v3 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 15/02/2023 05:36, RichardHo [何明忠] via groups.io wrote:
On 15/02/2023 05:36, RichardHo [何明忠] via groups.io wrote:
On 15/02/2023 05:36, RichardHo [何明忠] via groups.io wrote:
On 15/02/2023 05:36, RichardHo [何明忠] via groups.io wrote:
On 15/02/2023 05:36, RichardHo [何明忠] via groups.io wrote:
> +  if (Nic->RateLimitingEnable == TRUE) {
> +    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,
> +                    PcdGet32 (RateLimitingFactor) * 10000
> +                    );
> +    if (EFI_ERROR (Status)) {
> +      goto ErrorSetTimer;
> +    }
> +  }
> +
> +  if (Nic->UsbEth->UsbEthUndi.UsbEthUndiStart != NULL) {
> +    Status = Nic->UsbEth->UsbEthUndi.UsbEthUndiStart (Cdb, Nic);
> +    if (EFI_ERROR (Status)) {
> +      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; }

Thank you for incorporating the polling rate limiting feature.

There is a bug in the above code: if RateLimitingEnable==FALSE and then an error occurs in UsbEthUndiStart(), the error handling code will call
SetTimer() and CloseEvent() on an event that was never created.

The simplest fix is to move the conditional check for RateLimitingEnable==FALSE so that the event is always created even if it will not be used:

   Status = gBS->CreateEvent (
                   EVT_TIMER | EVT_NOTIFY_SIGNAL,
                   TPL_CALLBACK,
                   UndiRateLimiterCallback,
                   Nic,
                   &Nic->RateLimiter
                   );
   if (EFI_ERROR (Status)) {
     goto ErrorCreateEvent;
   }

   if (Nic->RateLimitingEnable == TRUE) {
     Status = gBS->SetTimer (
                     Nic->RateLimiter,
                     TimerPeriodic,
                     PcdGet32 (RateLimitingFactor) * 10000
                     );
     if (EFI_ERROR (Status)) {
       goto ErrorSetTimer;
     }
   }

> +  if (Nic->RateLimitingEnable == TRUE) {
> +    gBS->SetTimer (&Nic->RateLimiter, TimerCancel, 0);
> +    gBS->CloseEvent (&Nic->RateLimiter);  }

... and this conditional check may then also be removed.

> +  if ((Nic->RateLimitingCreditCount == 0) && (Nic->RateLimitingEnable == TRUE)) {
> +    return PXE_STATCODE_NO_DATA;
> +  }
> +
> +  Status = Nic->UsbEth->UsbEthReceive (Cdb, Nic->UsbEth, (VOID
> + *)BulkInData, &DataLength);  if (EFI_ERROR (Status)) {
> +    Nic->ReceiveStatus = 0;
> +    if (Nic->RateLimitingEnable == TRUE) {
> +      OriginalTpl = gBS->RaiseTPL (TPL_NOTIFY);
> +      if (Nic->RateLimitingCreditCount != 0) {
> +        Nic->RateLimitingCreditCount--;
> +      }
> +
> +      gBS->RestoreTPL (OriginalTpl);
> +    }

The check for RateLimitingCreditCount!=0 is redundant: if RateLimitingCreditCount is zero then you cannot reach that point in the code anyway.

> +[PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
> +  ## Support rate limiting
> +
> +gUsbNetworkPkgTokenSpaceGuid.EnableRateLimiting|FALSE|BOOLEAN|0x00010
> +001

I would suggest that the default value should be TRUE.

The downside of setting the default value to TRUE is that the overall throughput will be slower (as reported by you).

The downside of setting the default value to FALSE is that the system will lock up completely (as reported by me and others).

Setting to TRUE by default is therefore less harmful overall.

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.