[PATCH 0/7] ensure arguments to request_irq and free_irq are compatible

[PATCH 0/7] ensure arguments to request_irq and free_irq are compatible

[Julia Lawall]

As far as I can see, free_irq does nothing if its second argument is not
the same as the last argument of the corresponding call to request_irq.
These were found using the semantic match below (http://coccinelle.lip6.fr/).
This semantic match finds a number of other cases, but they are mostly in
platform driver probe functions, so the functions should be just converted
to use devm functions, eliminating the need to call free_irq at all.

// <smpl>
@r exists@
expression e,e1,e2,e3,e4,e5;
type T;
position p1,p2;
@@

request_irq@p1(e1,e2,e3,e4,e5)
...
(
 free_irq(e1,(T)e5);
|
free_irq@p2(e1,e);
)

@bad1 exists@
position r.p1,r.p2;
expression e1,e2,e3,e4,e5;
@@

request_irq@p1(e1,e2,e3,e4,(void *)e5)
...
free_irq@p2(e1,e5);

@bad2 exists@
position r.p1,r.p2;
statement S;
@@

if (request_irq@p1(...)) S else { <+... free_irq@p2(...); ...+> }

@script:python depends on !bad1 && !bad2@
p1 << r.p1;
p2 << r.p2;
@@

cocci.print_main("",p1)
cocci.print_secs("",p2)
// </smpl>

[PATCH 1/7] drivers/video/pvr2fb.c: ensure arguments to request_irq and free_irq are compatible

[Julia Lawall]

From: Julia Lawall <Julia.Lawall@lip6.fr>

Convert calls to free_irq so that the second argument is the same as the
last argument of the corresponding call to request_irq.  Without this
property, free_irq does nothing.

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
Not tested.

 drivers/video/pvr2fb.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/video/pvr2fb.c b/drivers/video/pvr2fb.c
index 3a3fdc6..bcd44c3 100644
--- a/drivers/video/pvr2fb.c
+++ b/drivers/video/pvr2fb.c
@@ -895,7 +895,7 @@ static int __init pvr2fb_dc_init(void)
 
 #ifdef CONFIG_PVR2_DMA
 	if (request_dma(pvr2dma, "pvr2") != 0) {
-		free_irq(HW_EVENT_VSYNC, 0);
+		free_irq(HW_EVENT_VSYNC, fb_info);
 		return -EBUSY;
 	}
 #endif
@@ -914,7 +914,7 @@ static void __exit pvr2fb_dc_exit(void)
 		currentpar->mmio_base = 0;
 	}
 
-	free_irq(HW_EVENT_VSYNC, 0);
+	free_irq(HW_EVENT_VSYNC, fb_info);
 #ifdef CONFIG_PVR2_DMA
 	free_dma(pvr2dma);
 #endif

Re: [PATCH 1/7] drivers/video/pvr2fb.c: ensure arguments to request_irq and free_irq are compatible

[Florian Tobias Schandinat]

On 03/11/2012 07:36 PM, Julia Lawall wrote:
> From: Julia Lawall <Julia.Lawall@lip6.fr>
> 
> Convert calls to free_irq so that the second argument is the same as the
> last argument of the corresponding call to request_irq.  Without this
> property, free_irq does nothing.
> 
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

Applied.


Thanks,

Florian Tobias Schandinat

> 
> ---
> Not tested.
> 
>  drivers/video/pvr2fb.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/video/pvr2fb.c b/drivers/video/pvr2fb.c
> index 3a3fdc6..bcd44c3 100644
> --- a/drivers/video/pvr2fb.c
> +++ b/drivers/video/pvr2fb.c
> @@ -895,7 +895,7 @@ static int __init pvr2fb_dc_init(void)
>  
>  #ifdef CONFIG_PVR2_DMA
>  	if (request_dma(pvr2dma, "pvr2") != 0) {
> -		free_irq(HW_EVENT_VSYNC, 0);
> +		free_irq(HW_EVENT_VSYNC, fb_info);
>  		return -EBUSY;
>  	}
>  #endif
> @@ -914,7 +914,7 @@ static void __exit pvr2fb_dc_exit(void)
>  		currentpar->mmio_base = 0;
>  	}
>  
> -	free_irq(HW_EVENT_VSYNC, 0);
> +	free_irq(HW_EVENT_VSYNC, fb_info);
>  #ifdef CONFIG_PVR2_DMA
>  	free_dma(pvr2dma);
>  #endif
> 
> 

[PATCH 2/7] drivers/scsi/arm/{cumana_2,eesox,powertec}.c: ensure arguments to request_irq and free_irq are compatible

[Julia Lawall]

From: Julia Lawall <Julia.Lawall@lip6.fr>

Convert calls to free_irq so that the second argument is the same as the
last argument of the corresponding call to request_irq.  Without this
property, free_irq does nothing.

In each case, the problem is only in the probe function.  The corresponding
call in the remove function already has the correct second argument.

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
Not tested.

 drivers/scsi/arm/cumana_2.c |    2 +-
 drivers/scsi/arm/eesox.c    |    2 +-
 drivers/scsi/arm/powertec.c |    2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/arm/cumana_2.c b/drivers/scsi/arm/cumana_2.c
index 547987b..693d383 100644
--- a/drivers/scsi/arm/cumana_2.c
+++ b/drivers/scsi/arm/cumana_2.c
@@ -480,7 +480,7 @@ cumanascsi2_probe(struct expansion_card *ec, const struct ecard_id *id)
 
 	if (info->info.scsi.dma != NO_DMA)
 		free_dma(info->info.scsi.dma);
