From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from a7-18.smtp-out.eu-west-1.amazonses.com (a7-18.smtp-out.eu-west-1.amazonses.com [54.240.7.18]) by mx.groups.io with SMTP id smtpd.web10.12021.1676456634767798048 for ; Wed, 15 Feb 2023 02:23:55 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@ipxe.org header.s=cphpx6z2rfcgehlykjjh3gknqe3hsoe2 header.b=GfQwDy0S; spf=pass (domain: eu-west-1.amazonses.com, ip: 54.240.7.18, mailfrom: 01020186549a20fd-079f978d-fbaf-4a60-b4a3-5d09c64f3f15-000000@eu-west-1.amazonses.com) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/simple; s=cphpx6z2rfcgehlykjjh3gknqe3hsoe2; d=ipxe.org; t=1676456632; h=Message-ID:Date:MIME-Version:To:Cc:References:From:Subject:In-Reply-To:Content-Type:Content-Transfer-Encoding; bh=949klj6WtwmEpo/9cfYzYZgSTZf5RLZoJwILtsRc5r4=; b=GfQwDy0SER6F65udkU8fUdAT25c5hsxnYOvC9g8UCHxNbpUdxgechakec42S+2iV CW4XW5HzNLxlnjBhbAUPTqMIlouhAcbLLKUrFM9oJS4ng+QTZCkiDR/AQsFFcmy7ftq V/FDq3YXBDZx31NyMh+nrzoZ3PaTCZxxzNsobaMN8j4t0ejWOEQsWJzXjBNrIHaNAlc qIEWVh8N+oI4nmT/5v3tWfz2zIf0HHYTalkldfASO9R8no6eX5iY61PAY5FdkY6TJTi MNuPe9T2tVFaeL9qDe8T19TSx1Fz54hQopBsppp+H5NSlx9XRM1awiJ2/MIqy6U+Obk zfDzWXX+iQ== DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/simple; s=ihchhvubuqgjsxyuhssfvqohv7z3u4hn; d=amazonses.com; t=1676456632; h=Message-ID:Date:MIME-Version:To:Cc:References:From:Subject:In-Reply-To:Content-Type:Content-Transfer-Encoding:Feedback-ID; bh=949klj6WtwmEpo/9cfYzYZgSTZf5RLZoJwILtsRc5r4=; b=TxVqUuRhOR+TmD0kKdTDc8Cxd4NNoNrGZLMakKZbm2uWHMIL45Nux47zX8VvhwrD OibgYpNODBWZC66qXDPjkuLzfzyB7YhVWDUCFziIRiVtAcaSSgwa3fmCB3QY/ijfuPV Uqk1HY2dDfYHBaK8YskzWGf2SqPUVsFX6CQ47peg= Message-ID: <01020186549a20fd-079f978d-fbaf-4a60-b4a3-5d09c64f3f15-000000@eu-west-1.amazonses.com> Date: Wed, 15 Feb 2023 10:23:52 +0000 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.6.0 To: devel@edk2.groups.io, richardho@ami.com Cc: Andrew Fish , Leif Lindholm , Michael D Kinney , Michael Kubacki , Zhiguang Liu , Liming Gao , =?UTF-8?B?VG9ueSBMbyAo576F6YeR5p2+KQ==?= References: <20230215053622.3746-1-richardho@ami.com> From: "Michael Brown" Subject: Re: [edk2-devel] [PATCH v3 1/3] UsbNetworkPkg/UsbRndis: Add USB RNDIS devices support In-Reply-To: <20230215053622.3746-1-richardho@ami.com> X-Spam-Status: No, score=-2.9 required=5.0 tests=ALL_TRUSTED,BAYES_00, URIBL_DBL_BLOCKED_OPENDNS,URIBL_ZEN_BLOCKED_OPENDNS autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on blyat.fensystems.co.uk Feedback-ID: 1.eu-west-1.fspj4M/5bzJ9NLRzJP0PaxRwxrpZqiDQJ1IF94CF2TA=:AmazonSES X-SES-Outgoing: 2023.02.15-54.240.7.18 Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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