Re: [PATCH v15 10/12] ALSA: virtio: add staged capture and serialize PCM lifecycle accounting

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] ALSA: virtio: staged capture ACK and pointer bypass prepared check in DRAINING
--

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

ALSA: virtio: add staged capture and serialize PCM lifecycle accounting

This commit adds an opt-in staged capture mode using request-owned RX
buffers with monotonic byte accounting for shared capture clients. It also
serializes PCM lifecycle transitions with lifecycle_lock and coordinates
pointer, reset, and ACK admission under the PCM stream lock.

> diff --git a/sound/virtio/virtio_pcm_ops.c b/sound/virtio/virtio_pcm_ops.c
> index 706d59d8c7201..a60f960287e2e 100644
> --- a/sound/virtio/virtio_pcm_ops.c
> +++ b/sound/virtio/virtio_pcm_ops.c

[ ... ]

> @@ -601,14 +1513,69 @@ static snd_pcm_uframes_t
>  virtsnd_pcm_cp_pointer(struct snd_pcm_substream *substream)
>  {
>  	struct virtio_pcm_substream *vss = snd_pcm_substream_chip(substream);
> +	struct snd_pcm_runtime *runtime = substream->runtime;
> +	size_t hw_ptr = 0;
> +	u64 total;
> +	bool free_running;
> +	bool xrun;
>  
> -	scoped_guard(spinlock_irqsave, &vss->lock)
> -		if (vss->xfer_xrun)
> +	if (vss->capture_staging) {
> +		scoped_guard(spinlock_irqsave, &vss->lock) {
> +			if (!vss->runtime_active || vss->preparing || vss->suspended ||
> +			    (!vss->prepared && runtime->state != SNDRV_PCM_STATE_DRAINING &&
> +			     !(runtime->state == SNDRV_PCM_STATE_PAUSED &&
> +			       virtsnd_pcm_pause_restart_enabled(vss) &&
> +			       vss->pause_restart_state == VIRTSND_PCM_PAUSE_RESTART_RELEASED)))
> +				return SNDRV_PCM_POS_XRUN;

[ ... ]

> @@ -647,86 +1658,176 @@ static int virtsnd_pcm_ack_state(struct snd_pcm_substream *substream)

[ ... ]

> +static int virtsnd_pcm_capture_staged_ack(struct virtio_pcm_substream *vss,
> +					  struct snd_pcm_runtime *runtime)
> +{
> +	struct virtio_snd_queue *queue = virtsnd_pcm_queue(vss);
> +	int rc = 0;
> +
> +	/*
> +	 * The RX callback holds the queue lock while draining completions.
> +	 * Account ALSA's read under the substream lock alone so completions later
> +	 * in that drain observe the newly released ring space.
> +	 */
> +	scoped_guard(spinlock_irqsave, &vss->lock) {
> +		snd_pcm_uframes_t appl_ptr;
> +		snd_pcm_uframes_t appl_diff;
> +		snd_pcm_uframes_t ack_frames;
> +		snd_pcm_sframes_t diff;
> +		size_t bytes;
> +		u64 outstanding;
> +		u64 next_acked_total;
> +		bool free_running;
> +
> +		if (!vss->runtime_active || vss->preparing || !runtime || !runtime->control ||
> +		    !runtime->dma_area || vss->suspended)
>  			return -EBADFD;
> -		if (vss->stopped)
> +		/* Preserve DRAINING tails only when no transfer XRUN is latched. */
> +		if (vss->stopped && vss->xfer_xrun)
>  			return -EBADFD;
> -		if (vss->xfer_xrun) {
> -			rc = -EPIPE;
> -		} else {
> -			transfer.indirect = vss->pcm_indirect;
> -			rc = snd_pcm_indirect_playback_transfer(substream,
> -							       &transfer.indirect,
> -							       virtsnd_pcm_trans_copy);
> -			if (!rc)
> -				rc = transfer.error;
> +		if (vss->xfer_xrun)
> +			return -EPIPE;
> +		if (!vss->prepared && runtime->state != SNDRV_PCM_STATE_DRAINING &&
> +		    !(runtime->state == SNDRV_PCM_STATE_PAUSED &&
> +		      virtsnd_pcm_pause_restart_enabled(vss) &&
> +		      vss->pause_restart_state == VIRTSND_PCM_PAUSE_RESTART_RELEASED))
> +			return -EBADFD;
> +		if (vss->stopped && runtime->state != SNDRV_PCM_STATE_DRAINING &&
> +		    !(runtime->state == SNDRV_PCM_STATE_PAUSED &&
> +		      virtsnd_pcm_pause_restart_enabled(vss) &&
> +		      vss->pause_restart_state == VIRTSND_PCM_PAUSE_RESTART_RELEASED))
> +			return -EBADFD;

[Severity: Medium]
Should virtsnd_pcm_cp_pointer() and virtsnd_pcm_capture_staged_ack()
exempt SNDRV_PCM_STATE_DRAINING from the !vss->prepared check?

When a capture stream enters SNDRV_PCM_STATE_DRAINING via
snd_pcm_drain() -> snd_pcm_do_drain_init() -> snd_pcm_do_stop() ->
virtsnd_pcm_trigger(SNDRV_PCM_TRIGGER_STOP), the driver sets
vss->stopped = true and vss->xfer_enabled = false while keeping
vss->prepared = true. Unlike SNDRV_PCM_STATE_PAUSED under
capture_pause_restart (which sends VIRTIO_SND_R_PCM_RELEASE and sets
vss->prepared = false), an active draining capture epoch always keeps
vss->prepared = true until teardown retires it.

If userspace subsequently calls snd_pcm_prepare() on a draining staged
capture stream, snd_pcm_do_prepare() leaves runtime->state as
SNDRV_PCM_STATE_DRAINING while invoking snd_pcm_sync_stop() ->
virtsnd_pcm_sync_stop() -> virtsnd_pcm_sync_stop_locked():

sound/virtio/virtio_pcm_ops.c:virtsnd_pcm_sync_stop_locked() {
	scoped_guard(pcm_stream_lock_irqsave, substream) {
		guard(spinlock_irqsave)(&vss->lock);
		preparing = vss->preparing;
		vss->preparing = true;
		vss->prepared = false;
		vss->xfer_enabled = false;
	}
	rc = virtsnd_pcm_release_locked(substream);
	scoped_guard(pcm_stream_lock_irqsave, substream) {
		guard(spinlock_irqsave)(&vss->lock);
		vss->preparing = preparing;
	}
	return rc;
}

Once virtsnd_pcm_sync_stop_locked() returns, vss->prepared and
vss->stopped are both false and vss->preparing is restored to false,
while runtime->state is still SNDRV_PCM_STATE_DRAINING. In the window
before virtsnd_pcm_prepare_locked() takes the PCM stream lock (or
permanently if virtsnd_pcm_prepare_locked() fails before
virtsnd_pcm_dev_prepare(), such as on -ECANCELED or a
virtsnd_pcm_dev_set_params() error, where virtsnd_pcm_prepare() resets
vss->preparing = false), concurrent SNDRV_PCM_IOCTL_SYNC_PTR or
SNDRV_PCM_IOCTL_HWSYNC calls bypass the !vss->prepared check and proceed
on the released, unprepared epoch.

By comparison, the non-staged path in virtsnd_pcm_cp_ack_locked() only
exempts SNDRV_PCM_STATE_DRAINING from vss->stopped and unconditionally
rejects !vss->prepared:

	if (!vss->prepared ||
	    (vss->stopped &&
	     runtime->state != SNDRV_PCM_STATE_DRAINING))
		return -EBADFD;

Should virtsnd_pcm_cp_pointer() and virtsnd_pcm_capture_staged_ack()
likewise require vss->prepared to be true unless the stream is in the
pause-restart released state?

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