Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider: - [Medium] ALSA: virtio: missing NOMMU copy when stopped in virtsnd_pcm_msg_complete() - [Medium] ALSA: virtio: reject short non-staged replies in virtsnd_pcm_msg_complete() - [Low] docs: kernel-parameters: document capture_staging dependency for virtio_snd - [Low] docs: kernel-parameters: use [HW,ALSA] instead of undefined [HW,SOUND] tag - [Low] docs: virtio-sound: remove prompt instructions from commit description --
commit 0d2f51e1deefbf07b12391911246c9ce81e71037 Author: Joonho Lee <[email protected]> docs: describe virtio-sound capture options This commit documents the five opt-in capture module parameters for virtio_snd in Documentation/admin-guide/kernel-parameters.txt. > docs: describe virtio-sound capture options > > Document the five opt-in capture module parameters introduced by this > series. All default to false. Describe the startup-empty, short-payload, > packed-status and pause-restart compatibility behavior alongside staging, > and require validation with the target device. > > Keep the virtio_snd entries after virtio_mmio in alphabetical order. > Include a diffstat in every mail patch. > > Assisted-by: LLM > Signed-off-by: Joonho Lee <[email protected]> [Severity: Low] This isn't a bug, but "Include a diffstat in every mail patch." in the commit description looks like a leftover patch-submission or prompt instruction. Should that sentence be removed from the changelog? > diff --git a/Documentation/admin-guide/kernel-parameters.txt > b/Documentation/admin-guide/kernel-parameters.txt > index e75344f4e0cde..de47885cd54b3 100644 > --- a/Documentation/admin-guide/kernel-parameters.txt > +++ b/Documentation/admin-guide/kernel-parameters.txt > @@ -8344,6 +8344,36 @@ Kernel parameters > > Can be used multiple times for multiple devices. > > + virtio_snd.capture_allow_empty= [HW,SOUND] [Severity: Low] This isn't a bug, but the parameter restriction list at the top of Documentation/admin-guide/kernel-parameters.txt defines ALSA: ALSA ALSA sound support is enabled. and does not define SOUND. Should these five virtio_snd.capture_* entries use [HW,ALSA] instead of [HW,SOUND]? > + Retry a bounded number of empty startup capture replies. > + Format: <bool> > + Default: false. Compatibility option for devices that > + require staged capture; validate with the target device. > + > + virtio_snd.capture_allow_short= [HW,SOUND] > + Accept non-empty, frame-aligned short capture replies. > + Format: <bool> > + Default: false. Compatibility option for devices that > + require staged capture; validate with the target device. > + > + virtio_snd.capture_packed_status= [HW,SOUND] > + Read status immediately after the returned capture data. > + Format: <bool> > + Default: false. Compatibility option for devices that > + require staged capture; validate with the target device. > + > + virtio_snd.capture_pause_restart= [HW,SOUND] > + Release and reprepare capture streams across pause. > + Format: <bool> > + Default: false. Compatibility option for devices that > + require staged capture; validate with the target device. [Severity: Low] In sound/virtio/virtio_pcm_msg.c and sound/virtio/virtio_pcm_ops.c, all four of these compatibility parameters only take effect when vss->capture_staging is enabled: sound/virtio/virtio_pcm_msg.c:virtsnd_pcm_msg_complete() { if (vss->direction == SNDRV_PCM_STREAM_CAPTURE && vss->capture_staging) { virtsnd_pcm_capture_complete(msg, written_bytes); return; } } sound/virtio/virtio_pcm_msg.c:virtsnd_pcm_capture_status() { if (msg->substream->capture_staging && virtsnd_capture_packed_status) { ... } } sound/virtio/virtio_pcm_ops.c:virtsnd_pcm_pause_restart_enabled() { return virtsnd_capture_pause_restart && vss->capture_staging && vss->direction == SNDRV_PCM_STREAM_CAPTURE; } If a user enables virtio_snd.capture_allow_empty, virtio_snd.capture_allow_short, virtio_snd.capture_packed_status, or virtio_snd.capture_pause_restart while leaving virtio_snd.capture_staging at its default value of false, the options silently have no effect. Should the documentation for these four parameters explicitly state that they require virtio_snd.capture_staging to be enabled? [Severity: Medium] Also looking at the underlying capture completion handling in sound/virtio/virtio_pcm_msg.c:virtsnd_pcm_msg_complete(), can non-staged capture on CONFIG_MMU=n builds advance vss->hw_ptr without copying the captured audio payload when vss->stopped is true (such as during SNDRV_PCM_STATE_DRAINING)? In virtsnd_pcm_msg_complete(), post-STOP capture completions are only discarded when written_bytes <= sizeof(*msg->status): if (vss->direction == SNDRV_PCM_STREAM_CAPTURE && (!vss->prepared || (vss->stopped && written_bytes <= sizeof(*msg->status)))) { msg->length = 0; vss->msg_count--; if (!vss->msg_count) wake_up_all(&vss->msg_empty); return; } When a draining stream stops via snd_pcm_drain() -> snd_pcm_do_drain_init() -> snd_pcm_do_stop() -> virtsnd_pcm_trigger(..., SNDRV_PCM_TRIGGER_STOP), vss->stopped is set to true while vss->prepared remains true. If an in-flight RX request then completes with written_bytes > sizeof(*msg->status), the #ifndef CONFIG_MMU bounce-buffer copy is skipped because of the !vss->stopped check: #ifndef CONFIG_MMU if (vss->direction == SNDRV_PCM_STREAM_CAPTURE && vss->prepared && !vss->stopped && written_bytes > sizeof(*msg->status)) { ... memcpy(runtime->dma_area + msg->data_offset, vss->owned_dma_area + msg->data_offset, data_bytes); } #endif vss->hw_ptr += completed_bytes; Since virtsnd_pcm_cp_pointer() and virtsnd_pcm_cp_ack_locked() allow reads while in SNDRV_PCM_STATE_DRAINING, doesn't advancing vss->hw_ptr and vss->capture_copied_total without copying from vss->owned_dma_area expose stale or zeroed bytes in runtime->dma_area to userspace? [Severity: Medium] In the same non-staged capture path in sound/virtio/virtio_pcm_msg.c:virtsnd_pcm_msg_complete(), what happens if a device returns a non-empty, frame-aligned short capture reply (0 < completed_bytes < msg->length) when virtio_snd.capture_staging is false? In staged mode, virtsnd_pcm_capture_complete() explicitly rejects short replies unless virtsnd_capture_allow_short is set: } else if (audio_bytes < msg->length && !virtsnd_capture_allow_short) { virtsnd_pcm_capture_xrun_locked(vss); However, in non-staged mode, virtsnd_pcm_msg_complete() only checks !completed_bytes after virtsnd_pcm_capture_status(): if (virtsnd_pcm_capture_status(msg, written_bytes, frame_bytes, &completed_bytes, &latency_bytes) || !completed_bytes) { vss->xfer_xrun = true; ... return; } Because virtsnd_pcm_msg_alloc() permanently binds each non-staged vss->msgs[i] to a fixed slice at runtime->dma_area + i * period_bytes, accepting completed_bytes < msg->length advances vss->hw_ptr by completed_bytes and resets vss->msgs[i]->length = 0 while the next descriptor vss->msgs[i + 1] writes at (i + 1) * period_bytes. Doesn't this desynchronize vss->hw_ptr from the fixed descriptor slices (exposing the unwritten tail of vss->msgs[i] to userspace on subsequent completions) and leave later descriptors with msg->length < period_bytes in virtsnd_pcm_msg_send() so they are not re-queued? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=12
