Is add uevent of controlC still a reliable synchronization point?

Is add uevent of controlC still a reliable synchronization point?

[Tzung-Bi Shih]

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 <tiwai@suse.de>
> 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 <tiwai@suse.de>

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?

Re: Is add uevent of controlC still a reliable synchronization point?

[Takashi Iwai]

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 <tiwai@suse.de>
> > 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 <tiwai@suse.de>
> 
> 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 <tiwai@suse.de>
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 <tzungbi@google.com>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 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);
 }
-- 
2.16.3

Re: Is add uevent of controlC still a reliable synchronization point?

[Tzung-Bi Shih]

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.

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?

On Wed, May 16, 2018 at 2:36 AM Takashi Iwai <tiwai@suse.de> wrote:

> 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 <tiwai@suse.de>
> > > 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 <tiwai@suse.de>
> >
> > 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 <tiwai@suse.de>
> 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 <tzungbi@google.com>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>      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);
>      }
> --
> 2.16.3

Re: Is add uevent of controlC still a reliable synchronization point?

[Takashi Iwai]

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.

If the patch works for you, please give your tested-by tag, so that
I'll merge it to for-next branch.  It looks like a too intrusive
change for 4.17-rc6, so maybe safer to queue for 4.18.


thanks,

Takashi

Re: 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@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@google.com>
Signed-off-by: Takashi Iwai <tiwai@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

Re: Is add uevent of controlC still a reliable synchronization point?

[Tzung-Bi Shih]

I think we have two objectives.

1. when initializing, make ctl to be the last one
2. when deinitializing, eventually free the resource

We have made 1. work by changing the enum.  For 2., we don't really care
about the order as long as the resource has eventually released.  If the
"free" part is a little bit cumbersome, is it possible to use a reference
count somewhere (e.g. struct snd_device) to make sure they will not
double-free?  So that we can keep snd_device_free_all() simple.

I have no idea whether it is feasible or not, just a rough idea.
On Wed, May 16, 2018 at 2:35 PM Takashi Iwai <tiwai@suse.de> wrote:

> 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@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@google.com>
> Signed-off-by: Takashi Iwai <tiwai@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

Re: Is add uevent of controlC still a reliable synchronization point?

[Takashi Iwai]

On Wed, 16 May 2018 15:43:19 +0200,
Tzung-Bi Shih wrote:
> 
> I think we have two objectives.
> 
> 1. when initializing, make ctl to be the last one
> 2. when deinitializing, eventually free the resource
> 
> We have made 1. work by changing the enum.  For 2., we don't really care
> about the order as long as the resource has eventually released.  If the
> "free" part is a little bit cumbersome, is it possible to use a reference
> count somewhere (e.g. struct snd_device) to make sure they will not
> double-free?  So that we can keep snd_device_free_all() simple.

That'd be ideal from the device management POV, but it will give too
much overhead in the current code base, as we'll need to add a
refcount to each ctl element object, for example.  And it'd make the
other parts far more complex in the end.  We write in C, and it's no
golden language for this kind of stuff, as you know :)

So, for keeping the rest as simple as possible, just tweaking the
release order should be simpler, IMO.


thanks,

Takashi

> I have no idea whether it is feasible or not, just a rough idea.
> On Wed, May 16, 2018 at 2:35 PM Takashi Iwai <tiwai@suse.de> wrote:
> 
> > 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@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@google.com>
> > Signed-off-by: Takashi Iwai <tiwai@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
> 

Re: Is add uevent of controlC still a reliable synchronization point?

[Tzung-Bi Shih]

We have tested the patch and make sure it fixes the bug we met.  In the
meantime, we also tested on the change of snd_device_free_all() by simply
count the number for each type of devices.  The number is correct.  And
free() of ctl device is later than all other types (except LOWLEVEL) as our
expectation.

Please use the tag: "Tested-by: Tzung-Bi Shih <tzungbi@google.com>"

Thanks so much.
On Wed, May 16, 2018 at 10:00 PM Takashi Iwai <tiwai@suse.de> wrote:

> On Wed, 16 May 2018 15:43:19 +0200,
> Tzung-Bi Shih wrote:
> >
> > I think we have two objectives.
> >
> > 1. when initializing, make ctl to be the last one
> > 2. when deinitializing, eventually free the resource
> >
> > We have made 1. work by changing the enum.  For 2., we don't really care
> > about the order as long as the resource has eventually released.  If the
> > "free" part is a little bit cumbersome, is it possible to use a
reference
> > count somewhere (e.g. struct snd_device) to make sure they will not
> > double-free?  So that we can keep snd_device_free_all() simple.

> That'd be ideal from the device management POV, but it will give too
> much overhead in the current code base, as we'll need to add a
> refcount to each ctl element object, for example.  And it'd make the
> other parts far more complex in the end.  We write in C, and it's no
> golden language for this kind of stuff, as you know :)

> So, for keeping the rest as simple as possible, just tweaking the
> release order should be simpler, IMO.


> thanks,

> Takashi

> > I have no idea whether it is feasible or not, just a rough idea.
> > On Wed, May 16, 2018 at 2:35 PM Takashi Iwai <tiwai@suse.de> wrote:
> >
> > > 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@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@google.com>
> > > Signed-off-by: Takashi Iwai <tiwai@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
> >

Re: Is add uevent of controlC still a reliable synchronization point?

[Takashi Iwai]

On Thu, 17 May 2018 01:28:30 +0200,
Tzung-Bi Shih wrote:
> 
> We have tested the patch and make sure it fixes the bug we met.  In the
> meantime, we also tested on the change of snd_device_free_all() by simply
> count the number for each type of devices.  The number is correct.  And
> free() of ctl device is later than all other types (except LOWLEVEL) as our
> expectation.
> 
> Please use the tag: "Tested-by: Tzung-Bi Shih <tzungbi@google.com>"

OK, I'll queue the fix up to for-next branch.

thanks,

Takashi

> 
> Thanks so much.
> On Wed, May 16, 2018 at 10:00 PM Takashi Iwai <tiwai@suse.de> wrote:
> 
> > On Wed, 16 May 2018 15:43:19 +0200,
> > Tzung-Bi Shih wrote:
> > >
> > > I think we have two objectives.
> > >
> > > 1. when initializing, make ctl to be the last one
> > > 2. when deinitializing, eventually free the resource
> > >
> > > We have made 1. work by changing the enum.  For 2., we don't really care
> > > about the order as long as the resource has eventually released.  If the
> > > "free" part is a little bit cumbersome, is it possible to use a
> reference
> > > count somewhere (e.g. struct snd_device) to make sure they will not
> > > double-free?  So that we can keep snd_device_free_all() simple.
> 
> > That'd be ideal from the device management POV, but it will give too
> > much overhead in the current code base, as we'll need to add a
> > refcount to each ctl element object, for example.  And it'd make the
> > other parts far more complex in the end.  We write in C, and it's no
> > golden language for this kind of stuff, as you know :)
> 
> > So, for keeping the rest as simple as possible, just tweaking the
> > release order should be simpler, IMO.
> 
> 
> > thanks,
> 
> > Takashi
> 
> > > I have no idea whether it is feasible or not, just a rough idea.
> > > On Wed, May 16, 2018 at 2:35 PM Takashi Iwai <tiwai@suse.de> wrote:
> > >
> > > > 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@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@google.com>
> > > > Signed-off-by: Takashi Iwai <tiwai@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
> > >
>