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 31E9E21B02B9B for ; Thu, 29 Jun 2017 04:12:57 -0700 (PDT) Received: from orsmga005.jf.intel.com ([10.7.209.41]) by fmsmga104.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 29 Jun 2017 04:14:27 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.40,280,1496127600"; d="scan'208";a="118863869" Received: from fmsmsx106.amr.corp.intel.com ([10.18.124.204]) by orsmga005.jf.intel.com with ESMTP; 29 Jun 2017 04:14:27 -0700 Received: from shsmsx102.ccr.corp.intel.com (10.239.4.154) by FMSMSX106.amr.corp.intel.com (10.18.124.204) with Microsoft SMTP Server (TLS) id 14.3.319.2; Thu, 29 Jun 2017 04:14:26 -0700 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.151]) by shsmsx102.ccr.corp.intel.com ([169.254.2.146]) with mapi id 14.03.0319.002; Thu, 29 Jun 2017 19:14:23 +0800 From: "Wu, Hao A" To: "Ni, Ruiyu" , "edk2-devel@lists.01.org" CC: "Tian, Feng" , "Zeng, Star" Thread-Topic: [edk2] [PATCH 3/3] MdeModulePkg/XhciDxe: Check timeout URB again after stopping endpoint Thread-Index: AQHS7/r54zLj1PUpq0mC5/iFz16/daI7iATQ Date: Thu, 29 Jun 2017 11:14:22 +0000 Message-ID: References: <20170628103959.397244-1-ruiyu.ni@intel.com> <20170628103959.397244-4-ruiyu.ni@intel.com> In-Reply-To: <20170628103959.397244-4-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 3/3] MdeModulePkg/XhciDxe: Check timeout URB again after stopping endpoint 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 11:12:57 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable The function XhcDequeueTrbFromEndpoint() will return EFI_ALREADY_STARTED to indicate that a 'timeout-detected' Urb finishes just before stopping the relating endpoint. So in this case, I think the caller of XhcDequeueTrbFromEndpoint() may not treat it as an error. If so, the error handling logic in XhcControlTransfer() may need to get updated as well. 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 3/3] MdeModulePkg/XhciDxe: Check timeout URB again > after stopping endpoint >=20 > This fixes BULK data loss when transfer is detected as timeout but > finished just before stopping endpoint. >=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/Xhci.c | 31 ++++++++------ > MdeModulePkg/Bus/Pci/XhciDxe/Xhci.h | 3 +- > MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 72 > +++++++++++++++++++++++++++----- > MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.h | 9 +++- > 4 files changed, 89 insertions(+), 26 deletions(-) >=20 > diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c > b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c > index 2f6137ef57..998946a168 100644 > --- a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c > +++ b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c > @@ -1249,27 +1249,34 @@ XhcBulkTransfer ( >=20 > Status =3D XhcExecTransfer (Xhc, FALSE, Urb, Timeout); >=20 > - *TransferResult =3D Urb->Result; > - *DataLength =3D Urb->Completed; >=20 > if (Status =3D=3D EFI_TIMEOUT) { > // > // The transfer timed out. Abort the transfer by dequeueing of the T= D. > // > RecoveryStatus =3D XhcDequeueTrbFromEndpoint(Xhc, Urb); > - if (EFI_ERROR(RecoveryStatus)) { > + if (RecoveryStatus =3D=3D EFI_ALREADY_STARTED) { > + // > + // The URB is finished just before stopping endpoint. > + // Change returning status from EFI_TIMEOUT to EFI_SUCCESS. > + // > + ASSERT (Urb->Result =3D=3D EFI_USB_NOERROR); > + Status =3D EFI_SUCCESS; > + DEBUG ((DEBUG_ERROR, "XhcBulkTransfer: pending URB is finished, > Length =3D %d.\n", Urb->Completed)); > + } else if (EFI_ERROR(RecoveryStatus)) { > DEBUG((EFI_D_ERROR, "XhcBulkTransfer: XhcDequeueTrbFromEndpoint > failed\n")); > } > - } else { > - if (*TransferResult =3D=3D EFI_USB_NOERROR) { > - Status =3D EFI_SUCCESS; > - } else if (*TransferResult =3D=3D EFI_USB_ERR_STALL) { > - RecoveryStatus =3D XhcRecoverHaltedEndpoint(Xhc, Urb); > - if (EFI_ERROR (RecoveryStatus)) { > - DEBUG ((EFI_D_ERROR, "XhcBulkTransfer: XhcRecoverHaltedEndpoint > failed\n")); > - } > - Status =3D EFI_DEVICE_ERROR; > + } > + > + *TransferResult =3D Urb->Result; > + *DataLength =3D Urb->Completed; > + > + if (*TransferResult =3D=3D EFI_USB_ERR_STALL) { > + RecoveryStatus =3D XhcRecoverHaltedEndpoint(Xhc, Urb); > + if (EFI_ERROR (RecoveryStatus)) { > + DEBUG ((DEBUG_ERROR, "XhcBulkTransfer: XhcRecoverHaltedEndpoint > failed\n")); > } > + ASSERT (Status =3D=3D EFI_DEVICE_ERROR); > } >=20 > Xhc->PciIo->Flush (Xhc->PciIo); > diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.h > b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.h > index 28e240245b..76daaff4a4 100644 > --- a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.h > +++ b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.h > @@ -2,7 +2,7 @@ >=20 > Provides some data structure definitions used by the XHCI host control= ler > driver. >=20 > -Copyright (c) 2011 - 2016, Intel Corporation. All rights reserved.
> +Copyright (c) 2011 - 2017, Intel Corporation. All rights reserved.
> This program and the accompanying materials > are licensed and made available under the terms and conditions of the BS= D > License > which accompanies this distribution. The full text of the license may b= e found > at > @@ -243,6 +243,7 @@ struct _USB_XHCI_INSTANCE { > UINT64 *DCBAA; > VOID *DCBAAMap; > UINT32 MaxSlotsEn; > + URB *PendingUrb; > // > // Cmd Transfer Ring > // > diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c > b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c > index dbc91023e1..3b0d56587f 100644 > --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c > +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c > @@ -696,6 +696,7 @@ Done: > @param Urb The urb which doesn't get completed in a= specified > timeout range. >=20 > @retval EFI_SUCCESS The dequeuing of the TDs is successful. > + @retval EFI_ALREADY_STARTED The Urb is finished so no deque is neede= d. > @retval Others Failed to stop the endpoint and dequeue = the TDs. >=20 > **/ > @@ -723,7 +724,7 @@ XhcDequeueTrbFromEndpoint ( > // > // 1) Send Stop endpoint command to stop xHC from executing of the TDs= on > the endpoint > // > - Status =3D XhcStopEndpoint(Xhc, SlotId, Dci); > + Status =3D XhcStopEndpoint(Xhc, SlotId, Dci, Urb); > if (EFI_ERROR(Status)) { > DEBUG ((EFI_D_ERROR, "XhcDequeueTrbFromEndpoint: Stop Endpoint > Failed, Status =3D %r\n", Status)); > goto Done; > @@ -732,10 +733,20 @@ XhcDequeueTrbFromEndpoint ( > // > // 2)Set dequeue pointer > // > - Status =3D XhcSetTrDequeuePointer(Xhc, SlotId, Dci, Urb); > - if (EFI_ERROR(Status)) { > - DEBUG ((EFI_D_ERROR, "XhcDequeueTrbFromEndpoint: Set Transfer Ring > Dequeue Pointer Failed, Status =3D %r\n", Status)); > - goto Done; > + if (Urb->Finished && Urb->Result =3D=3D EFI_USB_NOERROR) { > + // > + // Return Already Started to indicate the pending URB is finished. > + // This fixes BULK data loss when transfer is detected as timeout > + // but finished just before stopping endpoint. > + // > + Status =3D EFI_ALREADY_STARTED; > + DEBUG ((DEBUG_INFO, "XhcDequeueTrbFromEndpoint: Pending URB is > finished: Length Actual/Expect =3D %d/%d!\n", Urb->Completed, Urb->DataLe= n)); > + } else { > + Status =3D XhcSetTrDequeuePointer(Xhc, SlotId, Dci, Urb); > + if (EFI_ERROR (Status)) { > + DEBUG ((DEBUG_ERROR, "XhcDequeueTrbFromEndpoint: Set Transfer > Ring Dequeue Pointer Failed, Status =3D %r\n", Status)); > + goto Done; > + } > } >=20 > // > @@ -1128,12 +1139,14 @@ XhcCheckUrbResult ( > TRBPtr =3D (TRB_TEMPLATE *)(UINTN) UsbHcGetHostAddrForPciAddr (Xhc- > >MemPool, (VOID *)(UINTN) PhyAddr, sizeof (TRB_TEMPLATE)); >=20 > // > - // Update the status of Urb according to the finished event regardle= ss of > whether > - // the urb is current checked one or in the XHCI's async transfer li= st. > + // Update the status of URB including the pending URB, the URB that = is > currently checked, > + // and URBs in the XHCI's async interrupt transfer list. > // 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 (Xhc, TRBPtr, Urb)) { > + if (Xhc->PendingUrb !=3D NULL && IsTransferRingTrb (Xhc, TRBPtr, Xhc= - > >PendingUrb)) { > + CheckedUrb =3D Xhc->PendingUrb; > + } else if (IsTransferRingTrb (Xhc, TRBPtr, Urb)) { > CheckedUrb =3D Urb; > } else if (IsAsyncIntTrb (Xhc, TRBPtr, &AsyncUrb)) { > CheckedUrb =3D AsyncUrb; > @@ -1166,6 +1179,16 @@ XhcCheckUrbResult ( > DEBUG ((EFI_D_ERROR, "XhcCheckUrbResult: TRANSACTION_ERROR! > Completecode =3D %x\n",EvtTrb->Completecode)); > goto EXIT; >=20 > + case TRB_COMPLETION_STOPPED: > + case TRB_COMPLETION_STOPPED_LENGTH_INVALID: > + CheckedUrb->Result |=3D EFI_USB_ERR_TIMEOUT; > + CheckedUrb->Finished =3D TRUE; > + // > + // The pending URB is timeout and force stopped when stopping en= dpoint. > + // Continue the loop to receive the Command Complete Event for > stopping endpoint. > + // > + continue; > + > case TRB_COMPLETION_SHORT_PACKET: > case TRB_COMPLETION_SUCCESS: > if (EvtTrb->Completecode =3D=3D TRB_COMPLETION_SHORT_PACKET) { > @@ -3158,6 +3181,7 @@ XhcSetConfigCmd64 ( > @param Xhc The XHCI Instance. > @param SlotId The slot id to be configured. > @param Dci The device context index of endpoint. > + @param PendingUrb The pending URB to check completion stat= us > when stopping the end point. >=20 > @retval EFI_SUCCESS Stop endpoint successfully. > @retval Others Failed to stop endpoint. > @@ -3168,7 +3192,8 @@ EFIAPI > XhcStopEndpoint ( > IN USB_XHCI_INSTANCE *Xhc, > IN UINT8 SlotId, > - IN UINT8 Dci > + IN UINT8 Dci, > + IN URB *PendingUrb OPTIONAL > ) > { > EFI_STATUS Status; > @@ -3178,6 +3203,29 @@ XhcStopEndpoint ( > DEBUG ((EFI_D_INFO, "XhcStopEndpoint: Slot =3D 0x%x, Dci =3D 0x%x\n", = SlotId, > Dci)); >=20 > // > + // When XhcCheckUrbResult waits for the Stop_Endpoint completion, it a= lso > checks > + // the PendingUrb completion status, because it's possible that the > PendingUrb is > + // finished just before stopping the end point, but after the looping = check. > + // > + // The PendingUrb could be passed to XhcCmdTransfer to XhcExecTransfer= to > XhcCheckUrbResult > + // through function parameter, but That will cause every consumer of > XhcCmdTransfer, > + // XhcExecTransfer and XhcCheckUrbResult pass a NULL PendingUrb. > + // But actually only XhcCheckUrbResult is aware of the PendingUrb. > + // So we choose to save the PendingUrb into the USB_XHCI_INSTANCE and > use it in XhcCheckUrbResult. > + // > + ASSERT (Xhc->PendingUrb =3D=3D NULL); > + Xhc->PendingUrb =3D PendingUrb; > + // > + // Reset the URB result from Timeout to NoError. > + // The USB result will be: > + // changed to Timeout when Stop/StopInvalidLength Transfer Event is > received, or > + // remain NoError when Success/ShortPacket Transfer Event is receive= d. > + // > + if (PendingUrb !=3D NULL) { > + PendingUrb->Result =3D EFI_USB_NOERROR; > + } > + > + // > // Send stop endpoint command to transit Endpoint from running to stop= state > // > ZeroMem (&CmdTrbStopED, sizeof (CmdTrbStopED)); > @@ -3195,6 +3243,8 @@ XhcStopEndpoint ( > DEBUG ((EFI_D_ERROR, "XhcStopEndpoint: Stop Endpoint Failed, Status > =3D %r\n", Status)); > } >=20 > + Xhc->PendingUrb =3D NULL; > + > return Status; > } >=20 > @@ -3421,7 +3471,7 @@ XhcSetInterface ( > // XHCI 4.3.6 - Setting Alternate Interfaces > // 1) Stop any Running Transfer Rings affected by the Alternate In= terface > setting. > // > - Status =3D XhcStopEndpoint (Xhc, SlotId, Dci); > + Status =3D XhcStopEndpoint (Xhc, SlotId, Dci, NULL); > if (EFI_ERROR (Status)) { > return Status; > } > @@ -3623,7 +3673,7 @@ XhcSetInterface64 ( > // XHCI 4.3.6 - Setting Alternate Interfaces > // 1) Stop any Running Transfer Rings affected by the Alternate In= terface > setting. > // > - Status =3D XhcStopEndpoint (Xhc, SlotId, Dci); > + Status =3D XhcStopEndpoint (Xhc, SlotId, Dci, NULL); > if (EFI_ERROR (Status)) { > return Status; > } > diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.h > b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.h > index 931c7efa0c..4ed43ad57b 100644 > --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.h > +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.h > @@ -2,7 +2,7 @@ >=20 > This file contains the definition for XHCI host controller schedule ro= utines. >=20 > -Copyright (c) 2011 - 2015, Intel Corporation. All rights reserved.
> +Copyright (c) 2011 - 2017, Intel Corporation. All rights reserved.
> This program and the accompanying materials > are licensed and made available under the terms and conditions of the BS= D > License > which accompanies this distribution. The full text of the license may b= e found > at > @@ -80,6 +80,8 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY > KIND, EITHER EXPRESS OR IMPLIED. > #define TRB_COMPLETION_TRB_ERROR 5 > #define TRB_COMPLETION_STALL_ERROR 6 > #define TRB_COMPLETION_SHORT_PACKET 13 > +#define TRB_COMPLETION_STOPPED 26 > +#define TRB_COMPLETION_STOPPED_LENGTH_INVALID 27 >=20 > // > // The topology string used to present usb device location > @@ -1337,12 +1339,14 @@ XhcDequeueTrbFromEndpoint ( > IN URB *Urb > ); >=20 > + > /** > Stop endpoint through XHCI's Stop_Endpoint cmd. >=20 > @param Xhc The XHCI Instance. > @param SlotId The slot id to be configured. > @param Dci The device context index of endpoint. > + @param PendingUrb The pending URB to check completion stat= us > when stopping the end point. >=20 > @retval EFI_SUCCESS Stop endpoint successfully. > @retval Others Failed to stop endpoint. > @@ -1353,7 +1357,8 @@ EFIAPI > XhcStopEndpoint ( > IN USB_XHCI_INSTANCE *Xhc, > IN UINT8 SlotId, > - IN UINT8 Dci > + IN UINT8 Dci, > + IN URB *PendingUrb OPTIONAL > ); >=20 > /** > -- > 2.12.2.windows.2 >=20 > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel