seq module loading issue

seq module loading issue

[Adam Goode]

Hi,

I've observed a scenario in ALSA that I would like some feedback on
solving. On Linux, Chrome uses seq for WebMIDI. Eventually, there will be
hotplug detection code in Chrome that looks like this:

1. Connect to announce port
2. Listen for any announcements for client/port start/end
3. Send MIDIConnectionEvent events to JavaScript clients (
http://webaudio.github.io/web-midi-api/#midiconnectionevent-interface)

Here is the scenario where this fails:

1. Cold boot a machine with no MIDI devices plugged in
2. Start client, subscribe on the seq announce port
3. Insert USB MIDI device
4. snd_rawmidi module is loaded, triggered by insertion of MIDI device
5. snd_seq_midi is NOT loaded, but marked for loading later

At this point, there is no client start event sent to the announce port.
This is because of the deferred module loading, which won't trigger except
for a handful of operations (snd_seq_client_info_get_client is an example).

This is easily replicated with looking at aseqdump -p 0:1.


My question is: how to solve this? There are a few options:

1. Periodically poll for new clients. This is fairly ugly, but will work to
trigger module loading and won't result in latency once any device has been
plugged in, since announce will again work.

2. Somehow trigger module loading if there are any clients listening on the
announce port. This seems pretty tricky and requires keeping track of some
new state.

3. Do away with deferred module loading in seq. This is pretty invasive and
would increase the number of modules loaded, but would result in a lot of
deleted code in the kernel and a clean fix.

Number 3 is my preferred solution, but I wanted to see if these sorts of
patches would be acceptable, since it is invasive and would result in more
modules loaded.


Thanks for any comments.


Adam

Re: seq module loading issue

[Takashi Iwai]

At Wed, 8 Oct 2014 18:00:41 -0400,
Adam Goode wrote:
> 
> Hi,
> 
> I've observed a scenario in ALSA that I would like some feedback on
> solving. On Linux, Chrome uses seq for WebMIDI. Eventually, there will be
> hotplug detection code in Chrome that looks like this:
> 
> 1. Connect to announce port
> 2. Listen for any announcements for client/port start/end
> 3. Send MIDIConnectionEvent events to JavaScript clients (
> http://webaudio.github.io/web-midi-api/#midiconnectionevent-interface)
> 
> Here is the scenario where this fails:
> 
> 1. Cold boot a machine with no MIDI devices plugged in
> 2. Start client, subscribe on the seq announce port
> 3. Insert USB MIDI device
> 4. snd_rawmidi module is loaded, triggered by insertion of MIDI device
> 5. snd_seq_midi is NOT loaded, but marked for loading later
> 
> At this point, there is no client start event sent to the announce port.
> This is because of the deferred module loading, which won't trigger except
> for a handful of operations (snd_seq_client_info_get_client is an example).
> 
> This is easily replicated with looking at aseqdump -p 0:1.
> 
> 
> My question is: how to solve this? There are a few options:
> 
> 1. Periodically poll for new clients. This is fairly ugly, but will work to
> trigger module loading and won't result in latency once any device has been
> plugged in, since announce will again work.
> 
> 2. Somehow trigger module loading if there are any clients listening on the
> announce port. This seems pretty tricky and requires keeping track of some
> new state.
> 
> 3. Do away with deferred module loading in seq. This is pretty invasive and
> would increase the number of modules loaded, but would result in a lot of
> deleted code in the kernel and a clean fix.
> 
> Number 3 is my preferred solution, but I wanted to see if these sorts of
> patches would be acceptable, since it is invasive and would result in more
> modules loaded.

I don't like that option, it's too risky at this stage.  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.

How about a simple patch like below (totally untested)?


Takashi

---
diff --git a/sound/core/seq/seq_device.c b/sound/core/seq/seq_device.c
index 91a786a783e1..38e400bce205 100644
--- a/sound/core/seq/seq_device.c
+++ b/sound/core/seq/seq_device.c
@@ -51,6 +51,17 @@ MODULE_AUTHOR("Takashi Iwai <tiwai@suse.de>");
 MODULE_DESCRIPTION("ALSA sequencer device management");
 MODULE_LICENSE("GPL");
 
+static void call_autoload(struct work_struct *work)
+{
+	snd_seq_device_load_drivers();
+}
+
+static DECLARE_WORK(autoload_work, call_autoload);
+
+static bool autoload;
+module_param(autoload, bool, 0644);
+MODULE_PARM_DESC(autoload, "Load sequencer driver module automatically");
+
 /* driver state */
 #define DRIVER_EMPTY		0
 #define DRIVER_LOADED		(1<<0)
@@ -221,6 +232,8 @@ int snd_seq_device_new(struct snd_card *card, int device, char *id, int argsize,
 		return err;
 	}
 	
+	if (autoload)
+		schedule_work(&autoload_work);
 	if (result)
 		*result = dev;
 
@@ -554,6 +567,7 @@ static int __init alsa_seq_device_init(void)
 
 static void __exit alsa_seq_device_exit(void)
 {
+	cancel_work_sync(&autoload_work);
 	remove_drivers();
 #ifdef CONFIG_PROC_FS
 	snd_info_free_entry(info_entry);

Re: seq module loading issue

[Clemens Ladisch]

Takashi Iwai wrote:
> Adam Goode wrote:
>> 3. Do away with deferred module loading in seq. This is pretty invasive and
>> would increase the number of modules loaded, but would result in a lot of
>> deleted code in the kernel and a clean fix.

I've always wanted to do this (but this was very low priority while this
bug was not known).  The original purpose of the separate seq devices
was to save memory for people who want to only play MP3 files, but this
justification became meaningless long ago for desktop computers, and
embedded systems that care about memory could just disable the
sequencer.

>> Number 3 is my preferred solution, but I wanted to see if these sorts of
>> patches would be acceptable, since it is invasive and would result in more
>> modules loaded.
>
> I don't like that option, it's too risky at this stage.

"This stage" = "for 3.18"?

> 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).


Regards,
Clemens

Re: seq module loading issue

[Takashi Iwai]

At Thu, 09 Oct 2014 09:29:10 +0200,
Clemens Ladisch wrote:
> 
> Takashi Iwai wrote:
> > Adam Goode wrote:
> >> 3. Do away with deferred module loading in seq. This is pretty invasive and
> >> would increase the number of modules loaded, but would result in a lot of
> >> deleted code in the kernel and a clean fix.
> 
> I've always wanted to do this (but this was very low priority while this
> bug was not known).  The original purpose of the separate seq devices
> was to save memory for people who want to only play MP3 files, but this
> justification became meaningless long ago for desktop computers, and
> embedded systems that care about memory could just disable the
> sequencer.
> 
> >> Number 3 is my preferred solution, but I wanted to see if these sorts of
> >> patches would be acceptable, since it is invasive and would result in more
> >> modules loaded.
> >
> > I don't like that option, it's too risky at this stage.
> 
> "This stage" = "for 3.18"?
>
> > 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?
Some distros don't load sequencer modules nowadays as default.  So it
would result in a clear behavior change on the whole system.


