Skip to contentSkip to conversation

mii module broken under new scheme

PATCH In review Matt Reppert 5 participants 8 messages Nov 19, 2002 – Dec 2, 2002

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 mii module broken under new scheme

drivers/net/mii.c doesn't export module init/cleanup functions. That means it can't be loaded under the new module scheme. This patch adds do-nothing functions for it, which allows it to load. (8139too depends on mii, so without this I don't have network.)

Matt

net/mii.c renamed from net/mii.c~no +16 -0
@@ -348,3 +348,19 @@
 EXPORT_SYMBOL(mii_check_media);
 EXPORT_SYMBOL(generic_mii_ioctl);
 
+static void __init mii_init_module (void)
+{
+#ifdef MODULE
+	printk (KERN_INFO "mii" "\n");
+#endif
+
+	return 0;
+}
+
+
+static void __exit mii_cleanup_module (void)
+{
+}
+
+module_init(mii_init_module);
+module_exit(mii_cleanup_module);
#1 lore raw
Quoted 4 lines · Matt Reppert

drivers/net/mii.c doesn't export module init/cleanup functions. That means it can't be loaded under the new module scheme. This patch adds do-nothing functions for it, which allows it to load. (8139too depends on mii, so without this I don't have network.)

ahhh! I was wondering what was up, but since I was busy with other things I just compiled it into the kernel and continued on my way.

That's a bug in the new module loader.

Jeff

#2 lore raw
Quoted 8 lines · On Tue, 19 Nov 2002 12:51:44 -0500 Jeff Garzik <[email protected]> wrote:
Quoted 5 lines · Matt Reppert

drivers/net/mii.c doesn't export module init/cleanup functions. That means it
can't be loaded under the new module scheme. This patch adds do-nothing functions for it, which allows it to load. (8139too depends on mii, so without this I don't have network.)

Quoted 4 lines

ahhh! I was wondering what was up, but since I was busy with other
things I just compiled it into the kernel and continued on my way.

That's a bug in the new module loader.

Not so sure I agree ... recompiled the kernel with debugging output in module.c and when I try to insert mii.o without above patch it complains "Module has no name!" and returns -ENOEXEC from the syscall. I think naming mii.o would be a good idea. This may not be the best way to do it, but it works. (Granted, I'm not terribly familiar with all the modules code changes yet, but ... ) Having anonymous output in lsmod would be somewhat confusing :) ("Well, whatever it is, 8139too needs it, don't touch it!")

Just noticed above patch is -p0. Whoops.

Matt

#3 lore raw
In reply to Jeff Garzik

In message <[email protected]> you write:

Quoted 8 lines
Quoted 5 lines · Matt Reppert

drivers/net/mii.c doesn't export module init/cleanup functions. That means it
can't be loaded under the new module scheme. This patch adds do-nothing functions for it, which allows it to load. (8139too depends on mii, so without this I don't have network.)

Quoted 4 lines

ahhh! I was wondering what was up, but since I was busy with other
things I just compiled it into the kernel and continued on my way.

That's a bug in the new module loader.

Yes. But the workaround of calling the module "unknown" isn't nice
either: just put "no_module_init;" in and be done with it (it's also a big hint that the module doesn't do anything itself).

Richard Henderson, Kai and myself are discussing a post-link stage for modules, which will allow us to add the .modname section at that time, but 99% of the cases are already fixed.

Sorry for any trouble,
Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

#4 lore raw
In reply to Matt Reppert
Quoted 17 lines · Matt Reppert
Quoted 11 lines · On Tue, 19 Nov 2002 12:51:44 -0500 Jeff Garzik wrote:
Quoted 2 lines · Matt Reppert

drivers/net/mii.c doesn't export module init/cleanup functions. That means it

ahhh! I was wondering what was up, but since I was busy with other
things I just compiled it into the kernel and continued on my way.

That's a bug in the new module loader.

Quoted 8 lines

Not so sure I agree ... recompiled the kernel with debugging output in module.c and when I try to insert mii.o without above patch it complains "Module has no name!" and returns -ENOEXEC from the syscall. I think naming mii.o would be a good idea. This may not be the best way to do it, but it works. (Granted, I'm not terribly familiar with all the modules code changes yet, but ... ) Having anonymous output in lsmod would be somewhat confusing :) ("Well, whatever it is, 8139too needs it, don't touch it!")

Your patch is fine to get people going... I'm glad you posted it on lkml for others to benefit.

However, for inclusion in the kernel, it is not needed. Quite simply, mii.c is essentially a library, and it needs absolutely no initialization nor cleanup at all. Thus, it is a bug in the module loader that any code changes at all are required.

There exists a no_module_init tag, which is in theory the proper fix for this problem under rusty's system, but that is itself a bug: it's redundant just like the silly EXPORT_NO_SYMBOLS tag -- it's stating the obvious. The module loader needs to notice a lack of init_module and exit_module and handle it accordingly.

Jeff

#5 lore raw
Quoted 4 lines · Jeff Garzik

However, for inclusion in the kernel, it is not needed. Quite simply, mii.c is essentially a library, and it needs absolutely no initialization nor cleanup at all. Thus, it is a bug in the module loader that any code changes at all are required.

I'm afraid it's not.

Quoted 5 lines

There exists a no_module_init tag, which is in theory the proper fix for this problem under rusty's system, but that is itself a bug: it's redundant just like the silly EXPORT_NO_SYMBOLS tag -- it's stating the obvious. The module loader needs to notice a lack of init_module and exit_module and handle it accordingly.

It does.

I reread the discussion on this, and I still don't see what great
benefit the new loader brings, or why it was allowed to break all the existing modules.

A software interface is a contract between caller and service. As long
as it doesn't change you can rewrite either side of it without breaking the other. That reduces maintenance and is something I thought was standard software engineering, best practices, etc.

Signature · 3 lines
bill davidsen <[email protected]>
  CTO, TMR Associates, Inc
Doing interesting things with little computers since 1979.
#6 lore raw
In reply to Matt Reppert
Quoted 4 lines · Matt Reppert

drivers/net/mii.c doesn't export module init/cleanup functions. That means it can't be loaded under the new module scheme. This patch adds do-nothing functions for it, which allows it to load. (8139too depends on mii, so without this I don't have network.)

The bug is in the module layer. This isn't the right fix at all. When the module layer is fixed to behave sanely the problem will go away

#7 lore raw
In reply to Matt Reppert

In message <[email protected]> you write:

Quoted 4 lines

drivers/net/mii.c doesn't export module init/cleanup functions. That means it can't be loaded under the new module scheme. This patch adds do-nothing functions for it, which allows it to load. (8139too depends on mii, so without this I don't have network.)

Correct fix is to fix the module code, which is done in the (attached) patch.

Hope that helps!
Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

Name: Module Name Solution
Author: Kai Germaschewski

D: Well, I have another solution, which doesn't need additional Makefile
D: magic or anything.
D:
D: I just put the module name into each .o file where <linux/module.h> is
D: included. Putting it into the section .gnu.linkonce.modname has the effect
D: that even for multi-part modules, we only end up with one copy of the
D: name.
D:
D: Caveat: I'm using the preprocessor macro KBUILD_MODNAME to know what to
D: put into .gnu.linkonce.modname. The following used to happen:
D:
D: (drivers/isdn/eicon/Makefile)
D:
D: divas-objs := common.o Divas_mod.o ...
D: eicon-objs := common.o eicon_mod.o ...
D:
D: Divas_mod.o is compiled with -DKBUILD_MODNAME=divas
D: eicon_mod.o is compiled with -DKBUILD_MODNAME=eicon
D: common.o is compiled with -DKBUILD_MODNAME=divas_eicon
D:
D: So in the case above, both divas.o and eicon.o would end up with
D: a .gnu.linkonce.modname section containing "divas_eicon"
D:
D: My fix to this is to not define KBUILD_MODNAME when compiling an object
D: whilch will be linked into more than one module - so common.o gets no
D: .gnu.linkonce.modname section at all. Works fine here.
D:
D: Now, doing this I remove one of the reasons why we would need modules
D: linked as '.ko' ;), but it seems much cleaner than generating a temporary
D: file, using objcopy etc.

include/linux/init.h +0 -16
diff -urNp --exclude TAGS -X /home/rusty/current-dontdiff --minimal linux-2.5.49/include/linux/init.h working-2.5.49-modname/include/linux/init.h
@@ -125,14 +125,6 @@ extern struct kernel_param __setup_start
  */
 #define module_exit(x)	__exitcall(x);
 
-/**
- * no_module_init - code needs no initialization.
- *
- * The equivalent of declaring an empty init function which returns 0.
- * Every module must have exactly one module_init() or no_module_init
- * invocation.  */
-#define no_module_init
-
 #else /* MODULE */
 
 /* Don't use these in modules, but some people do... */
@@ -144,11 +136,6 @@ extern struct kernel_param __setup_start
 #define device_initcall(fn)		module_init(fn)
 #define late_initcall(fn)		module_init(fn)
 
-/* Each module knows its own name. */
-#define __DEFINE_MODULE_NAME						\
-	char __module_name[] __attribute__((section(".modulename"))) =	\
-	__stringify(KBUILD_MODNAME)
-
 /* These macros create a dummy inline: gcc 2.9x does not count alias
  as usage, hence the `unused function' warning when __init functions
  are declared static. We use the dummy __*_module_inline functions
@@ -157,12 +144,9 @@ extern struct kernel_param __setup_start
 
 /* Each module must use one module_init(), or one no_module_init */
 #define module_init(initfn)					\
-	__DEFINE_MODULE_NAME;					\
 	static inline initcall_t __inittest(void)		\
 	{ return initfn; }					\
 	int __initfn(void) __attribute__((alias(#initfn)));
-
-#define no_module_init __DEFINE_MODULE_NAME
 
 /* This is only required if you want to be unloadable. */
 #define module_exit(exitfn)					\
include/linux/module.h +6 -6
diff -urNp --exclude TAGS -X /home/rusty/current-dontdiff --minimal linux-2.5.49/include/linux/module.h working-2.5.49-modname/include/linux/module.h
@@ -40,6 +40,11 @@ struct kernel_symbol
 
 #ifdef MODULE
 
+#ifdef KBUILD_MODNAME
+static const char __module_name[MODULE_NAME_LEN] __attribute__((section(".gnu.linkonce.modname"))) = \
+  __stringify(KBUILD_MODNAME);
+#endif
+
 #define MODULE_GENERIC_TABLE(gtype,name)	\
 static const unsigned long __module_##gtype##_size \
   __attribute__ ((unused)) = sizeof(struct gtype##_id); \
@@ -306,12 +311,7 @@ extern int module_dummy_usage;
 /* Old-style "I'll just call it init_module and it'll be run at
    insert".  Use module_init(myroutine) instead. */
 #ifdef MODULE
-/* Used as "int init_module(void) { ... }".  Get funky to insert modname. */
-#define init_module(voidarg)						\
-	__initfn(void);							\
-	char __module_name[] __attribute__((section(".modulename"))) =	\
-	__stringify(KBUILD_MODNAME);					\
-	int __initfn(void)
+#define init_module(voidarg) __initfn(void)
 #define cleanup_module(voidarg) __exitfn(void)
 #endif
 
kernel/module.c +2 -2
diff -urNp --exclude TAGS -X /home/rusty/current-dontdiff --minimal linux-2.5.49/kernel/module.c working-2.5.49-modname/kernel/module.c
@@ -864,8 +864,8 @@ static struct module *load_module(void *
 			/* Internal symbols */
 			DEBUGP("Symbol table in section %u\n", i);
 			symindex = i;
-		} else if (strcmp(secstrings+sechdrs[i].sh_name, ".modulename")
-			   == 0) {
+		} else if (strcmp(secstrings+sechdrs[i].sh_name,
+				  ".gnu.linkonce.modname") == 0) {
 			/* This module's name */
 			DEBUGP("Module name in section %u\n", i);
 			modnameindex = i;
lib/zlib_deflate/deflate_syms.c +0 -2
diff -urNp --exclude TAGS -X /home/rusty/current-dontdiff --minimal linux-2.5.49/lib/zlib_deflate/deflate_syms.c working-2.5.49-modname/lib/zlib_deflate/deflate_syms.c
@@ -19,5 +19,3 @@ EXPORT_SYMBOL(zlib_deflateReset);
 EXPORT_SYMBOL(zlib_deflateCopy);
 EXPORT_SYMBOL(zlib_deflateParams);
 MODULE_LICENSE("GPL");
-
-no_module_init;
lib/zlib_inflate/inflate_syms.c +0 -2
diff -urNp --exclude TAGS -X /home/rusty/current-dontdiff --minimal linux-2.5.49/lib/zlib_inflate/inflate_syms.c working-2.5.49-modname/lib/zlib_inflate/inflate_syms.c
@@ -20,5 +20,3 @@ EXPORT_SYMBOL(zlib_inflateReset);
 EXPORT_SYMBOL(zlib_inflateSyncPoint);
 EXPORT_SYMBOL(zlib_inflateIncomp); 
 MODULE_LICENSE("GPL");
-
-no_module_init;
scripts/Makefile.lib +3 -3
diff -urNp --exclude TAGS -X /home/rusty/current-dontdiff --minimal linux-2.5.49/scripts/Makefile.lib working-2.5.49-modname/scripts/Makefile.lib
@@ -124,14 +124,14 @@ depfile = $(subst $(comma),_,$(@D)/.$(@F
 #       than one module. In that case KBUILD_MODNAME will be set to foo_bar,
 #       where foo and bar are the name of the modules.
 basename_flags = -DKBUILD_BASENAME=$(subst $(comma),_,$(subst -,_,$(*F)))
-modname_flags  = -DKBUILD_MODNAME=$(subst $(comma),_,$(subst -,_,$(modname)))
+modname_flags  = $(if $(filter 1,$(words $(modname))),-DKBUILD_MODNAME=$(subst $(comma),_,$(subst -,_,$(modname))))
 c_flags        = -Wp,-MD,$(depfile) $(CFLAGS) $(NOSTDINC_FLAGS) \
 	         $(modkern_cflags) $(EXTRA_CFLAGS) $(CFLAGS_$(*F).o) \
 	         $(basename_flags) $(modname_flags) $(export_flags) 
 
 # Finds the multi-part object the current object will be linked into
-modname-multi = $(subst $(space),_,$(sort $(foreach m,$(multi-used),\
-		$(if $(filter $(subst $(obj)/,,$*.o), $($(m:.o=-objs)) $($(m:.o=-y))),$(m:.o=)))))
+modname-multi = $(sort $(foreach m,$(multi-used),\
+		$(if $(filter $(subst $(obj)/,,$*.o), $($(m:.o=-objs)) $($(m:.o=-y))),$(m:.o=))))
 
 # Shipped files
 # ===========================================================================