-	free_irq(ec->irq, host);
+	free_irq(ec->irq, info);
 
  out_release:
 	fas216_release(host);
diff --git a/drivers/scsi/arm/eesox.c b/drivers/scsi/arm/eesox.c
index edfd12b..48c1a14 100644
--- a/drivers/scsi/arm/eesox.c
+++ b/drivers/scsi/arm/eesox.c
@@ -601,7 +601,7 @@ eesoxscsi_probe(struct expansion_card *ec, const struct ecard_id *id)
 
 	if (info->info.scsi.dma != NO_DMA)
 		free_dma(info->info.scsi.dma);
-	free_irq(ec->irq, host);
+	free_irq(ec->irq, info);
 
  out_remove:
 	fas216_remove(host);
diff --git a/drivers/scsi/arm/powertec.c b/drivers/scsi/arm/powertec.c
index 9274c06..c9d6e8b 100644
--- a/drivers/scsi/arm/powertec.c
+++ b/drivers/scsi/arm/powertec.c
@@ -393,7 +393,7 @@ powertecscsi_probe(struct expansion_card *ec, const struct ecard_id *id)
 
 	if (info->info.scsi.dma != NO_DMA)
 		free_dma(info->info.scsi.dma);
-	free_irq(ec->irq, host);
+	free_irq(ec->irq, info);
 
  out_release:
 	fas216_release(host);

[PATCH 3/7] drivers/net/irda/{ali-ircc,via-ircc,w83977af-ir}.c: ensure arguments to request_irq and free_irq are compatible

[Julia Lawall]

From: Julia Lawall <Julia.Lawall@lip6.fr>

Convert calls to free_irq so that the second argument is the same as the
last argument of the corresponding call to request_irq.  Without this
property, free_irq does nothing.

In each case, there is one call to free_irq with self as the second
argument and one with dev as the second argument.  Even though dev was also
the last argument of request_irq, I have changed all of the calls to use
self, as it fits better with the first argument to each function.

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
Not tested.  I don't know if I have made the right choice.

 drivers/net/irda/ali-ircc.c    |    4 ++--
 drivers/net/irda/via-ircc.c    |    4 ++--
 drivers/net/irda/w83977af_ir.c |    4 ++--
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/net/irda/ali-ircc.c b/drivers/net/irda/ali-ircc.c
index 963067d..7e732d7 100644
--- a/drivers/net/irda/ali-ircc.c
+++ b/drivers/net/irda/ali-ircc.c
@@ -1352,7 +1352,7 @@ static int ali_ircc_net_open(struct net_device *dev)
 	iobase = self->io.fir_base;
 	
 	/* Request IRQ and install Interrupt Handler */
