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 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id D27ECC433EF for ; Tue, 12 Oct 2021 10:43:12 +0000 (UTC) Received: from alsa0.perex.cz (alsa0.perex.cz [77.48.224.243]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 0D03061050 for ; Tue, 12 Oct 2021 10:43:12 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 0D03061050 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=suse.de Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=alsa-project.org Received: from alsa1.perex.cz (alsa1.perex.cz [207.180.221.201]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by alsa0.perex.cz (Postfix) with ESMTPS id 693D541; Tue, 12 Oct 2021 12:42:20 +0200 (CEST) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa0.perex.cz 693D541 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=alsa-project.org; s=default; t=1634035390; bh=YaSfAarn7+z8CApRLYVq4huoMcp6whecBWfVufdo4oc=; h=Date:From:To:Subject:In-Reply-To:References:Cc:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From; b=Jdo/7bi2ZHmf3pZ9RzqsDn6dgynotPBjaCKQG5yQ4oyY5l+XF14U0+cvHicLeNT39 lNzEWtl4QYZt1labkY0fSBOyfGi9jU0qXR5M84jLNEf7a2hpW+aVlhKHUY6D8F499k im6MvtKGSysFt301/VRHZPSotemX7/++UWRRSHbk= Received: from alsa1.perex.cz (localhost.localdomain [127.0.0.1]) by alsa1.perex.cz (Postfix) with ESMTP id C4E5CF801F7; Tue, 12 Oct 2021 12:42:19 +0200 (CEST) Received: by alsa1.perex.cz (Postfix, from userid 50401) id 1C438F80212; Tue, 12 Oct 2021 12:42:18 +0200 (CEST) Received: from smtp-out1.suse.de (smtp-out1.suse.de [195.135.220.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by alsa1.perex.cz (Postfix) with ESMTPS id 8376FF80088 for ; Tue, 12 Oct 2021 12:42:11 +0200 (CEST) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa1.perex.cz 8376FF80088 Authentication-Results: alsa1.perex.cz; dkim=pass (1024-bit key) header.d=suse.de header.i=@suse.de header.b="RX0fqao/"; dkim=permerror (0-bit key) header.d=suse.de header.i=@suse.de header.b="BDQ3OdDz" Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out1.suse.de (Postfix) with ESMTP id 5A2F7220E5; Tue, 12 Oct 2021 10:42:11 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1634035331; 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=e5PkYv61Jzcv11apgfDdJuETazSr8+IfEH/+SEjVzaw=; b=RX0fqao/oHMcq6KrnTAh2IQCstEC81DkgE72aA+FO3piNhZ6PVK7sXsTjwfzMNIufW/p+m 3dXPgpjS/6tS9IXnw+oNqsq6ZCNVWimhswVEbUjSTGlGF2T/3B1N8WM9owrrzl62CpxFvC r0ZZBBU0/ujhn9XUBCB1yfp9enCB06A= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1634035331; 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=e5PkYv61Jzcv11apgfDdJuETazSr8+IfEH/+SEjVzaw=; b=BDQ3OdDzrTzrerIeRurFcfuSBraliXpS+ZSKm3KEkc3Q6hh6MBqKMSH8rUW36Y+KEg5104 F8pG7A2ryUsfl4BA== Received: from alsa1.suse.de (alsa1.suse.de [10.160.4.42]) by relay2.suse.de (Postfix) with ESMTP id BC383A3B83; Tue, 12 Oct 2021 10:42:10 +0000 (UTC) Date: Tue, 12 Oct 2021 12:42:10 +0200 Message-ID: From: Takashi Iwai To: Pierre-Louis Bossart Subject: Re: [RFC PATCH v2 0/5] ASoC: soc-pcm: fix trigger race conditions with shared BE In-Reply-To: References: <20211004225441.233375-1-pierre-louis.bossart@linux.intel.com> <1efa1c31-7342-05f8-5f73-95e2462d4179@linux.intel.com> <3683cf39-632b-50df-c65d-63779c464850@nvidia.com> <11257d77-9975-3b00-94da-5dc1b5c95fc6@linux.intel.com> <80882fe6-ea30-43f6-8d83-8995dd28c748@linux.intel.com> <60c6a90b-290d-368c-ce61-4d86e70eaa78@linux.intel.com> <75894aba-ca1a-51d6-df7d-ad53fcd89f79@linux.intel.com> <29397354-dc5b-7837-c71b-df4bde707df2@linux.intel.com> <8c1353f0-e897-e7b0-c7b9-5712b05ac91f@linux.intel.com> 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 Cc: alsa-devel@alsa-project.org, Kuninori Morimoto , Sameer Pujar , vkoul@kernel.org, broonie@kernel.org, Gyeongtaek Lee , Peter Ujfalusi X-BeenThere: alsa-devel@alsa-project.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: "Alsa-devel mailing list for ALSA developers - http://www.alsa-project.org" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: "Alsa-devel" On Tue, 12 Oct 2021 08:34:07 +0200, Takashi Iwai wrote: > > On Mon, 11 Oct 2021 22:06:51 +0200, > Pierre-Louis Bossart wrote: > > > > > > >>> Basically we need to protect two things: > > >>> - The BE links > > >>> - The concurrent accesses to BEs > > >>> The former belongs to each FE that holds the links, and the FE stream > > >>> lock would cover. The latter is rather a per-BE business. > > >>> > > >>> An oft-seen risk of multiple locks is deadlocking, but in this case, > > >>> as long as we keep the lock order FE->BE, nothing wrong can happen. > > >> > > >> famous last words "nothing wrong can happen." :-) > > >> > > >> I already added a helper to do this FE lock, I can easily replace the > > >> implementation to remove the spin_lock and use the FE PCM lock. > > >> we might even add the lock in the definition of for_each_dpcm_be() to > > >> avoid misses. > > >> > > >> Let me try this out today, thanks for the suggestions. > > > > > > well, it's not successful at all... > > > > > > When I replace the existing dpcm_lock with the FE PCM lock as you > > > suggested, without any additional changes, speaker-test produces valid > > > audio on the endpoints, but if I try a Ctrl-C or limit the number of > > > loops with the '-l' option, I hear an endless loop on the same buffer > > > and I have to power cycle my test device to stop the sound. > > > > > > See 2 patches attached, the first patch with the introduction of the > > > helper works fine, the second with the use of the FE PCM lock doesn't. > > > In hindsight I am glad I worked on minimal patches, one after the other, > > > to identify problems. > > > > > > And when I add the BE lock, then nothing happens. Device stuck and no > > > audio... > > > > > > There must be something we're missing related to the locking... > > > > And indeed there's a deadlock! > > > > snd_pcm_period_elapsed() takes the FE pcm stream lock, and will call > > snd_pcm_trigger. > > Indeed, this would deadlock. > > > So if we also take the pcm stream lock in the BE > > trigger, there's a conceptual deadlock: we painted ourselves in a corner > > by using the same lock twice. > > > > Takashi, are you really sure we should protect these for_each_dpcm_be() > > loops with the same pcm lock? > > The call within the FE lock is done only in dpcm_dai_trigger_fe_be(), > and this should call dpcm_be_dai_trigger() as is. In other places, > the calls are without FE lock, hence they can take the lock, > e.g. create a variant dpcm_dai_trigger_fe_be_lock(). Or rather it was the code path of snd_soc_dpcm_check_state()? The function could be called from dpcm_be_dai_trigger(). Currently dpcm_lock seems to be added at places casually covering some of the for_each_dpcm_be() or whatever... The whole lock scheme has to be revisited. > > it seems like asking for trouble to > > revisit the ALSA core just to walking through a list of BEs? Would you > > object to changing dpcm_lock as I suggested, but not interfering with > > stream handling? > > That would work, too, it's just a pity to degrade the fine-grained > locks that have been already taken into global locks... For the performance problem, at least, you can make it rwlock and rwsem types (depending on nonatomic) so that the concurrent accesses would work. The only exclusive lock is the case for adding and removing the entries, which should be done with write lock / sem down, while other link traversals can be executed in the read lock / sem. Takashi