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