-	if (request_irq(self->io.irq, ali_ircc_interrupt, 0, dev->name, dev)) 
+	if (request_irq(self->io.irq, ali_ircc_interrupt, 0, dev->name, self))
 	{
 		IRDA_WARNING("%s, unable to allocate irq=%d\n",
 			     ALI_IRCC_DRIVER_NAME,
@@ -1424,7 +1424,7 @@ static int ali_ircc_net_close(struct net_device *dev)
 	/* Disable interrupts */
 	SetCOMInterrupts(self, FALSE);
 	       
-	free_irq(self->io.irq, dev);
+	free_irq(self->io.irq, self);
 	free_dma(self->io.dma);
 
 	IRDA_DEBUG(2, "%s(), ----------------- End ------------------\n", __func__ );
diff --git a/drivers/net/irda/via-ircc.c b/drivers/net/irda/via-ircc.c
index 2d456dd..c0678be 100644
--- a/drivers/net/irda/via-ircc.c
+++ b/drivers/net/irda/via-ircc.c
@@ -1483,7 +1483,7 @@ static int via_ircc_net_open(struct net_device *dev)
 	dev->stats.rx_packets = 0;
 	IRDA_ASSERT(self != NULL, return 0;);
 	iobase = self->io.fir_base;
-	if (request_irq(self->io.irq, via_ircc_interrupt, 0, dev->name, dev)) {
+	if (request_irq(self->io.irq, via_ircc_interrupt, 0, dev->name, self)) {
 		IRDA_WARNING("%s, unable to allocate irq=%d\n", driver_name,
 			     self->io.irq);
 		return -EAGAIN;
@@ -1562,7 +1562,7 @@ static int via_ircc_net_close(struct net_device *dev)
 
 	/* Disable interrupts */
 	EnAllInt(iobase, OFF);
-	free_irq(self->io.irq, dev);
+	free_irq(self->io.irq, self);
 	free_dma(self->io.dma);
 	if (self->io.dma2 != self->io.dma)
 		free_dma(self->io.dma2);
diff --git a/drivers/net/irda/w83977af_ir.c b/drivers/net/irda/w83977af_ir.c
index 7d43506..c0c430c 100644
--- a/drivers/net/irda/w83977af_ir.c
+++ b/drivers/net/irda/w83977af_ir.c
@@ -1164,7 +1164,7 @@ static int w83977af_net_open(struct net_device *dev)
 	iobase = self->io.fir_base;
 
 	if (request_irq(self->io.irq, w83977af_interrupt, 0, dev->name, 
-			(void *) dev)) {
+			self)) {
 		return -EAGAIN;
 	}
 	/*
@@ -1244,7 +1244,7 @@ static int w83977af_net_close(struct net_device *dev)
 	switch_bank(iobase, SET0);
 	outb(0, iobase+ICR); 
 
-	free_irq(self->io.irq, dev);
+	free_irq(self->io.irq, self);
 	free_dma(self->io.dma);
 
 	/* Restore bank register */

[PATCH 4/7] arch/sh/drivers/dma/{dma-g2,dmabrg}.c: ensure arguments to request_irq and free_irq are compatible

[Julia Lawall]

From: Julia Lawall <Julia.Lawall@lip6.fr>

Convert calls to free_irq so that the second argument is the same as the
last argument of the corresponding call to request_irq.  Without this
property, free_irq does nothing.

In the case of dmabrg.c the change is merely cosmetic - changing 0 to NULL.

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
 arch/sh/drivers/dma/dma-g2.c |    4 ++--
 arch/sh/drivers/dma/dmabrg.c |    4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/sh/drivers/dma/dma-g2.c b/arch/sh/drivers/dma/dma-g2.c
index be9ca7c..e1ab6eb 100644
--- a/arch/sh/drivers/dma/dma-g2.c
+++ b/arch/sh/drivers/dma/dma-g2.c
@@ -181,14 +181,14 @@ static int __init g2_dma_init(void)
 
 	ret = register_dmac(&g2_dma_info);
 	if (unlikely(ret != 0))
-		free_irq(HW_EVENT_G2_DMA, 0);
+		free_irq(HW_EVENT_G2_DMA, &g2_dma_info);
 
 	return ret;
 }
 
 static void __exit g2_dma_exit(void)
 {
-	free_irq(HW_EVENT_G2_DMA, 0);
+	free_irq(HW_EVENT_G2_DMA, &g2_dma_info);
 	unregister_dmac(&g2_dma_info);
 }
 
diff --git a/arch/sh/drivers/dma/dmabrg.c b/arch/sh/drivers/dma/dmabrg.c
index 3d66a32..c0dd904 100644
--- a/arch/sh/drivers/dma/dmabrg.c
+++ b/arch/sh/drivers/dma/dmabrg.c
@@ -189,8 +189,8 @@ static int __init dmabrg_init(void)
 	if (ret == 0)
 		return ret;
 
-	free_irq(DMABRGI1, 0);
-out1:	free_irq(DMABRGI0, 0);
+	free_irq(DMABRGI1, NULL);
+out1:	free_irq(DMABRGI0, NULL);
 out0:	kfree(dmabrg_handlers);
 	return ret;
 }

Re: [PATCH 4/7] arch/sh/drivers/dma/{dma-g2,dmabrg}.c: ensure arguments to request_irq and free_irq are compatible

[Paul Mundt]

On Sun, Mar 11, 2012 at 08:36:24PM +0100, Julia Lawall wrote:
> From: Julia Lawall <Julia.Lawall@lip6.fr>
> 
> Convert calls to free_irq so that the second argument is the same as the
> last argument of the corresponding call to request_irq.  Without this
> property, free_irq does nothing.
> 
> In the case of dmabrg.c the change is merely cosmetic - changing 0 to NULL.
> 
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
> 
Applied, thanks.

[PATCH 5/7] drivers/atm/eni.c: ensure arguments to request_irq and free_irq are compatible

[Julia Lawall]

From: Julia Lawall <Julia.Lawall@lip6.fr>

Convert call to request_irq so that the last argument is the same as the
second argument of the subsequent call to free_irq.  Without this
property, free_irq does nothing.

I have chosen to keep the call to free_irq as is, because its second
argument is more like its first argument.

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
Not tested.  I don't know if I have made the right choice.

 drivers/atm/eni.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/atm/eni.c b/drivers/atm/eni.c
index 956e9ac..89e7223 100644
--- a/drivers/atm/eni.c
+++ b/drivers/atm/eni.c
@@ -1800,7 +1800,8 @@ static int __devinit eni_start(struct atm_dev *dev)
 
 	DPRINTK(">eni_start\n");
 	eni_dev = ENI_DEV(dev);
-	if (request_irq(eni_dev->irq,&eni_int,IRQF_SHARED,DEV_LABEL,dev)) {
+	if (request_irq(eni_dev->irq, &eni_int, IRQF_SHARED, DEV_LABEL,
+			eni_dev)) {
 		printk(KERN_ERR DEV_LABEL "(itf %d): IRQ%d is already in use\n",
 		    dev->number,eni_dev->irq);
 		error = -EAGAIN;

Re: [PATCH 5/7] drivers/atm/eni.c: ensure arguments to request_irq and free_irq are compatible

[Chas Williams (CONTRACTOR)]

i am pretty sure that request_irq() shouldnt be changed but rather
free_irq() changed.  the last argument of request_irq() is passed to the
interrupt routine (ent_int in this case).  if you change that you need
to update the interrupt routine.  fixing free_irq would be easier.

In message <1331494587-12196-6-git-send-email-Julia.Lawall@lip6.fr>,Julia Lawall writes:
>Convert call to request_irq so that the last argument is the same as the
>second argument of the subsequent call to free_irq.  Without this
>property, free_irq does nothing.
>
>I have chosen to keep the call to free_irq as is, because its second
>argument is more like its first argument.

Re: [PATCH 5/7] drivers/atm/eni.c: ensure arguments to request_irq and free_irq are compatible

[Julia Lawall]

On Sun, 11 Mar 2012, Chas Williams (CONTRACTOR) wrote:

> i am pretty sure that request_irq() shouldnt be changed but rather
> free_irq() changed.  the last argument of request_irq() is passed to the
> interrupt routine (ent_int in this case).  if you change that you need
> to update the interrupt routine.  fixing free_irq would be easier.

OK, thanks for the feedback.  I will fix it up.

julia

>
> In message <1331494587-12196-6-git-send-email-Julia.Lawall@lip6.fr>,Julia Lawall writes:
>> Convert call to request_irq so that the last argument is the same as the
>> second argument of the subsequent call to free_irq.  Without this
>> property, free_irq does nothing.
>>
>> I have chosen to keep the call to free_irq as is, because its second
>> argument is more like its first argument.
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 5/7] drivers/atm/eni.c: ensure arguments to request_irq and free_irq are compatible

[Julia Lawall]

From: Julia Lawall <Julia.Lawall@lip6.fr>

Convert calls to free_irq so that the second argument is the same as the
last argument of the corresponding call to request_irq.  Without this
property, free_irq does nothing.

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
Version 2: change free_irq, not request_irq.

diff --git a/drivers/atm/eni.c b/drivers/atm/eni.c
index 956e9ac..485a11a 100644
--- a/drivers/atm/eni.c
+++ b/drivers/atm/eni.c
@@ -1873,7 +1873,7 @@ free_list:
  	kfree(eni_dev->free_list);

  free_irq:
-	free_irq(eni_dev->irq, eni_dev);
+	free_irq(eni_dev->irq, dev);

  out:
  	return error;

Re: [PATCH 5/7] drivers/atm/eni.c: ensure arguments to request_irq and free_irq are compatible

[David Miller]

From: Julia Lawall <julia.lawall@lip6.fr>
Date: Sun, 11 Mar 2012 22:16:25 +0100 (CET)

> From: Julia Lawall <Julia.Lawall@lip6.fr>
> 
> Convert calls to free_irq so that the second argument is the same as
> the
> last argument of the corresponding call to request_irq.  Without this
> property, free_irq does nothing.
> 
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

Applied, but this driver also has a larger problem, it doesn't
free the IRQ (nor do any other kind of chip shutdown and cleanups)
when the driver is unloaded.  The unload function is just one empty
function with a comment.

[PATCH 6/7] drivers/input/touchscreen/hp680_ts_input.c: ensure arguments to request_irq and free_irq are compatible

[Julia Lawall]

From: Julia Lawall <Julia.Lawall@lip6.fr>

Change 0 to NULL in the last argument of request_irq, since the argument
should have pointer type and so that the last argument of request_irq
syntactically matches the second argument of the later call to free_irq.

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
 drivers/input/touchscreen/hp680_ts_input.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/input/touchscreen/hp680_ts_input.c b/drivers/input/touchscreen/hp680_ts_input.c
index 639a604..85cf9be 100644
--- a/drivers/input/touchscreen/hp680_ts_input.c
+++ b/drivers/input/touchscreen/hp680_ts_input.c
@@ -93,7 +93,7 @@ static int __init hp680_ts_init(void)
 	hp680_ts_dev->phys = "hp680_ts/input0";
 
 	if (request_irq(HP680_TS_IRQ, hp680_ts_interrupt,
-			0, MODNAME, 0) < 0) {
+			0, MODNAME, NULL) < 0) {
 		printk(KERN_ERR "hp680_touchscreen.c: Can't allocate irq %d\n",
 		       HP680_TS_IRQ);
 		err = -EBUSY;

Re: [PATCH 6/7] drivers/input/touchscreen/hp680_ts_input.c: ensure arguments to request_irq and free_irq are compatible

[Dmitry Torokhov]

On Sun, Mar 11, 2012 at 08:36:26PM +0100, Julia Lawall wrote:
> From: Julia Lawall <Julia.Lawall@lip6.fr>
> 
> Change 0 to NULL in the last argument of request_irq, since the argument
> should have pointer type and so that the last argument of request_irq
> syntactically matches the second argument of the later call to free_irq.
> 
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

Applied, thanks Julia.

-- 
Dmitry

[PATCH 7/7] arch/unicore32/kernel/dma.c: ensure arguments to request_irq and free_irq are compatible

[Julia Lawall]

From: Julia Lawall <Julia.Lawall@lip6.fr>

Convert calls to free_irq so that the second argument is the same as the
last argument of the corresponding call to request_irq, rather than the
second to last.  Without this property, free_irq does nothing.

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
 arch/unicore32/kernel/dma.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/unicore32/kernel/dma.c b/arch/unicore32/kernel/dma.c
index ae441bc..c813fec 100644
--- a/arch/unicore32/kernel/dma.c
+++ b/arch/unicore32/kernel/dma.c
@@ -173,7 +173,7 @@ int __init puv3_init_dma(void)
 	ret = request_irq(IRQ_DMAERR, dma_err_handler, 0, "DMAERR", NULL);
 	if (ret) {
 		printk(KERN_CRIT "Can't register IRQ for DMAERR\n");
-		free_irq(IRQ_DMA, "DMA");
+		free_irq(IRQ_DMA, NULL);
 		return ret;
 	}
 

Re: [PATCH 7/7] arch/unicore32/kernel/dma.c: ensure arguments to request_irq and free_irq are compatible

[Guan Xuetao]

On Sun, 2012-03-11 at 20:36 +0100, Julia Lawall wrote:
> From: Julia Lawall <Julia.Lawall@lip6.fr>
> 
> Convert calls to free_irq so that the second argument is the same as the
> last argument of the corresponding call to request_irq, rather than the
> second to last.  Without this property, free_irq does nothing.
> 
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
> 
> ---
>  arch/unicore32/kernel/dma.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/unicore32/kernel/dma.c b/arch/unicore32/kernel/dma.c
> index ae441bc..c813fec 100644
> --- a/arch/unicore32/kernel/dma.c
> +++ b/arch/unicore32/kernel/dma.c
> @@ -173,7 +173,7 @@ int __init puv3_init_dma(void)
>  	ret = request_irq(IRQ_DMAERR, dma_err_handler, 0, "DMAERR", NULL);
>  	if (ret) {
>  		printk(KERN_CRIT "Can't register IRQ for DMAERR\n");
> -		free_irq(IRQ_DMA, "DMA");
> +		free_irq(IRQ_DMA, NULL);
>  		return ret;
>  	}
>  
Yeah, it's an obvious mistake. Thanks.
Because the dma device is just located inside PKUnity-3 SoC, and
request_irq() should always return 0, I prefer to remove this free_irq()
line.

Thanks & Regards,
Guan Xuetao

Re: [PATCH 7/7] arch/unicore32/kernel/dma.c: ensure arguments to request_irq and free_irq are compatible

[Julia Lawall]

On Mon, 12 Mar 2012, Guan Xuetao wrote:

> On Sun, 2012-03-11 at 20:36 +0100, Julia Lawall wrote:
>> From: Julia Lawall <Julia.Lawall@lip6.fr>
>>
>> Convert calls to free_irq so that the second argument is the same as the
>> last argument of the corresponding call to request_irq, rather than the
>> second to last.  Without this property, free_irq does nothing.
>>
>> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
>>
>> ---
>>  arch/unicore32/kernel/dma.c |    2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/unicore32/kernel/dma.c b/arch/unicore32/kernel/dma.c
>> index ae441bc..c813fec 100644
>> --- a/arch/unicore32/kernel/dma.c
>> +++ b/arch/unicore32/kernel/dma.c
>> @@ -173,7 +173,7 @@ int __init puv3_init_dma(void)
>>  	ret = request_irq(IRQ_DMAERR, dma_err_handler, 0, "DMAERR", NULL);
>>  	if (ret) {
>>  		printk(KERN_CRIT "Can't register IRQ for DMAERR\n");
>> -		free_irq(IRQ_DMA, "DMA");
>> +		free_irq(IRQ_DMA, NULL);
>>  		return ret;
>>  	}
>>
> Yeah, it's an obvious mistake. Thanks.
> Because the dma device is just located inside PKUnity-3 SoC, and
> request_irq() should always return 0, I prefer to remove this free_irq()
> line.

Remove the whole if test I guess.  Is there a nce way to indicate that the 
return value is not needed (eg for the benefit of future bug finding 
rules)?

julia

Re: [PATCH 7/7] arch/unicore32/kernel/dma.c: ensure arguments to request_irq and free_irq are compatible

[Guan Xuetao]

On Mon, 2012-03-12 at 06:27 +0100, Julia Lawall wrote:
> On Mon, 12 Mar 2012, Guan Xuetao wrote:
> 
> > On Sun, 2012-03-11 at 20:36 +0100, Julia Lawall wrote:
> >> From: Julia Lawall <Julia.Lawall@lip6.fr>
> >>
> >> Convert calls to free_irq so that the second argument is the same as the
> >> last argument of the corresponding call to request_irq, rather than the
> >> second to last.  Without this property, free_irq does nothing.
> >>
> >> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
> >>
> >> ---
> >>  arch/unicore32/kernel/dma.c |    2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/arch/unicore32/kernel/dma.c b/arch/unicore32/kernel/dma.c
> >> index ae441bc..c813fec 100644
> >> --- a/arch/unicore32/kernel/dma.c
> >> +++ b/arch/unicore32/kernel/dma.c
> >> @@ -173,7 +173,7 @@ int __init puv3_init_dma(void)
> >>  	ret = request_irq(IRQ_DMAERR, dma_err_handler, 0, "DMAERR", NULL);
> >>  	if (ret) {
> >>  		printk(KERN_CRIT "Can't register IRQ for DMAERR\n");
> >> -		free_irq(IRQ_DMA, "DMA");
> >> +		free_irq(IRQ_DMA, NULL);
> >>  		return ret;
> >>  	}
> >>
> > Yeah, it's an obvious mistake. Thanks.
> > Because the dma device is just located inside PKUnity-3 SoC, and
> > request_irq() should always return 0, I prefer to remove this free_irq()
> > line.
> 
> Remove the whole if test I guess.  Is there a nce way to indicate that the 
> return value is not needed (eg for the benefit of future bug finding 
> rules)?
> 
> julia
In this case, removing the line containing free_irq() is well enough,
because IRQ_DMA can work even when IRQ_DMAERR doesn't work. And we need
printk and error return value to get potential logical bug information.

Thanks & Regards,

Guan Xuetao

Re: [PATCH 7/7] arch/unicore32/kernel/dma.c: ensure arguments to request_irq and free_irq are compatible

[Julia Lawall]

On Tue, 13 Mar 2012, Guan Xuetao wrote:

> On Mon, 2012-03-12 at 06:27 +0100, Julia Lawall wrote:
>> On Mon, 12 Mar 2012, Guan Xuetao wrote:
>>
>>> On Sun, 2012-03-11 at 20:36 +0100, Julia Lawall wrote:
>>>> From: Julia Lawall <Julia.Lawall@lip6.fr>
>>>>
>>>> Convert calls to free_irq so that the second argument is the same as the
>>>> last argument of the corresponding call to request_irq, rather than the
>>>> second to last.  Without this property, free_irq does nothing.
>>>>
>>>> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
>>>>
>>>> ---
>>>>  arch/unicore32/kernel/dma.c |    2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/arch/unicore32/kernel/dma.c b/arch/unicore32/kernel/dma.c
>>>> index ae441bc..c813fec 100644
>>>> --- a/arch/unicore32/kernel/dma.c
>>>> +++ b/arch/unicore32/kernel/dma.c
>>>> @@ -173,7 +173,7 @@ int __init puv3_init_dma(void)
>>>>  	ret = request_irq(IRQ_DMAERR, dma_err_handler, 0, "DMAERR", NULL);
>>>>  	if (ret) {
>>>>  		printk(KERN_CRIT "Can't register IRQ for DMAERR\n");
>>>> -		free_irq(IRQ_DMA, "DMA");
>>>> +		free_irq(IRQ_DMA, NULL);
>>>>  		return ret;
>>>>  	}
>>>>
>>> Yeah, it's an obvious mistake. Thanks.
>>> Because the dma device is just located inside PKUnity-3 SoC, and
>>> request_irq() should always return 0, I prefer to remove this free_irq()
>>> line.
>>
>> Remove the whole if test I guess.  Is there a nce way to indicate that the
>> return value is not needed (eg for the benefit of future bug finding
>> rules)?
>>
>> julia
> In this case, removing the line containing free_irq() is well enough,
> because IRQ_DMA can work even when IRQ_DMAERR doesn't work. And we need
> printk and error return value to get potential logical bug information.

I'm not completely sure to understand.  The point is that the first 
request_irq can never fail, so we don't need to clean up when the second 
one fails?  Because the lack of cleaning up will not cause the first one 
to fail the next time?  free_irq removes an action from a list and does a 
module_put.  Are these operations both not needed?

thanks,julia

Re: [PATCH 7/7] arch/unicore32/kernel/dma.c: ensure arguments to request_irq and free_irq are compatible

[Guan Xuetao]

On Tue, 2012-03-13 at 09:23 +0100, Julia Lawall wrote:
> On Tue, 13 Mar 2012, Guan Xuetao wrote:
> 
> > On Mon, 2012-03-12 at 06:27 +0100, Julia Lawall wrote:
> >> On Mon, 12 Mar 2012, Guan Xuetao wrote:
> >>
> >>> On Sun, 2012-03-11 at 20:36 +0100, Julia Lawall wrote:
> >>>> From: Julia Lawall <Julia.Lawall@lip6.fr>
> >>>>
> >>>> Convert calls to free_irq so that the second argument is the same as the
> >>>> last argument of the corresponding call to request_irq, rather than the
> >>>> second to last.  Without this property, free_irq does nothing.
> >>>>
> >>>> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
> >>>>
> >>>> ---
> >>>>  arch/unicore32/kernel/dma.c |    2 +-
> >>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/arch/unicore32/kernel/dma.c b/arch/unicore32/kernel/dma.c
> >>>> index ae441bc..c813fec 100644
> >>>> --- a/arch/unicore32/kernel/dma.c
> >>>> +++ b/arch/unicore32/kernel/dma.c
> >>>> @@ -173,7 +173,7 @@ int __init puv3_init_dma(void)
> >>>>  	ret = request_irq(IRQ_DMAERR, dma_err_handler, 0, "DMAERR", NULL);
> >>>>  	if (ret) {
> >>>>  		printk(KERN_CRIT "Can't register IRQ for DMAERR\n");
> >>>> -		free_irq(IRQ_DMA, "DMA");
> >>>> +		free_irq(IRQ_DMA, NULL);
> >>>>  		return ret;
> >>>>  	}
> >>>>
> >>> Yeah, it's an obvious mistake. Thanks.
> >>> Because the dma device is just located inside PKUnity-3 SoC, and
> >>> request_irq() should always return 0, I prefer to remove this free_irq()
> >>> line.
> >>
> >> Remove the whole if test I guess.  Is there a nce way to indicate that the
> >> return value is not needed (eg for the benefit of future bug finding
> >> rules)?
> >>
> >> julia
> > In this case, removing the line containing free_irq() is well enough,
> > because IRQ_DMA can work even when IRQ_DMAERR doesn't work. And we need
> > printk and error return value to get potential logical bug information.
> 
> I'm not completely sure to understand.  The point is that the first 
> request_irq can never fail, so we don't need to clean up when the second 
> one fails?  Because the lack of cleaning up will not cause the first one 
> to fail the next time?  free_irq removes an action from a list and does a 
> module_put.  Are these operations both not needed?
> 
> thanks,julia
puv3_init_dma() is called ONCE when initializing.
In logical, if request_irq(IRQ_DMAERR, *) failed, free_irq(IRQ_DMA, *)
is unnecessary, and dma device/driver can keep on working.
The patch could be:
  	ret = request_irq(IRQ_DMAERR, dma_err_handler, 0, "DMAERR", NULL);
  	if (ret) {
  		printk(KERN_CRIT "Can't register IRQ for DMAERR\n");
 -		free_irq(IRQ_DMA, "DMA");
  		return ret;
  	}

Thanks and Regards,

Guan Xuetao

Re: [PATCH 7/7] arch/unicore32/kernel/dma.c: ensure arguments to request_irq and free_irq are compatible

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 557 bytes --]

On Wed, Mar 14, 2012 at 04:07:24PM +0800, Guan Xuetao wrote:
> puv3_init_dma() is called ONCE when initializing.
> In logical, if request_irq(IRQ_DMAERR, *) failed, free_irq(IRQ_DMA, *)
> is unnecessary, and dma device/driver can keep on working.
> The patch could be:
>   	ret = request_irq(IRQ_DMAERR, dma_err_handler, 0, "DMAERR", NULL);
>   	if (ret) {
>   		printk(KERN_CRIT "Can't register IRQ for DMAERR\n");
>  -		free_irq(IRQ_DMA, "DMA");
>   		return ret;
>   	}

It seems like you should remove the error return as well?

regards,
dan carpenter


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 7/7] arch/unicore32/kernel/dma.c: ensure arguments to request_irq and free_irq are compatible

