Message-ID: <87zg87jy4t.wl-tiwai@suse.de>
Date:   Mon, 20 Mar 2023 15:28:02 +0100
From:   Takashi Iwai <tiwai@...e.de>
To:     John Keeping <john@...anate.com>
Cc:     Takashi Sakamoto <o-takashi@...amocchi.jp>,
        Takashi Iwai <tiwai@...e.com>,
        "moderated list:SOUND" <alsa-devel@...a-project.org>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] ALSA: usb-audio: Fix recursive locking on XRUN

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@...anate.com>
> > > > > > ---
> > > > > >  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@...anate.com>

Thanks, then I'll submit a proper patch.


Takashi

Powered by blists - more mailing lists