On 2014-08-01 20:50, H Hartley Sweeten wrote:
[snip]
diff --git a/drivers/staging/comedi/comedidev.h b/drivers/staging/comedi/comedidev.h
index 58e58a3..73e5fd3 100644
--- a/drivers/staging/comedi/comedidev.h
+++ b/drivers/staging/comedi/comedidev.h
@@ -502,7 +502,7 @@ struct pci_driver;
struct pci_dev *comedi_to_pci_dev(struct comedi_device *);
int comedi_pci_enable(struct comedi_device *);
-void comedi_pci_disable(struct comedi_device *);
+void comedi_pci_detach(struct comedi_device *);
int comedi_pci_auto_config(struct pci_dev *, struct comedi_driver *,
unsigned long context);
@@ -543,7 +543,7 @@ static inline int comedi_pci_enable(struct comedi_device *dev)
return -ENOSYS;
}
-static inline void comedi_pci_disable(struct comedi_device *dev)
+static inline void comedi_pcmcia_disable(struct comedi_device *dev)
{
}
I think you meant to rename it to comedi_pci_detach() there?
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().
diff --git a/drivers/staging/comedi/drivers/addi_apci_3120.c b/drivers/staging/comedi/drivers/addi_apci_3120.c
index 0b77f10..2c4c23b 100644
--- a/drivers/staging/comedi/drivers/addi_apci_3120.c
+++ b/drivers/staging/comedi/drivers/addi_apci_3120.c
@@ -198,8 +198,6 @@ static void apci3120_detach(struct comedi_device *dev)
if (devpriv) {
if (dev->iobase)
apci3120_reset(dev);
- if (dev->irq)
- free_irq(dev->irq, dev);
if (devpriv->ul_DmaBufferVirtual[0]) {
free_pages((unsigned long)devpriv->
ul_DmaBufferVirtual[0],
@@ -211,7 +209,7 @@ static void apci3120_detach(struct comedi_device *dev)
devpriv->ui_DmaBufferPages[1]);
}
}
- comedi_pci_disable(dev);
+ comedi_pci_detach(dev);
}
This is probably okay as any running command should have been cancelled,
but it may be better to do the free_pages() after the
comedi_pci_detach() (specifically, the free_irq()).
diff --git a/drivers/staging/comedi/drivers/adl_pci9118.c b/drivers/staging/comedi/drivers/adl_pci9118.c
index f30b84e..073740b 100644
--- a/drivers/staging/comedi/drivers/adl_pci9118.c
+++ b/drivers/staging/comedi/drivers/adl_pci9118.c
@@ -2036,8 +2036,6 @@ static void pci9118_detach(struct comedi_device *dev)
if (devpriv) {
if (dev->iobase)
pci9118_reset(dev);
- if (dev->irq)
- free_irq(dev->irq, dev);
if (devpriv->dmabuf_virt[0])
free_pages((unsigned long)devpriv->dmabuf_virt[0],
devpriv->dmabuf_pages[0]);
@@ -2045,7 +2043,7 @@ static void pci9118_detach(struct comedi_device *dev)
free_pages((unsigned long)devpriv->dmabuf_virt[1],
devpriv->dmabuf_pages[1]);
}
- comedi_pci_disable(dev);
+ comedi_pci_detach(dev);
if (pcidev)
pci_dev_put(pcidev);
}
Same here - probably better to do the free_pages() after the
comedi_pci_detach().
diff --git a/drivers/staging/comedi/drivers/amplc_pci224.c b/drivers/staging/comedi/drivers/amplc_pci224.c
index 6fed6b8..21f06bb 100644
--- a/drivers/staging/comedi/drivers/amplc_pci224.c
+++ b/drivers/staging/comedi/drivers/amplc_pci224.c
@@ -1166,14 +1166,12 @@ static void pci224_detach(struct comedi_device *dev)
{
struct pci224_private *devpriv = dev->private;
- if (dev->irq)
- free_irq(dev->irq, dev);
if (devpriv) {
kfree(devpriv->ao_readback);
kfree(devpriv->ao_scan_vals);
kfree(devpriv->ao_scan_order);
}
- comedi_pci_disable(dev);
+ comedi_pci_detach(dev);
}
Again, it should be perfectly safe, but it makes more sense to free
stuff that the interrupt handler uses after freeing the interrupt
handler, or in this case, moving the kfree() calls after
comedi_pci_detach().
diff --git a/drivers/staging/comedi/drivers/cb_pcidas64.c b/drivers/staging/comedi/drivers/cb_pcidas64.c
index fa12614..b3041e3 100644
--- a/drivers/staging/comedi/drivers/cb_pcidas64.c
+++ b/drivers/staging/comedi/drivers/cb_pcidas64.c
@@ -4001,8 +4001,6 @@ static void detach(struct comedi_device *dev)
struct pcidas64_private *devpriv = dev->private;
unsigned int i;
- if (dev->irq)
- free_irq(dev->irq, dev);
if (devpriv) {
if (pcidev) {
if (devpriv->plx9080_iobase) {
@@ -4011,8 +4009,6 @@ static void detach(struct comedi_device *dev)
}
if (devpriv->main_iobase)
iounmap(devpriv->main_iobase);
- if (dev->mmio)
- iounmap(dev->mmio);
/* free pci dma buffers */
for (i = 0; i < ai_dma_ring_count(thisboard); i++) {
if (devpriv->ai_buffer[i])
@@ -4043,7 +4039,7 @@ static void detach(struct comedi_device *dev)
devpriv->ao_dma_desc_bus_addr);
}
}
- comedi_pci_disable(dev);
+ comedi_pci_detach(dev);
}
There may be a race condition with the interrupt handler here. And on
general principle, all the fancy unmapping and freeing of stuff used by
the interrupt handler should be done after freeing the interrupt handler.
There should be no need to test the 'pcidev' pointer is non-NULL though.
That's probably left over from when this driver supported the "legacy"
attach handler.
diff --git a/drivers/staging/comedi/drivers/daqboard2000.c b/drivers/staging/comedi/drivers/daqboard2000.c
index cd369cd..ac9f83c 100644
--- a/drivers/staging/comedi/drivers/daqboard2000.c
+++ b/drivers/staging/comedi/drivers/daqboard2000.c
@@ -755,15 +755,9 @@ static void daqboard2000_detach(struct comedi_device *dev)
{
struct daqboard2000_private *devpriv = dev->private;
- if (dev->irq)
- free_irq(dev->irq, dev);
- if (devpriv) {
- if (dev->mmio)
- iounmap(dev->mmio);
- if (devpriv->plx)
- iounmap(devpriv->plx);
- }
- comedi_pci_disable(dev);
+ if (devpriv && devpriv->plx)
+ iounmap(devpriv->plx);
+ comedi_pci_detach(dev);
}
This driver doesn't actually support interrupts, but if it did, the
interrupt handler would probably want to check the PLX registers that
have been unmapped, so better to do the unmap after comedi_pci_detach().
diff --git a/drivers/staging/comedi/drivers/gsc_hpdi.c b/drivers/staging/comedi/drivers/gsc_hpdi.c
index 91c1e8c..77817f3 100644
--- a/drivers/staging/comedi/drivers/gsc_hpdi.c
+++ b/drivers/staging/comedi/drivers/gsc_hpdi.c
@@ -685,15 +685,11 @@ static void gsc_hpdi_detach(struct comedi_device *dev)
struct hpdi_private *devpriv = dev->private;
unsigned int i;
- if (dev->irq)
- free_irq(dev->irq, dev);
if (devpriv) {
if (devpriv->plx9080_mmio) {
writel(0, devpriv->plx9080_mmio + PLX_INTRCS_REG);
iounmap(devpriv->plx9080_mmio);
}
- if (dev->mmio)
- iounmap(dev->mmio);
/* free pci dma buffers */
for (i = 0; i < NUM_DMA_BUFFERS; i++) {
if (devpriv->dio_buffer[i])
@@ -711,7 +707,7 @@ static void gsc_hpdi_detach(struct comedi_device *dev)
devpriv->dma_desc,
devpriv->dma_desc_phys_addr);
}
- comedi_pci_disable(dev);
+ comedi_pci_detach(dev);
}
Again, the unmapping and stuff should be done after freeing the
interrupt handler. OTOH, it would also be better to write 0 to the
PLX_INTRCS_REG before freeing the interrupt handler.
diff --git a/drivers/staging/comedi/drivers/me_daq.c b/drivers/staging/comedi/drivers/me_daq.c
index 37a6fa9..a1e52a9 100644
--- a/drivers/staging/comedi/drivers/me_daq.c
+++ b/drivers/staging/comedi/drivers/me_daq.c
@@ -554,14 +554,12 @@ static void me_detach(struct comedi_device *dev)
struct me_private_data *dev_private = dev->private;
if (dev_private) {
- if (dev->mmio) {
+ if (dev->mmio)
me_reset(dev);
- iounmap(dev->mmio);
- }
if (dev_private->plx_regbase)
iounmap(dev_private->plx_regbase);
}
- comedi_pci_disable(dev);
+ comedi_pci_detach(dev);
}
Doesn't really matter here as it doesn't support interrupts, but I'd
move the iounmap() after the comedi_pci_detach() for consistency.
diff --git a/drivers/staging/comedi/drivers/mf6x4.c b/drivers/staging/comedi/drivers/mf6x4.c
index 464f4b4..4316ad3 100644
--- a/drivers/staging/comedi/drivers/mf6x4.c
+++ b/drivers/staging/comedi/drivers/mf6x4.c
@@ -305,12 +305,10 @@ static void mf6x4_detach(struct comedi_device *dev)
if (devpriv->bar0_mem)
iounmap(devpriv->bar0_mem);
- if (dev->mmio)
- iounmap(dev->mmio);
if (devpriv->bar2_mem)
iounmap(devpriv->bar2_mem);
- comedi_pci_disable(dev);
+ comedi_pci_detach(dev);
}
Same here for consistency.
diff --git a/drivers/staging/comedi/drivers/ni_660x.c b/drivers/staging/comedi/drivers/ni_660x.c
index b0b03d4..5451b4b 100644
--- a/drivers/staging/comedi/drivers/ni_660x.c
+++ b/drivers/staging/comedi/drivers/ni_660x.c
@@ -1176,17 +1176,13 @@ static void ni_660x_detach(struct comedi_device *dev)
{
struct ni_660x_private *devpriv = dev->private;
- if (dev->irq)
- free_irq(dev->irq, dev);
if (devpriv) {
if (devpriv->counter_dev)
ni_gpct_device_destroy(devpriv->counter_dev);
ni_660x_free_mite_rings(dev);
mite_detach(devpriv->mite);
}
- if (dev->mmio)
- iounmap(dev->mmio);
- comedi_pci_disable(dev);
+ comedi_pci_detach(dev);
}
Again, do the unmapping and freeing after freeing the interrupt handler.
diff --git a/drivers/staging/comedi/drivers/ni_pcidio.c b/drivers/staging/comedi/drivers/ni_pcidio.c
index b5b36af..1aae277 100644
--- a/drivers/staging/comedi/drivers/ni_pcidio.c
+++ b/drivers/staging/comedi/drivers/ni_pcidio.c
@@ -986,8 +986,6 @@ static void nidio_detach(struct comedi_device *dev)
{
struct nidio96_private *devpriv = dev->private;
- if (dev->irq)
- free_irq(dev->irq, dev);
if (devpriv) {
if (devpriv->di_mite_ring) {
mite_free_ring(devpriv->di_mite_ring);
@@ -995,9 +993,7 @@ static void nidio_detach(struct comedi_device *dev)
}
mite_detach(devpriv->mite);
}
- if (dev->mmio)
- iounmap(dev->mmio);
- comedi_pci_disable(dev);
+ comedi_pci_detach(dev);
}
Ditto.
diff --git a/drivers/staging/comedi/drivers/ni_pcimio.c b/drivers/staging/comedi/drivers/ni_pcimio.c
index da61fa7..da55549 100644
--- a/drivers/staging/comedi/drivers/ni_pcimio.c
+++ b/drivers/staging/comedi/drivers/ni_pcimio.c
@@ -1113,8 +1113,6 @@ static void pcimio_detach(struct comedi_device *dev)
struct ni_private *devpriv = dev->private;
mio_common_detach(dev);
- if (dev->irq)
- free_irq(dev->irq, dev);
if (devpriv) {
mite_free_ring(devpriv->ai_mite_ring);
mite_free_ring(devpriv->ao_mite_ring);
@@ -1123,9 +1121,7 @@ static void pcimio_detach(struct comedi_device *dev)
mite_free_ring(devpriv->gpct_mite_ring[1]);
mite_detach(devpriv->mite);
}
- if (dev->mmio)
- iounmap(dev->mmio);
- comedi_pci_disable(dev);
+ comedi_pci_detach(dev);
}
Ditto.
diff --git a/drivers/staging/comedi/drivers/rtd520.c b/drivers/staging/comedi/drivers/rtd520.c
index 6fc4ed3..ae53307 100644
--- a/drivers/staging/comedi/drivers/rtd520.c
+++ b/drivers/staging/comedi/drivers/rtd520.c
@@ -1368,16 +1368,13 @@ static void rtd_detach(struct comedi_device *dev)
writel(readl(devpriv->lcfg + PLX_INTRCS_REG) &
~(ICS_PLIE | ICS_DMA0_E | ICS_DMA1_E),
devpriv->lcfg + PLX_INTRCS_REG);
- free_irq(dev->irq, dev);
}
- if (dev->mmio)
- iounmap(dev->mmio);
if (devpriv->las1)
iounmap(devpriv->las1);
if (devpriv->lcfg)
iounmap(devpriv->lcfg);
}
- comedi_pci_disable(dev);
+ comedi_pci_detach(dev);
}
Ditto.
diff --git a/drivers/staging/comedi/drivers/s626.c b/drivers/staging/comedi/drivers/s626.c
index 080608a..975cda4 100644
--- a/drivers/staging/comedi/drivers/s626.c
+++ b/drivers/staging/comedi/drivers/s626.c
@@ -2929,13 +2929,8 @@ static void s626_detach(struct comedi_device *dev)
s626_close_dma_b(dev, &devpriv->ana_buf,
S626_DMABUF_SIZE);
}
-
- if (dev->irq)
- free_irq(dev->irq, dev);
- if (dev->mmio)
- iounmap(dev->mmio);
}
- comedi_pci_disable(dev);
+ comedi_pci_detach(dev);
}
It would be better if the s626_close_dma_b() calls were done after
freeing the interrupt handler, but the original did it the wrong way
round as well!
--
-=( 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
