[PATCH 12/12] staging: comedi: comedi_pci: free_irq() in comedi_pci_disable()

[H Hartley Sweeten]

On Thursday, April 18, 2013 10:44 AM, Ian Abbott wrote:
> On 2013/04/18 04:51 PM, H Hartley Sweeten wrote:
>> On Thursday, April 18, 2013 5:06 AM, Ian Abbott wrote:
>>> On 2013-04-17 19:20, H Hartley Sweeten wrote:
>>>> All the PCI comedi drivers call comedi_pci_disable() either directly
>>>> or as part of their (*detach). Move the free_irq() into comedi_pci_disable()
>>>> so that the drivers don't have to deal with it.
>>>>
>>>> For drivers that then only call comedi_pci_disable() in their
>>>> private (*detach), remove the private function and use the helper
>>>> directly for the (*detach).
>>>>
>>>> For aesthetic reasons, tidy up the (*detach) in the icp_multi, ni_6527,
>>>> and ni_65xx drivers.
>>>>
>>>> In the rtd520 driver, the write to the PLX_INTRCS_REG register to disable
>>>> the interrupts is not needed. The previous call to rtd_reset() already
>>>> cleared the register.
>>>>
>>>> Signed-off-by: H Hartley Sweeten <hsweeten at visionengravers.com>
>>>> Cc: Ian Abbott <abbotti at mev.co.uk>
>>>> Cc: Greg Kroah-Hartman <gregkh at linuxfoundation.org>
>>>
>>> There is potential for this being unsafe for some drivers, particularly 
>>> for those interrupt handlers that don't check dev->attached as a 
>>> precaution before doing anything else, but instead read some ioremapped 
>>> register to check whether it needs to do anything.  PCI devices used 
>>> shared IRQs so their interrupt handlers can be called even when the 
>>> device is not interrupting.
>> 
>> But, the unsafe condition probably already exists. This patch shouldn't
>> change anything other than the point where the irq is released.
>> 
>> The PCI drivers should be disabling interrupts in their (*detach) and doing
>> any other driver specific cleanup before comedi_pci_disable() is called.
>
> You're missing the point that the interrupt handler will still be called
> even if the driver has stopped the PCI device generating interrupts,
> because the IRQ is (potentially) shared with other PCI devices, so the
> ordering of calls to free_irq() and freeing of things used by the
> interrupt handler matters unless the interrupt handler is careful not to
> access things that might have been freed.

I didn't miss the point, I think I just didn't think about it as deeply as you
have.

I was assuming that the core would only call the (*detach) if the device
was not processing a command. And interrupts are only generated by
the board if a command is being executed.

do_devconfig_ioctl() has this:

		if (is_device_busy(dev))
			return -EBUSY;

before calling comedi_device_detach().

I did overlook the dev->attached issue in the PCI interrupt functions
to make sure the board is actually attached and "could" be the source
of the interrupt.

>> Maybe a (*reset) callback should be added to the comedi_driver? I
>> think I saw a comment in one of the drivers about it.
>
> Certainly any driver that sets up the (*cmd) callback for a subdevice
> should also set up the (*cancel) callback, as asynchronous command
> support is the main use of interrupts.  The (*cancel) should disable any
> interrupt sources used by the asynchronous command.  Some of the drivers
> set (*cmd) without setting (*cancel) and I consider them broken.  Comedi
> should probably refuse to set up asynchronous command support for those.

I agree. Any comedi driver that supports commands should provide a
(*do_cmd), (*do_cmdtest), and (*cancel) function for the subdevice.

But, the (*cancel) is subdevice specific. I think a board level (*reset) function
might be a good idea. Many of the drivers actually have a function that does
this. It's just a matter of tying them into the comedi_driver.

>>> The only ones below that seem to suffer from the above problem are 
>>> cb_pcidas64.c, icp_multi.c, and ni6527.c.  Just checking dev->attached 
>>> and returning early before using anything that might have already been 
>>> freed would be enough to solve the problem for those drivers.
>> 
>> Since PCI interrupts are shared, all the comedi PCI drivers should probably
>> have the dev->attached check in their interrupt functions. But, that should
>> be a separate patch. Again, I don't think this patch makes the PCI drivers 
>> any worse than they already are.
>
> It does for the particular drivers I mentioned.

OK.

>>> amplc_pci224.c's interrupt handler also reads an interrupt status 
>>> register at the start, but since it isn't ioremapped, it shouldn't matter.
>>>
>>> ni_labpc.c's interrupt handler doesn't have the problem, although it 
>>> does print a nasty message if !dev->attached and returns IRQ_HANDLED 
>>> instead of IRQ_NONE.  I just noticed it in passing, so am leaving this 
>>> here as a reminder.
>>>
>>> I recommend skipping this patch until the problematic drivers are sorted 
>>> out.
>> 
>> If you still feel this patch should be skipped please let me know. If
>> possible I would like to get this series in before Greg closes his tree.
>
> I still think it should be skipped for now.  We certainly don't want it
> remaining broken at the time Greg closes his tree.

OK. I'll drop this piece until after the next merge window closes and Greg
reopens the staging tree.

I assume you are ok with the free_irq() patch for the legacy drivers. I'll
leave that patch in the series I will repost later today.

Thanks,
Hartley