Re: [PATCH v27 12/17] ALSA: virtio: use request-owned capture and serialize recovery

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] ALSA: virtio: stale out-of-order capture msgs on reset and
  xrun
- [Medium] ALSA: virtio: unrestored capture state in
  virtsnd_pcm_trigger_inner()
--

commit ad4ce44873820d014515d7d58c0d615ba2bfd397
Author: Joonho Lee <mynameisjune111@gmail.com>

ALSA: virtio: use request-owned capture and serialize recovery

This commit switches VirtIO sound capture to request-owned buffers that are
committed in submission order, and serializes PCM control state transitions,
preparation, and reset recovery.

> diff --git a/sound/virtio/virtio_pcm_msg.c b/sound/virtio/virtio_pcm_msg.c
> index a3e23fdbea149..f04e25c4cde77 100644
> --- a/sound/virtio/virtio_pcm_msg.c
> +++ b/sound/virtio/virtio_pcm_msg.c
[ ... ]
> @@ -473,6 +549,365 @@ int virtsnd_pcm_msg_send(struct virtio_pcm_substream *vss, unsigned long offset,
[ ... ]
> +static void virtsnd_pcm_capture_complete(struct virtio_pcm_msg *msg,
> +					 size_t written_bytes)
> +{
> +	struct virtio_pcm_substream *vss = msg->substream;
> +	struct snd_pcm_runtime *runtime;
> +	size_t audio_bytes = 0;
> +	u32 latency_bytes = 0;
> +	size_t frame_bytes;
> +	bool got_audio = false;
> +	bool accept_completion;
> +
> +	guard(spinlock)(&vss->lock);
> +	vss->msg_count--;
> +	accept_completion = vss->runtime_active && vss->prepared &&
> +		(!vss->stopped || vss->capture_stopping) && !vss->xfer_xrun;
> +	if (accept_completion) {
> +		runtime = vss->substream->runtime;
> +		if (runtime && runtime->dma_area) {
> +			frame_bytes = runtime->frame_bits /
> +				gcd(runtime->frame_bits, 8);
> +			msg->capture_valid = !virtsnd_pcm_capture_status(
> +				msg, written_bytes, frame_bytes, &audio_bytes,
> +				&latency_bytes);
> +			msg->capture_audio_bytes = audio_bytes;
> +			msg->capture_latency_bytes = latency_bytes;
> +			msg->capture_complete = true;
> +			got_audio = virtsnd_pcm_capture_commit_ready_locked(vss);
> +		} else {
> +			msg->length = 0;
> +			msg->capture_complete = false;
> +		}
> +	} else {
> +		msg->length = 0;
> +		msg->capture_complete = false;
> +	}

[Severity: Medium]
Can out-of-order RX completions leave stale buffered messages behind when
accept_completion is false or across virtsnd_pcm_reset()?

If request N+1 completes before request N, it is held in vss->msgs with
capture_complete set to true while waiting for capture_commit_sequence to
reach N+1.  If vss->xfer_xrun is then set (for example via
virtsnd_pcm_event() or virtsnd_pcm_cp_pointer()) before request N completes,
request N takes this else branch, which clears only request N without
advancing capture_commit_sequence or clearing request N+1.  Likewise,
virtsnd_pcm_reset() does not clear buffered capture_complete entries or
resynchronize capture_commit_sequence, so a reset while request N+1 is buffered
will either commit pre-reset audio once request N completes or leave
capture_commit_sequence wedged at N.

> diff --git a/sound/virtio/virtio_pcm_ops.c b/sound/virtio/virtio_pcm_ops.c
> index a5a682534acf7..6d6c1bcd8601b 100644
> --- a/sound/virtio/virtio_pcm_ops.c
> +++ b/sound/virtio/virtio_pcm_ops.c
[ ... ]
> @@ -325,182 +622,834 @@ static int virtsnd_pcm_hw_params(struct snd_pcm_substream *substream,
[ ... ]
>  	case SNDRV_PCM_TRIGGER_STOP:
>  	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
>  		scoped_guard(spinlock_irqsave, &vss->lock) {
> +			if (!vss->prepared)
> +				return 0;
>  			if (command != SNDRV_PCM_TRIGGER_PAUSE_PUSH)
>  				vss->stopped = true;
> +			if (vss->direction == SNDRV_PCM_STREAM_CAPTURE) {
> +				vss->capture_stopping = true;
> +				vss->capture_quiet_sequence =
> +					vss->capture_submit_sequence;
> +			}
>  			vss->xfer_enabled = false;
> +			if (!vss->device_running)
> +				return 0;
>  		}
>  
> -		msg = virtsnd_pcm_ctl_msg_alloc(vss, VIRTIO_SND_R_PCM_STOP,
> -						GFP_KERNEL);
> -		if (!msg)
> -			return -ENOMEM;
> +		rc = virtsnd_pcm_stop_device(vss, epoch);
> +		if (rc && command != SNDRV_PCM_TRIGGER_PAUSE_PUSH) {
> +			guard(spinlock_irqsave)(&vss->lock);
>  
> -		return virtsnd_ctl_msg_send_sync(snd, msg);
> +			if (vss->reset_epoch == epoch && !vss->resetting) {
> +				vss->xfer_xrun = true;
> +				substream->runtime->stop_operating = true;
> +				if (vss->direction == SNDRV_PCM_STREAM_CAPTURE)
> +					virtsnd_pcm_schedule_period_elapsed(vss);
> +			}
> +		}
> +		if (rc && command == SNDRV_PCM_TRIGGER_PAUSE_PUSH) {
> +			if (substream->runtime->state != SNDRV_PCM_STATE_RUNNING) {
> +				/* Failed resume rollback must leave a paused queue closed. */
> +				virtsnd_pcm_trigger_failed(vss, epoch);
> +			} else if (virtsnd_pcm_start_queue(vss, epoch)) {
> +				virtsnd_pcm_trigger_failed(vss, epoch);
> +			} else if (vss->direction == SNDRV_PCM_STREAM_CAPTURE) {
> +				virtsnd_pcm_schedule_period_elapsed(vss);
> +			}
> +		}
> +		return rc;

[Severity: Medium]
Does a failed SNDRV_PCM_TRIGGER_PAUSE_PUSH leave vss->capture_stopping set
and vss->capture_quiet_sequence advanced when rolling back a running capture
stream?

When virtsnd_pcm_stop_device() fails on a running capture substream, the
rollback path reopens the queue via virtsnd_pcm_start_queue(), but neither
that helper nor this branch restores capture_stopping or
capture_quiet_sequence.  All in-flight RX requests queued before the failed
pause then have capture_sequence < capture_quiet_sequence and bypass the
empty-completion XRUN checks in virtsnd_pcm_capture_commit_one_locked() even
though the stream is still running.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112004704.49758-1-mynameisjune111@gmail.com?part=12