2002-12-11 20:50:00

by Kees Bakker

[permalink] [raw]
Subject: 2.5.51 modprobe bttv hangs

With 2.5.50 and 2.5.51 I get a hanging modprobe if I try to load
bttv. I get all the normal messages from the modules, bt878 and
others, while it loads the modules. But then it hangs. Unfortunately I
haven't got much else to show you. The program does not want to terminate
with ^C. And also an lsmod while this happens is also going to hang.

I'm using module-init-tools-0.9.3. What do I do to get more debug output?
--
============================================================
Kees Bakker
The Tardis
Doorn, The Netherlands [email protected]
============================================================
A free society is one where it is safe to be unpopular


2002-12-12 00:31:23

by Rusty Russell

[permalink] [raw]
Subject: Re: 2.5.51 modprobe bttv hangs

In message <[email protected]> you write:
> With 2.5.50 and 2.5.51 I get a hanging modprobe if I try to load
> bttv. I get all the normal messages from the modules, bt878 and
> others, while it loads the modules. But then it hangs. Unfortunately I
> haven't got much else to show you. The program does not want to terminate
> with ^C. And also an lsmod while this happens is also going to hang.
>
> I'm using module-init-tools-0.9.3. What do I do to get more debug output?

Hi Kees,

Thanks for the report. Looks like bttv does a request module
from its init function. Please try the following fix and report back.

Thanks!
Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

Name: Module init reentry fix
Author: Rusty Russell
Status: Tested on 2.5.51

D: This changes the code to drop the module_mutex() before calling the
D: module's init function, so module init functions can call
D: request_module(). This was trivial before someone broke the module
D: code to start non-live. Now it requires us to keep info on the
D: exact module state.

diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .27854-linux-2.5.51/include/linux/module.h .27854-linux-2.5.51.updated/include/linux/module.h
--- .27854-linux-2.5.51/include/linux/module.h 2002-12-10 15:56:53.000000000 +1100
+++ .27854-linux-2.5.51.updated/include/linux/module.h 2002-12-12 11:31:28.000000000 +1100
@@ -116,10 +116,16 @@ struct module_ref
atomic_t count;
} ____cacheline_aligned;

+enum module_state
+{
+ MODULE_STATE_LIVE,
+ MODULE_STATE_COMING,
+ MODULE_STATE_GOING,
+};
+
struct module
{
- /* Am I live (yet)? */
- int live;
+ enum module_state state;

/* Member of list of modules */
struct list_head list;
@@ -177,6 +183,14 @@ struct module
char args[0];
};

+/* FIXME: It'd be nice to isolate modules during init, too, so they
+ aren't used before they (may) fail. But presently too much code
+ (IDE & SCSI) require entry into the module during init.*/
+static inline int module_is_live(struct module *mod)
+{
+ return mod->state != MODULE_STATE_GOING;
+}
+
#ifdef CONFIG_MODULE_UNLOAD

