Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] ALSA: virtio: staged capture ACK and pointer bypass prepared check in DRAINING --
commit 50d3f0b4df898e1d66fc877101e4baac71884d78 Author: Joonho Lee <[email protected]> ALSA: virtio: add staged capture and serialize PCM lifecycle accounting This commit adds an opt-in staged capture mode using request-owned RX buffers with monotonic byte accounting for shared capture clients. It also serializes PCM lifecycle transitions with lifecycle_lock and coordinates pointer, reset, and ACK admission under the PCM stream lock. > diff --git a/sound/virtio/virtio_pcm_ops.c b/sound/virtio/virtio_pcm_ops.c > index 706d59d8c7201..a60f960287e2e 100644 > --- a/sound/virtio/virtio_pcm_ops.c > +++ b/sound/virtio/virtio_pcm_ops.c [ ... ] > @@ -601,14 +1513,69 @@ static snd_pcm_uframes_t > virtsnd_pcm_cp_pointer(struct snd_pcm_substream *substream) > { > struct virtio_pcm_substream *vss = snd_pcm_substream_chip(substream); > + struct snd_pcm_runtime *runtime = substream->runtime; > + size_t hw_ptr = 0; > + u64 total; > + bool free_running; > + bool xrun; > > - scoped_guard(spinlock_irqsave, &vss->lock) > - if (vss->xfer_xrun) > + if (vss->capture_staging) { > + scoped_guard(spinlock_irqsave, &vss->lock) { > + if (!vss->runtime_active || vss->preparing || > vss->suspended || > + (!vss->prepared && runtime->state != > SNDRV_PCM_STATE_DRAINING && > + !(runtime->state == SNDRV_PCM_STATE_PAUSED && > + virtsnd_pcm_pause_restart_enabled(vss) && > + vss->pause_restart_state == > VIRTSND_PCM_PAUSE_RESTART_RELEASED))) > + return SNDRV_PCM_POS_XRUN; [ ... ] > @@ -647,86 +1658,176 @@ static int virtsnd_pcm_ack_state(struct > snd_pcm_substream *substream) [ ... ] > +static int virtsnd_pcm_capture_staged_ack(struct virtio_pcm_substream *vss, > + struct snd_pcm_runtime *runtime) > +{ > + struct virtio_snd_queue *queue = virtsnd_pcm_queue(vss); > + int rc = 0; > + > + /* > + * The RX callback holds the queue lock while draining completions. > + * Account ALSA's read under the substream lock alone so completions > later > + * in that drain observe the newly released ring space. > + */ > + scoped_guard(spinlock_irqsave, &vss->lock) { > + snd_pcm_uframes_t appl_ptr; > + snd_pcm_uframes_t appl_diff; > + snd_pcm_uframes_t ack_frames; > + snd_pcm_sframes_t diff; > + size_t bytes; > + u64 outstanding; > + u64 next_acked_total; > + bool free_running; > + > + if (!vss->runtime_active || vss->preparing || !runtime || > !runtime->control || > + !runtime->dma_area || vss->suspended) > return -EBADFD; > - if (vss->stopped) > + /* Preserve DRAINING tails only when no transfer XRUN is > latched. */ > + if (vss->stopped && vss->xfer_xrun) > return -EBADFD; > - if (vss->xfer_xrun) { > - rc = -EPIPE; > - } else { > - transfer.indirect = vss->pcm_indirect; > - rc = snd_pcm_indirect_playback_transfer(substream, > - > &transfer.indirect, > - > virtsnd_pcm_trans_copy); > - if (!rc) > - rc = transfer.error; > + if (vss->xfer_xrun) > + return -EPIPE; > + if (!vss->prepared && runtime->state != > SNDRV_PCM_STATE_DRAINING && > + !(runtime->state == SNDRV_PCM_STATE_PAUSED && > + virtsnd_pcm_pause_restart_enabled(vss) && > + vss->pause_restart_state == > VIRTSND_PCM_PAUSE_RESTART_RELEASED)) > + return -EBADFD; > + if (vss->stopped && runtime->state != SNDRV_PCM_STATE_DRAINING > && > + !(runtime->state == SNDRV_PCM_STATE_PAUSED && > + virtsnd_pcm_pause_restart_enabled(vss) && > + vss->pause_restart_state == > VIRTSND_PCM_PAUSE_RESTART_RELEASED)) > + return -EBADFD; [Severity: Medium] Should virtsnd_pcm_cp_pointer() and virtsnd_pcm_capture_staged_ack() exempt SNDRV_PCM_STATE_DRAINING from the !vss->prepared check? When a capture stream enters SNDRV_PCM_STATE_DRAINING via snd_pcm_drain() -> snd_pcm_do_drain_init() -> snd_pcm_do_stop() -> virtsnd_pcm_trigger(SNDRV_PCM_TRIGGER_STOP), the driver sets vss->stopped = true and vss->xfer_enabled = false while keeping vss->prepared = true. Unlike SNDRV_PCM_STATE_PAUSED under capture_pause_restart (which sends VIRTIO_SND_R_PCM_RELEASE and sets vss->prepared = false), an active draining capture epoch always keeps vss->prepared = true until teardown retires it. If userspace subsequently calls snd_pcm_prepare() on a draining staged capture stream, snd_pcm_do_prepare() leaves runtime->state as SNDRV_PCM_STATE_DRAINING while invoking snd_pcm_sync_stop() -> virtsnd_pcm_sync_stop() -> virtsnd_pcm_sync_stop_locked(): sound/virtio/virtio_pcm_ops.c:virtsnd_pcm_sync_stop_locked() { scoped_guard(pcm_stream_lock_irqsave, substream) { guard(spinlock_irqsave)(&vss->lock); preparing = vss->preparing; vss->preparing = true; vss->prepared = false; vss->xfer_enabled = false; } rc = virtsnd_pcm_release_locked(substream); scoped_guard(pcm_stream_lock_irqsave, substream) { guard(spinlock_irqsave)(&vss->lock); vss->preparing = preparing; } return rc; } Once virtsnd_pcm_sync_stop_locked() returns, vss->prepared and vss->stopped are both false and vss->preparing is restored to false, while runtime->state is still SNDRV_PCM_STATE_DRAINING. In the window before virtsnd_pcm_prepare_locked() takes the PCM stream lock (or permanently if virtsnd_pcm_prepare_locked() fails before virtsnd_pcm_dev_prepare(), such as on -ECANCELED or a virtsnd_pcm_dev_set_params() error, where virtsnd_pcm_prepare() resets vss->preparing = false), concurrent SNDRV_PCM_IOCTL_SYNC_PTR or SNDRV_PCM_IOCTL_HWSYNC calls bypass the !vss->prepared check and proceed on the released, unprepared epoch. By comparison, the non-staged path in virtsnd_pcm_cp_ack_locked() only exempts SNDRV_PCM_STATE_DRAINING from vss->stopped and unconditionally rejects !vss->prepared: if (!vss->prepared || (vss->stopped && runtime->state != SNDRV_PCM_STATE_DRAINING)) return -EBADFD; Should virtsnd_pcm_cp_pointer() and virtsnd_pcm_capture_staged_ack() likewise require vss->prepared to be true unless the stream is in the pause-restart released state? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=10
