Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Sat, 03 Jul 2021 09:56:47 +0200
Message-ID: <s5hh7hbakzk.wl-tiwai@suse.de>
From:   Takashi Iwai <tiwai@suse.de>
To:     Takashi Sakamoto <o-takashi@sakamocchi.jp>
Cc:     Linus Torvalds <torvalds@linux-foundation.org>,
        alsa-devel@alsa-project.org, Mark Brown <broonie@kernel.org>,
        Liam Girdwood <lgirdwood@gmail.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        marcan@marcan.st
Subject: Re: [GIT PULL] sound updates for 5.14-rc1
In-Reply-To: <YOAF+EnvdBvSeZnR@workstation>
References: <s5hbl7li0fe.wl-tiwai@suse.de>
        <CAHk-=wiTwX5mzzEcw3jk3QdW600Hntt=Ehgoyz8K-VU8zbEmBg@mail.gmail.com>
        <CAHk-=whhDWDsVz12mhKtnS6DG-GzVWxBk2XjoOp=gwNJ7T0bTw@mail.gmail.com>
        <CAHk-=wisOVeVpH42f6i5qW1gxtYxbRJQXvpt=mdVx+8p=w-yMg@mail.gmail.com>
        <YOAF+EnvdBvSeZnR@workstation>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka)
 FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/25.3
 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Sat, 03 Jul 2021 08:38:48 +0200,
Takashi Sakamoto wrote:
> 
> Hi,
> 
> On Fri, Jul 02, 2021 at 10:19:46PM -0700, Linus Torvalds wrote:
> > On Fri, Jul 2, 2021 at 9:37 PM Linus Torvalds
> > <torvalds@linux-foundation.org> wrote:
> > >
> > > But I thought I'd report this as a likely candidate.
> > 
> > Confirmed. The watchdog hang bisects right down to commit 9ce650a75a3b
> > ("ALSA: usb-audio: Reduce latency at playback start").
> > 
> > And reverting it on top of my tree also fixes the hang, so it's not
> > some bisection fluke.
> > 
> > I have no idea what is actually wrong with that commit, but it most
> > definitely is the problem, and I have reverted it in my tree so that I
> > can continue merging stuff tomorrow.
> 
> The cause seems to be the attempt to lock PCM substream recursively
> introduced by the issued commit.
> 
> Would I ask you to test with below patch? I apologize that the patch is
> still untested in my side since at present I have no preparation to debug
> USB stuffs instantly (I'm just a maintainer for ALSA firewire stack...),
> so I'm glad if getting your cooperation for the issue.

That's no ideal workaround because it'll call snd_pcm_period_elapsed()
before the stream actually gets started.  That said, it's not only
about the lock but also about the state change, too.

Below is another possible fix.  This moves conditionally the
snd_pcm_period_elapsed() call to the complete callback, so that it'll
be processed in a different context.

Unfortunately I can't test much right now in my side as I'm traveling
(until the next Tuesday).  So, Linus, Hector, please let me know if
this works.  Once when it's confirmed to work, I'll prepare the new PR
including the fix later in today.


thanks,

Takashi

-- 8< --
From: Takashi Iwai <tiwai@suse.de>
Subject: [PATCH] ALSA: usb-audio: Fix possible deadlock at playback start

The recent change for the PCM playback trigger caused an unexpected
deadlock due to the period-elapsed handling at
prepare_playback_urb().  This hasn't been a problem until now because
the stream got started before the trigger call, but now this callback
is called at the trigger, too, hence the problem surfaced.

As a workaround, this patch introduces a flag for delaying the
snd_pcm_period_elapsed() call to the retire_playback_urb(), which is
set when the hwptr reaches to the period boundary already at the PCM
playback start time.

Fixes: 9ce650a75a3b ("ALSA: usb-audio: Reduce latency at playback start")
Reported-by: Hector Martin <marcan@marcan.st>
Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/usb/card.h |  1 +
 sound/usb/pcm.c  | 10 ++++++++++
 2 files changed, 11 insertions(+)

diff --git a/sound/usb/card.h b/sound/usb/card.h
index 5577a776561b..f309a5fafc1d 100644
--- a/sound/usb/card.h
+++ b/sound/usb/card.h
@@ -158,6 +158,7 @@ struct snd_usb_substream {
 	unsigned int stream_offset_adj;	/* Bytes to drop from beginning of stream (for non-compliant devices) */
 
 	unsigned int running: 1;	/* running status */
+	unsigned int period_elapsed_pending;	/* issue at retire callback */
 
 	unsigned int buffer_bytes;	/* buffer size in bytes */
 	unsigned int inflight_bytes;	/* in-flight data bytes on buffer (for playback) */
diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
index c66831ee15f9..903f5d7e33e3 100644
--- a/sound/usb/pcm.c
+++ b/sound/usb/pcm.c
@@ -611,6 +611,7 @@ static int snd_usb_pcm_prepare(struct snd_pcm_substream *substream)
 	subs->hwptr_done = 0;
 	subs->transfer_done = 0;
 	subs->last_frame_number = 0;
+	subs->period_elapsed_pending = 0;
 	runtime->delay = 0;
 
  unlock:
@@ -1393,6 +1394,10 @@ static void prepare_playback_urb(struct snd_usb_substream *subs,
 		subs->trigger_tstamp_pending_update = false;
 	}
 
+	if (period_elapsed && !subs->running) {
+		subs->period_elapsed_pending = 1;
+		period_elapsed = 0;
+	}
 	spin_unlock_irqrestore(&subs->lock, flags);
 	urb->transfer_buffer_length = bytes;
 	if (period_elapsed)
@@ -1408,6 +1413,7 @@ static void retire_playback_urb(struct snd_usb_substream *subs,
 {
 	unsigned long flags;
 	struct snd_urb_ctx *ctx = urb->context;
+	bool period_elapsed;
 
 	spin_lock_irqsave(&subs->lock, flags);
 	if (ctx->queued) {
@@ -1418,7 +1424,11 @@ static void retire_playback_urb(struct snd_usb_substream *subs,
 	}
 
 	subs->last_frame_number = usb_get_current_frame_number(subs->dev);
+	period_elapsed = subs->period_elapsed_pending;
+	subs->period_elapsed_pending = 0;
 	spin_unlock_irqrestore(&subs->lock, flags);
+	if (period_elapsed)
+		snd_pcm_period_elapsed(subs->pcm_substream);
 }
 
 static int snd_usb_substream_playback_trigger(struct snd_pcm_substream *substream,
-- 
2.26.2