Thank you for your contribution! Sashiko AI review found 1 potential issue to consider: - [Medium] ALSA: virtio: partial RX on stop latches XRUN in virtsnd_pcm_msg_complete() --
commit 8701e733a8297edcfeb307fc6817693d437f58dc Author: Joonho Lee <[email protected]> ALSA: virtio: bound PCM transfers and retain DMA buffers This commit validates PCM buffer geometry, ACK transfer bounds, and RX completions while retaining backing DMA pages until virtqueue messages are freed. > diff --git a/sound/virtio/virtio_pcm_msg.c b/sound/virtio/virtio_pcm_msg.c > index 59ad99e21e899..c728e8fafb37f 100644 > --- a/sound/virtio/virtio_pcm_msg.c > +++ b/sound/virtio/virtio_pcm_msg.c [ ... ] > @@ -356,42 +536,81 @@ static void virtsnd_pcm_msg_complete(struct > virtio_pcm_msg *msg, > size_t written_bytes) > { > struct virtio_pcm_substream *vss = msg->substream; > + bool capture_invalid = false; > > /* > - * hw_ptr always indicates the buffer position of the first I/O message > - * in the virtqueue. Therefore, on each completion of an I/O message, > - * the hw_ptr value is unconditionally advanced. > + * Completed requests can return captured data after STOP while the PCM > + * buffer remains prepared. Once preparation ends, completions only > retire > + * queue ownership. > */ > guard(spinlock)(&vss->lock); > - /* > - * If the capture substream returned an incorrect status, then just > - * increase the hw_ptr by the message size. > - */ > - if (vss->direction == SNDRV_PCM_STREAM_PLAYBACK || > - written_bytes <= sizeof(msg->status)) > - vss->hw_ptr += msg->length; > - else > - vss->hw_ptr += written_bytes - sizeof(msg->status); > + if (vss->direction == SNDRV_PCM_STREAM_CAPTURE && > + (!vss->prepared || vss->xfer_xrun || > + (vss->stopped && written_bytes <= sizeof(msg->status)))) { > + msg->length = 0; > + vss->msg_count--; > + if (!vss->msg_count) > + wake_up_all(&vss->msg_empty); > + return; > + } > > - if (vss->hw_ptr >= vss->buffer_bytes) > - vss->hw_ptr -= vss->buffer_bytes; > + /* Only the next complete ring slice can extend captured data. */ > + if (vss->direction == SNDRV_PCM_STREAM_CAPTURE && > + (msg->data_offset != vss->hw_ptr || > + written_bytes < sizeof(msg->status) || > + le32_to_cpu(msg->status.status) != VIRTIO_SND_S_OK || > + written_bytes - sizeof(msg->status) != msg->length)) { > + dev_err_ratelimited(&vss->snd->vdev->dev, > + "SID %u: invalid capture position, status > or length\n", > + vss->sid); > + vss->xfer_xrun = true; > + capture_invalid = true; > + } [Severity: Medium] Does this latch vss->xfer_xrun when the device completes a partially filled RX buffer upon VIRTIO_SND_R_PCM_STOP? Under the VirtIO Sound specification (section 5.14.6.8.2.1), the last RX buffer returned when a capture stream stops may contain fewer than period_bytes of PCM frames. During SNDRV_PCM_IOCTL_DRAIN, virtsnd_pcm_trigger(SNDRV_PCM_TRIGGER_STOP) sets vss->stopped = true and sends VIRTIO_SND_R_PCM_STOP while leaving the substream in SNDRV_PCM_STATE_DRAINING. A partial RX completion has written_bytes > sizeof(msg->status) and written_bytes - sizeof(msg->status) < msg->length, so it bypasses the early return and sets vss->xfer_xrun = true. That causes virtsnd_pcm_cp_ack() to fail with -EBADFD and virtsnd_pcm_cp_pointer() to report SNDRV_PCM_POS_XRUN, preventing userspace from draining full periods that had already completed before STOP. Similarly, SNDRV_PCM_TRIGGER_PAUSE_PUSH sends VIRTIO_SND_R_PCM_STOP without setting vss->stopped, so any empty or partial in-flight RX buffer returned on pause also latches vss->xfer_xrun. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=9
