RFC: Typesafe callbacks

PATCH In review Rusty Russell 7 participants 23 messages Jan 20, 2008 – Jan 22, 2008

Subsystems (42)

Networking Network drivers Memory management Media (V4L/DVB) +38 more subsystems

lore

Share & tools

lore

Rusty Russell

author

2008-01-20 09:47 UTC #0 lore raw

cover RFC: Typesafe callbacks

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.

Rusty Russell

author

2008-01-20 09:48 UTC #1 lore raw

PATCH 1/6 typesafe: Convert stop_machine and callers

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 <[email protected]>

---
 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(-)

drivers/char/hw_random/intel-rng.c +1 -2

diff -r fa243a61ba85 drivers/char/hw_random/intel-rng.c
@@ -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 */

include/linux/stop_machine.h +9 -4

diff -r fa243a61ba85 include/linux/stop_machine.h
@@ -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 this function

@@ -38,8 +43,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();

kernel/module.c +3 -7

diff -r fa243a61ba85 kernel/module.c
@@ -623,10 +623,8 @@ struct stopref
 };
 
 /* Whole machine is stopped with interrupts off when this runs. */
-static int __try_stop_module(void *_sref)
+static int __try_stop_module(struct stopref *sref)
 {
-	struct stopref *sref = _sref;
-
 	/* If it's not unused, quit unless we are told to block. */
 	if ((sref->flags & O_NONBLOCK) && module_refcount(sref->mod) != 0) {
 		if (!(*sref->forced = try_force_unload(sref->flags)))

@@ -1305,9 +1303,8 @@ static void mod_kobject_remove(struct mo
  * link the module with the whole machine is stopped with interrupts off
  * - this defends against kallsyms not taking locks
  */
-static int __link_module(void *_mod)
+static int __link_module(struct module *mod)
 {
-	struct module *mod = _mod;
 	list_add(&mod->list, &modules);
 	return 0;
 }

@@ -1316,9 +1313,8 @@ static int __link_module(void *_mod)
  * unlink the module with the whole machine is stopped with interrupts off
  * - this defends against kallsyms not taking locks
  */
-static int __unlink_module(void *_mod)
+static int __unlink_module(struct module *mod)
 {
-	struct module *mod = _mod;
 	list_del(&mod->list);
 	return 0;
 }

kernel/stop_machine.c +2 -2

diff -r fa243a61ba85 kernel/stop_machine.c
@@ -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);

Rusty Russell

author

2008-01-20 09:49 UTC #2 lore raw

PATCH 2/6 typesafe: kthread_create and kthread_run

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 <[email protected]>

---
 include/linux/kthread.h |   30 +++++++++++++++++++++++++++---
 kernel/kthread.c        |   29 +++++------------------------
 2 files changed, 32 insertions(+), 27 deletions(-)

include/linux/kthread.h +27 -3

diff -r 110aa94129d0 include/linux/kthread.h
@@ -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, (data), namefmt);	\
+})
+
+struct task_struct *__kthread_create(int (*threadfn)(void *data),
+				     void *data,
+				     const char namefmt[], ...);
 
 /**
  * kthread_run - create and wake a thread.

kernel/kthread.c +5 ~~-24~~

diff -r 110aa94129d0 kernel/kthread.c
@@ -102,29 +102,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;

@@ -149,7 +130,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.

Rusty Russell

author

2008-01-20 09:51 UTC #3 lore raw

PATCH 3/6 typesafe: convert kthread users

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 <[email protected]>

---
 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                |    2 +-
 drivers/scsi/aacraid/commsup.c                |    3 +--
 drivers/scsi/libsas/sas_scsi_host.c           |    4 +---
 drivers/scsi/lpfc/lpfc_crtn.h                 |    2 +-
 drivers/scsi/lpfc/lpfc_hbadisc.c              |    3 +--
 drivers/scsi/qla2xxx/qla_os.c                 |    7 ++-----
 drivers/scsi/scsi_error.c                     |    4 +---
 drivers/scsi/scsi_priv.h                      |    2 +-
 drivers/scsi/scsi_scan.c                      |    3 +--
 drivers/usb/atm/ueagle-atm.c                  |    3 +--
 drivers/usb/gadget/file_storage.c             |    6 ++----
 drivers/usb/storage/usb.c                     |    7 ++-----
 drivers/w1/w1.c                               |    4 +---
 drivers/w1/w1.h                               |    2 +-
 fs/dlm/recoverd.c                             |    4 ++--
 fs/gfs2/daemon.c                              |   13 ++++---------
 fs/gfs2/daemon.h                              |    8 ++++----
 fs/gfs2/locking/dlm/thread.c                  |   11 +++++------
 fs/jbd/journal.c                              |    3 +--
 fs/jbd2/journal.c                             |    3 +--
 fs/nfs/delegation.c                           |    3 +--
 fs/nfs/nfs4state.c                            |    5 ++---
 fs/nfsd/nfs4callback.c                        |    3 +--
 fs/nfsd/nfs4state.c                           |    4 +---
 fs/ocfs2/cluster/heartbeat.c                  |    3 +--
 fs/ocfs2/dlm/dlmrecovery.c                    |    5 ++---
 fs/ocfs2/dlm/dlmthread.c                      |    5 ++---
 fs/ocfs2/journal.c                            |   10 ++++------
 fs/ocfs2/vote.c                               |    3 +--
 fs/ocfs2/vote.h                               |    2 +-
 fs/xfs/linux-2.6/xfs_buf.c                    |    5 ++---
 fs/xfs/linux-2.6/xfs_super.c                  |    3 +--
 kernel/audit.c                                |    3 +--
 kernel/audit.h                                |    2 +-
 kernel/rtmutex-tester.c                       |    3 +--
 kernel/stop_machine.c                         |    3 +--
 kernel/workqueue.c                            |    3 +--
 mm/vmscan.c                                   |    3 +--
 net/9p/mux.c                                  |    6 ++----
 net/core/pktgen.c                             |    3 +--
 sound/pci/emu10k1/emu10k1_main.c              |    3 +--
 70 files changed, 104 insertions(+), 183 deletions(-)

crypto/cryptd.c +3 -3

diff -r 110aa94129d0 crypto/cryptd.c
@@ -291,7 +291,8 @@ static struct crypto_template cryptd_tmp
 };
 
 static inline int cryptd_create_thread(struct cryptd_state *state,
-				       int (*fn)(void *data), const char *name)
+				       int (*fn)(struct cryptd_state *state),
+				       const char *name)
 {
 	spin_lock_init(&state->lock);
 	mutex_init(&state->mutex);

@@ -310,9 +311,8 @@ static inline void cryptd_stop_thread(st
 	kthread_stop(state->task);
 }
 
-static int cryptd_thread(void *data)
+static int cryptd_thread(struct cryptd_state *state)
 {
-	struct cryptd_state *state = data;
 	int stop;
 
 	current->flags |= PF_NOFREEZE;

crypto/cryptomgr.c +1 -2

diff -r 110aa94129d0 crypto/cryptomgr.c
@@ -47,9 +47,8 @@ struct cryptomgr_param {
 	char template[CRYPTO_MAX_ALG_NAME];
 };
 
-static int cryptomgr_probe(void *data)
+static int cryptomgr_probe(struct cryptomgr_param *param)
 {
-	struct cryptomgr_param *param = data;
 	struct crypto_template *tmpl;
 	struct crypto_instance *inst;
 	int err;

drivers/base/firmware_class.c +2 -3

diff -r 110aa94129d0 drivers/base/firmware_class.c
@@ -488,12 +488,11 @@ struct firmware_work {
 };
 
 static int
-request_firmware_work_func(void *arg)
+request_firmware_work_func(struct firmware_work *fw_work)
 {
-	struct firmware_work *fw_work = arg;
 	const struct firmware *fw;
 	int ret;
-	if (!arg) {
+	if (!fw_work) {
 		WARN_ON(1);
 		return 0;
 	}

drivers/block/loop.c +1 -2

diff -r 110aa94129d0 drivers/block/loop.c
@@ -577,9 +577,8 @@ static inline void loop_handle_bio(struc
  * once kthread_should_stop() is true and lo_bio is NULL, we are
  * done with the loop.
  */
