Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] ALSA: virtio: stale pcm_indirect state after RESET in PREPARED state --
commit c77024b62d8010ad66e1fdcc36816a226c5bcfcc Author: Joonho Lee <[email protected]> ALSA: virtio: rebase ordinary capture accounting on RESET This commit introduces virtsnd_pcm_indirect_reset() and virtsnd_pcm_capture_indirect_ops to rebase ordinary capture indirect positions and return discarded RX space when SNDRV_PCM_IOCTL_RESET is called. It also suppresses nested ACK refills during the RESET pointer observation and updates the period notification floor. Link: https://sashiko.dev/#/patchset/20261030052629.27742-1-mynameisjune111%40gmail.com?part=14 > diff --git a/sound/virtio/virtio_pcm_ops.c b/sound/virtio/virtio_pcm_ops.c > index f350d0c753763..9af0813acd77a 100644 > --- a/sound/virtio/virtio_pcm_ops.c > +++ b/sound/virtio/virtio_pcm_ops.c [ ... ] > @@ -1761,6 +1769,89 @@ static int virtsnd_pcm_sync_stop(struct > snd_pcm_substream *substream) > return virtsnd_pcm_sync_stop_locked(substream); > } > > +/* > + * The PCM lock covers indirect accounting and core completion. Once > lib_reset > + * changes hw_ptr, complete the core rebase even if refilling fails. The > latched > + * XRUN rejects subsequent pointer/ACK operations; the worker publishes XRUN > to > + * paused streams too. RESET success therefore does not imply stream > recovery. > + */ > +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 = {}; > + snd_pcm_uframes_t paused_ptr = 0; > + bool paused = runtime->state == SNDRV_PCM_STATE_PAUSED; > + int rc; > + > + /* Only PCM-locked callbacks access this observation flag. */ > + vss->indirect_reset_observing = true; > + rc = snd_pcm_lib_reset(substream); > + vss->indirect_reset_observing = false; > + if (rc) > + return rc; > + guard(spinlock_irqsave)(&queue->lock); > + guard(spinlock)(&vss->lock); > + if (vss->resetting) > + return 0; > + if (vss->xfer_xrun) { > + virtsnd_pcm_transfer_failed(vss); > + virtsnd_pcm_schedule_period_elapsed(vss); > + return 0; > + } > + if (!snd_pcm_running(substream) && !paused) > + return 0; [Severity: Medium] Can returning early here when the substream is in SNDRV_PCM_STATE_PREPARED leave vss->pcm_indirect.appl_ptr and vss->pcm_indirect.sw_ready out of sync with runtime->control->appl_ptr? While in SNDRV_PCM_STATE_PREPARED (where vss->prepared is true and vss->stopped is false), userspace can invoke SNDRV_PCM_IOCTL_SYNC_PTR without SNDRV_PCM_SYNC_PTR_APPL to advance appl_ptr to N: snd_pcm_sync_ptr() pcm_lib_apply_appl_ptr() virtsnd_pcm_cp_ack() virtsnd_pcm_cp_ack_locked() snd_pcm_indirect_capture_transfer() which sets vss->pcm_indirect.appl_ptr = N and vss->pcm_indirect.sw_ready = -frames_to_bytes(runtime, N). If userspace then calls SNDRV_PCM_IOCTL_RESET in SNDRV_PCM_STATE_PREPARED, snd_pcm_lib_reset() sets runtime->status->hw_ptr = 0, virtsnd_pcm_indirect_reset() returns 0 early without updating vss->pcm_indirect, and snd_pcm_reset_substream() resets runtime->control->appl_ptr = runtime->status->hw_ptr (0). When the stream is subsequently started and virtsnd_pcm_cp_pointer() or virtsnd_pcm_cp_ack() runs before appl_ptr reaches N, snd_pcm_indirect_capture_transfer() sees diff = appl_ptr - rec->appl_ptr < 0: if (diff) { if (diff < -(snd_pcm_sframes_t) (runtime->boundary / 2)) diff += runtime->boundary; if (diff < 0) return -EPIPE; causing a false -EPIPE XRUN and a dev_err() message in virtsnd_pcm_cp_ack_locked(). Should virtsnd_pcm_indirect_reset() also reset vss->pcm_indirect.sw_ready = 0 and vss->pcm_indirect.appl_ptr = runtime->status->hw_ptr when not running or paused, similar to how virtsnd_pcm_reset() updates vss->pcm_indirect.appl_ptr = runtime->status->hw_ptr? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=14
