Changeset 100774 in vbox for trunk/src

Timestamp:

Aug 1, 2023 8:01:46 PM (19 months ago)

Author:

vboxsync

Message:

DrvVUSBRootHub: Attempt at fixing situation where vusbR3RhSavePrep encounters a non-NULL pLoad member because the timer hasn't run yet. Immediate issue was using the wrong free function, but further issues turned up after that, leading to performing the depending re-attach prior whatever vusbR3RhSavePrep does so the state is somewhat sane. Regression from r150140, possibly.

File:

• trunk/src/VBox/Devices/USB/DrvVUSBRootHub.cpp (modified) (10 diffs)

trunk/src/VBox/Devices/USB/DrvVUSBRootHub.cpp

@@ -419 +419 @@
 static int vusbHubDetach(PVUSBROOTHUB pThis, PVUSBDEV pDev)
 {
-    Assert(pDev->i16Port != -1);
+    /*
+     * It is possible to race the re-attach timer callback in some extreme cases,
+     * typically involving custom VBox builds that does very little guest code
+     * execution before terminating the VM again (e.g. IEM debugging).
+     */
+    Assert(pDev->i16Port != -1 || pThis->pLoad);
+    if (pDev->i16Port == -1 && pThis->pLoad)
+        return VINF_SUCCESS;
 
     /*
@@ -1305 +1312 @@
 
 /**
+ * Helper that frees up pThis->pLoad.
+ *
+ * This is called in a few places.
+ */
+static void vushR3RhFreeLoadData(PVUSBROOTHUB pThis, PPDMDRVINS pDrvIns)
+{
+    PVUSBROOTHUBLOAD const pLoad = pThis->pLoad;
+    if (pLoad)
+    {
+        pThis->pLoad = NULL;
+        PDMDrvHlpTimerDestroy(pDrvIns, pLoad->hTimer);
+        pLoad->hTimer = NIL_TMTIMERHANDLE;
+        RTMemFree(pLoad);
+    }
+}
+
+
+/**
  * @callback_method_impl{FNSSMDRVSAVEPREP, All URBs needs to be canceled.}
  */
@@ -1312 +1337 @@
     LogFlow(("vusbR3RhSavePrep:\n"));
     RT_NOREF(pSSM);
+
+    /*
+     * If there is old load state hanging around, we'll have to execute it first
+     * to get the hub into the right state prior to saving. This isn't entirely
+     * right wrt snapshotting and continuing execution, but OTOH it will screw up
+     * if shutting down afterwards.
+     */
+    PVUSBROOTHUBLOAD const pLoad = pThis->pLoad;
+    if (pLoad)
+    {
+        for (unsigned i = 0; i < pLoad->cDevs; i++)
+            vusbHubAttach(pThis, pLoad->apDevs[i]);
+        vushR3RhFreeLoadData(pThis, pDrvIns);
+    }
 
     /*
@@ -1341 +1380 @@
     RTCritSectLeave(&pThis->CritSectDevices);
 
-    /*
-     * Kill old load data which might be hanging around.
-     */
-    if (pThis->pLoad)
-    {
-        PDMDrvHlpTimerDestroy(pDrvIns, pThis->pLoad->hTimer);
-        pThis->pLoad->hTimer = NIL_TMTIMERHANDLE;
-        PDMDrvHlpMMHeapFree(pDrvIns, pThis->pLoad);
-        pThis->pLoad = NULL;
-    }
-
     return VINF_SUCCESS;
 }
@@ -1406 +1434 @@
     if (!pThis->pLoad)
     {
+        /** @todo allocate first, it may fail later and we'll potentially leave things
+         *        dangling. */
         VUSBROOTHUBLOAD Load;
         unsigned        i;
@@ -1432 +1462 @@
         if (Load.cDevs)
         {
-            pThis->pLoad = (PVUSBROOTHUBLOAD)RTMemAllocZ(sizeof(Load));
+            pThis->pLoad = (PVUSBROOTHUBLOAD)RTMemDup(&Load, sizeof(Load));
             if (!pThis->pLoad)
                 return VERR_NO_MEMORY;
-            *pThis->pLoad = Load;
         }
     }
@@ -1445 +1474 @@
 
 /**
- * Reattaches devices after a saved state load.
+ * Timer callback that reattaches devices after a saved state load.
+ *
  */
 static DECLCALLBACK(void) vusbR3RhLoadReattachDevices(PPDMDRVINS pDrvIns, TMTIMERHANDLE hTimer, void *pvUser)
@@ -1451 +1481 @@
     PVUSBROOTHUB        pThis = PDMINS_2_DATA(pDrvIns, PVUSBROOTHUB);
     PVUSBROOTHUBLOAD    pLoad = pThis->pLoad;
+    AssertPtrReturnVoid(pLoad);
     LogFlow(("vusbR3RhLoadReattachDevices:\n"));
     Assert(hTimer == pLoad->hTimer); RT_NOREF(pvUser);
@@ -1463 +1494 @@
      * Cleanup.
      */
-    PDMDrvHlpTimerDestroy(pDrvIns, hTimer);
-    pLoad->hTimer = NIL_TMTIMERHANDLE;
-    RTMemFree(pLoad);
-    pThis->pLoad = NULL;
+    vushR3RhFreeLoadData(pThis, pDrvIns);
 }
 
@@ -1482 +1510 @@
      * Start a timer if we've got devices to reattach
      */
-    if (pThis->pLoad)
+    PVUSBROOTHUBLOAD const pLoad = pThis->pLoad;
+    if (pLoad)
     {
         int rc = PDMDrvHlpTMTimerCreate(pDrvIns, TMCLOCK_VIRTUAL, vusbR3RhLoadReattachDevices, NULL,
                                         TMTIMER_FLAGS_NO_CRIT_SECT | TMTIMER_FLAGS_NO_RING0,
-                                        "VUSB reattach on load", &pThis->pLoad->hTimer);
+                                        "VUSB reattach on load", &pLoad->hTimer);
+        AssertLogRelRC(rc);
         if (RT_SUCCESS(rc))
-            rc = PDMDrvHlpTimerSetMillies(pDrvIns, pThis->pLoad->hTimer, 250);
+            rc = PDMDrvHlpTimerSetMillies(pDrvIns, pLoad->hTimer, 250);
+        else
+            vushR3RhFreeLoadData(pThis, pDrvIns); /** @todo or call vusbR3RhLoadReattachDevices directly then fail? */
         return rc;
     }