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: <87v7wi484f.wl-tiwai@suse.de>
Date: Wed, 20 Nov 2024 09:33:36 +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 Wed, 20 Nov 2024 08:31:44 +0100,
Takashi Iwai wrote:
> 
> 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)

A bit more change would be needed because it can lead to a false xrun
at draining.  At stopping, it shouldn't reach to that code path.
The revised patch is below.


Takashi


--- a/sound/usb/endpoint.c
+++ b/sound/usb/endpoint.c
@@ -403,10 +403,15 @@ static int prepare_inbound_urb(struct snd_usb_endpoint *ep,
 static void notify_xrun(struct snd_usb_endpoint *ep)
 {
 	struct snd_usb_substream *data_subs;
+	struct snd_pcm_substream *psubs;
 
 	data_subs = READ_ONCE(ep->data_subs);
-	if (data_subs && data_subs->pcm_substream)
-		snd_pcm_stop_xrun(data_subs->pcm_substream);
+	if (!data_subs)
+		return;
+	psubs = data_subs->pcm_substream;
+	if (psubs && psubs->runtime &&
+	    psubs->runtime->state == SNDRV_PCM_STATE_RUNNING)
+		snd_pcm_stop_xrun(psubs);
 }
 
 static struct snd_usb_packet_info *
@@ -562,7 +567,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;
 		}