Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] ALSA: virtio: missing xfer_xrun latch in virtsnd_pcm_transfer_failed() --
commit 42ba5fb421db3215b1318325133b70f45151e844 Author: Joonho Lee <[email protected]> ALSA: virtio: serialize staged capture lifecycle and defer paused XRUNs This commit introduces request-owned RX staging for capture streams, serializes PCM lifecycle operations with lifecycle_lock, and defers XRUN handling for paused linked streams until resume. > diff --git a/sound/virtio/virtio_pcm_ops.c b/sound/virtio/virtio_pcm_ops.c > index 706d59d8c7201..36f8128a3f30c 100644 > --- a/sound/virtio/virtio_pcm_ops.c > +++ b/sound/virtio/virtio_pcm_ops.c [ ... ] > @@ -637,132 +1739,515 @@ static void virtsnd_pcm_transfer_failed(struct > virtio_pcm_substream *vss) > vss->substream->runtime->stop_operating = true; > } > > +/* Caller holds vss->lock; a paused ACK must not trigger linked XRUN. */ > +static int virtsnd_pcm_ack_xrun_locked(struct virtio_pcm_substream *vss, > + struct snd_pcm_runtime *runtime) > +{ > + vss->xfer_xrun = true; > + if (runtime->state == SNDRV_PCM_STATE_PAUSED) { > + vss->xfer_enabled = false; > + runtime->stop_operating = true; > + virtsnd_pcm_schedule_period_elapsed(vss); > + return -EBADFD; > + } > + return -EPIPE; > +} > + > +static int virtsnd_pcm_pb_ack_locked(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 = {}; > + bool paused; > + int rc; > + > + scoped_guard(spinlock_irqsave, &queue->lock) { > + guard(spinlock)(&vss->lock); > + paused = runtime->state == SNDRV_PCM_STATE_PAUSED; > + if (vss->preparing) > + return -EBADFD; > + /* A latched XRUN must not re-arm sync_stop once teardown > began. */ > + if (vss->stopped && vss->xfer_xrun) > + return -EBADFD; > + if (vss->xfer_xrun) { > + rc = virtsnd_pcm_ack_xrun_locked(vss, runtime); > + } else { > + if (!vss->prepared || vss->stopped) > + return -EBADFD; > + transfer.indirect = vss->pcm_indirect; > + rc = snd_pcm_indirect_playback_transfer(substream, > + > &transfer.indirect, > + > virtsnd_pcm_trans_copy); > + } > + if (!rc) > + rc = transfer.error; > + if (rc) { > + if (paused) > + rc = virtsnd_pcm_ack_xrun_locked(vss, runtime); > + else > + virtsnd_pcm_transfer_failed(vss); > + } else { > + vss->pcm_indirect = transfer.indirect; > + } > + } [ ... ] > +static int virtsnd_pcm_cp_ack_locked(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 = {}; > + bool paused; > + int rc; > + > + if (vss->capture_staging) { > + rc = virtsnd_pcm_capture_staged_ack(vss, runtime); > + if (rc == -EPIPE) { > + guard(spinlock_irqsave)(&vss->lock); > + virtsnd_pcm_transfer_failed(vss); > + } > + return rc; > + } > + > + scoped_guard(spinlock_irqsave, &queue->lock) { > + guard(spinlock)(&vss->lock); > + paused = runtime->state == SNDRV_PCM_STATE_PAUSED; > + if (vss->preparing) > + return -EBADFD; > + /* A latched XRUN must not re-arm sync_stop once teardown > began. */ > + if (vss->stopped && vss->xfer_xrun) > + return -EBADFD; > + if (vss->xfer_xrun) { > + rc = virtsnd_pcm_ack_xrun_locked(vss, runtime); > + } else { > + if (!vss->prepared || > + (vss->stopped && > + runtime->state != SNDRV_PCM_STATE_DRAINING)) > + return -EBADFD; > + transfer.indirect = vss->pcm_indirect; > + rc = snd_pcm_indirect_capture_transfer(substream, > + > &transfer.indirect, > + > virtsnd_pcm_trans_copy); > + } > + if (!rc) > + rc = transfer.error; > + if (rc) { > + if (paused) > + rc = virtsnd_pcm_ack_xrun_locked(vss, runtime); > + else > + virtsnd_pcm_transfer_failed(vss); > + } else { > + vss->pcm_indirect = transfer.indirect; > + } > + } [Severity: Medium] Should virtsnd_pcm_transfer_failed() in sound/virtio/virtio_pcm_ops.c also latch vss->xfer_xrun = true? When a non-paused indirect playback or capture ACK transfer fails in virtsnd_pcm_pb_ack_locked() or virtsnd_pcm_cp_ack_locked() (for example, when snd_pcm_indirect_playback_transfer() or snd_pcm_indirect_capture_transfer() returns -EPIPE on a negative appl_ptr diff, or virtsnd_pcm_msg_send() sets transfer.error), the error path calls virtsnd_pcm_transfer_failed(): static void virtsnd_pcm_transfer_failed(struct virtio_pcm_substream *vss) { vss->xfer_enabled = false; vss->stopped = true; vss->substream->runtime->stop_operating = true; } Unlike the paused path (which calls virtsnd_pcm_ack_xrun_locked()) and the staged capture path (where virtsnd_pcm_capture_staged_ack() calls virtsnd_pcm_ack_xrun_locked() before virtsnd_pcm_transfer_failed()), virtsnd_pcm_transfer_failed() leaves vss->xfer_xrun false while vss->prepared and vss->stopped are true. Because vss->xfer_xrun is not set: 1. In sound/virtio/virtio_pcm_msg.c, in-flight non-empty RX completions arriving in virtsnd_pcm_msg_complete() for a non-staged capture stream bypass the early retirement check: if (vss->direction == SNDRV_PCM_STREAM_CAPTURE && (!vss->prepared || vss->xfer_xrun || (vss->stopped && written_bytes <= sizeof(*msg->status)))) { and continue copying data and advancing vss->hw_ptr and vss->capture_copied_total after the failed ACK rolled back vss->pcm_indirect and appl_ptr. 2. If the capture stream is in SNDRV_PCM_STATE_DRAINING, the stopped-plus-XRUN admission check (if (vss->stopped && vss->xfer_xrun) return -EBADFD) in virtsnd_pcm_cp_ack_locked() is not armed. 3. Any call to virtsnd_pcm_pb_pointer() or virtsnd_pcm_cp_pointer() before teardown (such as across the stream lock drop when locking a linked group in __snd_pcm_xrun()) does not see vss->xfer_xrun set, so it invokes snd_pcm_indirect_playback_pointer() or snd_pcm_indirect_capture_pointer() instead of returning SNDRV_PCM_POS_XRUN. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=10
