Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] ALSA: virtio: uncompleted hw_ptr rebase on virtsnd_pcm_indirect_reset() error --
commit 0cc03a4b001753550dc69b32471d30cec1719208 Author: Joonho Lee <[email protected]> ALSA: virtio: synchronize indirect capture positions on reset This commit adds a dedicated reset operation for indirect capture streams to synchronize private indirect capture accounting with the core PCM pointer rebase on reset. It discards unread data, returns the freed RX buffer space to the virtqueue, and updates the indirect application pointer to match the rebased hardware pointer. > diff --git a/sound/virtio/virtio_pcm_ops.c b/sound/virtio/virtio_pcm_ops.c > index ba14f395eef8a..e4a7a4da4d3a8 100644 > --- a/sound/virtio/virtio_pcm_ops.c > +++ b/sound/virtio/virtio_pcm_ops.c [ ... ] > @@ -1598,6 +1602,54 @@ static int virtsnd_pcm_sync_stop(struct > snd_pcm_substream *substream) > return virtsnd_pcm_sync_stop_locked(substream); > } > > +/* The PCM lock covers both indirect accounting and core completion. */ > +static int virtsnd_pcm_indirect_reset(struct snd_pcm_substream *substream) > +{ > + struct virtio_pcm_substream *vss = snd_pcm_substream_chip(substream); > + struct snd_pcm_runtime *runtime = substream->runtime; > + struct virtio_snd_queue *queue = virtsnd_pcm_queue(vss); > + struct virtsnd_pcm_transfer transfer = {}; > + int rc; > + > + rc = snd_pcm_lib_reset(substream); > + if (rc) > + return rc; > + if (!snd_pcm_running(substream) && > + runtime->state != SNDRV_PCM_STATE_PAUSED) > + return 0; > + > + guard(spinlock_irqsave)(&queue->lock); > + guard(spinlock)(&vss->lock); > + if (vss->resetting) > + return -ECANCELED; > + if (!vss->prepared || vss->stopped) > + return -EBADFD; > + > + /* Pausing does not move the device's fixed DMA buffer positions. */ > + if (runtime->state == SNDRV_PCM_STATE_PAUSED) > + runtime->status->hw_ptr = > + bytes_to_frames(runtime, vss->pcm_indirect.sw_io); > + > + transfer.indirect = vss->pcm_indirect; > + transfer.indirect.sw_ready = 0; > + /* Consume no application delta while returning discarded RX space. */ > + transfer.indirect.appl_ptr = runtime->control->appl_ptr; > + rc = snd_pcm_indirect_capture_transfer(substream, &transfer.indirect, > + virtsnd_pcm_trans_copy); > + if (!rc) > + rc = transfer.error; > + if (rc) { > + virtsnd_pcm_transfer_failed(vss); > + return rc; > + } [Severity: High] What happens to the PCM ring buffer accounting if virtsnd_pcm_indirect_reset() returns an error after snd_pcm_lib_reset() has already rebased runtime->status->hw_ptr? On an indirect capture substream in SNDRV_PCM_STATE_RUNNING or SNDRV_PCM_STATE_PAUSED that has wrapped past runtime->buffer_size (so runtime->hw_ptr_base >= runtime->buffer_size and runtime->control->appl_ptr >= runtime->buffer_size), virtsnd_pcm_indirect_reset() first calls snd_pcm_lib_reset() (and in the paused state sets runtime->status->hw_ptr from vss->pcm_indirect.sw_io), which rebases runtime->status->hw_ptr into [0, runtime->buffer_size). If virtsnd_pcm_indirect_reset() then returns -ECANCELED, -EBADFD, or transfer.error, snd_pcm_do_reset() in sound/core/pcm_native.c aborts early without completing the rest of the rebase: sound/core/pcm_native.c:snd_pcm_do_reset() { ... err = substream->ops->reset(substream); if (err < 0) return err; runtime->hw_ptr_base = 0; runtime->hw_ptr_interrupt = runtime->status->hw_ptr - runtime->status->hw_ptr % runtime->period_size; runtime->silence_start = runtime->status->hw_ptr; runtime->silence_filled = 0; runtime->control->appl_ptr = runtime->status->hw_ptr; ... } This leaves runtime->status->hw_ptr rebased below runtime->control->appl_ptr while runtime->hw_ptr_base and runtime->control->appl_ptr remain at their pre-reset values, and runtime->state stays in SNDRV_PCM_STATE_RUNNING or SNDRV_PCM_STATE_PAUSED. Although virtsnd_pcm_transfer_failed() sets runtime->stop_operating = true on transfer errors, stop_operating is only acted on by snd_pcm_sync_stop() and is not checked by pcm_accessible_state() or __snd_pcm_lib_xfer(). In include/sound/pcm.h:snd_pcm_capture_avail(): snd_pcm_sframes_t avail = runtime->status->hw_ptr - runtime->control->appl_ptr; if (avail < 0) avail += runtime->boundary; return avail; avail underflows and wraps by runtime->boundary. Wouldn't a subsequent read via sound/core/pcm_lib.c:__snd_pcm_lib_xfer() see this wrapped avail value and copy stale DMA ring buffer frames to userspace via writer() before pcm_accessible_state() or pcm_lib_apply_appl_ptr() stops the transfer? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=14
