[PATCH] : More module parameter compatibility for 2.5.52

[PATCH] : More module parameter compatibility for 2.5.52

[Jean Tourrilhes]

	Hi,

	I've just downloaded 2.5.52 to try the new module parameter
support. Unfortunately, the letter 'c' was not implemented, and I need
it. This is used in few drivers (such as hp100, wavelan, wlan_cs...).
	I've only checked that with this patch my hp100 cards load
fine, but I didn't test the various corner cases.

	Have fun...

	Jean

---------------------------------------------------------------

diff -u -p linux/include/linux/moduleparam.r1.h linux/include/linux/moduleparam.h
--- linux/include/linux/moduleparam.r1.h	Mon Dec 16 16:19:59 2002
+++ linux/include/linux/moduleparam.h	Mon Dec 16 16:23:55 2002
@@ -25,6 +25,7 @@ struct kernel_param {
 	param_set_fn set;
 	param_get_fn get;
 	void *arg;
+	void *priv;
 };
 
 /* Special one for strings we want to copy into */
diff -u -p linux/kernel/module.r1.c linux/kernel/module.c
--- linux/kernel/module.r1.c	Mon Dec 16 16:08:42 2002
+++ linux/kernel/module.c	Mon Dec 16 16:39:00 2002
@@ -569,9 +569,41 @@ static int param_string(const char *name
 	return 0;
 }
 
+/* Do char array. This probably should be made available in the new stuff.
+ * Note : driver has already allocated the char array and tell us the
+ * size. The array is in fact two dimensional.
+ * Quick'n'dirty hack by Jean II */
+int param_set_chara(const char *val, struct kernel_param *kp)
+{
+	int	maxchar = (int) kp->priv;
+
+	if (!val) {
+		printk(KERN_ERR "%s: char array parameter expected\n",
+		       kp->name);
+		return -EINVAL;
+	}
+
+	/* Let's be on the safe side with respect to nul-termination
+	 * and always set strings with a nul. */
+	if ((strlen(val) + 1) > maxchar) {
+		printk(KERN_ERR "%s: char array parameter too long\n",
+		       kp->name);
+		return -ENOSPC;
+	}
+
+	strcpy((char *)kp->arg, (char *) val);
+	return 0;
+}
+
+int param_get_chara(char *buffer, struct kernel_param *kp)
+{
+	/* We may want to check the array size ? */
+	return sprintf(buffer, "%s", ((char *)kp->arg));
+}
+
 extern int set_obsolete(const char *val, struct kernel_param *kp)
 {
-	unsigned int min, max;
+	unsigned int min, max, line;
 	char *p, *endp;
 	struct obsolete_modparm *obsparm = kp->arg;
 
@@ -581,6 +613,7 @@ extern int set_obsolete(const char *val,
 	}
 
 	/* type is: [min[-max]]{b,h,i,l,s} */
+	/* or [min[-max]]{c}line for 2d array of chars - Jean II */
 	p = obsparm->type;
 	min = simple_strtol(p, &endp, 10);
 	if (endp == obsparm->type)
@@ -605,6 +638,13 @@ extern int set_obsolete(const char *val,
 				   sizeof(long), param_set_long);
 	case 's':
 		return param_string(kp->name, val, min, max, obsparm->addr);
+	case 'c':
+		p = endp+1;
+		line = simple_strtol(p, &endp, 10);
+		/* Maybe we should check that line doesn't get absurdly big */
+		kp->priv = (void *) line;
+		return param_array(kp->name, val, min, max, obsparm->addr,
+				   line, param_set_chara);
 	}
 	printk(KERN_ERR "Unknown obsolete parameter type %s\n", obsparm->type);
 	return -EINVAL;

Re: [PATCH] : More module parameter compatibility for 2.5.52

[Jean Tourrilhes]

