From nobody Thu May 29 10:25:26 2025 X-Original-To: dev-commits-src-all@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4b7MvW2g9jz5wprc; Thu, 29 May 2025 10:25:27 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from mxrelay.nyi.freebsd.org (mxrelay.nyi.freebsd.org [IPv6:2610:1c1:1:606c::19:3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "mxrelay.nyi.freebsd.org", Issuer "R11" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4b7MvV4tMWz3xM8; Thu, 29 May 2025 10:25:26 +0000 (UTC) (envelope-from git@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1748514326; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=Xuhk/573uoeeEYiY4w92T8X8kGk2WzK6eyv/RLKfKRI=; b=S7Omc2kcpojVYI+WSM6faEDNQLq+XANy+Lw8cWZQSzWDqyktaCvZ7v95hujw54LpRacYfO oMjU+J/RvwXx+HJxxLtoM2UjFmlvWXaZ3ZMQExngob6JrwKHanS0R1BfVv5XTxVEamUNTy j2qK3R1WbQaDLAUqOjS0AQxhgU/A//hl45T666r2mIxhiCxPOhv2WFh1XvEH0eaitAAlDp Kv9W8ea6SzKO3TSKC0ruWa/lX/lz+pg0tm0lXVdVyMMt/xiVOkHynfM3a31vFHotxclonN 9dEYTBUZGSYjB1hopuTH1KPwAQ8Mgcr/AxYJyjy0YuOlO1EpdEj7EEtWNyFHug== ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1748514326; a=rsa-sha256; cv=none; b=ODMiUUAAGMN9dTe4d2r1D8uHyFqFOOKrsqEXzVyNVvgAXkOsVfWE5oWq8OBELChXuTwIWV ZvyAVCmhoo2Bh+NWY5ej2jqpNGzUmgQBBOdkGIF8yTAF6ndUp2D+tB81Wf5K8z0zCO0QCD uoPuiyTCrzJuufEsrAo4BX2nFPHcElsEGlxB7jNYQcEDrrQARHMHgH/IzBcOW5QU7ZkhIt 2yAMgMizjt/eIKGz1fcGWheXf/YH5uZDQbCeA4wSP9cgpvwxUJkYIjMDU/Yx3uYkGLxSeK OS+aas1hAremEP60vifvxIUtViNKh1B3PxcySoF3V8UV4Hv028RganTvz2rOxw== ARC-Authentication-Results: i=1; mx1.freebsd.org; none ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1748514326; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=Xuhk/573uoeeEYiY4w92T8X8kGk2WzK6eyv/RLKfKRI=; b=N98ALp6t7l5LR45WpLs73HP3H1sudG2psMTIEfLqKHFQ7NLM/P7C6PTNK7EQ60wtTnBgs6 Zn7FEdTR9O95MbFRZzC7TKk3AgMuRuB7OIVa9E+Ptw4GbPsf9wqhNf6nHOh30z2qRMCJwD E5SEV9Uln8od4Wy1fbD4leOZ9+g15nLcOeUzIm8c7ABW/CKwsH3BEWQ9ARTK3zU9b01i71 TSm5Hc3kbMYV6PBGHD3IrabZYuScepaq8GVDx8fqg4Y+1+fXfAwIL8+oM4xPCkoncMd3xc LwPI154btSsYoyrbYQU7QM7Q/DONL/BfhrF1OvPRn6OwvrB676LynuH2n9dOTQ== Received: from gitrepo.freebsd.org (gitrepo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:5]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (Client did not present a certificate) by mxrelay.nyi.freebsd.org (Postfix) with ESMTPS id 4b7MvV4B5nz1HPl; Thu, 29 May 2025 10:25:26 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from gitrepo.freebsd.org ([127.0.1.44]) by gitrepo.freebsd.org (8.18.1/8.18.1) with ESMTP id 54TAPQGb061065; Thu, 29 May 2025 10:25:26 GMT (envelope-from git@gitrepo.freebsd.org) Received: (from git@localhost) by gitrepo.freebsd.org (8.18.1/8.18.1/Submit) id 54TAPQ0Q061062; Thu, 29 May 2025 10:25:26 GMT (envelope-from git) Date: Thu, 29 May 2025 10:25:26 GMT Message-Id: <202505291025.54TAPQ0Q061062@gitrepo.freebsd.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org From: Christos Margiolis Subject: git: f6430bc61df7 - main - sound: Terminate stream properly when closing vchans List-Id: Commit messages for all branches of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-all List-Help: List-Post: List-Subscribe: List-Unsubscribe: X-BeenThere: dev-commits-src-all@freebsd.org Sender: owner-dev-commits-src-all@FreeBSD.org MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-Git-Committer: christos X-Git-Repository: src X-Git-Refname: refs/heads/main X-Git-Reftype: branch X-Git-Commit: f6430bc61df78be070209d52b4452ae9cf4cd015 Auto-Submitted: auto-generated The branch main has been updated by christos: URL: https://cgit.FreeBSD.org/src/commit/?id=f6430bc61df78be070209d52b4452ae9cf4cd015 commit f6430bc61df78be070209d52b4452ae9cf4cd015 Author: Christos Margiolis AuthorDate: 2025-05-29 10:24:17 +0000 Commit: Christos Margiolis CommitDate: 2025-05-29 10:25:18 +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 --- 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 f1adcdbc5d70..aa6ad4a59778 100644 --- a/sys/dev/sound/pcm/dsp.c +++ b/sys/dev/sound/pcm/dsp.c @@ -289,19 +289,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) { @@ -314,19 +320,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);