Re: [PATCH] ALSA: line6: Bound capture copies by the PCM buffer size

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] ALSA: line6: clamping frames to buffer_size in line6_capture_copy() stalls PCM
--

commit dece7d69941f7080a104841df1e03f3fae6b7577
Author: Sinan Turunc <enscrpt@pm.me>

ALSA: line6: Bound capture copies by the PCM buffer size

This patch clamps the number of captured frames in line6_capture_copy() to
runtime->buffer_size so that oversized incoming USB packets do not overflow
the single ring-buffer wrap calculation.

> diff --git a/sound/usb/line6/capture.c b/sound/usb/line6/capture.c
> index 17f49f567efbf..8b6a0dd2ddde2 100644
> --- a/sound/usb/line6/capture.c
> +++ b/sound/usb/line6/capture.c
> @@ -94,6 +94,17 @@ void line6_capture_copy(struct snd_line6_pcm *line6pcm, char *fbuf, int fsize)
>  	if (runtime == NULL)
>  		return;
>  
> +	/*
> +	 * A packet can carry up to max_packet_size_in, which the device
> +	 * chooses, while the ring buffer can be as small as one 64-byte
> +	 * period. The single wrap below is only valid for
> +	 * frames <= buffer_size, so keep what fits.
> +	 */
> +	if (unlikely(frames > runtime->buffer_size)) {
> +		frames = runtime->buffer_size;
> +		fsize = frames * bytes_per_frame;
> +	}

[Severity: High]
Does clamping frames to runtime->buffer_size here cause the capture stream
to stall whenever an incoming packet exceeds the buffer size (for example,
when a user configures a 1-period 64-byte capture buffer of 11 or 16 frames
and receives a regular 44- or 48-frame 1 ms packet or a larger USB/IP
packet)?

At the end of line6_capture_copy(), pos_done is updated with:

sound/usb/line6/capture.c:line6_capture_copy() {
	...
	line6pcm->in.pos_done += frames;
	if (line6pcm->in.pos_done >= runtime->buffer_size)
		line6pcm->in.pos_done -= runtime->buffer_size;
}

When frames is clamped to runtime->buffer_size, adding and immediately
subtracting runtime->buffer_size leaves line6pcm->in.pos_done unchanged on
every packet.

When audio_in_callback() then calls snd_pcm_period_elapsed(),
snd_pcm_update_hw_ptr0() reads the same position via snd_line6_pointer().
In the interrupt path, snd_pcm_update_hw_ptr0() only treats an unchanged
pointer as a full-buffer wrap when hdelta exceeds
runtime->hw_ptr_buffer_jiffies / 2 + 1:

sound/core/pcm_lib.c:snd_pcm_update_hw_ptr0() {
	...
	if (in_interrupt) {
		/* we know that one period was processed */
		/* delta = "expected next hw_ptr" for in_interrupt != 0 */
		delta = runtime->hw_ptr_interrupt + runtime->period_size;
		if (delta > new_hw_ptr) {
			/* check for double acknowledged interrupts */
			hdelta = curr_jiffies - runtime->hw_ptr_jiffies;
			if (hdelta > runtime->hw_ptr_buffer_jiffies/2 + 1) {
				hw_base += runtime->buffer_size;
				...
			}
		}
	}
	/* new_hw_ptr might be lower than old_hw_ptr in case of */
	/* pointer crosses the end of the ring buffer */
	if (runtime->status->hw_ptr == new_hw_ptr) {
		runtime->hw_ptr_jiffies = curr_jiffies;
		update_audio_tstamp(substream, &curr_tstamp, &audio_tstamp);
		return 0;
	}
}

Because isochronous URBs complete every 1 ms (or 125 us), hdelta is at most
1 jiffy, so snd_pcm_update_hw_ptr0() takes the
runtime->status->hw_ptr == new_hw_ptr return path and refreshes
runtime->hw_ptr_jiffies on every URB without ever advancing hw_ptr or waking
user space.

Also, fsize is not read again after frames is computed at the top of
line6_capture_copy(), so is the assignment to fsize here unused?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103120723.328-1-enscrpt@pm.me?part=1