On Mon, Dec 16, 2002 at 05:26:46PM -0800, jt wrote:
> 	Hi,
> 
> 	I've just downloaded 2.5.52 to try the new module parameter
> support. Unfortunately, the letter 'c' was not implemented, and I need
> it. This is used in few drivers (such as hp100, wavelan, wlan_cs...).
> 	I've only checked that with this patch my hp100 cards load
> fine, but I didn't test the various corner cases.
> 
> 	Have fun...
> 
> 	Jean

	Yeah, and I really should have tested this stuff properly...
	New (corrected) patch attached... Maybe even correct, who
knows...

	Jean

---------------------------------------------------------------

diff -u -p linux/include/linux/moduleparam.r1.h linux/include/linux/moduleparam.h
--- linux/include/linux/moduleparam.r1.h	Mon Dec 16 16:19:59 2002
+++ linux/include/linux/moduleparam.h	Mon Dec 16 18:37:02 2002
@@ -25,6 +25,7 @@ struct kernel_param {
 	param_set_fn set;
 	param_get_fn get;
 	void *arg;
+	void *priv;
 };
 
 /* Special one for strings we want to copy into */
@@ -123,5 +124,6 @@ int param_array(const char *name,
 		const char *val,
 		unsigned int min, unsigned int max,
 		void *elem, int elemsize,
-		int (*set)(const char *, struct kernel_param *kp));
+		int (*set)(const char *, struct kernel_param *kp),
+		void *priv);
 #endif /* _LINUX_MODULE_PARAM_TYPES_H */
diff -u -p linux/kernel/params.r1.c linux/kernel/params.c
--- linux/kernel/params.r1.c	Mon Dec 16 16:08:35 2002
+++ linux/kernel/params.c	Mon Dec 16 18:38:33 2002
@@ -228,7 +228,8 @@ int param_array(const char *name,
 		const char *val,
 		unsigned int min, unsigned int max,
 		void *elem, int elemsize,
-		int (*set)(const char *, struct kernel_param *kp))
+		int (*set)(const char *, struct kernel_param *kp),
+		void *priv)
 {
 	int ret;
 	unsigned int count = 0;
@@ -237,6 +238,7 @@ int param_array(const char *name,
 	/* Get the name right for errors. */
 	kp.name = name;
 	kp.arg = elem;
+	kp.priv = priv;
 
 	/* No equals sign? */
 	if (!val) {
@@ -285,7 +287,7 @@ int param_set_intarray(const char *val, 
 	/* Grab min and max as first two elements */
 	array = kp->arg;
 	return param_array(kp->name, val, array[0], array[1], &array[2],
-			   sizeof(int), param_set_int);
+			   sizeof(int), param_set_int, NULL);
 }
 
 int param_get_intarray(char *buffer, struct kernel_param *kp)
diff -u -p linux/kernel/module.r1.c linux/kernel/module.c
--- linux/kernel/module.r1.c	Mon Dec 16 16:08:42 2002
+++ linux/kernel/module.c	Mon Dec 16 18:58:32 2002
@@ -569,9 +569,41 @@ static int param_string(const char *name
 	return 0;
 }
 
+/* Do char array. This probably should be made available in the new stuff.
+ * Note : driver has already allocated the char array and tell us the
+ * size. The array is in fact two dimensional.
+ * Quick'n'dirty hack by Jean II */
+int param_set_chara(const char *val, struct kernel_param *kp)
+{
+	int	maxchar = (int) kp->priv;
+
+	if (!val) {
+		printk(KERN_ERR "%s: char array parameter expected\n",
+		       kp->name);
+		return -EINVAL;
+	}
+
+	/* Let's be on the safe side with respect to nul-termination
+	 * and always set strings with a nul. */
+	if ((strlen(val) + 1) > maxchar) {
+		printk(KERN_ERR "%s: char array parameter too long\n",
+		       kp->name);
+		return -ENOSPC;
+	}
+
+	strcpy((char *)kp->arg, (char *) val);
+	return 0;
+}
+
+int param_get_chara(char *buffer, struct kernel_param *kp)
+{
+	/* We may want to check the array size ? */
+	return sprintf(buffer, "%s", ((char *)kp->arg));
+}
+
 extern int set_obsolete(const char *val, struct kernel_param *kp)
 {
-	unsigned int min, max;
+	unsigned int min, max, line;
 	char *p, *endp;
 	struct obsolete_modparm *obsparm = kp->arg;
 
@@ -581,6 +613,7 @@ extern int set_obsolete(const char *val,
 	}
 
 	/* type is: [min[-max]]{b,h,i,l,s} */
+	/* or [min[-max]]{c}line for 2d array of chars - Jean II */
 	p = obsparm->type;
 	min = simple_strtol(p, &endp, 10);
 	if (endp == obsparm->type)
@@ -593,18 +626,24 @@ extern int set_obsolete(const char *val,
 	switch (*endp) {
 	case 'b':
 		return param_array(kp->name, val, min, max, obsparm->addr,
-				   1, param_set_byte);
+				   1, param_set_byte, NULL);
 	case 'h':
 		return param_array(kp->name, val, min, max, obsparm->addr,
-				   sizeof(short), param_set_short);
+				   sizeof(short), param_set_short, NULL);
 	case 'i':
 		return param_array(kp->name, val, min, max, obsparm->addr,
-				   sizeof(int), param_set_int);
+				   sizeof(int), param_set_int, NULL);
 	case 'l':
 		return param_array(kp->name, val, min, max, obsparm->addr,
-				   sizeof(long), param_set_long);
+				   sizeof(long), param_set_long, NULL);
 	case 's':
 		return param_string(kp->name, val, min, max, obsparm->addr);
+	case 'c':
+		p = endp+1;
+		line = simple_strtol(p, &endp, 10);
+		/* Maybe we should check that line doesn't get absurdly big */
+		return param_array(kp->name, val, min, max, obsparm->addr,
+				   line, param_set_chara, (void *) line);
 	}
 	printk(KERN_ERR "Unknown obsolete parameter type %s\n", obsparm->type);
 	return -EINVAL;

