Hi all,
   Converting to and from void * for callback functions loses type safety:

everywhere else we expect the compiler to catch incorrect pointer types

handed to functions.
   It's pretty simple to create typesafe callback functions using typeof, and

with a little gcc trickery we can allow both old-style and typesafe callbacks

to avoid churn on commonly-used routines.
Feedback welcomed,

Rusty.

--

I wasn't too convinced with only irq handler conversion but with other

things converted, I'm liking it very much.  I really like the timer

conversion as casting between void * and unsigned long is annoying.

Possible improvements.
* Passing NULL as data to callback which takes non-void pointer

  triggers warning.  I think this should be allowed.  Something like

  the following?
* Allowing non-pointer integral types which fit into pointer would be

  nice.  It's often convenient to pass integers to simple callbacks.
The following macro kinda-sorta achieves the above two but it doesn't

consider promotion of integer types and thus is too strict.  There

gotta be some way.
#define kthread_create(threadfn, data, namefmt...) ({		\

	int (*_threadfn)(typeof(data));				\

	void *_data;						\

	_threadfn = __builtin_choose_expr(!__builtin_types_compatible_p(typeof(data), typeof(NULL)), \

			(threadfn), (void *)(threadfn));	\

	_data = __builtin_choose_expr(sizeof(data) <= sizeof(void *), \

			(void *)(unsigned long)data, (void *)data); \

	__kthread_create((void *)_threadfn, _data, namefmt);	\

})
Thanks.
--

tejun

--

What should be do are
* Check that the threadfn's argument fits into void *.
* Trigger overflow in implicit constant conversion warning if the

specified data is too large for the argument type.
Tricky...
--

tejun

--

For everything but timer, you'll get a warning if the data isn't assignable to

a void *, so you get a warning if you use a non-pointer already.
But it would be cool to allow functions which take an unsigned long.  To do

