From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-15.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 84320C07E98 for ; Sat, 3 Jul 2021 07:57:57 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 56CD261421 for ; Sat, 3 Jul 2021 07:57:57 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229981AbhGCH7W (ORCPT ); Sat, 3 Jul 2021 03:59:22 -0400 Received: from smtp-out2.suse.de ([195.135.220.29]:50106 "EHLO smtp-out2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229947AbhGCH7W (ORCPT ); Sat, 3 Jul 2021 03:59:22 -0400 Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out2.suse.de (Postfix) with ESMTP id E5F39201F3; Sat, 3 Jul 2021 07:56:47 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1625299007; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=S9lUuaKKlL1yqpBR0tOcM/5tXs+wCxPEG/augijVS3A=; b=sjHpXjXHyh79bndkh7aTwlVqpfvn2rr6TfDwtBj0TAUXGRIaALL82h2LlfHvwYfSVp4bhX YYkd66OHfnnhEyfHAhKGZrORUEFXkWl0sBdlcA2hlBowbx7XuNCBo2OXo8wHD+TqWpNcMb P1n1yabze52SgIGuU1XC8c984IE6hEY= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1625299007; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=S9lUuaKKlL1yqpBR0tOcM/5tXs+wCxPEG/augijVS3A=; b=xEpz0jMkzspMTuOwqBAHiofNod6PSV/OrfnV6mpvelQtLo7O5pX7hQrUWhdVKtporb4iVR lFnPIIaeQszH/kDw== Received: from alsa1.suse.de (alsa1.suse.de [10.160.4.42]) by relay2.suse.de (Postfix) with ESMTP id 84D55A3B81; Sat, 3 Jul 2021 07:56:47 +0000 (UTC) Date: Sat, 03 Jul 2021 09:56:47 +0200 Message-ID: From: Takashi Iwai To: Takashi Sakamoto Cc: Linus Torvalds , alsa-devel@alsa-project.org, Mark Brown , Liam Girdwood , Linux Kernel Mailing List , marcan@marcan.st Subject: Re: [GIT PULL] sound updates for 5.14-rc1 In-Reply-To: References: 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 List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > > 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 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 Reported-by: Linus Torvalds Signed-off-by: Takashi Iwai --- 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