modprobe + request_module() deadlock

modprobe + request_module() deadlock

[Johannes Stezenbach]

Hi,

it seems that modprobe in newer versions of module-init-tools
(here: 3.1-pre6) gets an exclusive lock on the module's .ko file:

                struct flock lock;
                lock.l_type = F_WRLCK;
                lock.l_whence = SEEK_SET;
                lock.l_start = 0;
                lock.l_len = 1;
                fcntl(fd, F_SETLKW, &lock);

This leads to a deadlock when the loaded module calls
request_module() in its module_init() function, to load
a module which in turn depends on the first module.

E.g. drivers/media/video/saa7134/saa7134-core.c calls
request_module("saa7134-empress") when it detected that
the card is a BMK MPEG2 encoder card.

Question: Is this a bug in modprobe or in saa7134-core.c?


Here are the two modprobe call stacks from SysRq-T:

Nov 16 22:38:33 abc kernel: modprobe      D DB9A2D54     0  7915   7334                     (NOTLB)
Nov 16 22:38:33 abc kernel: db9a2d68 00200082 00000000 db9a2d54 c011452a 00000001 db9a2d64 c0114304 
Nov 16 22:38:33 abc kernel: 00000864 86734aec 00000293 f73129e0 f7312b3c db9a2e24 db9a2000 db9a2000 
Nov 16 22:38:33 abc kernel: db9a2dbc c039a2c7 00000000 f73129e0 c0114d5e 00000000 00000000 00001230 
Nov 16 22:38:33 abc kernel: Call Trace:
Nov 16 22:38:33 abc kernel: [<c039a2c7>] wait_for_completion+0x85/0xd9
Nov 16 22:38:33 abc kernel: [<c0125fcc>] call_usermodehelper+0x90/0x98
Nov 16 22:38:33 abc kernel: [<c0125d90>] request_module+0x88/0xca
Nov 16 22:38:33 abc kernel: [<f8dd8745>] saa7134_initdev+0x63a/0x785 [saa7134]
Nov 16 22:38:33 abc kernel: [<c020a92a>] pci_device_probe_static+0x49/0x58
Nov 16 22:38:33 abc kernel: [<c020a96c>] __pci_device_probe+0x33/0x43
Nov 16 22:38:33 abc kernel: [<c020a9a2>] pci_device_probe+0x26/0x42
Nov 16 22:38:33 abc kernel: [<c024546a>] driver_probe_device+0x2c/0x62
Nov 16 22:38:33 abc kernel: [<c0245582>] driver_attach+0x53/0x7d
Nov 16 22:38:33 abc kernel: [<c02459ec>] bus_add_driver+0x9c/0xc1
Nov 16 22:38:33 abc kernel: [<c0245f24>] driver_register+0x2b/0x2f
Nov 16 22:38:33 abc kernel: [<c020ab96>] pci_register_driver+0x5f/0x7b
Nov 16 22:38:33 abc kernel: [<c012dc96>] sys_init_module+0x18a/0x23e
Nov 16 22:38:33 abc kernel: [<c0102413>] syscall_call+0x7/0xb

Nov 16 22:38:33 abc kernel: modprobe      S F72B8324     0  7989   7988                     (NOTLB)
Nov 16 22:38:33 abc kernel: db99cf14 00000086 00000003 f72b8324 00000246 f72b8324 8672a32e 00000293 
Nov 16 22:38:33 abc kernel: 001ea8f3 8672a32e 00000293 dc6b2a60 dc6b2bbc f70bce34 f70bce50 db99c000 
Nov 16 22:38:33 abc kernel: db99cf90 c015fcfd c015e321 db99c000 00000000 fffffff5 f72b8324 00000007 
Nov 16 22:38:33 abc kernel: Call Trace:
Nov 16 22:38:33 abc kernel: [<c015fcfd>] fcntl_setlk+0x202/0x283
Nov 16 22:38:33 abc kernel: [<c015bdfc>] do_fcntl+0xd5/0x15e
Nov 16 22:38:33 abc kernel: [<c015bf34>] sys_fcntl64+0x63/0x6f
Nov 16 22:38:33 abc kernel: [<c0102413>] syscall_call+0x7/0xb

# ps -p 7915 -o pid,args
  PID COMMAND
 7915 /sbin/modprobe -s -q saa7134
# ps -p 7989 -o pid,args
  PID COMMAND
 7989 /sbin/modprobe -q -- saa7134_empress


Johannes

Re: modprobe + request_module() deadlock

[Rusty Russell]

