Improve type handling in interrupt handlers

Just a trivial example.

---
 drivers/lguest/lguest_device.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff -r 00ab7672f658 drivers/lguest/lguest_device.c

Just a trivial example.

---
 drivers/lguest/lguest_device.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff -r 00ab7672f658 drivers/lguest/lguest_device.c

drivers/lguest/lguest_device.c static void lg_notify(struct virtqueue * · line 184

Quoted 5 lines · Rusty Russell

drivers/lguest/lguest_device.c +1 -2

-static irqreturn_t lguest_interrupt(int irq, void *_vq)
+static irqreturn_t lguest_interrupt(int irq, struct virtqueue *vq)
 {
-	struct virtqueue *vq = _vq;
 	struct lguest_device_desc *desc = to_lgdev(vq->vdev)->desc;

Ugh.

This will be a compatibility nightmare. I don't see how void* is so evil for this, or timers.

It's not like there's a huge cost associated with a pointer alias.

drivers/lguest/lguest_device.c static void lg_notify(struct virtqueue * · line 184

Quoted 5 lines · Rusty Russell

drivers/lguest/lguest_device.c +1 -2

-static irqreturn_t lguest_interrupt(int irq, void *_vq)
+static irqreturn_t lguest_interrupt(int irq, struct virtqueue *vq)
 {
-	struct virtqueue *vq = _vq;
 	struct lguest_device_desc *desc = to_lgdev(vq->vdev)->desc;

Ugh.

This will be a compatibility nightmare. I don't see how void* is so evil for this, or timers.

It's not like there's a huge cost associated with a pointer alias.

Just a trivial example.

---
 drivers/lguest/lguest_device.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff -r 00ab7672f658 drivers/lguest/lguest_device.c

Just a trivial example.

---
 drivers/lguest/lguest_device.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff -r 00ab7672f658 drivers/lguest/lguest_device.c

Type safety is good but I doubt this would be worth the complexity. It has some benefits but there's much larger benefit in keeping things in straight C. People know that functions take fixed types and are also familiar with the convention of passing void * for callback arguments. IMHO, staying in line with those common knowledges easily trumps having type checking on interrupt handler.

Also, how often do we see a bug where things go wrong because interrupt handler is given the wrong type of argument? Even when such bug happens, I doubt it can escape the developer's workstation if he/she is paying any attention to testing.

Type safety is good but I doubt this would be worth the complexity. It has some benefits but there's much larger benefit in keeping things in straight C. People know that functions take fixed types and are also familiar with the convention of passing void * for callback arguments. IMHO, staying in line with those common knowledges easily trumps having type checking on interrupt handler.

Also, how often do we see a bug where things go wrong because interrupt handler is given the wrong type of argument? Even when such bug happens, I doubt it can escape the developer's workstation if he/she is paying any attention to testing.

Quoted 6 lines · On Saturday 19 January 2008 10:12:33 Tejun Heo wrote:

Type safety is good but I doubt this would be worth the complexity. It has some benefits but there's much larger benefit in keeping things in straight C. People know that functions take fixed types and are also familiar with the convention of passing void * for callback arguments. IMHO, staying in line with those common knowledges easily trumps having type checking on interrupt handler.

I sympathise with this argument, but I think just because people are familiar with existing hacks shouldn't prevent improvement. I think the resulting code is clearer and more readable.

Even in the implementation, the tricky part is the check_either_type() macro:
the rest is straight-forward.

Quoted 4 lines

Also, how often do we see a bug where things go wrong because interrupt handler is given the wrong type of argument? Even when such bug happens, I doubt it can escape the developer's workstation if he/she is paying any attention to testing.

I agree this one is unlikely. But I am trying to spread type-safety more widely (see previous kthread patches).

I like changing the kernel to make life simpler for developers. We don't do enough of it.

Quoted 6 lines · On Saturday 19 January 2008 10:12:33 Tejun Heo wrote:

Type safety is good but I doubt this would be worth the complexity. It has some benefits but there's much larger benefit in keeping things in straight C. People know that functions take fixed types and are also familiar with the convention of passing void * for callback arguments. IMHO, staying in line with those common knowledges easily trumps having type checking on interrupt handler.

I sympathise with this argument, but I think just because people are familiar with existing hacks shouldn't prevent improvement. I think the resulting code is clearer and more readable.

Even in the implementation, the tricky part is the check_either_type() macro:
the rest is straight-forward.

Quoted 4 lines

Also, how often do we see a bug where things go wrong because interrupt handler is given the wrong type of argument? Even when such bug happens, I doubt it can escape the developer's workstation if he/she is paying any attention to testing.

I agree this one is unlikely. But I am trying to spread type-safety more widely (see previous kthread patches).

I like changing the kernel to make life simpler for developers. We don't do enough of it.

so I think the question is "do we want to change all callbacks to take native pointer type instead of void pointer?".

so I think the question is "do we want to change all callbacks to take native pointer type instead of void pointer?".

Quoted 2 lines · Tejun Heo

so I think the question is "do we want to change all callbacks to take native pointer type instead of void pointer?".

Lemme clarity myself a bit. I'm not saying that we should convert all at once or literally every callback should be converted. What I'm saying is whether we're headed that way in general and converting big ones - timer for example - and getting the conversion agreed upon should be enough to set the norm.

Quoted 2 lines · Tejun Heo

so I think the question is "do we want to change all callbacks to take native pointer type instead of void pointer?".

Lemme clarity myself a bit. I'm not saying that we should convert all at once or literally every callback should be converted. What I'm saying is whether we're headed that way in general and converting big ones - timer for example - and getting the conversion agreed upon should be enough to set the norm.

There are three possibilities: (1) force everyone to use void *, (2) force
everyone to be type-correct, (3) allow both with some tricks. Currently we're on (1). For kthread, with only dozens of users, I chose (2) (very simple, easy to understand). I think for widespread things like timer and interrupt handlers, I think (3) is the right way to go.

I wanted to get this patch out there and see what the reaction was. I can
do timers next, if that's going to add fuel to the discussion.

There are three possibilities: (1) force everyone to use void *, (2) force
everyone to be type-correct, (3) allow both with some tricks. Currently we're on (1). For kthread, with only dozens of users, I chose (2) (very simple, easy to understand). I think for widespread things like timer and interrupt handlers, I think (3) is the right way to go.

I wanted to get this patch out there and see what the reaction was. I can
do timers next, if that's going to add fuel to the discussion.

Quoted 5 lines · Rusty Russell

There are three possibilities: (1) force everyone to use void *, (2)
force everyone to be type-correct, (3) allow both with some tricks. Currently we're on (1). For kthread, with only dozens of users, I chose
(2) (very simple, easy to understand). I think for widespread things like timer and interrupt handlers, I think (3) is the right way to go.

Yeah, during transition, we definitely want (3).

Yeah, I think we need a good flame war to determine our heading and converting timer shouldn't take too much of your time, right?

Quoted 5 lines · Rusty Russell

There are three possibilities: (1) force everyone to use void *, (2)
force everyone to be type-correct, (3) allow both with some tricks. Currently we're on (1). For kthread, with only dozens of users, I chose
(2) (very simple, easy to understand). I think for widespread things like timer and interrupt handlers, I think (3) is the right way to go.

Yeah, during transition, we definitely want (3).

Yeah, I think we need a good flame war to determine our heading and converting timer shouldn't take too much of your time, right?

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

To do this it uses a gcc extension, cast-to-union, which allows a type to be cast to any type within the union. When used in a wrapper macro, it's a simple way of allowing one of several types.

(Some drivers now need to be cleaned up to compile, previous patch).

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

diff -r 0fe1a980708b include/linux/interrupt.h

include/linux/interrupt.h +9 -1

@@ -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),						\
+		      check_either_type((handler), irq_handler_t,	\
+					int (*)(int, typeof(dev_id))),	\
+		      (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;

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

To do this it uses a gcc extension, cast-to-union, which allows a type to be cast to any type within the union. When used in a wrapper macro, it's a simple way of allowing one of several types.

(Some drivers now need to be cleaned up to compile, previous patch).

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

diff -r 0fe1a980708b include/linux/interrupt.h

include/linux/interrupt.h +9 -1

@@ -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),						\
+		      check_either_type((handler), irq_handler_t,	\
+					int (*)(int, typeof(dev_id))),	\
+		      (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;

This improves typechecking of interrupt handlers by removing unnecessary (void *) casts and storing handlers in correctly-typed variables.

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

Cc Jeff Garzik <[email protected]>

Cc Ash Willis <[email protected]>

Cc [email protected]

diff -r 0fe1a980708b drivers/net/eth16i.c

drivers/net/eth16i.c +1 -1

@@ -529,7 +529,7 @@ static int __init eth16i_probe1(struct n
 
 	/* Try to obtain interrupt vector */
 
-	if ((retval = request_irq(dev->irq, (void *)&eth16i_interrupt, 0, cardname, dev))) {
+	if ((retval = request_irq(dev->irq, eth16i_interrupt, 0, cardname, dev))) {
 		printk(KERN_WARNING "%s at %#3x, but is unusable due to conflicting IRQ %d.\n",
 		       cardname, ioaddr, dev->irq);
 		goto out;

diff -r 0fe1a980708b drivers/net/ewrk3.c

drivers/net/ewrk3.c +1 -1

@@ -635,7 +635,7 @@ static int ewrk3_open(struct net_device 
 	STOP_EWRK3;
 
 	if (!lp->hard_strapped) {
-		if (request_irq(dev->irq, (void *) ewrk3_interrupt, 0, "ewrk3", dev)) {
+		if (request_irq(dev->irq, ewrk3_interrupt, 0, "ewrk3", dev)) {
 			printk("ewrk3_open(): Requested IRQ%d is busy\n", dev->irq);
 			status = -EAGAIN;
 		} else {

diff -r 0fe1a980708b drivers/net/skfp/skfddi.c

diff -r 0fe1a980708b include/pcmcia/cs.h

include/pcmcia/cs.h +1 -1

@@ -170,7 +170,7 @@ typedef struct irq_req_t {
     u_int	Attributes;
     u_int	AssignedIRQ;
     u_int	IRQInfo1, IRQInfo2; /* IRQInfo2 is ignored */
-    void	*Handler;
+    int		(*Handler)(int, void *);
     void	*Instance;
 } irq_req_t;
 

diff -r 0fe1a980708b sound/pci/als300.c

sound/pci/als300.c +1 -1

@@ -678,7 +678,7 @@ static int __devinit snd_als300_create(s
 				       struct snd_als300 **rchip)
 {
 	struct snd_als300 *chip;
-	void *irq_handler;
+	int (*irq_handler)(int, void *);
 	int err;
 
 	static struct snd_device_ops ops = {

diff -r 0fe1a980708b drivers/net/e1000e/netdev.c

drivers/net/e1000e/netdev.c +1 -1

@@ -935,7 +935,7 @@ static int e1000_request_irq(struct e100
 static int e1000_request_irq(struct e1000_adapter *adapter)
 {
 	struct net_device *netdev = adapter->netdev;
-	void (*handler) = &e1000_intr;
+	int (*handler)(int, void *) = &e1000_intr;
 	int irq_flags = IRQF_SHARED;
 	int err;
 

diff -r 0fe1a980708b drivers/net/e1000/e1000_main.c

You should be using irq_handler_t for all these.

(Coincedentally, doing so makes it easier for me to later on remove the almost-never-used 'irq' argument from all irq handlers)

Quoted 1 line · On Saturday 19 January 2008 07:41:41 Jeff Garzik wrote:

You should be using irq_handler_t for all these.

Well, these are your drivers, but for mine I dislike the obfuscation.

It's not like you can declare the function itself to be an irq_handler_t, so it's a strange turd to drop in a driver.

FWIW, I have been working in this area extensively.

Check out the 'irq-cleanups' and 'irq-remove' branches of
git://git.kernel.org/pub/scm/linux/kernel/git/jgarzik/misc-2.6.git