Re: [PATCH] ALSA: usb-audio: Fix recursive locking at XRUN during syncing

[Takashi Sakamoto <o-takashi@sakamocchi.jp>]

Hi,

On Mon, Mar 20, 2023 at 03:28:38PM +0100, Takashi Iwai wrote:
> The recent support of low latency playback in USB-audio driver made
> the snd_usb_queue_pending_output_urbs() function to be called via PCM
> ack ops.  In the new code path, the function is performed alread in

's/alread/already/' or slang.

> the PCM stream lock.  The problem is that, when an XRUN is detected,
> the  function calls snd_pcm_xrun() to notify, but snd_pcm_xrun() is

's/the  function/the function/'

> supposed to be called only outside the stream lock.  As a result, it
> leads to a deadlock of PCM stream locking.
> 
> For avoiding such a recursive locking, this patch adds an additional
> check to the code paths in PCM core that call the ack callback; now it
> checks the error code from the callback, and if it's -EPIPE, the XRUN
> is handled in the PCM core side gracefully.  Along with it, the
> USB-audio driver code is changed to follow that, i.e. -EPIPE is
> returned instead of the explicit snd_pcm_xrun() call when the function
> is performed already in the stream lock.
 
Practically, the implementation of 'pcm_hw' in alsa-lib never evaluates
the return value (see 'snd_pcm_hw_mmap_commit()' and the others). I guess
that it is inconvenient for the low-latency mode of USB Audio device class
driver for the case of failure.

My additional concern is PCM indirect layer, since typically the layer
is used by drivers with SNDRV_PCM_INFO_SYNC_APPLPTR. But as long as I
read, the change does not matter to them.

> Fixes: d5f871f89e21 ("ALSA: usb-audio: Improved lowlatency playback support")
> Reported-and-tested-by: John Keeping <john@metanate.com>
> Link: https://lore.kernel.org/r/20230317195128.3911155-1-john@metanate.com
> Cc: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>  sound/core/pcm_lib.c |  2 ++
>  sound/usb/endpoint.c | 22 ++++++++++++++--------
>  sound/usb/endpoint.h |  4 ++--
>  sound/usb/pcm.c      |  2 +-
>  4 files changed, 19 insertions(+), 11 deletions(-)
 
Anyway, John Keeping reports the change works well to solve the issue. I
have no objection to it.

Reviewed-by; Takashi Sakamoto <o-takashi@sakamocchi.jp>

It is also convenient to IEC 61883-1/6 packet streaming engine for audio
and music unit in IEEE 1394 bus. I will post patches enough later for it.

> diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
> index 8b6aeb8a78f7..02fd65993e7e 100644
> --- a/sound/core/pcm_lib.c
> +++ b/sound/core/pcm_lib.c
> @@ -2155,6 +2155,8 @@ int pcm_lib_apply_appl_ptr(struct snd_pcm_substream *substream,
>  		ret = substream->ops->ack(substream);
>  		if (ret < 0) {
>  			runtime->control->appl_ptr = old_appl_ptr;
> +			if (ret == -EPIPE)
> +				__snd_pcm_xrun(substream);
>  			return ret;
>  		}
>  	}
> diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c
> index 419302e2057e..647fa054d8b1 100644
> --- a/sound/usb/endpoint.c
> +++ b/sound/usb/endpoint.c
> @@ -455,8 +455,8 @@ static void push_back_to_ready_list(struct snd_usb_endpoint *ep,
>   * This function is used both for implicit feedback endpoints and in low-
>   * latency playback mode.
>   */
> -void snd_usb_queue_pending_output_urbs(struct snd_usb_endpoint *ep,
> -				       bool in_stream_lock)
> +int snd_usb_queue_pending_output_urbs(struct snd_usb_endpoint *ep,
> +				      bool in_stream_lock)
>  {
>  	bool implicit_fb = snd_usb_endpoint_implicit_feedback_sink(ep);
>  
> @@ -480,7 +480,7 @@ void snd_usb_queue_pending_output_urbs(struct snd_usb_endpoint *ep,
>  		spin_unlock_irqrestore(&ep->lock, flags);
>  
>  		if (ctx == NULL)
> -			return;
> +			break;
>  
>  		/* copy over the length information */
>  		if (implicit_fb) {
> @@ -495,11 +495,14 @@ void 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)
> +			if (err == -EAGAIN) {
>  				push_back_to_ready_list(ep, ctx);
> -			else
> +				break;
> +			}
> +
> +			if (!in_stream_lock)
>  				notify_xrun(ep);
> -			return;
> +			return -EPIPE;
>  		}
>  
>  		err = usb_submit_urb(ctx->urb, GFP_ATOMIC);
> @@ -507,13 +510,16 @@ void snd_usb_queue_pending_output_urbs(struct snd_usb_endpoint *ep,
>  			usb_audio_err(ep->chip,
>  				      "Unable to submit urb #%d: %d at %s\n",
>  				      ctx->index, err, __func__);
> -			notify_xrun(ep);
> -			return;
> +			if (!in_stream_lock)
> +				notify_xrun(ep);
> +			return -EPIPE;
>  		}
>  
>  		set_bit(ctx->index, &ep->active_mask);
>  		atomic_inc(&ep->submitted_urbs);
>  	}
> +
> +	return 0;
>  }
>  
>  /*
> diff --git a/sound/usb/endpoint.h b/sound/usb/endpoint.h
> index 924f4351588c..c09f68ce08b1 100644
> --- a/sound/usb/endpoint.h
> +++ b/sound/usb/endpoint.h
> @@ -52,7 +52,7 @@ int snd_usb_endpoint_implicit_feedback_sink(struct snd_usb_endpoint *ep);
>  int snd_usb_endpoint_next_packet_size(struct snd_usb_endpoint *ep,
>  				      struct snd_urb_ctx *ctx, int idx,
>  				      unsigned int avail);
> -void snd_usb_queue_pending_output_urbs(struct snd_usb_endpoint *ep,
> -				       bool in_stream_lock);
> +int snd_usb_queue_pending_output_urbs(struct snd_usb_endpoint *ep,
> +				      bool in_stream_lock);
>  
>  #endif /* __USBAUDIO_ENDPOINT_H */
> diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
> index d959da7a1afb..eec5232f9fb2 100644
> --- a/sound/usb/pcm.c
> +++ b/sound/usb/pcm.c
> @@ -1639,7 +1639,7 @@ static int snd_usb_pcm_playback_ack(struct snd_pcm_substream *substream)
>  	 * outputs here
>  	 */
>  	if (!ep->active_mask)
> -		snd_usb_queue_pending_output_urbs(ep, true);
> +		return snd_usb_queue_pending_output_urbs(ep, true);
>  	return 0;
>  }
>  
> -- 
> 2.35.3


Thanks

Takashi Sakamoto