PARAM 2/4

Just fixed arch/i386/mach-voyager/setup_arch_pre.h from previous send.

Makes a nice, easy to use, hard to misuse PARAM() macro for modules and boot parameters alike. Implements the types required for current kernel usage.

Jeff Garzik doesn't like the name PARAM (I had to rename the asm-i386/setup.h PARAM to "PARAMS", but it's only used in that header and one other. I can't think of a better name. KPARAM seems redundant. MODULE_PARAM is misleading and wrong. DRIVER_PARAM is wrong too.

OTOH, linux/param.h was taken, so I put it in linux/params.h, which is pretty sucky.

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

Name: Parameter Core Patch
Author: Rusty Russell
Status: Tested on 2.5.38
Depends: Misc/strcspn.patch.gz
Depends: Module/module-i386.patch.gz

D: This patch is a rewrite of the insmod and boot parameter handling,
D: to unify them.
D:
D: The new format is fairly simple: built on top of __PARAM_CALL there
D: are several helpers, eg "PARAM(foo, int, 000)". The final argument
D: is the permissions bits, for exposing parameters in driverfs (if
D: non-zero) at a later stage.

diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .31348-linux-2.5-bk/arch/i386/mach-voyager/setup_arch_pre.h .31348-linux-2.5-bk.updated/arch/i386/mach-voyager/setup_arch_pre.h
@@ -1,5 +1,5 @@
 #include <asm/voyager.h>
-#define VOYAGER_BIOS_INFO ((struct voyager_bios_info *)(PARAM+0x40))
+#define VOYAGER_BIOS_INFO ((struct voyager_bios_info *)(PARAMS+0x40))
 
 /* Hook to call BIOS initialisation function */
 

diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .31348-linux-2.5-bk/arch/i386/vmlinux.lds.S .31348-linux-2.5-bk.updated/arch/i386/vmlinux.lds.S
@@ -67,6 +67,9 @@ SECTIONS
   __setup_start = .;
   .init.setup : { *(.init.setup) }
   __setup_end = .;
