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

[Takashi Iwai <tiwai@suse.de>]

On Tue, 21 Mar 2023 05:02:58 +0100,
Takashi Sakamoto wrote:
> 
> 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/'

Corrected at applying.

> > 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.

I find rather that's an extra bonus :)  It allows the existing code to
give a more proper error handling.  For example, the indirect PCM
helper returned -EINVAL, but this can be switched to -EPIPE for
stopping the stream.

I'm going to submit the patch together with the documentation
updates.


thanks,

Takashi