From patchwork Tue May 15 18:36:09 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Takashi Iwai X-Patchwork-Id: 10401777 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 3D1FD601E9 for ; Tue, 15 May 2018 18:42:15 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 2C17E284AF for ; Tue, 15 May 2018 18:42:15 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 2A722284C3; Tue, 15 May 2018 18:42:15 +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=-2.9 required=2.0 tests=BAYES_00, MAILING_LIST_MULTI, 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 12AFC28458 for ; Tue, 15 May 2018 18:42:13 +0000 (UTC) Received: from alsa0.perex.cz (localhost [127.0.0.1]) by alsa0.perex.cz (Postfix) with ESMTP id E4CE5266CCE; Tue, 15 May 2018 20:36:16 +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 9F2D4266DF0; Tue, 15 May 2018 20:36:14 +0200 (CEST) Received: from mx2.suse.de (mx2.suse.de [195.135.220.15]) by alsa0.perex.cz (Postfix) with ESMTP id 81FD8266C1C for ; Tue, 15 May 2018 20:36:11 +0200 (CEST) X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id 5177CAD21; Tue, 15 May 2018 18:36:10 +0000 (UTC) Date: Tue, 15 May 2018 20:36:09 +0200 Message-ID: From: Takashi Iwai To: Tzung-Bi Shih 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") Cc: alsa-devel@alsa-project.org, Dylan Reid , Jimmy Cheng-Yi Chiang Subject: Re: [alsa-devel] Is add uevent of controlC still a reliable synchronization point? 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, 15 May 2018 19:49:59 +0200, Tzung-Bi Shih wrote: > > Hi, > > We found an assumption in 78-sound-cards.rules and commit > 289ca025ee1d78223e3368801fc2b984e5efbfc7 are conflict. The control device > will not be the last one to register. > > The assumption in 78-sound-card.rules: > > The control device node creation can be used as synchronization point. > All other devices that belong to a card are created in the kernel before it. > > The commit: > > commit 289ca025ee1d78223e3368801fc2b984e5efbfc7 > > Author: Takashi Iwai > > Date: Wed Jan 29 15:53:35 2014 +0100 > > > > ALSA: Use priority list for managing device list > > > > Basically, the device type specifies the priority of the device to be > > registered / freed, too. However, the priority value isn't well > > utilized but only it's checked as a group. This results in > > inconsistent register and free order (where each of them should be in > > reversed direction). > > > > This patch simplifies the device list management code by simply > > inserting a list entry at creation time in an incremental order for > > the priority value. Since we can just follow the link for register, > > disconnect and free calls, we don't have to specify the group; so the > > whole enum definitions are also simplified as well. > > > > The visible change to outside is that the priorities of some object > > types are revisited. For example, now the SNDRV_DEV_LOWLEVEL object > > is registered before others (control, PCM, etc) and, in return, > > released after others. Similarly, SNDRV_DEV_CODEC is in a lower > > priority than SNDRV_DEV_BUS for ensuring the dependency. > > > > Also, the unused SNDRV_DEV_TOPLEVEL, SNDRV_DEV_LOWLEVEL_PRE and > > SNDRV_DEV_LOWLEVEL_NORMAL are removed as a cleanup. > > > > Signed-off-by: Takashi Iwai > > The device types: > > enum snd_device_type { > > SNDRV_DEV_LOWLEVEL, > > SNDRV_DEV_CONTROL, > > SNDRV_DEV_INFO, > > SNDRV_DEV_BUS, > > SNDRV_DEV_CODEC, > > SNDRV_DEV_SEQUENCER, > > SNDRV_DEV_HWDEP, > > SNDRV_DEV_JACK, > > }; > > The commit sorts device types ascendantly and registers them from the list > head. As a result, SNDRV_DEV_CONTROL devices will be registered before > most other devices. > > We are writing to ask: Is add uevent of controlC still a reliable signal > for indicating everything is ready for a sound card? Or is there now a > better way to do so? It's a good point, and I think it's rather a bug that was overlooked by the commit. A simple fix would be to adjust the priority order to move SNDRV_DEV_CONTROL at the end. This would work for register and disconnect, but for free, we need to tweak it manually so that the control is freed at last. It's because some components do create / free ctl elements, hence otherwise it may lead to double-free (if not written correctly). So it'd be a patch like below. (WARNING: totally untested!!) thanks, Takashi -- 8< -- From: Takashi Iwai Subject: [PATCH] ALSA: Register/disconnect control device always at last The commit 289ca025ee1d ("ALSA: Use priority list for managing device list") changed the way to register/disconnect/free devices via a single priority list. This helped to make behavior consistent, but it also changed a slight behavior change: namely, the control device is registered earlier than others, while it was supposed to be the very last one. I've put SNDRV_DEV_CONTROL in the current position as the release of ctl elements often conflict with the private ctl elements some PCM or other components may create, which often leads to a double-free. But, the order of register and disconnect should be indeed fixed as expected in the early days: the control device gets registered at last, and disconnected at first. This patch changes the priority list order to move SNDRV_DEV_CONTROL as the last guy to assure the register / disconnect order. Meanwhile, for keeping the messy resource release order, manually treat the control device as last freed one. Fixes: 289ca025ee1d ("ALSA: Use priority list for managing device list") Reported-by: Tzung-Bi Shih Signed-off-by: Takashi Iwai --- include/sound/core.h | 2 +- sound/core/device.c | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/include/sound/core.h b/include/sound/core.h index 5f181b875c2f..36a5934cf4b1 100644 --- a/include/sound/core.h +++ b/include/sound/core.h @@ -51,7 +51,6 @@ struct completion; */ enum snd_device_type { SNDRV_DEV_LOWLEVEL, - SNDRV_DEV_CONTROL, SNDRV_DEV_INFO, SNDRV_DEV_BUS, SNDRV_DEV_CODEC, @@ -62,6 +61,7 @@ enum snd_device_type { SNDRV_DEV_SEQUENCER, SNDRV_DEV_HWDEP, SNDRV_DEV_JACK, + SNDRV_DEV_CONTROL, /* NOTE: this must be the last one */ }; enum snd_device_state { diff --git a/sound/core/device.c b/sound/core/device.c index cb0e46f66cc9..dae845fd5852 100644 --- a/sound/core/device.c +++ b/sound/core/device.c @@ -240,6 +240,14 @@ void snd_device_free_all(struct snd_card *card) if (snd_BUG_ON(!card)) return; + list_for_each_entry_safe_reverse(dev, next, &card->devices, list) { + /* exception: free the control device at last */ + if (dev->type == SNDRV_DEV_CONTROL) + continue; + __snd_device_free(dev); + } + + /* free all */ list_for_each_entry_safe_reverse(dev, next, &card->devices, list) __snd_device_free(dev); }