+  __param_start = .;
+  .param.init : { *(.param.init) }
+  __param_end = .;
   __initcall_start = .;
   .initcall.init : {
 	*(.initcall1.init) 

diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .31348-linux-2.5-bk/arch/ia64/vmlinux.lds.S .31348-linux-2.5-bk.updated/arch/ia64/vmlinux.lds.S
@@ -102,6 +102,10 @@ SECTIONS
   .init.setup : AT(ADDR(.init.setup) - PAGE_OFFSET)
         { *(.init.setup) }
   __setup_end = .;
+  __param_start = .;
+  .param.init : AT(ADDR(.param.init) - PAGE_OFFSET)
+        { *(.param.init) }
+  __param_end = .;
   __initcall_start = .;
   .initcall.init : AT(ADDR(.initcall.init) - PAGE_OFFSET)
 	{

diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .31348-linux-2.5-bk/arch/ppc/vmlinux.lds.S .31348-linux-2.5-bk.updated/arch/ppc/vmlinux.lds.S
@@ -103,6 +103,9 @@ SECTIONS
   __setup_start = .;
   .init.setup : { *(.init.setup) }
   __setup_end = .;
+  __param_start = .;
+  .param.init : { *(.param.init) }
+  __param_end = .;
   __initcall_start = .;
   .initcall.init : {
 	*(.initcall1.init) 

diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .31348-linux-2.5-bk/arch/ppc64/vmlinux.lds.S .31348-linux-2.5-bk.updated/arch/ppc64/vmlinux.lds.S
@@ -91,6 +91,9 @@ SECTIONS
   __setup_start = .;
   .init.setup : { *(.init.setup) }
   __setup_end = .;
+  __param_start = .;
+  .param.init : { *(.param.init) }
+  __param_end = .;
   __initcall_start = .;
   .initcall.init : {
 	*(.initcall1.init) 

diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .31348-linux-2.5-bk/arch/sparc64/vmlinux.lds.S .31348-linux-2.5-bk.updated/arch/sparc64/vmlinux.lds.S
@@ -49,6 +49,9 @@ SECTIONS
   __setup_start = .;
   .init.setup : { *(.init.setup) }
   __setup_end = .;
+  __param_start = .;
+  .param.init : { *(.param.init) }
+  __param_end = .;
   __initcall_start = .;
   .initcall.init : {
 	*(.initcall1.init) 

diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .31348-linux-2.5-bk/include/linux/init.h .31348-linux-2.5-bk.updated/include/linux/init.h
@@ -86,19 +86,15 @@ typedef void (*exitcall_t)(void);
 #define __exitcall(fn)							\
 	static exitcall_t __exitcall_##fn __exit_call = fn
 
-/*
- * Used for kernel command line parameter setup
- */
-struct kernel_param {
+struct obs_kernel_param {
 	const char *str;
 	int (*setup_func)(char *);
 };
 
-extern struct kernel_param __setup_start, __setup_end;
-
+/* OBSOLETE: see params.h for the right way. */
 #define __setup(str, fn)						\
 	static char __setup_str_##fn[] __initdata = str;		\
-	static struct kernel_param __setup_##fn				\
+	static struct obs_kernel_param __setup_##fn			\
 		 __attribute__((unused,__section__ (".init.setup")))	\
 		= { __setup_str_##fn, fn }
 

@@ -177,6 +173,16 @@ extern struct kernel_param __setup_start
 /* Data marked not to be saved by software_suspend() */
 #define __nosavedata __attribute__ ((__section__ (".data.nosave")))
 
+/* This means "can be init if no module support, otherwise module load
+   may call it." */
+#ifdef CONFIG_MODULES
+#define __init_or_module
+#define __initdata_or_module
+#else
+#define __init_or_module __init
+#define __initdata_or_module __initdata
+#endif /*CONFIG_MODULES*/
+
 #ifdef CONFIG_HOTPLUG
 #define __devinit
 #define __devinitdata

diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .31348-linux-2.5-bk/include/linux/kernel.h .31348-linux-2.5-bk.updated/include/linux/kernel.h
@@ -71,7 +71,7 @@ extern int sscanf(const char *, const ch
 extern int vsscanf(const char *, const char *, va_list);
 
 extern int get_option(char **str, int *pint);
-extern char *get_options(char *str, int nints, int *ints);
+extern char *get_options(const char *str, int nints, int *ints);
 extern unsigned long long memparse(char *ptr, char **retptr);
 extern void dev_probe_lock(void);
 extern void dev_probe_unlock(void);

diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .31348-linux-2.5-bk/kernel/Makefile .31348-linux-2.5-bk.updated/kernel/Makefile
@@ -4,13 +4,13 @@
 
 export-objs = signal.o sys.o kmod.o workqueue.o ksyms.o pm.o exec_domain.o \
 		printk.o platform.o suspend.o dma.o module.o cpufreq.o \
-		profile.o rcupdate.o intermodule.o
+		profile.o rcupdate.o intermodule.o params.o
 
 obj-y     = sched.o fork.o exec_domain.o panic.o printk.o profile.o \
 	    exit.o itimer.o time.o softirq.o resource.o \
 	    sysctl.o capability.o ptrace.o timer.o user.o \
 	    signal.o sys.o kmod.o workqueue.o futex.o platform.o pid.o \
-	    rcupdate.o intermodule.o
+	    rcupdate.o intermodule.o params.o
 
 obj-$(CONFIG_GENERIC_ISA_DMA) += dma.o
 obj-$(CONFIG_SMP) += cpu.o

diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .31348-linux-2.5-bk/lib/cmdline.c .31348-linux-2.5-bk.updated/lib/cmdline.c
@@ -64,12 +64,12 @@ int get_option (char **str, int *pint)
  *	completely parseable).
  */
  
-char *get_options (char *str, int nints, int *ints)
+char *get_options (const char *str, int nints, int *ints)
 {
 	int res, i = 1;
 
 	while (i < nints) {
-		res = get_option (&str, ints + i);
+		res = get_option ((char **)&str, ints + i);
 		if (res == 0)
 			break;
 		i++;

@@ -77,7 +77,7 @@ char *get_options (char *str, int nints,
 			break;
 	}
 	ints[0] = i - 1;
-	return (str);
+	return (char *)str;
 }
 
 /**

That's only because you never bother with drivers. _Please_ look at the bigger picture.

1) I note you ignored Matthew Wilcox's example of module_init being used in two different ways.

2) "proper", converted-to-Rusty-style driver code is going to have

MODULE_blah
MODULE_foo
MODULE_bar
PARAM

You think that looks good??

3) modules a.k.a. drivers are going to be the more common users of this interface.

4) even where arch code uses boot cmdline params, they are typically clustered together, which makes their purpose all the more obvious.

Another namespace collision? Who would have thought...

PARAM is ugly in drivers, and way too generic.

Jeff

In message <[email protected]> you write:

I didn't see it, sorry. Valid point.

Hmm... OK, you've swayed me. I'll redo the patch.

Well, we completely disagree on this point. If I was starting from scratch, MODULE_AUTHOR would simply be AUTHOR(), etc.

But I've always opposed this kind of "neatening" as the ultimite in stupidity, so consistency wins.

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

On Fri, Nov 15, 2002 at 05:51:48PM -0500, Jeff Garzik wrote:
[snip]

[snip]

Why not DRIVER_PARAM in this case ? It's enough explicit and not misleading. I too agree that PARAM is a bit too generic.

Cheers,
Willy

Why is MODULE_PARAM() misleading and wrong? I think it's a lot more descriptive, and these things are "modules" whether they are actually compiled in or not.

The MM layer is just "another module". Granted, you can't compile it out of the kernel, but it has a specific set of things it does, and is clearly not the same thing as the scheduler, which is it's own "module".

The fact that PARAM was already used as a name should have been a big hint that the name is not specific or descriptive enough.

Also, can we please stop shouting? I'd much rather see

	module_param(debug, int, 0600)

than

	PARAM(debug, int, 0600)

(see, for similarities, "module_init()", "module_exit()" etc: you don't need to shout to make a point, and "module_xxx()" is already the accepted practice for something that works both for compiled-in and modular objects).

Linus

In message <[email protected]> you wri te:

I've already conceded this to Jeff Garzik, but...

In theory. In practice our current build process in mm/ is not compiled as a module. eg: if we put in mm/readahead.c

	module_param(debug, int, 0600);

It'd be called "readahead.debug" not "mm.debug". Maybe that's fine, but indicates the subtle difference.

Sure. MODULE_AUTHOR(), MODULE_DESCRIPTION(), MODULE_PARM_DESC() and MODULE_LICENSE() fight the other way. They were previously module only, but that's an implmentation detail: plan is to expose the first three at least through sysfs, or just leave them as stubs for documentation purposes.

Am off to visit Extremadura to see the LinEx project: will update patch and re-send within 24 hrs,
Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

And they should stay MODULE-only.

I do _not_ want to see these be compiled in and taking up space in a static kernel.

I personally see the rule as: shouting MODULE_XXX() stuff is a no-op when the module is statically compiled into the kernel.

Linus