Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] ALSA: virtio: virtsnd_pcm_sync_stop() resends PCM_RELEASE after drain failure --
commit 8526e884d4c74a3678dceb6e148567c0ca936c95 Author: Joonho Lee <[email protected]> ALSA: virtio: Recover PCM state after a failed start This patch recovers PCM substream state when a start trigger fails by marking the stream stopped, disabling transfers, arming stop_operating, and protecting accesses to the stopped flag with vss->lock. > diff --git a/sound/virtio/virtio_pcm_ops.c b/sound/virtio/virtio_pcm_ops.c > index a40ea394bf174..970499fe419b4 100644 > --- a/sound/virtio/virtio_pcm_ops.c > +++ b/sound/virtio/virtio_pcm_ops.c [ ... ] > @@ -115,7 +124,8 @@ static int virtsnd_pcm_open(struct snd_pcm_substream > *substream) > snd_pcm_hw_constraint_integer(substream->runtime, > SNDRV_PCM_HW_PARAM_PERIODS); > > - vss->stopped = !!virtsnd_pcm_msg_pending_num(vss); > + scoped_guard(spinlock_irqsave, &vss->lock) > + vss->stopped |= !!vss->msg_count; > vss->suspended = false; [ ... ] > @@ -393,21 +434,31 @@ static int virtsnd_pcm_sync_stop(struct > snd_pcm_substream *substream) > struct virtio_snd *snd = vss->snd; > struct virtio_snd_msg *msg; > unsigned int js = msecs_to_jiffies(virtsnd_msg_timeout_ms); > + bool need_release; > int rc; > > + scoped_guard(spinlock_irqsave, &vss->lock) { > + vss->xfer_enabled = false; > + need_release = vss->stopped || vss->msg_count; > + if (need_release) > + vss->stopped = true; > + } > cancel_work_sync(&vss->elapsed_period); > - > - if (!vss->stopped) > + if (!need_release) > return 0; > > msg = virtsnd_pcm_ctl_msg_alloc(vss, VIRTIO_SND_R_PCM_RELEASE, > GFP_KERNEL); > - if (!msg) > + if (!msg) { > + substream->runtime->stop_operating = true; > return -ENOMEM; > + } > > rc = virtsnd_ctl_msg_send_sync(snd, msg); > - if (rc) > + if (rc) { > + substream->runtime->stop_operating = true; > return rc; > + } > > /* > * The spec states that upon receipt of the RELEASE command "the device > @@ -421,11 +472,13 @@ static int virtsnd_pcm_sync_stop(struct > snd_pcm_substream *substream) > if (rc <= 0) { > dev_warn(&snd->vdev->dev, "SID %u: failed to flush I/O queue\n", > vss->sid); > + substream->runtime->stop_operating = true; > > return !rc ? -ETIMEDOUT : rc; > } > > - vss->stopped = false; > + scoped_guard(spinlock_irqsave, &vss->lock) > + vss->stopped = false; > > return 0; > } [Severity: Medium] Can virtsnd_pcm_sync_stop() send a duplicate VIRTIO_SND_R_PCM_RELEASE command if the post-release queue drain wait is interrupted or times out? In virtsnd_pcm_sync_stop(), after virtsnd_ctl_msg_send_sync() succeeds for VIRTIO_SND_R_PCM_RELEASE, wait_event_interruptible_timeout() waits for in-flight I/O messages to drain. If a signal is pending or the wait times out before the last message completes, virtsnd_pcm_sync_stop() sets substream->runtime->stop_operating = true and returns an error while leaving vss->stopped = true. Once the device finishes returning the remaining I/O messages and vss->msg_count reaches 0, vss->stopped remains true because a single boolean tracks both whether VIRTIO_SND_R_PCM_RELEASE still needs to be sent and whether pending I/O messages have finished draining. Because virtsnd_pcm_open() now uses vss->stopped |= !!vss->msg_count, and virtsnd_pcm_hw_params() and virtsnd_pcm_prepare() reject the substream with -EBADFD while vss->stopped is true, the next virtsnd_pcm_sync_stop() call (during close, hw_free, prepare, hw_params, or reopen via virtsnd_pcm_open()) sees need_release = vss->stopped || vss->msg_count as true and sends a second VIRTIO_SND_R_PCM_RELEASE command for a stream that was already released. If the device rejects the duplicate release command, virtsnd_pcm_sync_stop() fails again and leaves vss->stopped = true, wedging the substream. Note that a later commit in the series ("ALSA: virtio: add staged capture accounting and lifecycle recovery") changes the drain wait in virtsnd_pcm_sync_stop_locked() to wait_event_timeout() and tracks VIRTSND_PCM_PAUSE_RESTART_RELEASE_SENT in the opt-in pause-restart state machine, but the default virtsnd_pcm_sync_stop_locked() path still leaves vss->stopped = true (and vss->prepared = true) if the drain wait times out after VIRTIO_SND_R_PCM_RELEASE succeeded. Should the driver track whether VIRTIO_SND_R_PCM_RELEASE has already succeeded separately from whether vss->msg_count has drained to 0, so a subsequent virtsnd_pcm_sync_stop() only waits for remaining messages to drain instead of resending VIRTIO_SND_R_PCM_RELEASE? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
