Re: [PATCH 7/7] arch/unicore32/kernel/dma.c: ensure arguments torequest_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@xxxxxxx>
> > > >
> > > > 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@xxxxxxx>
> > > >
> > > > ---
> > > >  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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/