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

[Ian Abbott]

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 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).

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 at mev.co.uk>        )=-
-=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=-