Changeset 82343 in vbox for trunk/src

Timestamp:

Dec 3, 2019 11:27:10 AM (5 years ago)

Author:

vboxsync

Message:

DevIchAc97: Cleanups. bugref:9218

File:

• trunk/src/VBox/Devices/Audio/DevIchAc97.cpp (modified) (18 diffs)

trunk/src/VBox/Devices/Audio/DevIchAc97.cpp

@@ -143 +143 @@
 /** @} */
 
-/* AC'97 uses 1.5dB steps, we use 0.375dB steps: 1 AC'97 step equals 4 PDM steps. */
+/** AC'97 uses 1.5dB steps, we use 0.375dB steps: 1 AC'97 step equals 4 PDM steps. */
 #define AC97_DB_FACTOR                  4
 
@@ -265 +265 @@
 *   Structures and Typedefs                                                                                                      *
 *********************************************************************************************************************************/
+/** The ICH AC'97 (Intel) controller. */
+typedef struct AC97STATE *PAC97STATE;
 
 /**
@@ -301 +303 @@
 #ifdef VBOX_WITH_AUDIO_AC97_ASYNC_IO
 /**
- * Structure keeping the AC'97 stream's state for asynchronous I/O.
+ * Asynchronous I/O state for an AC'97 stream.
  */
 typedef struct AC97STREAMSTATEAIO
@@ -318 +320 @@
     volatile bool           fEnabled;
     bool                    afPadding[5];
-} AC97STREAMSTATEAIO, *PAC97STREAMSTATEAIO;
+} AC97STREAMSTATEAIO;
+/** Pointer to the async I/O state for an AC'97 stream. */
+typedef AC97STREAMSTATEAIO *PAC97STREAMSTATEAIO;
 #endif
 
-/** The ICH AC'97 (Intel) controller. */
-typedef struct AC97STATE *PAC97STATE;
-
-/**
- * Structure for keeping the internal state of an AC'97 stream.
+
+/**
+ * The internal state of an AC'97 stream.
  */
 typedef struct AC97STREAMSTATE
@@ -367 +369 @@
 
 /**
- * Structure containing AC'97 stream debug stuff, configurable at runtime.
- */
-typedef struct AC97STREAMDBGINFORT
+ * Runtime configurable debug stuff for an AC'97 stream.
+ */
+typedef struct AC97STREAMDEBUGRT
 {
     /** Whether debugging is enabled or not. */
@@ -382 +384 @@
      *  whereas for output streams this dumps data being read from the device DMA. */
     R3PTRTYPE(PPDMAUDIOFILE) pFileDMA;
-} AC97STREAMDBGINFORT, *PAC97STREAMDBGINFORT;
-
-/**
- * Structure containing AC'97 stream debug information.
- */
-typedef struct AC97STREAMDBGINFO
-{
-    /** Runtime debug info. */
-    AC97STREAMDBGINFORT      Runtime;
-} AC97STREAMDBGINFO ,*PAC97STREAMDBGINFO;
-
-/**
- * Structure for an AC'97 stream.
+} AC97STREAMDEBUGRT;
+
+/**
+ * Debug stuff for an AC'97 stream.
+ */
+typedef struct AC97STREAMDEBUG
+{
+    /** Runtime debug stuff. */
+    AC97STREAMDEBUGRT       Runtime;
+} AC97STREAMDEBUG;
+
+/**
+ * An AC'97 stream.
  */
 typedef struct AC97STREAM
@@ -410 +412 @@
     uint32_t              Padding1;
 #endif
-    /** Debug information. */
-    AC97STREAMDBGINFO     Dbg;
-} AC97STREAM, *PAC97STREAM;
+    /** Debug stuff. */
+    AC97STREAMDEBUG         Dbg;
+} AC97STREAM;
 AssertCompileSizeAlignment(AC97STREAM, 8);
 /** Pointer to an AC'97 stream (registers + state). */
@@ -420 +422 @@
 #ifdef VBOX_WITH_AUDIO_AC97_ASYNC_IO
 /**
- * Structure for the async I/O thread context.
+ * Asynchronous I/O thread context (arguments).
  */
 typedef struct AC97STREAMTHREADCTX
@@ -426 +428 @@
     PAC97STATE  pThis;
     PAC97STREAM pStream;
-} AC97STREAMTHREADCTX, *PAC97STREAMTHREADCTX;
+} AC97STREAMTHREADCTX;
+/** Pointer to the context for an async I/O thread. */
+typedef AC97STREAMTHREADCTX *PAC97STREAMTHREADCTX;
 #endif
 
 /**
- * Structure defining a (host backend) driver stream.
+ * A driver stream (host backend).
+ *
  * Each driver has its own instances of audio mixer streams, which then
  * can go into the same (or even different) audio mixer sinks.
@@ -438 +443 @@
     /** Associated mixer stream handle. */
     R3PTRTYPE(PAUDMIXSTREAM)           pMixStrm;
