From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 40DA821B02B92 for ; Thu, 29 Jun 2017 01:43:04 -0700 (PDT) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by fmsmga104.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 29 Jun 2017 01:44:36 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.40,280,1496127600"; d="scan'208";a="105046665" Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by orsmga002.jf.intel.com with ESMTP; 29 Jun 2017 01:44:35 -0700 Received: from shsmsx151.ccr.corp.intel.com (10.239.6.50) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.319.2; Thu, 29 Jun 2017 01:44:35 -0700 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.151]) by SHSMSX151.ccr.corp.intel.com ([169.254.3.56]) with mapi id 14.03.0319.002; Thu, 29 Jun 2017 16:44:31 +0800 From: "Wu, Hao A" To: "Ni, Ruiyu" , "edk2-devel@lists.01.org" CC: "Tian, Feng" , "Zeng, Star" Thread-Topic: [edk2] [PATCH 1/3] MdeModulePkg/XhciDxe: Refine IsTransferRingTrb and IsAsyncIntTrb Thread-Index: AQHS7/r5c8GLa074Lkq9zIUEtRGl2aI7hpKg Date: Thu, 29 Jun 2017 08:44:31 +0000 Message-ID: References: <20170628103959.397244-1-ruiyu.ni@intel.com> <20170628103959.397244-2-ruiyu.ni@intel.com> In-Reply-To: <20170628103959.397244-2-ruiyu.ni@intel.com> Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [PATCH 1/3] MdeModulePkg/XhciDxe: Refine IsTransferRingTrb and IsAsyncIntTrb X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 29 Jun 2017 08:43:04 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Comments are made below. Best Regards, Hao Wu > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ru= iyu > Ni > Sent: Wednesday, June 28, 2017 6:40 PM > To: edk2-devel@lists.01.org > Cc: Wu, Hao A; Tian, Feng; Zeng, Star > Subject: [edk2] [PATCH 1/3] MdeModulePkg/XhciDxe: Refine IsTransferRingTr= b > and IsAsyncIntTrb >=20 > Current implementation of IsTransferRingTrb only checks whether > the TRB is in the RING of the URB. > The patch enhanced the logic to check that whether the TRB belongs > to the transaction of URB. >=20 > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Ruiyu Ni > Cc: Hao A Wu > Cc: Star Zeng > Cc: Feng Tian > --- > MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 93 ++++++++++++++++----------= - > ----- > 1 file changed, 47 insertions(+), 46 deletions(-) >=20 > diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c > b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c > index 457344051b..93803c352e 100644 > --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c > +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c > @@ -977,45 +977,45 @@ XhcFreeSched ( > } >=20 > /** > - Check if the Trb is a transaction of the URBs in XHCI's asynchronous t= ransfer > list. > + Check if the Trb is a transaction of the URB. >=20 > - @param Xhc The XHCI Instance. > - @param Trb The TRB to be checked. > - @param Urb The pointer to the matched Urb. > + @param Trb The TRB to be checked > + @param Urb The URB to be checked. >=20 > - @retval TRUE The Trb is matched with a transaction of the URBs in the= async > list. > - @retval FALSE The Trb is not matched with any URBs in the async list. > + @retval TRUE It is a transaction of the URB. > + @retval FALSE It is not any transaction of the URB. >=20 > **/ > BOOLEAN > -IsAsyncIntTrb ( > +IsTransferRingTrb ( > IN USB_XHCI_INSTANCE *Xhc, > IN TRB_TEMPLATE *Trb, > - OUT URB **Urb > + IN URB *Urb > ) > { > - LIST_ENTRY *Entry; > - LIST_ENTRY *Next; > - TRB_TEMPLATE *CheckedTrb; > - URB *CheckedUrb; > - UINTN Index; > + LINK_TRB *LinkTrb; > + TRB_TEMPLATE *CheckedTrb; > + UINTN Index; > + EFI_PHYSICAL_ADDRESS PhyAddr; >=20 > - EFI_LIST_FOR_EACH_SAFE (Entry, Next, &Xhc->AsyncIntTransfers) { > - CheckedUrb =3D EFI_LIST_CONTAINER (Entry, URB, UrbList); > - CheckedTrb =3D CheckedUrb->TrbStart; > - for (Index =3D 0; Index < CheckedUrb->TrbNum; Index++) { > - if (Trb =3D=3D CheckedTrb) { > - *Urb =3D CheckedUrb; > - return TRUE; > - } > - CheckedTrb++; > - // > - // If the checked TRB is the link TRB at the end of the transfer r= ing, > - // recircle it to the head of the ring. > - // > - if (CheckedTrb->Type =3D=3D TRB_TYPE_LINK) { > - CheckedTrb =3D (TRB_TEMPLATE*) CheckedUrb->Ring->RingSeg0; > - } > + CheckedTrb =3D Urb->TrbStart; > + if (CheckedTrb->Type =3D=3D TRB_TYPE_NORMAL) { > + ASSERT (Urb->TrbNum =3D=3D 1); > + } Is the above if-ASSERT check needed here? I think it's possible for the 'Urb->TrbNum' to be greater than 1 when 'Urb->DataLen' is larger than 0x100= 00 for Bulk and Interrupt transter (whose trb type will be TRB_TYPE_NORMAL). > + for (Index =3D 0; Index < Urb->TrbNum; Index++) { > + if (Trb =3D=3D CheckedTrb) { > + return TRUE; > + } > + CheckedTrb++; > + // > + // If the checked TRB is the link TRB at the end of the transfer rin= g, > + // recircle it to the head of the ring. > + // > + if (CheckedTrb->Type =3D=3D TRB_TYPE_LINK) { > + LinkTrb =3D (LINK_TRB *) CheckedTrb; > + PhyAddr =3D (EFI_PHYSICAL_ADDRESS)(LinkTrb->PtrLo | LShiftU64 ((UI= NT64) > LinkTrb->PtrHi, 32)); > + CheckedTrb =3D (TRB_TEMPLATE *)(UINTN) UsbHcGetHostAddrForPciAddr > (Xhc->MemPool, (VOID *)(UINTN) PhyAddr, sizeof (TRB_TEMPLATE)); > + ASSERT (CheckedTrb =3D=3D Urb->Ring->RingSeg0); > } > } >=20 > @@ -1023,38 +1023,39 @@ IsAsyncIntTrb ( > } >=20 > /** > - Check if the Trb is a transaction of the URB. > + Check if the Trb is a transaction of the URBs in XHCI's asynchronous t= ransfer > list. >=20 > - @param Trb The TRB to be checked > - @param Urb The transfer ring to be checked. > + @param Xhc The XHCI Instance. > + @param Trb The TRB to be checked. > + @param Urb The pointer to the matched Urb. >=20 > - @retval TRUE It is a transaction of the URB. > - @retval FALSE It is not any transaction of the URB. > + @retval TRUE The Trb is matched with a transaction of the URBs in the > async list. > + @retval FALSE The Trb is not matched with any URBs in the async list. >=20 > **/ > BOOLEAN > -IsTransferRingTrb ( > +IsAsyncIntTrb ( > + IN USB_XHCI_INSTANCE *Xhc, > IN TRB_TEMPLATE *Trb, > - IN URB *Urb > + OUT URB **Urb > ) > { > - TRB_TEMPLATE *CheckedTrb; > - UINTN Index; > - > - CheckedTrb =3D Urb->Ring->RingSeg0; > - > - ASSERT (Urb->Ring->TrbNumber =3D=3D CMD_RING_TRB_NUMBER || Urb->Ring- > >TrbNumber =3D=3D TR_RING_TRB_NUMBER); > + LIST_ENTRY *Entry; > + LIST_ENTRY *Next; > + URB *CheckedUrb; >=20 > - for (Index =3D 0; Index < Urb->Ring->TrbNumber; Index++) { > - if (Trb =3D=3D CheckedTrb) { > + EFI_LIST_FOR_EACH_SAFE (Entry, Next, &Xhc->AsyncIntTransfers) { > + CheckedUrb =3D EFI_LIST_CONTAINER (Entry, URB, UrbList); > + if (IsTransferRingTrb (Xhc, Trb, CheckedUrb)) { > + *Urb =3D CheckedUrb; > return TRUE; > } > - CheckedTrb++; > } >=20 > return FALSE; > } >=20 > + > /** > Check the URB's execution result and update the URB's > result accordingly. > @@ -1131,7 +1132,7 @@ XhcCheckUrbResult ( > // This way is used to avoid that those completed async transfer eve= nts don't > get > // handled in time and are flushed by newer coming events. > // > - if (IsTransferRingTrb (TRBPtr, Urb)) { > + if (IsTransferRingTrb (Xhc, TRBPtr, Urb)) { > CheckedUrb =3D Urb; > } else if (IsAsyncIntTrb (Xhc, TRBPtr, &AsyncUrb)) { > CheckedUrb =3D AsyncUrb; > -- > 2.12.2.windows.2 >=20 > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel