Re: [PATCH 1/2] drivers/media/platform/soc_camera/pxa_camera.c: reposition free_irq to avoid access to invalid data

[Robert Jarzmik <robert.jarzmik@free.fr>]

Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:

> (adding Robert to CC)
> 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?

Agreed that pxa_free_dma() doesn't free anything, that one is easy :)

And agreed too for the second part, with a slighly different explanation :
 - pxa_camera_remove_device() has been called as you said
 - inside this function, check comment
   "/* disable capture, disable interrupts */"
   => this ensures no interrupt can be generated anymore

So after pxa_camera_remove_device() has been called, no interrupts can be
generated.

Yet as you said, it leaves the "almost impossible" scenario :
 - a user begins a capture
 - the user closes the capture device and unloads pxa-camera.ko:
     soc_camera_close()
       pxa_camera_remove_device()
         the IRQ line is asserted but doesn't trigger yet the interrupt handler
         (yes I know, improbable)
         meanwhile, IRQs are disabled, DMA channels are stopped
     switch_to(rmmod)
       => yes I know, impossible, the interrupt handler must be run before, but
       let's continue for love of discussion ...
     rmmod pxa-camera
       pxa_camera_remove()
         pxa_free_dma() * 3
         ----> here the IRQ handler kicks in !!!
               => pxa_camera_irq()
                    pxa_dma_start_channels()
         ----> it hurts !

My call is that this is impossible because the switch_to() should run the IRQ
handler before pxa_camera_remove() is called.

So all this to say that I think we're safe, unless a heavy ion or a cosmic ray
strikes the PXA :)

Cheers.

--
Robert