-static int loop_thread(void *data)
+static int loop_thread(struct loop_device *lo)
 {
-	struct loop_device *lo = data;
 	struct bio *bio;
 
 	set_user_nice(current, -20);

drivers/block/pktcdvd.c +1 -2

diff -r 110aa94129d0 drivers/block/pktcdvd.c
@@ -1595,9 +1595,8 @@ static void pkt_count_states(struct pktc
  * kcdrwd is woken up when writes have been queued for one of our
  * registered devices
  */
-static int kcdrwd(void *foobar)
+static int kcdrwd(struct pktcdvd_device *pd)
 {
-	struct pktcdvd_device *pd = foobar;
 	struct packet_data *pkt;
 	long min_sleep_time, residue;

drivers/char/ipmi/ipmi_si_intf.c +1 -2

diff -r 110aa94129d0 drivers/char/ipmi/ipmi_si_intf.c
@@ -867,9 +867,8 @@ static void set_run_to_completion(void *
 	spin_unlock_irqrestore(&(smi_info->si_lock), flags);
 }
 
-static int ipmi_thread(void *data)
+static int ipmi_thread(struct smi_info *smi_info)
 {
-	struct smi_info *smi_info = data;
 	unsigned long flags;
 	enum si_sm_result smi_result;

drivers/ieee1394/nodemgr.c +1 -2

diff -r 110aa94129d0 drivers/ieee1394/nodemgr.c
@@ -1686,9 +1686,8 @@ static int nodemgr_check_irm_capability(
 	return 1;
 }
 
-static int nodemgr_host_thread(void *__hi)
+static int nodemgr_host_thread(struct host_info *hi)
 {
-	struct host_info *hi = (struct host_info *)__hi;
 	struct hpsb_host *host = hi->host;
 	unsigned int g, generation = 0;
 	int i, reset_cycles = 0;

drivers/infiniband/core/fmr_pool.c +1 -3

diff -r 110aa94129d0 drivers/infiniband/core/fmr_pool.c
@@ -177,10 +177,8 @@ static void ib_fmr_batch_release(struct 
 	spin_unlock_irq(&pool->pool_lock);
 }
 
-static int ib_fmr_cleanup_thread(void *pool_ptr)
+static int ib_fmr_cleanup_thread(struct ib_fmr_pool *pool)
 {
-	struct ib_fmr_pool *pool = pool_ptr;
-
 	do {
 		if (pool->dirty_len >= pool->dirty_watermark ||
 		    atomic_read(&pool->flush_ser) - atomic_read(&pool->req_ser) < 0) {

drivers/input/touchscreen/ucb1400_ts.c +1 -2

diff -r 110aa94129d0 drivers/input/touchscreen/ucb1400_ts.c
@@ -282,9 +282,8 @@ static void ucb1400_handle_pending_irq(s
 	enable_irq(ucb->irq);
 }
 
-static int ucb1400_ts_thread(void *_ucb)
+static int ucb1400_ts_thread(struct ucb1400 *ucb)
 {
-	struct ucb1400 *ucb = _ucb;
 	struct task_struct *tsk = current;
 	int valid = 0;
 	struct sched_param param = { .sched_priority = 1 };

drivers/md/md.c +2 -4

diff -r 110aa94129d0 drivers/md/md.c
@@ -4643,10 +4643,8 @@ static struct block_device_operations md
 	.revalidate_disk= md_revalidate,
 };
 
-static int md_thread(void * arg)
-{
-	mdk_thread_t *thread = arg;
-
+static int md_thread(mdk_thread_t *thread)
+{
 	/*
 	 * md_thread is a 'system-thread', it's priority should be very
 	 * high. We avoid resource deadlocks individually in each

drivers/media/dvb/dvb-core/dvb_ca_en50221.c +1 -2

diff -r 110aa94129d0 drivers/media/dvb/dvb-core/dvb_ca_en50221.c
@@ -955,9 +955,8 @@ static void dvb_ca_en50221_thread_update
 /**
  * Kernel thread which monitors CA slots for CAM changes, and performs data transfers.
  */
-static int dvb_ca_en50221_thread(void *data)
+static int dvb_ca_en50221_thread(struct dvb_ca_private *ca)
 {
-	struct dvb_ca_private *ca = data;
 	int slot;
 	int flags;
 	int status;

drivers/media/dvb/dvb-core/dvb_frontend.c +1 -2

diff -r 110aa94129d0 drivers/media/dvb/dvb-core/dvb_frontend.c
@@ -503,9 +503,8 @@ static void dvb_frontend_wakeup(struct d
 	wake_up_interruptible(&fepriv->wait_queue);
 }
 
-static int dvb_frontend_thread(void *data)
+static int dvb_frontend_thread(struct dvb_frontend *fe)
 {
-	struct dvb_frontend *fe = data;
 	struct dvb_frontend_private *fepriv = fe->frontend_priv;
 	unsigned long timeout;
 	fe_status_t s;

drivers/media/video/cx88/cx88-tvaudio.c +1 -2

diff -r 110aa94129d0 drivers/media/video/cx88/cx88-tvaudio.c
@@ -907,9 +907,8 @@ void cx88_set_stereo(struct cx88_core *c
 	return;
 }
 
-int cx88_audio_thread(void *data)
+int cx88_audio_thread(struct cx88_core *core)
 {
-	struct cx88_core *core = data;
 	struct v4l2_tuner t;
 	u32 mode = 0;

drivers/media/video/cx88/cx88.h +1 -1

diff -r 110aa94129d0 drivers/media/video/cx88/cx88.h
@@ -609,7 +609,7 @@ void cx88_newstation(struct cx88_core *c
 void cx88_newstation(struct cx88_core *core);
 void cx88_get_stereo(struct cx88_core *core, struct v4l2_tuner *t);
 void cx88_set_stereo(struct cx88_core *core, u32 mode, int manual);
-int cx88_audio_thread(void *data);
+int cx88_audio_thread(struct cx88_core *core);
 
 int cx8802_register_driver(struct cx8802_driver *drv);
 int cx8802_unregister_driver(struct cx8802_driver *drv);

drivers/media/video/msp3400-driver.c +1 -1

diff -r 110aa94129d0 drivers/media/video/msp3400-driver.c
@@ -805,7 +805,7 @@ static int msp_attach(struct i2c_adapter
 {
 	struct i2c_client *client;
 	struct msp_state *state;
-	int (*thread_func)(void *data) = NULL;
+	int (*thread_func)(struct i2c_client *client) = NULL;
 	int msp_hard;
 	int msp_family;
 	int msp_revision;

drivers/media/video/msp3400-driver.h +3 -3

diff -r 110aa94129d0 drivers/media/video/msp3400-driver.h
@@ -110,9 +110,9 @@ const char *msp_standard_std_name(int st
 const char *msp_standard_std_name(int std);
 void msp_set_audmode(struct i2c_client *client);
 int msp_detect_stereo(struct i2c_client *client);
-int msp3400c_thread(void *data);
-int msp3410d_thread(void *data);
-int msp34xxg_thread(void *data);
+int msp3400c_thread(struct i2c_client *client);
+int msp3410d_thread(struct i2c_client *client);
+int msp34xxg_thread(struct i2c_client *client);
 void msp3400c_set_mode(struct i2c_client *client, int mode);
 void msp3400c_set_carrier(struct i2c_client *client, int cdo1, int cdo2);

drivers/media/video/msp3400-kthreads.c +3 -6

diff -r 110aa94129d0 drivers/media/video/msp3400-kthreads.c
@@ -460,9 +460,8 @@ static void watch_stereo(struct i2c_clie
 		state->watch_stereo = 0;
 }
 
-int msp3400c_thread(void *data)
+int msp3400c_thread(struct i2c_client *client)
 {
-	struct i2c_client *client = data;
 	struct msp_state *state = i2c_get_clientdata(client);
 	struct msp3400c_carrier_detect *cd;
 	int count, max1, max2, val1, val2, val, this;

@@ -641,9 +640,8 @@ int msp3400c_thread(void *data)
 }
 
 
-int msp3410d_thread(void *data)
+int msp3410d_thread(struct i2c_client *client)
 {
-	struct i2c_client *client = data;
 	struct msp_state *state = i2c_get_clientdata(client);
 	int val, i, std, count;

@@ -935,9 +933,8 @@ static void msp34xxg_reset(struct i2c_cl
 	msp_write_dem(client, 0x22, msp_stereo_thresh);
 }
 
-int msp34xxg_thread(void *data)
+int msp34xxg_thread(struct i2c_client *client)
 {
-	struct i2c_client *client = data;
 	struct msp_state *state = i2c_get_clientdata(client);
 	int val, i;

drivers/media/video/saa7134/saa7134-tvaudio.c +3 -5

diff -r 110aa94129d0 drivers/media/video/saa7134/saa7134-tvaudio.c
@@ -496,9 +496,8 @@ static int tvaudio_setstereo(struct saa7
 	return 0;
 }
 
-static int tvaudio_thread(void *data)
+static int tvaudio_thread(struct saa7134_dev *dev)
 {
-	struct saa7134_dev *dev = data;
 	int carr_vals[ARRAY_SIZE(mainscan)];
 	unsigned int i, audio, nscan;
 	int max1,max2,carrier,rx,mode,lastmode,default_carrier;

@@ -781,9 +780,8 @@ static int mute_input_7133(struct saa713
 	return 0;
 }
 
-static int tvaudio_thread_ddep(void *data)
+static int tvaudio_thread_ddep(struct saa7134_dev *dev)
 {
-	struct saa7134_dev *dev = data;
 	u32 value, norms, clock;

@@ -980,7 +978,7 @@ int saa7134_tvaudio_getstereo(struct saa
 
 int saa7134_tvaudio_init2(struct saa7134_dev *dev)
 {
-	int (*my_thread)(void *data) = NULL;
+	int (*my_thread)(struct saa7134_dev *) = NULL;
 
 	switch (dev->pci->device) {
 	case PCI_DEVICE_ID_PHILIPS_SAA7134:

drivers/media/video/tvaudio.c +1 -2

diff -r 110aa94129d0 drivers/media/video/tvaudio.c
@@ -264,9 +264,8 @@ static void chip_thread_wake(unsigned lo
 	wake_up_process(chip->thread);
 }
 
-static int chip_thread(void *data)
+static int chip_thread(struct CHIPSTATE *chip)
 {
-	struct CHIPSTATE *chip = data;
 	struct CHIPDESC  *desc = chiplist + chip->type;
 
 	v4l_dbg(1, debug, &chip->c, "%s: thread started\n", chip->c.name);

drivers/media/video/videobuf-dvb.c +1 -2

diff -r 110aa94129d0 drivers/media/video/videobuf-dvb.c
@@ -39,9 +39,8 @@ MODULE_PARM_DESC(debug,"enable debug mes
 
 /* ------------------------------------------------------------------ */
 
-static int videobuf_dvb_thread(void *data)
+static int videobuf_dvb_thread(struct videobuf_dvb *dvb)
 {
-	struct videobuf_dvb *dvb = data;
 	struct videobuf_buffer *buf;
 	unsigned long flags;
 	int err;

drivers/media/video/vivi.c +1 -3

diff -r 110aa94129d0 drivers/media/video/vivi.c
@@ -455,10 +455,8 @@ static void vivi_sleep(struct vivi_dmaqu
 	try_to_freeze();
 }
 
-static int vivi_thread(void *data)
+static int vivi_thread(struct vivi_dmaqueue  *dma_q)
 {
-	struct vivi_dmaqueue  *dma_q=data;
-
 	dprintk(1,"thread started\n");
 
 	mod_timer(&dma_q->timeout, jiffies+BUFFER_TIMEOUT);

drivers/mmc/card/queue.c +1 -2

diff -r 110aa94129d0 drivers/mmc/card/queue.c
@@ -41,9 +41,8 @@ static int mmc_prep_request(struct reque
 	return BLKPREP_OK;
 }
 
-static int mmc_queue_thread(void *d)
+static int mmc_queue_thread(struct mmc_queue *mq)
 {
-	struct mmc_queue *mq = d;
 	struct request_queue *q = mq->queue;
 
 	current->flags |= PF_MEMALLOC;

drivers/mmc/core/sdio_irq.c +1 -2

diff -r 110aa94129d0 drivers/mmc/core/sdio_irq.c
@@ -63,9 +63,8 @@ static int process_sdio_pending_irqs(str
 	return ret;
 }
 
-static int sdio_irq_thread(void *_host)
+static int sdio_irq_thread(struct mmc_host *host)
 {
-	struct mmc_host *host = _host;
 	struct sched_param param = { .sched_priority = 1 };
 	unsigned long period, idle_period;
 	int ret;

drivers/mtd/mtd_blkdevs.c +1 -2

diff -r 110aa94129d0 drivers/mtd/mtd_blkdevs.c
@@ -74,9 +74,8 @@ static int do_blktrans_request(struct mt
 	}
 }
 
-static int mtd_blktrans_thread(void *arg)
+static int mtd_blktrans_thread(struct mtd_blktrans_ops *tr)
 {
-	struct mtd_blktrans_ops *tr = arg;
 	struct request_queue *rq = tr->blkcore_priv->rq;
 
 	/* we might get involved when memory gets low, so use PF_MEMALLOC */

drivers/mtd/ubi/wl.c +1 -2

diff -r 110aa94129d0 drivers/mtd/ubi/wl.c
@@ -1303,10 +1303,9 @@ static void tree_destroy(struct rb_root 
  * ubi_thread - UBI background thread.
  * @u: the UBI device description object pointer
  */
-static int ubi_thread(void *u)
+static int ubi_thread(struct ubi_device *ubi)
 {
 	int failures = 0;
-	struct ubi_device *ubi = u;
 
 	ubi_msg("background thread \"%s\" started, PID %d",
 		ubi->bgt_name, task_pid_nr(current));

drivers/net/irda/stir4200.c +1 -2

diff -r 110aa94129d0 drivers/net/irda/stir4200.c
@@ -732,9 +732,8 @@ static void stir_send(struct stir_cb *st
 /*
  * Transmit state machine thread
  */
-static int stir_transmit_thread(void *arg)
+static int stir_transmit_thread(struct stir_cb *stir)
 {
-	struct stir_cb *stir = arg;
 	struct net_device *dev = stir->netdev;
 	struct sk_buff *skb;

drivers/net/wireless/airo.c +2 -3

diff -r 110aa94129d0 drivers/net/wireless/airo.c
@@ -1123,7 +1123,7 @@ static int waitbusy (struct airo_info *a
 static int waitbusy (struct airo_info *ai);
 
 static irqreturn_t airo_interrupt( int irq, void* dev_id);
-static int airo_thread(void *data);
+static int airo_thread(struct net_device *dev);
 static void timer_func( struct net_device *dev );
 static int airo_ioctl(struct net_device *dev, struct ifreq *rq, int cmd);
 static struct iw_statistics *airo_get_wireless_stats (struct net_device *dev);

@@ -3080,8 +3080,7 @@ out:
 	wireless_send_event(ai->dev, SIOCGIWSCAN, &wrqu, NULL);
 }
 
-static int airo_thread(void *data) {
-	struct net_device *dev = data;
+static int airo_thread(struct net_device *dev) {
 	struct airo_info *ai = dev->priv;
 	int locked;

drivers/net/wireless/libertas/main.c +1 -2

diff -r 110aa94129d0 drivers/net/wireless/libertas/main.c
@@ -752,9 +752,8 @@ static void libertas_set_multicast_list(
  *  @param data    A pointer to wlan_thread structure
  *  @return 	   0
  */
-static int libertas_thread(void *data)
+static int libertas_thread(struct net_device *dev)
 {
-	struct net_device *dev = data;
 	wlan_private *priv = dev->priv;
 	wlan_adapter *adapter = priv->adapter;
 	wait_queue_t wait;

drivers/pcmcia/cs.c +2 -3

diff -r 110aa94129d0 drivers/pcmcia/cs.c
@@ -170,7 +170,7 @@ static void pcmcia_release_socket(struct
 	complete(&socket->socket_released);
 }
 
-static int pccardd(void *__skt);
+static int pccardd(struct pcmcia_socket *skt);
 
 /**
  * pcmcia_register_socket - add a new pcmcia socket device

@@ -632,9 +632,8 @@ static void socket_detect_change(struct 
 	}
 }
 
-static int pccardd(void *__skt)
+static int pccardd(struct pcmcia_socket *skt)
 {
-	struct pcmcia_socket *skt = __skt;
 	DECLARE_WAITQUEUE(wait, current);
 	int ret;

drivers/scsi/aacraid/aacraid.h +1 -1

diff -r 110aa94129d0 drivers/scsi/aacraid/aacraid.h
@@ -1851,7 +1851,7 @@ unsigned int aac_intr_normal(struct aac_
 unsigned int aac_intr_normal(struct aac_dev * dev, u32 Index);
 int aac_reset_adapter(struct aac_dev * dev, int forced);
 int aac_check_health(struct aac_dev * dev);
-int aac_command_thread(void *data);
+int aac_command_thread(struct aac_dev * dev);
 int aac_close_fib_context(struct aac_dev * dev, struct aac_fib_context *fibctx);
 int aac_fib_adapter_complete(struct fib * fibptr, unsigned short size);
 struct aac_driver_ident* aac_get_driver_ident(int devtype);

drivers/scsi/aacraid/commsup.c +1 -2

diff -r 110aa94129d0 drivers/scsi/aacraid/commsup.c
@@ -1400,9 +1400,8 @@ out:
  *	more FIBs.
  */
  
-int aac_command_thread(void *data)
+int aac_command_thread(struct aac_dev *dev)
 {
-	struct aac_dev *dev = data;
 	struct hw_fib *hw_fib, *hw_newfib;
 	struct fib *fib, *newfib;
 	struct aac_fib_context *fibctx;

drivers/scsi/libsas/sas_scsi_host.c +1 -3

diff -r 110aa94129d0 drivers/scsi/libsas/sas_scsi_host.c
@@ -900,10 +900,8 @@ static void sas_queue(struct sas_ha_stru
  * sas_queue_thread -- The Task Collector thread
  * @_sas_ha: pointer to struct sas_ha
  */
-static int sas_queue_thread(void *_sas_ha)
+static int sas_queue_thread(struct sas_ha_struct *sas_ha)
 {
-	struct sas_ha_struct *sas_ha = _sas_ha;
-
 	while (1) {
 		set_current_state(TASK_INTERRUPTIBLE);
 		schedule();

drivers/scsi/lpfc/lpfc_crtn.h +1 -1

diff -r 110aa94129d0 drivers/scsi/lpfc/lpfc_crtn.h
@@ -78,7 +78,7 @@ struct lpfc_nodelist *lpfc_findnode_rpi(
 
 void lpfc_worker_wake_up(struct lpfc_hba *);
 int lpfc_workq_post_event(struct lpfc_hba *, void *, void *, uint32_t);
-int lpfc_do_work(void *);
+int lpfc_do_work(struct lpfc_hba *phba);
 int lpfc_disc_state_machine(struct lpfc_vport *, struct lpfc_nodelist *, void *,
 			    uint32_t);

drivers/scsi/lpfc/lpfc_hbadisc.c +1 -2

diff -r 110aa94129d0 drivers/scsi/lpfc/lpfc_hbadisc.c
@@ -470,9 +470,8 @@ check_work_wait_done(struct lpfc_hba *ph
 
 
 int
-lpfc_do_work(void *p)
+lpfc_do_work(struct lpfc_hba *phba)
 {
-	struct lpfc_hba *phba = p;
 	int rc;
 	DECLARE_WAIT_QUEUE_HEAD_ONSTACK(work_waitq);

drivers/scsi/qla2xxx/qla_os.c +2 -5

diff -r 110aa94129d0 drivers/scsi/qla2xxx/qla_os.c
@@ -210,7 +210,7 @@ qla2x00_stop_timer(scsi_qla_host_t *ha)
 	ha->timer_active = 0;
 }
 
-static int qla2x00_do_dpc(void *data);
+static int qla2x00_do_dpc(scsi_qla_host_t *ha);
 
 static void qla2x00_rst_aen(scsi_qla_host_t *);

@@ -2360,15 +2360,12 @@ qla2x00_free_sp_pool( scsi_qla_host_t *h
 * bits then wake us up.
 **************************************************************************/
 static int
-qla2x00_do_dpc(void *data)
+qla2x00_do_dpc(scsi_qla_host_t *ha)
 {
 	int		rval;
-	scsi_qla_host_t *ha;
 	fc_port_t	*fcport;
 	uint8_t		status;
 	uint16_t	next_loopid;
-
-	ha = (scsi_qla_host_t *)data;
 
 	set_user_nice(current, -20);

drivers/scsi/scsi_error.c +1 -3

diff -r 110aa94129d0 drivers/scsi/scsi_error.c
@@ -1541,10 +1541,8 @@ static void scsi_unjam_host(struct Scsi_
  *    This is the main error handling loop.  This is run as a kernel thread
  *    for every SCSI host and handles all error handling activity.
  **/
-int scsi_error_handler(void *data)
+int scsi_error_handler(struct Scsi_Host *shost)
 {
-	struct Scsi_Host *shost = data;
-
 	/*
 	 * We use TASK_INTERRUPTIBLE so that the thread is not
 	 * counted against the load average as a running process.

drivers/scsi/scsi_priv.h +1 -1

diff -r 110aa94129d0 drivers/scsi/scsi_priv.h
@@ -53,7 +53,7 @@ extern void scsi_add_timer(struct scsi_c
 		void (*)(struct scsi_cmnd *));
 extern int scsi_delete_timer(struct scsi_cmnd *);
 extern void scsi_times_out(struct scsi_cmnd *cmd);
-extern int scsi_error_handler(void *host);
+extern int scsi_error_handler(struct Scsi_Host *shost);
 extern int scsi_decide_disposition(struct scsi_cmnd *cmd);
 extern void scsi_eh_wakeup(struct Scsi_Host *shost);
 extern int scsi_eh_scmd_add(struct scsi_cmnd *, int);

drivers/scsi/scsi_scan.c +1 -2

diff -r 110aa94129d0 drivers/scsi/scsi_scan.c
@@ -1788,9 +1788,8 @@ static void do_scsi_scan_host(struct Scs
 	}
 }
 
-static int do_scan_async(void *_data)
+static int do_scan_async(struct async_scan_data *data)
 {
-	struct async_scan_data *data = _data;
 	do_scsi_scan_host(data->shost);
 	scsi_finish_async_scan(data);
 	return 0;

drivers/usb/atm/ueagle-atm.c +1 -2

diff -r 110aa94129d0 drivers/usb/atm/ueagle-atm.c
@@ -1868,9 +1868,8 @@ static int uea_start_reset(struct uea_so
  * Monitor the modem every 1s.
  */
 
-static int uea_kthread(void *data)
+static int uea_kthread(struct uea_softc *sc)
 {
-	struct uea_softc *sc = data;
 	int ret = -EAGAIN;
 
 	set_freezable();

drivers/usb/gadget/file_storage.c +2 -4

diff -r 110aa94129d0 drivers/usb/gadget/file_storage.c
@@ -3383,10 +3383,8 @@ static void handle_exception(struct fsg_
 
 /*-------------------------------------------------------------------------*/
 
-static int fsg_main_thread(void *fsg_)
-{
-	struct fsg_dev		*fsg = fsg_;
-
+static int fsg_main_thread(struct fsg_dev *fsg)
+{
 	/* Allow the thread to be killed by a signal, but set the signal mask
 	 * to block everything but INT, TERM, KILL, and USR1. */
 	allow_signal(SIGINT);

drivers/usb/storage/usb.c +2 -5

diff -r 110aa94129d0 drivers/usb/storage/usb.c
@@ -302,9 +302,8 @@ void fill_inquiry_response(struct us_dat
 	usb_stor_set_xfer_buf(data, data_len, us->srb);
 }
 
-static int usb_stor_control_thread(void * __us)
+static int usb_stor_control_thread(struct us_data *us)
 {
-	struct us_data *us = (struct us_data *)__us;
 	struct Scsi_Host *host = us_to_host(us);
 
 	for(;;) {

@@ -895,10 +894,8 @@ static void release_everything(struct us
 }
 
 /* Thread to carry out delayed SCSI-device scanning */
-static int usb_stor_scan_thread(void * __us)
+static int usb_stor_scan_thread(struct us_data *us)
 {
-	struct us_data *us = (struct us_data *)__us;
-
 	printk(KERN_DEBUG
 		"usb-storage: device found at %d\n", us->pusb_dev->devnum);

drivers/w1/w1.c +1 -3

diff -r 110aa94129d0 drivers/w1/w1.c
@@ -879,10 +879,8 @@ void w1_search_process(struct w1_master 
 		dev->search_count--;
 }
 
-int w1_process(void *data)
+int w1_process(struct w1_master *dev)
 {
-	struct w1_master *dev = (struct w1_master *) data;
-
 	while (!kthread_should_stop() && !test_bit(W1_MASTER_NEED_EXIT, &dev->flags)) {
 		try_to_freeze();
 		msleep_interruptible(w1_timeout * 1000);

drivers/w1/w1.h +1 -1

diff -r 110aa94129d0 drivers/w1/w1.h
@@ -217,7 +217,7 @@ extern struct list_head w1_masters;
 extern struct list_head w1_masters;
 extern struct mutex w1_mlock;
 
-extern int w1_process(void *);
+extern int w1_process(struct w1_master *);
 
 #endif /* __KERNEL__ */

fs/dlm/recoverd.c +2 -2

diff -r 110aa94129d0 fs/dlm/recoverd.c
@@ -260,7 +260,7 @@ static void do_ls_recovery(struct dlm_ls
 	}
 }
 
-static int dlm_recoverd(void *arg)
+static int dlm_recoverd(dlm_lockspace_t *arg)
 {
 	struct dlm_ls *ls;

@@ -295,7 +295,7 @@ int dlm_recoverd_start(struct dlm_ls *ls
 	struct task_struct *p;
 	int error = 0;
 
-	p = kthread_run(dlm_recoverd, ls, "dlm_recoverd");
+	p = kthread_run(dlm_recoverd, (dlm_lockspace_t *)ls, "dlm_recoverd");
 	if (IS_ERR(p))
 		error = PTR_ERR(p);
 	else

fs/gfs2/daemon.c +4 -9

diff -r 110aa94129d0 fs/gfs2/daemon.c
@@ -42,10 +42,8 @@
  * Number of daemons can be set by user, with num_glockd mount option.
  */
 
-int gfs2_glockd(void *data)
+int gfs2_glockd(struct gfs2_sbd *sdp)
 {
-	struct gfs2_sbd *sdp = data;
-
 	while (!kthread_should_stop()) {
 		while (atomic_read(&sdp->sd_reclaim_count))
 			gfs2_reclaim_glock(sdp);

@@ -66,9 +64,8 @@ int gfs2_glockd(void *data)
  *
  */
 
-int gfs2_recoverd(void *data)
+int gfs2_recoverd(struct gfs2_sbd *sdp)
 {
-	struct gfs2_sbd *sdp = data;
 	unsigned long t;
 
 	while (!kthread_should_stop()) {

@@ -90,9 +87,8 @@ int gfs2_recoverd(void *data)
  * journal index.
  */
 
-int gfs2_logd(void *data)
+int gfs2_logd(struct gfs2_sbd *sdp)
 {
-	struct gfs2_sbd *sdp = data;
 	struct gfs2_holder ji_gh;
 	unsigned long t;
 	int need_flush;

@@ -138,9 +134,8 @@ int gfs2_logd(void *data)
  *
  */
 
-int gfs2_quotad(void *data)
+int gfs2_quotad(struct gfs2_sbd *sdp)
 {
-	struct gfs2_sbd *sdp = data;
 	unsigned long t;
 	int error;

fs/gfs2/daemon.h +4 -4

diff -r 110aa94129d0 fs/gfs2/daemon.h
@@ -10,9 +10,9 @@
 #ifndef __DAEMON_DOT_H__
 #define __DAEMON_DOT_H__
 
-int gfs2_glockd(void *data);
-int gfs2_recoverd(void *data);
-int gfs2_logd(void *data);
-int gfs2_quotad(void *data);
+int gfs2_glockd(struct gfs2_sbd *sdp);
+int gfs2_recoverd(struct gfs2_sbd *sdp);
+int gfs2_logd(struct gfs2_sbd *sdp);
+int gfs2_quotad(struct gfs2_sbd *sdp);
 
 #endif /* __DAEMON_DOT_H__ */

fs/gfs2/locking/dlm/thread.c +5 -6

diff -r 110aa94129d0 fs/gfs2/locking/dlm/thread.c
@@ -268,9 +268,8 @@ static inline int check_drop(struct gdlm
 	return 0;
 }
 
-static int gdlm_thread(void *data, int blist)
+static int gdlm_thread(struct gdlm_ls *ls, int blist)
 {
-	struct gdlm_ls *ls = (struct gdlm_ls *) data;
 	struct gdlm_lock *lp = NULL;
 	uint8_t complete, blocking, submit, drop;
 	DECLARE_WAITQUEUE(wait, current);

@@ -329,14 +328,14 @@ static int gdlm_thread(void *data, int b
 	return 0;
 }
 
-static int gdlm_thread1(void *data)
+static int gdlm_thread1(struct gdlm_ls *ls)
 {
-	return gdlm_thread(data, 1);
+	return gdlm_thread(ls, 1);
 }
 
-static int gdlm_thread2(void *data)
+static int gdlm_thread2(struct gdlm_ls *ls)
 {
-	return gdlm_thread(data, 0);
+	return gdlm_thread(ls, 0);
 }
 
 int gdlm_init_threads(struct gdlm_ls *ls)

fs/jbd/journal.c +1 -2

diff -r 110aa94129d0 fs/jbd/journal.c
@@ -112,9 +112,8 @@ static void commit_timeout(unsigned long
  *    known as checkpointing, and this thread is responsible for that job.
  */
 
-static int kjournald(void *arg)
+static int kjournald(journal_t *journal)
 {
-	journal_t *journal = arg;
 	transaction_t *transaction;
 
 	/*

fs/jbd2/journal.c +1 -2

diff -r 110aa94129d0 fs/jbd2/journal.c
@@ -112,9 +112,8 @@ static void commit_timeout(unsigned long
  *    known as checkpointing, and this thread is responsible for that job.
  */
 
-static int kjournald2(void *arg)
+static int kjournald2(journal_t *journal)
 {
-	journal_t *journal = arg;
 	transaction_t *transaction;
 
 	/*

fs/nfs/delegation.c +1 -2

diff -r 110aa94129d0 fs/nfs/delegation.c
@@ -275,9 +275,8 @@ restart:
 	rcu_read_unlock();
 }
 
-static int nfs_do_expire_all_delegations(void *ptr)
+static int nfs_do_expire_all_delegations(struct nfs_client *clp)
 {
-	struct nfs_client *clp = ptr;
 	struct nfs_delegation *delegation;
 	struct inode *inode;

fs/nfs/nfs4state.c +2 -3

diff -r 110aa94129d0 fs/nfs/nfs4state.c
@@ -738,7 +738,7 @@ out:
 	return status;
 }
 
-static int reclaimer(void *);
+static int reclaimer(struct nfs_client *clp);
 
 static inline void nfs4_clear_recover_bit(struct nfs_client *clp)
 {

@@ -895,9 +895,8 @@ static void nfs4_state_mark_reclaim(stru
 	}
 }
 
-static int reclaimer(void *ptr)
+static int reclaimer(struct nfs_client *clp)
 {
-	struct nfs_client *clp = ptr;
 	struct nfs4_state_owner *sp;
 	struct rb_node *pos;
 	struct nfs4_state_recovery_ops *ops;

fs/nfsd/nfs4callback.c +1 -2

diff -r 110aa94129d0 fs/nfsd/nfs4callback.c
@@ -347,9 +347,8 @@ static struct rpc_version *	nfs_cb_versi
 /* Reference counting, callback cleanup, etc., all look racy as heck.
  * And why is cb_set an atomic? */
 
-static int do_probe_callback(void *data)
+static int do_probe_callback(struct nfs4_client *clp)
 {
-	struct nfs4_client *clp = data;
 	struct nfs4_callback *cb = &clp->cl_callback;
 	struct rpc_message msg = {
 		.rpc_proc       = &nfs4_cb_procedures[NFSPROC4_CLNT_CB_NULL],

fs/nfsd/nfs4state.c +1 -3

diff -r 110aa94129d0 fs/nfsd/nfs4state.c
@@ -1312,10 +1312,8 @@ nfs4_file_downgrade(struct file *filp, u
  * Recall a delegation
  */
 static int
-do_recall(void *__dp)
+do_recall(struct nfs4_delegation *dp)
 {
-	struct nfs4_delegation *dp = __dp;
-
 	dp->dl_file->fi_had_conflict = true;
 	nfsd4_cb_recall(dp);
 	return 0;

fs/ocfs2/cluster/heartbeat.c +1 -2

diff -r 110aa94129d0 fs/ocfs2/cluster/heartbeat.c
@@ -840,10 +840,9 @@ static unsigned int o2hb_elapsed_msecs(s
  * dir is removed and drops it ref it will wait to tear down this
  * thread.
  */
-static int o2hb_thread(void *data)
+static int o2hb_thread(struct o2hb_region *reg)
 {
 	int i, ret;
-	struct o2hb_region *reg = data;
 	struct o2hb_bio_wait_ctxt write_wc;
 	struct timeval before_hb, after_hb;
 	unsigned int elapsed_msec;

fs/ocfs2/dlm/dlmrecovery.c +2 -3

diff -r 110aa94129d0 fs/ocfs2/dlm/dlmrecovery.c
@@ -55,7 +55,7 @@
 
 static void dlm_do_local_recovery_cleanup(struct dlm_ctxt *dlm, u8 dead_node);
 
-static int dlm_recovery_thread(void *data);
+static int dlm_recovery_thread(struct dlm_ctxt *dlm);
 void dlm_complete_recovery_thread(struct dlm_ctxt *dlm);
 int dlm_launch_recovery_thread(struct dlm_ctxt *dlm);
 void dlm_kick_recovery_thread(struct dlm_ctxt *dlm);

@@ -302,10 +302,9 @@ static void dlm_print_reco_node_status(s
 
 #define DLM_RECO_THREAD_TIMEOUT_MS (5 * 1000)
 
-static int dlm_recovery_thread(void *data)
+static int dlm_recovery_thread(struct dlm_ctxt *dlm)
 {
 	int status;
-	struct dlm_ctxt *dlm = data;
 	unsigned long timeout = msecs_to_jiffies(DLM_RECO_THREAD_TIMEOUT_MS);
 
 	mlog(0, "dlm thread running for %s...\n", dlm->name);

fs/ocfs2/dlm/dlmthread.c +2 -3

diff -r 110aa94129d0 fs/ocfs2/dlm/dlmthread.c
@@ -53,7 +53,7 @@
 #define MLOG_MASK_PREFIX (ML_DLM|ML_DLM_THREAD)
 #include "cluster/masklog.h"
 
-static int dlm_thread(void *data);
+static int dlm_thread(struct dlm_ctxt *dlm);
 static void dlm_flush_asts(struct dlm_ctxt *dlm);
 
 #define dlm_lock_is_remote(dlm, lock)     ((lock)->ml.node != (dlm)->node_num)

@@ -608,10 +608,9 @@ static void dlm_flush_asts(struct dlm_ct
 #define DLM_THREAD_MAX_DIRTY  100
 #define DLM_THREAD_MAX_ASTS   10
 
-static int dlm_thread(void *data)
+static int dlm_thread(struct dlm_ctxt *dlm)
 {
 	struct dlm_lock_resource *res;
-	struct dlm_ctxt *dlm = data;
 	unsigned long timeout = msecs_to_jiffies(DLM_THREAD_TIMEOUT_MS);
 
 	mlog(0, "dlm thread running for %s...\n", dlm->name);

fs/ocfs2/journal.c +4 -6

diff -r 110aa94129d0 fs/ocfs2/journal.c
@@ -54,7 +54,7 @@ static int ocfs2_force_read_journal(stru
 static int ocfs2_force_read_journal(struct inode *inode);
 static int ocfs2_recover_node(struct ocfs2_super *osb,
 			      int node_num);
-static int __ocfs2_recovery_thread(void *arg);
+static int __ocfs2_recovery_thread(struct ocfs2_super *osb);
 static int ocfs2_commit_cache(struct ocfs2_super *osb);
 static int ocfs2_wait_on_mount(struct ocfs2_super *osb);
 static int ocfs2_journal_toggle_dirty(struct ocfs2_super *osb,

@@ -63,7 +63,7 @@ static int ocfs2_trylock_journal(struct 
 				 int slot_num);
 static int ocfs2_recover_orphans(struct ocfs2_super *osb,
 				 int slot);
-static int ocfs2_commit_thread(void *arg);
+static int ocfs2_commit_thread(struct ocfs2_super *osb);
 
 static int ocfs2_commit_cache(struct ocfs2_super *osb)
 {

@@ -841,10 +841,9 @@ void ocfs2_complete_mount_recovery(struc
 	}
 }
 
-static int __ocfs2_recovery_thread(void *arg)
+static int __ocfs2_recovery_thread(struct ocfs2_super *osb)
 {
 	int status, node_num;
-	struct ocfs2_super *osb = arg;
 
 	mlog_entry_void();

@@ -1419,10 +1418,9 @@ static int ocfs2_wait_on_mount(struct oc
 	return 0;
 }
 
-static int ocfs2_commit_thread(void *arg)
+static int ocfs2_commit_thread(struct ocfs2_super *osb)
 {
 	int status;
-	struct ocfs2_super *osb = arg;
 	struct ocfs2_journal *journal = osb->journal;
 
 	/* we can trust j_num_trans here because _should_stop() is only set in

fs/ocfs2/vote.c +1 -2

diff -r 110aa94129d0 fs/ocfs2/vote.c
@@ -280,10 +280,9 @@ static int ocfs2_vote_thread_should_wake
 	return should_wake;
 }
 
-int ocfs2_vote_thread(void *arg)
+int ocfs2_vote_thread(struct ocfs2_super *osb)
 {
 	int status = 0;
-	struct ocfs2_super *osb = arg;
 
 	/* only quit once we've been asked to stop and there is no more
 	 * work available */

fs/ocfs2/vote.h +1 -1

diff -r 110aa94129d0 fs/ocfs2/vote.h
@@ -27,7 +27,7 @@
 #ifndef VOTE_H
 #define VOTE_H
 
-int ocfs2_vote_thread(void *arg);
+int ocfs2_vote_thread(struct ocfs2_super *osb);
 static inline void ocfs2_kick_vote_thread(struct ocfs2_super *osb)
 {
 	spin_lock(&osb->vote_task_lock);

fs/xfs/linux-2.6/xfs_buf.c +2 -3

diff -r 110aa94129d0 fs/xfs/linux-2.6/xfs_buf.c
@@ -35,7 +35,7 @@
 #include <linux/freezer.h>
 
 static kmem_zone_t *xfs_buf_zone;
-STATIC int xfsbufd(void *);
+STATIC int xfsbufd(xfs_buftarg_t *);
 STATIC int xfsbufd_wakeup(int, gfp_t);
 STATIC void xfs_buf_delwri_queue(xfs_buf_t *, int);
 static struct shrinker xfs_buf_shake = {

@@ -1734,10 +1734,9 @@ xfs_buf_delwri_split(
 
 STATIC int
 xfsbufd(
-	void		*data)
+	xfs_buftarg_t	*target)
 {
 	struct list_head tmp;
-	xfs_buftarg_t	*target = (xfs_buftarg_t *)data;
 	int		count;
 	xfs_buf_t	*bp;

fs/xfs/linux-2.6/xfs_super.c +1 -2

diff -r 110aa94129d0 fs/xfs/linux-2.6/xfs_super.c
@@ -549,9 +549,8 @@ xfs_sync_worker(
 
 STATIC int
 xfssyncd(
-	void			*arg)
+	struct xfs_mount	*mp)
 {
-	struct xfs_mount	*mp = arg;
 	long			timeleft;
 	bhv_vfs_sync_work_t	*work, *n;
 	LIST_HEAD		(tmp);

kernel/audit.c +1 -2

diff -r 110aa94129d0 kernel/audit.c
@@ -450,9 +450,8 @@ out:
 	return err;
 }
 
-int audit_send_list(void *_dest)
+int audit_send_list(struct audit_netlink_list *dest)
 {
-	struct audit_netlink_list *dest = _dest;
 	int pid = dest->pid;
 	struct sk_buff *skb;

kernel/audit.h +1 -1

diff -r 110aa94129d0 kernel/audit.h
@@ -126,7 +126,7 @@ struct audit_netlink_list {
 	struct sk_buff_head q;
 };
 
-int audit_send_list(void *);
+int audit_send_list(struct audit_netlink_list *dest);
 
 struct inotify_watch;
 extern void audit_free_parent(struct inotify_watch *);

kernel/rtmutex-tester.c +1 -2

diff -r 110aa94129d0 kernel/rtmutex-tester.c
@@ -254,9 +254,8 @@ void schedule_rt_mutex_test(struct rt_mu
 	td->opdata = dat;
 }
 
-static int test_func(void *data)
+static int test_func(struct test_thread_data *td)
 {
-	struct test_thread_data *td = data;
 	int ret;
 
 	current->flags |= PF_MUTEX_TESTER;

kernel/stop_machine.c +1 -2

diff -r 110aa94129d0 kernel/stop_machine.c
@@ -143,9 +143,8 @@ struct stop_machine_data
 	struct completion done;
 };
 
-static int do_stop(void *_smdata)
+static int do_stop(struct stop_machine_data *smdata)
 {
-	struct stop_machine_data *smdata = _smdata;
 	int ret;
 
 	ret = stop_machine();

kernel/workqueue.c +1 -2

diff -r 110aa94129d0 kernel/workqueue.c
@@ -296,9 +296,8 @@ static void run_workqueue(struct cpu_wor
 	spin_unlock_irq(&cwq->lock);
 }
 
-static int worker_thread(void *__cwq)
+static int worker_thread(struct cpu_workqueue_struct *cwq)
 {
-	struct cpu_workqueue_struct *cwq = __cwq;
 	DEFINE_WAIT(wait);
 
 	if (cwq->wq->freezeable)

mm/vmscan.c +1 -2

diff -r 110aa94129d0 mm/vmscan.c
@@ -1491,10 +1491,9 @@ out:
  * If there are applications that are active memory-allocators
  * (most normal use), this basically shouldn't matter.
  */
-static int kswapd(void *p)
+static int kswapd(pg_data_t *pgdat)
 {
 	unsigned long order;
-	pg_data_t *pgdat = (pg_data_t*)p;
 	struct task_struct *tsk = current;
 	DEFINE_WAIT(wait);
 	struct reclaim_state reclaim_state = {

net/9p/mux.c +2 -4

diff -r 110aa94129d0 net/9p/mux.c
@@ -106,7 +106,7 @@ struct p9_mux_rpc {
 	wait_queue_head_t wqueue;
 };
 
-static int p9_poll_proc(void *);
+static int p9_poll_proc(struct p9_mux_poll_task *vpt);
 static void p9_read_work(struct work_struct *work);
 static void p9_write_work(struct work_struct *work);
 static void p9_pollwait(struct file *filp, wait_queue_head_t *wait_address,

@@ -442,12 +442,10 @@ static void p9_poll_mux(struct p9_conn *
  * p9_poll_proc - polls all v9fs transports for new events and queues
  * 	the appropriate work to the work queue
  */
-static int p9_poll_proc(void *a)
+static int p9_poll_proc(struct p9_mux_poll_task *vpt)
 {
 	struct p9_conn *m, *mtmp;
-	struct p9_mux_poll_task *vpt;
 
-	vpt = a;
 	P9_DPRINTK(P9_DEBUG_MUX, "start %p %p\n", current, vpt);
 	while (!kthread_should_stop()) {
 		set_current_state(TASK_INTERRUPTIBLE);

net/core/pktgen.c +1 -2

diff -r 110aa94129d0 net/core/pktgen.c
@@ -3499,10 +3499,9 @@ out:;
  * Main loop of the thread goes here
  */
 
-static int pktgen_thread_worker(void *arg)
+static int pktgen_thread_worker(struct pktgen_thread *t)
 {
 	DEFINE_WAIT(wait);
-	struct pktgen_thread *t = arg;
 	struct pktgen_dev *pkt_dev = NULL;
 	int cpu = t->cpu;

sound/pci/emu10k1/emu10k1_main.c +1 -2

diff -r 110aa94129d0 sound/pci/emu10k1/emu10k1_main.c
@@ -704,8 +704,7 @@ static int snd_emu1010_load_firmware(str
 	return 0;
 }
 
-int emu1010_firmware_thread(void *data) {
-	struct snd_emu10k1 * emu = data;
+int emu1010_firmware_thread(struct snd_emu10k1 * emu) {
 	int tmp,tmp2;
 	int reg;
 	int err;

Rusty Russell

author

2008-01-20 09:52 UTC #4 lore raw

PATCH 4/6 typesafe: cast_if_type to allow macros functions which take more than one type.

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 <[email protected]>

include/linux/compiler-gcc.h +17 -0

diff -r e6626cc7bdc2 include/linux/compiler-gcc.h
@@ -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))

include/linux/compiler-intel.h +2 -0

diff -r e6626cc7bdc2 include/linux/compiler-intel.h
@@ -29,3 +29,5 @@
 #endif
 
 #define uninitialized_var(x) x
+
+#define cast_if_type(expr, oktype, desttype) ((desttype)(expr))

Rusty Russell

author

2008-01-20 09:54 UTC #5 lore raw

PATCH 5/6 typesafe: request_irq and devm_request_irq

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 <[email protected]>

---
 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(-)

include/linux/interrupt.h +15 -2

diff -r 0fe1a980708b include/linux/interrupt.h
@@ -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);

kernel/irq/devres.c +5 -5

diff -r 0fe1a980708b kernel/irq/devres.c
@@ -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

kernel/irq/manage.c +3 -3

diff -r 0fe1a980708b kernel/irq/manage.c
@@ -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);

Rusty Russell

author

2008-01-20 09:58 UTC #6 lore raw

PATCH 6/6 typesafe: timers

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 <[email protected]>

include/linux/timer.h +16 -8

diff -r 142a2cf4a8dc include/linux/timer.h
@@ -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;

Jan Engelhardt

2008-01-20 11:25 UTC #7 lore raw

Re: [PATCH 2/6] typesafe: kthread_create and kthread_run

In reply to Rusty Russell

include/linux/kthread.h line 29

Quoted 5 lines · Rusty Russell

+ */

include/linux/kthread.h +4 -0

+#define kthread_create(threadfn, data, namefmt...) ({		\
+	int (*_threadfn)(typeof(data)) = (threadfn);		\
+	__kthread_create((void *)_threadfn, (data), namefmt);	\
+})

If you have namefmt... you need that varagrs cpp trick. IIRC:

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

Bert Wesarg

2008-01-20 12:07 UTC #8 lore raw

Re: [PATCH 2/6] typesafe: kthread_create and kthread_run

Quoted 11 lines · Jan Engelhardt

include/linux/kthread.h line 29
Quoted 5 lines · Rusty Russell
+ */
include/linux/kthread.h +4 -0
+#define kthread_create(threadfn, data, namefmt...) ({         \
+      int (*_threadfn)(typeof(data)) = (threadfn);            \
+      __kthread_create((void *)_threadfn, (data), namefmt);   \
+})
If you have namefmt... you need that varagrs cpp trick. IIRC:
        __kthread_create((void *)_threadfn, (data), namefmt, __VA_ARGS__);

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__);

Bert

Quoted 7 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Tejun Heo

2008-01-20 12:57 UTC #13 lore raw

In reply to Rusty Russell

Quoted 9 lines · Rusty Russell

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.

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.

Signature · 1 line

tejun

Tejun Heo

2008-01-20 13:01 UTC #14 lore raw

Quoted 3 lines · Tejun Heo

* Passing NULL as data to callback which takes non-void pointer triggers warning. I think this should be allowed. Something like the following?

Please ignore the last sentence.

Quoted 6 lines

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

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...

Signature · 1 line

tejun

Johannes Weiner

2008-01-20 16:24 UTC #9 lore raw

Re: [PATCH 2/6] typesafe: kthread_create and kthread_run

In reply to Bert Wesarg

Hi,

Quoted 11 lines · Bert Wesarg

Quoted 10 lines · Jan Engelhardt
include/linux/kthread.h line 29
Quoted 5 lines · Rusty Russell
+ */
include/linux/kthread.h +4 -0
+#define kthread_create(threadfn, data, namefmt...) ({         \
+      int (*_threadfn)(typeof(data)) = (threadfn);            \
+      __kthread_create((void *)_threadfn, (data), namefmt);   \
+})
If you have namefmt... you need that varagrs cpp trick. IIRC:

Quoted 8 lines

Quoted 1 line

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

almost

either:

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

No. This is bad because it gives the impression that it takes only two essential arguments which is not the case.

Quoted 5 lines

or:

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

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

Bert Wesarg

2008-01-20 16:43 UTC #10 lore raw

Re: [PATCH 2/6] typesafe: kthread_create and kthread_run

Quoted 18 lines · Johannes Weiner

Hi,
Quoted 14 lines · Bert Wesarg
Quoted 11 lines · Jan Engelhardt
include/linux/kthread.h line 29
Quoted 5 lines · Rusty Russell
+ */
include/linux/kthread.h +4 -0
+#define kthread_create(threadfn, data, namefmt...) ({         \
+      int (*_threadfn)(typeof(data)) = (threadfn);            \
+      __kthread_create((void *)_threadfn, (data), namefmt);   \
+})
If you have namefmt... you need that varagrs cpp trick. IIRC:
        __kthread_create((void *)_threadfn, (data), namefmt, __VA_ARGS__);
almost

Quoted 8 lines

Quoted 5 lines
either:
#define kthread_create(threadfn, data, ...) ({         \
        __kthread_create((void *)_threadfn, (data), __VA_ARGS__);
No. This is bad because it gives the impression that it takes only two essential arguments which is not the case.

Right, I just forget to mention this in a comment.

Quoted 7 lines

Quoted 6 lines

or:

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

Quoted 8 lines

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.

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

Bert

Quoted 3 lines

Hannes

Rusty Russell

author

2008-01-20 22:05 UTC #11 lore raw

Re: [PATCH 2/6] typesafe: kthread_create and kthread_run

Quoted 1 line · On Monday 21 January 2008 03:43:40 Bert Wesarg wrote:

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.

Rusty Russell

author

2008-01-20 22:18 UTC #15 lore raw

In reply to Tejun Heo

Quoted 3 lines · On Monday 21 January 2008 00:00:52 Tejun Heo wrote:

What should be do are
* Check that the threadfn's argument fits into void *.

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)),
			      (void *)(unsigned long)(data), (data))

Quoted 2 lines

* Trigger overflow in implicit constant conversion warning if the specified data is too large for the argument type.

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.

Bert Wesarg

2008-01-21 07:56 UTC #12 lore raw

Re: [PATCH 2/6] typesafe: kthread_create and kthread_run

In reply to Rusty Russell

Quoted 10 lines · Rusty Russell

Quoted 1 line · On Monday 21 January 2008 03:43:40 Bert Wesarg wrote:
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.

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
Bert

Quoted 4 lines

Cheers,
Rusty.

Rusty Russell

author

2008-01-21 11:34 UTC #16 lore raw

In reply to Rusty Russell

Quoted 9 lines · On Monday 21 January 2008 09:17:30 Rusty Russell wrote:

Quoted 3 lines · On Monday 21 January 2008 00:00:52 Tejun Heo wrote:
What should be do are
* Check that the threadfn's argument fits into void *.
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.

...

Quoted 1 line

I'll test this out and see what I can make...

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 <[email protected]>

---
 include/linux/compiler-gcc.h   |   72 +++++++++++++++++++++++++++++++++++++++++
 include/linux/compiler-intel.h |    3 +
 2 files changed, 75 insertions(+)

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

diff -r f41638047650 include/linux/compiler-gcc.h
@@ -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 takes "void *" in two
+ * cases: a function which takes a different pointer type or an
+ * unsigned long.
+ *
+ * Typechecking is done in two stages: this macro handles the cases where
+ * @fn takes an unsigned long and @arg is compatible with that, and also
+ * the case where @fn and @arg match types.  For other cases, @fn is not
+ * cast, so using the results of this macro should cause a warning unless
+ * @fn is already of if type @fntype.
+ *
+ * Logic looks like this:
+ * if (typeof(@arg) compatible with unsigned long) {
+ *     if (typeof(@fn) == @ulongtype)
+ *         (@fntype)(@fn); // OK!
+ *     else
+ *         @fn; // will give type warning unless it's of @fntype already.
+ * } else if (typeof(@fn) == @safetype)
+ *     (@fntype)(@fn); // OK!
+ * else
+ *     (@fn);  // will give type warning unless it's of @fntype already.
+ */
+#define typesafe_function(fn, ulongtype, safetype, fntype, arg)		\
+__builtin_choose_expr((ulong_compatible(arg)				\
+		       && __builtin_types_compatible_p(typeof(1?(fn):NULL), \
+						       ulongtype))	\
+		      || (!ulong_compatible(arg)			\
+			  && __builtin_types_compatible_p(typeof(1?(fn):NULL), \
+							  safetype)),	\
+		      ((fntype)(fn)),					\
+		      (fn))
+
+/**
+ * typesafe_arg - cast to void * if function expects an unsigned long
+ * @fn: the function or function pointer
+ * @ulongtype: the function pointer type if arg is an integer
+ * @arg: the function argument
+ *
+ * If @fn expects an unsigned long, cast @arg, otherwise leave it
+ * alone (if it's not void * compatible, this will cause a warning
+ * when used).
+ *
+ * Note that we've already checked @arg/@fn compatibility in
+ * typesafe_function(): this makes sure that @arg is a pointer or an integer
+ * if @fn expects an unsigned long.
+ */
+#define typesafe_arg(fn, ulongtype, arg)				\
+__builtin_choose_expr(__builtin_types_compatible_p(typeof(1?(fn):NULL), \
+						   ulongtype),		\
+		      ((void *)(long)(arg)), (arg))

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

diff -r f41638047650 include/linux/compiler-intel.h
@@ -29,3 +29,6 @@
 #endif
 
 #define uninitialized_var(x) x
+
+#define typesafe_function(fn, ulongtype, safetype, fntype, arg) ((fntype)(fn))
+#define typesafe_arg(fn, ulongtype, arg) ((void *)arg)

===
typesafe: Convert stop_machine and callers

Signed-off-by Rusty Russell <[email protected]>

---
 drivers/char/hw_random/intel-rng.c |    3 -
 include/linux/stop_machine.h       |   16 ++++--
 kernel/module.c                    |   10 +---
 kernel/stop_machine.c              |   89 ++++++++++++++++++++++++++++++++++++-
 4 files changed, 103 insertions(+), 15 deletions(-)

drivers/char/hw_random/intel-rng.c +1 -2

diff -r e279190b7b43 drivers/char/hw_random/intel-rng.c
@@ -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 */

include/linux/stop_machine.h +12 -4

diff -r e279190b7b43 include/linux/stop_machine.h
@@ -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,15 @@
  *
  * 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_function((fn), int(*)(unsigned long),	\
+					  int(*)(typeof(data)),		\
+					  int(*)(void *), (data)),	\
+			typesafe_arg((fn), int(*)(unsigned long), (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 +46,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();

kernel/module.c +3 -7

diff -r e279190b7b43 kernel/module.c
@@ -623,10 +623,8 @@ struct stopref
 };
 
 /* Whole machine is stopped with interrupts off when this runs. */
-static int __try_stop_module(void *_sref)
+static int __try_stop_module(struct stopref *sref)
 {
-	struct stopref *sref = _sref;
-
 	/* If it's not unused, quit unless we are told to block. */
 	if ((sref->flags & O_NONBLOCK) && module_refcount(sref->mod) != 0) {
 		if (!(*sref->forced = try_force_unload(sref->flags)))

@@ -1305,9 +1303,8 @@ static void mod_kobject_remove(struct mo
  * link the module with the whole machine is stopped with interrupts off
  * - this defends against kallsyms not taking locks
  */
-static int __link_module(void *_mod)
+static int __link_module(struct module *mod)
 {
-	struct module *mod = _mod;
 	list_add(&mod->list, &modules);
 	return 0;
 }

@@ -1316,9 +1313,8 @@ static int __link_module(void *_mod)
  * unlink the module with the whole machine is stopped with interrupts off
  * - this defends against kallsyms not taking locks
  */
-static int __unlink_module(void *_mod)
+static int __unlink_module(struct module *mod)
 {
-	struct module *mod = _mod;
 	list_del(&mod->list);
 	return 0;
 }

kernel/stop_machine.c +87 -2

98 lines not on this page · raw message

Tejun Heo

2008-01-21 12:39 UTC #17 lore raw

Quoted 5 lines · Rusty Russell

Quoted 4 lines · On Monday 21 January 2008 09:17:30 Rusty Russell wrote:
Quoted 3 lines · On Monday 21 January 2008 00:00:52 Tejun Heo wrote:
What should be do are
* Check that the threadfn's argument fits into void *.

Quoted 8 lines

Quoted 4 lines
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.
...
Quoted 1 line
I'll test this out and see what I can make...
I think this comes under "too ugly", but here's an attempt.

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.

Signature · 1 line

tejun

Rusty Russell

author

2008-01-21 23:28 UTC #18 lore raw

Quoted 10 lines · On Monday 21 January 2008 23:38:47 Tejun Heo wrote:

Quoted 5 lines · Rusty Russell
Quoted 2 lines · On Monday 21 January 2008 09:17:30 Rusty Russell wrote:
But it would be cool to allow functions which take an unsigned long. I'll test this out and see what I can make...
I think this comes under "too ugly", but here's an attempt.
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. :-(

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 <[email protected]>

---
 include/linux/compiler-gcc.h   |   61 +++++++++++++++++++++++++++++++++++++++++
 include/linux/compiler-intel.h |    3 ++
 include/linux/kernel.h         |   21 ++++++++++++++
 3 files changed, 85 insertions(+)

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

diff -r 951ffe1c5238 include/linux/compiler-gcc.h
@@ -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.
+ *
+ * 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 takes "void *" in two
+ * cases: a function which takes a different pointer type or an
+ * unsigned long.
+ *
+ * Typechecking is done by the caller when they assign or pass these
+ * values.  This macro handles the cases where @fn takes an unsigned
+ * long and @arg is compatible with that, and also the case where @fn
+ * and @arg match types.  For other cases, @fn is not cast, so using
+ * the results of this macro should cause a warning unless @fn is
+ * already of if type @voidtype.
+ *
+ * Logic looks like this:
+ * if (typeof(@arg) compatible with unsigned long) {
+ *     if (typeof(@fn) == @ulongtype)
+ *         (@voidtype)@fn and (void *)(unsigned long)@arg; // OK!
+ *     else
+ *         @fn and @arg; // @fn will warn unless it's of @voidtype already.
+ * } else if (typeof(@fn) == @safetype)
+ *     (@voidtype)@fn and @arg; // OK: @arg will warn unless it's a pointer.
+ * else
+ *     @fn and @arg; // @fn will warn unless it's of @voidtype already.
+ */
+#define typesafe_fn_and_arg(fn, arg, ulongtype, safetype, voidtype)	\
+__builtin_choose_expr((ulong_compatible(arg)				\
+		       && __builtin_types_compatible_p(typeof(1?(fn):NULL), \
+						       ulongtype))	\
+		      || (!ulong_compatible(arg)			\
+			  && __builtin_types_compatible_p(typeof(1?(fn):NULL), \
+							  safetype)),	\
+		      ((voidtype)(fn)),					\
+		      (fn)),						\
+__builtin_choose_expr(__builtin_types_compatible_p(typeof(1?(fn):NULL), \
+						   ulongtype),		\
+		      ((void *)(long)(arg)), (arg))
+

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

diff -r 951ffe1c5238 include/linux/compiler-intel.h
@@ -29,3 +29,6 @@
 #endif
 
 #define uninitialized_var(x) x
+
+#define typesafe_fn_and_arg(fn, arg, ulongtype, safetype, fntype) \
+	((fntype)(fn)), ((void *)arg)

include/linux/kernel.h +21 -0

diff -r 951ffe1c5238 include/linux/kernel.h
@@ -379,6 +379,27 @@ static inline int __attribute__ ((format
 	(void)__tmp; \
 })
 
+/**
+ * callback_and_arg - simple one-argument typesafe callback with argument
+ * @fn: the function or function pointer
+ * @arg: the function argument.
+ * @rettype: the return type of fn
+ *
+ * This macro evaluates to two comma separated values: the function pointer,
+ * then the argument.  It will check that @fn and @arg are compatible with
+ * each other and with a void * callback.  One of
+ * - fn takes a void * and arg is a pointer
+ * - fn takes a typeof(arg) and arg is a pointer
+ * - fn takes an unsigned long and arg is an integer type.
+ *
+ * See typesafe_fn_and_arg() for the gory details.
+ */
+#define callback_and_arg(fn, arg, rettype)		\
+	typesafe_fn_and_arg((fn), (arg),		\
+			    rettype (*)(unsigned long),	\
+			    rettype (*)(typeof(arg)),	\
+			    rettype (*)(void *))
+
 struct sysinfo;
 extern int do_sysinfo(struct sysinfo *info);

Linus Torvalds

2008-01-21 23:57 UTC #19 lore raw

Quoted 3 lines · Rusty Russell

Attempt to create callbacks which take unsigned long as well as correct pointer types.

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

Andi Kleen

2008-01-22 04:21 UTC #22 lore raw

In reply to Rusty Russell

Quoted 3 lines · Rusty Russell

===
Attempt to create callbacks which take unsigned long as well as correct pointer types.

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

Rusty Russell

author

2008-01-22 07:17 UTC #20 lore raw

In reply to Linus Torvalds

Quoted 6 lines · On Tuesday 22 January 2008 10:57:03 Linus Torvalds wrote:

Quoted 2 lines · Rusty Russell
Attempt to create callbacks which take unsigned long as well as correct pointer types.
I bow down before you.

Quoted 7 lines

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.

Err, thanks. I read some old SCSI drivers and felt inspired...

Quoted 3 lines

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..

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.

Linus Torvalds

2008-01-22 15:53 UTC #21 lore raw

Quoted 4 lines · Rusty Russell

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.

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