Re: [PATCH] : More module parameter compatibility for 2.5.52

[Rusty Russell]

In message <20021217012646.GA18021@bougret.hpl.hp.com> you write:
> 	Hi,
> 
> 	I've just downloaded 2.5.52 to try the new module parameter
> support. Unfortunately, the letter 'c' was not implemented, and I need
> it. This is used in few drivers (such as hp100, wavelan, wlan_cs...).

Cool, an undocumented type!  And they have explit sizes, and they're
used in arrays.  Just wonderful.

I prefer the fix below.  Does it work for you?

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

Name: Implement c in MODULE_PARM compatibility wedge
Author: Rusty Russell and Jean Tourrilhes
Status: Experimental

D: The "c" MODULE_PARM type was missing, as pointed out by Jean Tourrilhes.

diff -urNp --exclude TAGS -X /home/rusty/current-dontdiff --minimal linux-2.5.52/kernel/module.c working-2.5.52-cparam/kernel/module.c
--- linux-2.5.52/kernel/module.c	Tue Dec 17 08:11:03 2002
+++ working-2.5.52-cparam/kernel/module.c	Tue Dec 17 14:42:05 2002
@@ -569,10 +569,19 @@ static int param_string(const char *name
 	return 0;
 }
 
+/* Bounds checking done below */
+static int obsparm_copy_string(const char *val, struct kernel_param *kp)
+{
+	strcpy(kp->arg, val);
+	return 0;
+}
+
 extern int set_obsolete(const char *val, struct kernel_param *kp)
 {
 	unsigned int min, max;
-	char *p, *endp;
+	unsigned int size, maxsize;
+	char *endp;
+	const char *p;
 	struct obsolete_modparm *obsparm = kp->arg;
 
 	if (!val) {
@@ -605,8 +614,29 @@ extern int set_obsolete(const char *val,
 				   sizeof(long), param_set_long);
 	case 's':
 		return param_string(kp->name, val, min, max, obsparm->addr);
+
+	case 'c':
+		/* Undocumented: 1-5c50 means 1-5 strings of up to 49 chars,
+		   and the decl is "char xxx[5][50];" */
+		p = endp+1;
+		maxsize = simple_strtol(p, &endp, 10);
+		/* We check lengths here (yes, this is a hack). */
+		p = val;
+		while (p[size = strcspn(p, ",")]) {
+			if (size >= maxsize) 
+				goto oversize;
+			p += size+1;
+		}
+		if (size >= maxsize) 
+			goto oversize;
+		return param_array(kp->name, val, min, max, obsparm->addr,
+				   maxsize, obsparm_copy_string);
 	}
 	printk(KERN_ERR "Unknown obsolete parameter type %s\n", obsparm->type);
+	return -EINVAL;
+ oversize:
+	printk(KERN_ERR
+	       "Parameter %s doesn't fit in %u chars.\n", kp->name, maxsize);
 	return -EINVAL;
 }
 

