Re: [PATCH v23 12/14] ALSA: virtio: add opt-in staged capture and recovery

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] ALSA: virtio: missed playback wakeup and wrap in
  virtsnd_pcm_trigger_inner()
--

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

ALSA: virtio: add opt-in staged capture and recovery

This commit adds an opt-in request-owned staged capture path with compatibility
options and updates PCM control serialization, pause/resume handling, and
reset callbacks.

> diff --git a/sound/virtio/virtio_pcm_msg.c b/sound/virtio/virtio_pcm_msg.c
> index 0c56f7f4af0f6..4528259597a36 100644
> --- a/sound/virtio/virtio_pcm_msg.c
> +++ b/sound/virtio/virtio_pcm_msg.c
[ ... ]
> @@ -536,78 +876,103 @@ static void virtsnd_pcm_msg_complete(struct virtio_pcm_msg *msg,
[ ... ]
> +	vss->hw_ptr += completed_bytes;
> +	if (vss->direction == SNDRV_PCM_STREAM_CAPTURE) {
> +		/* A full unseen ring cannot be represented by the modulo pointer. */
> +		if (check_add_overflow(vss->capture_copied_total,
> +				       (u64)completed_bytes,
> +				       &vss->capture_copied_total) ||
> +		    vss->capture_copied_total < vss->capture_pointer_seen_total ||
> +		    vss->capture_copied_total - vss->capture_pointer_seen_total >=
> +			vss->buffer_bytes)
> +			vss->xfer_xrun = true;
> +	}
>  
> +	if (vss->hw_ptr >= vss->buffer_bytes)
> +		vss->hw_ptr -= vss->buffer_bytes;
> +
>  	msg->length = 0;
> +
> +	/* Keep latched errors until prepare establishes a fresh epoch. */
>  	vss->msg_count--;
>  
> -	if (vss->xfer_enabled) {
> -		if (!capture_invalid) {
> -			struct snd_pcm_runtime *runtime = vss->substream->runtime;
> +	if (vss->xfer_enabled ||
> +	    (vss->direction == SNDRV_PCM_STREAM_CAPTURE && vss->xfer_xrun)) {
> +		struct snd_pcm_runtime *runtime = vss->substream->runtime;
>  
> -			runtime->delay =
> -				bytes_to_frames(runtime,
> -						le32_to_cpu(msg->status.latency_bytes));
> -		}
> -		schedule_work(&vss->elapsed_period);
> +		runtime->delay = bytes_to_frames(runtime, latency_bytes);
> +		virtsnd_pcm_schedule_period_elapsed(vss);
>  	}
>  	if (!vss->msg_count)
>  		wake_up_all(&vss->msg_empty);
>  }
[ ... ]
> diff --git a/sound/virtio/virtio_pcm_ops.c b/sound/virtio/virtio_pcm_ops.c
> index 1496725cba005..d953cd4babf6b 100644
> --- a/sound/virtio/virtio_pcm_ops.c
> +++ b/sound/virtio/virtio_pcm_ops.c
[ ... ]
> +	switch (command) {
> +	case SNDRV_PCM_TRIGGER_START:
> +	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> +		scoped_guard(spinlock_irqsave, &vss->lock)
> +			running = vss->device_running;
> +		/* Preserve queued requests and unread RX data across native pause. */
> +		rc = virtsnd_pcm_start_queue(vss, epoch);
> +		if (rc) {
> +			virtsnd_pcm_trigger_failed(vss, epoch);
> +			return rc;
>  		}
> +
> +		if (running) {
> +			if (vss->direction == SNDRV_PCM_STREAM_CAPTURE)
> +				virtsnd_pcm_schedule_period_elapsed(vss);
> +			return 0;
> +		}
> +
> +		msg = virtsnd_pcm_ctl_msg_alloc(vss, VIRTIO_SND_R_PCM_START,
> +						GFP_KERNEL, epoch);
>  		if (!msg) {
> -			virtsnd_pcm_start_failed(vss);
> +			virtsnd_pcm_trigger_failed(vss, epoch);
>  			return -ENOMEM;
>  		}
>  
> -		rc = virtsnd_ctl_msg_send_sync(snd, msg);
> -		if (rc)
> -			virtsnd_pcm_start_failed(vss);
> +		rc = virtsnd_pcm_send_sync(vss, msg);
> +		if (rc && rc != -ECANCELED)
> +			virtsnd_pcm_trigger_failed(vss, epoch);
>  
> +		if (!rc) {
> +			guard(spinlock_irqsave)(&vss->lock);
> +			if (vss->resetting || vss->reset_epoch != epoch)
> +				return -ECANCELED;
> +			vss->device_running = true;
> +		}
> +		if (!rc && vss->direction == SNDRV_PCM_STREAM_CAPTURE)
> +			virtsnd_pcm_schedule_period_elapsed(vss);
>  		return rc;
>  	case SNDRV_PCM_TRIGGER_SUSPEND:
> -		vss->suspended = true;
> +		scoped_guard(spinlock_irqsave, &vss->lock)
> +			vss->suspended = true;
>  		fallthrough;
>  	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;
>  			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_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]
Can pausing a playback stream stall playback when in-flight TX descriptors
complete while vss->xfer_enabled is false?

SNDRV_PCM_TRIGGER_PAUSE_PUSH clears vss->xfer_enabled before
virtsnd_pcm_stop_device() sends VIRTIO_SND_R_PCM_STOP.  Any TX descriptors
that complete in virtsnd_pcm_msg_complete() while vss->xfer_enabled is false
advance vss->hw_ptr and decrement vss->msg_count without scheduling
vss->elapsed_period, and any work already queued returns early in
virtsnd_pcm_period_elapsed() while the substream is paused.

When playback resumes via SNDRV_PCM_TRIGGER_PAUSE_RELEASE (or recovers from a
failed pause push while still running), virtsnd_pcm_trigger_inner() only
schedules vss->elapsed_period for capture.  If all queued TX descriptors
completed while paused (vss->msg_count == 0), no further TX interrupt will
arrive to run virtsnd_pcm_pb_pointer(), reclaim vss->pcm_indirect.hw_ready, or
submit pending vss->pcm_indirect.sw_ready bytes.

In addition, virtsnd_pcm_msg_complete() only checks for a full-buffer unseen
completion wrap on capture.  Because virtsnd_pcm_playback_ack_validate()
allows queuing a full buffer of TX descriptors, completing all of them before
virtsnd_pcm_pb_pointer() runs wraps vss->hw_ptr modulo vss->buffer_bytes back
to vss->pcm_indirect.hw_io, so snd_pcm_indirect_playback_pointer() observes
zero progress and leaves vss->pcm_indirect.hw_ready stuck at
vss->buffer_bytes.

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