Re: [PATCH v13 12/12] docs: describe virtio-sound capture options

[sashiko-bot@kernel.org]

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 <mynameisjune111@gmail.com>

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 <mynameisjune111@gmail.com>

[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/20261103080821.16983-1-mynameisjune111@gmail.com?part=12