git: 2839ad58dd8a - main - sound: Fix hot-unload panics

The branch main has been updated by christos:

URL: https://cgit.FreeBSD.org/src/commit/?id=2839ad58dd8a4cf5294180fc599800c437a8d4d8

commit 2839ad58dd8a4cf5294180fc599800c437a8d4d8
Author:     Christos Margiolis <christos@FreeBSD.org>
AuthorDate: 2024-11-26 14:48:24 +0000
Commit:     Christos Margiolis <christos@FreeBSD.org>
CommitDate: 2024-11-26 14:48:24 +0000

    sound: Fix hot-unload panics
    
    This patch fixes multiple different panic scenarios occuring during
    hot-unload:
    
    1. The channel is unlocked in chn_read()/chn_write() for uiomove(9) and
       in the meantime we enter pcm_killchans() and free it. By the time we
       have returned from userland and try to lock it back, the channel will
       have been freed.
    2. The parent channel has been freed in pcm_killchans(), but at the same
       time, some yet-unstopped vchan's chn_read()/chn_write() calls
       chn_start(), which eventually calls vchan_trigger(), which references
       the freed parent.
    3. PCM_WAIT() panics because it references a freed PCM lock.
    
    For scenarios 1 and 2, refactor pcm_killchans() to first make sure all
    channels have been stopped, and then proceed to free them one by one, as
    opposed to freeing the first free channel until all channels have been
    freed. This change makes the code more robust, but might introduce some
    performance overhead when many channels are allocated, since we
    continuously loop through the channel list until all of them are
    stopped, and then we loop one last time to free them.
    
    For scenario 3, restructure the code so that we can use destroy_dev(9)
    instead of destroy_dev_sched(9) in dsp_destroy_dev(). Because
    destroy_dev(9) blocks until all references to the device have went away,
    we ensure that the PCM cv and lock will be freed safely.
    
    While here, move the delete_unrhdr(9) calls to pcm_killchans() and
    re-order some lines.
    
    Sponsored by:   The FreeBSD Foundation
    MFC after:      2 days
    Reviewed by:    dev_submerge.ch
    Differential Revision:  https://reviews.freebsd.org/D47462
---
 sys/dev/sound/pcm/dsp.c   |  2 +-
 sys/dev/sound/pcm/sound.c | 97 ++++++++++++++++++++---------------------------
 2 files changed, 43 insertions(+), 56 deletions(-)

diff --git a/sys/dev/sound/pcm/dsp.c b/sys/dev/sound/pcm/dsp.c
index 6a59bcfc1ade..0f19f064a227 100644
--- a/sys/dev/sound/pcm/dsp.c
+++ b/sys/dev/sound/pcm/dsp.c
@@ -137,7 +137,7 @@ dsp_destroy_dev(device_t dev)
 	struct snddev_info *d;
 
 	d = device_get_softc(dev);
-	destroy_dev_sched(d->dsp_dev);
+	destroy_dev(d->dsp_dev);
 }
 
 static void
