Skip to contentSkip to conversation

Several Misc SCSI updates...

DISCUSSION Doug Ledford 3 participants 6 messages Nov 17, 2002 SCSI

lore
Share & tools
lore
#0 lore raw

These bring the scsi subsys up to the new module loader semantics. There is more work to be done on inter-module locking here, but we need to solve the whole module->live is 0 during init problem first or else it's a waste of time.

bk://linux-scsi.bkbits.net/scsi-dledford

[email protected], 2002-11-17 15:02:31-05:00, [email protected]
Merge bk://linux.bkbits.net/linux-2.5
into flossy.devel.redhat.com:/usr/local/home/dledford/bk/linus-2.5

[email protected], 2002-11-16 21:31:05-05:00, [email protected]
Christoph Hellwig posted a patch that conflicted with a lot of my own changes, so this is the merge of his work into my own.

[email protected], 2002-11-16 21:22:06-05:00, [email protected]
aic7xxx_old: fix check_region/request_region usage so that the module
may be loaded/unloaded/reloaded

[email protected], 2002-11-16 20:59:23-05:00, [email protected]
Update high level scsi drivers to use struct list_head in templates Update scsi.c for struct list_head in upper layer templates Update scsi.c for new module loader semantics

[email protected], 2002-11-16 14:51:42-05:00, [email protected]
scsi.c:
- Comment out GET_USE_COUNT, it's a bogus test anyway
- Don't panic on failure to allocate sg table slab slots, fail gracefully
- init_scsi() leaks all sorts of crap on failed module load

[email protected], 2002-11-11 18:02:55-05:00, [email protected]
mptscsih.h: compile fix

Signature · 4 lines
  Doug Ledford <[email protected]>     919-754-3700 x44233
         Red Hat, Inc. 
         1801 Varsity Dr.
         Raleigh, NC 27606
#1 lore raw
Quoted 5 lines · Doug Ledford

These bring the scsi subsys up to the new module loader semantics. There is more work to be done on inter-module locking here, but we need to solve the whole module->live is 0 during init problem first or else it's a waste of time.

Hey, just remove the "live" test, I think it's over-eager and likely to just cause extra code to work around it rather than fix anything.

Linus

#2 lore raw
Quoted 10 lines · Linus Torvalds
Quoted 5 lines · Doug Ledford

These bring the scsi subsys up to the new module loader semantics. There is more work to be done on inter-module locking here, but we need to solve the whole module->live is 0 during init problem first or else it's a waste of time.

Hey, just remove the "live" test, I think it's over-eager and likely to just cause extra code to work around it rather than fix anything.

Won't work. module->live is what Rusty uses to indicate that the module is in the process of unloading, which is when we *do* want the attempt to module_get() to fail. I think the process out to basically be:

load module into mem
set module->live = 1
call module_init
export module syms
done loading module

on module exit:
unexport module syms
set module->live 0
call module_exit
free module memory
done unloading.

That *should* solve all the races Rusty is trying to solve without the problems we've had so far, but this is only after a few minutes of thinking....

Signature · 4 lines
  Doug Ledford <[email protected]>     919-754-3700 x44233
         Red Hat, Inc. 
         1801 Varsity Dr.
         Raleigh, NC 27606
#3 lore raw
Quoted 4 lines · Doug Ledford

Won't work. module->live is what Rusty uses to indicate that the module is in the process of unloading, which is when we *do* want the attempt to module_get() to fail.

That's fine, as long as "module_get()" is the only thing that cares. Just make it go "live" early as you indicate, and everybody should be happy. I certainly agree that it should be illegal to do more module_get()'s once we've already started unloading..

Linus

#4 lore raw
Quoted 11 lines · Linus Torvalds
Quoted 4 lines · Doug Ledford

Won't work. module->live is what Rusty uses to indicate that the module is in the process of unloading, which is when we *do* want the attempt to module_get() to fail.

That's fine, as long as "module_get()" is the only thing that cares. Just make it go "live" early as you indicate, and everybody should be happy. I certainly agree that it should be illegal to do more module_get()'s once we've already started unloading..

On the unload side it's OK. module_get() also breaks during _init_ and that's the problem. IOW, you'll need to make every block device driver to set ->live manually. Smells like a wrong API...

#5 lore raw
Quoted 11 lines · Alexander Viro
Quoted 7 lines · Linus Torvalds
Quoted 4 lines · Doug Ledford

Won't work. module->live is what Rusty uses to indicate that the module is in the process of unloading, which is when we *do* want the attempt to module_get() to fail.

Quoted 8 lines
Quoted 4 lines

That's fine, as long as "module_get()" is the only thing that cares. Just make it go "live" early as you indicate, and everybody should be happy. I certainly agree that it should be illegal to do more module_get()'s once we've already started unloading..

On the unload side it's OK. module_get() also breaks during _init_ and that's the problem. IOW, you'll need to make every block device driver to set ->live manually. Smells like a wrong API...

This is the patch I just put into my tree (beware cut-n-paste breakage is in effect, but Linus should have the change in his tree momentarily anyway):

kernel/module.c Sun Nov 17 17:02:48 2002 +12 -3
diff -Nru a/kernel/module.c b/kernel/module.c
@@ -1058,6 +1058,15 @@
        list_add(&mod->extable.list, &extables);
        spin_unlock_irq(&modlist_lock);
 
+       /* 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) {
@@ -1070,9 +1079,10 @@
                        /* Mark it "live" so that they can force
                           deletion later, and we don't keep getting
                           woken on every decrement. */
-                       mod->live = 1;
-               } else
+               } else {
+                       mod->live = 0;
                        free_module(mod);
+               }
                up(&module_mutex);
                return ret;
        }
@@ -1087,7 +1097,6 @@
        mod->module_init = NULL;
 
        /* All ok! */
-       mod->live = 1;
        up(&module_mutex);
        return 0;
 }
Signature · 4 lines
  Doug Ledford <[email protected]>     919-754-3700 x44233
         Red Hat, Inc. 
         1801 Varsity Dr.
         Raleigh, NC 27606