* modprobe + request_module() deadlock
@ 2004-11-17 22:29 Johannes Stezenbach
2004-11-18 3:48 ` Rusty Russell
0 siblings, 1 reply; 18+ messages in thread
From: Johannes Stezenbach @ 2004-11-17 22:29 UTC (permalink / raw)
To: linux-kernel; +Cc: Gerd Knorr, Rusty Russell
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
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: modprobe + request_module() deadlock 2004-11-17 22:29 modprobe + request_module() deadlock Johannes Stezenbach @ 2004-11-18 3:48 ` Rusty Russell 2004-11-18 13:55 ` Johannes Stezenbach 0 siblings, 1 reply; 18+ messages in thread From: Rusty Russell @ 2004-11-18 3:48 UTC (permalink / raw) To: Johannes Stezenbach; +Cc: lkml - Kernel Mailing List, Gerd Knorr 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: modprobe + request_module() deadlock 2004-11-18 3:48 ` Rusty Russell @ 2004-11-18 13:55 ` Johannes Stezenbach 2004-11-18 19:05 ` Takashi Iwai 0 siblings, 1 reply; 18+ messages in thread From: Johannes Stezenbach @ 2004-11-18 13:55 UTC (permalink / raw) To: Rusty Russell; +Cc: lkml - Kernel Mailing List, Gerd Knorr 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 > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: modprobe + request_module() deadlock 2004-11-18 13:55 ` Johannes Stezenbach @ 2004-11-18 19:05 ` Takashi Iwai 2004-11-19 4:04 ` Alexander E. Patrakov 0 siblings, 1 reply; 18+ messages in thread From: Takashi Iwai @ 2004-11-18 19:05 UTC (permalink / raw) To: Johannes Stezenbach; +Cc: Rusty Russell, lkml - Kernel Mailing List, Gerd Knorr 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: modprobe + request_module() deadlock 2004-11-18 19:05 ` Takashi Iwai @ 2004-11-19 4:04 ` Alexander E. Patrakov 2004-11-19 11:10 ` Takashi Iwai 0 siblings, 1 reply; 18+ messages in thread From: Alexander E. Patrakov @ 2004-11-19 4:04 UTC (permalink / raw) To: linux-kernel 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: modprobe + request_module() deadlock 2004-11-19 4:04 ` Alexander E. Patrakov @ 2004-11-19 11:10 ` Takashi Iwai 2004-11-19 11:50 ` Gerd Knorr 0 siblings, 1 reply; 18+ messages in thread From: Takashi Iwai @ 2004-11-19 11:10 UTC (permalink / raw) To: Alexander E. Patrakov; +Cc: linux-kernel, Rusty Russell, Gerd Knorr 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: modprobe + request_module() deadlock 2004-11-19 11:10 ` Takashi Iwai @ 2004-11-19 11:50 ` Gerd Knorr 2004-11-19 12:42 ` Alexander E. Patrakov 2004-11-21 8:39 ` Rusty Russell 0 siblings, 2 replies; 18+ messages in thread From: Gerd Knorr @ 2004-11-19 11:50 UTC (permalink / raw) To: Takashi Iwai; +Cc: Alexander E. Patrakov, linux-kernel, Rusty Russell > > 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: modprobe + request_module() deadlock 2004-11-19 11:50 ` Gerd Knorr @ 2004-11-19 12:42 ` Alexander E. Patrakov 2004-11-21 8:39 ` Rusty Russell 1 sibling, 0 replies; 18+ messages in thread From: Alexander E. Patrakov @ 2004-11-19 12:42 UTC (permalink / raw) To: linux-kernel 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: modprobe + request_module() deadlock 2004-11-19 11:50 ` Gerd Knorr 2004-11-19 12:42 ` Alexander E. Patrakov @ 2004-11-21 8:39 ` Rusty Russell 2004-11-22 10:25 ` Gerd Knorr 1 sibling, 1 reply; 18+ messages in thread From: Rusty Russell @ 2004-11-21 8:39 UTC (permalink / raw) To: Gerd Knorr Cc: Takashi Iwai, Alexander E. Patrakov, lkml - Kernel Mailing List 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: modprobe + request_module() deadlock 2004-11-21 8:39 ` Rusty Russell @ 2004-11-22 10:25 ` Gerd Knorr 2004-11-22 14:16 ` Johannes Stezenbach 0 siblings, 1 reply; 18+ messages in thread From: Gerd Knorr @ 2004-11-22 10:25 UTC (permalink / raw) To: Rusty Russell Cc: Gerd Knorr, Takashi Iwai, Alexander E. Patrakov, lkml - Kernel Mailing List > > > 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) ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: modprobe + request_module() deadlock 2004-11-22 10:25 ` Gerd Knorr @ 2004-11-22 14:16 ` Johannes Stezenbach 2004-11-22 14:44 ` Gerd Knorr 0 siblings, 1 reply; 18+ messages in thread From: Johannes Stezenbach @ 2004-11-22 14:16 UTC (permalink / raw) To: Gerd Knorr Cc: Rusty Russell, Takashi Iwai, Alexander E. Patrakov, linux-kernel (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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: modprobe + request_module() deadlock 2004-11-22 14:16 ` Johannes Stezenbach @ 2004-11-22 14:44 ` Gerd Knorr 2004-11-22 15:36 ` Johannes Stezenbach 0 siblings, 1 reply; 18+ messages in thread From: Gerd Knorr @ 2004-11-22 14:44 UTC (permalink / raw) To: Johannes Stezenbach, Gerd Knorr, Rusty Russell, Takashi Iwai, Alexander E. Patrakov, linux-kernel > > 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: modprobe + request_module() deadlock 2004-11-22 14:44 ` Gerd Knorr @ 2004-11-22 15:36 ` Johannes Stezenbach 2004-11-22 16:52 ` Gerd Knorr 0 siblings, 1 reply; 18+ messages in thread From: Johannes Stezenbach @ 2004-11-22 15:36 UTC (permalink / raw) To: Gerd Knorr Cc: Johannes Stezenbach, Rusty Russell, Takashi Iwai, Alexander E. Patrakov, linux-kernel 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: modprobe + request_module() deadlock 2004-11-22 15:36 ` Johannes Stezenbach @ 2004-11-22 16:52 ` Gerd Knorr 2004-11-24 5:02 ` Rusty Russell 0 siblings, 1 reply; 18+ messages in thread From: Gerd Knorr @ 2004-11-22 16:52 UTC (permalink / raw) To: Johannes Stezenbach, Gerd Knorr, Johannes Stezenbach, Rusty Russell, Takashi Iwai, Alexander E. Patrakov, linux-kernel > > 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: modprobe + request_module() deadlock 2004-11-22 16:52 ` Gerd Knorr @ 2004-11-24 5:02 ` Rusty Russell 2004-11-24 12:11 ` Gerd Knorr 2004-11-25 16:03 ` Gerd Knorr 0 siblings, 2 replies; 18+ messages in thread From: Rusty Russell @ 2004-11-24 5:02 UTC (permalink / raw) To: Gerd Knorr Cc: Johannes Stezenbach, Johannes Stezenbach, Takashi Iwai, Alexander E. Patrakov, lkml - Kernel Mailing List 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(¬ify_mutex); + notifier_call_chain(&module_notify_list, state, mod); + up(¬ify_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(¬ify_mutex); - notifier_call_chain(&module_notify_list, MODULE_STATE_COMING, mod); - up(¬ify_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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: modprobe + request_module() deadlock 2004-11-24 5:02 ` Rusty Russell @ 2004-11-24 12:11 ` Gerd Knorr 2004-11-25 16:03 ` Gerd Knorr 1 sibling, 0 replies; 18+ messages in thread From: Gerd Knorr @ 2004-11-24 12:11 UTC (permalink / raw) To: Rusty Russell Cc: Gerd Knorr, Johannes Stezenbach, Johannes Stezenbach, Takashi Iwai, Alexander E. Patrakov, lkml - Kernel Mailing List 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: modprobe + request_module() deadlock 2004-11-24 5:02 ` Rusty Russell 2004-11-24 12:11 ` Gerd Knorr @ 2004-11-25 16:03 ` Gerd Knorr 2004-11-26 0:34 ` Rusty Russell 1 sibling, 1 reply; 18+ messages in thread From: Gerd Knorr @ 2004-11-25 16:03 UTC (permalink / raw) To: Rusty Russell Cc: Gerd Knorr, Johannes Stezenbach, Johannes Stezenbach, Takashi Iwai, Alexander E. Patrakov, lkml - Kernel Mailing List 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); ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: modprobe + request_module() deadlock 2004-11-25 16:03 ` Gerd Knorr @ 2004-11-26 0:34 ` Rusty Russell 0 siblings, 0 replies; 18+ messages in thread From: Rusty Russell @ 2004-11-26 0:34 UTC (permalink / raw) To: Gerd Knorr Cc: Gerd Knorr, Johannes Stezenbach, Johannes Stezenbach, Takashi Iwai, Alexander E. Patrakov, lkml - Kernel Mailing List 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2004-11-26 20:12 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2004-11-17 22:29 modprobe + request_module() deadlock Johannes Stezenbach 2004-11-18 3:48 ` Rusty Russell 2004-11-18 13:55 ` Johannes Stezenbach 2004-11-18 19:05 ` Takashi Iwai 2004-11-19 4:04 ` Alexander E. Patrakov 2004-11-19 11:10 ` Takashi Iwai 2004-11-19 11:50 ` Gerd Knorr 2004-11-19 12:42 ` Alexander E. Patrakov 2004-11-21 8:39 ` Rusty Russell 2004-11-22 10:25 ` Gerd Knorr 2004-11-22 14:16 ` Johannes Stezenbach 2004-11-22 14:44 ` Gerd Knorr 2004-11-22 15:36 ` Johannes Stezenbach 2004-11-22 16:52 ` Gerd Knorr 2004-11-24 5:02 ` Rusty Russell 2004-11-24 12:11 ` Gerd Knorr 2004-11-25 16:03 ` Gerd Knorr 2004-11-26 0:34 ` Rusty Russell
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).