Message ID | s5hzjd5jlft.wl-tiwai@suse.de (mailing list archive) |
---|---|
State | Superseded |
Delegated to: | Takashi Iwai |
Headers | show |
On Thu, Oct 9, 2014 at 11:45 AM, Takashi Iwai <tiwai@suse.de> wrote: > At Thu, 09 Oct 2014 11:46:02 +0200, > Takashi Iwai wrote: > > > > At Thu, 09 Oct 2014 11:21:28 +0200, > > Takashi Iwai wrote: > > > > > > At Thu, 09 Oct 2014 11:14:57 +0200, > > > Clemens Ladisch wrote: > > > > > > > > Takashi Iwai wrote: > > > > > Clemens Ladisch wrote: > > > > >> Takashi Iwai wrote: > > > > >>> If it were a simple cleanup, I'm fine with it. But this leads to > > > > >>> a major behavior change, which has a high risk of > incompatibility. > > > > >> > > > > >> But there would be no changed behaviour as far as the API is > concerned > > > > >> (except for this particular issue, which is a bug). > > > > > > > > > > Currently, the sequencer stuff can be suppressed by simply not > loading > > > > > snd-seq core module itself. Do you mean to drop this feature? > > > > > > > > Yes. > > > > > > > > > Some distros don't load sequencer modules nowadays as default. > > > > > > > > Do you know why? Any reason except memory? > > > > > > Mostly yes, and also reduce the installation base. > > > > > > > > So it would result in a clear behavior change on the whole system. > > > > > > > > And alsa-lib tries its best to do autoloading to hide the fact that > > > > snd-seq might not have been loaded. Therefore, it has never been > > > > possible to assume, at any time, that snd-seq is _not_ loaded. > > > > > > What if the sound system doesn't exist at all like a server? The > > > module is built and provided even on such a system by a distro, but > > > it's just not enabled. > > > > > > > Is the high risk of incompatibility that you mentioned this > assumption, > > > > or anything else? > > > > > > It results in a surprising outcome, and it's what I'd like to avoid. > > > > Thinking of this again, what's missing right now is the instantaneous > > binding *only when* sequencer stuff is ready to use. That is, if > > snd-seq isn't loaded, we should assume that it's not for use, thus not > > forcibly load the module there. If so, a patch like below should work > > (again untested). It's a bit more than the previous one, but it's > > still simple enough and doesn't break. > > Scratch this patch. It still has the old known deadlocking due to the > request_module() in module init path. > > Below is a more comprehensive patch. And this time I actually tested > it :) Adam, could you give it a try? > > > thanks, > > Takashi > > -- 8< -- > From: Takashi Iwai <tiwai@suse.de> > Subject: [PATCH] ALSA: seq: bind seq driver automatically > > Currently the sequencer module binding is performed independently from > the card module itself. The reason behind it is to keep the sequencer > stuff optional and allow the system running without it (e.g. for using > PCM or rawmidi only). This works in most cases, but a remaining > problem is that the binding isn't done automatically when a new driver > module is probed. Typically this becomes visible when a hotplug > driver like usb audio is used. > > This patch tries to address this and other potential issues. First, > the seq-binder (seq_device.c) tries to load a missing driver module at > creating a new device object. This is done asynchronously in a workq > for avoiding the deadlock (modprobe call in module init path). > > This action, however, should be enabled only when the sequencer stuff > was already initialized, i.e. snd-seq module was already loaded. For > that, a new function, snd_seq_autoload_init() is introduced here; this > clears the blocking of autoloading, and also tries to load all pending > driver modules. > > Since we're calling request_module() asynchronously, we can get rid of > the autoload lock in snd_seq_device_register_driver(). This enables > the automatic loading of dependent sequencer modules, such as > snd-seq-virmidi from snd-emu10k1-synth. > > Last but not least, yet another fix is to use atomic_t for the > refcount, just to be more robust. > > Reported-by: Adam Goode <agoode@chromium.org> > Signed-off-by: Takashi Iwai <tiwai@suse.de> > --- > include/sound/seq_kernel.h | 4 ++ > sound/core/seq/seq.c | 5 ++- > sound/core/seq/seq_device.c | 103 > +++++++++++++++++++++++++++++++------------- > 3 files changed, 79 insertions(+), 33 deletions(-) > > diff --git a/include/sound/seq_kernel.h b/include/sound/seq_kernel.h > index 2398521f0998..eea5400fe373 100644 > --- a/include/sound/seq_kernel.h > +++ b/include/sound/seq_kernel.h > @@ -108,9 +108,13 @@ int snd_seq_event_port_detach(int client, int port); > #ifdef CONFIG_MODULES > void snd_seq_autoload_lock(void); > void snd_seq_autoload_unlock(void); > +void snd_seq_autoload_init(void); > +#define snd_seq_autoload_exit() snd_seq_autoload_lock() > #else > #define snd_seq_autoload_lock() > #define snd_seq_autoload_unlock() > +#define snd_seq_autoload_init() > +#define snd_seq_autoload_exit() > #endif > > #endif /* __SOUND_SEQ_KERNEL_H */ > diff --git a/sound/core/seq/seq.c b/sound/core/seq/seq.c > index 712110561082..7e0aabb808a6 100644 > --- a/sound/core/seq/seq.c > +++ b/sound/core/seq/seq.c > @@ -86,7 +86,6 @@ static int __init alsa_seq_init(void) > { > int err; > > - snd_seq_autoload_lock(); > if ((err = client_init_data()) < 0) > goto error; > > @@ -110,8 +109,8 @@ static int __init alsa_seq_init(void) > if ((err = snd_seq_system_client_init()) < 0) > goto error; > > + snd_seq_autoload_init(); > error: > - snd_seq_autoload_unlock(); > return err; > } > > @@ -131,6 +130,8 @@ static void __exit alsa_seq_exit(void) > > /* release event memory */ > snd_sequencer_memory_done(); > + > + snd_seq_autoload_exit(); > } > > module_init(alsa_seq_init) > diff --git a/sound/core/seq/seq_device.c b/sound/core/seq/seq_device.c > index 91a786a783e1..6b31ca09caf7 100644 > --- a/sound/core/seq/seq_device.c > +++ b/sound/core/seq/seq_device.c > @@ -56,6 +56,7 @@ MODULE_LICENSE("GPL"); > #define DRIVER_LOADED (1<<0) > #define DRIVER_REQUESTED (1<<1) > #define DRIVER_LOCKED (1<<2) > +#define DRIVER_REQUESTING (1<<3) > > struct ops_list { > char id[ID_LEN]; /* driver id */ > @@ -127,42 +128,82 @@ static void snd_seq_device_info(struct > snd_info_entry *entry, > > #ifdef CONFIG_MODULES > /* avoid auto-loading during module_init() */ > -static int snd_seq_in_init; > +static atomic_t snd_seq_in_init = ATOMIC_INIT(1); /* blocked as default */ > void snd_seq_autoload_lock(void) > { > - snd_seq_in_init++; > + atomic_inc(&snd_seq_in_init); > } > > void snd_seq_autoload_unlock(void) > { > - snd_seq_in_init--; > + atomic_dec(&snd_seq_in_init); > } > -#endif > > -void snd_seq_device_load_drivers(void) > +static void autoload_drivers(void) > { > -#ifdef CONFIG_MODULES > - struct ops_list *ops; > + /* avoid reentrance */ > + if (atomic_inc_return(&snd_seq_in_init) == 1) { > + struct ops_list *ops; > + > + mutex_lock(&ops_mutex); > + list_for_each_entry(ops, &opslist, list) { > + if ((ops->driver & DRIVER_REQUESTING) && > + !(ops->driver & DRIVER_REQUESTED)) { > + ops->used++; > + mutex_unlock(&ops_mutex); > + ops->driver |= DRIVER_REQUESTED; > + request_module("snd-%s", ops->id); > + mutex_lock(&ops_mutex); > + ops->used--; > + } > + } > + mutex_unlock(&ops_mutex); > + } > + atomic_dec(&snd_seq_in_init); > +} > > - /* Calling request_module during module_init() > - * may cause blocking. > - */ > - if (snd_seq_in_init) > - return; > +static void call_autoload(struct work_struct *work) > +{ > + autoload_drivers(); > +} > > - mutex_lock(&ops_mutex); > - list_for_each_entry(ops, &opslist, list) { > - if (! (ops->driver & DRIVER_LOADED) && > - ! (ops->driver & DRIVER_REQUESTED)) { > - ops->used++; > - mutex_unlock(&ops_mutex); > - ops->driver |= DRIVER_REQUESTED; > - request_module("snd-%s", ops->id); > - mutex_lock(&ops_mutex); > - ops->used--; > - } > +static DECLARE_WORK(autoload_work, call_autoload); > + > +static void try_autoload(struct ops_list *ops) > +{ > + if (!ops->driver) { > + ops->driver |= DRIVER_REQUESTING; > + schedule_work(&autoload_work); > } > +} > + > +static void queue_autoload_drivers(void) > +{ > + struct ops_list *ops; > + > + mutex_lock(&ops_mutex); > + list_for_each_entry(ops, &opslist, list) > + try_autoload(ops); > mutex_unlock(&ops_mutex); > +} > + > +void snd_seq_autoload_init(void) > +{ > + atomic_dec(&snd_seq_in_init); > +#ifdef CONFIG_SND_SEQUENCER_MODULE > + /* initial autoload only when snd-seq is a module */ > + queue_autoload_drivers(); > +#endif > +} > +#else > +#define try_autoload() /* NOP */ > +#endif > + > +void snd_seq_device_load_drivers(void) > +{ > +#ifdef CONFIG_MODULES > + queue_autoload_drivers(); > + flush_work(&autoload_work); > #endif > } > > @@ -214,13 +255,14 @@ int snd_seq_device_new(struct snd_card *card, int > device, char *id, int argsize, > ops->num_devices++; > mutex_unlock(&ops->reg_mutex); > > - unlock_driver(ops); > - > if ((err = snd_device_new(card, SNDRV_DEV_SEQUENCER, dev, &dops)) > < 0) { > snd_seq_device_free(dev); > return err; > } > > + try_autoload(ops); > + unlock_driver(ops); > + > if (result) > *result = dev; > > @@ -318,16 +360,12 @@ int snd_seq_device_register_driver(char *id, struct > snd_seq_dev_ops *entry, > entry->init_device == NULL || entry->free_device == NULL) > return -EINVAL; > > - snd_seq_autoload_lock(); > ops = find_driver(id, 1); > - if (ops == NULL) { > - snd_seq_autoload_unlock(); > + if (ops == NULL) > return -ENOMEM; > - } > if (ops->driver & DRIVER_LOADED) { > pr_warn("ALSA: seq: driver_register: driver '%s' already > exists\n", id); > unlock_driver(ops); > - snd_seq_autoload_unlock(); > return -EBUSY; > } > > @@ -344,7 +382,6 @@ int snd_seq_device_register_driver(char *id, struct > snd_seq_dev_ops *entry, > mutex_unlock(&ops->reg_mutex); > > unlock_driver(ops); > - snd_seq_autoload_unlock(); > > return 0; > } > @@ -554,6 +591,9 @@ static int __init alsa_seq_device_init(void) > > static void __exit alsa_seq_device_exit(void) > { > +#ifdef CONFIG_MODULES > + cancel_work_sync(&autoload_work); > +#endif > remove_drivers(); > #ifdef CONFIG_PROC_FS > snd_info_free_entry(info_entry); > @@ -570,6 +610,7 @@ EXPORT_SYMBOL(snd_seq_device_new); > EXPORT_SYMBOL(snd_seq_device_register_driver); > EXPORT_SYMBOL(snd_seq_device_unregister_driver); > #ifdef CONFIG_MODULES > +EXPORT_SYMBOL(snd_seq_autoload_init); > EXPORT_SYMBOL(snd_seq_autoload_lock); > EXPORT_SYMBOL(snd_seq_autoload_unlock); > #endif > -- > 2.1.2 > > Hi Takashi, I did test the code and it appears to work! Thanks! Note: I did not review the code in any other way than compiling and manual test. Adam
diff --git a/include/sound/seq_kernel.h b/include/sound/seq_kernel.h index 2398521f0998..eea5400fe373 100644 --- a/include/sound/seq_kernel.h +++ b/include/sound/seq_kernel.h @@ -108,9 +108,13 @@ int snd_seq_event_port_detach(int client, int port); #ifdef CONFIG_MODULES void snd_seq_autoload_lock(void); void snd_seq_autoload_unlock(void); +void snd_seq_autoload_init(void); +#define snd_seq_autoload_exit() snd_seq_autoload_lock() #else #define snd_seq_autoload_lock() #define snd_seq_autoload_unlock() +#define snd_seq_autoload_init() +#define snd_seq_autoload_exit() #endif #endif /* __SOUND_SEQ_KERNEL_H */ diff --git a/sound/core/seq/seq.c b/sound/core/seq/seq.c index 712110561082..7e0aabb808a6 100644 --- a/sound/core/seq/seq.c +++ b/sound/core/seq/seq.c @@ -86,7 +86,6 @@ static int __init alsa_seq_init(void) { int err; - snd_seq_autoload_lock(); if ((err = client_init_data()) < 0) goto error; @@ -110,8 +109,8 @@ static int __init alsa_seq_init(void) if ((err = snd_seq_system_client_init()) < 0) goto error; + snd_seq_autoload_init(); error: - snd_seq_autoload_unlock(); return err; } @@ -131,6 +130,8 @@ static void __exit alsa_seq_exit(void) /* release event memory */ snd_sequencer_memory_done(); + + snd_seq_autoload_exit(); } module_init(alsa_seq_init) diff --git a/sound/core/seq/seq_device.c b/sound/core/seq/seq_device.c index 91a786a783e1..6b31ca09caf7 100644 --- a/sound/core/seq/seq_device.c +++ b/sound/core/seq/seq_device.c @@ -56,6 +56,7 @@ MODULE_LICENSE("GPL"); #define DRIVER_LOADED (1<<0) #define DRIVER_REQUESTED (1<<1) #define DRIVER_LOCKED (1<<2) +#define DRIVER_REQUESTING (1<<3) struct ops_list { char id[ID_LEN]; /* driver id */ @@ -127,42 +128,82 @@ static void snd_seq_device_info(struct snd_info_entry *entry, #ifdef CONFIG_MODULES /* avoid auto-loading during module_init() */ -static int snd_seq_in_init; +static atomic_t snd_seq_in_init = ATOMIC_INIT(1); /* blocked as default */ void snd_seq_autoload_lock(void) { - snd_seq_in_init++; + atomic_inc(&snd_seq_in_init); } void snd_seq_autoload_unlock(void) { - snd_seq_in_init--; + atomic_dec(&snd_seq_in_init); } -#endif -void snd_seq_device_load_drivers(void) +static void autoload_drivers(void) { -#ifdef CONFIG_MODULES - struct ops_list *ops; + /* avoid reentrance */ + if (atomic_inc_return(&snd_seq_in_init) == 1) { + struct ops_list *ops; + + mutex_lock(&ops_mutex); + list_for_each_entry(ops, &opslist, list) { + if ((ops->driver & DRIVER_REQUESTING) && + !(ops->driver & DRIVER_REQUESTED)) { + ops->used++; + mutex_unlock(&ops_mutex); + ops->driver |= DRIVER_REQUESTED; + request_module("snd-%s", ops->id); + mutex_lock(&ops_mutex); + ops->used--; + } + } + mutex_unlock(&ops_mutex); + } + atomic_dec(&snd_seq_in_init); +} - /* Calling request_module during module_init() - * may cause blocking. - */ - if (snd_seq_in_init) - return; +static void call_autoload(struct work_struct *work) +{ + autoload_drivers(); +} - mutex_lock(&ops_mutex); - list_for_each_entry(ops, &opslist, list) { - if (! (ops->driver & DRIVER_LOADED) && - ! (ops->driver & DRIVER_REQUESTED)) { - ops->used++; - mutex_unlock(&ops_mutex); - ops->driver |= DRIVER_REQUESTED; - request_module("snd-%s", ops->id); - mutex_lock(&ops_mutex); - ops->used--; - } +static DECLARE_WORK(autoload_work, call_autoload); + +static void try_autoload(struct ops_list *ops) +{ + if (!ops->driver) { + ops->driver |= DRIVER_REQUESTING; + schedule_work(&autoload_work); } +} + +static void queue_autoload_drivers(void) +{ + struct ops_list *ops; + + mutex_lock(&ops_mutex); + list_for_each_entry(ops, &opslist, list) + try_autoload(ops); mutex_unlock(&ops_mutex); +} + +void snd_seq_autoload_init(void) +{ + atomic_dec(&snd_seq_in_init); +#ifdef CONFIG_SND_SEQUENCER_MODULE + /* initial autoload only when snd-seq is a module */ + queue_autoload_drivers(); +#endif +} +#else +#define try_autoload() /* NOP */ +#endif + +void snd_seq_device_load_drivers(void) +{ +#ifdef CONFIG_MODULES + queue_autoload_drivers(); + flush_work(&autoload_work); #endif } @@ -214,13 +255,14 @@ int snd_seq_device_new(struct snd_card *card, int device, char *id, int argsize, ops->num_devices++; mutex_unlock(&ops->reg_mutex); - unlock_driver(ops); - if ((err = snd_device_new(card, SNDRV_DEV_SEQUENCER, dev, &dops)) < 0) { snd_seq_device_free(dev); return err; } + try_autoload(ops); + unlock_driver(ops); + if (result) *result = dev; @@ -318,16 +360,12 @@ int snd_seq_device_register_driver(char *id, struct snd_seq_dev_ops *entry, entry->init_device == NULL || entry->free_device == NULL) return -EINVAL; - snd_seq_autoload_lock(); ops = find_driver(id, 1); - if (ops == NULL) { - snd_seq_autoload_unlock(); + if (ops == NULL) return -ENOMEM; - } if (ops->driver & DRIVER_LOADED) { pr_warn("ALSA: seq: driver_register: driver '%s' already exists\n", id); unlock_driver(ops); - snd_seq_autoload_unlock(); return -EBUSY; } @@ -344,7 +382,6 @@ int snd_seq_device_register_driver(char *id, struct snd_seq_dev_ops *entry, mutex_unlock(&ops->reg_mutex); unlock_driver(ops); - snd_seq_autoload_unlock(); return 0; } @@ -554,6 +591,9 @@ static int __init alsa_seq_device_init(void) static void __exit alsa_seq_device_exit(void) { +#ifdef CONFIG_MODULES + cancel_work_sync(&autoload_work); +#endif remove_drivers(); #ifdef CONFIG_PROC_FS snd_info_free_entry(info_entry); @@ -570,6 +610,7 @@ EXPORT_SYMBOL(snd_seq_device_new); EXPORT_SYMBOL(snd_seq_device_register_driver); EXPORT_SYMBOL(snd_seq_device_unregister_driver); #ifdef CONFIG_MODULES +EXPORT_SYMBOL(snd_seq_autoload_init); EXPORT_SYMBOL(snd_seq_autoload_lock); EXPORT_SYMBOL(snd_seq_autoload_unlock); #endif