On 2014-08-05 17:12, Hartley Sweeten wrote:
On Tuesday, August 05, 2014 2:59 AM, Ian Abbott wrote:
On 2014-08-04 23:55, H Hartley Sweeten wrote:
Use comedi_pci_detach() to handle the boilerplate part of the (*detach)
for this PCI driver.
This also fixes a race condition where the interrupt handler is released
before the interrupts are disabled in the hardware.
Signed-off-by: H Hartley Sweeten <hsweeten@xxxxxxxxxxxxxxxxxxx>
Cc: Ian Abbott <abbotti@xxxxxxxxx>
Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
---
drivers/staging/comedi/drivers/cb_pcidas64.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/drivers/staging/comedi/drivers/cb_pcidas64.c b/drivers/staging/comedi/drivers/cb_pcidas64.c
index cc492ee..f9bbd7d 100644
--- a/drivers/staging/comedi/drivers/cb_pcidas64.c
+++ b/drivers/staging/comedi/drivers/cb_pcidas64.c
@@ -4038,8 +4038,6 @@ static void detach(struct comedi_device *dev)
{
struct pcidas64_private *devpriv = dev->private;
- if (dev->irq)
- free_irq(dev->irq, dev);
if (devpriv) {
if (devpriv->plx9080_iobase) {
disable_plx_interrupts(dev);
@@ -4047,10 +4045,8 @@ static void detach(struct comedi_device *dev)
}
if (devpriv->main_iobase)
iounmap(devpriv->main_iobase);
- if (dev->mmio)
- iounmap(dev->mmio);
}
- comedi_pci_disable(dev);
+ comedi_pci_detach(dev);
cb_pcidas64_free_dma(dev);
}
It still has a race condition between the interrupt handler and
iounmap(devpriv->main_iobase) though. (Even though you've disabled PLX
interrupts, the handler could already be running.)
I guess this could be a potential race condition in any comedi driver that enables
interrupts in the (*attach).
It can be, if it hasn't been set up enough at the point where the
interrupt handler is requested. The initialization code in the attach
ought to initialize the hardware enough to stop it asserting interrupts.
The handler may still be called if the IRQ is shared, though.
Interrupts _should_ only be enabled when a (*do_cmd) starts an async command.
They should then be disabled when the command completes or is canceled.
There may be several interrupt sources though for different parts of the
device, controlled by different subdevices.
Maybe the core should be modified to call all the subdevice (*cancel) operations
before doing the (*detach)? This would also allow removing the (*cancel) calls
from all the (*detach) functions. All the (*cancel) functions should also be checked
to make sure they disable interrupt generation for the subdevice.
It already does that. The cancel will be called on detach if the
subdevice has the SRF_RUNNING flag set.
But, doesn't the core check to make sure the comedi_device is not busy before
doing the (*detach)? I need to look into that...
Yes it does if the detach is via the COMEDI_DEVCONFIG ioctl, but there
are other places the detach will be called, e.g. if the low-level driver
module is being unloaded, or the device is being removed by its
underlying subsystem.
--
-=( 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/driverdev-devel
