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
