From patchwork Tue Oct 17 10:37:42 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Takashi Iwai X-Patchwork-Id: 10011629 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id D245660235 for ; Tue, 17 Oct 2017 10:37:50 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id C40362881B for ; Tue, 17 Oct 2017 10:37:50 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id B8D2228844; Tue, 17 Oct 2017 10:37:50 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-1.9 required=2.0 tests=BAYES_00, RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.1 Received: from alsa0.perex.cz (alsa0.perex.cz [77.48.224.243]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id E29F72881B for ; Tue, 17 Oct 2017 10:37:49 +0000 (UTC) Received: from alsa0.perex.cz (localhost [127.0.0.1]) by alsa0.perex.cz (Postfix) with ESMTP id 34F0D2672C0; Tue, 17 Oct 2017 12:37:48 +0200 (CEST) X-Original-To: alsa-devel@alsa-project.org Delivered-To: alsa-devel@alsa-project.org Received: by alsa0.perex.cz (Postfix, from userid 1000) id BE5E926731F; Tue, 17 Oct 2017 12:37:46 +0200 (CEST) Received: from mx2.suse.de (mx2.suse.de [195.135.220.15]) by alsa0.perex.cz (Postfix) with ESMTP id 72D9B266F4E for ; Tue, 17 Oct 2017 12:37:44 +0200 (CEST) X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay1.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id C88C4ABDC; Tue, 17 Oct 2017 10:37:42 +0000 (UTC) Date: Tue, 17 Oct 2017 12:37:42 +0200 Message-ID: From: Takashi Iwai To: Kuninori Morimoto In-Reply-To: <87k1zu7g1l.wl%kuninori.morimoto.gx@renesas.com> References: <20171011101618.8736-1-tiwai@suse.de> <87fuanv70u.wl%kuninori.morimoto.gx@renesas.com> <87efq7v2qf.wl%kuninori.morimoto.gx@renesas.com> <87k1zvn8ci.wl%kuninori.morimoto.gx@renesas.com> <87k1zu7g1l.wl%kuninori.morimoto.gx@renesas.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") Cc: alsa-devel@alsa-project.org, Mark Brown Subject: Re: [alsa-devel] [PATCH v2 0/2] Add snd_card_disconnect_sync() helper X-BeenThere: alsa-devel@alsa-project.org X-Mailman-Version: 2.1.14 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-bounces@alsa-project.org X-Virus-Scanned: ClamAV using ClamSMTP On Tue, 17 Oct 2017 02:59:26 +0200, Kuninori Morimoto wrote: > > > Hi Takashi-san, Mark > > Thank you for your feedback > > > > Then, snd_pcm_relase() side will calls > > > snd_pcm_detach_substream() and snd_pcm_dev_disconnect() side will die. > > > > This must be also specific to DPCM. Something is really wrong there. > > > > Basically snd_pcm_dev_disconnect() and snd_pcm_release() can't race > > since both are protected via pcm->open_mutex. snd_pcm_stop() calls > > are protected even more with substream lock. > > > > > Mark's suggestion (= hiding BE) can solve this ? > > > > Some of the issues might be addressed, yes, but I'm skeptical whether > > it covers all. The BE needs proper locking and refcounting that is > > coupled with the FE, I suppose. > > So, this means, your helper patch seems OK, > but DPCM side needs more hack. > > Mark > I'm happy to solve this issue, but I'm not good at DPCM. > If you can give me some help/advice, I can debug it. It seems that the whole disconnect call can be dropped for the BE, as it does nothing practically for the register callback, either. Other than that, managing the object with the ALSA core device list is still worth for tracking the memory release. Below is the patch to do that. It can be applied on top of the previous two patches (snd_card_disconnect_sync() and PCM-stop-at-disconnect). thanks, Takashi -- 8< -- From: Takashi Iwai Subject: [PATCH] ALSA: pcm: Don't call register and disconnect callbacks for internal PCM The internal PCM (aka DPCM backend PCM) doesn't need any registration procedure, thus currently we bail out immediately at dev_register callback. Similarly, its counterpart, dev_disconnect callback, is superfluous for the internal PCM. For simplifying and avoiding the conflicting disconnect call for internal PCM objects, this patch drops dev_register and dev_disconnect callbacks for the internal ops. The only uncertain thing by this action is whether skipping the PCM state change to SNDRV_PCM_STATE_DISCONNECT for the internal PCM is mandatory. Looking through the current implementations, this doesn't look so, hence dropping the whole dev_disconnect would make more sense. Signed-off-by: Takashi Iwai Tested-by: Kuninori Morimoto --- sound/core/pcm.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/sound/core/pcm.c b/sound/core/pcm.c index 7eadb7fd8074..1b073ed0b1f9 100644 --- a/sound/core/pcm.c +++ b/sound/core/pcm.c @@ -775,6 +775,9 @@ static int _snd_pcm_new(struct snd_card *card, const char *id, int device, .dev_register = snd_pcm_dev_register, .dev_disconnect = snd_pcm_dev_disconnect, }; + static struct snd_device_ops internal_ops = { + .dev_free = snd_pcm_dev_free, + }; if (snd_BUG_ON(!card)) return -ENXIO; @@ -801,7 +804,8 @@ static int _snd_pcm_new(struct snd_card *card, const char *id, int device, if (err < 0) goto free_pcm; - err = snd_device_new(card, SNDRV_DEV_PCM, pcm, &ops); + err = snd_device_new(card, SNDRV_DEV_PCM, pcm, + internal ? &internal_ops : &ops); if (err < 0) goto free_pcm; @@ -1099,8 +1103,6 @@ static int snd_pcm_dev_register(struct snd_device *device) if (snd_BUG_ON(!device || !device->device_data)) return -ENXIO; pcm = device->device_data; - if (pcm->internal) - return 0; mutex_lock(®ister_mutex); err = snd_pcm_add(pcm); @@ -1159,12 +1161,10 @@ static int snd_pcm_dev_disconnect(struct snd_device *device) snd_pcm_stream_unlock_irq(substream); } } - if (!pcm->internal) { - pcm_call_notify(pcm, n_disconnect); - } + + pcm_call_notify(pcm, n_disconnect); for (cidx = 0; cidx < 2; cidx++) { - if (!pcm->internal) - snd_unregister_device(&pcm->streams[cidx].dev); + snd_unregister_device(&pcm->streams[cidx].dev); free_chmap(&pcm->streams[cidx]); } mutex_unlock(&pcm->open_mutex);