On Wed, 2004-11-17 at 23:29 +0100, Johannes Stezenbach wrote:
> Hi,
> 
> it seems that modprobe in newer versions of module-init-tools
> (here: 3.1-pre6) gets an exclusive lock on the module's .ko file:
> 
>                 struct flock lock;
>                 lock.l_type = F_WRLCK;
>                 lock.l_whence = SEEK_SET;
>                 lock.l_start = 0;
>                 lock.l_len = 1;
>                 fcntl(fd, F_SETLKW, &lock);
> 
> This leads to a deadlock when the loaded module calls
> request_module() in its module_init() function, to load
> a module which in turn depends on the first module.

My bug, I think.  Does this help?

Rusty.

--- modprobe.c.~70~	2004-09-30 20:16:19.000000000 +1000
+++ modprobe.c	2004-11-18 14:44:57.000000000 +1100
@@ -735,6 +735,11 @@
 		       strip_vermagic, strip_modversion);
 	}
 
+	/* Don't do ANYTHING if already in kernel. */
+	if (!ignore_proc
+	    && module_in_kernel(newname ?: mod->modname, NULL) == 1)
+		goto exists_error;
+
 	fd = lock_file(mod->filename);
 	if (fd < 0) {
 		error("Could not open '%s': %s\n",
@@ -742,11 +747,6 @@
 		goto out_optstring;
 	}
 
-	/* Don't do ANYTHING if already in kernel. */
-	if (!ignore_proc
-	    && module_in_kernel(newname ?: mod->modname, NULL) == 1)
-		goto exists_error;
-
 	command = find_command(mod->modname, commands);
 	if (command && !ignore_commands) {
 		/* It might recurse: unlock. */
@@ -799,7 +799,7 @@
 	if (first_time)
 		error("Module %s already in kernel.\n",
 		      newname ?: mod->modname);
-	goto out_unlock;
+	goto out_optstring;
 }
 
 /* Do recursive removal. */

-- 
A bad analogy is like a leaky screwdriver -- Richard Braakman

Re: modprobe + request_module() deadlock

[Johannes Stezenbach]

On Thu, Nov 18, 2004 at 02:48:22PM +1100, Rusty Russell wrote:
> On Wed, 2004-11-17 at 23:29 +0100, Johannes Stezenbach wrote:
> > Hi,
> > 
> > it seems that modprobe in newer versions of module-init-tools
> > (here: 3.1-pre6) gets an exclusive lock on the module's .ko file:
> > 
> >                 struct flock lock;
> >                 lock.l_type = F_WRLCK;
> >                 lock.l_whence = SEEK_SET;
> >                 lock.l_start = 0;
> >                 lock.l_len = 1;
> >                 fcntl(fd, F_SETLKW, &lock);
> > 
> > This leads to a deadlock when the loaded module calls
> > request_module() in its module_init() function, to load
> > a module which in turn depends on the first module.
> 
> My bug, I think.  Does this help?

Yes and no. The deadlock is gone, but now I get:

Nov 18 14:30:03 abc kernel: saa7130/34: v4l2 driver version 0.2.12 loaded
Nov 18 14:30:03 abc kernel: saa7134[0]: found at 0000:02:0c.0, rev: 1, irq: 20, latency: 64, mmio: 0xfeafec00
Nov 18 14:30:03 abc kernel: saa7134[0]: subsystem: 1131:6752, board: BMK MPEX Tuner [card=23,insmod option]
Nov 18 14:30:03 abc kernel: saa7134[0]: board init: gpio is 10000
Nov 18 14:30:04 abc kernel: saa7134[0]: i2c eeprom 00: 31 11 52 67 06 80 00 01 00 00 00 00 00 00 01 00
Nov 18 14:30:04 abc kernel: saa7134[0]: i2c eeprom 10: 00 ff c2 0f ff ff ff ff ff ff ff ff ff ff ff ff
Nov 18 14:30:04 abc kernel: saa7134[0]: i2c eeprom 20: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
Nov 18 14:30:04 abc kernel: saa7134[0]: i2c eeprom 30: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
Nov 18 14:30:04 abc kernel: saa7134[0]/irq[10,91590999]: r=0x4000 s=0x00 GPIO16?
Nov 18 14:30:04 abc kernel: saa7134[0]/irq: looping -- clearing all enable bits

(Maybe Gerd has an idea what's wrong with the irq.
BTW, Documentation/video4linux/CARDLIST.saa7134 is out of date wrt
drivers/media/video/saa7134/saa7134.h)

Nov 18 14:30:04 abc kernel: tuner: chip found at addr 0xc0 i2c-bus saa7134[0]
Nov 18 14:30:04 abc kernel: tuner: type set to 5 (Philips PAL_BG (FI1216 and compatibles)) by saa7134[0]
Nov 18 14:30:04 abc udev[16759]: configured rule in '/etc/udev/rules.d/compat-full.rules' at line 29 applied, added symlNov 18 14:30:04 abc udev[16759]: configured rule in '/etc/udev/rules.d/devfs.rules' at line 70 applied, 'i2c-2' becomes Nov 18 14:30:04 abc udev[16759]: creating device node '/dev/i2c/2'
Nov 18 14:30:04 abc kernel: saa7134_empress: Unknown symbol saa7134_devlist
Nov 18 14:30:04 abc kernel: saa7134_empress: Unknown symbol saa7134_common_ioctl
Nov 18 14:30:04 abc kernel: saa7134_empress: Unknown symbol saa7134_boards
Nov 18 14:30:04 abc kernel: saa7134_empress: Unknown symbol saa7134_ts_register
Nov 18 14:30:04 abc kernel: saa7134_empress: Unknown symbol saa7134_print_ioctl
Nov 18 14:30:04 abc kernel: saa7134_empress: Unknown symbol saa7134_ts_qops
Nov 18 14:30:04 abc kernel: saa7134_empress: Unknown symbol saa7134_i2c_call_clients
Nov 18 14:30:04 abc kernel: saa7134_empress: Unknown symbol saa7134_ts_unregister
Nov 18 14:30:04 abc modprobe: FATAL: Error inserting saa7134_empress (/lib/modules/2.6.10-rc2/kernel/drivers/media/video/saa7134/saa7134-empress.ko): Unknown symbol in module, or unknown parameter (see dmesg)

However, the symbols are exported:

$ grep EXPORT_SYMBOL *.c
saa7134-core.c:EXPORT_SYMBOL(saa7134_ts_register);
saa7134-core.c:EXPORT_SYMBOL(saa7134_ts_unregister);
saa7134-core.c:EXPORT_SYMBOL(saa7134_print_ioctl);
saa7134-core.c:EXPORT_SYMBOL(saa7134_i2c_call_clients);
saa7134-core.c:EXPORT_SYMBOL(saa7134_devlist);
saa7134-core.c:EXPORT_SYMBOL(saa7134_boards);
saa7134-ts.c:EXPORT_SYMBOL_GPL(saa7134_ts_qops);
saa7134-video.c:EXPORT_SYMBOL(saa7134_common_ioctl);

If I 'modprobe saa7134-empress' manually after saa7134.ko is loaded, it works.

This is an unpatched 2.6.10-rc2 kernel, BTW.


Johannes

> --- modprobe.c.~70~	2004-09-30 20:16:19.000000000 +1000
> +++ modprobe.c	2004-11-18 14:44:57.000000000 +1100
> @@ -735,6 +735,11 @@
>  		       strip_vermagic, strip_modversion);
>  	}
>  
> +	/* Don't do ANYTHING if already in kernel. */
> +	if (!ignore_proc
> +	    && module_in_kernel(newname ?: mod->modname, NULL) == 1)
> +		goto exists_error;
> +
>  	fd = lock_file(mod->filename);
>  	if (fd < 0) {
>  		error("Could not open '%s': %s\n",
> @@ -742,11 +747,6 @@
>  		goto out_optstring;
>  	}
>  
> -	/* Don't do ANYTHING if already in kernel. */
> -	if (!ignore_proc
> -	    && module_in_kernel(newname ?: mod->modname, NULL) == 1)
> -		goto exists_error;
> -
>  	command = find_command(mod->modname, commands);
>  	if (command && !ignore_commands) {
>  		/* It might recurse: unlock. */
> @@ -799,7 +799,7 @@
>  	if (first_time)
>  		error("Module %s already in kernel.\n",
>  		      newname ?: mod->modname);
> -	goto out_unlock;
> +	goto out_optstring;
>  }
>  
>  /* Do recursive removal. */
> 
> -- 
> A bad analogy is like a leaky screwdriver -- Richard Braakman
> 

Re: modprobe + request_module() deadlock

[Takashi Iwai]

At Thu, 18 Nov 2004 14:55:22 +0100,
Johannes Stezenbach wrote:
> 
> On Thu, Nov 18, 2004 at 02:48:22PM +1100, Rusty Russell wrote:
> > On Wed, 2004-11-17 at 23:29 +0100, Johannes Stezenbach wrote:
> > > Hi,
> > > 
> > > it seems that modprobe in newer versions of module-init-tools
> > > (here: 3.1-pre6) gets an exclusive lock on the module's .ko file:
> > > 
> > >                 struct flock lock;
> > >                 lock.l_type = F_WRLCK;
> > >                 lock.l_whence = SEEK_SET;
> > >                 lock.l_start = 0;
> > >                 lock.l_len = 1;
> > >                 fcntl(fd, F_SETLKW, &lock);
> > > 
> > > This leads to a deadlock when the loaded module calls
> > > request_module() in its module_init() function, to load
> > > a module which in turn depends on the first module.
> > 
> > My bug, I think.  Does this help?
> 
> Yes and no. The deadlock is gone, but now I get:

IIRC, request_module() in module_init() doesn't work any more in
general.

A weak linking like i2c doesn't provide the module dependency, so
modprobe can't load it by itself, so far.  I see the same problem in
ALSA snd-powermac module.

Can we add a module dependency manually somehow?

--
Takashi Iwai <tiwai@suse.de>		ALSA Developer - www.alsa-project.org

Re: modprobe + request_module() deadlock

[Alexander E. Patrakov]

Takashi Iwai wrote:

> Can we add a module dependency manually somehow?

Yes, recommend the user to add something like this to /etc/modprobe.conf:

install a modprobe -i a ; modprobe b ; true

But IMHO Rusty should provide a way to specify those "recommended" install
lines in the modules themselves (so that they finally go
to /lib/modules/`uname -r`/modules.alias or a similar autogenerated file),
like that's already done wih aliases and char-majors.

Also Greg KH said in the past that module autoloading (i.e. the non-empty
request_module() function) should be marked as obsolete (and therefore the
use of request_module() should be eliminated).

-- 
Alexander E. Patrakov

Re: modprobe + request_module() deadlock

[Takashi Iwai]

At Fri, 19 Nov 2004 09:04:16 +0500,
Alexander E. Patrakov wrote:
> 
> Takashi Iwai wrote:
> 
> > Can we add a module dependency manually somehow?
> 
> Yes, recommend the user to add something like this to /etc/modprobe.conf:
> 
> install a modprobe -i a ; modprobe b ; true
> 
> But IMHO Rusty should provide a way to specify those "recommended" install
> lines in the modules themselves (so that they finally go
> to /lib/modules/`uname -r`/modules.alias or a similar autogenerated file),
> like that's already done wih aliases and char-majors.

Well, how about to add a new marker in the driver code such as

	MODULE_DEPENDS_ON("somemodule");

so that depmod can pick it up?


Takashi

Re: modprobe + request_module() deadlock

[Gerd Knorr]

> > But IMHO Rusty should provide a way to specify those "recommended" install
> > lines in the modules themselves (so that they finally go
> > to /lib/modules/`uname -r`/modules.alias or a similar autogenerated file),
> > like that's already done wih aliases and char-majors.
> 
> Well, how about to add a new marker in the driver code such as
> 
> 	MODULE_DEPENDS_ON("somemodule");
> 
> so that depmod can pick it up?

Wouldn't work for me as this isn't static.  saa7134 has to look at the
hardware, then decide whenever it should load saa7134-empress,
saa7134-dvb or none of them.

On the other hand I don't depend on request_module() waiting for the
modprobe being finished.  So maybe we can solve that with a
request_module_async()?

  Gerd

Re: modprobe + request_module() deadlock

[Alexander E. Patrakov]

Gerd Knorr wrote:

>> Well, how about to add a new marker in the driver code such as
>> 
>> MODULE_DEPENDS_ON("somemodule");
>> 
>> so that depmod can pick it up?
> 
> Wouldn't work for me as this isn't static.  saa7134 has to look at the
> hardware, then decide whenever it should load saa7134-empress,
> saa7134-dvb or none of them.
> 
> On the other hand I don't depend on request_module() waiting for the
> modprobe being finished.  So maybe we can solve that with a
> request_module_async()?

Unfortunately I am not an expert here, but wouldn't it be sufficient to
export in sysfs the information needed to distinguish between those two
saa7134-* modules, and then submit a new agent to linux-hotplug-devel? Then
hotplug will take care of loading the correct module (and hotplug is always
called asynchronously).

-- 
Alexander E. Patrakov

Re: modprobe + request_module() deadlock

[Rusty Russell]

On Fri, 2004-11-19 at 12:50 +0100, Gerd Knorr wrote:
> > > But IMHO Rusty should provide a way to specify those "recommended" install
> > > lines in the modules themselves (so that they finally go
> > > to /lib/modules/`uname -r`/modules.alias or a similar autogenerated file),
> > > like that's already done wih aliases and char-majors.
> > 
> > Well, how about to add a new marker in the driver code such as
> > 
> > 	MODULE_DEPENDS_ON("somemodule");
> > 
> > so that depmod can pick it up?
> 
> Wouldn't work for me as this isn't static.  saa7134 has to look at the
> hardware, then decide whenever it should load saa7134-empress,
> saa7134-dvb or none of them.
> 
> On the other hand I don't depend on request_module() waiting for the
> modprobe being finished.  So maybe we can solve that with a
> request_module_async()?

The problem is fairly simple: we don't let you get at the symbols from a
module which hasn't finished initializing yet.  This means that a
request_module() inside a module's init() will fail if the requested
module depends on this one.  async() will race with init() finishing, so
won't really help.

The traditional way to do this has been to have saa7134-empress do its
own probe, and likewise saa7134-dvb.  Unfortunately, these days modules
are not supposed to fail to load simply because there are no devices, so
wild module loading has a real cost.  Otherwise I'd be tempted to make
multiple aliases load *all* of them, and solve the problem that way.

Thoughts, anyone?
Rusty.
-- 
A bad analogy is like a leaky screwdriver -- Richard Braakman

Re: modprobe + request_module() deadlock

[Gerd Knorr]

> > > 	MODULE_DEPENDS_ON("somemodule");
> > 
> > On the other hand I don't depend on request_module() waiting for the
> > modprobe being finished.  So maybe we can solve that with a
> > request_module_async()?
> 
> The problem is fairly simple: we don't let you get at the symbols from a
> module which hasn't finished initializing yet.  This means that a
> request_module() inside a module's init() will fail if the requested
> module depends on this one.  async() will race with init() finishing, so
> won't really help.

Hmm, so module loading is not serialized by a lock?  I assumed that
caused the deadlock in $subject which started this thread ...

> The traditional way to do this has been to have saa7134-empress do its
> own probe, and likewise saa7134-dvb.

They can't actually probe themself.  It's _one_ PCI device (driven by
the saa7134 module) which can handle (among other v4l-related things)
the DMA transfer of mpeg streams.  That can be used in different ways
(or not at all) and the different use cases are handled by the
sub-modules.

So the way it is intended to work is that saa7134 has the pci table and
gets autoloaded by hotplug, it will have a look at the hardware and then
load either saa7134-empress or saa7134-dvb or none of them, so you'll
get everything nicely autoloaded.

> Unfortunately, these days modules are not supposed to fail to load
> simply because there are no devices, so wild module loading has a real
> cost.

Yes, thats exactly the problem.  And this is especially true for the
saa7134-dvb module as this one has a bunch of dependencies to other
modules of the dvb subsystem.  Thats why I try to avoid loading them
if they are not needed.

> Otherwise I'd be tempted to make multiple aliases load *all* of them,
> and solve the problem that way.

Well, that will actually work.  You can simply load both saa7134-empress
and saa7134-dvb, the saa7134 core module will take care that they can
only attach to devices they can actually handle.  Drawback is the memory
footprint ...

  Gerd

-- 
#define printk(args...) fprintf(stderr, ## args)

Re: modprobe + request_module() deadlock

[Johannes Stezenbach]

(someone dropped me off the Cc: list for this thread :-(, which is
doubly bad because I'm not subscribed to lkml :-(( )

Gerd Knorr wrote:
> > The traditional way to do this has been to have saa7134-empress do its
> > own probe, and likewise saa7134-dvb.
> 
> They can't actually probe themself.  It's _one_ PCI device (driven by
> the saa7134 module) which can handle (among other v4l-related things)
> the DMA transfer of mpeg streams.  That can be used in different ways
> (or not at all) and the different use cases are handled by the
> sub-modules.
> 
> So the way it is intended to work is that saa7134 has the pci table and
> gets autoloaded by hotplug, it will have a look at the hardware and then
> load either saa7134-empress or saa7134-dvb or none of them, so you'll
> get everything nicely autoloaded.

The saa7146 driver seems to have a working solution for this
problem: The PCI ids are registered to the subdrivers (e.g. dvb-ttpci
or mxb)  so that these are loaded via hotplug. They then register to the
saa7146 core as an "extension" module, and the core then does the probing.
Grep for saa7146_register_extension().

Johannes

Re: modprobe + request_module() deadlock

[Gerd Knorr]

> > They can't actually probe themself.  It's _one_ PCI device (driven by
> > the saa7134 module) which can handle (among other v4l-related things)
> > the DMA transfer of mpeg streams.  That can be used in different ways
> > (or not at all) and the different use cases are handled by the
> > sub-modules.
> > 
> > So the way it is intended to work is that saa7134 has the pci table and
> > gets autoloaded by hotplug, it will have a look at the hardware and then
> > load either saa7134-empress or saa7134-dvb or none of them, so you'll
> > get everything nicely autoloaded.
> 
> The saa7146 driver seems to have a working solution for this
> problem: The PCI ids are registered to the subdrivers (e.g. dvb-ttpci
> or mxb)  so that these are loaded via hotplug. They then register to the
> saa7146 core as an "extension" module, and the core then does the probing.
> Grep for saa7146_register_extension().

That would be kida ugly because I'd need a dummy module then for the
cards which need neither saa7134-empress nor saa7134-dvb (which is true
for most of the existing cards btw).

I can fix that in the driver, by delaying the request_module() somehow
until the saa7134 module initialization is finished.  I don't think that
this is a good idea through as it looks like I'm not the only one with
that problem ...

  Gerd

Re: modprobe + request_module() deadlock

[Johannes Stezenbach]

On Mon, Nov 22, 2004 at 03:44:33PM +0100, Gerd Knorr wrote:
> > The saa7146 driver seems to have a working solution for this
> > problem: The PCI ids are registered to the subdrivers (e.g. dvb-ttpci
> > or mxb)  so that these are loaded via hotplug. They then register to the
> > saa7146 core as an "extension" module, and the core then does the probing.
> > Grep for saa7146_register_extension().
> 
> That would be kida ugly because I'd need a dummy module then for the
> cards which need neither saa7134-empress nor saa7134-dvb (which is true
> for most of the existing cards btw).

You already have a saa7134-cards.c which you could turn
into a seperate module. I doubt users would care if they need
saa7134.o only or an additional module, if hotplug takes
care of loading them.

> I can fix that in the driver, by delaying the request_module() somehow
> until the saa7134 module initialization is finished.  I don't think that
> this is a good idea through as it looks like I'm not the only one with
> that problem ...

Delaying request_module() sounds ugly. Anyway, if you can
get it to work reliably...

Actually dvb-bt8xx.ko has a similar problem (no hotplug for DVB). It
uses bttv_sub_register(), though, but this doesn't do probing
and the PCI ids have to be in bttv-cards.c. It would be nicer
if dvb-bt8xx.ko could use a similar mechanism as dvb-ttpci.ko.
Or do you plan to add request_module("dvb-bt8xx") in bttv-driver.c?

And how about cx88 (I haven't checked this)?


Johannes

Re: modprobe + request_module() deadlock

[Gerd Knorr]

> > I can fix that in the driver, by delaying the request_module() somehow
> > until the saa7134 module initialization is finished.  I don't think that
> > this is a good idea through as it looks like I'm not the only one with
> > that problem ...
> 
> Delaying request_module() sounds ugly. Anyway, if you can
> get it to work reliably...

I think I can, havn't tried yet through.

> Actually dvb-bt8xx.ko has a similar problem (no hotplug for DVB). It
> uses bttv_sub_register(), though, but this doesn't do probing
> and the PCI ids have to be in bttv-cards.c. It would be nicer
> if dvb-bt8xx.ko could use a similar mechanism as dvb-ttpci.ko.

Well, you can use the second PCI function.

> Or do you plan to add request_module("dvb-bt8xx") in bttv-driver.c?

I can do that as well, bttv knows anyway which ones are dvb cards and
which ones are not.

> And how about cx88 (I haven't checked this)?

cx88-dvb has a PCI ID table.  The cx2388x has a separate PCI function
for the MPEG stuff which makes it a bit easier to get that handled
by hotplug directly ;)

  Gerd

Re: modprobe + request_module() deadlock

[Rusty Russell]

On Mon, 2004-11-22 at 17:52 +0100, Gerd Knorr wrote:
> > > I can fix that in the driver, by delaying the request_module() somehow
> > > until the saa7134 module initialization is finished.  I don't think that
> > > this is a good idea through as it looks like I'm not the only one with
> > > that problem ...
> > 
> > Delaying request_module() sounds ugly. Anyway, if you can
> > get it to work reliably...
> 
> I think I can, havn't tried yet through.

I still don't really like it as a solution, but this patch (pending
anyway) should help.

Rusty.

Name: More Module Notifiers
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Status: Trivial

Put in the rest of the module notifiers, esp. one when a module is
being removed.

Index: linux-2.6.10-rc2-bk8-Module/kernel/module.c
===================================================================
--- linux-2.6.10-rc2-bk8-Module.orig/kernel/module.c	2004-11-16 15:30:10.000000000 +1100
+++ linux-2.6.10-rc2-bk8-Module/kernel/module.c	2004-11-24 15:28:10.745436776 +1100
@@ -82,6 +82,13 @@
 }
 EXPORT_SYMBOL(unregister_module_notifier);
 
+static void module_notify(struct module *mod, enum module_state state)
+{
+	down(&notify_mutex);
+	notifier_call_chain(&module_notify_list, state, mod);
+	up(&notify_mutex);
+}
+
 /* We require a truly strong try_module_get() */
 static inline int strong_try_module_get(struct module *mod)
 {
@@ -579,6 +586,8 @@
 	if (ret != 0)
 		goto out;
 
+	module_notify(mod, MODULE_STATE_GOING);
+
 	/* Never wait if forced. */
 	if (!forced && module_refcount(mod) != 0)
 		wait_for_zero_refcount(mod);
@@ -589,6 +598,7 @@
 		mod->exit();
 		down(&module_mutex);
 	}
+	module_notify(mod, MODULE_STATE_GONE);
 	free_module(mod);
 
  out:
@@ -1792,9 +1802,7 @@
 	/* Drop lock so they can recurse */
 	up(&module_mutex);
 
-	down(&notify_mutex);
-	notifier_call_chain(&module_notify_list, MODULE_STATE_COMING, mod);
-	up(&notify_mutex);
+	module_notify(mod, MODULE_STATE_COMING);
 
 	/* Start the module */
 	if (mod->init != NULL)
@@ -1803,12 +1811,14 @@
 		/* Init routine failed: abort.  Try to protect us from
                    buggy refcounters. */
 		mod->state = MODULE_STATE_GOING;
+		module_notify(mod, MODULE_STATE_GOING);
 		synchronize_kernel();
 		if (mod->unsafe)
 			printk(KERN_ERR "%s: module is now stuck!\n",
 			       mod->name);
 		else {
 			module_put(mod);
+			module_notify(mod, MODULE_STATE_GONE);
 			down(&module_mutex);
 			free_module(mod);
 			up(&module_mutex);
@@ -1826,6 +1836,7 @@
 	mod->init_size = 0;
 	mod->init_text_size = 0;
 	up(&module_mutex);
+	module_notify(mod, MODULE_STATE_LIVE);
 
 	return 0;
 }
Index: linux-2.6.10-rc2-bk8-Module/include/linux/module.h
===================================================================
--- linux-2.6.10-rc2-bk8-Module.orig/include/linux/module.h	2004-11-16 15:30:07.000000000 +1100
+++ linux-2.6.10-rc2-bk8-Module/include/linux/module.h	2004-11-24 15:28:10.767433432 +1100
@@ -220,6 +220,7 @@
 	MODULE_STATE_LIVE,
 	MODULE_STATE_COMING,
 	MODULE_STATE_GOING,
+	MODULE_STATE_GONE, /* Only for notifier: module about to be freed */
 };
 
 /* Similar stuff for section attributes. */

-- 
A bad analogy is like a leaky screwdriver -- Richard Braakman

Re: modprobe + request_module() deadlock

[Gerd Knorr]

On Wed, Nov 24, 2004 at 04:02:31PM +1100, Rusty Russell wrote:
> On Mon, 2004-11-22 at 17:52 +0100, Gerd Knorr wrote:
> > > Delaying request_module() sounds ugly. Anyway, if you can
> > > get it to work reliably...
> > 
> > I think I can, havn't tried yet through.
> 
> I still don't really like it as a solution,

I don't either, but loading modules unconditionally even if not needed
(especially if they pull in the complete dvb subsystem) isn't very nice
as well.

I'm open to other suggestions ...

> but this patch (pending anyway) should help.

Thanks.

  Gerd

Re: modprobe + request_module() deadlock

[Gerd Knorr]

On Wed, Nov 24, 2004 at 04:02:31PM +1100, Rusty Russell wrote:
> On Mon, 2004-11-22 at 17:52 +0100, Gerd Knorr wrote:
> > > > I can fix that in the driver, by delaying the request_module() somehow
> > > > until the saa7134 module initialization is finished.  I don't think that
> > > > this is a good idea through as it looks like I'm not the only one with
> > > > that problem ...
> > > 
> > > Delaying request_module() sounds ugly. Anyway, if you can
> > > get it to work reliably...
> > 
> > I think I can, havn't tried yet through.

Untested proof-of-concept code (don't have a saa7134 card in my machine
at the moment), but that way it could work I think.  Tried to keep it
generic.  Basically it keeps a list of pending module loads and the
dependencies.  Then it hooks into the module state notifier chain and
calls request_module() once the depending module went to LIVE state.

Comments?

  Gerd

Index: saa7134-core.c
===================================================================
RCS file: /home/cvsroot/video4linux/saa7134-core.c,v
retrieving revision 1.20
diff -u -p -r1.20 saa7134-core.c
--- saa7134-core.c	23 Nov 2004 17:29:09 -0000	1.20
+++ saa7134-core.c	25 Nov 2004 15:54:23 -0000
@@ -233,6 +233,75 @@ static void dump_statusregs(struct saa71
 }
 #endif
 
+/* ----------------------------------------------------------- */
+
+struct pending_module {
+	struct module    *dep;
+	char             *name;
+	struct list_head next;
+};
+
+static int pending_call(struct notifier_block *self, unsigned long state,
+			void *module);
+
+static LIST_HEAD(pending_modules);
+static struct notifier_block pending_notifier = {
+	.notifier_call = pending_call,
+};
+
+static int pending_call(struct notifier_block *self, unsigned long state,
+			void *module)
+{
+	struct list_head *item;
+	struct pending_module *mod = NULL;
+
+	list_for_each(item,&pending_modules) {
+		mod = list_entry(item, struct pending_module, next);
+		if (mod->dep == module)
+			break;
+		mod = NULL;
+	}
+	if (NULL == mod)
+		return NOTIFY_DONE;
+
+	switch (state) {
+	case MODULE_STATE_LIVE:
+		request_module(mod->name);
+		/* fall through */
+	case MODULE_STATE_GOING:
+		list_del(&mod->next);
+		kfree(mod);
+		if (list_empty(&pending_modules))
+			unregister_module_notifier(&pending_notifier);
+		break;
+	}
+	return NOTIFY_DONE;
+}
+
+static void request_module_depend(struct module *dep, char *name)
+{
+	struct pending_module *mod;
+
+	switch (dep->state) {
+	case MODULE_STATE_COMING:
+		mod = kmalloc(sizeof(mod),GFP_KERNEL);
+		if (NULL == mod)
+			return;
+		mod->dep  = dep;
+		mod->name = name;
+		if (list_empty(&pending_modules))
+			register_module_notifier(&pending_notifier);
+		list_add(&mod->next,&pending_modules);
+		break;
+	case MODULE_STATE_LIVE:
+		request_module(name);
+		break;
+	default:
+		/* nothing */;
+		break;
+	}
+}
+
 /* ------------------------------------------------------------------ */
 
 /* nr of (saa7134-)pages for the given buffer size */
@@ -954,12 +1023,12 @@ static int __devinit saa7134_initdev(str
 		request_module("tuner");
 	if (dev->tda9887_conf)
 		request_module("tda9887");
-  	if (card_is_empress(dev)) {
-		request_module("saa7134-empress");
+  	if (1 /* card_is_empress(dev) */) {
+		request_module_depend(THIS_MODULE,"saa7134-empress");
 		request_module("saa6752hs");
 	}
-  	if (card_is_dvb(dev))
-		request_module("saa7134-dvb");
+  	if (1 /* card_is_dvb(dev) */)
+		request_module_depend(THIS_MODULE,"saa7134-dvb");
 
 	v4l2_prio_init(&dev->prio);
 

Re: modprobe + request_module() deadlock

[Rusty Russell]

On Thu, 2004-11-25 at 17:03 +0100, Gerd Knorr wrote:
> On Wed, Nov 24, 2004 at 04:02:31PM +1100, Rusty Russell wrote:
> > On Mon, 2004-11-22 at 17:52 +0100, Gerd Knorr wrote:
> > > > > I can fix that in the driver, by delaying the request_module() somehow
> > > > > until the saa7134 module initialization is finished.  I don't think that
> > > > > this is a good idea through as it looks like I'm not the only one with
> > > > > that problem ...
> > > > 
> > > > Delaying request_module() sounds ugly. Anyway, if you can
> > > > get it to work reliably...
> > > 
> > > I think I can, havn't tried yet through.
> 
> Untested proof-of-concept code (don't have a saa7134 card in my machine
> at the moment), but that way it could work I think.  Tried to keep it
> generic.  Basically it keeps a list of pending module loads and the
> dependencies.  Then it hooks into the module state notifier chain and
> calls request_module() once the depending module went to LIVE state.
> 
> Comments?

A little generic for my tastes.  I was thinking more like the below
(equally untested).  Note that strictly we should call the module
notifier for NULL at the end of the boot sequence, too.
===
static int want_empress, want_dvb;

/* These need our symbols: we must be fully loaded for them to load */
static int pending_call(struct notifier_block *self, unsigned long state,
			void *module)
{
	if (module != THIS_MODULE || state != MODULE_STATE_LIVE)
		return NOTIFY_DONE;

	if (want_empress)
		request_module("saa7134-empress");
	if (want_dvb)
		request_module("saa7134-dvb);
	return NOTIFY_DONE;
}

static struct notifier_block pending_notifier = {
	.notifier_call = pending_call,
};

int init(void)
{
,,,
	register_module_notifier(&pending_notifier);
}

void cleanup(void)
{
...
	unregister_module_notifier(&pending_notifier);
}


-- 
A bad analogy is like a leaky screwdriver -- Richard Braakman