diff --git a/sys/dev/sound/pcm/sound.c b/sys/dev/sound/pcm/sound.c
index fc9d13593b9e..c3e7fee41ab3 100644
--- a/sys/dev/sound/pcm/sound.c
+++ b/sys/dev/sound/pcm/sound.c
@@ -211,40 +211,53 @@ static void
 pcm_killchans(struct snddev_info *d)
 {
 	struct pcm_channel *ch;
-	bool found;
+	bool again;
 
 	PCM_BUSYASSERT(d);
-	do {
-		found = false;
+	KASSERT(!PCM_REGISTERED(d), ("%s(): still registered\n", __func__));
+
+	for (;;) {
+		again = false;
+		/* Make sure all channels are stopped. */
 		CHN_FOREACH(ch, d, channels.pcm) {
 			CHN_LOCK(ch);
-			/*
-			 * Make sure no channel has went to sleep in the
-			 * meantime.
-			 */
-			chn_shutdown(ch);
-			/*
-			 * We have to give a thread sleeping in chn_sleep() a
-			 * chance to observe that the channel is dead.
-			 */
-			if ((ch->flags & CHN_F_SLEEPING) == 0) {
-				found = true;
+			if ((ch->flags & CHN_F_SLEEPING) == 0 &&
+			    CHN_STOPPED(ch) && ch->inprog == 0) {
 				CHN_UNLOCK(ch);
-				break;
+				continue;
 			}
+			chn_shutdown(ch);
+			if (ch->direction == PCMDIR_PLAY)
+				chn_flush(ch);
+			else
+				chn_abort(ch);
 			CHN_UNLOCK(ch);
+			again = true;
 		}
-
 		/*
-		 * All channels are still sleeping. Sleep for a bit and try
-		 * again to see if any of them is awake now.
+		 * Some channels are still active. Sleep for a bit and try
+		 * again.
 		 */
-		if (!found) {
-			pause_sbt("pcmkillchans", SBT_1MS * 5, 0, 0);
-			continue;
-		}
+		if (again)
+			pause_sbt("pcmkillchans", mstosbt(5), 0, 0);
+		else
+			break;
+	}
+
+	/* All channels are finally dead. */
+	while (!CHN_EMPTY(d, channels.pcm)) {
+		ch = CHN_FIRST(d, channels.pcm);
 		chn_kill(ch);
-	} while (!CHN_EMPTY(d, channels.pcm));
+	}
+
+	if (d->p_unr != NULL)
+		delete_unrhdr(d->p_unr);
+	if (d->vp_unr != NULL)
+		delete_unrhdr(d->vp_unr);
+	if (d->r_unr != NULL)
+		delete_unrhdr(d->r_unr);
+	if (d->vr_unr != NULL)
+		delete_unrhdr(d->vr_unr);
 }
 
 static int
@@ -512,7 +525,6 @@ int
 pcm_unregister(device_t dev)
 {
 	struct snddev_info *d;
-	struct pcm_channel *ch;
 
 	d = device_get_softc(dev);
 
@@ -525,28 +537,15 @@ pcm_unregister(device_t dev)
 	PCM_WAIT(d);
 
 	d->flags |= SD_F_DETACHING;
+	d->flags |= SD_F_DYING;
+	d->flags &= ~SD_F_REGISTERED;
 
 	PCM_ACQUIRE(d);
 	PCM_UNLOCK(d);
 
-	CHN_FOREACH(ch, d, channels.pcm) {
-		CHN_LOCK(ch);
-		/*
-		 * Do not wait for the timeout in chn_read()/chn_write(). Wake
-		 * up the sleeping thread and kill the channel.
-		 */
-		chn_shutdown(ch);
-		chn_abort(ch);
-		CHN_UNLOCK(ch);
-	}
+	pcm_killchans(d);
 
-	/* remove /dev/sndstat entry first */
-	sndstat_unregister(dev);
-
-	PCM_LOCK(d);
-	d->flags |= SD_F_DYING;
-	d->flags &= ~SD_F_REGISTERED;
-	PCM_UNLOCK(d);
+	PCM_RELEASE_QUICK(d);
 
 	if (d->play_sysctl_tree != NULL) {
 		sysctl_ctx_free(&d->play_sysctl_ctx);
@@ -557,24 +556,12 @@ pcm_unregister(device_t dev)
 		d->rec_sysctl_tree = NULL;
 	}
 
+	sndstat_unregister(dev);
+	mixer_uninit(dev);
 	dsp_destroy_dev(dev);
-	(void)mixer_uninit(dev);
-
-	pcm_killchans(d);
 
-	PCM_LOCK(d);
-	PCM_RELEASE(d);
 	cv_destroy(&d->cv);
-	PCM_UNLOCK(d);
 	snd_mtxfree(d->lock);
-	if (d->p_unr != NULL)
-		delete_unrhdr(d->p_unr);
-	if (d->vp_unr != NULL)
-		delete_unrhdr(d->vp_unr);
-	if (d->r_unr != NULL)
-		delete_unrhdr(d->r_unr);
-	if (d->vr_unr != NULL)
-		delete_unrhdr(d->vr_unr);
 
 	if (snd_unit == device_get_unit(dev)) {
 		snd_unit = pcm_best_unit(-1);