Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] ALSA: virtio: unrecoverable stream state after failed suspend sync_stop() - [Medium] ALSA: virtio: unprepared stream left startable if virtsnd_pcm_prepare() fails --
commit ff74351afcffdc15b798435ae2935317313176c5 Author: Joonho Lee <[email protected]> ALSA: virtio: Recover PCM state after a failed start This commit recovers virtio PCM substream state when a start trigger fails by disabling transfers, marking the stream as stopped under vss->lock, and arming runtime->stop_operating so virtsnd_pcm_sync_stop() releases pending I/O before subsequent prepare, hw_params, or start operations. > 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; > > /* [ ... ] > @@ -273,6 +295,16 @@ static int virtsnd_pcm_prepare(struct snd_pcm_substream > *substream) > struct virtio_pcm_substream *vss = snd_pcm_substream_chip(substream); > struct virtio_device *vdev = vss->snd->vdev; > struct virtio_snd_msg *msg; > + bool stopped; > + > + scoped_guard(spinlock_irqsave, &vss->lock) > + stopped = vss->stopped; > + if (stopped) { > + dev_err(&vdev->dev, "SID %u: stream release failed\n", > + vss->sid); > + substream->runtime->stop_operating = true; > + return -EBADFD; > + } > > if (!vss->suspended) { > if (virtsnd_pcm_msg_pending_num(vss)) { [Severity: High] Can a failed virtsnd_pcm_sync_stop() during system suspend leave the PCM substream in a permanently unrecoverable state after resume? During system suspend, do_pcm_suspend() -> snd_pcm_suspend_all() runs: snd_pcm_do_suspend() virtsnd_pcm_trigger(substream, SNDRV_PCM_TRIGGER_SUSPEND) vss->suspended = true; vss->stopped = true; snd_pcm_sync_stop(substream, false) virtsnd_pcm_sync_stop(substream) and ignores the return value of snd_pcm_sync_stop(). In virtsnd_pcm_sync_stop(), allocating or sending VIRTIO_SND_R_PCM_RELEASE or waiting in wait_event_interruptible_timeout() for vss->msg_empty can fail or be interrupted (for example, returning -ERESTARTSYS when a signal or the freezer is pending, or -ETIMEDOUT while vss->msg_count > 0). With this change, virtsnd_pcm_sync_stop() leaves vss->stopped = true and re-arms substream->runtime->stop_operating = true. Subsequently, virtsnd_freeze() resets the device and deletes the virtqueues without clearing vss->stopped or vss->msg_count: virtsnd_freeze() virtio_reset_device(vdev); vdev->config->del_vqs(vdev); which returns all device-side streams to their initial unconfigured state. Previously, post-resume snd_pcm_prepare() could recover because runtime->stop_operating had been cleared by snd_pcm_sync_stop(), virtsnd_pcm_prepare() did not check vss->stopped, and the vss->suspended branch skipped the pending-message check, re-sent VIRTIO_SND_R_PCM_SET_PARAMS to the reset device, and reset vss->msg_count = 0. With this patch, when userspace calls snd_pcm_prepare() after resume: 1. Because runtime->stop_operating was re-armed, snd_pcm_do_prepare() calls virtsnd_pcm_sync_stop() first. Seeing vss->stopped == true, virtsnd_pcm_sync_stop() sends VIRTIO_SND_R_PCM_RELEASE to a freshly reset, unconfigured stream before VIRTIO_SND_R_PCM_SET_PARAMS has been sent, and waits on vss->msg_empty for a stale vss->msg_count on the recreated empty virtqueue, timing out with -ETIMEDOUT. 2. Even if virtsnd_pcm_prepare() is reached, checking if (stopped) before if (!vss->suspended) returns -EBADFD immediately and makes the vss->suspended recovery path unreachable. 3. Reconfiguring via virtsnd_pcm_hw_params() is also rejected by the new stopped and msg_count checks, and closing and reopening the substream via virtsnd_pcm_open() preserves the stale vss->stopped = true (due to vss->stopped |= !!vss->msg_count), clears vss->suspended = false, and calls virtsnd_pcm_sync_stop() again. Later commits in this series ("ALSA: virtio: Retain queued messages through device reset" and "ALSA: virtio: wait through signals for PCM teardown") detach unused buffers in virtsnd_freeze() and make the control message wait non-interruptible, but wait_event_interruptible_timeout() in virtsnd_pcm_sync_stop() remains interruptible and virtsnd_freeze() still does not clear vss->stopped when virtio_reset_device() resets the device-side stream state. If virtsnd_pcm_sync_stop() fails during suspend, post-resume sync_stop() will still send VIRTIO_SND_R_PCM_RELEASE to an unconfigured stream before SET_PARAMS and block prepare(), hw_params(), and open() if the reset device rejects RELEASE in its initial state. [ ... ] > @@ -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] What happens if virtsnd_pcm_sync_stop() succeeds after a failed start, but the subsequent VIRTIO_SND_R_PCM_PREPARE command in virtsnd_pcm_prepare() fails? In sound/core/pcm_native.c:snd_pcm_do_start(): err = substream->ops->trigger(substream, SNDRV_PCM_TRIGGER_START); /* XRUN happened during the start */ if (err == -EPIPE) __snd_pcm_set_state(substream->runtime, SNDRV_PCM_STATE_XRUN); return err; When virtsnd_pcm_trigger(SNDRV_PCM_TRIGGER_START) fails with an error other than -EPIPE (such as -ENOMEM from virtsnd_pcm_ctl_msg_alloc(), or an error from virtsnd_pcm_msg_send() or virtsnd_ctl_msg_send_sync()), substream->runtime->state remains SNDRV_PCM_STATE_PREPARED while virtsnd_pcm_start_failed() sets vss->stopped = true and runtime->stop_operating = true. When userspace then calls snd_pcm_prepare() to recover: 1. snd_pcm_do_prepare() calls snd_pcm_sync_stop(substream, true), which clears runtime->stop_operating = false and runs virtsnd_pcm_sync_stop(). 2. virtsnd_pcm_sync_stop() sends VIRTIO_SND_R_PCM_RELEASE to the device (moving the device stream out of the prepared state into the released state), waits for queued messages to drain, and clears vss->stopped = false. 3. snd_pcm_do_prepare() then calls virtsnd_pcm_prepare(), which allocates and sends VIRTIO_SND_R_PCM_PREPARE: msg = virtsnd_pcm_ctl_msg_alloc(vss, VIRTIO_SND_R_PCM_PREPARE, GFP_KERNEL); if (!msg) return -ENOMEM; return virtsnd_ctl_msg_send_sync(vss->snd, msg); If allocating or sending VIRTIO_SND_R_PCM_PREPARE fails here, snd_pcm_do_prepare() returns the error without changing substream->runtime->state. At that point, the device stream is in the released (unprepared) state, but substream->runtime->state is still SNDRV_PCM_STATE_PREPARED, runtime->stop_operating is false, and vss->stopped is false. Wouldn't a subsequent SNDRV_PCM_IOCTL_START, write(), or SNDRV_PCM_IOCTL_SYNC_PTR bypass both ALSA's state check and the vss->stopped check in virtsnd_pcm_trigger(), queueing I/O messages and sending VIRTIO_SND_R_PCM_START to an unprepared device stream? Note that a later commit in this series ("ALSA: virtio: stage VZ capture and restart paused streams") handles virtsnd_pcm_dev_prepare() failure when pause_restart is true, but leaves the standard (!pause_restart) path unhandled. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
