Re: seq module loading issue

[Takashi Iwai <tiwai@suse.de>]

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