Re: [PATCH v1 1/5] ALSA: virtio: Notify queued PCM messages after send errors

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] alsa: virtio: state desync on msg send failure
--

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

ALSA: virtio: Notify queued PCM messages after send errors

virtsnd_pcm_msg_send() advances a message length before adding it to
the virtqueue. If a later add fails, the error path returns without
restoring that length or notifying the device about earlier messages
added by the same call. The device can therefore leave successfully
queued messages unprocessed, while the failed message appears
already submitted.

> diff --git a/sound/virtio/virtio_pcm_msg.c b/sound/virtio/virtio_pcm_msg.c
> index 72103f6067169..5371f4985fd06 100644
> --- a/sound/virtio/virtio_pcm_msg.c
> +++ b/sound/virtio/virtio_pcm_msg.c
> @@ -260,6 +260,18 @@ int virtsnd_pcm_msg_send(struct virtio_pcm_substream *vss, unsigned long offset,
>  				dev_err(&vdev->dev,
>  					"SID %u: failed to send I/O message\n",
>  					vss->sid);
> +				msg->length -= n;

[Severity: High]
Does this length rollback desynchronize the virtio message state from the ALSA
indirect layer?

When virtsnd_pcm_msg_send() fails and returns an error, its caller,
virtsnd_pcm_trans_copy(), ignores the return value:

sound/virtio/virtio_pcm_ops.c:virtsnd_pcm_trans_copy() {
	struct virtio_pcm_substream *vss = snd_pcm_substream_chip(substream);

	virtsnd_pcm_msg_send(vss, rec->sw_data, bytes);
}

Because virtsnd_pcm_trans_copy() returns void, the ALSA indirect layer 
(snd_pcm_indirect_playback_transfer()) assumes success and unconditionally 
advances its software offset pointers. 

Since the ALSA pointer advanced but the virtio message length rolled back, the
two states become out of sync. On the next wrap-around, will the skipped
message artificially reach period_bytes with old/new mixed data and be
submitted to the virtqueue out of order?

Should the driver set substream->runtime->stop_operating = true inside
virtsnd_pcm_msg_send() to properly abort the stream when an I/O message
fails?

> +
> +				/*
> +				 * Do not leave earlier messages queued without notifying
> +				 * the device when a later message could not be added.
> +				 */
> +				if (msg_count != vss->msg_count &&
> +				    !(vss->features &
> +				      (1U << VIRTIO_SND_PCM_F_MSG_POLLING)) &&
> +				    virtqueue_kick_prepare(vqueue))
> +					virtqueue_notify(vqueue);
> +
>  				return rc;
>  			}
>

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