[PATCH 0/3] Check timeout URB again after stopping endpoint

[PATCH 0/3] Check timeout URB again after stopping endpoint

[Ruiyu Ni]

This fixes BULK data loss when transfer is detected as timeout but
finished just before stopping endpoint.

Ruiyu Ni (3):
  MdeModulePkg/XhciDxe: Refine IsTransferRingTrb and IsAsyncIntTrb
  MdeModulePkg/XhciDxe: Dump the CMD/EVENT/INT/BULK ring information
  MdeModulePkg/XhciDxe: Check timeout URB again after stopping endpoint

 MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c      |  31 +++--
 MdeModulePkg/Bus/Pci/XhciDxe/Xhci.h      |   3 +-
 MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 190 +++++++++++++++++++++----------
 MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.h |   9 +-
 4 files changed, 159 insertions(+), 74 deletions(-)

-- 
2.12.2.windows.2

[PATCH 1/3] MdeModulePkg/XhciDxe: Refine IsTransferRingTrb and IsAsyncIntTrb

[Ruiyu Ni]

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.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Feng Tian <feng.tian@intel.com>
---
 MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 93 ++++++++++++++++----------------
 1 file changed, 47 insertions(+), 46 deletions(-)

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 (
 }
 
 /**
-  Check if the Trb is a transaction of the URBs in XHCI's asynchronous transfer list.
+  Check if the Trb is a transaction of the URB.
 
-  @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.
 
-  @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.
 
 **/
 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;
 