[Guan Xuetao]

On Wed, 2012-03-14 at 11:19 +0300, Dan Carpenter wrote:
> On Wed, Mar 14, 2012 at 04:07:24PM +0800, Guan Xuetao wrote:
> > puv3_init_dma() is called ONCE when initializing.
> > In logical, if request_irq(IRQ_DMAERR, *) failed, free_irq(IRQ_DMA, *)
> > is unnecessary, and dma device/driver can keep on working.
> > The patch could be:
> >   	ret = request_irq(IRQ_DMAERR, dma_err_handler, 0, "DMAERR", NULL);
> >   	if (ret) {
> >   		printk(KERN_CRIT "Can't register IRQ for DMAERR\n");
> >  -		free_irq(IRQ_DMA, "DMA");
> >   		return ret;
> >   	}
> 
> It seems like you should remove the error return as well?
> 
> regards,
> dan carpenter
> 
The error return value will only generate an extra warning message, and
have no side-effect.

Thanks and Regards,
Guan Xuetao

Re: [PATCH 7/7] arch/unicore32/kernel/dma.c: ensure arguments to request_irq and free_irq are compatible

[Julia Lawall]

On Wed, 14 Mar 2012, Guan Xuetao wrote:

> On Wed, 2012-03-14 at 11:19 +0300, Dan Carpenter wrote:
>> On Wed, Mar 14, 2012 at 04:07:24PM +0800, Guan Xuetao wrote:
>>> puv3_init_dma() is called ONCE when initializing.
>>> In logical, if request_irq(IRQ_DMAERR, *) failed, free_irq(IRQ_DMA, *)
>>> is unnecessary, and dma device/driver can keep on working.
>>> The patch could be:
>>>   	ret = request_irq(IRQ_DMAERR, dma_err_handler, 0, "DMAERR", NULL);
>>>   	if (ret) {
>>>   		printk(KERN_CRIT "Can't register IRQ for DMAERR\n");
>>>  -		free_irq(IRQ_DMA, "DMA");
>>>   		return ret;
>>>   	}
>>
>> It seems like you should remove the error return as well?
>>
>> regards,
>> dan carpenter
>>
> The error return value will only generate an extra warning message, and
> have no side-effect.

