On Mon, 20 Mar 2023 13:04:22 +0100, John Keeping wrote: > > On Sun, Mar 19, 2023 at 10:15:55AM +0100, Takashi Iwai wrote: > > On Sun, 19 Mar 2023 08:57:03 +0100, > > Takashi Iwai wrote: > > > > > > On Sun, 19 Mar 2023 04:28:53 +0100, > > > Takashi Sakamoto wrote: > > > > > > > > Hi, > > > > > > > > On Sat, Mar 18, 2023 at 09:20:05AM +0900, Takashi Sakamoto wrote: > > > > > On Fri, Mar 17, 2023 at 07:51:27PM +0000, John Keeping wrote: > > > > > > snd_usb_queue_pending_output_urbs() may be called from > > > > > > snd_pcm_ops::ack() which means the PCM stream is locked. > > > > > > > > > > > > For the normal case where the call back into the PCM core is via > > > > > > prepare_output_urb() the "_under_stream_lock" variant of > > > > > > snd_pcm_period_elapsed() is called, but when an error occurs and the > > > > > > stream is stopped as XRUN then snd_pcm_xrun() tries to recursively lock > > > > > > the stream which results in deadlock. > > > > > > > > > > > > Follow the example of snd_pcm_period_elapsed() by adding > > > > > > snd_pcm_xrun_under_stream_lock() and use this when the PCM substream > > > > > > lock is already held. > > > > > > > > > > > > Signed-off-by: John Keeping <john@xxxxxxxxxxxx> > > > > > > --- > > > > > > include/sound/pcm.h | 1 + > > > > > > sound/core/pcm_native.c | 28 ++++++++++++++++++++++++---- > > > > > > sound/usb/endpoint.c | 18 +++++++++++------- > > > > > > 3 files changed, 36 insertions(+), 11 deletions(-) > > > > > > > > > > The name of added kernel API implies me that you refer to existent > > > > > 'snd_pcm_period_elapsed_under_stream_lock()' which I added to Linux > > > > > v5.14. > > > > > > > > > > In my opinion, unlike the version of period elapsed API, the version of > > > > > XRUN API seems not to be necessarily required to ALSA PCM core, since PCM > > > > > device drivers can implement .pointer callback in the part of PCM operation. > > > > > When the callback returns SNDRV_PCM_POS_XRUN, ALSA PCM application get > > > > > occurence of XRUN as a result of any operation relevant to hwptr movement > > > > > (e.g. SNDRV_PCM_IOCTL_HWSYNC). > > > > > > > > > > Therefore I think it possible to fix the issue without the proposed > > > > > kernel API. I can assume some scenario: > > > > > > > > > > 1. Failure at tasklet for URB completion > > > > > > > > > > It is softIRQ context. The stream lock is not acquired. It doesn't > > > > > matter to call current XRUN API. > > > > > > > > > > 2. Failure at PCM operation called by ALSA PCM application > > > > > > > > > > It is process context. The stream lock is acquired before calling driver > > > > > code. When detecting any type of failure, driver code stores the state. > > > > > Then .pointer callback should return SNDRV_PCM_POS_XRUNrefering to > > > > > the state. > > > > > > > > Although being inexperienced to hack driver for USB audio device class, > > > > I attempt to post the patch to fix the issue of recursive stream lock. > > > > I apologies in advance since the patch is not tested yet... > > > > > > > > The 'in_xrun' member is newly added to 'struct snd_usb_substream'. When > > > > detecting any failure, false is assigned to the member. The assignment > > > > is expected to be done in both softIRQ context, and process context with > > > > stream lock, thus no need to take care of cocurrent access (e.g. by usage > > > > of WRITE_ONCE/READ_ONCE). > > > > > > > > Typical ALSA PCM application periodically calls PCM operation which calls > > > > .pointer in driver code. As I described, returning SNDRV_PCM_POS_XRUN > > > > takes ALSA PCM core to handle XRUN state of PCM substream in the timing. > > > > > > > > The negative point of the patch is the delay of XRUN notification to user > > > > space application. In the point, I think the new kernel API introduced by > > > > your patch has advantage. > > > > > > > > The in_xrun member can be replaced with a kind of EP_STATE_ > > > > enumerations; i.e. EP_STATE_XRUN. In the case, we need some care so that > > > > the state should be referred from pcm.c. > > > > > > Thanks for the patch. That would work, but the shortcoming side of > > > this implementation is that it misses stopping / reporting the error > > > immediately but waiting for the next pointer update. > > > > > > It might be simpler if we perform the xrun handling in the caller > > > side, i.e. a change like below: > > > > > > --- 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; > > > } > > > } > > > > > > ... and let the caller returning -EPIPE for XRUN: > > > > and that misses the XRUN in the case of non-stream-lock. > > A revised version is below. > > Yes, it looks like this also solves the problem. If you roll this into > a proper patch feel free to add: > > Tested-by: John Keeping <john@xxxxxxxxxxxx> Thanks, then I'll submit a proper patch. Takashi