linux-kernel - Re: [RFC] ALSA: usb-audio: Fix missing xrun report in lowlatency mode

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <874j425pjz.wl-tiwai@suse.de>
Date: Wed, 20 Nov 2024 08:31:44 +0100
From: Takashi Iwai <tiwai@...e.de>
To: Leonard Crestez <cdleonard@...il.com>
Cc: Takashi Iwai <tiwai@...e.com>,
	linux-sound@...r.kernel.org,
	Jaroslav Kysela <perex@...ex.cz>,
	linux-kernel@...r.kernel.org
Subject: Re: [RFC] ALSA: usb-audio: Fix missing xrun report in lowlatency mode

On Tue, 19 Nov 2024 22:54:19 +0100,
Leonard Crestez wrote:
> 
> Hello,
> 
> I’m investigating an issue where USB Audio does not properly send
> XRUN notifications.
> 
> The issue can be reproduced with aplay: enable xrun_debug, aplay -D
> plughw:0 and CTRL-Z - no XRUN message is seen
> 
> Disabling lowlatency_playback via modprobe parameter does make this
> issue go away - XRUNs are reported correctly without any changes.
> 
> 
> After a lot of tracing the following seems to be happening:
> 
> - prepare_playback_urb find avail=48, meaning 48 bytes still to-be-played
> - snd_usb_endpoint_next_packet_size decides that 48 is too little and
> returns -EAGAIN. Specifically -EAGAIN is returned from
> next_packet_size
> - The return value of prepare_playback_urb is propagated through
> prepare_outbound_urb back to snd_usb_queue_pending_output_urbs
> - snd_usb_queue_pending_output_urbs receives -EAGAIN from
> prepare_outbound_urb
> - since err is -EAGAIN the ctx is pushed back to the ready list and
> transmission is aborted but notify_xrun is skipped
> - no more playback?
> 
> It is possible to make XRUNs happen by caling notify_xrun even on
> -EAGAIN, diff looks like this:
> 
> diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c
> index 568099467dbb..da64ee0cf60a 100644
> --- a/sound/usb/endpoint.c
> +++ b/sound/usb/endpoint.c
> @@ -495,10 +495,11 @@ int snd_usb_queue_pending_output_urbs(struct
> snd_usb_endpoint *ep,
>                         break;
>                 if (err < 0) {
>                         /* push back to ready list again for -EAGAIN */
>                         if (err == -EAGAIN) {
>                                 push_back_to_ready_list(ep, ctx);
> +                               notify_xrun(ep);
>                                 break;
>                         }
> 
>                         if (!in_stream_lock)
>                                 notify_xrun(ep);
> 
> 
> This mail was not formatted as proper patch because this seems very
> likely incorrect, it undoes an explicit check. What would a correct
> solution look like?

The -EAGAIN there itself doesn't mean the crucial xrun yet.  There may
be still pending URBS to be processed.  The real XRUN happens only
when there is no URBs pending, hence nothing will be taken further --
at least for low-latency operation.  (In the case of implicit feedback
mode, it can be driven by the feedback from the capture stream, and
the empty URB check might be wrong.)

Could you check the change below?  (totally untested)


thanks,

Takashi

--- a/sound/usb/endpoint.c
+++ b/sound/usb/endpoint.c
@@ -562,7 +562,10 @@ static void snd_complete_urb(struct urb *urb)
 			push_back_to_ready_list(ep, ctx);
 			clear_bit(ctx->index, &ep->active_mask);
 			snd_usb_queue_pending_output_urbs(ep, false);
-			atomic_dec(&ep->submitted_urbs); /* decrement at last */
+			/* decrement at last, and check xrun */
+			if (atomic_dec_and_test(&ep->submitted_urbs) &&
+			    !snd_usb_endpoint_implicit_feedback_sink(ep))
+				notify_xrun(ep);
 			return;
 		}