The whole thing seems a little strange.  I guess your point is that the 
call site never looks at the return value?  Wouldn't it be better to make 
there be no return value in that case?  If there is a return value, some 
calling context in the future might take that into account and then the 
lack of a free_irq would be a memory leak.  Also if the first request_irq 
can never fail, perhaps that should be made explicit by not testing the 
return value?

julia

Re: [PATCH 7/7] arch/unicore32/kernel/dma.c: ensure arguments to request_irq and free_irq are compatible

[Guan Xuetao]

On Wed, 2012-03-14 at 10:23 +0100, Julia Lawall wrote:
> 
> On Wed, 14 Mar 2012, Guan Xuetao wrote:
> 
> > On Wed, 2012-03-14 at 11:19 +0300, Dan Carpenter wrote:
> >> On Wed, Mar 14, 2012 at 04:07:24PM +0800, Guan Xuetao wrote:
> >>> puv3_init_dma() is called ONCE when initializing.
> >>> In logical, if request_irq(IRQ_DMAERR, *) failed, free_irq(IRQ_DMA, *)
> >>> is unnecessary, and dma device/driver can keep on working.
> >>> The patch could be:
> >>>   	ret = request_irq(IRQ_DMAERR, dma_err_handler, 0, "DMAERR", NULL);
> >>>   	if (ret) {
> >>>   		printk(KERN_CRIT "Can't register IRQ for DMAERR\n");
> >>>  -		free_irq(IRQ_DMA, "DMA");
> >>>   		return ret;
> >>>   	}
> >>
> >> It seems like you should remove the error return as well?
> >>
> >> regards,
> >> dan carpenter
> >>
> > The error return value will only generate an extra warning message, and
> > have no side-effect.
> 
> The whole thing seems a little strange.  I guess your point is that the 
> call site never looks at the return value?  Wouldn't it be better to make 
> there be no return value in that case?  If there is a return value, some 
> calling context in the future might take that into account and then the 
> lack of a free_irq would be a memory leak.  Also if the first request_irq 
> can never fail, perhaps that should be made explicit by not testing the 
> return value?
> 
> julia
This function is an init_call, not a probe function, and it is only
called ONCE.
The dma device here has two interrupts, one IRQ_DMA, another IRQ_DMAERR.
And the device could work without IRQ_DMAERR.
The return value should indicate whether there is something wrong during
initialization, so the function needs return errno when any request_irq
is failed.
For the first request_irq, some code has prepared its resources before
this call, so I suppose it successful. However, the return value must be
tested.