Takashi

Re: seq module loading issue

[Clemens Ladisch]

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?

> 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.

Is the high risk of incompatibility that you mentioned this assumption,
or anything else?


Regards,
Clemens

Re: seq module loading issue

[Takashi Iwai]

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.


Takashi

Re: seq module loading issue

[Takashi Iwai]

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

Re: seq module loading issue

[Takashi Iwai]

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

Re: seq module loading issue

[Adam Goode]

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

Re: seq module loading issue

[Clemens Ladisch]

Adam Goode wrote:
> I've observed a scenario in ALSA that I would like some feedback on
> solving. On Linux, Chrome uses seq for WebMIDI. Eventually, there will be
> hotplug detection code in Chrome that looks like this:
>
> 1. Connect to announce port
> 2. Listen for any announcements for client/port start/end
> 3. Send MIDIConnectionEvent events to JavaScript clients (
> http://webaudio.github.io/web-midi-api/#midiconnectionevent-interface)
>
> Here is the scenario where this fails:
>
> 1. Cold boot a machine with no MIDI devices plugged in
> 2. Start client, subscribe on the seq announce port
> 3. Insert USB MIDI device
> 4. snd_rawmidi module is loaded, triggered by insertion of MIDI device
> 5. snd_seq_midi is NOT loaded, but marked for loading later
>
> At this point, there is no client start event sent to the announce port.

In practice, programs that listen to these announcements do so in order
to dynamically update their own list of MIDI clients/ports, and
enumerating those would have triggered autoloading.  This might be the
reason that this problem has not been reported so far.

I don't know about your implementation of the MIDIAccess interface, but
it looks as if enumeration is needed when the client accesses either the
"inputs" or "outputs" fields.  Couldn't you trigger the same enumeration
when the onconnect/ondisconnect event handlers are set?


Regards,
Clemens