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] [day] [month] [year] [list]

Message-ID: <57a3a34c-261d-44ec-b96e-05fecb0b7cab@gmail.com>
Date: Wed, 20 Nov 2024 23:02:45 +0200
From: Leonard Crestez <cdleonard@...il.com>
To: Takashi Iwai <tiwai@...e.de>
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 11/20/24 10:33, Takashi Iwai wrote:
> 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.

> --- 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;
>   		}

This makes more sense than what I hacked to together and seems to work 
well in initial testing.

I'll report back if issues are found.

--
Thanks,
Leonard