Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] ALSA: virtio: unupdated capture_notified_periods in virtsnd_pcm_reset() - [High] ALSA: virtio: false XRUN and skipped rebase in virtsnd_pcm_reset() race --
commit 4e51f7611d17678ff733d2492023b2e0abcb06d3 Author: Joonho Lee <[email protected]> ALSA: virtio: synchronize staged capture reset with PCM positions This commit introduces a custom PCM reset operation for staged capture streams to synchronize private copied and acknowledged byte counters with ALSA core pointer resets. When resetting a running stream, it calls snd_pcm_lib_reset(), advances capture_acked_total to capture_pointer_seen_total, and attempts to refill the RX queue. > diff --git a/sound/virtio/virtio_pcm_ops.c b/sound/virtio/virtio_pcm_ops.c > index 66560d82b7f8e..ba14f395eef8a 100644 > --- a/sound/virtio/virtio_pcm_ops.c > +++ b/sound/virtio/virtio_pcm_ops.c [ ... ] > @@ -1591,6 +1598,58 @@ static int virtsnd_pcm_sync_stop(struct > snd_pcm_substream *substream) > return virtsnd_pcm_sync_stop_locked(substream); > } > > +/* The core holds the stream lock through the matching appl_ptr reset. */ > +static int virtsnd_pcm_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); > + size_t position; > + size_t period; > + int rc; > + > + /* Keep ALSA's pointer validation, time accounting and XRUN processing. > */ > + rc = snd_pcm_lib_reset(substream); > + if (rc || !vss->capture_staging) > + return rc; > + > + guard(spinlock_irqsave)(&queue->lock); > + guard(spinlock)(&vss->lock); > + if (vss->resetting) > + return -ECANCELED; > + /* The common reset has already handled a pointer-reported XRUN. */ > + if (vss->xfer_xrun) > + return 0; [Severity: High] Can an RX completion arriving between snd_pcm_lib_reset() and acquiring vss->lock trigger a false XRUN and skip the private counter rebase? During snd_pcm_do_reset() -> virtsnd_pcm_reset(), snd_pcm_lib_reset() calls virtsnd_pcm_cp_pointer(), which records vss->capture_pointer_seen_total and releases vss->lock while vss->capture_ready still holds the pre-reset unacknowledged byte count: sound/virtio/virtio_pcm_ops.c:virtsnd_pcm_cp_pointer() { ... } else { vss->capture_pointer_seen_total = total; hw_ptr = virtsnd_pcm_ring_offset(total, vss->buffer_bytes); } ... } In managed mode (!vss->capture_free_running), if an in-flight RX completion arrives before virtsnd_pcm_reset() acquires queue->lock and vss->lock, virtsnd_pcm_capture_complete() checks the incoming bytes against the stale pre-reset vss->capture_ready: sound/virtio/virtio_pcm_msg.c:virtsnd_pcm_capture_complete() { ... else if (!free_running && audio_bytes >= vss->buffer_bytes - vss->capture_ready) invalid_reason = "ring_capacity"; else if (!free_running && check_add_overflow(vss->capture_ready, audio_bytes, &next_ready)) invalid_reason = "ready_overflow"; else if (!free_running && next_ready >= vss->buffer_bytes) invalid_reason = "ring_full"; ... } When the pre-reset unacknowledged bytes plus audio_bytes reach vss->buffer_bytes, virtsnd_pcm_capture_complete() rejects the completion, drops the captured audio, and sets vss->xfer_xrun = true. When virtsnd_pcm_reset() then acquires the locks, it sees vss->xfer_xrun and returns 0 without updating vss->capture_acked_total, vss->capture_ready, or vss->pcm_indirect.appl_ptr, even though snd_pcm_lib_reset() did not observe the XRUN and snd_pcm_do_reset() proceeds to rebase runtime->status->hw_ptr and runtime->control->appl_ptr. > + if (snd_pcm_running(substream)) { > + /* Keep completions arriving after the core's pointer > observation. */ > + vss->capture_acked_total = vss->capture_pointer_seen_total; > + vss->capture_ready = vss->capture_free_running ? 0 : > + vss->capture_copied_total - vss->capture_acked_total; > + } else { > + position = frames_to_bytes(runtime, runtime->status->hw_ptr); > + vss->hw_ptr = position; > + period = snd_pcm_lib_period_bytes(substream); > + vss->capture_copied_total = position; > + vss->capture_acked_total = position; > + vss->capture_pointer_seen_total = position; > + vss->capture_ready = 0; > + vss->capture_notified_periods = period ? position / period : 0; > + } [Severity: High] Should the running-stream branch in virtsnd_pcm_reset() also advance vss->capture_notified_periods to match vss->capture_pointer_seen_total? If a completion in virtsnd_pcm_capture_complete() crosses a period boundary and schedules vss->elapsed_period, and userspace invokes SNDRV_PCM_IOCTL_RESET before virtsnd_pcm_period_elapsed() acquires the PCM stream lock, snd_pcm_lib_reset() -> snd_pcm_update_hw_ptr() -> virtsnd_pcm_cp_pointer() observes the new vss->capture_copied_total and updates vss->capture_pointer_seen_total. In snd_pcm_do_reset(), the core then folds that position into runtime->hw_ptr_interrupt: sound/core/pcm_native.c:snd_pcm_do_reset() { ... 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; ... } Because the running branch in virtsnd_pcm_reset() leaves vss->capture_notified_periods at its pre-reset value, the pending virtsnd_pcm_period_elapsed() worker later sees period_floor > vss->capture_notified_periods for a period boundary already accounted for in runtime->hw_ptr_interrupt: sound/virtio/virtio_pcm.c:virtsnd_pcm_period_elapsed() { ... period_floor = div64_ul(vss->capture_copied_total, period_bytes); if (period_floor > vss->capture_notified_periods) { /* Reserve crossed boundaries before the callback. */ vss->capture_notified_periods = period_floor; notify = true; } ... } This triggers a duplicate snd_pcm_period_elapsed_under_stream_lock() -> snd_pcm_update_hw_ptr0(substream, 1) call where delta > new_hw_ptr: sound/core/pcm_lib.c:snd_pcm_update_hw_ptr0() { ... if (in_interrupt) { /* we know that one period was processed */ /* delta = "expected next hw_ptr" for in_interrupt != 0 */ delta = runtime->hw_ptr_interrupt + runtime->period_size; if (delta > new_hw_ptr) { /* check for double acknowledged interrupts */ hdelta = curr_jiffies - runtime->hw_ptr_jiffies; if (hdelta > runtime->hw_ptr_buffer_jiffies/2 + 1) { hw_base += runtime->buffer_size; ... } If the jiffies delta exceeds half the buffer duration, snd_pcm_update_hw_ptr0() treats the duplicate notification as a missed full-buffer wrap-around and advances runtime->status->hw_ptr by a full buffer_size of uncaptured frames. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=13
