Re: [PATCH v7 15/16] ALSA: pcm: synchronize stopped paused streams

[sashiko-bot@kernel.org]

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 <mynameisjune111@gmail.com>

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/20261028094132.20364-1-mynameisjune111@gmail.com?part=15