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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxxxxxxxxx>
>> Cc: Ian Abbott <abbotti@xxxxxxxxx>
>> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
>
> 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.

Maybe a (*reset) callback should be added to the comedi_driver? I
think I saw a comment in one of the drivers about it.

> 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.

> 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.

Thanks,
Hartley

_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel




[Index of Archives]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux