[RFC/PATCH] Per-device parameter support (4/16)

[RFC/PATCH] Per-device parameter support (4/16)

[Tejun Heo]

 dp_04_module_param_ranged.diff

 This is the 4th patch of 16 patches for devparam.

 This patch implements module_param_flag() and module_param_invflag().
They appear as boolean parameter to the outside, and bitwise OR the
specified flag to flags when the specified boolean value is 1 and 0
respectively.


Signed-off-by: Tejun Heo <tj@home-tj.org>


Index: linux-devparam-export/include/linux/moduleparam.h
===================================================================
--- linux-devparam-export.orig/include/linux/moduleparam.h	2004-10-23 11:09:28.000000000 +0900
+++ linux-devparam-export/include/linux/moduleparam.h	2004-10-23 11:09:28.000000000 +0900
@@ -26,12 +26,7 @@ struct kernel_param {
 	param_set_fn set;
 	param_get_fn get;
 	void *arg;
-};
-
-/* Special one for strings we want to copy into */
-struct kparam_string {
-	unsigned int maxlen;
-	char *string;
+	long min, max;
 };
 
 /* Special one for arrays */
@@ -55,32 +50,41 @@ struct kparam_flag
    parameters.  perm sets the visibility in driverfs: 000 means it's
    not there, read bits mean it's readable, write bits mean it's
    writable. */
-#define __module_param_call(prefix, name, set, get, arg, perm)		\
+#define __module_param_call(prefix, name, set, get, arg, min, max, perm) \
 	static char __param_str_##name[] = prefix #name;		\
 	static struct kernel_param const __param_##name			\
 	__attribute_used__						\
     __attribute__ ((unused,__section__ ("__param"),aligned(sizeof(void *)))) \
