Date:   Sun, 19 Mar 2023 08:57:03 +0100
Message-ID: <87sfe1mawg.wl-tiwai@suse.de>
From:   Takashi Iwai <tiwai@suse.de>
To:     Takashi Sakamoto <o-takashi@sakamocchi.jp>
Cc:     John Keeping <john@metanate.com>, Takashi Iwai <tiwai@suse.com>,
        "moderated list:SOUND" <alsa-devel@alsa-project.org>,
        open list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] ALSA: usb-audio: Fix recursive locking on XRUN
In-Reply-To: <20230319032853.GA99783@workstation>
References: <20230317195128.3911155-1-john@metanate.com>
        <20230318002005.GA84781@workstation>
        <20230319032853.GA99783@workstation>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) Emacs/27.2 Mule/6.0
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

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@metanate.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:

--- a/sound/usb/endpoint.c
+++ b/sound/usb/endpoint.c
@@ -455,8 +455,8 @@ static void push_back_to_ready_list(struct snd_usb_endpoint *ep,
  * This function is used both for implicit feedback endpoints and in low-
  * latency playback mode.
  */
-void snd_usb_queue_pending_output_urbs(struct snd_usb_endpoint *ep,
-				       bool in_stream_lock)
+int snd_usb_queue_pending_output_urbs(struct snd_usb_endpoint *ep,
+				      bool in_stream_lock)
 {
 	bool implicit_fb = snd_usb_endpoint_implicit_feedback_sink(ep);
 
@@ -480,7 +480,7 @@ void snd_usb_queue_pending_output_urbs(struct snd_usb_endpoint *ep,
 		spin_unlock_irqrestore(&ep->lock, flags);
 
 		if (ctx == NULL)
-			return;
+			return 0;
 
 		/* copy over the length information */
 		if (implicit_fb) {
@@ -495,11 +495,11 @@ void snd_usb_queue_pending_output_urbs(struct snd_usb_endpoint *ep,
 			break;
 		if (err < 0) {
 			/* push back to ready list again for -EAGAIN */
-			if (err == -EAGAIN)
+			if (err == -EAGAIN) {
 				push_back_to_ready_list(ep, ctx);
-			else
-				notify_xrun(ep);
-			return;
+				return 0;
+			}
+			return -EPIPE;
 		}
 
 		err = usb_submit_urb(ctx->urb, GFP_ATOMIC);
@@ -507,8 +507,7 @@ void snd_usb_queue_pending_output_urbs(struct snd_usb_endpoint *ep,
 			usb_audio_err(ep->chip,
 				      "Unable to submit urb #%d: %d at %s\n",
 				      ctx->index, err, __func__);
-			notify_xrun(ep);
-			return;
+			return -EPIPE;
 		}
 
 		set_bit(ctx->index, &ep->active_mask);
--- a/sound/usb/endpoint.h
+++ b/sound/usb/endpoint.h
@@ -52,7 +52,7 @@ int snd_usb_endpoint_implicit_feedback_sink(struct snd_usb_endpoint *ep);
 int snd_usb_endpoint_next_packet_size(struct snd_usb_endpoint *ep,
 				      struct snd_urb_ctx *ctx, int idx,
 				      unsigned int avail);
-void snd_usb_queue_pending_output_urbs(struct snd_usb_endpoint *ep,
-				       bool in_stream_lock);
+int snd_usb_queue_pending_output_urbs(struct snd_usb_endpoint *ep,
+				      bool in_stream_lock);
 
 #endif /* __USBAUDIO_ENDPOINT_H */
--- a/sound/usb/pcm.c
+++ b/sound/usb/pcm.c
@@ -1639,7 +1639,7 @@ static int snd_usb_pcm_playback_ack(struct snd_pcm_substream *substream)
 	 * outputs here
 	 */
 	if (!ep->active_mask)
-		snd_usb_queue_pending_output_urbs(ep, true);
+		return snd_usb_queue_pending_output_urbs(ep, true);
 	return 0;
 }
 

thanks,

Takashi