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 <[email protected]> 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/[email protected]?part=10
