On Mon, 7 Jan 2013, Guennadi Liakhovetski wrote: > (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. OK, I seem to have been thrown off by the clearing fo the name field, but that doesn't seem to be very important. > 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? OK, thanks for the explanation. > 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(). OK, if it is safe to call free_irq much later, then I can propose a patch for that. > 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;-) Sure. Thanks for the suggestion! julia > 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 > -- 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
