Re: [PATCH 3/5] ALSA: usx2y: us144mkii: only touch the buffer of a running PCM stream

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] ALSA: usx2y: us144mkii: pointer desync when streams start
  sequentially
--

commit e23f2c6273b24e6b8b4a5dbee2dc9fa0b6647cd7
Author: Alexandre Becholey <alex@becholey.ch>

ALSA: usx2y: us144mkii: only touch the buffer of a running PCM stream

This commit gates DMA buffer accesses in tascam_capture_work_handler() and
playback_urb_complete() on snd_pcm_running() so that starting one PCM
direction does not read or write the other substream's buffer before it is
running.

> diff --git a/sound/usb/usx2y/us144mkii_capture.c b/sound/usb/usx2y/us144mkii_capture.c
> index 973bed60bfcd1..9c142a85e8b21 100644
> --- a/sound/usb/usx2y/us144mkii_capture.c
> +++ b/sound/usb/usx2y/us144mkii_capture.c
> @@ -231,7 +231,13 @@ void tascam_capture_work_handler(struct work_struct *work)
>  						  routed_block);
>  
>  		scoped_guard(spinlock_irqsave, &tascam->lock) {
> -			if (atomic_read(&tascam->capture_active)) {
> +			/*
> +			 * capture_active is also set when only the playback
> +			 * stream was started; the capture buffer is only valid
> +			 * while the capture stream itself is running.
> +			 */
> +			if (atomic_read(&tascam->capture_active) &&
> +			    snd_pcm_running(substream)) {
>  				int f;
>  
>  				for (f = 0; f < FRAMES_PER_DECODE_BLOCK; ++f) {
> diff --git a/sound/usb/usx2y/us144mkii_playback.c b/sound/usb/usx2y/us144mkii_playback.c
> index 0df95dbf585db..e6266488cf325 100644
> --- a/sound/usb/usx2y/us144mkii_playback.c
> +++ b/sound/usb/usx2y/us144mkii_playback.c
> @@ -166,6 +166,7 @@ void playback_urb_complete(struct urb *urb)
>  	size_t total_bytes_for_urb = 0;
>  	snd_pcm_uframes_t offset_frames;
>  	snd_pcm_uframes_t frames_to_copy;
> +	bool running;
>  	int ret, i;
>  
>  	if (urb->status) {
> @@ -207,13 +208,25 @@ void playback_urb_complete(struct urb *urb)
>  		}
>  		urb->transfer_buffer_length = total_bytes_for_urb;
>  
> -		offset_frames = tascam->driver_playback_pos;
> -		frames_to_copy = bytes_to_frames(runtime, total_bytes_for_urb);
> -		tascam->driver_playback_pos =
> -			(offset_frames + frames_to_copy) % runtime->buffer_size;
> +		/*
> +		 * The URBs also run when only the capture stream was started;
> +		 * the playback buffer is only valid while the playback stream
> +		 * itself is running.  Send silence otherwise.
> +		 */
> +		running = snd_pcm_running(substream);
> +		if (running) {
> +			offset_frames = tascam->driver_playback_pos;
> +			frames_to_copy = bytes_to_frames(runtime,
> +							 total_bytes_for_urb);
> +			tascam->driver_playback_pos =
> +				(offset_frames + frames_to_copy) %
> +				runtime->buffer_size;
> +		}
>  	}

[Severity: Medium]
Does holding driver_playback_pos and driver_capture_pos at 0 while the
substream is not running desynchronize them from playback_frames_consumed
and capture_frames_processed?

feedback_urb_complete() still advances both frame counters unconditionally
while the URBs are active:

sound/usb/usx2y/us144mkii_playback.c:feedback_urb_complete() {
    ...
		if (total_frames_in_urb > 0) {
			tascam->playback_frames_consumed += total_frames_in_urb;
			if (atomic_read(&tascam->capture_active))
				tascam->capture_frames_processed +=
					total_frames_in_urb;
		}
    ...
}

When both substreams are prepared and one is started before the other,
feedback_urb_complete() increments the prepared substream's frame counter
while its driver_*_pos stays at 0.  Starting the second substream does not
call .prepare() again, so tascam_playback_pointer() or
tascam_capture_pointer() reports a hardware pointer ahead of where the
driver reads or writes in runtime->dma_area, causing misaligned audio or
xruns.  Should feedback_urb_complete() also gate each stream's counter and
period updates on snd_pcm_running()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114172806.88710-1-alex@becholey.ch?part=3