[alsa-devel] Is add uevent of controlC still a reliable synchronization point?
Takashi Iwai
tiwai at suse.de
Wed May 16 16:00:35 CEST 2018
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 at 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 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
>
More information about the Alsa-devel
mailing list