Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] ALSA: virtio-snd: un-armed paused stop on read XRUN in virtsnd_pcm_cp_ack() --
commit ceb71ae8baa14854bd1fb36ae47981ac52e944e5 Author: Joonho Lee <[email protected]> ALSA: virtio-snd: fix dsnoop capture with staged buffers This commit adds free-running capture mode support for staged capture buffers when stop_threshold equals the boundary, allowing dsnoop clients to consume the mmap ring without triggering capacity rejections. It also handles transitions back to bounded capture and arms synchronous stop on paused XRUN paths. > diff --git a/sound/virtio/virtio_pcm_msg.c b/sound/virtio/virtio_pcm_msg.c > index 663f7695b5be8..e7258bce2e6f0 100644 > --- a/sound/virtio/virtio_pcm_msg.c > +++ b/sound/virtio/virtio_pcm_msg.c [ ... ] > @@ -508,24 +511,54 @@ int virtsnd_pcm_msg_send(struct virtio_pcm_substream > *vss, unsigned long offset, > return 0; > } > [ ... ] > +/* Caller holds vss->lock. */ > +int virtsnd_pcm_capture_refresh_mode_locked(struct virtio_pcm_substream *vss, > + bool *free_running) > +{ > + struct snd_pcm_runtime *runtime; > + snd_pcm_uframes_t stop_threshold; > + u64 outstanding; > + bool new_free_running; > + > + if (!vss->capture_staging || !vss->runtime_active || !vss->substream) > + return -EBADFD; > + > + runtime = vss->substream->runtime; > + if (!runtime || !vss->buffer_bytes || !vss->capture_boundary) > + return -EBADFD; > + > + /* Validate the currently committed policy before observing a > transition. */ > + if (!virtsnd_pcm_capture_counters_valid(vss)) > + return -EIO; > + > + stop_threshold = READ_ONCE(runtime->stop_threshold); > + new_free_running = stop_threshold == vss->capture_boundary; > + if (new_free_running != vss->capture_free_running) { > + outstanding = vss->capture_copied_total - > + vss->capture_acked_total; > + if (new_free_running) { > + vss->capture_ready = 0; > + } else { > + if (outstanding >= vss->buffer_bytes) > + return -EPIPE; > + vss->capture_ready = outstanding; > + } > + vss->capture_free_running = new_free_running; > + } > + > + *free_running = new_free_running; > + return 0; > +} [ ... ] > diff --git a/sound/virtio/virtio_pcm_ops.c b/sound/virtio/virtio_pcm_ops.c > index d50924b792f65..cbf4e479d295f 100644 > --- a/sound/virtio/virtio_pcm_ops.c > +++ b/sound/virtio/virtio_pcm_ops.c [ ... ] > @@ -1314,8 +1348,14 @@ static int virtsnd_pcm_cp_ack(struct snd_pcm_substream > *substream) > struct virtsnd_pcm_transfer transfer = {}; > int rc; > > - if (vss->capture_staging) > - return virtsnd_pcm_capture_staged_ack(vss, runtime); > + if (vss->capture_staging) { > + rc = virtsnd_pcm_capture_staged_ack(vss, runtime); > + if (rc == -EPIPE) { > + guard(spinlock_irqsave)(&vss->lock); > + virtsnd_pcm_capture_arm_paused_stop(vss); > + } > + return rc; > + } [Severity: Medium] Can a paused staged-capture stream still enter SNDRV_PCM_STATE_XRUN during a partial read without arming vss->stopped and runtime->stop_operating? If a staged-capture stream is paused in SNDRV_PCM_STATE_PAUSED with unread frames in the ring (where outstanding < vss->buffer_bytes), and userspace lowers runtime->stop_threshold via snd_pcm_sw_params() from boundary to a bounded threshold smaller than the frames that will remain after a partial read, snd_pcm_sw_params() does not call snd_pcm_update_state() because snd_pcm_running(substream) is false while paused. When userspace then performs a partial read through __snd_pcm_lib_xfer() that leaves remaining avail >= runtime->stop_threshold: __snd_pcm_lib_xfer() pcm_lib_apply_appl_ptr() virtsnd_pcm_cp_ack() virtsnd_pcm_capture_staged_ack() virtsnd_pcm_capture_refresh_mode_locked() virtsnd_pcm_capture_refresh_mode_locked() in sound/virtio/virtio_pcm_msg.c only checks outstanding >= vss->buffer_bytes when transitioning from free-running to bounded mode, so virtsnd_pcm_capture_staged_ack() succeeds with rc == 0. Because virtsnd_pcm_cp_ack() in sound/virtio/virtio_pcm_ops.c only calls virtsnd_pcm_capture_arm_paused_stop(vss) when rc == -EPIPE, it returns 0 without setting vss->stopped or runtime->stop_operating. Immediately afterward at the end of __snd_pcm_lib_xfer() in sound/core/pcm_lib.c: _end_unlock: runtime->twake = 0; if (xfer > 0 && err >= 0) snd_pcm_update_state(substream, runtime); snd_pcm_update_state() sees avail >= runtime->stop_threshold and calls __snd_pcm_xrun() -> snd_pcm_stop(substream, SNDRV_PCM_STATE_XRUN). Because snd_pcm_running() is false in SNDRV_PCM_STATE_PAUSED, snd_pcm_do_stop() skips trigger(STOP) and does not set runtime->stop_operating = true. On the subsequent recovery call to snd_pcm_prepare(), snd_pcm_sync_stop() is skipped and virtsnd_pcm_prepare() in sound/virtio/virtio_pcm_ops.c fails with -EBADFD on vss->stopped (in pause-restart mode) or virtsnd_pcm_msg_pending_num(vss) (in native pause mode): scoped_guard(spinlock_irqsave, &vss->lock) stopped = vss->stopped; if (stopped) { dev_err(&vdev->dev, "SID %u: stream release failed\n", vss->sid); substream->runtime->stop_operating = true; return -EBADFD; } if (!vss->suspended) { if (virtsnd_pcm_msg_pending_num(vss)) { dev_err(&vdev->dev, "SID %u: invalid I/O queue state\n", vss->sid); return -EBADFD; } Should virtsnd_pcm_cp_ack() also check whether a paused stream has remaining capture avail >= runtime->stop_threshold (or check stop_threshold in virtsnd_pcm_capture_staged_ack()) so virtsnd_pcm_capture_arm_paused_stop() is armed before snd_pcm_update_state() moves the paused stream to SNDRV_PCM_STATE_XRUN? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=11
