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 77FAAC433EF for ; Fri, 15 Oct 2021 16:56:53 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 63151611C3 for ; Fri, 15 Oct 2021 16:56:53 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S241819AbhJOQ66 (ORCPT ); Fri, 15 Oct 2021 12:58:58 -0400 Received: from smtp-out1.suse.de ([195.135.220.28]:34122 "EHLO smtp-out1.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S237724AbhJOQ6z (ORCPT ); Fri, 15 Oct 2021 12:58:55 -0400 Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out1.suse.de (Postfix) with ESMTP id 356E821A6D; Fri, 15 Oct 2021 16:56:48 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1634317008; 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=dhDQfRzdWjoQAA9wLkDzmZrDRzBCVOaY+5kBnn+OZ+g=; b=NSSOPO5dO3DmBJs2cFghl1oPUO8fa6oH3Cpq0EZQg+Gc8L09smGJgx7ubuCrnvChFWJ9HH m8msQ1ifgDIAsHgr12GDWTmRFe8NFwR1TfeQO66yRY4qp9QBZVQ2GzYnhIKHYoE+VAQSmz R8ZdT5nPEDt11oj3hUSzD9SNgkCtp7c= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1634317008; 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=dhDQfRzdWjoQAA9wLkDzmZrDRzBCVOaY+5kBnn+OZ+g=; b=WerHXpzNkZtBKCeOCuB72DTfG8Sa4VnztHlHvr6o6NJ2f1SUvngyoqyaHaajqMg1Nc/xY8 GJpVf05XfRxqbDAw== Received: from alsa1.suse.de (alsa1.suse.de [10.160.4.42]) by relay2.suse.de (Postfix) with ESMTP id 967BBA3B89; Fri, 15 Oct 2021 16:56:47 +0000 (UTC) Date: Fri, 15 Oct 2021 18:56:47 +0200 Message-ID: From: Takashi Iwai To: Pierre-Louis Bossart Cc: alsa-devel@alsa-project.org, Kuninori Morimoto , open list , Sameer Pujar , Liam Girdwood , Takashi Iwai , vkoul@kernel.org, broonie@kernel.org, Gyeongtaek Lee , Peter Ujfalusi Subject: Re: [RFC PATCH v3 05/13] ASoC: soc-pcm: align BE 'atomicity' with that of the FE In-Reply-To: <8aa4fa07-2b55-3927-f482-c2fd2b01a22e@linux.intel.com> References: <20211013143050.244444-1-pierre-louis.bossart@linux.intel.com> <20211013143050.244444-6-pierre-louis.bossart@linux.intel.com> <2847a6d1-d97f-4161-c8b6-03672cf6645c@nvidia.com> <8aa4fa07-2b55-3927-f482-c2fd2b01a22e@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 Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, 15 Oct 2021 18:22:58 +0200, Pierre-Louis Bossart wrote: > > > > The FE stream locks are necessary only two points: at adding and > > deleting the BE in the link. We used dpcm_lock in other places, but > > those are superfluous or would make problem if converted to a FE > > stream lock. > > I must be missing a fundamental concept here - possibly a set of concepts... > > It is my understanding that the FE-BE connection can be updated > dynamically without any relationship to the usual ALSA steps, e.g. as a > result of a control being changed by a user. > > So if you only protect the addition/removal, isn't there a case where > the for_each_dpcm_be() loop would either miss a BE or point to an > invalid one? No, for sleepable context, pcm_mutex is *always* taken when adding/deleting a BE, and that's the main protection for the link. The BE stream lock is taken additionally over it at adding/deleting a BE, just for the code path via FE and BE trigger. > In other words, don't we need the *same* lock to be used > a) before changing and > b) walking through the list? > I also don't get what would happen if the dpcm_lock was converted to an > FE stream lock. It works fine in my tests, so if there's limitation I > didn't see it. dpcm_lock was put in the places that could be recursively taken. So this caused some deadlock, I suppose. > >>> In addition, a lock around dpcm_show_state() might be needed to be > >>> replaced with card->pcm_mutex, and we may need to revisit whether all > >>> other paths take card->pcm_mutex. > >> > >> What happens if we show the state while a trigger happens? That's my > >> main concern with using two separate locks (pcm_mutex and FE stream > >> lock) to protect the same list, there are still windows of time where > >> the list is not protected. > > > > With the proper use of mutex, the list itself is protected. > > If we need to protect the concurrent access to each BE in the show > > method, an additional BE lock is needed in that part. But that's a > > subtle issue, as the link traversal itself is protected by the mutex. > > If I look at your patch2, dpcm_be_disconnect() protects the list removal > with the fe stream lock, but the show state is protected by both the > pcm_mutex and the fe stream lock. No, show_state() itself doesn't take any lock, but its caller dpcm_state_read_file() takes the pcm_mutex. That protects the list addition / deletion. > I have not been able to figure out when you need > a) the pcm_mutex only > b) the fe stream lock only > c) both pcm_mutex and fe stream lock The pcm_mutex is needed for every sleepable function that treat DPCM FE link, but the mutex is taken only at the upper level, i.e. the top-most caller like PCM ops FE itself or the DAPM calls. That said, pcm_mutex is the top-most protection of BE links in FE. But, there is a code path where a mutex can't be used, and that's the FE and BE trigger. For protecting against this, the FE stream lock is taken only at the placing both adding and deleting a BE *in addition*. At those places, both pcm_mutex and FE stream lock are taken. BE stream lock is taken in addition below the above mutex and FE locks. Takashi