void __symbol_put(const char *symbol);
@@ -195,7 +209,7 @@ static inline int try_module_get(struct

if (module) {
unsigned int cpu = get_cpu();
- if (likely(module->live))
+ if (likely(module_is_live(module)))
local_inc(&module->ref[cpu].count);
else
ret = 0;
@@ -210,7 +224,7 @@ static inline void module_put(struct mod
unsigned int cpu = get_cpu();
local_dec(&module->ref[cpu].count);
/* Maybe they're waiting for us to drop reference? */
- if (unlikely(!module->live))
+ if (unlikely(!module_is_live(module)))
wake_up_process(module->waiter);
put_cpu();
}
@@ -219,7 +233,7 @@ static inline void module_put(struct mod
#else /*!CONFIG_MODULE_UNLOAD*/
static inline int try_module_get(struct module *module)
{
- return !module || module->live;
+ return !module || module_is_live(module);
}
static inline void module_put(struct module *module)
{
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .27854-linux-2.5.51/kernel/module.c .27854-linux-2.5.51.updated/kernel/module.c
--- .27854-linux-2.5.51/kernel/module.c 2002-12-10 15:56:54.000000000 +1100
+++ .27854-linux-2.5.51.updated/kernel/module.c 2002-12-12 11:31:28.000000000 +1100
@@ -44,6 +44,14 @@
static DECLARE_MUTEX(module_mutex);
LIST_HEAD(modules); /* FIXME: Accessed w/o lock on oops by some archs */

+/* We require a truly strong try_module_get() */
+static inline int strong_try_module_get(struct module *mod)
+{
+ if (mod && mod->state == MODULE_STATE_COMING)
+ return 0;
+ return try_module_get(mod);
+}
+
/* Convenient structure for holding init and core sizes */
struct sizes
{
@@ -378,7 +386,7 @@ sys_delete_module(const char *name_user,
}

/* Already dying? */
- if (!mod->live) {
+ if (mod->state == MODULE_STATE_GOING) {
/* FIXME: if (force), slam module count and wake up
waiter --RR */
DEBUGP("%s already dying\n", mod->name);
@@ -386,6 +394,16 @@ sys_delete_module(const char *name_user,
goto out;
}

+ /* Coming up? Allow force on stuck modules. */
+ if (mod->state == MODULE_STATE_COMING) {
+ forced = try_force(flags);
+ if (!forced) {
+ /* This module can't be removed */
+ ret = -EBUSY;
+ goto out;
+ }
+ }
+
if (!mod->exit || mod->unsafe) {
forced = try_force(flags);
if (!forced) {
@@ -407,7 +425,7 @@ sys_delete_module(const char *name_user,
ret = -EWOULDBLOCK;
} else {
mod->waiter = current;
- mod->live = 0;
+ mod->state = MODULE_STATE_GOING;
}
restart_refcounts();

@@ -507,7 +525,7 @@ static inline void module_unload_free(st

static inline int use_module(struct module *a, struct module *b)
{
- return try_module_get(b);
+ return strong_try_module_get(b);
}

static inline void module_unload_init(struct module *mod)
@@ -578,7 +596,7 @@ void *__symbol_get(const char *symbol)

spin_lock_irqsave(&modlist_lock, flags);
value = __find_symbol(symbol, &ksg);
- if (value && !try_module_get(ksg->owner))
+ if (value && !strong_try_module_get(ksg->owner))
value = 0;
spin_unlock_irqrestore(&modlist_lock, flags);

@@ -935,12 +953,8 @@ static struct module *load_module(void *
goto free_mod;
}

- /* Initialize the lists, since they will be list_del'd if init fails */
- INIT_LIST_HEAD(&mod->extable.list);
- INIT_LIST_HEAD(&mod->list);
- INIT_LIST_HEAD(&mod->symbols.list);
mod->symbols.owner = mod;
- mod->live = 0;
+ mod->state = MODULE_STATE_COMING;
module_unload_init(mod);

/* How much space will we need? (Common area in first) */
@@ -1097,51 +1111,40 @@ sys_init_module(void *umod,
flush_icache_range((unsigned long)mod->module_core,
(unsigned long)mod->module_core + mod->core_size);

- /* Now sew it into exception list (just in case...). */
+ /* Now sew it into the lists. They won't access us, since
+ strong_try_module_get() will fail. */
spin_lock_irq(&modlist_lock);
list_add(&mod->extable.list, &extables);
+ list_add_tail(&mod->symbols.list, &symbols);
spin_unlock_irq(&modlist_lock);
+ list_add(&mod->list, &modules);
+
+ /* Drop lock so they can recurse */
+ up(&module_mutex);

- /* Note, setting the mod->live to 1 here is safe because we haven't
- * linked the module into the system's kernel symbol table yet,
- * which means that the only way any other kernel code can call
- * into this module right now is if this module hands out entry
- * pointers to the other code. We assume that no module hands out
- * entry pointers to the rest of the kernel unless it is ready to
- * have them used.
- */
- mod->live = 1;
/* Start the module */
ret = mod->init ? mod->init() : 0;
if (ret < 0) {
/* Init routine failed: abort. Try to protect us from
buggy refcounters. */
+ mod->state = MODULE_STATE_GOING;
synchronize_kernel();
- if (mod->unsafe) {
+ if (mod->unsafe)
printk(KERN_ERR "%s: module is now stuck!\n",
mod->name);
- /* Mark it "live" so that they can force
- deletion later, and we don't keep getting
- woken on every decrement. */
- } else {
- mod->live = 0;
+ else {
+ down(&module_mutex);
free_module(mod);
+ up(&module_mutex);
}
- up(&module_mutex);
return ret;
}

/* Now it's a first class citizen! */
- spin_lock_irq(&modlist_lock);
- list_add_tail(&mod->symbols.list, &symbols);
- spin_unlock_irq(&modlist_lock);
- list_add(&mod->list, &modules);
-
+ mod->state = MODULE_STATE_LIVE;
module_free(mod, mod->module_init);
mod->module_init = NULL;

- /* All ok! */
- up(&module_mutex);
return 0;
}

2002-12-12 09:19:09

by Kees Bakker

[permalink] [raw]
Subject: Re: 2.5.51 modprobe bttv hangs

>>>>> "Rusty" == Rusty Russell <[email protected]> writes:

Rusty> Thanks for the report. Looks like bttv does a request module
Rusty> from its init function. Please try the following fix and report back.

Yep, now it loads my bttv modules.
Thanks. (Still have to try and see if xawtv works.)

FYI
iris:~# modprobe bttv
iris:~# lsmod
Module Size Used by
tuner 10671 1 [unsafe]
tvaudio 14049 1 [unsafe]
msp3400 17120 1 [unsafe]
bttv 85118 0
video_buf 13473 1 bttv [permanent]
i2c_algo_bit 8551 2 bttv [unsafe]
i2c_core 19367 5 tuner tvaudio msp3400 bttv i2c_algo_bit
ov511 75091 0
rtc 8637 0

Kees