[PATCH] staging: comedi: comedi_pci: enhance comedi_pci_disable()

[Hartley Sweeten]

On Monday, August 04, 2014 3:49 AM, Ian Abbott [mailto:abbotti at mev.co.uk] wrote:
>> diff --git a/drivers/staging/comedi/drivers/addi_apci_2032.c b/drivers/staging/comedi/drivers/addi_apci_2032.c
>> index be0a8a7..d35998d 100644
>> --- a/drivers/staging/comedi/drivers/addi_apci_2032.c
>> +++ b/drivers/staging/comedi/drivers/addi_apci_2032.c
>> @@ -339,11 +339,9 @@ static void apci2032_detach(struct comedi_device *dev)
>>   {
>>   	if (dev->iobase)
>>   		apci2032_reset(dev);
>> -	if (dev->irq)
>> -		free_irq(dev->irq, dev);
>>   	if (dev->read_subdev)
>>   		kfree(dev->read_subdev->private);
>> -	comedi_pci_disable(dev);
>> +	comedi_pci_detach(dev);
>>   }
>
> That could cause the interrupt handler to access freed memory due to a 
> race condition.  To avoid that, move the kfree() after the call to 
> comedi_pci_detach().

Ian,

I looked over these and I think the correct order in the (*detach) should be:

1) Do any 'reset' call that the driver has to disable interrupts.
2) iounmap() any extra addresses (in the private data)
3) call comedi_pci_detach(), which does:
	a) free_irq(dev->irq)
	b) iounmap(dev->mmio)
	c) pci_release_regions(pcidev)
	d) pci_disable_device(pcidev)
4) then do any additional cleanup (kfree() etc.)

I'm not sure if it matters, but I think all the iounmap() should be done
before the PCI regions are released and the device is disabled.

I'm rebasing the patch based on this and will post it shortly.

Regards,
Hartley