John,
Ok, following patch (removing f_state and f_sem cruft) works
on all cases of driver/module internal loadable/external loadable
and handles manual unloads and demand reloads. I'll let the
one that put the f_state and f_sem cruft in explain why it was
so necessary.
I always find it interesting when code can be fixed by deleting
most of it...
I'm going with this.
--brian
Index: head/mod.c
===================================================================
RCS file: /home/common/cvsroot/LiSnew/head/mod.c,v
retrieving revision 1.1.1.4
diff -U5 -r1.1.1.4 mod.c
--- head/mod.c 22 Nov 2003 23:01:43 -0000 1.1.1.4
+++ head/mod.c 16 Feb 2004 06:04:27 -0000
@@ -451,11 +451,11 @@
if (list->npush < 0 || list->npush > MAXAPUSH)
return -1;
for (i = 0; i < list->npush; ++i) {
modID_t mod = list->mod[i];
- if (lis_fmod_sw[mod].f_state & LIS_MODSTATE_INITED)
+ if (lis_fmod_sw[mod].f_str != NULL)
continue; /* registered */
return -1; /* not registered or configured */
}
return 0;
}
@@ -635,11 +635,10 @@
modID_t
lis_register_strmod(struct streamtab *strtab, const char *name)
{
modID_t id = LIS_NULL_MID;
int inx ;
- int new_entry = 0 ;
if (name == NULL)
return LIS_NULL_MID;
/* See if the module is already registered */
@@ -656,14 +655,11 @@
else
{ /* not in the table */
/* Find a free id */
for (id = 1; id < lis_fmodcnt; ++id)
if (lis_fmod_sw[id].f_name[0] == 0)
- {
- new_entry = 1 ;
break;
- }
}
if (id == lis_fmodcnt) {
if (id == MAX_STRMOD) {
printk("Unable to register module \"%s\", "
@@ -673,19 +669,12 @@
lis_fmodcnt++;
}
lis_fmod_sw[id].f_str = strtab; /* always save strtab */
- if (!(lis_fmod_sw[id].f_state & LIS_MODSTATE_INITED))
- { /* do this once */
- lis_fmod_sw[id].f_count = 0;
- strncpy(lis_fmod_sw[id].f_name, name, LIS_NAMESZ);
- lis_sem_init(&lis_fmod_sw[id].f_sem, 1) ;
- if (new_entry && strtab) /* must be extl module, now loaded */
- lis_fmod_sw[id].f_state = LIS_MODSTATE_LOADED ;
- lis_fmod_sw[id].f_state |= LIS_MODSTATE_INITED ;
- }
+ lis_fmod_sw[id].f_count = 0;
+ strncpy(lis_fmod_sw[id].f_name, name, LIS_NAMESZ);
printk("STREAMS module \"%s\" registered%s, id %d\n",
lis_fmod_sw[id].f_name,
strtab == NULL ? " (loadable)" : "",
id);
@@ -697,14 +686,13 @@
/* unregister this module
*/
static int unregister_module(fmodsw_t *slot)
{
modID_t id;
- int err;
char name[LIS_NAMESZ + 1];
- if (!(slot->f_state & LIS_MODSTATE_INITED))
+ if (slot->f_str == NULL)
return(0) ; /* already done */
if (slot->f_count)
{
printk("LiS: unregister module \"%s\" f_count=%d -- busy\n",
@@ -713,37 +701,15 @@
}
strncpy(name, slot->f_name, LIS_NAMESZ);
id = slot - lis_fmod_sw;
- if ((err = lis_down(&slot->f_sem)) < 0)
- {
- printk("LiS: unregister module \"%s\" semaphore-error=%d\n",
- slot->f_name, err) ;
- return(err);
- }
-
- switch (slot->f_state & LIS_MODSTATE_MASK)
- {
- case LIS_MODSTATE_LINKED:
- case LIS_MODSTATE_LOADED:
- break ;
-
- case LIS_MODSTATE_LOADING:
- lis_up(&slot->f_sem) ;
- printk("LiS: unregister module \"%s\" -- loading\n", slot->f_name) ;
- return(-EBUSY) ;
-
- case LIS_MODSTATE_UNLOADED:
- slot->f_str = NULL;
- break ;
- }
+ slot->f_str = NULL;
+ if (slot->f_objname[0] == '\0')
+ slot->f_name[0] = '\0';
apush_drop_mod(id);
- lis_up(&slot->f_sem) ;
- lis_sem_destroy(&slot->f_sem) ;
- slot->f_state &= ~LIS_MODSTATE_INITED ;
printk("STREAMS module \"%s\" unregistered, id %d\n", name, id);
return 0;
}
@@ -775,94 +741,69 @@
return(LIS_NULL_MID) ;
/* Look for the module */
for (id = lis_fmodcnt; id > 0; id--)
if (!strcmp(lis_fmod_sw[id].f_name, name))
- break ;
+ return id; /* found it */
- return (id > 0 ? id : LIS_NULL_MID);
+ return LIS_NULL_MID;
} /* lis_findmod */
/* ------------------------------------------------------------------- */
/* Load module by name into lis_fmod_sw[]
*/
modID_t
lis_loadmod(const char *name)
{
- int id = lis_findmod(name);
- int err;
+ int id;
int configured;
const char *objname;
#ifdef LIS_LOADABLE_SUPPORT
char req[LIS_NAMESZ + 10];
#endif
+ id = lis_findmod(name);
- if (id == LIS_NULL_MID)
- return LIS_NULL_MID;
+ if (id != LIS_NULL_MID && lis_fmod_sw[id].f_str != NULL)
+ return id;
/* Find object name of this module */
objname = name; /* default objname */
configured = 0;
- if (lis_fmod_sw[id].f_objname[0])
+ if (id != LIS_NULL_MID && lis_fmod_sw[id].f_objname[0])
{
objname = lis_fmod_sw[id].f_objname ;
configured = 1;
}
- if ((err = lis_down(&lis_fmod_sw[id].f_sem)) < 0)
- return(LIS_NULL_MID);
-
- switch (lis_fmod_sw[id].f_state & LIS_MODSTATE_MASK)
- {
- case LIS_MODSTATE_LINKED:
- case LIS_MODSTATE_LOADED:
- lis_up(&lis_fmod_sw[id].f_sem) ;
- return id ; /* found and loaded */
-
- case LIS_MODSTATE_UNLOADED: /* needs loading */
- break ;
-
- case LIS_MODSTATE_LOADING:
- printk("Bad module state for %s (LOADING)\n", name) ;
- lis_up(&lis_fmod_sw[id].f_sem) ;
- return(LIS_NULL_MID);
- }
-
#ifdef LIS_LOADABLE_SUPPORT
/* Ask kerneld to load the module.
* When the module loads it will call lis_register_strmod()
* to register itself and will then acquire an available
* slot in the fmod_sw table. So if the module can now
* be found the load succeeded, otherwise not.
*/
sprintf(req, "streams-%s", objname);
- lis_fmod_sw[id].f_state &= ~LIS_MODSTATE_MASK ;
- lis_fmod_sw[id].f_state |= LIS_MODSTATE_LOADING ;
- if (request_module(req) < 0 || lis_fmod_sw[id].f_str == NULL)
+ request_module(req);
+
+ id = lis_findmod(name);
+ if (id == LIS_NULL_MID || lis_fmod_sw[id].f_str == NULL)
{
if (configured)
printk("Unable to demand load LiS objname %s, "
"STREAMS module %s\n ",
objname, (name) ? name : "(null)");
else
printk("Unable to demand load STREAMS module %s\n",
(name) ? name : "(null)");
- lis_fmod_sw[id].f_state &= ~LIS_MODSTATE_MASK ;
- lis_fmod_sw[id].f_state |= LIS_MODSTATE_UNLOADED ;
- lis_up(&lis_fmod_sw[id].f_sem) ;
return LIS_NULL_MID;
}
-
- lis_fmod_sw[id].f_state &= ~LIS_MODSTATE_MASK ;
- lis_fmod_sw[id].f_state |= LIS_MODSTATE_LOADED ;
#else
printk("Cannot load %s, LIS_LOADABLE_SUPPORT not set\n", name) ;
id = LIS_NULL_MID;
#endif
- lis_up(&lis_fmod_sw[id].f_sem) ;
return id;
} /* lis_loadmod */
/* ------------------------------------------------------------------- */
void
@@ -1274,21 +1215,12 @@
printk("Failed to register module \"%s\".\n",
lis_module_config[i].cnf_name);
continue ;
}
- /*
- * If the module has an object file name then it is
- * loadable, otherwise it is linked in with LiS.
- */
strncpy(lis_fmod_sw[modid].f_objname,
lis_module_config[i].cnf_objname, LIS_NAMESZ) ;
- lis_fmod_sw[i].f_state &= ~LIS_MODSTATE_MASK ;
- if (lis_fmod_sw[modid].f_objname[0])
- lis_fmod_sw[modid].f_state |= LIS_MODSTATE_UNLOADED ;
- else
- lis_fmod_sw[modid].f_state |= LIS_MODSTATE_LINKED ;
}
for (i = 0; i < DRV_CONFIG_SIZE; ++i) {
for (j = 0; j < lis_driver_config[i].cnf_n_majors; j++) {
if (lis_register_strdev(lis_driver_config[i].cnf_major[j],
Index: include/sys/LiS/mod.h
===================================================================
RCS file: /home/common/cvsroot/LiSnew/include/sys/LiS/mod.h,v
retrieving revision 1.1.1.3
diff -U5 -r1.1.1.3 mod.h
--- include/sys/LiS/mod.h 22 Nov 2003 23:02:25 -0000 1.1.1.3
+++ include/sys/LiS/mod.h 16 Feb 2004 05:50:20 -0000
@@ -151,28 +151,15 @@
struct streamtab *f_str;
ushort f_count; /* open count */
short f_flags; /* module/driver flags */
char f_name[LIS_NAMESZ+1];
char f_objname[LIS_NAMESZ+1];
- int f_state; /* state of module */
- lis_semaphore_t f_sem; /* to synchronize loading */
} fmodsw_t;
#define LIS_MODFLG_CLONE 0x0001 /* module is 'clone' */
#define LIS_MODFLG_FIFO 0x0002 /* module is 'fifo' */
#define LIS_MODFLG_REOPEN 0x0004 /* cloned minors can reopen */
-
-/*
- * States
- */
-#define LIS_MODSTATE_MASK 0x0FF /* isolates state variable */
-#define LIS_MODSTATE_LINKED 0 /* linked in with LiS */
-#define LIS_MODSTATE_UNLOADED 1 /* not loaded */
-#define LIS_MODSTATE_LOADING 2 /* loading */
-#define LIS_MODSTATE_LOADED 3 /* loaded */
-
-#define LIS_MODSTATE_INITED 0x100 /* initialized */
#endif /* __KERNEL__ */
/* ------------------------------------------------------------------- */
/* Glob. Vars. */
On Sun, 15 Feb 2004, Brian F. G. Bidulock wrote:
> John,
>
> Well, its really external loadable modules.
>
> I_PUSH calls lis_loadmod which calls lis_findmod which doesn't
> find anything on name and returns LIS_NULL_MODID which returns
> from lis_loadmod with LIS_NULL_MODID which fails in I_PUSH.
>
> I didn't have any problems with LiS included loadable modules
> such as timod.
>
> The above has not worked since 2.16.15 where lines such as:
>
> if (id == LIS_NULL_MODID)
> return LIS_NULL_MODID;
>
> were added to the top of lis_loadmod.
>
> I have also found that if f_name is not nulled, problems
> ensue: lis_register_strmod looks for existing registered
> modules on name first, and then never sets new_entry which
> results in incorrect state.
>
> I have also found that in unregister_module, f_str is never
> nulled. (It is only nulled for the non-existent case where
> f_state contains LIS_MODSTATE_UNLOADED, which should never
> occur.) The result is that once loaded, f_str is left as a
> hanging reference after module unloading and the module will
> not reload.
>
> There are so many bugs in this code I'm beginning to get angry
> about it. The thing is not going to work without significant
> rework. The original patch between 2.16.14 and 2.16.15 was a
> very poor one. It might work for LiS included modules but is
> completely busted for external modules. It was obviously
> never tested for external modules.
>
> Also, I don't see any need for the semaphore. Whoever calls
> request_module already holds the big kernel lock.
>
> --brian
>
>
> On Sun, 15 Feb 2004, John A. Boyd Jr. wrote:
>
> > Let me try to synchronize my recollection with yours.
> >
> > I submitted patches to Dave in response to brokenness of module (not
> > driver) loading in 2.16.15, and the patched behavior was evident in
> > 2.16.16. I.e., my patches were thus not the source of the problems
> > you had, but were apparently a response to the same problem(s) you
> > had and just might not have completely resolved them. That's pretty
> > much consistent with what I first wrote about this issue last night,
> > i.e., that I tried to fix it but don't know if I did it completely
> > or not.
> >
> > But you say "in 2.16.15 forward, if the module is not found, it is never
> > loaded." That's not my recollection at all. What I found was that in
> > 2.16.15, if a module was not at first found to be loaded, it might
> > actually get loaded, but the attempt would return failure state/status,
> > thus falsely indicating that it didn't load. But since 2.16.16, my
> > tests have shown that module loading works OK, as far as my tests go
> > (I_PUSH/I_FIND). I just tested with 2.16.18, and I_PUSH/I_FIND still
> > work OK for me.
> >
> > As I recall, the person who reported the problem with 2.16.15 was
> > satisfied with the patched behavior of 2.16.16 as well.
> >
> > -John
> >
> > Brian F. G. Bidulock wrote:
> > > John,
> > >
> > > Well, whoever made changes between 2.16.14 and 2.16.15 broke module
> > > ("module" as opposed to "driver") demand loading altogether. Which
> > > is where I started having problems.
> > >
> > > In 2.16.14 if lis_findmod could not find a module by name and KMOD
> > > is configured, it would attempt to load module "streams-%s".
> > >
> > > In 2.16.15 forward, if the module is not found, it is never loaded.
> > >
> > > I'm patching back to the 2.16.14 lis_findmod. I'll send a patch in
> > > a little while.
> > >
> > > --brian
> > >
> > > On Sun, 15 Feb 2004, John A. Boyd Jr. wrote:
> > >
> > >
> > >>Let me restate in the form of a suggestion - instead of clearing
> > >>the name, you might want to look at how to get the state change
> > >>to work, so as to prevent this problem. The author apparently
> > >>intended clearing the _INITED state flag to have somewhat the
> > >>effect as your clearing of the name...
> > >>
> > >>-John
> > >>
> > >>Brian F. G. Bidulock wrote:
> > >>
> > >>>There is a module loading bug in head/mod.c When a module
> > >>>is unregistered the f_name component is left alloing lis_findmod
> > >>>to find it an lis_loadmod will attempt to lock a destroyed
> > >>>semaphore. The problem can be recreated by manually loading a
> > >>>streams kernel module (modprobe), unloading it (rmmod) and then
> > >>>demand loading it (e.g. I_PUSH). The result is a kernel oops.
> > >>>
> > >>>The patch below fixes the problem (but may break other things).
> > >>>
> > >>>--brian
> > >>>
> > >>>Index: head/mod.c
> > >>>===================================================================
> > >>>RCS file: /home/common/cvsroot/LiSnew/head/mod.c,v
> > >>>retrieving revision 1.1.1.4
> > >>>diff -U3 -r1.1.1.4 mod.c
> > >>>--- head/mod.c 22 Nov 2003 23:01:43 -0000 1.1.1.4
> > >>>+++ head/mod.c 15 Feb 2004 18:05:31 -0000
> > >>>@@ -742,6 +742,7 @@
> > >>> lis_up(&slot->f_sem) ;
> > >>> lis_sem_destroy(&slot->f_sem) ;
> > >>> slot->f_state &= ~LIS_MODSTATE_INITED ;
> > >>>+ slot->f_name[0] = '\0';
> > >>>
> > >>> printk("STREAMS module \"%s\" unregistered, id %d\n", name, id);
> > >>>
> > >>>
> > >>
> > >>_______________________________________________
> > >>Linux-streams mailing list
> > >>[EMAIL PROTECTED]
> > >>http://gsyc.escet.urjc.es/mailman/listinfo/linux-streams
> > >
> > >
> >
> > _______________________________________________
> > Linux-streams mailing list
> > [EMAIL PROTECTED]
> > http://gsyc.escet.urjc.es/mailman/listinfo/linux-streams
>
> --
> Brian F. G. Bidulock � The reasonable man adapts himself to the �
> [EMAIL PROTECTED] � world; the unreasonable one persists in �
> http://www.openss7.org/ � trying to adapt the world to himself. �
> � Therefore all progress depends on the �
> � unreasonable man. -- George Bernard Shaw �
> _______________________________________________
> Linux-streams mailing list
> [EMAIL PROTECTED]
> http://gsyc.escet.urjc.es/mailman/listinfo/linux-streams
--
Brian F. G. Bidulock � The reasonable man adapts himself to the �
[EMAIL PROTECTED] � world; the unreasonable one persists in �
http://www.openss7.org/ � trying to adapt the world to himself. �
� Therefore all progress depends on the �
� unreasonable man. -- George Bernard Shaw �
_______________________________________________
Linux-streams mailing list
[EMAIL PROTECTED]
http://gsyc.escet.urjc.es/mailman/listinfo/linux-streams
