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|0x00010001
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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#100234): https://edk2.groups.io/g/devel/message/100234
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]
-=-=-=-=-=-=-=-=-=-=-=-