-} AC97DRIVERSTREAM, *PAC97DRIVERSTREAM;
-
-/**
- * Struct for maintaining a host backend driver.
+} AC97DRIVERSTREAM;
+/** Pointer to a driver stream. */
+typedef AC97DRIVERSTREAM *PAC97DRIVERSTREAM;
+
+/**
+ * A host backend driver (LUN).
  */
 typedef struct AC97DRIVER
@@ -468 +475 @@
     /** Driver stream for output. */
     AC97DRIVERSTREAM                   Out;
-} AC97DRIVER, *PAC97DRIVER;
-
-typedef struct AC97STATEDBGINFO
+} AC97DRIVER;
+/** Pointer to a host backend driver (LUN). */
+typedef AC97DRIVER *PAC97DRIVER;
+
+/**
+ * Debug settings.
+ */
+typedef struct AC97STATEDEBUG
 {
     /** Whether debugging is enabled or not. */
@@ -476 +488 @@
     /** Path where to dump the debug output to.
      *  Defaults to VBOX_AUDIO_DEBUG_DUMP_PCM_DATA_PATH. */
-    char                               szOutPath[RTPATH_MAX + 1];
-} AC97STATEDBGINFO, *PAC97STATEDBGINFO;
-
-/**
- * Structure for maintaining an AC'97 device state.
+    char                               szOutPath[RTPATH_MAX];
+} AC97STATEDEBUG;
+
+
+/**
+ * The shared AC'97 device state.
  */
 typedef struct AC97STATE
@@ -524 +537 @@
     /** The base interface for LUN\#0. */
     PDMIBASE                IBase;
-    AC97STATEDBGINFO        Dbg;
+    /** Debug settings. */
+    AC97STATEDEBUG          Dbg;
 
     /** PCI region \#0: NAM I/O ports. */
@@ -540 +554 @@
 } AC97STATE;
 AssertCompileMemberAlignment(AC97STATE, aStreams, 8);
-/** Pointer to a AC'97 state. */
-typedef AC97STATE *PAC97STATE;
+
 
 /**
@@ -633 +646 @@
 
 # ifdef VBOX_WITH_AUDIO_AC97_ASYNC_IO
-static DECLCALLBACK(int)  ichac97R3StreamAsyncIOThread(RTTHREAD hThreadSelf, void *pvUser);
 static int                ichac97R3StreamAsyncIOCreate(PAC97STATE pThis, PAC97STREAM pStream);
 static int                ichac97R3StreamAsyncIODestroy(PAC97STATE pThis, PAC97STREAM pStream);
@@ -952 +964 @@
             RTStrPrintf(szFile, sizeof(szFile), "ac97StreamReadSD%RU8", pStream->u8SD);
 
-        char szPath[RTPATH_MAX + 1];
+        char szPath[RTPATH_MAX];
         int rc2 = DrvAudioHlpFileNameGet(szPath, sizeof(szPath), pThis->Dbg.szOutPath, szFile,
                                          0 /* uInst */, PDMAUDIOFILETYPE_WAV, PDMAUDIOFILENAME_FLAGS_NONE);
@@ -1207 +1219 @@
     RTThreadUserSignal(hThreadSelf);
 
+    /** @todo r=bird: What wasn't mentioned by the original author of this
+     *        code, is that pCtx is now invalid as it must be assumed to be out
+     *        of scope in the parent thread.  It is a 'ing stack object! */
+
     LogFunc(("[SD%RU8] Started\n", pStream->u8SD));
 
@@ -1269 +1285 @@
             if (RT_SUCCESS(rc))
             {
+/** @todo r=bird: Why is Ctx on the stack?  There is no mention of this in
+ *        the thread structure.  Besides, you only wait 10seconds, if the
+ *        host is totally overloaded, it may go out of scope before the new
+ *        thread has finished with it and it will like crash and burn.
+ *
+ *        Also, there is RTThreadCreateF for giving threads complicated
+ *        names.
+ *
+ *        Why aren't this code using the PDM threads (PDMDevHlpThreadCreate)?
+ *        They would help you with managing stuff like VM suspending, resuming
+ *        and powering off.
+ *
+ *        Finally, just create the threads at construction time. */
                 AC97STREAMTHREADCTX Ctx = { pThis, pStream };
+# error "Busted code! Do not pass a structure living on the parent stack to the poor thread!"
 
                 char szThreadName[64];