Regards,
Guan Xuetao

Re: [PATCH 7/7] arch/unicore32/kernel/dma.c: ensure arguments to request_irq and free_irq are compatible

[Julia Lawall]

On Thu, 15 Mar 2012, Guan Xuetao wrote:

> On Wed, 2012-03-14 at 10:23 +0100, Julia Lawall wrote:
>>
>> On Wed, 14 Mar 2012, Guan Xuetao wrote:
>>
>>> On Wed, 2012-03-14 at 11:19 +0300, Dan Carpenter wrote:
>>>> On Wed, Mar 14, 2012 at 04:07:24PM +0800, Guan Xuetao wrote:
>>>>> puv3_init_dma() is called ONCE when initializing.
>>>>> In logical, if request_irq(IRQ_DMAERR, *) failed, free_irq(IRQ_DMA, *)
>>>>> is unnecessary, and dma device/driver can keep on working.
>>>>> The patch could be:
>>>>>   	ret = request_irq(IRQ_DMAERR, dma_err_handler, 0, "DMAERR", NULL);
>>>>>   	if (ret) {
>>>>>   		printk(KERN_CRIT "Can't register IRQ for DMAERR\n");
>>>>>  -		free_irq(IRQ_DMA, "DMA");
>>>>>   		return ret;
>>>>>   	}
>>>>
>>>> It seems like you should remove the error return as well?
>>>>
>>>> regards,
>>>> dan carpenter
>>>>
>>> The error return value will only generate an extra warning message, and
>>> have no side-effect.
>>
>> The whole thing seems a little strange.  I guess your point is that the
>> call site never looks at the return value?  Wouldn't it be better to make
>> there be no return value in that case?  If there is a return value, some
>> calling context in the future might take that into account and then the
>> lack of a free_irq would be a memory leak.  Also if the first request_irq
>> can never fail, perhaps that should be made explicit by not testing the
>> return value?
>>
>> julia
> This function is an init_call, not a probe function, and it is only
> called ONCE.
> The dma device here has two interrupts, one IRQ_DMA, another IRQ_DMAERR.
> And the device could work without IRQ_DMAERR.
> The return value should indicate whether there is something wrong during
> initialization, so the function needs return errno when any request_irq
> is failed.
> For the first request_irq, some code has prepared its resources before
> this call, so I suppose it successful. However, the return value must be
> tested.

