MdeModulePkg/XhciDxe: Fix Broken Timeouts

REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2948

Timeouts in the XhciDxe driver are taking longer than
expected due to the timeout loops not accounting for
code execution time. As en example, 5 second timeouts
have been observed to take around 36 seconds to complete.
Use SetTimer and Create/CheckEvent from Boot Services to
determine when timeout occurred.

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Signed-off-by: Patrick Henz <patrick.henz@hpe.com>
Reviewed-by: Hao A Wu <hao.a.wu@intel.com>

MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c

@@ -423,14 +423,15 @@ XhcClearOpRegBit (
   Wait the operation register's bit as specified by Bit
   to become set (or clear).
 
-  @param  Xhc          The XHCI Instance.
+  @param  Xhc                    The XHCI Instance.
-  @param  Offset       The offset of the operation register.
+  @param  Offset                 The offset of the operation register.
-  @param  Bit          The bit of the register to wait for.
+  @param  Bit                    The bit of the register to wait for.
-  @param  WaitToSet    Wait the bit to set or clear.
+  @param  WaitToSet              Wait the bit to set or clear.
-  @param  Timeout      The time to wait before abort (in millisecond, ms).
+  @param  Timeout                The time to wait before abort (in millisecond, ms).
 
-  @retval EFI_SUCCESS  The bit successfully changed by host controller.
+  @retval EFI_SUCCESS            The bit successfully changed by host controller.
-  @retval EFI_TIMEOUT  The time out occurred.
+  @retval EFI_TIMEOUT            The time out occurred.
+  @retval EFI_OUT_OF_RESOURCES   Memory for the timer event could not be allocated.
 
 **/
 EFI_STATUS
@@ -442,20 +443,52 @@ XhcWaitOpRegBit (
   IN UINT32               Timeout
   )
 {
-  UINT32                  Index;
+  EFI_STATUS Status;
-  UINT64                  Loop;
+  EFI_EVENT  TimeoutEvent;
 
-  Loop   = Timeout * XHC_1_MILLISECOND;
+  TimeoutEvent = NULL;
 
-  for (Index = 0; Index < Loop; Index++) {
+  if (Timeout == 0) {
+    return EFI_TIMEOUT;
+  }
+
+  Status = gBS->CreateEvent (
+                  EVT_TIMER,
+                  TPL_CALLBACK,
+                  NULL,
+                  NULL,
+                  &TimeoutEvent
+                  );
+
+  if (EFI_ERROR(Status)) {
+    goto DONE;
+  }
+
+  Status = gBS->SetTimer (TimeoutEvent,
+                          TimerRelative,
+                          EFI_TIMER_PERIOD_MILLISECONDS(Timeout));
+
+  if (EFI_ERROR(Status)) {
+    goto DONE;
+  }
+
+  do {
     if (XHC_REG_BIT_IS_SET (Xhc, Offset, Bit) == WaitToSet) {
-      return EFI_SUCCESS;
+      Status = EFI_SUCCESS;
+      goto DONE;
     }
 
     gBS->Stall (XHC_1_MICROSECOND);
+  } while (EFI_ERROR(gBS->CheckEvent (TimeoutEvent)));
+
+  Status = EFI_TIMEOUT;
+
+DONE:
+  if (TimeoutEvent != NULL) {
+    gBS->CloseEvent (TimeoutEvent);
   }
 
-  return EFI_TIMEOUT;
+  return Status;
 }
 
 /**

MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c

@@ -1254,14 +1254,15 @@ EXIT:
 /**
   Execute the transfer by polling the URB. This is a synchronous operation.
 
-  @param  Xhc               The XHCI Instance.
+  @param  Xhc                    The XHCI Instance.
-  @param  CmdTransfer       The executed URB is for cmd transfer or not.
+  @param  CmdTransfer            The executed URB is for cmd transfer or not.
-  @param  Urb               The URB to execute.
+  @param  Urb                    The URB to execute.
-  @param  Timeout           The time to wait before abort, in millisecond.
+  @param  Timeout                The time to wait before abort, in millisecond.
 
-  @return EFI_DEVICE_ERROR  The transfer failed due to transfer error.
+  @return EFI_DEVICE_ERROR       The transfer failed due to transfer error.
-  @return EFI_TIMEOUT       The transfer failed due to time out.
+  @return EFI_TIMEOUT            The transfer failed due to time out.
-  @return EFI_SUCCESS       The transfer finished OK.
+  @return EFI_SUCCESS            The transfer finished OK.
+  @retval EFI_OUT_OF_RESOURCES   Memory for the timer event could not be allocated.
 
 **/
 EFI_STATUS
@@ -1273,11 +1274,16 @@ XhcExecTransfer (
   )
 {
   EFI_STATUS              Status;
-  UINTN                   Index;
-  UINT64                  Loop;
   UINT8                   SlotId;
   UINT8                   Dci;
   BOOLEAN                 Finished;
+  EFI_EVENT               TimeoutEvent;
+  BOOLEAN                 IndefiniteTimeout;
+
+  Status            = EFI_SUCCESS;
+  Finished          = FALSE;
+  TimeoutEvent      = NULL;
+  IndefiniteTimeout = FALSE;
 
   if (CmdTransfer) {
     SlotId = 0;
@@ -1291,29 +1297,56 @@ XhcExecTransfer (
     ASSERT (Dci < 32);
   }
 
-  Status = EFI_SUCCESS;
-  Loop   = Timeout * XHC_1_MILLISECOND;
   if (Timeout == 0) {
-    Loop = 0xFFFFFFFF;
+    IndefiniteTimeout = TRUE;
+    goto RINGDOORBELL;
   }
 
+  Status = gBS->CreateEvent (
+                  EVT_TIMER,
+                  TPL_CALLBACK,
+                  NULL,
+                  NULL,
+                  &TimeoutEvent
+                  );
+
+  if (EFI_ERROR (Status)) {
+    goto DONE;
+  }
+
+  Status = gBS->SetTimer (TimeoutEvent,
+                          TimerRelative,
+                          EFI_TIMER_PERIOD_MILLISECONDS(Timeout));
+
+  if (EFI_ERROR (Status)) {
+    goto DONE;
+  }
+
+RINGDOORBELL:
   XhcRingDoorBell (Xhc, SlotId, Dci);
 
-  for (Index = 0; Index < Loop; Index++) {
+  do {
     Finished = XhcCheckUrbResult (Xhc, Urb);
     if (Finished) {
       break;
     }
     gBS->Stall (XHC_1_MICROSECOND);
-  }
+  } while (IndefiniteTimeout || EFI_ERROR(gBS->CheckEvent (TimeoutEvent)));
 
-  if (Index == Loop) {
+DONE:
+  if (EFI_ERROR(Status)) {
+    Urb->Result = EFI_USB_ERR_NOTEXECUTE;
+  } else if (!Finished) {
     Urb->Result = EFI_USB_ERR_TIMEOUT;
     Status      = EFI_TIMEOUT;
   } else if (Urb->Result != EFI_USB_NOERROR) {
     Status      = EFI_DEVICE_ERROR;
   }
 
+  if (TimeoutEvent != NULL) {
+    gBS->CloseEvent (TimeoutEvent);
+  }
+
   return Status;
 }