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@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. 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. > 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. >> 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. >> 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. -- -=( Ian Abbott @ MEV Ltd. E-mail: <abbotti@xxxxxxxxx> )=- -=( Tel: +44 (0)161 477 1898 FAX: +44 (0)161 718 3587 )=- _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
