[alsa-devel] Is add uevent of controlC still a reliable synchronization point?

[Takashi Iwai]

On Wed, 16 May 2018 07:29:44 +0200,
Takashi Iwai wrote:
> 
> On Wed, 16 May 2018 00:22:40 +0200,
> Tzung-Bi Shih wrote:
> > 
> > We noticed the issue because in some rarely case the code will be executed
> > by worker of workqueue (i.e. deferral probe since v3.4) instead of udevd.
> > As a result, applications rely on the change uevent of sound card (fired
> > from 78-sound-card.rules) will be signaled prematurely.  The applications
> > assume when they receive the signal, everything of the sound card should be
> > ready.  But in the case, they are still initializing.
> > 
> > We have tried to change the order of SNDRV_DEV_CONTROL to the last one to
> > fix the bug we met and it was doing great.  So if there will not bring any
> > side effect, it should be fine to apply the patch.
> 
> The register and disconnect should be OK, as they are merely the parts
> to expose to and hide from user-space.
> 
> > By the way, out of curiosity, prior to the commit 289ca025ee1d, it seems
> > the list is merely a FIFO.  How to ensure control device is the last one to
> > register at that time?
> 
> It's just the fact that snd_ctl_new() is called inside the
> snd_card_new(), i.e. it's the very first one to be added.

On the second thought, calling the free for control at the very last
isn't always good.  A ctl element might have a reference to a card
"chip" object that was allocated as lowlevel device object, and often
it refers in the private_free call, too.  So, at best, we should
release the control before the lowlevel.

A revised patch is below.  Give it a try.


thanks,

Takashi

-- 8< --
From: Takashi Iwai <tiwai at suse.de>
Subject: [PATCH] ALSA: core: Assure control device to be registered 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 and lowlevel devices as last freed one.

Additional note:
The lowlevel device is the device where a card driver creates at
probe.  And, we still keep the release order control -> lowlevel, as
there might  be link from a control element back to a lowlevel object.

Fixes: 289ca025ee1d ("ALSA: Use priority list for managing device list")
Reported-by: Tzung-Bi Shih <tzungbi at google.com>
Signed-off-by: Takashi Iwai <tiwai at suse.de>
---
 include/sound/core.h | 2 +-
 sound/core/device.c  | 9 +++++++++
 2 files changed, 10 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..535102d564e3 100644
--- a/sound/core/device.c
+++ b/sound/core/device.c
@@ -240,6 +240,15 @@ 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 ctl and lowlevel stuff later */
+		if (dev->type == SNDRV_DEV_CONTROL ||
+		    dev->type == SNDRV_DEV_LOWLEVEL)
+			continue;
+		__snd_device_free(dev);
+	}
+
+	/* free all */
 	list_for_each_entry_safe_reverse(dev, next, &card->devices, list)
 		__snd_device_free(dev);
 }
-- 
2.16.3