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

[Takashi Iwai <tiwai@suse.de>]

On Thu, 23 Mar 2023 14:29:16 +0100,
Takashi Sakamoto wrote:
> 
> On Thu, Mar 23, 2023 at 07:19:45AM +0100, Takashi Iwai wrote:
> > 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.
> 
> In my opinion, extra care is required for the simple replacement of
> -EINVAL with -EPIPE, since it may not come from any operation relevant
> to hardware. In my understanding, it comes from just careless
> implementation of user space PCM application, thus it is still reasonable
> to return -EINVAL.

Yeah, we can't always replace such condition blindly.  But in the case
of indirect PCM helpers, it's the place where the bogus values have
been already set and processed, so we can't return -EINVAL, hence an
XRUN notification would be appreciate.


Takashi