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