this, I think that would need to be a special case for the data arg (which

we'd really want to wrap in a macro), like:
	/* If fn expects an unsigned long, cast the data.  If not, we'll

	 * get a warning if data is not void * compatible. */

	__builtin_choose_expr(__builtin_compatible_p(typeof(1?(threadfn):NULL),

			      int (*)(unsigned long)),
Hmm, u64 on 32-bit platforms?  I think that will fail the above test: the type

of the function ptr will be "int (*)(u64)" and so you'll end up passing data

(a u64) to a void * argument, which will elicit a warning...
I'll test this out and see what I can make...
Thanks!

Rusty.

--

I think this comes under "too ugly", but here's an attempt.
Two patches, the infrastructure then the stop_machine change, and some

test cases (#if 0'd out).

===

Attempt to create callbacks which take unsigned long as well as

correct pointer types.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

---

 include/linux/compiler-gcc.h   |   72 +++++++++++++++++++++++++++++++++++++++++

 include/linux/compiler-intel.h |    3 +

 2 files changed, 75 insertions(+)
diff -r f41638047650 include/linux/compiler-gcc.h

--- a/include/linux/compiler-gcc.h	Mon Jan 21 11:46:30 2008 +1100

+++ b/include/linux/compiler-gcc.h	Mon Jan 21 15:05:45 2008 +1100

@@ -53,3 +53,75 @@

 #define  noinline			__attribute__((noinline))

 #define __attribute_const__		__attribute__((__const__))

 #define __maybe_unused			__attribute__((unused))

+

+/* This is not complete: I can't figure out a way to handle enums. */

+#define ulong_compatible(arg)						\

+	(__builtin_types_compatible_p(typeof(arg), char)		\

+	 || __builtin_types_compatible_p(typeof(arg), unsigned char)	\

+	 || __builtin_types_compatible_p(typeof(arg), signed char)	\

+	 || __builtin_types_compatible_p(typeof(arg), unsigned short)	\

+	 || __builtin_types_compatible_p(typeof(arg), short)		\

+	 || __builtin_types_compatible_p(typeof(arg), unsigned int)	\

+	 || __builtin_types_compatible_p(typeof(arg), int)		\

+	 || __builtin_types_compatible_p(typeof(arg), unsigned long)	\

+	 || __builtin_types_compatible_p(typeof(arg), long))

+

+/**

+ * typesafe_function - cast function type if it's compatible

+ * @fn: the function or function pointer

+ * @ulongtype: the function pointer type if arg is an integer

+ * @safetype: the function pointer type which matches typeof(arg)

+ * @fntype: the generic function pointer type to cast to

+ * @arg: the function argument.

+ *

+ * Callback functions are usually declared to take a "void *arg"

+ * argument, which stops the compiler from doing type checking.  We

+ * can freely cast to function pointer which ta...
[ message continues ]

Maybe we should be content with pointer type checking.  It seems like

it's going too far and after all the clutter it's not possible to use

int as argument.  :-(
Thanks.
--

tejun

--

It is possible, but the function has to take an unsigned long.
But I share your dislike of this.  Here's my final version, but it's still

ugly.
Rusty.
===

Attempt to create callbacks which take unsigned long as well as

correct pointer types.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

---

 include/linux/compiler-gcc.h   |   61 +++++++++++++++++++++++++++++++++++++++++

 include/linux/compiler-intel.h |    3 ++

 include/linux/kernel.h         |   21 ++++++++++++++

 3 files changed, 85 insertions(+)
diff -r 951ffe1c5238 include/linux/compiler-gcc.h

--- a/include/linux/compiler-gcc.h	Tue Jan 22 09:31:56 2008 +1100

+++ b/include/linux/compiler-gcc.h	Tue Jan 22 10:15:15 2008 +1100

@@ -53,3 +53,64 @@

 #define  noinline			__attribute__((noinline))

 #define __attribute_const__		__attribute__((__const__))

 #define __maybe_unused			__attribute__((unused))

+

+/* This is not complete: I can't figure out a way to handle enums. */

+#define ulong_compatible(arg)						\

+	(__builtin_types_compatible_p(typeof(arg), char)		\

+	 || __builtin_types_compatible_p(typeof(arg), unsigned char)	\

+	 || __builtin_types_compatible_p(typeof(arg), signed char)	\

+	 || __builtin_types_compatible_p(typeof(arg), unsigned short)	\

+	 || __builtin_types_compatible_p(typeof(arg), short)		\

+	 || __builtin_types_compatible_p(typeof(arg), unsigned int)	\

+	 || __builtin_types_compatible_p(typeof(arg), int)		\

+	 || __builtin_types_compatible_p(typeof(arg), unsigned long)	\

+	 || __builtin_types_compatible_p(typeof(arg), long))

+

+/**

+ * typesafe_fn_and_arg - cast function type and arg if compatible

+ * @fn: the function or function pointer

+ * @arg: the function argument.

+ * @ulongtype: the function pointer type if arg is an integer

+ * @safetype: the function pointer type which matches typeof(arg)

+ * @voidtype: the function pointer type which takes a void * (to cast to)

+ *

+ * This macro evaluates to two comma separated values: the function pointer,

+ * then the argument.

+ *

...
[ message continues ]

FWIW i had something similar using the gcc union extension at some

point for ioctls because I was tired for all the ugly casts from

unsigned long arg to void * in ioctl handlers.
But I decided to not push it because sparse would have likely choked

on it, and sparse actually finds a lot of bugs so it's more important than

having a few more casts.
-Andi

--

I bow down before you.
I thought I had done some rather horrible things with gcc built-ins and

macros, but I hereby hand over my crown to you.
As my daughter would say: that patch fell out of the ugly tree, and hit

every branch on the way down. Very impressive.
All hail Rusty, undisputed ruler of Ugly-land.
Side note: can you verify that __builtin_choose_expr() exists in gcc-3? I

don't think we've relied on it before except on arm, and that one has

always had its own compiler version dependencies..
			Linus

--

Hmm, looks like not in 3.0.4, is in 3.1.1.  I'll make it appropriately

#ifdef'ed (which as a bonus will make things that little bit uglier still...)
    If we can stomach it the effect is nice, but the version which simply

allows pointer correctness (rather than trying to do unsigned long too) is

less bletcherous.
Thanks,

Rusty.

--

I'd suggest trying to get by with just the pointer version for now. But we

know whom to turn to when we need bletcherousness.
		Linus

--

Simple statement expression with a temporary variable does the

typechecking for us: that the stop_machine callback matches the data

type.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

---

 drivers/char/hw_random/intel-rng.c |    3 +--

 include/linux/stop_machine.h       |   13 +++++++++----

 kernel/module.c                    |   10 +++-------

 kernel/stop_machine.c              |    4 ++--

 4 files changed, 15 insertions(+), 15 deletions(-)
diff -r fa243a61ba85 drivers/char/hw_random/intel-rng.c

--- a/drivers/char/hw_random/intel-rng.c	Thu Jan 17 13:06:16 2008 +1100

+++ b/drivers/char/hw_random/intel-rng.c	Thu Jan 17 14:47:11 2008 +1100

@@ -227,9 +227,8 @@ struct intel_rng_hw {

 	u8 fwh_dec_en1_val;

 };
-static int __init intel_rng_hw_init(void *_intel_rng_hw)

+static int __init intel_rng_hw_init(struct intel_rng_hw *intel_rng_hw)

 {

-	struct intel_rng_hw *intel_rng_hw = _intel_rng_hw;

 	u8 mfc, dvc;
 	/* interrupts disabled in stop_machine_run call */

diff -r fa243a61ba85 include/linux/stop_machine.h

--- a/include/linux/stop_machine.h	Thu Jan 17 13:06:16 2008 +1100

+++ b/include/linux/stop_machine.h	Thu Jan 17 14:47:11 2008 +1100

@@ -7,7 +7,6 @@

 #include <linux/cpu.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 +20,13 @@

  *

  * 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) ({			\

+	int (*_fn)(typeof(data)) = (fn);			\

+	stop_machine_run_notype((void *)_fn, (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 t...
[ message continues ]

Simple statement expression with a temporary variable does the

typechecking for us: that the kthread function matches the data type.
If the function doesn't match, we get:
	warning: initialization from incompatible pointer type
It's actually slightly over-strict, since you can hand void * data to

any function callback type, but there's only one such case in the kernel

anyway.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

---

 include/linux/kthread.h |   30 +++++++++++++++++++++++++++---

 kernel/kthread.c        |   29 +++++------------------------

 2 files changed, 32 insertions(+), 27 deletions(-)
diff -r 110aa94129d0 include/linux/kthread.h

--- a/include/linux/kthread.h	Fri Jan 18 10:32:31 2008 +1100

+++ b/include/linux/kthread.h	Fri Jan 18 11:31:49 2008 +1100

@@ -4,9 +4,33 @@

 #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...) ({		\

+	int (*_threadfn)(typeof(data)) = (threadfn);		\

+	__kthread_create((void *)_threadfn, (dat...
[ message continues ]

almost
either:
#define kthread_create(threadfn, data, ...) ({         \

        __kthread_create((void *)_threadfn, (data), __VA_ARGS__);
or:
#define kthread_create(threadfn, data, namefmt, ...) ({         \

        __kthread_create((void *)_threadfn, (data), namefmt, ##__VA_ARGS__);
--

Hi,
No.  This is bad because it gives the impression that it takes only two
This is better.  I prefer naming the rest args instead of using __VA_ARGS__:
#define kthread_create(threadfn, data, namefmt, fmtargs...) ({ \

	... \

	__kthread_create((void *)_threadfn, (data), namefmt, ##	fmtargs) \

})
but I think that is just a matter of taste.
	Hannes

--

No, it is a matter of conforming to C99 or to GNU extensions.
--

Hi Bert!
Not sure I see the point of your message.
The original use the ... varargs GNU extension, your two argument version is

the C99-safe variant, and your three args + __VA_ARGS__ uses the gcc ##

extension.
Cheers,

Rusty.

--

You're right. I didn't know that the ## is a gnu extension too,

thanks. I thought C99 did it right from the beginning.
Sincerely

--

This removes the warnings created by the patch to make kthread typesafe.
Other than fs/dlm/recoverd.c (which use a typedef to void and now

needs a cast), the changes were trivial and obvious.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

---

 crypto/cryptd.c                               |    6 +++---

 crypto/cryptomgr.c                            |    3 +--

 drivers/base/firmware_class.c                 |    5 ++---

 drivers/block/loop.c                          |    3 +--

 drivers/block/pktcdvd.c                       |    3 +--

 drivers/char/ipmi/ipmi_si_intf.c              |    3 +--

 drivers/ieee1394/nodemgr.c                    |    3 +--

 drivers/infiniband/core/fmr_pool.c            |    4 +---

 drivers/input/touchscreen/ucb1400_ts.c        |    3 +--

 drivers/md/md.c                               |    6 ++----

 drivers/media/dvb/dvb-core/dvb_ca_en50221.c   |    3 +--

 drivers/media/dvb/dvb-core/dvb_frontend.c     |    3 +--

 drivers/media/video/cx88/cx88-tvaudio.c       |    3 +--

 drivers/media/video/cx88/cx88.h               |    2 +-

 drivers/media/video/msp3400-driver.c          |    2 +-

 drivers/media/video/msp3400-driver.h          |    6 +++---

 drivers/media/video/msp3400-kthreads.c        |    9 +++------

 drivers/media/video/saa7134/saa7134-tvaudio.c |    8 +++-----

 drivers/media/video/tvaudio.c                 |    3 +--

 drivers/media/video/videobuf-dvb.c            |    3 +--

 drivers/media/video/vivi.c                    |    4 +---

 drivers/mmc/card/queue.c                      |    3 +--

 drivers/mmc/core/sdio_irq.c                   |    3 +--

 drivers/mtd/mtd_blkdevs.c                     |    3 +--

 drivers/mtd/ubi/wl.c                          |    3 +--

 drivers/net/irda/stir4200.c                   |    3 +--

 drivers/net/wireless/airo.c                   |    5 ++---

 drivers/net/wireless/libertas/main.c          |    3 +--

 drivers/pcmcia/cs.c                           |    5 ++---

 drivers/scsi/aacraid/aacraid.h                ...
[ message continues ]

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.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
diff -r e6626cc7bdc2 include/linux/compiler-gcc.h

--- a/include/linux/compiler-gcc.h	Sun Jan 20 18:51:51 2008 +1100

+++ b/include/linux/compiler-gcc.h	Sun Jan 20 18:57:14 2008 +1100

@@ -53,3 +53,20 @@

 #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. */

+#define cast_if_type(expr, oktype, desttype)				\

+  __builtin_choose_expr(__builtin_types_compatible_p(typeof(1?(expr):NULL), \

+						     oktype),		\

+			(desttype)(expr), (expr))

diff -r e6626cc7bdc2 include/linux/compiler-intel.h

--- a/include/linux/compiler-intel.h	Sun Jan 20 18:51:51 2008 +1100

+++ b/include/linux/compiler-intel.h	Sun Jan 20 18:57:14 2008 +1100

@@ -29,3 +29,5 @@

 #endif
 #define uninitialized_var(x) x

+

+#define cast_if_type(expr, oktype, desttype) ((desttype)(expr))

--

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 1...
[ message continues ]

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.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
diff -r 142a2cf4a8dc include/linux/timer.h

--- a/include/linux/timer.h	Sun Jan 20 19:01:56 2008 +1100

+++ b/include/linux/timer.h	Sun Jan 20 19:36:33 2008 +1100

@@ -24,11 +24,13 @@ struct timer_list {
 extern struct tvec_t_base_s boot_tvec_bases;
-#define TIMER_INITIALIZER(_function, _expires, _data) {		\

-		.function = (_function),			\

-		.expires = (_expires),				\

-		.data = (_data),				\

-		.base = &boot_tvec_bases,			\

+

+#define TIMER_INITIALIZER(_function, _expires, _data) {			\

+		.function = cast_if_type(_function, void (*)(typeof(_data)), \

+					 void (*)(unsigned long)),	\

+		.expires = (_expires),					\

+		.data = (unsigned long)(_data),				\

+		.base = &boot_tvec_bases,				\

 	}
 #define DEFINE_TIMER(_name, _function, _expires, _data)		\

@@ -38,9 +40,15 @@ void fastcall init_timer(struct timer_li

 void fastcall init_timer(struct timer_list * timer);

 void fastcall 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),						\

+		      cast_if_type(function, void (*)(typeof(data)),	\

+				   void (*)(unsigned long)),		\

+		      (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;

--