Re: seq module loading issue

[Takashi Iwai <tiwai@suse.de>]

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.


Takashi

--
diff --git a/include/sound/seq_kernel.h b/include/sound/seq_kernel.h
index 2398521f0998..24c1dc1523bb 100644
--- a/include/sound/seq_kernel.h
+++ b/include/sound/seq_kernel.h
@@ -106,9 +106,11 @@ int snd_seq_event_port_attach(int client, struct snd_seq_port_callback *pcbp,
 int snd_seq_event_port_detach(int client, int port);
 
 #ifdef CONFIG_MODULES
+void snd_seq_autoload_init(void);
 void snd_seq_autoload_lock(void);
 void snd_seq_autoload_unlock(void);
 #else
+#define snd_seq_autoload_init()
 #define snd_seq_autoload_lock()
 #define snd_seq_autoload_unlock()
 #endif
diff --git a/sound/core/seq/seq_clientmgr.c b/sound/core/seq/seq_clientmgr.c
index 225c73152ee9..92ce558cc9b6 100644
--- a/sound/core/seq/seq_clientmgr.c
+++ b/sound/core/seq/seq_clientmgr.c
@@ -2589,6 +2589,7 @@ int __init snd_sequencer_device_init(void)
 	
 	mutex_unlock(&register_mutex);
 
+	snd_seq_autoload_init();
 	return 0;
 }
 
diff --git a/sound/core/seq/seq_device.c b/sound/core/seq/seq_device.c
index 91a786a783e1..a77455c67f1d 100644
--- a/sound/core/seq/seq_device.c
+++ b/sound/core/seq/seq_device.c
@@ -127,7 +127,7 @@ 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 int snd_seq_in_init = 1; /* default blocked until cleared by seq core */
 void snd_seq_autoload_lock(void)
 {
 	snd_seq_in_init++;
@@ -137,6 +137,25 @@ void snd_seq_autoload_unlock(void)
 {
 	snd_seq_in_init--;
 }
+
+static void try_autoload(struct ops_list *ops)
+{
+	if (!snd_seq_in_init &&
+	    !(ops->driver & DRIVER_LOADED) &&
+	    !(ops->driver & DRIVER_REQUESTED)) {
+		ops->driver |= DRIVER_REQUESTED;
+		request_module("snd-%s", ops->id);
+	}
+}
+
+/* called by sequencer core */
+void snd_seq_autoload_init(void)
+{
+	snd_seq_in_init = 0;
+	snd_seq_device_load_drivers();
+}
+#else
+#define try_autoload(ops)
 #endif
 
 void snd_seq_device_load_drivers(void)
@@ -214,13 +233,15 @@ 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) {
+		unlock_driver(ops);
 		snd_seq_device_free(dev);
 		return err;
 	}
 	
+	try_autoload(ops);
+	unlock_driver(ops);
+
 	if (result)
 		*result = dev;
 
@@ -572,4 +593,5 @@ EXPORT_SYMBOL(snd_seq_device_unregister_driver);
 #ifdef CONFIG_MODULES
 EXPORT_SYMBOL(snd_seq_autoload_lock);
 EXPORT_SYMBOL(snd_seq_autoload_unlock);
+EXPORT_SYMBOL(snd_seq_autoload_init);
 #endif