[PATCH 1/3] Improve type handling in interrupt handlers

[PATCH 1/3] Improve type handling in interrupt handlers

[Rusty Russell]

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

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Cc: Jeff Garzik <jgarzik@pobox.com>
Cc: Ash Willis <ashwillis@programmer.net>
Cc: linux-pcmcia@lists.infradead.org
---
 drivers/net/e1000/e1000_main.c |    2 +-
 drivers/net/e1000e/netdev.c    |    2 +-
 drivers/net/eth16i.c           |    2 +-
 drivers/net/ewrk3.c            |    2 +-
 drivers/net/skfp/skfddi.c      |    2 +-
 include/pcmcia/cs.h            |    2 +-
 sound/pci/als300.c             |    2 +-
 7 files changed, 7 insertions(+), 7 deletions(-)

diff -r 0fe1a980708b drivers/net/eth16i.c
--- a/drivers/net/eth16i.c	Thu Jan 17 14:48:56 2008 +1100
+++ b/drivers/net/eth16i.c	Thu Jan 17 15:42:00 2008 +1100
@@ -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
--- a/drivers/net/ewrk3.c	Thu Jan 17 14:48:56 2008 +1100
+++ b/drivers/net/ewrk3.c	Thu Jan 17 15:42:00 2008 +1100
@@ -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
--- a/drivers/net/skfp/skfddi.c	Thu Jan 17 14:48:56 2008 +1100
+++ b/drivers/net/skfp/skfddi.c	Thu Jan 17 15:42:00 2008 +1100
@@ -495,7 +495,7 @@ static int skfp_open(struct net_device *
 
 	PRINTK(KERN_INFO "entering skfp_open\n");
 	/* Register IRQ - support shared interrupts by passing device ptr */
-	err = request_irq(dev->irq, (void *) skfp_interrupt, IRQF_SHARED,
+	err = request_irq(dev->irq, skfp_interrupt, IRQF_SHARED,
 			  dev->name, dev);
 	if (err)
 		return err;
diff -r 0fe1a980708b include/pcmcia/cs.h
--- a/include/pcmcia/cs.h	Thu Jan 17 14:48:56 2008 +1100
+++ b/include/pcmcia/cs.h	Thu Jan 17 15:42:01 2008 +1100
@@ -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
--- a/sound/pci/als300.c	Thu Jan 17 14:48:56 2008 +1100
+++ b/sound/pci/als300.c	Thu Jan 17 15:42:01 2008 +1100
@@ -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
--- a/drivers/net/e1000e/netdev.c	Thu Jan 17 14:48:56 2008 +1100
+++ b/drivers/net/e1000e/netdev.c	Thu Jan 17 15:42:00 2008 +1100
@@ -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
--- a/drivers/net/e1000/e1000_main.c	Thu Jan 17 14:48:56 2008 +1100
+++ b/drivers/net/e1000/e1000_main.c	Thu Jan 17 15:42:00 2008 +1100
@@ -299,7 +299,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;
+	irqreturn_t (*handler)(int, void *) = &e1000_intr;
 	int irq_flags = IRQF_SHARED;
 	int err;
 

[PATCH 2/3] Make IRQ handlers typesafe.

[Rusty Russell]

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 <rusty@rustcorp.com.au>
---
 include/linux/interrupt.h |   17 +++++++++++++++--
 include/linux/kernel.h    |    7 +++++++
 kernel/irq/devres.c       |   10 +++++-----
 kernel/irq/manage.c       |    6 +++---
 4 files changed, 30 insertions(+), 10 deletions(-)

diff -r 0fe1a980708b include/linux/interrupt.h
--- a/include/linux/interrupt.h	Thu Jan 17 14:48:56 2008 +1100
+++ b/include/linux/interrupt.h	Thu Jan 17 15:42:01 2008 +1100
@@ -69,13 +69,26 @@ struct irqaction {
 };
 
 extern irqreturn_t no_action(int cpl, void *dev_id);
-extern int __must_check request_irq(unsigned int, irq_handler_t handler,
+
+/* Typesafe version of request_irq.  Also takes old-style void * handlers. */
+#define request_irq(irq, handler, flags, name, dev_id)			\
+	__request_irq((irq),						\
+		      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;
 
-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),				\
+		      check_either_type((handler), irq_handler_t,	\
+					int (*)(int, typeof(dev_id))),	\
+		      (flags), (name), (dev_id))
+extern int __must_check __devm_request_irq(struct device *dev, unsigned int irq,
 			    irq_handler_t handler, unsigned long irqflags,
 			    const char *devname, void *dev_id);
 extern void devm_free_irq(struct device *dev, unsigned int irq, void *dev_id);
diff -r 0fe1a980708b include/linux/kernel.h
--- a/include/linux/kernel.h	Thu Jan 17 14:48:56 2008 +1100
+++ b/include/linux/kernel.h	Thu Jan 17 15:42:01 2008 +1100
@@ -379,6 +379,13 @@ static inline int __attribute__ ((format
 	(void)__tmp; \
 })
 
+/*
+ * Returns var as a type1.  If it's not a type1 or type2 you'll get:
+ * "error: cast to union type from type not present in union"
+ */
+#define check_either_type(var, type1, type2) \
+	(((union { typeof(type1) _t1; typeof(type2) _t2; })var)._t1)
+
 struct sysinfo;
 extern int do_sysinfo(struct sysinfo *info);
 
diff -r 0fe1a980708b kernel/irq/devres.c
--- a/kernel/irq/devres.c	Thu Jan 17 14:48:56 2008 +1100
+++ b/kernel/irq/devres.c	Thu Jan 17 15:42:01 2008 +1100
@@ -41,9 +41,9 @@ static int devm_irq_match(struct device 
  *	If an IRQ allocated with this function needs to be freed
  *	separately, dev_free_irq() must be used.
  */
-int devm_request_irq(struct device *dev, unsigned int irq,
-		     irq_handler_t handler, unsigned long irqflags,
-		     const char *devname, void *dev_id)
+int __devm_request_irq(struct device *dev, unsigned int irq,
+		       irq_handler_t handler, unsigned long irqflags,
+		       const char *devname, void *dev_id)
 {
 	struct irq_devres *dr;
 	int rc;
@@ -53,7 +53,7 @@ int devm_request_irq(struct device *dev,
 	if (!dr)
 		return -ENOMEM;
 
-	rc = request_irq(irq, handler, irqflags, devname, dev_id);
+	rc = __request_irq(irq, handler, irqflags, devname, dev_id);
 	if (rc) {
 		devres_free(dr);
 		return rc;
@@ -65,7 +65,7 @@ int devm_request_irq(struct device *dev,
 
 	return 0;
 }
-EXPORT_SYMBOL(devm_request_irq);
+EXPORT_SYMBOL(__devm_request_irq);
 
 /**
  *	devm_free_irq - free an interrupt
diff -r 0fe1a980708b kernel/irq/manage.c
--- a/kernel/irq/manage.c	Thu Jan 17 14:48:56 2008 +1100
+++ b/kernel/irq/manage.c	Thu Jan 17 15:42:01 2008 +1100
@@ -514,8 +514,8 @@ EXPORT_SYMBOL(free_irq);
  *	IRQF_SAMPLE_RANDOM	The interrupt can be used for entropy
  *
  */
-int request_irq(unsigned int irq, irq_handler_t handler,
-		unsigned long irqflags, const char *devname, void *dev_id)
+int __request_irq(unsigned int irq, irq_handler_t handler,
+		  unsigned long irqflags, const char *devname, void *dev_id)
 {
 	struct irqaction *action;
 	int retval;
@@ -576,4 +576,4 @@ int request_irq(unsigned int irq, irq_ha
 
 	return retval;
 }
-EXPORT_SYMBOL(request_irq);
+EXPORT_SYMBOL(__request_irq);

[PATCH 3/3] Makes lguest's irq handler typesafe

[Rusty Russell]

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
--- a/drivers/lguest/lguest_device.c	Thu Jan 17 16:54:00 2008 +1100
+++ b/drivers/lguest/lguest_device.c	Thu Jan 17 16:59:46 2008 +1100
@@ -179,9 +179,8 @@ static void lg_notify(struct virtqueue *
 	hcall(LHCALL_NOTIFY, lvq->config.pfn << PAGE_SHIFT, 0, 0);
 }
 
-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;
 
 	if (unlikely(desc->config_change)) {

Re: [PATCH 1/3] Improve type handling in interrupt handlers

[Jeff Garzik]

Rusty Russell wrote:
> This improves typechecking of interrupt handlers by removing
> unnecessary (void *) casts and storing handlers in correctly-typed
> variables.
> 
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> Cc: Jeff Garzik <jgarzik@pobox.com>
> Cc: Ash Willis <ashwillis@programmer.net>
> Cc: linux-pcmcia@lists.infradead.org

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

(irq-remove is built on top of irq-cleanups)


> diff -r 0fe1a980708b drivers/net/eth16i.c
> --- a/drivers/net/eth16i.c	Thu Jan 17 14:48:56 2008 +1100
> +++ b/drivers/net/eth16i.c	Thu Jan 17 15:42:00 2008 +1100
> @@ -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
> --- a/drivers/net/ewrk3.c	Thu Jan 17 14:48:56 2008 +1100
> +++ b/drivers/net/ewrk3.c	Thu Jan 17 15:42:00 2008 +1100
> @@ -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
> --- a/drivers/net/skfp/skfddi.c	Thu Jan 17 14:48:56 2008 +1100
> +++ b/drivers/net/skfp/skfddi.c	Thu Jan 17 15:42:00 2008 +1100
> @@ -495,7 +495,7 @@ static int skfp_open(struct net_device *
>  
>  	PRINTK(KERN_INFO "entering skfp_open\n");
>  	/* Register IRQ - support shared interrupts by passing device ptr */
> -	err = request_irq(dev->irq, (void *) skfp_interrupt, IRQF_SHARED,
> +	err = request_irq(dev->irq, skfp_interrupt, IRQF_SHARED,
>  			  dev->name, dev);
>  	if (err)
>  		return err;

ACK all of the above


> diff -r 0fe1a980708b include/pcmcia/cs.h
> --- a/include/pcmcia/cs.h	Thu Jan 17 14:48:56 2008 +1100
> +++ b/include/pcmcia/cs.h	Thu Jan 17 15:42:01 2008 +1100
> @@ -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
> --- a/sound/pci/als300.c	Thu Jan 17 14:48:56 2008 +1100
> +++ b/sound/pci/als300.c	Thu Jan 17 15:42:01 2008 +1100
> @@ -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
> --- a/drivers/net/e1000e/netdev.c	Thu Jan 17 14:48:56 2008 +1100
> +++ b/drivers/net/e1000e/netdev.c	Thu Jan 17 15:42:00 2008 +1100
> @@ -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
> --- a/drivers/net/e1000/e1000_main.c	Thu Jan 17 14:48:56 2008 +1100
> +++ b/drivers/net/e1000/e1000_main.c	Thu Jan 17 15:42:00 2008 +1100
> @@ -299,7 +299,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;
> +	irqreturn_t (*handler)(int, void *) = &e1000_intr;
>  	int irq_flags = IRQF_SHARED;
>  	int err;

NAK the rest.

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)

	Jeff

Re: [PATCH 2/3] Make IRQ handlers typesafe.

[Jeff Garzik]

Rusty Russell wrote:
> 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 <rusty@rustcorp.com.au>
> ---
>  include/linux/interrupt.h |   17 +++++++++++++++--
>  include/linux/kernel.h    |    7 +++++++
>  kernel/irq/devres.c       |   10 +++++-----
>  kernel/irq/manage.c       |    6 +++---
>  4 files changed, 30 insertions(+), 10 deletions(-)
> 
> diff -r 0fe1a980708b include/linux/interrupt.h
> --- a/include/linux/interrupt.h	Thu Jan 17 14:48:56 2008 +1100
> +++ b/include/linux/interrupt.h	Thu Jan 17 15:42:01 2008 +1100
> @@ -69,13 +69,26 @@ struct irqaction {
>  };
>  
>  extern irqreturn_t no_action(int cpl, void *dev_id);
> -extern int __must_check request_irq(unsigned int, irq_handler_t handler,
> +
> +/* Typesafe version of request_irq.  Also takes old-style void * handlers. */
> +#define request_irq(irq, handler, flags, name, dev_id)			\
> +	__request_irq((irq),						\
> +		      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;
>  
> -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),				\
> +		      check_either_type((handler), irq_handler_t,	\
> +					int (*)(int, typeof(dev_id))),	\
> +		      (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);

This is ugly and unnecessary.

Anything that does not use irq_handler_t is broken and should be fixed 
(as noted in last email, I think I've covered all this driver work already)

And if by definition everything uses irq_handler_t, there is no need for 
any of this "check_either_type" silliness.

	Jeff

Re: [PATCH 3/3] Makes lguest's irq handler typesafe

[Jeff Garzik]

Rusty Russell wrote:
> 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
> --- a/drivers/lguest/lguest_device.c	Thu Jan 17 16:54:00 2008 +1100
> +++ b/drivers/lguest/lguest_device.c	Thu Jan 17 16:59:46 2008 +1100
> @@ -179,9 +179,8 @@ static void lg_notify(struct virtqueue *
>  	hcall(LHCALL_NOTIFY, lvq->config.pfn << PAGE_SHIFT, 0, 0);
>  }
>  
> -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.

	Jeff

Re: [PATCH 1/3] Improve type handling in interrupt handlers

[Rusty Russell]

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.

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

Slightly, but the compiler would tell you if you miss one, so I don't think 
this is real.

Cheers,
Rusty.

Re: [PATCH 3/3] Makes lguest's irq handler typesafe

[Rusty Russell]

On Saturday 19 January 2008 07:45:41 Jeff Garzik wrote:
> Rusty Russell wrote:
> > -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.

The compiler checks types, and we should use it (though note that when I 
typesafed the kthread code I didn't find any bugs, so safety arguments must 
be muted).  The gratuitous casts back and forth are annoying and silly.

The compatibility nightmare is one reason for the previous patch (which you  
didn't understand, I hope it's now clear).

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

True, but this is not about performance.  It's about making code simpler and 
compiler-checkable.

Rusty.

Re: [PATCH 1/3] Improve type handling in interrupt handlers

[Jeff Garzik]

Rusty Russell wrote:
> 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.

The others need to be irq_handler_t because that's the precise type 
that's being used in each particularly situation.  Each time the code 
re-creates that definition creates a problem for future irq handler 
changes of any type, really.

As I noted, I've fixed all this crap already, and read through each one 
of those drivers.

	Jeff

Re: [PATCH 3/3] Makes lguest's irq handler typesafe

[Tejun Heo]

Rusty Russell wrote:
> 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
> --- a/drivers/lguest/lguest_device.c	Thu Jan 17 16:54:00 2008 +1100
> +++ b/drivers/lguest/lguest_device.c	Thu Jan 17 16:59:46 2008 +1100
> @@ -179,9 +179,8 @@ static void lg_notify(struct virtqueue *
>  	hcall(LHCALL_NOTIFY, lvq->config.pfn << PAGE_SHIFT, 0, 0);
>  }
>  
> -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;
>  
>  	if (unlikely(desc->config_change)) {

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.

Thanks.

-- 
tejun

Re: [PATCH 3/3] Makes lguest's irq handler typesafe

[Rusty Russell]

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.

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

Cheers,
Rusty.

Re: [PATCH 1/3] Improve type handling in interrupt handlers

[Rusty Russell]

On Saturday 19 January 2008 07:41:41 Jeff Garzik wrote:
> FWIW, I have been working in this area extensively.

Excellent...

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

Your irq-cleanups branch is nice work!  But AFAICT these patches are not 
included in your irq-cleanups branch.  Did you want me to switch my patch 
over to irqreturn_t and send them for you to roll in?

Cheers,
Rusty.

Re: [PATCH 3/3] Makes lguest's irq handler typesafe

[Tejun Heo]

Hello, Rusty.

Rusty Russell wrote:
> 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.

The change is a small one and both the cost and benefit aren't big.

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

I'm in full agreement here but the cost / benefit equation doesn't seem
quite right to me.  If we're gonna convert all callbacks to take native
pointers, I'm fine with the irq handler part too.  If not, it just adds
confusion which is much worse than any benefit it can bring, so I think
the question is "do we want to change all callbacks to take native
pointer type instead of void pointer?".

-- 
tejun

Re: [PATCH 3/3] Makes lguest's irq handler typesafe

[Tejun Heo]

Tejun Heo wrote:
> 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.

Thanks.

-- 
tejun

Re: [PATCH 3/3] Makes lguest's irq handler typesafe

[Rusty Russell]

On Saturday 19 January 2008 12:44:52 Tejun Heo wrote:
> Tejun Heo wrote:
> > 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.

Hi Tejun

    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.

Thanks!
Rusty.

Re: [PATCH 3/3] Makes lguest's irq handler typesafe

[Tejun Heo]

Hello,

Rusty Russell wrote:
>     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).

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

I think you successfully got a very small sample of possible reactions.
 Jeff vetoing it (and for good reasons) and me a bit more positive but
not quite sold.  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?

Thanks.

-- 
tejun

Re: [PATCH 3/3] Makes lguest's irq handler typesafe

[Rusty Russell]

On Saturday 19 January 2008 15:08:28 Tejun Heo wrote:
> Rusty Russell wrote:
> >     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).

Hi Tejun.

    I'm thinking an open-ended transition.  I don't see a big reason to churn 
working code, since we can easily support both.

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

   Insightful comment, you've convinced me!

Thanks,
Rusty.