Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] ALSA: pcm: snd_pcm_do_stop() arms sync_stop on paused streams without STOP trigger --
commit 9cef4af5ba9c6105fe851ee2ad98ad262f35643e Author: Joonho Lee <[email protected]> ALSA: pcm: synchronize stopped paused streams This commit marks runtime->stop_operating = true in snd_pcm_do_stop() when a PCM substream is in SNDRV_PCM_STATE_PAUSED so that subsequent prepare or drop operations invoke sync_stop to drain outstanding DMA requests. > diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c > index a2ea44c3c1d29..ac2e5c2ee5065 100644 > --- a/sound/core/pcm_native.c > +++ b/sound/core/pcm_native.c > @@ -1611,6 +1611,9 @@ static int snd_pcm_do_stop(struct snd_pcm_substream > *substream, > substream->ops->trigger(substream, SNDRV_PCM_TRIGGER_STOP); > substream->runtime->stop_operating = true; > } > + /* Paused DMA can still own requests which sync_stop must drain. */ > + if (substream->runtime->state == SNDRV_PCM_STATE_PAUSED) > + substream->runtime->stop_operating = true; [Severity: High] Does setting substream->runtime->stop_operating = true here without issuing SNDRV_PCM_TRIGGER_STOP leave drivers in a paused state when transitioning to SNDRV_PCM_STATE_XRUN or SNDRV_PCM_STATE_SETUP? Normally, when stopping a paused stream via snd_pcm_prepare() or snd_pcm_drop(), ALSA unpauses the stream first before calling snd_pcm_stop(): sound/core/pcm_native.c:snd_pcm_prepare() { scoped_guard(pcm_stream_lock_irq, substream) { switch (substream->runtime->state) { case SNDRV_PCM_STATE_PAUSED: snd_pcm_pause(substream, false); fallthrough; case SNDRV_PCM_STATE_SUSPENDED: snd_pcm_stop(substream, SNDRV_PCM_STATE_SETUP); break; } } } Because snd_pcm_pause(substream, false) sets runtime->state to SNDRV_PCM_STATE_RUNNING, snd_pcm_do_stop() sees snd_pcm_running(substream) == true and invokes substream->ops->trigger(substream, SNDRV_PCM_TRIGGER_STOP). However, when snd_pcm_stop() -> snd_pcm_do_stop() is called directly on a stream in SNDRV_PCM_STATE_PAUSED (for example, when __snd_pcm_lib_xfer() -> snd_pcm_update_state() -> __snd_pcm_xrun() triggers an XRUN after a partial read/write while paused leaves avail >= runtime->stop_threshold, when pcm_lib_apply_appl_ptr() -> ops->ack() returns -EPIPE, or if snd_pcm_pause(substream, false) fails), snd_pcm_running(substream) is false. With this change, snd_pcm_do_stop() sets runtime->stop_operating = true without calling ops->trigger(SNDRV_PCM_TRIGGER_STOP), and snd_pcm_post_stop() then overwrites runtime->state to SNDRV_PCM_STATE_XRUN (or SNDRV_PCM_STATE_SETUP). A subsequent snd_pcm_prepare() or snd_pcm_drop() no longer sees SNDRV_PCM_STATE_PAUSED, skips snd_pcm_pause() and snd_pcm_stop(), and calls snd_pcm_sync_stop() -> ops->sync_stop() on a stream that only ever received SNDRV_PCM_TRIGGER_PAUSE_PUSH and never received SNDRV_PCM_TRIGGER_STOP: 1. In sound/virtio/virtio_pcm_ops.c, virtsnd_pcm_trigger() does not set vss->stopped = true on SNDRV_PCM_TRIGGER_PAUSE_PUSH: case SNDRV_PCM_TRIGGER_STOP: case SNDRV_PCM_TRIGGER_PAUSE_PUSH: scoped_guard(spinlock_irqsave, &vss->lock) { if (command != SNDRV_PCM_TRIGGER_PAUSE_PUSH) vss->stopped = true; vss->xfer_enabled = false; } At this commit, virtsnd_pcm_sync_stop() checks: scoped_guard(spinlock_irqsave, &vss->lock) { vss->xfer_enabled = false; need_release = vss->stopped || vss->msg_count; if (need_release) vss->stopped = true; } cancel_work_sync(&vss->elapsed_period); if (!need_release) return 0; If vss->msg_count is 0 when the paused stream enters XRUN, vss->stopped is false and virtsnd_pcm_sync_stop() skips sending VIRTIO_SND_R_PCM_RELEASE. Furthermore, once the follow-up commit ("ALSA: virtio: serialize capture lifecycle and reset state") removes the paused-ACK stop_threshold check from virtsnd_pcm_cp_ack(), a paused partial read/write that triggers XRUN in snd_pcm_update_state() leaves both vss->stopped and vss->xfer_xrun false while runtime->state is SNDRV_PCM_STATE_XRUN. A subsequent SNDRV_PCM_IOCTL_SYNC_PTR ioctl (via snd_pcm_sync_ptr() -> pcm_lib_apply_appl_ptr()) can then invoke virtsnd_pcm_pb_ack() or virtsnd_pcm_cp_ack(), bypass the vss->stopped and vss->xfer_xrun guards, and queue new virtqueue DMA requests or advance staged capture counters while in XRUN. 2. In sound/core/pcm_dmaengine.c, SNDRV_PCM_TRIGGER_PAUSE_PUSH calls dmaengine_pause() while dmaengine_terminate_async() is only called on SNDRV_PCM_TRIGGER_STOP. In snd_dmaengine_pcm_sync_stop(): status = dmaengine_tx_status(prtd->dma_chan, prtd->cookie, &state); if (status != DMA_PAUSED) dmaengine_synchronize(prtd->dma_chan); Because the channel remains in DMA_PAUSED and was never terminated, snd_dmaengine_pcm_sync_stop() skips synchronization and leaves the paused DMA transfer active across prepare. 3. In sound/usb/pcm.c and sound/usb/endpoint.c, SNDRV_PCM_TRIGGER_PAUSE_PUSH only clears the URB data callback whereas stop_endpoints() is only called on SNDRV_PCM_TRIGGER_STOP. When snd_usb_pcm_sync_stop() calls wait_clear_urbs(): if (atomic_read(&ep->state) != EP_STATE_STOPPING) return 0; It returns immediately without stopping or draining the USB endpoints. Should snd_pcm_do_stop() also ensure the stream is triggered to stop (or Only treat SNDRV_PCM_STATE_PAUSED as running for the stop trigger when substream->runtime->trigger_master == substream) rather than only setting stop_operating = true? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=15