-	= { __param_str_##name, perm, set, get, arg }
+	= { __param_str_##name, perm, set, get, arg, min, max }
+
+#define module_param_call_ranged(name, set, get, arg, min, max, perm)	\
+	__module_param_call(MODULE_PARAM_PREFIX, name, set, get, arg,	\
+			    min, max, perm)
 
-#define module_param_call(name, set, get, arg, perm)			      \
-	__module_param_call(MODULE_PARAM_PREFIX, name, set, get, arg, perm)
+#define module_param_call(name, set, get, arg, perm)			\
+	module_param_call_ranged(name, set, get, arg, 1, 0, perm)
 
 /* Helper functions: type is byte, short, ushort, int, uint, long,
    ulong, charp, bool or invbool, or XXX if you define param_get_XXX,
    param_set_XXX and param_check_XXX. */
-#define module_param_named(name, value, type, perm)			   \
-	param_check_##type(name, &(value));				   \
-	module_param_call(name, param_set_##type, param_get_##type, &value, perm)
+#define module_param_named_ranged(name, value, type, min, max, perm)	\
+	param_check_##type(name, &(value));				\
+	module_param_call_ranged(name, param_set_##type, param_get_##type, \
+				 &value, min, max, perm)
+
+#define module_param_ranged(name, type, min, max, perm)			\
+	module_param_named_ranged(name, name, type, min, max, perm)	\
+
+#define module_param_named(name, value, type, perm)			\
+	module_param_named_ranged(name, value, type, 1, 0, perm)
 
-#define module_param(name, type, perm)				\
+#define module_param(name, type, perm)					\
 	module_param_named(name, name, type, perm)
 
 /* Actually copy string: maxlen param is usually sizeof(string). */
 #define module_param_string(name, string, len, perm)			\
-	static struct kparam_string __param_string_##name		\
-		= { len, string };					\
-	module_param_call(name, param_set_copystring, param_get_string,	\
-		   &__param_string_##name, perm)
+	module_param_call_ranged(name, param_set_copystring,		\
+				 param_get_string, string, 0, len, perm)
 
 /* Bit flag */
 #define __module_param_flag(name, flags, flag, inv, perm)		\
@@ -150,14 +154,20 @@ extern int param_get_invbool(char *buffe
 #define param_check_invbool(name, p) __param_check(name, p, int)
 
 /* Comma-separated array: *nump is set to number they actually specified. */
-#define module_param_array_named(name, array, type, nump, perm)		\
+#define module_param_array_named_ranged(name, array, type, nump, min, max, perm)\
 	static struct kparam_array __param_arr_##name			\
 	= { ARRAY_SIZE(array), nump, param_set_##type, param_get_##type,\
 	    sizeof(array[0]), array };					\
-	module_param_call(name, param_array_set, param_array_get, 	\
-			  &__param_arr_##name, perm)
+	module_param_call_ranged(name, param_array_set, param_array_get,\
+				 &__param_arr_##name, min, max, perm)
+
+#define module_param_array_ranged(name, type, nump, min, max, perm)	\
+	module_param_array_named_ranged(name, name, type, nump, min, max, perm)
+
+#define module_param_array_named(name, array, type, nump, perm)		\
+	module_param_array_named_ranged(name, array, type, nump, 1, 0, perm)
 
-#define module_param_array(name, type, nump, perm)		\
+#define module_param_array(name, type, nump, perm)			\
 	module_param_array_named(name, name, type, nump, perm)
 
 extern int param_array_set(const char *val, struct kernel_param *kp);
@@ -169,9 +179,8 @@ extern int param_get_string(char *buffer
 extern int param_set_flag(const char *val, struct kernel_param *kp);
 extern int param_get_flag(char *buffer, struct kernel_param *kp);
 
-int param_array(const char *name,
-		const char *val,
-		unsigned int min, unsigned int max,
+int param_array(const char *name, const char *val,
+		long min_val, long max_val, unsigned int min, unsigned int max,
 		void *elem, int elemsize,
 		int (*set)(const char *, struct kernel_param *kp),
 		int *num);
Index: linux-devparam-export/kernel/module.c
===================================================================
--- linux-devparam-export.orig/kernel/module.c	2004-10-22 17:13:33.000000000 +0900
+++ linux-devparam-export/kernel/module.c	2004-10-23 11:09:28.000000000 +0900
@@ -748,19 +748,19 @@ int set_obsolete(const char *val, struct
 		max = min;
 	switch (*endp) {
 	case 'b':
-		return param_array(kp->name, val, min, max, obsparm->addr,
+		return param_array(kp->name, val, 1, 0, min, max, obsparm->addr,
 				   1, param_set_byte, &dummy);
 	case 'h':
-		return param_array(kp->name, val, min, max, obsparm->addr,
+		return param_array(kp->name, val, 1, 0, min, max, obsparm->addr,
 				   sizeof(short), param_set_short, &dummy);
 	case 'i':
-		return param_array(kp->name, val, min, max, obsparm->addr,
+		return param_array(kp->name, val, 1, 0, min, max, obsparm->addr,
 				   sizeof(int), param_set_int, &dummy);
 	case 'l':
-		return param_array(kp->name, val, min, max, obsparm->addr,
+		return param_array(kp->name, val, 1, 0, min, max, obsparm->addr,
 				   sizeof(long), param_set_long, &dummy);
 	case 's':
-		return param_array(kp->name, val, min, max, obsparm->addr,
+		return param_array(kp->name, val, 1, 0, min, max, obsparm->addr,
 				   sizeof(char *), param_set_charp, &dummy);
 
 	case 'c':
@@ -777,7 +777,7 @@ int set_obsolete(const char *val, struct
 		}
 		if (size >= maxsize) 
 			goto oversize;
-		return param_array(kp->name, val, min, max, obsparm->addr,
+		return param_array(kp->name, val, 1, 0, min, max, obsparm->addr,
 				   maxsize, obsparm_copy_string, &dummy);
 	}
 	printk(KERN_ERR "Unknown obsolete parameter type %s\n", obsparm->type);
Index: linux-devparam-export/kernel/params.c
===================================================================
--- linux-devparam-export.orig/kernel/params.c	2004-10-23 11:09:28.000000000 +0900
+++ linux-devparam-export/kernel/params.c	2004-10-23 11:09:28.000000000 +0900
@@ -138,6 +138,11 @@ int parse_args(const char *name,
 			       "%s: `%s' too large for parameter `%s'\n",
 			       name, val ?: "", param);
 			return ret;
+		case -ERANGE:
+			printk(KERN_ERR
+			       "%s: `%s' out of range for parameter `%s'\n",
+			       name, val ?: "", param);
+			return ret;
 		case 0:
 			break;
 		default:
@@ -163,6 +168,9 @@ int parse_args(const char *name,
 		l = strtolfn(val, &endp, 0);				\
 		if (endp == val || ((type)l != l))			\
 			return -EINVAL;					\
+		if (!(kp->min == 1 && kp->max == 0) &&			\
+		    (l < (type)kp->min || l > (type)kp->max))		\
+			return -ERANGE;					\
 		*((type *)kp->arg) = l;					\
 		return 0;						\
 	}								\
@@ -181,15 +189,26 @@ STANDARD_PARAM_DEF(ulong, unsigned long,
 
 int param_set_charp(const char *val, struct kernel_param *kp)
 {
+	size_t min, max, len;
+
 	if (!val) {
 		printk(KERN_ERR "%s: string parameter expected\n",
 		       kp->name);
 		return -EINVAL;
 	}
 
-	if (strlen(val) > 1024) {
-		printk(KERN_ERR "%s: string parameter too long\n",
-		       kp->name);
+	if (kp->min == 1 && kp->max == 0) {
+		min = 0;
+		max = 1024;
+	} else {
+		min = kp->min;
+		max = kp->max;
+	}
+
+	len = strlen(val);
+	if (len < min || len > max) {
+		printk(KERN_ERR "%s: string parameter too %s\n",
+		       kp->name, len < min ? "short" : "long");
 		return -ENOSPC;
 	}
 
@@ -246,9 +265,8 @@ int param_get_invbool(char *buffer, stru
 }
 
 /* We cheat here and temporarily mangle the string. */
-int param_array(const char *name,
-		const char *val,
-		unsigned int min, unsigned int max,
+int param_array(const char *name, const char *val,
+		long min_val, long max_val, unsigned int min, unsigned int max,
 		void *elem, int elemsize,
 		int (*set)(const char *, struct kernel_param *kp),
 		int *num)
@@ -260,6 +278,8 @@ int param_array(const char *name,
 	/* Get the name right for errors. */
 	kp.name = name;
 	kp.arg = elem;
+	kp.min = min_val;
+	kp.max = max_val;
 
 	/* No equals sign? */
 	if (!val) {
@@ -304,8 +324,9 @@ int param_array_set(const char *val, str
 	struct kparam_array *arr = kp->arg;
 	unsigned int t;
 
-	return param_array(kp->name, val, 1, arr->max, arr->elem,
-			   arr->elemsize, arr->set, arr->num ?: &t);
+	return param_array(kp->name, val, kp->min, kp->max,
+			   1, arr->max, arr->elem, arr->elemsize,
+			   arr->set, arr->num ?: &t);
 }
 
 int param_array_get(char *buffer, struct kernel_param *kp)
@@ -330,21 +351,18 @@ int param_array_get(char *buffer, struct
 
 int param_set_copystring(const char *val, struct kernel_param *kp)
 {
-	struct kparam_string *kps = kp->arg;
-
-	if (strlen(val)+1 > kps->maxlen) {
-		printk(KERN_ERR "%s: string doesn't fit in %u chars.\n",
-		       kp->name, kps->maxlen-1);
+	if (strlen(val)+1 > kp->max) {
+		printk(KERN_ERR "%s: string doesn't fit in %lu chars.\n",
+		       kp->name, kp->max - 1);
 		return -ENOSPC;
 	}
-	strcpy(kps->string, val);
+	strcpy(kp->arg, val);
 	return 0;
 }
 
 int param_get_string(char *buffer, struct kernel_param *kp)
 {
-	struct kparam_string *kps = kp->arg;
-	return strlcpy(buffer, kps->string, kps->maxlen);
+	return strlcpy(buffer, kp->arg, kp->max);
 }
 
 int param_set_flag(const char *val, struct kernel_param *kp)

Re: [RFC/PATCH] Per-device parameter support (4/16)

[Tejun Heo]

 Oops, I've used explanation for dp_3 again for dp_4.  Sorry.

On Sat, Oct 23, 2004 at 01:25:51PM +0900, Tejun Heo wrote:
>  dp_04_module_param_ranged.diff
> 
>  This is the 4th patch of 16 patches for devparam.

 This patch adds min and max fields of long type to struct
kernel_param and adds range checking for integer, charp and array
parameters.  Also, struct kparam_string isn't necessary anymore as we
can use max field instead.  I think this simple range-checking can
remove a lot of dulicated codes from drivers.

-- 
tejun

Re: [RFC/PATCH] Per-device parameter support (4/16)

[Rusty Russell]

On Sat, 2004-10-23 at 13:25 +0900, Tejun Heo wrote:
>  dp_04_module_param_ranged.diff
> 
>  This is the 4th patch of 16 patches for devparam.
> 
>  This patch implements module_param_flag() and module_param_invflag().
> They appear as boolean parameter to the outside, and bitwise OR the
> specified flag to flags when the specified boolean value is 1 and 0
> respectively.

Comment is wrong, of course: this patch adds range to the kernel_param
structure.  But I'm not convinced that it's a great idea.  It could be
added using the same method (kp->arg) used to extend the others instead.

(Same logic applied to spinlocking param variants).

It comes down to usage: currently there are few range-needing users, but
maybe that's because MODULE_PARM didn't support it.  So I would drop
this or implement it as a wrapper, and later we can add it when it takes
over the world.  Although I'd probably just add args to
module_param_call: I like having it as the "base".

The other thing is the min=1 max=0 case: I'd prefer INT_MAX, INT_MIN
etc. instead I think so there's no special cases.

Thanks!
Rusty.
-- 
A bad analogy is like a leaky screwdriver -- Richard Braakman

Re: [RFC/PATCH] Per-device parameter support (4/16)

[Tejun Heo]

On Mon, Oct 25, 2004 at 02:45:20PM +1000, Rusty Russell wrote:
> On Sat, 2004-10-23 at 13:25 +0900, Tejun Heo wrote:
> >  dp_04_module_param_ranged.diff
> > 
> >  This is the 4th patch of 16 patches for devparam.
> > 
> >  This patch implements module_param_flag() and module_param_invflag().
> > They appear as boolean parameter to the outside, and bitwise OR the
> > specified flag to flags when the specified boolean value is 1 and 0
> > respectively.
> 
> Comment is wrong, of course:

 Yeah, definitely.  Sorry. :-P

> this patch adds range to the kernel_param
> structure.  But I'm not convinced that it's a great idea.  It could be
> added using the same method (kp->arg) used to extend the others instead.
> 
> (Same logic applied to spinlocking param variants).
> 
> It comes down to usage: currently there are few range-needing users, but
> maybe that's because MODULE_PARM didn't support it.  So I would drop
> this or implement it as a wrapper, and later we can add it when it takes
> over the world.  Although I'd probably just add args to
> module_param_call: I like having it as the "base".

 And yes again.  I also thought it would look cleaner to implement
range checking using a wrapping argument.  But my rationales to include
them directly were

 1. Most driver parameters need range-checking.  I haven't seen many
    driver parameters which don't require range-checking.  The range
    values are also useful for non-integer parameters including strings
    and arrays.

 2. If we use separate range structure.  We would either
    - have two different sets (vanilla and ranged) of standard parameter
      set/get functions
    - or, end up using the wrapped version for both. (turning off range
      checking with some special value.)
    And both didn't look very attractive to me.  #1 is just not good
    and #2 seems a bit pointless as we'll always be using the wrapping
    argument for all types of paramters (integer, charp, string, array...).

 So, I folded range into the kernel_param structure.

> The other thing is the min=1 max=0 case: I'd prefer INT_MAX, INT_MIN
> etc. instead I think so there's no special cases.

 The thing is that the min, max values are used for all types
including signed and unsigned types.  min=MIN_MAX, max=INT_MIN will be
interpreted as a valid [INT_MAX, (unsigned)INT_MAX + 1] range for
unsigned int parameters.  So, I had to choose some value which,
regardless of type, maintains their relative relation, so the 1 and 0.
However, I do agree that plain 1, 0 doesn't look very clear.  How
about using something like the following?

#define PARAM_UNRANGED_MIN	1
#define PARAM_UNRANGED_MAX	0

or

#define PARAM_UNRANGED		1, 0

 I think it's better to be a bit feature-rich in module/device
paramter handling because by doing so we can avoid duplicated codes in
many places.

-- 
tejun