Re: [PATCH v8 02/14] ALSA: virtio: Recover PCM state after a failed start

[sashiko-bot@kernel.org]

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 <mynameisjune111@gmail.com>

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/20261029054354.2931-1-mynameisjune111@gmail.com?part=2