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 <mc...@ipxe.org>
Sent: 2023年2月15日 6:24 PM
To: devel@edk2.groups.io; Richard Ho (何明忠) <richar...@ami.com>
Cc: Andrew Fish <af...@apple.com>; Leif Lindholm <quic_llind...@quicinc.com>;
Michael D Kinney <michael.d.kin...@intel.com>; Michael Kubacki
<michael.kuba...@microsoft.com>; Zhiguang Liu <zhiguang....@intel.com>; Liming
Gao <gaolim...@byosoft.com.cn>; Tony Lo (羅金松) <ton...@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.
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#100260): https://edk2.groups.io/g/devel/message/100260
Mute This Topic: https://groups.io/mt/96977783/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
