Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] ALSA: virtio: virtsnd_pcm_msg_complete() drops NOMMU capture data on pause --
commit a5392b9f568d1dcfcb1d686f254cd14115ceec53 Author: Joonho Lee <[email protected]> ALSA: virtio: Keep PCM DMA buffers alive through device ownership This commit retains references to the pages backing ALSA's vmalloc buffer until its queued I/O messages are freed, uses a driver-owned bounce buffer on NOMMU systems, and clamps device-reported capture lengths to the message capacity. > diff --git a/sound/virtio/virtio_pcm_msg.c b/sound/virtio/virtio_pcm_msg.c > index 762480363b861..b7f2334bb221c 100644 > --- a/sound/virtio/virtio_pcm_msg.c > +++ b/sound/virtio/virtio_pcm_msg.c [ ... ] > @@ -363,15 +539,48 @@ static void virtsnd_pcm_msg_complete(struct > virtio_pcm_msg *msg, > * the hw_ptr value is unconditionally advanced. > */ > guard(spinlock)(&vss->lock); > +#ifndef CONFIG_MMU > + if (vss->direction == SNDRV_PCM_STREAM_CAPTURE && > + vss->xfer_enabled && written_bytes > sizeof(msg->status)) { > + struct snd_pcm_runtime *runtime = > + vss->substream ? vss->substream->runtime : NULL; > + size_t reported_bytes = written_bytes - sizeof(msg->status); > + size_t data_bytes = min_t(size_t, reported_bytes, msg->length); > + > + if (!runtime || !runtime->dma_area || !vss->owned_dma_area || > + msg->data_offset > runtime->dma_bytes || > + data_bytes > runtime->dma_bytes - msg->data_offset || > + msg->data_offset > vss->owned_dma_bytes || > + data_bytes > vss->owned_dma_bytes - msg->data_offset) > + dev_err_ratelimited(&vss->snd->vdev->dev, > + "SID %u: invalid capture buffer for > I/O message\n", > + vss->sid); > + else > + memcpy(runtime->dma_area + msg->data_offset, > + vss->owned_dma_area + msg->data_offset, > data_bytes); > + } > +#endif > + if (vss->direction == SNDRV_PCM_STREAM_CAPTURE && > + written_bytes > sizeof(msg->status) && > + written_bytes - sizeof(msg->status) > msg->length) > + dev_err_ratelimited(&vss->snd->vdev->dev, > + "SID %u: capture length exceeds I/O message > capacity\n", > + vss->sid); > /* > * If the capture substream returned an incorrect status, then just > - * increase the hw_ptr by the message size. > + * increase the hw_ptr by the message size. Clamp valid capture lengths > to > + * the actual SG payload capacity before accounting them. > */ > if (vss->direction == SNDRV_PCM_STREAM_PLAYBACK || > - written_bytes <= sizeof(msg->status)) > + written_bytes <= sizeof(msg->status)) { > vss->hw_ptr += msg->length; > - else > - vss->hw_ptr += written_bytes - sizeof(msg->status); > + } else { > + size_t data_bytes = min_t(size_t, > + written_bytes - sizeof(msg->status), > + msg->length); > + > + vss->hw_ptr += data_bytes; > + } [Severity: High] Can gating the NOMMU capture bounce copy on vss->xfer_enabled drop captured audio data when a stream is paused? In sound/virtio/virtio_pcm_ops.c, virtsnd_pcm_trigger() handles SNDRV_PCM_TRIGGER_PAUSE_PUSH by clearing vss->xfer_enabled while leaving vss->stopped false before synchronously sending VIRTIO_SND_R_PCM_STOP: case SNDRV_PCM_TRIGGER_STOP: case SNDRV_PCM_TRIGGER_PAUSE_PUSH: scoped_guard(spinlock_irqsave, &vss->lock) { if (command != SNDRV_PCM_TRIGGER_PAUSE_PUSH) vss->stopped = true; vss->xfer_enabled = false; } msg = virtsnd_pcm_ctl_msg_alloc(vss, VIRTIO_SND_R_PCM_STOP, GFP_KERNEL); if (!msg) return -ENOMEM; return virtsnd_ctl_msg_send_sync(snd, msg); If an in-flight capture I/O message completes while vss->xfer_enabled is false during pause, virtsnd_pcm_msg_complete() skips the memcpy() from vss->owned_dma_area to runtime->dma_area, but still advances vss->hw_ptr by data_bytes and resets msg->length to 0. When the stream is later resumed with SNDRV_PCM_TRIGGER_PAUSE_RELEASE, will userspace read stale or zeroed bytes from runtime->dma_area for that completed period? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
