[PATCH 5/6] typesafe: request_irq and devm_request_irq

[PATCH 5/6] typesafe: request_irq and devm_request_irq

[Rusty Russell]

This patch lets interrupt handler functions have their natural type
(ie. exactly match the data pointer type); it allows the old irq_handler_t
type as well.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 include/linux/interrupt.h |   17 +++++++++++++++--
 include/linux/kernel.h    |    7 +++++++
 kernel/irq/devres.c       |   10 +++++-----
 kernel/irq/manage.c       |    6 +++---
 4 files changed, 30 insertions(+), 10 deletions(-)

diff -r 0fe1a980708b include/linux/interrupt.h
--- a/include/linux/interrupt.h	Thu Jan 17 14:48:56 2008 +1100
+++ b/include/linux/interrupt.h	Thu Jan 17 15:42:01 2008 +1100
@@ -69,13 +69,26 @@ struct irqaction {
 };
 
 extern irqreturn_t no_action(int cpl, void *dev_id);
-extern int __must_check request_irq(unsigned int, irq_handler_t handler,
+
+/* Typesafe version of request_irq.  Also takes old-style void * handlers. */
+#define request_irq(irq, handler, flags, name, dev_id)			\
+	__request_irq((irq),						\
+		      cast_if_type((handler), int (*)(int, typeof(dev_id)), \
+				   irq_handler_t),			\
+		      (flags), (name), (dev_id))
+
+extern int __must_check __request_irq(unsigned int, irq_handler_t handler,
 		       unsigned long, const char *, void *);
 extern void free_irq(unsigned int, void *);
 
 struct device;
 
-extern int __must_check devm_request_irq(struct device *dev, unsigned int irq,
+#define devm_request_irq(dev, irq, handler, flags, name, dev_id)	\
+	__devm_request_irq((dev), (irq),				\
+		      cast_if_type((handler), int (*)(int, typeof(dev_id)), \
+				   irq_handler_t),			\
+		      (flags), (name), (dev_id))
+extern int __must_check __devm_request_irq(struct device *dev, unsigned int irq,
 			    irq_handler_t handler, unsigned long irqflags,
 			    const char *devname, void *dev_id);
 extern void devm_free_irq(struct device *dev, unsigned int irq, void *dev_id);
diff -r 0fe1a980708b kernel/irq/devres.c
--- a/kernel/irq/devres.c	Thu Jan 17 14:48:56 2008 +1100
+++ b/kernel/irq/devres.c	Thu Jan 17 15:42:01 2008 +1100
@@ -41,9 +41,9 @@ static int devm_irq_match(struct device 
  *	If an IRQ allocated with this function needs to be freed
  *	separately, dev_free_irq() must be used.
  */
-int devm_request_irq(struct device *dev, unsigned int irq,
-		     irq_handler_t handler, unsigned long irqflags,
-		     const char *devname, void *dev_id)
+int __devm_request_irq(struct device *dev, unsigned int irq,
+		       irq_handler_t handler, unsigned long irqflags,
+		       const char *devname, void *dev_id)
 {
 	struct irq_devres *dr;
 	int rc;
@@ -53,7 +53,7 @@ int devm_request_irq(struct device *dev,
 	if (!dr)
 		return -ENOMEM;
 
-	rc = request_irq(irq, handler, irqflags, devname, dev_id);
+	rc = __request_irq(irq, handler, irqflags, devname, dev_id);
 	if (rc) {
 		devres_free(dr);
 		return rc;
@@ -65,7 +65,7 @@ int devm_request_irq(struct device *dev,
 
 	return 0;
 }
-EXPORT_SYMBOL(devm_request_irq);
+EXPORT_SYMBOL(__devm_request_irq);
 
 /**
  *	devm_free_irq - free an interrupt
diff -r 0fe1a980708b kernel/irq/manage.c
--- a/kernel/irq/manage.c	Thu Jan 17 14:48:56 2008 +1100
+++ b/kernel/irq/manage.c	Thu Jan 17 15:42:01 2008 +1100
@@ -514,8 +514,8 @@ EXPORT_SYMBOL(free_irq);
  *	IRQF_SAMPLE_RANDOM	The interrupt can be used for entropy
  *
  */
-int request_irq(unsigned int irq, irq_handler_t handler,
-		unsigned long irqflags, const char *devname, void *dev_id)
+int __request_irq(unsigned int irq, irq_handler_t handler,
+		  unsigned long irqflags, const char *devname, void *dev_id)
 {
 	struct irqaction *action;
 	int retval;
@@ -576,4 +576,4 @@ int request_irq(unsigned int irq, irq_ha
 
 	return retval;
 }
-EXPORT_SYMBOL(request_irq);
+EXPORT_SYMBOL(__request_irq);

[PATCH 0/6] typesafe callbacks

[Rusty Russell]

The idea is to create a general typesafe callback mechanism which doesn't 
break compile on existing code and doesn't allow any unsafe callback types.

Al had a very long timer conversion series followed by a different mechanism, 
and that just covered timers; unfortunately that technique provides 
insufficient typechecking for the general case (eg. int return types and 
callback functions which take integer args as well as the data arg).

Note that these typechecks end up being *too* strict in some cases, 
disallowing some potentially valid cases.  But since you can still use the 
current via-void* method, these corner cases lose nothing.

[PATCH 1/6] cast_if_type: allow macros functions which take more than one type.

[Rusty Russell]

To create functions which can take two types, but still warn on any
other types, we need a way of casting one type and no others.

To make things more complex, it should correctly handle function args,
NULL, and be usable in initializers.  __builtin_choose_expr was
introduced in gcc 3.1 (kernel needs >= 3.2 anyway).

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 include/linux/compiler-gcc.h   |   19 +++++++++++++++++++
 include/linux/compiler-intel.h |    2 ++
 2 files changed, 21 insertions(+)

diff -r bde8a949d956 include/linux/compiler-gcc.h
--- a/include/linux/compiler-gcc.h	Mon Apr 07 15:27:30 2008 +1000
+++ b/include/linux/compiler-gcc.h	Mon Apr 07 16:19:49 2008 +1000
@@ -53,3 +53,22 @@
 #define  noinline			__attribute__((noinline))
 #define __attribute_const__		__attribute__((__const__))
 #define __maybe_unused			__attribute__((unused))
+
+/**
+ * cast_if_type - allow an alternate type
+ * @expr: the expression to optionally cast
+ * @oktype: the type to allow.
+ * @desttype: the type to cast to.
+ *
+ * This is used to accept a particular alternate type for an expression:
+ * because any other types will not be cast, they will cause a warning as
+ * normal.
+ *
+ * Note that the unnecessary trinary forces functions to devolve into
+ * function pointers as users expect, but means @expr must be a pointer or
+ * integer.
+ */
+#define cast_if_type(expr, oktype, desttype)				\
+  __builtin_choose_expr(__builtin_types_compatible_p(typeof(1?(expr):0),\
+						     oktype),		\
+			(desttype)(expr), (expr))
diff -r bde8a949d956 include/linux/compiler-intel.h
--- a/include/linux/compiler-intel.h	Mon Apr 07 15:27:30 2008 +1000
+++ b/include/linux/compiler-intel.h	Mon Apr 07 16:19:49 2008 +1000
@@ -29,3 +29,5 @@
 #endif
 
 #define uninitialized_var(x) x
+
+#define cast_if_type(expr, oktype, desttype) ((desttype)(expr))

[PATCH 2/6] typesafe_cb: wrappers for typesafe callbacks.

[Rusty Russell]

cast_if_type is clever but annoying to use, so we make some slightly
less annoying wrappers for callback registration.

These allow const pointer-taking functions as well as the normal
non-const ones (volatile variant omitted for simplicity).  We also
have a typesafe_cb_preargs() for callbacks which don't just take one
argument.

Here's an example of use:

BEFORE:
	void foo_set_callback(int (*callback)(void *), void *arg);

AFTER:
	#define foo_set_callback(cb, arg) \
		__foo_set_callback(typesafe_cb(int, (cb), (arg)), (arg))
	void __foo_set_callback(int (*callback)(void *), void *arg);

o If cb is of form "int cb(typeof(arg))" or "int cb(const typeof(arg))" it
  will be cast to form "int cb(void *)".
o Otherwise, if cb is not already of form "int cb(void *)" it will cause a
  warning when passed to __foo_set_callback().
o If arg is not a pointer, then handing it to __foo_set_callback() will cause
  a warning.

Note: there are at least two possible additions we don't yet need.  We
don't cover callbacks which take a volatile pointer, even though that
would be fine, and we don't have a typesafe_cb_postargs().

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 include/linux/kernel.h |   63 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

diff -r 907c55be656f include/linux/kernel.h
--- a/include/linux/kernel.h	Mon Apr 21 06:58:00 2008 +1000
+++ b/include/linux/kernel.h	Mon Apr 21 07:04:02 2008 +1000
@@ -436,4 +436,39 @@ struct sysinfo {
 #define NUMA_BUILD 0
 #endif
 
+/* If fn is of type ok1 or ok2, cast to desttype */
+#define __typesafe_cb(desttype, fn, ok1, ok2) \
+	cast_if_type(cast_if_type((fn), ok1, desttype), ok2, desttype)
+
+/**
+ * typesafe_cb - cast a callback function if it matches the arg
+ * @rettype: the return type of the callback function
+ * @fn: the callback function to cast
+ * @arg: the (pointer) argument to hand to the callback function.
+ *
+ * If a callback function takes a single argument, this macro does
+ * appropriate casts to a function which takes a single void * argument if the
+ * callback provided matches the @arg (or a const or volatile version).
+ *
+ * It is assumed that @arg is of pointer type: usually @arg is passed
+ * or assigned to a void * elsewhere anyway.
+ */
+#define typesafe_cb(rettype, fn, arg)					\
+	__typesafe_cb(rettype (*)(void *), (fn),			\
+		      rettype (*)(const typeof(arg)),			\
+		      rettype (*)(typeof(arg)))
+
+/**
+ * typesafe_cb_preargs - cast a callback function if it matches the arg
+ * @rettype: the return type of the callback function
+ * @fn: the callback function to cast
+ * @arg: the (pointer) argument to hand to the callback function.
+ *
+ * This is a version of typesafe_cb() for callbacks that take other arguments
+ * before the @arg.
+ */
+#define typesafe_cb_preargs(rettype, fn, arg, ...)			\
+	__typesafe_cb(rettype (*)(__VA_ARGS__, void *), (fn),		\
+		      rettype (*)(__VA_ARGS__, const typeof(arg)),	\
+		      rettype (*)(__VA_ARGS__, typeof(arg)))
 #endif

[PATCH 3/6] typesafe: Convert stop_machine

[Rusty Russell]

Straightforward conversion of stop_machine_run.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 include/linux/stop_machine.h |   12 +++--
 kernel/stop_machine.c        |   89 ++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 95 insertions(+), 6 deletions(-)

diff -r 103cfcb193b5 include/linux/stop_machine.h
--- a/include/linux/stop_machine.h	Mon Apr 07 15:39:49 2008 +1000
+++ b/include/linux/stop_machine.h	Mon Apr 07 15:44:29 2008 +1000
@@ -5,9 +5,9 @@
    (and more).  So the "read" side to such a lock is anything which
    diables preeempt. */
 #include <linux/cpu.h>
+#include <linux/compiler.h>
 #include <asm/system.h>
 
-#if defined(CONFIG_STOP_MACHINE) && defined(CONFIG_SMP)
 /**
  * stop_machine_run: freeze the machine on all CPUs and run this function
  * @fn: the function to run
@@ -21,7 +21,11 @@
  *
  * This can be thought of as a very heavy write lock, equivalent to
  * grabbing every spinlock in the kernel. */
-int stop_machine_run(int (*fn)(void *), void *data, unsigned int cpu);
+#define stop_machine_run(fn, data, cpu)					\
+	stop_machine_run_notype(typesafe_cb(int, (fn), (data)), (data), (cpu))
+
+#if defined(CONFIG_STOP_MACHINE) && defined(CONFIG_SMP)
+int stop_machine_run_notype(int (*fn)(void *), void *data, unsigned int cpu);
 
 /**
  * __stop_machine_run: freeze the machine on all CPUs and run this function
@@ -38,8 +42,8 @@ struct task_struct *__stop_machine_run(i
 
 #else
 
-static inline int stop_machine_run(int (*fn)(void *), void *data,
-				   unsigned int cpu)
+static inline int stop_machine_run_notype(int (*fn)(void *), void *data,
+					  unsigned int cpu)
 {
 	int ret;
 	local_irq_disable();
diff -r 103cfcb193b5 kernel/stop_machine.c
--- a/kernel/stop_machine.c	Mon Apr 07 15:39:49 2008 +1000
+++ b/kernel/stop_machine.c	Mon Apr 07 15:44:29 2008 +1000
@@ -197,7 +197,7 @@ struct task_struct *__stop_machine_run(i
 	return p;
 }
 
-int stop_machine_run(int (*fn)(void *), void *data, unsigned int cpu)
+int stop_machine_run_notype(int (*fn)(void *), void *data, unsigned int cpu)
 {
 	struct task_struct *p;
 	int ret;
@@ -213,4 +213,4 @@ int stop_machine_run(int (*fn)(void *), 
 
 	return ret;
 }
-EXPORT_SYMBOL_GPL(stop_machine_run);
+EXPORT_SYMBOL_GPL(stop_machine_run_notype);

[PATCH 4/6] typesafe: kthread_create and kthread_run

[Rusty Russell]

Straight-forward conversion to allow typesafe callbacks.  Needs the
previously-posted __attribute__((format)) one-liner patch.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 include/linux/kthread.h |   28 +++++++++++++++++++++++++---
 kernel/kthread.c        |   29 +++++------------------------
 2 files changed, 30 insertions(+), 27 deletions(-)

diff -r f5a7f7f9683c include/linux/kthread.h
--- a/include/linux/kthread.h	Mon Apr 07 18:36:20 2008 +1000
+++ b/include/linux/kthread.h	Mon Apr 07 19:12:04 2008 +1000
@@ -4,9 +4,31 @@
 #include <linux/err.h>
 #include <linux/sched.h>
 
-struct task_struct *kthread_create(int (*threadfn)(void *data),
-				   void *data,
-				   const char namefmt[], ...)
+/**
+ * kthread_create - create a kthread.
+ * @threadfn: the function to run until signal_pending(current).
+ * @data: data ptr for @threadfn.
+ * @namefmt: printf-style name for the thread.
+ *
+ * Description: This helper function creates and names a kernel
+ * thread.  The thread will be stopped: use wake_up_process() to start
+ * it.  See also kthread_run(), kthread_create_on_cpu().
+ *
+ * When woken, the thread will run @threadfn() with @data as its
+ * argument. @threadfn() can either call do_exit() directly if it is a
+ * standalone thread for which noone will call kthread_stop(), or
+ * return when 'kthread_should_stop()' is true (which means
+ * kthread_stop() has been called).  The return value should be zero
+ * or a negative error number; it will be passed to kthread_stop().
+ *
+ * Returns a task_struct or ERR_PTR(-ENOMEM).
+ */
+#define kthread_create(threadfn, data, namefmt...)			\
+	__kthread_create(typesafe_cb(int,(threadfn),(data)), (data), namefmt)
+
+struct task_struct *__kthread_create(int (*threadfn)(void *data),
+				     void *data,
+				     const char namefmt[], ...)
 	__attribute__((format(printf, 3, 4)));
 
 /**
diff -r f5a7f7f9683c kernel/kthread.c
--- a/kernel/kthread.c	Mon Apr 07 18:36:20 2008 +1000
+++ b/kernel/kthread.c	Mon Apr 07 19:12:04 2008 +1000
@@ -112,29 +112,10 @@ static void create_kthread(struct kthrea
 	complete(&create->done);
 }
 
-/**
- * kthread_create - create a kthread.
- * @threadfn: the function to run until signal_pending(current).
- * @data: data ptr for @threadfn.
- * @namefmt: printf-style name for the thread.
- *
- * Description: This helper function creates and names a kernel
- * thread.  The thread will be stopped: use wake_up_process() to start
- * it.  See also kthread_run(), kthread_create_on_cpu().
- *
- * When woken, the thread will run @threadfn() with @data as its
- * argument. @threadfn() can either call do_exit() directly if it is a
- * standalone thread for which noone will call kthread_stop(), or
- * return when 'kthread_should_stop()' is true (which means
- * kthread_stop() has been called).  The return value should be zero
- * or a negative error number; it will be passed to kthread_stop().
- *
- * Returns a task_struct or ERR_PTR(-ENOMEM).
- */
-struct task_struct *kthread_create(int (*threadfn)(void *data),
-				   void *data,
-				   const char namefmt[],
-				   ...)
+struct task_struct *__kthread_create(int (*threadfn)(void *data),
+				     void *data,
+				     const char namefmt[],
+				     ...)
 {
 	struct kthread_create_info create;
 
@@ -159,7 +140,7 @@ struct task_struct *kthread_create(int (
 	}
 	return create.result;
 }
-EXPORT_SYMBOL(kthread_create);
+EXPORT_SYMBOL(__kthread_create);
 
 /**
  * kthread_bind - bind a just-created kthread to a cpu.

[PATCH 5/6] typesafe: request_irq and devm_request_irq

[Rusty Russell]

This patch lets interrupt handler functions have their natural type
(ie. exactly match the data pointer type); it allows the old irq_handler_t
type as well.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 include/linux/interrupt.h |   14 ++++++++++++--
 kernel/irq/devres.c       |   10 +++++-----
 kernel/irq/manage.c       |    6 +++---
 3 files changed, 20 insertions(+), 10 deletions(-)

diff -r b285fd1d3230 include/linux/interrupt.h
--- a/include/linux/interrupt.h	Mon Apr 07 15:48:58 2008 +1000
+++ b/include/linux/interrupt.h	Mon Apr 07 15:57:39 2008 +1000
@@ -69,13 +69,23 @@ struct irqaction {
 };
 
 extern irqreturn_t no_action(int cpl, void *dev_id);
-extern int __must_check request_irq(unsigned int, irq_handler_t handler,
+
+/* Typesafe version of request_irq.  Also takes old-style void * handlers. */
+#define request_irq(irq, handler, flags, name, dev_id)			\
+	__request_irq((irq), typesafe_cb_preargs(int,(handler),(dev_id),int), \
+		      (flags), (name), (dev_id))
+
+extern int __must_check __request_irq(unsigned int, irq_handler_t handler,
 		       unsigned long, const char *, void *);
 extern void free_irq(unsigned int, void *);
 
 struct device;
 
-extern int __must_check devm_request_irq(struct device *dev, unsigned int irq,
+#define devm_request_irq(dev, irq, handler, flags, name, dev_id)	\
+	__devm_request_irq((dev), (irq),				\
+			   typesafe_cb_preargs(int, (handler), (dev_id), int), \
+			   (flags), (name), (dev_id))
+extern int __must_check __devm_request_irq(struct device *dev, unsigned int irq,
 			    irq_handler_t handler, unsigned long irqflags,
 			    const char *devname, void *dev_id);
 extern void devm_free_irq(struct device *dev, unsigned int irq, void *dev_id);
diff -r b285fd1d3230 kernel/irq/devres.c
--- a/kernel/irq/devres.c	Mon Apr 07 15:48:58 2008 +1000
+++ b/kernel/irq/devres.c	Mon Apr 07 15:57:39 2008 +1000
@@ -41,9 +41,9 @@ static int devm_irq_match(struct device 
  *	If an IRQ allocated with this function needs to be freed
  *	separately, dev_free_irq() must be used.
  */
-int devm_request_irq(struct device *dev, unsigned int irq,
-		     irq_handler_t handler, unsigned long irqflags,
-		     const char *devname, void *dev_id)
+int __devm_request_irq(struct device *dev, unsigned int irq,
+		       irq_handler_t handler, unsigned long irqflags,
+		       const char *devname, void *dev_id)
 {
 	struct irq_devres *dr;
 	int rc;
@@ -53,7 +53,7 @@ int devm_request_irq(struct device *dev,
 	if (!dr)
 		return -ENOMEM;
 
-	rc = request_irq(irq, handler, irqflags, devname, dev_id);
+	rc = __request_irq(irq, handler, irqflags, devname, dev_id);
 	if (rc) {
 		devres_free(dr);
 		return rc;
@@ -65,7 +65,7 @@ int devm_request_irq(struct device *dev,
 
 	return 0;
 }
-EXPORT_SYMBOL(devm_request_irq);
+EXPORT_SYMBOL(__devm_request_irq);
 
 /**
  *	devm_free_irq - free an interrupt
diff -r b285fd1d3230 kernel/irq/manage.c
--- a/kernel/irq/manage.c	Mon Apr 07 15:48:58 2008 +1000
+++ b/kernel/irq/manage.c	Mon Apr 07 15:57:39 2008 +1000
@@ -517,8 +517,8 @@ EXPORT_SYMBOL(free_irq);
  *	IRQF_SAMPLE_RANDOM	The interrupt can be used for entropy
  *
  */
-int request_irq(unsigned int irq, irq_handler_t handler,
-		unsigned long irqflags, const char *devname, void *dev_id)
+int __request_irq(unsigned int irq, irq_handler_t handler,
+		  unsigned long irqflags, const char *devname, void *dev_id)
 {
 	struct irqaction *action;
 	int retval;
@@ -579,4 +579,4 @@ int request_irq(unsigned int irq, irq_ha
 
 	return retval;
 }
-EXPORT_SYMBOL(request_irq);
+EXPORT_SYMBOL(__request_irq);

[PATCH 6/6] typesafe: TIMER_INITIALIZER and setup_timer

[Rusty Russell]

This patch lets timer callback functions have their natural type
(ie. exactly match the data pointer type); it allows the old "unsigned
long data" type as well.

Downside: if you use the old "unsigned long" callback type, you won't
get a warning if your data is not an unsigned long, due to the cast.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 include/linux/timer.h |   24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff -r 1a72bd2d9ea6 include/linux/timer.h
--- a/include/linux/timer.h	Mon Apr 21 07:04:32 2008 +1000
+++ b/include/linux/timer.h	Mon Apr 21 07:28:46 2008 +1000
@@ -24,11 +24,21 @@ struct timer_list {
 
 extern struct tvec_base boot_tvec_bases;
 
-#define TIMER_INITIALIZER(_function, _expires, _data) {		\
-		.function = (_function),			\
-		.expires = (_expires),				\
-		.data = (_data),				\
-		.base = &boot_tvec_bases,			\
+/*
+ * For historic reasons the timer function takes an unsigned long, so
+ * we use this variant of typesafe_cb.  data is converted to an unsigned long
+ * if it is another integer type, by adding 0UL.
+ */
+#define typesafe_timerfn(fn, data)				\
+	__typesafe_cb(void (*)(unsigned long), (fn),		\
+		      void (*)(const typeof((data)+0UL)),	\
+		      void (*)(typeof((data)+0UL)))
+
+#define TIMER_INITIALIZER(_function, _expires, _data) {			\
+		.function = typesafe_timerfn((_function), (_data)),	\
+		.expires = (_expires),					\
+		.data = (unsigned long)(_data),				\
+		.base = &boot_tvec_bases,				\
 	}
 
 #define DEFINE_TIMER(_name, _function, _expires, _data)		\
@@ -38,9 +48,13 @@ void init_timer(struct timer_list *timer
 void init_timer(struct timer_list *timer);
 void init_timer_deferrable(struct timer_list *timer);
 
-static inline void setup_timer(struct timer_list * timer,
-				void (*function)(unsigned long),
-				unsigned long data)
+#define setup_timer(timer, function, data)				\
+	__setup_timer((timer), typesafe_timerfn((function), (data)),	\
+		      (unsigned long)(data))
+
+static inline void __setup_timer(struct timer_list * timer,
+				 void (*function)(unsigned long),
+				 unsigned long data)
 {
 	timer->function = function;
 	timer->data = data;