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();
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' alreadyexists\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