git: 99b8be414cbc - releng/14.3 - sound: Terminate stream properly when closing vchans
- Go to: [ bottom of page ] [ top of archives ] [ this month ]
Date: Thu, 29 May 2025 22:51:56 UTC
The branch releng/14.3 has been updated by christos:
URL: https://cgit.FreeBSD.org/src/commit/?id=99b8be414cbcb4577859985ef80eb053d761ae5f
commit 99b8be414cbcb4577859985ef80eb053d761ae5f
Author: Christos Margiolis <christos@FreeBSD.org>
AuthorDate: 2025-05-29 10:24:17 +0000
Commit: Christos Margiolis <christos@FreeBSD.org>
CommitDate: 2025-05-29 22:51:30 +0000
sound: Terminate stream properly when closing vchans
When a channel is closed, dsp_close() either calls vchan_destroy() on vchans,
or chn_abort()/chn_flush() on primary channels. However, the problem with this
is that, when closing a vchan, we end up not terminating the stream properly.
The call sequence we are interested in is the following:
vchan_destroy(vchan) -> chn_kill(vchan) -> chn_trigger(vchan) ->
vchan_trigger(vchan) -> chn_notify(parent)
Even though chn_notify() contains codepaths which call chn_abort(parent),
apparently we do not execute any of those codepaths in this case, so the
DMA remains unterminated, hence why we keep seeing the primary
channel(s) being interrupted even once the application has exited:
root@freebsd:~ # sndctl interrupts
dsp0.play.0.interrupts=1139
dsp0.record.0.interrupts=0
root@freebsd:~ # sndctl interrupts
dsp0.play.0.interrupts=1277
dsp0.record.0.interrupts=0
root@freebsd:~ # sndctl interrupts
dsp0.play.0.interrupts=1394
dsp0.record.0.interrupts=0
The only applications that do not have this issue are those (e.g., mpv) that
manually call ioctls which end up calling chn_abort(), like SNDCTL_DSP_HALT, to
abort the channel(s) during shutdown. For all other applications that do not
manually abort the channel(s), we can confirm that chn_abort()/chn_flush(), or
even chn_trigger(PCMTRIG_ABORT) on the parent, doesn't happen during shutdown.
root@freebsd:~ # dtrace -n 'fbt::chn_abort:entry,fbt::chn_flush:entry { printf("%s", args[0]->name); stack(); }'
dtrace: description 'fbt::chn_abort:entry,fbt::chn_flush:entry ' matched 2 probes
dtrace: buffer size lowered to 1m
^C
[...]
root@freebsd:~ # dtrace -n 'fbt::chn_trigger:entry /args[1] == -1/ { printf("%s", args[0]->name); stack(); }'
dtrace: description 'fbt::chn_trigger:entry ' matched 1 probe
dtrace: buffer size lowered to 1m
CPU ID FUNCTION:NAME
0 68037 chn_trigger:entry dsp0.virtual_play.0
sound.ko`chn_kill+0x134
sound.ko`vchan_destroy+0x94
sound.ko`dsp_close+0x39b
kernel`devfs_destroy_cdevpriv+0xab
kernel`devfs_close_f+0x63
kernel`_fdrop+0x1a
kernel`closef+0x1e3
kernel`closefp_impl+0x76
kernel`amd64_syscall+0x151
kernel`0xffffffff8103841b1
To fix this, modify dsp_close() to execute the primary channel case on both
primary and virtual channels. While what we really care about are the
chn_abort()/chn_flush() calls, it shouldn't hurt to call the rest of the
functions on the vchans as well, to avoid complicating things; they get deleted
right below, anyway.
With the patch applied:
root@freebsd:~ # dtrace -n 'fbt::chn_trigger:entry /args[1] == -1/ { printf("%s", args[0]->name); stack(); }'
dtrace: description 'fbt::chn_trigger:entry ' matched 1 probe
dtrace: buffer size lowered to 1m
CPU ID FUNCTION:NAME
1 68037 chn_trigger:entry dsp0.virtual_play.0
sound.ko`chn_flush+0x2a
sound.ko`dsp_close+0x330
kernel`devfs_destroy_cdevpriv+0xab
kernel`devfs_close_f+0x63
kernel`_fdrop+0x1a
kernel`closef+0x1e3
kernel`closefp_impl+0x76
kernel`amd64_syscall+0x151
kernel`0xffffffff8103841b
0 68037 chn_trigger:entry dsp0.play.0
sound.ko`chn_notify+0x4ce
sound.ko`vchan_trigger+0x105
sound.ko`chn_trigger+0xb4
sound.ko`chn_flush+0x2a
sound.ko`dsp_close+0x330
kernel`devfs_destroy_cdevpriv+0xab
kernel`devfs_close_f+0x63
kernel`_fdrop+0x1a
kernel`closef+0x1e3
kernel`closefp_impl+0x76
kernel`amd64_syscall+0x151
kernel`0xffffffff8103841b
Above we can see a chn_trigger(PCMTRIG_ABORT) on the parent (dsp0.play.0),
which is coming from the chn_abort() (inlined) in chn_notify():
root@freebsd:~ # dtrace -n 'kinst::chn_abort:entry { stack(); }'
dtrace: description 'kinst::chn_abort:entry ' matched 5 probes
dtrace: buffer size lowered to 1m
CPU ID FUNCTION:NAME
1 72580 chn_notify:1192
sound.ko`0xffffffff8296cab4
sound.ko`vchan_trigger+0x105
sound.ko`chn_trigger+0xb4
sound.ko`chn_flush+0x2a
sound.ko`dsp_close+0x330
kernel`devfs_destroy_cdevpriv+0xab
kernel`devfs_close_f+0x63
kernel`_fdrop+0x1a
kernel`closef+0x1e3
kernel`closefp_impl+0x76
kernel`amd64_syscall+0x151
kernel`0xffffffff8103841b
We can also confirm the primary channel(s) are not interrupted anymore:
root@freebsd:/mnt/src # sndctl interrupts
dsp0.play.0.interrupts=0
dsp0.record.0.interrupts=0
In collaboration with: adrian
Tested by: adrian, christos, thj
Sponsored by: The FreeBSD Foundation
MFC after: 2 days
Reviewed by: thj, adrian, emaste
Differential Revision: https://reviews.freebsd.org/D50488
(cherry picked from commit f6430bc61df78be070209d52b4452ae9cf4cd015)
(cherry picked from commit 0c6aa445ec0c85e7c9653d20562907742569de6f)
Approved by: re (cperciva)
---
sys/dev/sound/pcm/dsp.c | 33 +++++++++++++++++++--------------
1 file changed, 19 insertions(+), 14 deletions(-)
diff --git a/sys/dev/sound/pcm/dsp.c b/sys/dev/sound/pcm/dsp.c
index c5caeea8a002..8ee3d1d3f2a8 100644
--- a/sys/dev/sound/pcm/dsp.c
+++ b/sys/dev/sound/pcm/dsp.c
@@ -290,19 +290,25 @@ dsp_close(void *data)
if (sg_ids != 0)
free_unr(pcmsg_unrhdr, sg_ids);
+ /*
+ * Go through the channel abort/flush path for both
+ * primary and virtual channels to ensure that, in the
+ * case of vchans, the stream is always properly
+ * stopped, and the primary channels do not keep being
+ * interrupted even if all vchans are gone.
+ */
+ CHN_LOCK(rdch);
+ chn_abort(rdch); /* won't sleep */
+ rdch->flags &= ~(CHN_F_RUNNING | CHN_F_MMAP |
+ CHN_F_DEAD | CHN_F_EXCLUSIVE);
+ chn_reset(rdch, 0, 0);
+ chn_release(rdch);
if (rdch->flags & CHN_F_VIRTUAL) {
parent = rdch->parentchannel;
CHN_LOCK(parent);
CHN_LOCK(rdch);
vchan_destroy(rdch);
CHN_UNLOCK(parent);
- } else {
- CHN_LOCK(rdch);
- chn_abort(rdch); /* won't sleep */
- rdch->flags &= ~(CHN_F_RUNNING | CHN_F_MMAP |
- CHN_F_DEAD | CHN_F_EXCLUSIVE);
- chn_reset(rdch, 0, 0);
- chn_release(rdch);
}
}
if (wrch != NULL) {
@@ -315,19 +321,18 @@ dsp_close(void *data)
if (sg_ids != 0)
free_unr(pcmsg_unrhdr, sg_ids);
+ CHN_LOCK(wrch);
+ chn_flush(wrch); /* may sleep */
+ wrch->flags &= ~(CHN_F_RUNNING | CHN_F_MMAP |
+ CHN_F_DEAD | CHN_F_EXCLUSIVE);
+ chn_reset(wrch, 0, 0);
+ chn_release(wrch);
if (wrch->flags & CHN_F_VIRTUAL) {
parent = wrch->parentchannel;
CHN_LOCK(parent);
CHN_LOCK(wrch);
vchan_destroy(wrch);
CHN_UNLOCK(parent);
- } else {
- CHN_LOCK(wrch);
- chn_flush(wrch); /* may sleep */
- wrch->flags &= ~(CHN_F_RUNNING | CHN_F_MMAP |
- CHN_F_DEAD | CHN_F_EXCLUSIVE);
- chn_reset(wrch, 0, 0);
- chn_release(wrch);
}
}
PCM_LOCK(d);