summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorIra Snyder <iws@ovro.caltech.edu>2012-01-26 11:00:14 +0000
committerBenjamin Herrenschmidt <benh@kernel.crashing.org>2012-02-27 11:33:59 +1100
commit6c15d7afbb2f9e2d3114b513306dae736b56f535 (patch)
tree17ad7aa4544ad1d83b50eb4929f77b7793b5f0f4
parent75ff85a81680e5779383aa6210a4f89ed76e40ec (diff)
carma-fpga: fix race between data dumping and DMA callback
When the system is under heavy load, we occasionally saw a problem where the system would get a legitimate interrupt when they should be disabled. This was caused by the data_dma_cb() DMA callback unconditionally re-enabling FPGA interrupts even when data dumping is disabled. When data dumping was re-enabled, the irq handler would fire while a DMA was in progress. The "BUG_ON(priv->inflight != NULL);" during the second invocation of the DMA callback caused the system to crash. To fix the issue, the priv->enabled boolean is moved under the protection of the priv->lock spinlock. The DMA callback checks the boolean to know whether to re-enable FPGA interrupts before it returns. Now that it is fixed, the driver keeps FPGA interrupts disabled when it expects that they are disabled, fixing the bug. Signed-off-by: Ira W. Snyder <iws@ovro.caltech.edu> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
-rw-r--r--drivers/misc/carma/carma-fpga.c101
1 files changed, 61 insertions, 40 deletions
diff --git a/drivers/misc/carma/carma-fpga.c b/drivers/misc/carma/carma-fpga.c
index 4fd896deda0d..0cfc5bf13fa5 100644
--- a/drivers/misc/carma/carma-fpga.c
+++ b/drivers/misc/carma/carma-fpga.c
@@ -560,6 +560,9 @@ static void data_enable_interrupts(struct fpga_device *priv)
/* flush the writes */
fpga_read_reg(priv, 0, MMAP_REG_STATUS);
+ fpga_read_reg(priv, 1, MMAP_REG_STATUS);
+ fpga_read_reg(priv, 2, MMAP_REG_STATUS);
+ fpga_read_reg(priv, 3, MMAP_REG_STATUS);
/* switch back to the external interrupt source */
iowrite32be(0x3F, priv->regs + SYS_IRQ_SOURCE_CTL);
@@ -591,8 +594,12 @@ static void data_dma_cb(void *data)
list_move_tail(&priv->inflight->entry, &priv->used);
priv->inflight = NULL;
- /* clear the FPGA status and re-enable interrupts */
- data_enable_interrupts(priv);
+ /*
+ * If data dumping is still enabled, then clear the FPGA
+ * status registers and re-enable FPGA interrupts
+ */
+ if (priv->enabled)
+ data_enable_interrupts(priv);
spin_unlock_irqrestore(&priv->lock, flags);
@@ -708,6 +715,15 @@ static irqreturn_t data_irq(int irq, void *dev_id)
spin_lock(&priv->lock);
+ /*
+ * This is an error case that should never happen.
+ *
+ * If this driver has a bug and manages to re-enable interrupts while
+ * a DMA is in progress, then we will hit this statement and should
+ * start paying attention immediately.
+ */
+ BUG_ON(priv->inflight != NULL);
+
/* hide the interrupt by switching the IRQ driver to GPIO */
data_disable_interrupts(priv);
@@ -762,11 +778,15 @@ out:
*/
static int data_device_enable(struct fpga_device *priv)
{
+ bool enabled;
u32 val;
int ret;
/* multiple enables are safe: they do nothing */
- if (priv->enabled)
+ spin_lock_irq(&priv->lock);
+ enabled = priv->enabled;
+ spin_unlock_irq(&priv->lock);
+ if (enabled)
return 0;
/* check that the FPGAs are programmed */
@@ -797,6 +817,9 @@ static int data_device_enable(struct fpga_device *priv)
goto out_error;
}
+ /* prevent the FPGAs from generating interrupts */
+ data_disable_interrupts(priv);
+
/* hookup the irq handler */
ret = request_irq(priv->irq, data_irq, IRQF_SHARED, drv_name, priv);
if (ret) {
@@ -804,11 +827,13 @@ static int data_device_enable(struct fpga_device *priv)
goto out_error;
}
- /* switch to the external FPGA IRQ line */
- data_enable_interrupts(priv);
-
- /* success, we're enabled */
+ /* allow the DMA callback to re-enable FPGA interrupts */
+ spin_lock_irq(&priv->lock);
priv->enabled = true;
+ spin_unlock_irq(&priv->lock);
+
+ /* allow the FPGAs to generate interrupts */
+ data_enable_interrupts(priv);
return 0;
out_error:
@@ -834,41 +859,40 @@ out_error:
*/
static int data_device_disable(struct fpga_device *priv)
{
- int ret;
+ spin_lock_irq(&priv->lock);
/* allow multiple disable */
- if (!priv->enabled)
+ if (!priv->enabled) {
+ spin_unlock_irq(&priv->lock);
return 0;
+ }
+
+ /*
+ * Mark the device disabled
+ *
+ * This stops DMA callbacks from re-enabling interrupts
+ */
+ priv->enabled = false;
- /* switch to the internal GPIO IRQ line */
+ /* prevent the FPGAs from generating interrupts */
data_disable_interrupts(priv);
+ /* wait until all ongoing DMA has finished */
+ while (priv->inflight != NULL) {
+ spin_unlock_irq(&priv->lock);
+ wait_event(priv->wait, priv->inflight == NULL);
+ spin_lock_irq(&priv->lock);
+ }
+
+ spin_unlock_irq(&priv->lock);
+
/* unhook the irq handler */
free_irq(priv->irq, priv);
- /*
- * wait for all outstanding DMA to complete
- *
- * Device interrupts are disabled, therefore another buffer cannot
- * be marked inflight.
- */
- ret = wait_event_interruptible(priv->wait, priv->inflight == NULL);
- if (ret)
- return ret;
-
/* free the correlation table */
sg_free_table(&priv->corl_table);
priv->corl_nents = 0;
- /*
- * We are taking the spinlock not to protect priv->enabled, but instead
- * to make sure that there are no readers in the process of altering
- * the free or used lists while we are setting this flag.
- */
- spin_lock_irq(&priv->lock);
- priv->enabled = false;
- spin_unlock_irq(&priv->lock);
-
/* free all buffers: the free and used lists are not being changed */
data_free_buffers(priv);
return 0;
@@ -896,15 +920,6 @@ static unsigned int list_num_entries(struct list_head *list)
static int data_debug_show(struct seq_file *f, void *offset)
{
struct fpga_device *priv = f->private;
- int ret;
-
- /*
- * Lock the mutex first, so that we get an accurate value for enable
- * Lock the spinlock next, to get accurate list counts
- */
- ret = mutex_lock_interruptible(&priv->mutex);
- if (ret)
- return ret;
spin_lock_irq(&priv->lock);
@@ -917,7 +932,6 @@ static int data_debug_show(struct seq_file *f, void *offset)
seq_printf(f, "num_dropped: %d\n", priv->num_dropped);
spin_unlock_irq(&priv->lock);
- mutex_unlock(&priv->mutex);
return 0;
}
@@ -970,7 +984,13 @@ static ssize_t data_en_show(struct device *dev, struct device_attribute *attr,
char *buf)
{
struct fpga_device *priv = dev_get_drvdata(dev);
- return snprintf(buf, PAGE_SIZE, "%u\n", priv->enabled);
+ int ret;
+
+ spin_lock_irq(&priv->lock);
+ ret = snprintf(buf, PAGE_SIZE, "%u\n", priv->enabled);
+ spin_unlock_irq(&priv->lock);
+
+ return ret;
}
static ssize_t data_en_set(struct device *dev, struct device_attribute *attr,
@@ -986,6 +1006,7 @@ static ssize_t data_en_set(struct device *dev, struct device_attribute *attr,
return -EINVAL;
}
+ /* protect against concurrent enable/disable */
ret = mutex_lock_interruptible(&priv->mutex);
if (ret)
return ret;