-  EFI_LIST_FOR_EACH_SAFE (Entry, Next, &Xhc->AsyncIntTransfers) {
-    CheckedUrb = EFI_LIST_CONTAINER (Entry, URB, UrbList);
-    CheckedTrb = CheckedUrb->TrbStart;
-    for (Index = 0; Index < CheckedUrb->TrbNum; Index++) {
-      if (Trb == CheckedTrb) {
-        *Urb = CheckedUrb;
-        return TRUE;
-      }
-      CheckedTrb++;
-      //
-      // If the checked TRB is the link TRB at the end of the transfer ring,
-      // recircle it to the head of the ring.
-      //
-      if (CheckedTrb->Type == TRB_TYPE_LINK) {
-        CheckedTrb = (TRB_TEMPLATE*) CheckedUrb->Ring->RingSeg0;
-      }
+  CheckedTrb = Urb->TrbStart;
+  if (CheckedTrb->Type == TRB_TYPE_NORMAL) {
+    ASSERT (Urb->TrbNum == 1);
+  }
+  for (Index = 0; Index < Urb->TrbNum; Index++) {
+    if (Trb == CheckedTrb) {
+      return TRUE;
+    }
+    CheckedTrb++;
+    //
+    // If the checked TRB is the link TRB at the end of the transfer ring,
+    // recircle it to the head of the ring.
+    //
+    if (CheckedTrb->Type == TRB_TYPE_LINK) {
+      LinkTrb = (LINK_TRB *) CheckedTrb;
+      PhyAddr = (EFI_PHYSICAL_ADDRESS)(LinkTrb->PtrLo | LShiftU64 ((UINT64) LinkTrb->PtrHi, 32));
+      CheckedTrb = (TRB_TEMPLATE *)(UINTN) UsbHcGetHostAddrForPciAddr (Xhc->MemPool, (VOID *)(UINTN) PhyAddr, sizeof (TRB_TEMPLATE));
+      ASSERT (CheckedTrb == Urb->Ring->RingSeg0);
     }
   }
 
@@ -1023,38 +1023,39 @@ IsAsyncIntTrb (
 }
 
 /**
-  Check if the Trb is a transaction of the URB.
+  Check if the Trb is a transaction of the URBs in XHCI's asynchronous transfer list.
 
-  @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.
 
-  @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.
 
 **/
 BOOLEAN
-IsTransferRingTrb (
+IsAsyncIntTrb (
+  IN  USB_XHCI_INSTANCE   *Xhc,
   IN  TRB_TEMPLATE        *Trb,
-  IN  URB                 *Urb
+  OUT URB                 **Urb
   )
 {
-  TRB_TEMPLATE  *CheckedTrb;
-  UINTN         Index;
-
-  CheckedTrb = Urb->Ring->RingSeg0;
-
-  ASSERT (Urb->Ring->TrbNumber == CMD_RING_TRB_NUMBER || Urb->Ring->TrbNumber == TR_RING_TRB_NUMBER);
+  LIST_ENTRY              *Entry;
+  LIST_ENTRY              *Next;
+  URB                     *CheckedUrb;
 
-  for (Index = 0; Index < Urb->Ring->TrbNumber; Index++) {
-    if (Trb == CheckedTrb) {
+  EFI_LIST_FOR_EACH_SAFE (Entry, Next, &Xhc->AsyncIntTransfers) {
+    CheckedUrb = EFI_LIST_CONTAINER (Entry, URB, UrbList);
+    if (IsTransferRingTrb (Xhc, Trb, CheckedUrb)) {
+      *Urb = CheckedUrb;
       return TRUE;
     }
-    CheckedTrb++;
   }
 
   return FALSE;
 }
 
+
 /**
   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 events don't get
     // handled in time and are flushed by newer coming events.
     //
-    if (IsTransferRingTrb (TRBPtr, Urb)) {
+    if (IsTransferRingTrb (Xhc, TRBPtr, Urb)) {
       CheckedUrb = Urb;
     } else if (IsAsyncIntTrb (Xhc, TRBPtr, &AsyncUrb)) {    
       CheckedUrb = AsyncUrb;
-- 
2.12.2.windows.2

Re: [PATCH 1/3] MdeModulePkg/XhciDxe: Refine IsTransferRingTrb and IsAsyncIntTrb

[Wu, Hao A]

Comments are made below.

Best Regards,
Hao Wu


> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ruiyu
> 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 IsTransferRingTrb
> and IsAsyncIntTrb
> 
> 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.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Feng Tian <feng.tian@intel.com>
> ---
>  MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 93 ++++++++++++++++-----------
> -----
>  1 file changed, 47 insertions(+), 46 deletions(-)
> 
> 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 (
>  }
> 
>  /**
> -  Check if the Trb is a transaction of the URBs in XHCI's asynchronous transfer
> list.
> +  Check if the Trb is a transaction of the URB.
> 
> -  @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.
> 
> -  @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.
> 
>  **/
>  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;
> 
> -  EFI_LIST_FOR_EACH_SAFE (Entry, Next, &Xhc->AsyncIntTransfers) {
> -    CheckedUrb = EFI_LIST_CONTAINER (Entry, URB, UrbList);
> -    CheckedTrb = CheckedUrb->TrbStart;
> -    for (Index = 0; Index < CheckedUrb->TrbNum; Index++) {
> -      if (Trb == CheckedTrb) {
> -        *Urb = CheckedUrb;
> -        return TRUE;
> -      }
> -      CheckedTrb++;
> -      //
> -      // If the checked TRB is the link TRB at the end of the transfer ring,
> -      // recircle it to the head of the ring.
> -      //
> -      if (CheckedTrb->Type == TRB_TYPE_LINK) {
> -        CheckedTrb = (TRB_TEMPLATE*) CheckedUrb->Ring->RingSeg0;
> -      }
> +  CheckedTrb = Urb->TrbStart;
> +  if (CheckedTrb->Type == TRB_TYPE_NORMAL) {
> +    ASSERT (Urb->TrbNum == 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 0x10000
for Bulk and Interrupt transter (whose trb type will be TRB_TYPE_NORMAL).

> +  for (Index = 0; Index < Urb->TrbNum; Index++) {
> +    if (Trb == CheckedTrb) {
> +      return TRUE;
> +    }
> +    CheckedTrb++;
> +    //
> +    // If the checked TRB is the link TRB at the end of the transfer ring,
> +    // recircle it to the head of the ring.
> +    //
> +    if (CheckedTrb->Type == TRB_TYPE_LINK) {
> +      LinkTrb = (LINK_TRB *) CheckedTrb;
> +      PhyAddr = (EFI_PHYSICAL_ADDRESS)(LinkTrb->PtrLo | LShiftU64 ((UINT64)
> LinkTrb->PtrHi, 32));
> +      CheckedTrb = (TRB_TEMPLATE *)(UINTN) UsbHcGetHostAddrForPciAddr
> (Xhc->MemPool, (VOID *)(UINTN) PhyAddr, sizeof (TRB_TEMPLATE));
> +      ASSERT (CheckedTrb == Urb->Ring->RingSeg0);
>      }
>    }
> 
> @@ -1023,38 +1023,39 @@ IsAsyncIntTrb (
>  }
> 
>  /**
> -  Check if the Trb is a transaction of the URB.
> +  Check if the Trb is a transaction of the URBs in XHCI's asynchronous transfer
> list.
> 
> -  @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.
> 
> -  @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.
> 
>  **/
>  BOOLEAN
> -IsTransferRingTrb (
> +IsAsyncIntTrb (
> +  IN  USB_XHCI_INSTANCE   *Xhc,
>    IN  TRB_TEMPLATE        *Trb,
> -  IN  URB                 *Urb
> +  OUT URB                 **Urb
>    )
>  {
> -  TRB_TEMPLATE  *CheckedTrb;
> -  UINTN         Index;
> -
> -  CheckedTrb = Urb->Ring->RingSeg0;
> -
> -  ASSERT (Urb->Ring->TrbNumber == CMD_RING_TRB_NUMBER || Urb->Ring-
> >TrbNumber == TR_RING_TRB_NUMBER);
> +  LIST_ENTRY              *Entry;
> +  LIST_ENTRY              *Next;
> +  URB                     *CheckedUrb;
> 
> -  for (Index = 0; Index < Urb->Ring->TrbNumber; Index++) {
> -    if (Trb == CheckedTrb) {
> +  EFI_LIST_FOR_EACH_SAFE (Entry, Next, &Xhc->AsyncIntTransfers) {
> +    CheckedUrb = EFI_LIST_CONTAINER (Entry, URB, UrbList);
> +    if (IsTransferRingTrb (Xhc, Trb, CheckedUrb)) {
> +      *Urb = CheckedUrb;
>        return TRUE;
>      }
> -    CheckedTrb++;
>    }
> 
>    return FALSE;
>  }
> 
> +
>  /**
>    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 events don't
> get
>      // handled in time and are flushed by newer coming events.
>      //
> -    if (IsTransferRingTrb (TRBPtr, Urb)) {
> +    if (IsTransferRingTrb (Xhc, TRBPtr, Urb)) {
>        CheckedUrb = Urb;
>      } else if (IsAsyncIntTrb (Xhc, TRBPtr, &AsyncUrb)) {
>        CheckedUrb = AsyncUrb;
> --
> 2.12.2.windows.2
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

[PATCH 2/3] MdeModulePkg/XhciDxe: Dump the CMD/EVENT/INT/BULK ring information

[Ruiyu Ni]

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Feng Tian <feng.tian@intel.com>
---
 MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
index 93803c352e..dbc91023e1 100644
--- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
+++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
@@ -604,8 +604,6 @@ XhcInitSched (
   XhcWriteOpReg (Xhc, XHC_CRCR_OFFSET, XHC_LOW_32BIT(CmdRingPhy));
   XhcWriteOpReg (Xhc, XHC_CRCR_OFFSET + 4, XHC_HIGH_32BIT (CmdRingPhy));
 
-  DEBUG ((EFI_D_INFO, "XhcInitSched:XHC_CRCR=0x%x\n", Xhc->CmdRing.RingSeg0));
-
   //
   // Disable the 'interrupter enable' bit in USB_CMD
   // and clear IE & IP bit in all Interrupter X Management Registers.
@@ -620,7 +618,10 @@ XhcInitSched (
   // Allocate EventRing for Cmd, Ctrl, Bulk, Interrupt, AsynInterrupt transfer
   //
   CreateEventRing (Xhc, &Xhc->EventRing);
-  DEBUG ((EFI_D_INFO, "XhcInitSched:XHC_EVENTRING=0x%x\n", Xhc->EventRing.EventRingSeg0));
+  DEBUG ((DEBUG_INFO, "XhcInitSched: Created CMD ring [%p~%p) EVENT ring [%p~%p)\n",
+    Xhc->CmdRing.RingSeg0,        (UINTN)Xhc->CmdRing.RingSeg0 + sizeof (TRB_TEMPLATE) * CMD_RING_TRB_NUMBER,
+    Xhc->EventRing.EventRingSeg0, (UINTN)Xhc->EventRing.EventRingSeg0 + sizeof (TRB_TEMPLATE) * EVENT_RING_TRB_NUMBER
+    ));
 }
 
 /**
@@ -2671,6 +2672,11 @@ XhcInitializeEndpointContext (
           EndpointTransferRing = AllocateZeroPool(sizeof (TRANSFER_RING));
           Xhc->UsbDevContext[SlotId].EndpointTransferRing[Dci-1] = (VOID *) EndpointTransferRing;
           CreateTransferRing(Xhc, TR_RING_TRB_NUMBER, (TRANSFER_RING *)Xhc->UsbDevContext[SlotId].EndpointTransferRing[Dci-1]);
+          DEBUG ((DEBUG_INFO, "Endpoint[%x]: Created BULK ring [%p~%p)\n",
+                  EpDesc->EndpointAddress,
+                  EndpointTransferRing->RingSeg0,
+                  (UINTN) EndpointTransferRing->RingSeg0 + TR_RING_TRB_NUMBER * sizeof (TRB_TEMPLATE)
+                  ));
         }
 
         break;
@@ -2739,6 +2745,11 @@ XhcInitializeEndpointContext (
           EndpointTransferRing = AllocateZeroPool(sizeof (TRANSFER_RING));
           Xhc->UsbDevContext[SlotId].EndpointTransferRing[Dci-1] = (VOID *) EndpointTransferRing;
           CreateTransferRing(Xhc, TR_RING_TRB_NUMBER, (TRANSFER_RING *)Xhc->UsbDevContext[SlotId].EndpointTransferRing[Dci-1]);
+          DEBUG ((DEBUG_INFO, "Endpoint[%x]: Created INT ring [%p~%p)\n",
+                  EpDesc->EndpointAddress,
+                  EndpointTransferRing->RingSeg0,
+                  (UINTN) EndpointTransferRing->RingSeg0 + TR_RING_TRB_NUMBER * sizeof (TRB_TEMPLATE)
+                  ));
         }
         break;
 
@@ -2853,6 +2864,11 @@ XhcInitializeEndpointContext64 (
           EndpointTransferRing = AllocateZeroPool(sizeof (TRANSFER_RING));
           Xhc->UsbDevContext[SlotId].EndpointTransferRing[Dci-1] = (VOID *) EndpointTransferRing;
           CreateTransferRing(Xhc, TR_RING_TRB_NUMBER, (TRANSFER_RING *)Xhc->UsbDevContext[SlotId].EndpointTransferRing[Dci-1]);
+          DEBUG ((DEBUG_INFO, "Endpoint64[%x]: Created BULK ring [%p~%p)\n",
+                  EpDesc->EndpointAddress,
+                  EndpointTransferRing->RingSeg0,
+                  (UINTN) EndpointTransferRing->RingSeg0 + TR_RING_TRB_NUMBER * sizeof (TRB_TEMPLATE)
+                  ));
         }
 
         break;
@@ -2921,6 +2937,11 @@ XhcInitializeEndpointContext64 (
           EndpointTransferRing = AllocateZeroPool(sizeof (TRANSFER_RING));
           Xhc->UsbDevContext[SlotId].EndpointTransferRing[Dci-1] = (VOID *) EndpointTransferRing;
           CreateTransferRing(Xhc, TR_RING_TRB_NUMBER, (TRANSFER_RING *)Xhc->UsbDevContext[SlotId].EndpointTransferRing[Dci-1]);
+          DEBUG ((DEBUG_INFO, "Endpoint64[%x]: Created INT ring [%p~%p)\n",
+                  EpDesc->EndpointAddress,
+                  EndpointTransferRing->RingSeg0,
+                  (UINTN) EndpointTransferRing->RingSeg0 + TR_RING_TRB_NUMBER * sizeof (TRB_TEMPLATE)
+                  ));
         }
         break;
 
-- 
2.12.2.windows.2

Re: [PATCH 2/3] MdeModulePkg/XhciDxe: Dump the CMD/EVENT/INT/BULK ring information

[Wu, Hao A]

Reviewed-by: Hao Wu <hao.a.wu@intel.com>


Best Regards,
Hao Wu


> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ruiyu
> 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 2/3] MdeModulePkg/XhciDxe: Dump the
> CMD/EVENT/INT/BULK ring information
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Feng Tian <feng.tian@intel.com>
> ---
>  MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 27
> ++++++++++++++++++++++++---
>  1 file changed, 24 insertions(+), 3 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> index 93803c352e..dbc91023e1 100644
> --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> @@ -604,8 +604,6 @@ XhcInitSched (
>    XhcWriteOpReg (Xhc, XHC_CRCR_OFFSET, XHC_LOW_32BIT(CmdRingPhy));
>    XhcWriteOpReg (Xhc, XHC_CRCR_OFFSET + 4, XHC_HIGH_32BIT
> (CmdRingPhy));
> 
> -  DEBUG ((EFI_D_INFO, "XhcInitSched:XHC_CRCR=0x%x\n", Xhc-
> >CmdRing.RingSeg0));
> -
>    //
>    // Disable the 'interrupter enable' bit in USB_CMD
>    // and clear IE & IP bit in all Interrupter X Management Registers.
> @@ -620,7 +618,10 @@ XhcInitSched (
>    // Allocate EventRing for Cmd, Ctrl, Bulk, Interrupt, AsynInterrupt transfer
>    //
>    CreateEventRing (Xhc, &Xhc->EventRing);
> -  DEBUG ((EFI_D_INFO, "XhcInitSched:XHC_EVENTRING=0x%x\n", Xhc-
> >EventRing.EventRingSeg0));
> +  DEBUG ((DEBUG_INFO, "XhcInitSched: Created CMD ring [%p~%p) EVENT
> ring [%p~%p)\n",
> +    Xhc->CmdRing.RingSeg0,        (UINTN)Xhc->CmdRing.RingSeg0 + sizeof
> (TRB_TEMPLATE) * CMD_RING_TRB_NUMBER,
> +    Xhc->EventRing.EventRingSeg0, (UINTN)Xhc->EventRing.EventRingSeg0 +
> sizeof (TRB_TEMPLATE) * EVENT_RING_TRB_NUMBER
> +    ));
>  }
> 
>  /**
> @@ -2671,6 +2672,11 @@ XhcInitializeEndpointContext (
>            EndpointTransferRing = AllocateZeroPool(sizeof (TRANSFER_RING));
>            Xhc->UsbDevContext[SlotId].EndpointTransferRing[Dci-1] = (VOID *)
> EndpointTransferRing;
>            CreateTransferRing(Xhc, TR_RING_TRB_NUMBER, (TRANSFER_RING
> *)Xhc->UsbDevContext[SlotId].EndpointTransferRing[Dci-1]);
> +          DEBUG ((DEBUG_INFO, "Endpoint[%x]: Created BULK ring [%p~%p)\n",
> +                  EpDesc->EndpointAddress,
> +                  EndpointTransferRing->RingSeg0,
> +                  (UINTN) EndpointTransferRing->RingSeg0 +
> TR_RING_TRB_NUMBER * sizeof (TRB_TEMPLATE)
> +                  ));
>          }
> 
>          break;
> @@ -2739,6 +2745,11 @@ XhcInitializeEndpointContext (
>            EndpointTransferRing = AllocateZeroPool(sizeof (TRANSFER_RING));
>            Xhc->UsbDevContext[SlotId].EndpointTransferRing[Dci-1] = (VOID *)
> EndpointTransferRing;
>            CreateTransferRing(Xhc, TR_RING_TRB_NUMBER, (TRANSFER_RING
> *)Xhc->UsbDevContext[SlotId].EndpointTransferRing[Dci-1]);
> +          DEBUG ((DEBUG_INFO, "Endpoint[%x]: Created INT ring [%p~%p)\n",
> +                  EpDesc->EndpointAddress,
> +                  EndpointTransferRing->RingSeg0,
> +                  (UINTN) EndpointTransferRing->RingSeg0 +
> TR_RING_TRB_NUMBER * sizeof (TRB_TEMPLATE)
> +                  ));
>          }
>          break;
> 
> @@ -2853,6 +2864,11 @@ XhcInitializeEndpointContext64 (
>            EndpointTransferRing = AllocateZeroPool(sizeof (TRANSFER_RING));
>            Xhc->UsbDevContext[SlotId].EndpointTransferRing[Dci-1] = (VOID *)
> EndpointTransferRing;
>            CreateTransferRing(Xhc, TR_RING_TRB_NUMBER, (TRANSFER_RING
> *)Xhc->UsbDevContext[SlotId].EndpointTransferRing[Dci-1]);
> +          DEBUG ((DEBUG_INFO, "Endpoint64[%x]: Created BULK ring [%p~%p)\n",
> +                  EpDesc->EndpointAddress,
> +                  EndpointTransferRing->RingSeg0,
> +                  (UINTN) EndpointTransferRing->RingSeg0 +
> TR_RING_TRB_NUMBER * sizeof (TRB_TEMPLATE)
> +                  ));
>          }
> 
>          break;
> @@ -2921,6 +2937,11 @@ XhcInitializeEndpointContext64 (
>            EndpointTransferRing = AllocateZeroPool(sizeof (TRANSFER_RING));
>            Xhc->UsbDevContext[SlotId].EndpointTransferRing[Dci-1] = (VOID *)
> EndpointTransferRing;
>            CreateTransferRing(Xhc, TR_RING_TRB_NUMBER, (TRANSFER_RING
> *)Xhc->UsbDevContext[SlotId].EndpointTransferRing[Dci-1]);
> +          DEBUG ((DEBUG_INFO, "Endpoint64[%x]: Created INT ring [%p~%p)\n",
> +                  EpDesc->EndpointAddress,
> +                  EndpointTransferRing->RingSeg0,
> +                  (UINTN) EndpointTransferRing->RingSeg0 +
> TR_RING_TRB_NUMBER * sizeof (TRB_TEMPLATE)
> +                  ));
>          }
>          break;
> 
> --
> 2.12.2.windows.2
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

[PATCH 3/3] MdeModulePkg/XhciDxe: Check timeout URB again after stopping endpoint

[Ruiyu Ni]

This fixes BULK data loss when transfer is detected as timeout but
finished just before stopping endpoint.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Feng Tian <feng.tian@intel.com>
---
 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(-)

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 (
 
   Status = XhcExecTransfer (Xhc, FALSE, Urb, Timeout);
 
-  *TransferResult = Urb->Result;
-  *DataLength     = Urb->Completed;
 
   if (Status == EFI_TIMEOUT) {
     //
     // The transfer timed out. Abort the transfer by dequeueing of the TD.
     //
     RecoveryStatus = XhcDequeueTrbFromEndpoint(Xhc, Urb);
-    if (EFI_ERROR(RecoveryStatus)) {
+    if (RecoveryStatus == EFI_ALREADY_STARTED) {
+      //
+      // The URB is finished just before stopping endpoint.
+      // Change returning status from EFI_TIMEOUT to EFI_SUCCESS.
+      //
+      ASSERT (Urb->Result == EFI_USB_NOERROR);
+      Status = EFI_SUCCESS;
+      DEBUG ((DEBUG_ERROR, "XhcBulkTransfer: pending URB is finished, Length = %d.\n", Urb->Completed));
+    } else if (EFI_ERROR(RecoveryStatus)) {
       DEBUG((EFI_D_ERROR, "XhcBulkTransfer: XhcDequeueTrbFromEndpoint failed\n"));
     }
-  } else {
-    if (*TransferResult == EFI_USB_NOERROR) {
-      Status = EFI_SUCCESS;
-    } else if (*TransferResult == EFI_USB_ERR_STALL) {
-      RecoveryStatus = XhcRecoverHaltedEndpoint(Xhc, Urb);
-      if (EFI_ERROR (RecoveryStatus)) {
-        DEBUG ((EFI_D_ERROR, "XhcBulkTransfer: XhcRecoverHaltedEndpoint failed\n"));
-      }
-      Status = EFI_DEVICE_ERROR;
+  }
+
+  *TransferResult = Urb->Result;
+  *DataLength     = Urb->Completed;
+
+  if (*TransferResult == EFI_USB_ERR_STALL) {
+    RecoveryStatus = XhcRecoverHaltedEndpoint(Xhc, Urb);
+    if (EFI_ERROR (RecoveryStatus)) {
+      DEBUG ((DEBUG_ERROR, "XhcBulkTransfer: XhcRecoverHaltedEndpoint failed\n"));
     }
+    ASSERT (Status == EFI_DEVICE_ERROR);
   }
 
   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 @@
 
   Provides some data structure definitions used by the XHCI host controller driver.
 
-Copyright (c) 2011 - 2016, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2011 - 2017, Intel Corporation. All rights reserved.<BR>
 This program and the accompanying materials
 are licensed and made available under the terms and conditions of the BSD License
 which accompanies this distribution.  The full text of the license may be 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.
 
   @retval EFI_SUCCESS           The dequeuing of the TDs is successful.
+  @retval EFI_ALREADY_STARTED   The Urb is finished so no deque is needed.
   @retval Others                Failed to stop the endpoint and dequeue the TDs.
 
 **/
@@ -723,7 +724,7 @@ XhcDequeueTrbFromEndpoint (
   //
   // 1) Send Stop endpoint command to stop xHC from executing of the TDs on the endpoint
   //
-  Status = XhcStopEndpoint(Xhc, SlotId, Dci);
+  Status = XhcStopEndpoint(Xhc, SlotId, Dci, Urb);
   if (EFI_ERROR(Status)) {
     DEBUG ((EFI_D_ERROR, "XhcDequeueTrbFromEndpoint: Stop Endpoint Failed, Status = %r\n", Status));
     goto Done;
@@ -732,10 +733,20 @@ XhcDequeueTrbFromEndpoint (
   //
   // 2)Set dequeue pointer
   //
-  Status = XhcSetTrDequeuePointer(Xhc, SlotId, Dci, Urb);
-  if (EFI_ERROR(Status)) {
-    DEBUG ((EFI_D_ERROR, "XhcDequeueTrbFromEndpoint: Set Transfer Ring Dequeue Pointer Failed, Status = %r\n", Status));
-    goto Done;
+  if (Urb->Finished && Urb->Result == 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 = EFI_ALREADY_STARTED;
+    DEBUG ((DEBUG_INFO, "XhcDequeueTrbFromEndpoint: Pending URB is finished: Length Actual/Expect = %d/%d!\n", Urb->Completed, Urb->DataLen));
+  } else {
+    Status = XhcSetTrDequeuePointer(Xhc, SlotId, Dci, Urb);
+    if (EFI_ERROR (Status)) {
+      DEBUG ((DEBUG_ERROR, "XhcDequeueTrbFromEndpoint: Set Transfer Ring Dequeue Pointer Failed, Status = %r\n", Status));
+      goto Done;
+    }
   }
 
   //
@@ -1128,12 +1139,14 @@ XhcCheckUrbResult (
     TRBPtr = (TRB_TEMPLATE *)(UINTN) UsbHcGetHostAddrForPciAddr (Xhc->MemPool, (VOID *)(UINTN) PhyAddr, sizeof (TRB_TEMPLATE));
 
     //
-    // Update the status of Urb according to the finished event regardless of whether
-    // the urb is current checked one or in the XHCI's async transfer list.
+    // 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 events don't get
     // handled in time and are flushed by newer coming events.
     //
-    if (IsTransferRingTrb (Xhc, TRBPtr, Urb)) {
+    if (Xhc->PendingUrb != NULL && IsTransferRingTrb (Xhc, TRBPtr, Xhc->PendingUrb)) {
+      CheckedUrb = Xhc->PendingUrb;
+    } else if (IsTransferRingTrb (Xhc, TRBPtr, Urb)) {
       CheckedUrb = Urb;
     } else if (IsAsyncIntTrb (Xhc, TRBPtr, &AsyncUrb)) {    
       CheckedUrb = AsyncUrb;
@@ -1166,6 +1179,16 @@ XhcCheckUrbResult (
         DEBUG ((EFI_D_ERROR, "XhcCheckUrbResult: TRANSACTION_ERROR! Completecode = %x\n",EvtTrb->Completecode));
         goto EXIT;
 
+      case TRB_COMPLETION_STOPPED:
+      case TRB_COMPLETION_STOPPED_LENGTH_INVALID:
+        CheckedUrb->Result  |= EFI_USB_ERR_TIMEOUT;
+        CheckedUrb->Finished = TRUE;
+        //
+        // The pending URB is timeout and force stopped when stopping endpoint.
+        // 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 == 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 status when stopping the end point.
 
   @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 = 0x%x, Dci = 0x%x\n", SlotId, Dci));
 
   //
+  // When XhcCheckUrbResult waits for the Stop_Endpoint completion, it also 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 == NULL);
+  Xhc->PendingUrb = 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 received.
+  //
+  if (PendingUrb != NULL) {
+    PendingUrb->Result = 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 = %r\n", Status));
   }
 
+  Xhc->PendingUrb = NULL;
+
   return Status;
 }
 
@@ -3421,7 +3471,7 @@ XhcSetInterface (
       // XHCI 4.3.6 - Setting Alternate Interfaces
       // 1) Stop any Running Transfer Rings affected by the Alternate Interface setting.
       //
-      Status = XhcStopEndpoint (Xhc, SlotId, Dci);
+      Status = 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 Interface setting.
       //
-      Status = XhcStopEndpoint (Xhc, SlotId, Dci);
+      Status = 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 @@
 
   This file contains the definition for XHCI host controller schedule routines.
 
-Copyright (c) 2011 - 2015, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2011 - 2017, Intel Corporation. All rights reserved.<BR>
 This program and the accompanying materials
 are licensed and made available under the terms and conditions of the BSD License
 which accompanies this distribution.  The full text of the license may be 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
 
 //
 // The topology string used to present usb device location
@@ -1337,12 +1339,14 @@ XhcDequeueTrbFromEndpoint (
   IN  URB                 *Urb
   );
 
+
 /**
   Stop endpoint through XHCI's Stop_Endpoint cmd.
 
   @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 status when stopping the end point.
 
   @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
   );
 
 /**
-- 
2.12.2.windows.2

Re: [PATCH 3/3] MdeModulePkg/XhciDxe: Check timeout URB again after stopping endpoint

[Wu, Hao A]

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 Ruiyu
> 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
> 
> This fixes BULK data loss when transfer is detected as timeout but
> finished just before stopping endpoint.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Feng Tian <feng.tian@intel.com>
> ---
>  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(-)
> 
> 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 (
> 
>    Status = XhcExecTransfer (Xhc, FALSE, Urb, Timeout);
> 
> -  *TransferResult = Urb->Result;
> -  *DataLength     = Urb->Completed;
> 
>    if (Status == EFI_TIMEOUT) {
>      //
>      // The transfer timed out. Abort the transfer by dequeueing of the TD.
>      //
>      RecoveryStatus = XhcDequeueTrbFromEndpoint(Xhc, Urb);
> -    if (EFI_ERROR(RecoveryStatus)) {
> +    if (RecoveryStatus == EFI_ALREADY_STARTED) {
> +      //
> +      // The URB is finished just before stopping endpoint.
> +      // Change returning status from EFI_TIMEOUT to EFI_SUCCESS.
> +      //
> +      ASSERT (Urb->Result == EFI_USB_NOERROR);
> +      Status = EFI_SUCCESS;
> +      DEBUG ((DEBUG_ERROR, "XhcBulkTransfer: pending URB is finished,
> Length = %d.\n", Urb->Completed));
> +    } else if (EFI_ERROR(RecoveryStatus)) {
>        DEBUG((EFI_D_ERROR, "XhcBulkTransfer: XhcDequeueTrbFromEndpoint
> failed\n"));
>      }
> -  } else {
> -    if (*TransferResult == EFI_USB_NOERROR) {
> -      Status = EFI_SUCCESS;
> -    } else if (*TransferResult == EFI_USB_ERR_STALL) {
> -      RecoveryStatus = XhcRecoverHaltedEndpoint(Xhc, Urb);
> -      if (EFI_ERROR (RecoveryStatus)) {
> -        DEBUG ((EFI_D_ERROR, "XhcBulkTransfer: XhcRecoverHaltedEndpoint
> failed\n"));
> -      }
> -      Status = EFI_DEVICE_ERROR;
> +  }
> +
> +  *TransferResult = Urb->Result;
> +  *DataLength     = Urb->Completed;
> +
> +  if (*TransferResult == EFI_USB_ERR_STALL) {
> +    RecoveryStatus = XhcRecoverHaltedEndpoint(Xhc, Urb);
> +    if (EFI_ERROR (RecoveryStatus)) {
> +      DEBUG ((DEBUG_ERROR, "XhcBulkTransfer: XhcRecoverHaltedEndpoint
> failed\n"));
>      }
> +    ASSERT (Status == EFI_DEVICE_ERROR);
>    }
> 
>    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 @@
> 
>    Provides some data structure definitions used by the XHCI host controller
> driver.
> 
> -Copyright (c) 2011 - 2016, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2011 - 2017, Intel Corporation. All rights reserved.<BR>
>  This program and the accompanying materials
>  are licensed and made available under the terms and conditions of the BSD
> License
>  which accompanies this distribution.  The full text of the license may be 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.
> 
>    @retval EFI_SUCCESS           The dequeuing of the TDs is successful.
> +  @retval EFI_ALREADY_STARTED   The Urb is finished so no deque is needed.
>    @retval Others                Failed to stop the endpoint and dequeue the TDs.
> 
>  **/
> @@ -723,7 +724,7 @@ XhcDequeueTrbFromEndpoint (
>    //
>    // 1) Send Stop endpoint command to stop xHC from executing of the TDs on
> the endpoint
>    //
> -  Status = XhcStopEndpoint(Xhc, SlotId, Dci);
> +  Status = XhcStopEndpoint(Xhc, SlotId, Dci, Urb);
>    if (EFI_ERROR(Status)) {
>      DEBUG ((EFI_D_ERROR, "XhcDequeueTrbFromEndpoint: Stop Endpoint
> Failed, Status = %r\n", Status));
>      goto Done;
> @@ -732,10 +733,20 @@ XhcDequeueTrbFromEndpoint (
>    //
>    // 2)Set dequeue pointer
>    //
> -  Status = XhcSetTrDequeuePointer(Xhc, SlotId, Dci, Urb);
> -  if (EFI_ERROR(Status)) {
> -    DEBUG ((EFI_D_ERROR, "XhcDequeueTrbFromEndpoint: Set Transfer Ring
> Dequeue Pointer Failed, Status = %r\n", Status));
> -    goto Done;
> +  if (Urb->Finished && Urb->Result == 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 = EFI_ALREADY_STARTED;
> +    DEBUG ((DEBUG_INFO, "XhcDequeueTrbFromEndpoint: Pending URB is
> finished: Length Actual/Expect = %d/%d!\n", Urb->Completed, Urb->DataLen));
> +  } else {
> +    Status = XhcSetTrDequeuePointer(Xhc, SlotId, Dci, Urb);
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((DEBUG_ERROR, "XhcDequeueTrbFromEndpoint: Set Transfer
> Ring Dequeue Pointer Failed, Status = %r\n", Status));
> +      goto Done;
> +    }
>    }
> 
>    //
> @@ -1128,12 +1139,14 @@ XhcCheckUrbResult (
>      TRBPtr = (TRB_TEMPLATE *)(UINTN) UsbHcGetHostAddrForPciAddr (Xhc-
> >MemPool, (VOID *)(UINTN) PhyAddr, sizeof (TRB_TEMPLATE));
> 
>      //
> -    // Update the status of Urb according to the finished event regardless of
> whether
> -    // the urb is current checked one or in the XHCI's async transfer list.
> +    // 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 events don't
> get
>      // handled in time and are flushed by newer coming events.
>      //
> -    if (IsTransferRingTrb (Xhc, TRBPtr, Urb)) {
> +    if (Xhc->PendingUrb != NULL && IsTransferRingTrb (Xhc, TRBPtr, Xhc-
> >PendingUrb)) {
> +      CheckedUrb = Xhc->PendingUrb;
> +    } else if (IsTransferRingTrb (Xhc, TRBPtr, Urb)) {
>        CheckedUrb = Urb;
>      } else if (IsAsyncIntTrb (Xhc, TRBPtr, &AsyncUrb)) {
>        CheckedUrb = AsyncUrb;
> @@ -1166,6 +1179,16 @@ XhcCheckUrbResult (
>          DEBUG ((EFI_D_ERROR, "XhcCheckUrbResult: TRANSACTION_ERROR!
> Completecode = %x\n",EvtTrb->Completecode));
>          goto EXIT;
> 
> +      case TRB_COMPLETION_STOPPED:
> +      case TRB_COMPLETION_STOPPED_LENGTH_INVALID:
> +        CheckedUrb->Result  |= EFI_USB_ERR_TIMEOUT;
> +        CheckedUrb->Finished = TRUE;
> +        //
> +        // The pending URB is timeout and force stopped when stopping endpoint.
> +        // 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 == 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 status
> when stopping the end point.
> 
>    @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 = 0x%x, Dci = 0x%x\n", SlotId,
> Dci));
> 
>    //
> +  // When XhcCheckUrbResult waits for the Stop_Endpoint completion, it also
> 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 == NULL);
> +  Xhc->PendingUrb = 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 received.
> +  //
> +  if (PendingUrb != NULL) {
> +    PendingUrb->Result = 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
> = %r\n", Status));
>    }
> 
> +  Xhc->PendingUrb = NULL;
> +
>    return Status;
>  }
> 
> @@ -3421,7 +3471,7 @@ XhcSetInterface (
>        // XHCI 4.3.6 - Setting Alternate Interfaces
>        // 1) Stop any Running Transfer Rings affected by the Alternate Interface
> setting.
>        //
> -      Status = XhcStopEndpoint (Xhc, SlotId, Dci);
> +      Status = 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 Interface
> setting.
>        //
> -      Status = XhcStopEndpoint (Xhc, SlotId, Dci);
> +      Status = 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 @@
> 
>    This file contains the definition for XHCI host controller schedule routines.
> 
> -Copyright (c) 2011 - 2015, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2011 - 2017, Intel Corporation. All rights reserved.<BR>
>  This program and the accompanying materials
>  are licensed and made available under the terms and conditions of the BSD
> License
>  which accompanies this distribution.  The full text of the license may be 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
> 
>  //
>  // The topology string used to present usb device location
> @@ -1337,12 +1339,14 @@ XhcDequeueTrbFromEndpoint (
>    IN  URB                 *Urb
>    );
> 
> +
>  /**
>    Stop endpoint through XHCI's Stop_Endpoint cmd.
> 
>    @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 status
> when stopping the end point.
> 
>    @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
>    );
> 
>  /**
> --
> 2.12.2.windows.2
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH 0/3] Check timeout URB again after stopping endpoint

[Zeng, Star]

Ray,

Does XhciPei need the same update?


Thanks,
Star
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ruiyu Ni
Sent: Wednesday, June 28, 2017 6:40 PM
To: edk2-devel@lists.01.org
Subject: [edk2] [PATCH 0/3] Check timeout URB again after stopping endpoint

This fixes BULK data loss when transfer is detected as timeout but finished just before stopping endpoint.

Ruiyu Ni (3):
  MdeModulePkg/XhciDxe: Refine IsTransferRingTrb and IsAsyncIntTrb
  MdeModulePkg/XhciDxe: Dump the CMD/EVENT/INT/BULK ring information
  MdeModulePkg/XhciDxe: Check timeout URB again after stopping endpoint

 MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c      |  31 +++--
 MdeModulePkg/Bus/Pci/XhciDxe/Xhci.h      |   3 +-
 MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 190 +++++++++++++++++++++----------
 MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.h |   9 +-
 4 files changed, 159 insertions(+), 74 deletions(-)

--
2.12.2.windows.2

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH 0/3] Check timeout URB again after stopping endpoint

[Ni, Ruiyu]

This patch supports a certain USB device that:
1. contain BULK IN endpoint
2. BULK read from HOST gets timeout if
    a. there is no data from the endpoint in the device
    b. the data comes late

For now, I haven't found any other USB devices that meet the above conditions
except the Bluetooth host controller.
Given the fact PEI USB functionality is merely to support USB storage access,
I don't have chance to test in PEI phase if I made the similar change.

So I don't prefer to update the PEI XHCI module.

If we need to update PEI XHCI in future, we could do a "cherry-pick" later,
with necessary unit test.

Thanks/Ray

> -----Original Message-----
> From: Zeng, Star
> Sent: Wednesday, June 28, 2017 9:31 PM
> To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org
> Cc: Tian, Feng <feng.tian@intel.com>; Wu, Hao A <hao.a.wu@intel.com>
> Subject: RE: [edk2] [PATCH 0/3] Check timeout URB again after stopping
> endpoint
> 
> Ray,
> 
> Does XhciPei need the same update?
> 
> 
> Thanks,
> Star
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Ruiyu Ni
> Sent: Wednesday, June 28, 2017 6:40 PM
> To: edk2-devel@lists.01.org
> Subject: [edk2] [PATCH 0/3] Check timeout URB again after stopping
> endpoint
> 
> This fixes BULK data loss when transfer is detected as timeout but finished
> just before stopping endpoint.
> 
> Ruiyu Ni (3):
>   MdeModulePkg/XhciDxe: Refine IsTransferRingTrb and IsAsyncIntTrb
>   MdeModulePkg/XhciDxe: Dump the CMD/EVENT/INT/BULK ring
> information
>   MdeModulePkg/XhciDxe: Check timeout URB again after stopping endpoint
> 
>  MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c      |  31 +++--
>  MdeModulePkg/Bus/Pci/XhciDxe/Xhci.h      |   3 +-
>  MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 190
> +++++++++++++++++++++----------
>  MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.h |   9 +-
>  4 files changed, 159 insertions(+), 74 deletions(-)
> 
> --
> 2.12.2.windows.2
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH 0/3] Check timeout URB again after stopping endpoint

[Zeng, Star]

Get the point, thanks.


Star
-----Original Message-----
From: Ni, Ruiyu 
Sent: Thursday, June 29, 2017 10:50 AM
To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
Cc: Tian, Feng <feng.tian@intel.com>; Wu, Hao A <hao.a.wu@intel.com>
Subject: RE: [edk2] [PATCH 0/3] Check timeout URB again after stopping endpoint

This patch supports a certain USB device that:
1. contain BULK IN endpoint
2. BULK read from HOST gets timeout if
    a. there is no data from the endpoint in the device
    b. the data comes late

For now, I haven't found any other USB devices that meet the above conditions except the Bluetooth host controller.
Given the fact PEI USB functionality is merely to support USB storage access, I don't have chance to test in PEI phase if I made the similar change.

So I don't prefer to update the PEI XHCI module.

If we need to update PEI XHCI in future, we could do a "cherry-pick" later, with necessary unit test.

Thanks/Ray

> -----Original Message-----
> From: Zeng, Star
> Sent: Wednesday, June 28, 2017 9:31 PM
> To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org
> Cc: Tian, Feng <feng.tian@intel.com>; Wu, Hao A <hao.a.wu@intel.com>
> Subject: RE: [edk2] [PATCH 0/3] Check timeout URB again after stopping 
> endpoint
> 
> Ray,
> 
> Does XhciPei need the same update?
> 
> 
> Thanks,
> Star
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of 
> Ruiyu Ni
> Sent: Wednesday, June 28, 2017 6:40 PM
> To: edk2-devel@lists.01.org
> Subject: [edk2] [PATCH 0/3] Check timeout URB again after stopping 
> endpoint
> 
> This fixes BULK data loss when transfer is detected as timeout but 
> finished just before stopping endpoint.
> 
> Ruiyu Ni (3):
>   MdeModulePkg/XhciDxe: Refine IsTransferRingTrb and IsAsyncIntTrb
>   MdeModulePkg/XhciDxe: Dump the CMD/EVENT/INT/BULK ring information
>   MdeModulePkg/XhciDxe: Check timeout URB again after stopping 
> endpoint
> 
>  MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c      |  31 +++--
>  MdeModulePkg/Bus/Pci/XhciDxe/Xhci.h      |   3 +-
>  MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 190
> +++++++++++++++++++++----------
>  MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.h |   9 +-
>  4 files changed, 159 insertions(+), 74 deletions(-)
> 
> --
> 2.12.2.windows.2
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel