(adding Robert to CC) Hi Julia Thanks for the patch. On Mon, 7 Jan 2013, Julia Lawall wrote: > From: Julia Lawall <Julia.Lawall@xxxxxxx> > > The data referenced by an interrupt handler should not be freed before the > interrupt is ended. The handler is pxa_camera_irq. This handler may call > pxa_dma_start_channels, which references the channels that are freed on the > lines before the call to free_irq. I don't think any data is freed by pxa_free_dma(), it only disables DMA on a certain channel. Theoretically there could be a different problem: pxa_free_dma() deactivates DMA, whereas pxa_dma_start_channels() activates it. But I think we're also protected against that: by the time pxa_camera_remove() is called, and operation on the interface has been stopped, client devices have been detached, pxa_camera_remove_device() has been called, which has also stopped the interface clock. And with clock stopped no interrupts can be generated. And the case of interrupt having been generated before clk_disabled() and only delivered to the driver so much later, that we're already unloading the module, seems really impossible to me. Robert, you agree? OTOH, it would be nice to convert also this driver to managed allocations, which also would include devm_request(_threaded)_irq(), but that would mean, that free_irq() would be called even later than now, also after pxa_free_dma(). Speaking about managed allocations, those can be dangerous too: if you request an IRQ before, say, remapping memory, or if you only use managed IRQ requesting and ioremap() memory in your driver manually, that would be wrong. But from a quick grep looks like most (all?) drivers get ir right - first ioremap(), then request IRQ, but to be certain maybe coccinelle could run a test for that too;-) Thanks Guennadi > The semantic match that finds this problem is as follows: > (http://coccinelle.lip6.fr/) > > // <smpl> > @fn exists@ > expression list es; > expression a,b; > identifier f; > @@ > > if (...) { > ... when any > free_irq(a,b); > ... when any > f(es); > ... when any > return ...; > } > > @@ > expression list fn.es; > expression fn.a,fn.b; > identifier fn.f; > @@ > > *f(es); > ... when any > *free_irq(a,b); > // </smpl> > > Signed-off-by: Julia Lawall <Julia.Lawall@xxxxxxx> > > --- > Not compiled. I have not observed the problem in practice; the code just > looks suspicious. > > drivers/media/platform/soc_camera/pxa_camera.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/media/platform/soc_camera/pxa_camera.c b/drivers/media/platform/soc_camera/pxa_camera.c > index f91f7bf..2a19aba 100644 > --- a/drivers/media/platform/soc_camera/pxa_camera.c > +++ b/drivers/media/platform/soc_camera/pxa_camera.c > @@ -1810,10 +1810,10 @@ static int pxa_camera_remove(struct platform_device *pdev) > > clk_put(pcdev->clk); > > + free_irq(pcdev->irq, pcdev); > pxa_free_dma(pcdev->dma_chans[0]); > pxa_free_dma(pcdev->dma_chans[1]); > pxa_free_dma(pcdev->dma_chans[2]); > - free_irq(pcdev->irq, pcdev); > > soc_camera_host_unregister(soc_host); > > --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
