sound: Terminate stream properly when closing vchans

[christos]

Summary

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
Sponsored by:		The FreeBSD Foundation
MFC after:		2 days

[adrian]

sys/dev/sound/pcm/dsp.c

291–304

Maybe add a comment here before you call chn_abort() to explain now why we're always going through the channel abort path?

eg something like

/*

• Go through the channel abort/reset path for both virtual and non-virtual
• channels. This and the following vchan_destroy() ensures that the physical
• channel will stop DMA/interrupts if none of the virtual channels are running. */

That way someone in the future trying to fix a DIFFERENT unknown bug will not just go "oh this is the problem, I'll mke this conditional again" and re-break it. :-)

[christos]

Address Adrian's comment.