OK, thank you for the explanation. I will change the patch.

julia

Re: [PATCH 7/7] arch/unicore32/kernel/dma.c: ensure arguments to request_irq and free_irq are compatible

[walter harms]

Am 15.03.2012 07:10, schrieb Julia Lawall:
> 
> 
> On Thu, 15 Mar 2012, Guan Xuetao wrote:
> 
>> On Wed, 2012-03-14 at 10:23 +0100, Julia Lawall wrote:
>>>
>>> On Wed, 14 Mar 2012, Guan Xuetao wrote:
>>>
>>>> On Wed, 2012-03-14 at 11:19 +0300, Dan Carpenter wrote:
>>>>> On Wed, Mar 14, 2012 at 04:07:24PM +0800, Guan Xuetao wrote:
>>>>>> puv3_init_dma() is called ONCE when initializing.
>>>>>> In logical, if request_irq(IRQ_DMAERR, *) failed,
>>>>>> free_irq(IRQ_DMA, *)
>>>>>> is unnecessary, and dma device/driver can keep on working.
>>>>>> The patch could be:
>>>>>>       ret = request_irq(IRQ_DMAERR, dma_err_handler, 0, "DMAERR",
>>>>>> NULL);
>>>>>>       if (ret) {
>>>>>>           printk(KERN_CRIT "Can't register IRQ for DMAERR\n");
>>>>>>  -        free_irq(IRQ_DMA, "DMA");
>>>>>>           return ret;
>>>>>>       }
>>>>>
>>>>> It seems like you should remove the error return as well?
>>>>>
>>>>> regards,
>>>>> dan carpenter
>>>>>
>>>> The error return value will only generate an extra warning message, and
>>>> have no side-effect.
>>>
>>> The whole thing seems a little strange.  I guess your point is that the
>>> call site never looks at the return value?  Wouldn't it be better to
>>> make
>>> there be no return value in that case?  If there is a return value, some
>>> calling context in the future might take that into account and then the
>>> lack of a free_irq would be a memory leak.  Also if the first
>>> request_irq
>>> can never fail, perhaps that should be made explicit by not testing the
>>> return value?
>>>
>>> julia
>> This function is an init_call, not a probe function, and it is only
>> called ONCE.
>> The dma device here has two interrupts, one IRQ_DMA, another IRQ_DMAERR.
>> And the device could work without IRQ_DMAERR.
>> The return value should indicate whether there is something wrong during
>> initialization, so the function needs return errno when any request_irq
>> is failed.
>> For the first request_irq, some code has prepared its resources before
>> this call, so I suppose it successful. However, the return value must be
>> tested.
> 
> OK, thank you for the explanation. I will change the patch.
> 

hi Julia,
would you mind to add the explaination to the code ? there is a good chance
that someone will find the same problem again.

re,
 wh