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 7A01FC433F5 for ; Thu, 7 Oct 2021 18:14:39 +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 1B94F60F39 for ; Thu, 7 Oct 2021 18:14:37 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 1B94F60F39 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linux.intel.com 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 5D60115E2; Thu, 7 Oct 2021 20:13:46 +0200 (CEST) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa0.perex.cz 5D60115E2 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=alsa-project.org; s=default; t=1633630476; bh=CorTNxY65AqHDioCaz24I7yAKnSpyjqVkmJQMbfmdgQ=; h=Subject:To:References:From:Date:In-Reply-To:Cc:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From; b=cBIaWJ6BPu/oh2hF2ssYeE1B4yn7K6ne1OALC7+/TLoTuM+jUlPJoJG+W7/Drw1Sc p2wSso/HgO6CPdTX68N/gVZjrYcBgOdKwvn99i2XTXvzFvKi35AUVb8QBWP94z4I9e ycUWUuxNyll7mUGEBM6THm1yzD2WXbvKOftgxMrw= Received: from alsa1.perex.cz (localhost.localdomain [127.0.0.1]) by alsa1.perex.cz (Postfix) with ESMTP id CA19DF80259; Thu, 7 Oct 2021 20:13:45 +0200 (CEST) Received: by alsa1.perex.cz (Postfix, from userid 50401) id F25C0F8027D; Thu, 7 Oct 2021 20:13:32 +0200 (CEST) Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by alsa1.perex.cz (Postfix) with ESMTPS id AF62BF80130 for ; Thu, 7 Oct 2021 20:13:28 +0200 (CEST) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa1.perex.cz AF62BF80130 X-IronPort-AV: E=McAfee;i="6200,9189,10130"; a="289826400" X-IronPort-AV: E=Sophos;i="5.85,355,1624345200"; d="scan'208";a="289826400" Received: from orsmga003.jf.intel.com ([10.7.209.27]) by orsmga105.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 07 Oct 2021 11:13:26 -0700 X-IronPort-AV: E=Sophos;i="5.85,355,1624345200"; d="scan'208";a="440362659" Received: from klmutolo-mobl.amr.corp.intel.com (HELO [10.212.1.203]) ([10.212.1.203]) by orsmga003-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 07 Oct 2021 11:13:25 -0700 Subject: Re: [RFC PATCH v2 0/5] ASoC: soc-pcm: fix trigger race conditions with shared BE To: Takashi Iwai 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> From: Pierre-Louis Bossart Message-ID: <60c6a90b-290d-368c-ce61-4d86e70eaa78@linux.intel.com> Date: Thu, 7 Oct 2021 13:13:22 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Firefox/78.0 Thunderbird/78.13.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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" >> Using snd_pcm_stream_lock_irqsave(be_substream, flags); will prevent >> multiple triggers indeed, but the state management is handled by >> dpcm_lock, so I think we have to use dpcm_lock/mutex in all BE transitions. >> >> if ((be->dpcm[stream].state != SND_SOC_DPCM_STATE_PREPARE) && >> (be->dpcm[stream].state != SND_SOC_DPCM_STATE_STOP) && >> (be->dpcm[stream].state != SND_SOC_DPCM_STATE_PAUSED)) >> >> if ((be->dpcm[stream].state != SND_SOC_DPCM_STATE_PREPARE) && >> (be->dpcm[stream].state != SND_SOC_DPCM_STATE_STOP) && >> (be->dpcm[stream].state != SND_SOC_DPCM_STATE_PAUSED)) > > The stream lock can be put around those appropriate places, I suppose? I doubled checked the code a bit more, and all functions using be->dpcm[stream].state and be->dpcm[stream].users are protected by the card->mutex. The exceptions are dpcm_be_dai_trigger() and dpcm_show_state() so we probably don't need to worry too much about these fields. I am more nervous about that the dpcm_lock was supposed to protect. It was added in "ASoC: dpcm: prevent snd_soc_dpcm use after free" to solve a race condition, according to the commit log between void dpcm_be_disconnect( ... list_del(&dpcm->list_be); list_del(&dpcm->list_fe); kfree(dpcm); ... and for_each_dpcm_fe() for_each_dpcm_be*() That would suggest that every one of those loops should be protected, but that's not the case at all. In some cases the spinlock is taken *inside* of the loops. I think this is what explains the problem reported by Gyeongtaek Lee in https://lore.kernel.org/alsa-devel/002f01d7b4f5$c030f4a0$4092dde0$@samsung.com/ the for_each_dpcm_be() loop in dpcm_be_dai_trigger() is NOT protected. But if we add a spin-lock in there, the atomicity remains a problem. I think the only solution is to follow the example of the PCM case, where the type of lock depends on the FE types, with the assumption that there are no mixed atomic/non-atomic FE configurations.