Re: [PATCH v20 10/12] ALSA: virtio: serialize staged capture lifecycle and defer paused XRUNs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] ALSA: virtio: start race in virtsnd_pcm_start_queue() while
  preparing is set
- [Medium] ALSA: virtio: virtsnd_pcm_sync_stop() drops stop_operating
  when paused
--

commit 55539e87ca936ab2bb90a3215a575e0a0a27b88c
Author: Joonho Lee <mynameisjune111@gmail.com>

ALSA: virtio: serialize staged capture lifecycle and defer paused XRUNs

This commit adds staged capture support using request-owned RX buffers,
serializes PCM lifecycle operations with lifecycle_lock, and defers
paused XRUN notifications until stream resume.

> diff --git a/sound/virtio/virtio_pcm_ops.c b/sound/virtio/virtio_pcm_ops.c
> index 8fe6974130270..c7125074f6425 100644
> --- a/sound/virtio/virtio_pcm_ops.c
> +++ b/sound/virtio/virtio_pcm_ops.c

[ ... ]

> @@ -323,182 +645,779 @@ static int virtsnd_pcm_hw_params(struct snd_pcm_substream *substream,

[ ... ]

> +static int virtsnd_pcm_prepare_locked(struct snd_pcm_substream *substream)
>  {

[ ... ]

> +	scoped_guard(pcm_stream_lock_irqsave, substream) {
> +		guard(spinlock_irqsave)(&vss->lock);
> +		if (epoch != vss->reset_epoch || vss->resetting)
> +			return -ECANCELED;
>  		vss->release_required = true;
> +		vss->buffer_bytes = snd_pcm_lib_buffer_bytes(substream);
> +		vss->hw_ptr = 0;
> +		vss->suspended = false;
> +		vss->msg_count = 0;
> +		virtsnd_pcm_msg_reset(vss);
> +		memset(&vss->pcm_indirect, 0, sizeof(vss->pcm_indirect));
> +		vss->pcm_indirect.sw_buffer_size = vss->buffer_bytes;
> +		vss->pcm_indirect.hw_buffer_size = vss->buffer_bytes;
> +		if (vss->capture_staging) {
> +			vss->capture_boundary = substream->runtime->boundary;
> +			rc = virtsnd_pcm_capture_refresh_mode_locked(vss,
> +							     &free_running);
> +			if (rc) {
> +				vss->xfer_xrun = true;
> +				vss->stopped = true;
> +				return rc;
> +			}
> +		}
> +		vss->prepared = true;
> +		vss->stopped = false;
> +		vss->xfer_xrun = false;
>  	}
> +
> +	return rc;
>  }

[ ... ]

> +static int virtsnd_pcm_start_queue(struct virtio_pcm_substream *vss, u64 epoch)
> +{
> +	struct virtio_snd_queue *queue = virtsnd_pcm_queue(vss);
> +	int rc = 0;
> +
> +	guard(spinlock_irqsave)(&queue->lock);
> +	guard(spinlock)(&vss->lock);
> +	if (vss->resetting || vss->reset_epoch != epoch)
> +		return -ECANCELED;
> +	if (vss->stopped || !vss->prepared || vss->xfer_xrun)
> +		return vss->xfer_xrun ? -EPIPE : -EBADFD;

[Severity: Medium]
Can SNDRV_PCM_TRIGGER_START succeed while virtsnd_pcm_prepare() is still
in progress?

Previously, virtsnd_pcm_prepare_device() kept vss->prepared false until
virtsnd_pcm_reset() ran, and virtsnd_pcm_trigger() rejected start when
vss->preparing was set.  Now virtsnd_pcm_prepare_locked() sets
vss->prepared = true before snd_pcm_do_prepare() calls
snd_pcm_reset_substream(), and neither virtsnd_pcm_start_queue() nor
virtsnd_pcm_pause_restart_start() checks vss->preparing.

Unlike snd_pcm_pre_start(), snd_pcm_drain() does not take
runtime->buffer_mutex or check runtime->buffer_accessing.  When a
prepared playback substream with queued frames (appl_ptr > hw_ptr) is
re-prepared, a concurrent drain ioctl can start the device before the
pointer reset finishes:

CPU0                                  CPU1
snd_pcm_do_prepare()
  virtsnd_pcm_prepare()
    vss->preparing = true
    vss->prepared = true
                                      snd_pcm_drain()
                                        snd_pcm_do_drain_init()
                                          snd_pcm_do_start()
                                            virtsnd_pcm_trigger(START)
                                              vss->device_running = true
                                              vss->xfer_enabled = true
  snd_pcm_reset_substream()
    virtsnd_pcm_playback_reset()
      vss->preparing = false
  snd_pcm_post_prepare()
    state = SNDRV_PCM_STATE_PREPARED  <-- device left running in PREPARED

Should virtsnd_pcm_start_queue() and virtsnd_pcm_pause_restart_start()
also reject start while vss->preparing is true?

[ ... ]

> @@ -808,42 +2060,275 @@ static int virtsnd_pcm_cp_ack(struct snd_pcm_substream *substream)

[ ... ]

> +static int virtsnd_pcm_sync_stop(struct snd_pcm_substream *substream)
> +{
> +	struct virtio_pcm_substream *vss = snd_pcm_substream_chip(substream);
> +	bool owned;
> +
> +	guard(mutex)(&vss->lifecycle_lock);
> +	scoped_guard(pcm_stream_lock_irqsave, substream) {
> +		snd_pcm_state_t state = substream->runtime->state;
> +
> +		/*
> +		 * Trigger holds the PCM lock through its control requests. Wait
> +		 * for it, then leave a replacement active generation alone.
> +		 * lifecycle_lock keeps prepare from replacing an inactive one
> +		 * until sync_stop_locked has finished its teardown.
> +		 */
> +		guard(spinlock_irqsave)(&vss->lock);
> +		owned = vss->prepared || vss->release_required || vss->msg_count ||
> +			vss->device_running;
> +		if (state == SNDRV_PCM_STATE_RUNNING ||
> +		    state == SNDRV_PCM_STATE_PAUSED ||
> +		    state == SNDRV_PCM_STATE_PREPARED ||
> +		    (state == SNDRV_PCM_STATE_DRAINING &&
> +		     substream->stream == SNDRV_PCM_STREAM_PLAYBACK)) {
> +			/* Core may consume this marker before a later inactive teardown. */
> +			if (owned)
> +				substream->runtime->stop_operating = true;
> +			return 0;
> +		}

[Severity: Medium]
Does virtsnd_pcm_sync_stop() drop runtime->stop_operating when a staged
capture stream with capture_pause_restart enabled is paused?

When SNDRV_PCM_TRIGGER_PAUSE_PUSH runs with capture_pause_restart
enabled, virtsnd_pcm_pause_restart_pause() sets vss->stopped = true and
calls virtsnd_pcm_release_wait(), which waits for vss->msg_count to reach
0 and clears vss->prepared and vss->release_required while transitioning
to VIRTSND_PCM_PAUSE_RESTART_RELEASED (with vss->device_running already
false).  virtsnd_pcm_trigger() then sets runtime->stop_operating = true so
that a later teardown from SNDRV_PCM_STATE_PAUSED will invoke
ops->sync_stop.

If snd_pcm_sync_stop() runs while the stream is still in
SNDRV_PCM_STATE_PAUSED, it clears runtime->stop_operating before calling
virtsnd_pcm_sync_stop().  Because owned does not include vss->stopped, it
evaluates to false and runtime->stop_operating stays false.  If the
paused stream is then torn down without resuming to RUNNING (for example
via snd_pcm_drop()), snd_pcm_running() is false so ALSA skips
virtsnd_pcm_sync_stop(), leaving vss->stopped set and vss->elapsed_period
unquiesced.  Should owned include vss->stopped?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261107143422.29319-1-mynameisjune111@gmail.com?part=10