Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [Medium] ALSA: virtio: unnotified capture periods across pause cause lost wakeup or XRUN - [Medium] ALSA: virtio: staged capture counters not updated on SNDRV_PCM_IOCTL_RESET - [Medium] ALSA: virtio: race between virtsnd_pcm_pause_restart_sync_stop() and prepare --
commit 769b2eda886498f63ca7e4ba1a0aa3578ffd2f05 Author: Joonho Lee <[email protected]> ALSA: virtio: stage VZ capture and restart paused streams This commit adds opt-in capture staging using request-owned buffers with response validation and ring accounting, along with a pause release and reprepare state machine for staged capture streams. > diff --git a/sound/virtio/virtio_pcm.c b/sound/virtio/virtio_pcm.c > index be3893de40a55..a3478fc8848db 100644 > --- a/sound/virtio/virtio_pcm.c > +++ b/sound/virtio/virtio_pcm.c > @@ -310,8 +310,94 @@ static void virtsnd_pcm_period_elapsed(struct > work_struct *work) > { > struct virtio_pcm_substream *vss = > container_of(work, struct virtio_pcm_substream, elapsed_period); > + struct snd_pcm_substream *substream = vss->substream; > + struct snd_pcm_runtime *runtime; > + size_t period_bytes; > + u64 period_floor; > + u64 snapshot; > + bool staged_capture; > + bool paused_xrun = false; > + bool notify = false; > + bool requeue = false; > + > + scoped_guard(spinlock_irqsave, &vss->lock) { > + staged_capture = vss->direction == SNDRV_PCM_STREAM_CAPTURE && > + vss->capture_staging; > + } > + > + if (!staged_capture) { > + snd_pcm_period_elapsed(substream); > + return; > + } > + > + /* Serialize the pointer snapshot with ALSA's pointer and ACK > callbacks. */ > + guard(pcm_stream_lock_irqsave)(substream); > + if (!snd_pcm_running(substream)) { > + scoped_guard(spinlock_irqsave, &vss->lock) { > + runtime = substream->runtime; > + paused_xrun = runtime && > + runtime->state == SNDRV_PCM_STATE_PAUSED > && > + vss->runtime_active && vss->xfer_xrun; > + } > + if (paused_xrun) { > + /* Wake readers without sending STOP for a paused > stream. */ > + snd_pcm_stop(substream, SNDRV_PCM_STATE_XRUN); > + } > + return; > + } [Severity: Medium] What happens if a staged capture completion crosses a period boundary right before SNDRV_PCM_TRIGGER_PAUSE_PUSH? When virtsnd_pcm_capture_complete() copies data crossing a period boundary and schedules vss->elapsed_period right as snd_pcm_pause() runs, snd_pcm_pause() holds pcm_stream_lock across virtsnd_pcm_trigger() and snd_pcm_post_pause(), which sets runtime->state to SNDRV_PCM_STATE_PAUSED. When virtsnd_pcm_period_elapsed() in sound/virtio/virtio_pcm.c subsequently acquires pcm_stream_lock, !snd_pcm_running(substream) is true, so it returns early without advancing vss->capture_notified_periods or calling snd_pcm_period_elapsed_under_stream_lock() to update runtime->status->hw_ptr. Later, when the stream is resumed via SNDRV_PCM_TRIGGER_PAUSE_RELEASE in virtsnd_pcm_pause_restart_start() or virtsnd_pcm_trigger() in sound/virtio/virtio_pcm_ops.c: sound/virtio/virtio_pcm_ops.c:virtsnd_pcm_pause_restart_start() { ... scoped_guard(spinlock_irqsave, &queue->lock) { guard(spinlock)(&vss->lock); if (vss->pause_restart_state != VIRTSND_PCM_PAUSE_RESTART_PREPARED || vss->msg_count || !vss->runtime_active || vss->xfer_xrun) { rc = -EBADFD; } else { rc = virtsnd_pcm_capture_send(vss); if (!rc) vss->xfer_enabled = true; } } } vss->elapsed_period is not rescheduled even though vss->capture_copied_total / period_bytes > vss->capture_notified_periods, and snd_pcm_post_pause() does not call snd_pcm_update_hw_ptr(). Could this leave readers in poll() or wait_for_avail() sleeping with avail == 0, and on a 2-period buffer cause the next completed period to hit audio_bytes >= vss->buffer_bytes - vss->capture_ready in virtsnd_pcm_capture_complete() and force a premature XRUN? [ ... ] > diff --git a/sound/virtio/virtio_pcm_ops.c b/sound/virtio/virtio_pcm_ops.c > index f74bf365065b6..003832d112551 100644 > --- a/sound/virtio/virtio_pcm_ops.c > +++ b/sound/virtio/virtio_pcm_ops.c [ ... ] > @@ -420,6 +932,106 @@ static int virtsnd_pcm_trigger(struct snd_pcm_substream > *substream, int command) > } > } > > +static int > +virtsnd_pcm_pause_restart_sync_stop(struct snd_pcm_substream *substream) > +{ > + struct virtio_pcm_substream *vss = snd_pcm_substream_chip(substream); > + unsigned int js = msecs_to_jiffies(virtsnd_msg_timeout_ms); > + enum virtio_pcm_pause_restart_state state; > + struct virtio_snd_msg *msg; > + long waited; > + int rc; > + > + scoped_guard(spinlock_irqsave, &vss->lock) { > + vss->xfer_enabled = false; > + vss->stopped = true; > + state = vss->pause_restart_state; > + } > + cancel_work_sync(&vss->elapsed_period); > + > + if (state == VIRTSND_PCM_PAUSE_RESTART_RESET_REQUIRED || > + virtsnd_pcm_pause_restart_transitioning(state)) { > + virtsnd_pcm_pause_restart_fail(vss, true); > + dev_err(&vss->snd->vdev->dev, > + "SID %u: PCM state is unknown; device reprobe > required\n", > + vss->sid); > + return -EIO; > + } [Severity: Medium] Can a concurrent SNDRV_PCM_IOCTL_DROP and SNDRV_PCM_IOCTL_PREPARE race while virtsnd_pcm_pause_restart_sync_stop() is sleeping and permanently wedge the substream in VIRTSND_PCM_PAUSE_RESTART_RESET_REQUIRED? In sound/core/pcm_native.c, snd_pcm_drop() transitions runtime->state to SNDRV_PCM_STATE_SETUP and sets runtime->stop_operating = true under pcm_stream_lock_irq(), then drops the stream lock and calls snd_pcm_sync_stop(substream, true) without holding runtime->buffer_mutex: snd_pcm_drop() snd_pcm_stop(substream, SNDRV_PCM_STATE_SETUP) runtime->stop_operating = true; snd_pcm_stream_unlock_irq(substream) snd_pcm_sync_stop(substream, true) substream->runtime->stop_operating = false; substream->ops->sync_stop(substream) virtsnd_pcm_pause_restart_sync_stop() Because snd_pcm_sync_stop() clears runtime->stop_operating before calling virtsnd_pcm_pause_restart_sync_stop(), a second thread invoking SNDRV_PCM_IOCTL_PREPARE while virtsnd_pcm_pause_restart_sync_stop() is sleeping in virtsnd_pcm_pause_restart_send() or wait_event_timeout() will see runtime->state == SNDRV_PCM_STATE_SETUP and runtime->stop_operating == false. Its own call to snd_pcm_sync_stop() in snd_pcm_do_prepare() will then return immediately without waiting for the first thread's sync_stop to finish, and virtsnd_pcm_prepare() (or virtsnd_pcm_pause_restart_send()) in sound/virtio/virtio_pcm_ops.c will observe the in-flight *_SUBMITTED state: sound/virtio/virtio_pcm_ops.c:virtsnd_pcm_prepare() { if (pause_restart) { enum virtio_pcm_pause_restart_state state; scoped_guard(spinlock_irqsave, &vss->lock) state = vss->pause_restart_state; if (state == VIRTSND_PCM_PAUSE_RESTART_RESET_REQUIRED || virtsnd_pcm_pause_restart_transitioning(state)) { virtsnd_pcm_pause_restart_fail(vss, true); return -EIO; } } } Wouldn't this transition vss->pause_restart_state to VIRTSND_PCM_PAUSE_RESTART_RESET_REQUIRED and leave the substream unusable until device reprobe? [ ... ] > @@ -586,11 +1233,66 @@ static int virtsnd_pcm_cp_ack(struct snd_pcm_substream > *substream) > { > struct virtio_pcm_substream *vss = snd_pcm_substream_chip(substream); > struct virtio_snd_queue *queue = virtsnd_pcm_queue(vss); > + struct snd_pcm_runtime *runtime = substream->runtime; > struct virtsnd_pcm_transfer transfer = {}; > int rc; > > scoped_guard(spinlock_irqsave, &queue->lock) { > guard(spinlock)(&vss->lock); > + if (vss->capture_staging) { > + snd_pcm_uframes_t appl_ptr; > + snd_pcm_sframes_t diff; > + size_t bytes; > + u64 next_acked_total; > + > + if (!vss->runtime_active || !runtime || > !runtime->control || > + !runtime->dma_area) > + return -EBADFD; > + if (vss->xfer_xrun) > + return -EPIPE; > + if (!virtsnd_pcm_capture_counters_valid(vss)) { > + vss->xfer_xrun = true; > + schedule_work(&vss->elapsed_period); > + return -EPIPE; > + } > + appl_ptr = runtime->control->appl_ptr; > + diff = appl_ptr - vss->pcm_indirect.appl_ptr; > + > + if (diff < -(snd_pcm_sframes_t)(runtime->boundary / 2)) > + diff += runtime->boundary; > + if (diff < 0 || > + (snd_pcm_uframes_t)diff > runtime->buffer_size) { > + vss->xfer_xrun = true; > + schedule_work(&vss->elapsed_period); > + return -EPIPE; > + } > + > + /* ACK includes any subperiod tail already copied to > the ring. */ > + bytes = frames_to_bytes(runtime, diff); > + if (bytes > vss->capture_ready || > + check_add_overflow(vss->capture_acked_total, > (u64)bytes, > + &next_acked_total) || > + next_acked_total > vss->capture_copied_total) { > + vss->xfer_xrun = true; > + schedule_work(&vss->elapsed_period); > + return -EPIPE; > + } [Severity: Medium] How does the staged capture accounting handle SNDRV_PCM_IOCTL_RESET when a stream is running or paused? virtsnd_pcm_ops uses snd_pcm_lib_ioctl, which handles SNDRV_PCM_IOCTL1_RESET via snd_pcm_lib_ioctl_reset(). When the stream is running or paused, snd_pcm_lib_ioctl_reset() updates runtime->status->hw_ptr (wrapping it modulo runtime->buffer_size or zeroing it if paused), zeroes runtime->hw_ptr_base, and sets runtime->control->appl_ptr = runtime->status->hw_ptr in snd_pcm_post_reset() without calling substream->ops->ack(). Because vss->capture_ready, vss->capture_acked_total, and vss->pcm_indirect.appl_ptr are not synchronized with that reset, any unread bytes discarded by advancing appl_ptr to hw_ptr remain counted in vss->capture_ready, which can cause virtsnd_pcm_capture_complete() to reject subsequent completions with a ring_capacity or ring_full XRUN before the ALSA ring is actually full. Furthermore, once runtime->hw_ptr_base is non-zero (after a buffer wrap) or when resetting while paused, resetting runtime->control->appl_ptr without updating vss->pcm_indirect.appl_ptr causes the next call to virtsnd_pcm_cp_ack() in sound/virtio/virtio_pcm_ops.c to compute a negative diff = appl_ptr - vss->pcm_indirect.appl_ptr and fail with -EPIPE. Should SNDRV_PCM_IOCTL1_RESET be intercepted in the ioctl callback to reconcile vss->pcm_indirect.appl_ptr, vss->capture_acked_total, and vss->capture_ready when vss->capture_staging is enabled? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=7
