[PATCH 13/16] staging: comedi: cb_pcidas64: use comedi_pci_detach()

[Hartley Sweeten]

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 at visionengravers.com>
>> Cc: Ian Abbott <abbotti at mev.co.uk>
>> Cc: Greg Kroah-Hartman <gregkh at linuxfoundation.org>
>> ---
>>   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