On 2013-04-18 21:39, H Hartley Sweeten wrote:
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.
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.
Yes, commands wouldn't be being processed any more and the device should
no longer be not be generating interrupts (at least for asynchronous
commands, and assuming they're not broken - the lack of a cancel routine
means a driver might not know it's no longer supposed to be processing a
command, but chances are it will have already crashed by then). If it
needs to, the detach routine should stop it generating interrupts. But
despite the device no longer generating interrupts, its interrupt
handler can still be called due to an interrupt from another device; for
a shared IRQ, all the handlers using that IRQ get called when any using
the IRQ raises an interrupt.
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.
Even that's not completely safe. We could do with a smb_wmb() after
setting dev->attached to 0, and a smb_rmb() in the interrupt handlers
before they check dev->attached to make it safer.
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.
I'm not sure when the core would call a *reset handler.
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.
Yes, the free_irq() for the legacy drivers looked okay. I'm about to
look over your v2 series.
--
-=( 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
