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@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. 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 _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
