Changeset 90026 in vbox for trunk/src/VBox/Devices

Timestamp:

Jul 5, 2021 1:54:56 PM (3 years ago)

Author:

vboxsync

Message:

VUSB/Win: Improved logging, reworked Windows backend so that usbProxyWinUrbReap() doesn't return NULL for no good reason.

File:

• trunk/src/VBox/Devices/USB/win/USBProxyDevice-win.cpp (modified) (11 diffs)

trunk/src/VBox/Devices/USB/win/USBProxyDevice-win.cpp

@@ -57 +57 @@
     uint8_t         bInterfaceNumber;
     bool            fClaimed;
+    /** Set if reaper should exit ASAP. */
+    bool            fWakeUpNow;
     /** The allocated size of paHandles and paQueuedUrbs. */
     unsigned        cAllocatedUrbs;
@@ -97 +99 @@
         || dwErr == ERROR_BAD_COMMAND)
     {
-        Log(("usbproxy: device %x unplugged!!\n", pPriv->hDev));
+        Log(("usbproxy: device %x unplugged!! (usbProxyWinHandleUnpluggedDevice)\n", pPriv->hDev));
         pProxyDev->fDetached = true;
     }
@@ -277 +279 @@
     if (rc == ERROR_DEVICE_REMOVED)
     {
-        Log(("usbproxy: device %p unplugged!!\n", pPriv->hDev));
+        Log(("usbproxy: device %p unplugged!! (usbProxyWinReset)\n", pPriv->hDev));
         pProxyDev->fDetached = true;
     }
@@ -495 +497 @@
                 || dwErr == ERROR_BAD_COMMAND)
             {
-                Log(("usbproxy: device %p unplugged!!\n", pPriv->hDev));
+                Log(("usbproxy: device %p unplugged!! (usbProxyWinUrbQueue)\n", pPriv->hDev));
                 pProxyDev->fDetached = true;
             }
@@ -566 +568 @@
         && pPriv->cPendingUrbs == 0)
     {
+        Log(("usbproxy: Nothing pending\n"));
         if (   cMillies != 0
             && pPriv->cPendingUrbs == 0)
         {
             /* Wait for the wakeup call. */
+            Log(("usbproxy: Waiting for wakeup call\n"));
             DWORD cMilliesWait = cMillies == RT_INDEFINITE_WAIT ? INFINITE : cMillies;
-            DWORD dwRc = WaitForMultipleObjects(1, &pPriv->hEventWakeup, FALSE, cMilliesWait);
-            NOREF(dwRc);
-        }
-
+            DWORD rc = WaitForMultipleObjects(1, &pPriv->hEventWakeup, FALSE, cMilliesWait);
+            Log(("usbproxy: Initial wait rc=%X\n", rc));
+            if (rc != WAIT_OBJECT_0) {
+                Log(("usbproxy: Initial wait failed, rc=%X\n", rc));
+                return NULL;
+            }
+        }
         return NULL;
     }
@@ -580 +587 @@
 again:
     /* Check for pending URBs. */
+    Log(("usbproxy: %u pending URBs\n", pPriv->cPendingUrbs));
     if (pPriv->cPendingUrbs)
     {
@@ -623 +631 @@
      * Wait/poll.
      *
-     * ASSUMPTIONS:
-     *   1. The usbProxyWinUrbReap can not be run concurrently with each other
-     *      so racing the cQueuedUrbs access/modification can not occur.
-     *   2. The usbProxyWinUrbReap can not be run concurrently with
-     *      usbProxyWinUrbQueue so they can not race the pPriv->paHandles
-     *      access/realloc.
+     * ASSUMPTION: Multiple usbProxyWinUrbReap calls can not be run concurrently
+     *   with each other so racing the cQueuedUrbs access/modification can not occur.
+     *
+     * However, usbProxyWinUrbReap can be run concurrently with usbProxyWinUrbQueue
+     * and pPriv->paHandles access/realloc must be synchronized.
+     *
+     * NB: Due to the design of Windows overlapped I/O, DeviceIoControl calls to submit
+     * URBs use individual event objects. When a new URB is submitted, we have to add its
+     * event object to the list of objects that WaitForMultipleObjects is waiting on. Thus
+     * hEventWakeup has dual purpose, serving to handle proxy wakeup calls meant to abort
+     * reaper waits, but also waking up the reaper after every URB submit so that the newly
+     * submitted URB can be added to the list of waiters.
      */
     unsigned cQueuedUrbs = ASMAtomicReadU32((volatile uint32_t *)&pPriv->cQueuedUrbs);
@@ -634 +648 @@
     PVUSBURB pUrb = NULL;
     DWORD rc = WaitForMultipleObjects(cQueuedUrbs + 1, pPriv->paHandles, FALSE, cMilliesWait);
+    Log(("usbproxy: Wait (%d milliseconds) returned with rc=%X\n", cMilliesWait, rc));
 
     /* If the wakeup event fired return immediately. */
     if (rc == WAIT_OBJECT_0 + cQueuedUrbs)
     {
-        if (pPriv->cPendingUrbs)
-            goto again;
-        return NULL;
+        /* Get outta here flag set? If so, bail now. */
+        if (ASMAtomicXchgBool(&pPriv->fWakeUpNow, false))
+        {
+            Log(("usbproxy: Reaper woken up, returning NULL\n"));
+            return NULL;
+        }
+
+        /* A new URBs was queued through usbProxyWinUrbQueue() and needs to be
+         * added to the wait list. Go again.
+         */
+        Log(("usbproxy: Reaper woken up after queuing new URB, go again.\n"));
+        goto again;
     }
 
@@ -724 +748 @@
     AssertPtrReturn(pQUrbWin, VERR_INVALID_PARAMETER);
 
-    in.bEndpoint = pUrb->EndPt | (pUrb->enmDir == VUSBDIRECTION_IN ? 0x80 : 0);
+    in.bEndpoint = pUrb->EndPt | ((pUrb->EndPt && pUrb->enmDir == VUSBDIRECTION_IN) ? 0x80 : 0);
     Log(("usbproxy: Cancel urb %p, endpoint %x\n", pUrb, in.bEndpoint));
 
@@ -735 +759 @@
         || dwErr == ERROR_BAD_COMMAND)
     {
-        Log(("usbproxy: device %x unplugged!!\n", pPriv->hDev));
+        Log(("usbproxy: device %x unplugged!! (usbProxyWinUrbCancel)\n", pPriv->hDev));
         pProxyDev->fDetached = true;
         return VINF_SUCCESS; /* Fake success and deal with the unplugged device elsewhere. */
@@ -748 +772 @@
     PPRIV_USBW32 pPriv = USBPROXYDEV_2_DATA(pProxyDev, PPRIV_USBW32);
 
+    Log(("usbproxy: device %x wakeup\n", pPriv->hDev));
+    ASMAtomicXchgBool(&pPriv->fWakeUpNow, true);
     SetEvent(pPriv->hEventWakeup);
     return VINF_SUCCESS;