Re: [PATCH] : More module parameter compatibility for 2.5.52

[Jean Tourrilhes]

On Tue, Dec 17, 2002 at 03:20:10PM +1100, Rusty Russell wrote:
> In message <20021217012646.GA18021@bougret.hpl.hp.com> you write:
> > 	Hi,
> > 
> > 	I've just downloaded 2.5.52 to try the new module parameter
> > support. Unfortunately, the letter 'c' was not implemented, and I need
> > it. This is used in few drivers (such as hp100, wavelan, wlan_cs...).
> 
> Cool, an undocumented type!  And they have explit sizes, and they're
> used in arrays.  Just wonderful.

	I can only say that they are more efficient than 's'...

> I prefer the fix below.  Does it work for you?
> 
> Rusty.

	With all due respect, your fix is quite ugly.
	And think about it this way : your new param architecture is
supposed to be flexible and supposed to allow modules to get
parameters in any shape or form. But, on the other hand, it's
impossible to implement something as simple as 'c' without ugly hacks.
	Maybe we can deduct from this that the new param API is not
flexible enough and that the simple addition of an opaque type (priv)
can have some value. From personal experience, most APIs that didn't
had mechanism to pass private data between an entity and the entity
handlers ended up regretting it and having to add it back at some
point.

	Have fun...

	Jean

Re: [PATCH] : More module parameter compatibility for 2.5.52

[Rusty Russell]

In message <20021217173346.GA22924@bougret.hpl.hp.com> you write:
> On Tue, Dec 17, 2002 at 03:20:10PM +1100, Rusty Russell wrote:
> > I prefer the fix below.  Does it work for you?
> > 
> > Rusty.
> 
> 	With all due respect, your fix is quite ugly.

Yes.

> 	And think about it this way : your new param architecture is
> supposed to be flexible and supposed to allow modules to get
> parameters in any shape or form. But, on the other hand, it's
> impossible to implement something as simple as 'c' without ugly hacks.

'c' is trivial.  1-20c50, which is a the two dimensional array of
variable bounds, which is outside the scope of current param_array
implementation (which was designed to handle 1d arrays).

> 	Maybe we can deduct from this that the new param API is not
> flexible enough and that the simple addition of an opaque type (priv)
> can have some value.

They *do* have a mechanism to pass extra parameters (kp->arg), it's
just that the standard "param_array" code already uses it to hand the
address of the variable.  Your patch added a second one.

The new param code was not meant to do *everything*, it was meant to
add type safety and unification of boot and module parameters, and
allow extensibility.

I think you're confusing "param_array() doesn't handle 2d arrays" with
"infrastructure not powerful enough".  Since __module_param_call() is
functionally equivalent to __setup(), the second one seems unlikely.

Writing such an extension is a job for the next mail...

Does that clarify?
Rusty.
--
  Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

Re: [PATCH] : More module parameter compatibility for 2.5.52

[Jean Tourrilhes]

On Wed, Dec 18, 2002 at 01:24:57PM +1100, Rusty Russell wrote:
>
> I think you're confusing "param_array() doesn't handle 2d arrays" with
> "infrastructure not powerful enough".  Since __module_param_call() is
> functionally equivalent to __setup(), the second one seems unlikely.
> 
> Writing such an extension is a job for the next mail...
> 
> Does that clarify?

	Perfect.

> Rusty.

	Good luck...

	Jean