Thread (11 messages) 11 messages, 3 authors, 2011-02-10

Re: [PATCH 1/2] misc: add CARMA DATA-FPGA Access Driver

flat view

From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Date: 2011-02-09 08:33:35
Also in: lkml

Hi Ira,

On Tue, Feb 08, 2011 at 03:37:46PM -0800, Ira W. Snyder wrote:
This driver allows userspace to access the data processing FPGAs on the
OVRO CARMA board. It has two modes of operation:
Thank you for making the changes, some more comments below.
quoted hunk ↗ jump to hunk
+
+#define inode_to_dev(inode) container_of(inode->i_cdev, struct fpga_device, cdev)
+
Does not seem to be used.
quoted hunk ↗ jump to hunk
+/*
+ * Data Buffer Allocation Helpers
+ */
+
+static int data_map_buffer(struct device *dev, struct data_buf *buf)
+{
+	return videobuf_dma_map(dev, &buf->vb);
+}
+
+static void data_unmap_buffer(struct device *dev, struct data_buf *buf)
+{
+	videobuf_dma_unmap(dev, &buf->vb);
+}
Why can't we all videobuf_dma_map() and videobuf_dma_unmap() directly?
What these helpers supposed to solve?
quoted hunk ↗ jump to hunk
+static int data_alloc_buffers(struct fpga_device *priv)
+{
+	struct data_buf *buf;
+	int i, ret;
+
+	for (i = 0; i < MAX_DATA_BUFS; i++) {
+
+		/* allocate a buffer */
+		buf = data_alloc_buffer(priv->bufsize);
+		if (!buf)
+			continue;
break?
quoted hunk ↗ jump to hunk
+
+		/* map it for DMA */
+		ret = data_map_buffer(priv->dev, buf);
+		if (ret) {
+			data_free_buffer(buf);
+			continue;
and here as well?
quoted hunk ↗ jump to hunk
+		}
+
+		/* add it to the list of free buffers */
+		list_add_tail(&buf->entry, &priv->free);
+		priv->num_buffers++;
+	}
+
+	/* Make sure we allocated the minimum required number of buffers */
+	if (priv->num_buffers < MIN_DATA_BUFS) {
+		dev_err(priv->dev, "Unable to allocate enough data buffers\n");
+		data_free_buffers(priv);
+		return -ENOMEM;
+	}
+
+	/* Warn if we are running in a degraded state, but do not fail */
+	if (priv->num_buffers < MAX_DATA_BUFS) {
+		dev_warn(priv->dev, "Unable to allocate one second worth of "
+				    "buffers, using %d buffers instead\n", i);
The latest style is not break strings even if they exceed 80 column
limit to make sure they are easily greppable.
quoted hunk ↗ jump to hunk
+static void data_dma_cb(void *data)
+{
+	struct fpga_device *priv = data;
+	struct data_buf *buf;
+	unsigned long flags;
+
+	spin_lock_irqsave(&priv->lock, flags);
+
+	/* clear the FPGA status and re-enable interrupts */
+	data_enable_interrupts(priv);
+
+	/* If the inflight list is empty, we've got a bug */
+	BUG_ON(list_empty(&priv->inflight));
+
+	/* Grab the first buffer from the inflight list */
+	buf = list_first_entry(&priv->inflight, struct data_buf, entry);
+	list_del_init(&buf->entry);
+
+	/* Add it to the used list */
+	list_add_tail(&buf->entry, &priv->used);
or
	list_move_tail(&buf->entry, &priv->used);
quoted hunk ↗ jump to hunk
+
+static irqreturn_t data_irq(int irq, void *dev_id)
+{
+	struct fpga_device *priv = dev_id;
+	struct data_buf *buf;
+	u32 status;
+	int i;
+
+	/* detect spurious interrupts via FPGA status */
+	for (i = 0; i < 4; i++) {
+		status = fpga_read_reg(priv, i, MMAP_REG_STATUS);
+		if (!(status & (CORL_DONE | CORL_ERR))) {
+			dev_err(priv->dev, "spurious irq detected (FPGA)\n");
+			return IRQ_NONE;
+		}
+	}
+
+	/* detect spurious interrupts via raw IRQ pin readback */
+	status = ioread32be(priv->regs + SYS_IRQ_INPUT_DATA);
+	if (status & IRQ_CORL_DONE) {
+		dev_err(priv->dev, "spurious irq detected (IRQ)\n");
+		return IRQ_NONE;
+	}
+
+	spin_lock(&priv->lock);
+
+	/* hide the interrupt by switching the IRQ driver to GPIO */
+	data_disable_interrupts(priv);
+
+	/* Check that we actually have a free buffer */
+	if (list_empty(&priv->free)) {
+		priv->num_dropped++;
+		data_enable_interrupts(priv);
+		goto out_unlock;
+	}
+
+	buf = list_first_entry(&priv->free, struct data_buf, entry);
+	list_del_init(&buf->entry);
+
+	/* Check the buffer size */
+	BUG_ON(buf->size != priv->bufsize);
+
+	/* Submit a DMA transfer to get the correlation data */
+	if (data_submit_dma(priv, buf)) {
+		dev_err(priv->dev, "Unable to setup DMA transfer\n");
+		list_add_tail(&buf->entry, &priv->free);
+		data_enable_interrupts(priv);
+		goto out_unlock;
+	}
+
+	/* DMA setup succeeded, GO!!! */
+	list_add_tail(&buf->entry, &priv->inflight);
+	dma_async_memcpy_issue_pending(priv->chan);
+
+out_unlock:
+	spin_unlock(&priv->lock);
+	return IRQ_HANDLED;
+}
Hmm, I wonder if we could simplify this a bit by using list_move()...

	bool submitted = false;
	...

	if (list_empty(&priv->free)) {
		priv->num_dropped++;
		goto out;
	}

	buf = list_first_entry(&priv->free, struct data_buf, entry);
	BUG_ON(buf->size != priv->bufsize);

	/* Submit a DMA transfer to get the correlation data */
	if (data_submit_dma(priv, buf)) {
		dev_err(priv->dev, "Unable to setup DMA transfer\n");
		goto out;
	}

	/* DMA setup succeeded, GO!!! */
	list_move_tail(&buf->entry, &priv->inflight);
	dma_async_memcpy_issue_pending(priv->chan);
	submitted = true;

out:
	if (!submitted)
		data_enable_interrupts(priv);
	spin_unlock(&priv->lock);
	return IRQ_HANDLED;
}

OTOH, we can't have more than 1 in-flight buffer, can we? Should
we even have a list?
quoted hunk ↗ jump to hunk
+
+/*
+ * Realtime Device Enable Helpers
+ */
+
+/**
+ * data_device_enable() - enable the device for buffered dumping
+ * @priv: the driver's private data structure
+ *
+ * Enable the device for buffered dumping. Allocates buffers and hooks up
+ * the interrupt handler. When this finishes, data will come pouring in.
+ *
+ * LOCKING: must hold dev->mutex
+ * CONTEXT: user context only
+ *
+ * Returns 0 on success, -ERRNO otherwise
+ */
+static int data_device_enable(struct fpga_device *priv)
+{
+	u32 val;
+	int ret;
+
+	/* multiple enables are safe: they do nothing */
+	if (priv->enabled)
+		return 0;
+
+	/* check that the FPGAs are programmed */
+	val = ioread32be(priv->regs + SYS_FPGA_CONFIG_STATUS);
+	if (!(val & (1 << 18))) {
+		dev_err(priv->dev, "DATA-FPGAs are not enabled\n");
+		return -ENODATA;
+	}
+
+	/* read the FPGAs to calculate the buffer size */
+	ret = data_calculate_bufsize(priv);
+	if (ret) {
+		dev_err(priv->dev, "unable to calculate buffer size\n");
+		goto out_error;
+	}
+
+	/* allocate the correlation data buffers */
+	ret = data_alloc_buffers(priv);
+	if (ret) {
+		dev_err(priv->dev, "unable to allocate buffers\n");
+		goto out_error;
+	}
+
+	/* setup the source scatterlist for dumping correlation data */
+	ret = data_setup_corl_table(priv);
+	if (ret) {
+		dev_err(priv->dev, "unable to setup correlation DMA table\n");
+		goto out_error;
+	}
+
+	/* switch to the external FPGA IRQ line */
+	data_enable_interrupts(priv);
Not after setting interrupt handler? Can't that lead to "unhandled
interrupt" scenarios?
quoted hunk ↗ jump to hunk
+
+	/* hookup the irq handler */
+	ret = request_irq(priv->irq, data_irq, IRQF_SHARED, drv_name, priv);
+	if (ret) {
+		dev_err(priv->dev, "unable to request IRQ handler\n");
+		goto out_error;
+	}
+
+	/* success, we're enabled */
+	priv->enabled = true;
+	return 0;
+
+out_error:
+	sg_free_table(&priv->corl_table);
+	priv->corl_nents = 0;
+
+	data_free_buffers(priv);
+	return ret;
+}
+
+/**
+ * data_device_disable() - disable the device for buffered dumping
+ * @priv: the driver's private data structure
+ *
+ * Disable the device for buffered dumping. Stops new DMA transactions from
+ * being generated, waits for all outstanding DMA to complete, and then frees
+ * all buffers.
+ *
+ * LOCKING: must hold dev->mutex
+ * CONTEXT: user only
+ *
+ * Returns 0 on success, -ERRNO otherwise
+ */
+static int data_device_disable(struct fpga_device *priv)
+{
+	struct list_head *list;
+	int ret;
+
+	/* allow multiple disable */
+	if (!priv->enabled)
+		return 0;
+
+	/* switch to the internal GPIO IRQ line */
+	data_disable_interrupts(priv);
+
+	/* unhook the irq handler */
+	free_irq(priv->irq, priv);
+
+	/*
+	 * wait for all outstanding DMA to complete
+	 *
+	 * Device interrupts are disabled, so no more buffers can be added to
+	 * the inflight list. Therefore we do not need any locking.
+	 */
+	list = &priv->inflight;
+	while (!list_empty(list)) {
+		ret = wait_event_interruptible(priv->wait, list_empty(list));
+		if (ret)
+			return ret;
+	}
Simply

	ret = wait_event_interruptible(priv->wait, list_empty(&prov->inflight));
	if (ret)
		return ret;

is enough, no need to loop.
quoted hunk ↗ jump to hunk
+
+	/* free the correlation table */
+	sg_free_table(&priv->corl_table);
+	priv->corl_nents = 0;
+
+	/* free all of the buffers */
+	data_free_buffers(priv);
+	priv->enabled = false;
I think this should be:

	/*
	 * We are taking lock not to protect priv->enabled but to make
	 * sure there are no readers in process of altering free/used
	 * lists while we are setting the flag.
	 */
	spin_lock_irq(&priv->lock);
	priv->enabled = false;
	spin_unlock_irq(&priv->lock);

	/*
	 * Wake up readers so they error out and hopefully close their
	 * file descriptors.
	 */
	wake_up(&priv->wait);

	/* We now know that readers will not touch free/used lists */
	data_free_buffers();

Also please see the comments in data_read().

quoted hunk ↗ jump to hunk
+	return 0;
+}
+
+/*
+ * SYSFS Attributes
+ */
+
+/*
+ * Count the number of entries in the given list
+ */
+static unsigned int list_num_entries(struct list_head *list)
+{
+	struct list_head *entry;
+	unsigned int ret = 0;
+
+	list_for_each(entry, list)
+		ret++;
+
+	return ret;
+}
+
+static ssize_t data_num_buffers_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->num_buffers);
+}
+
+static ssize_t data_bufsize_show(struct device *dev,
+				 struct device_attribute *attr, char *buf)
+{
+	struct fpga_device *priv = dev_get_drvdata(dev);
+	return snprintf(buf, PAGE_SIZE, "%zu\n", priv->bufsize);
+}
+
+static ssize_t data_inflight_show(struct device *dev,
+				  struct device_attribute *attr, char *buf)
+{
+	struct fpga_device *priv = dev_get_drvdata(dev);
+	unsigned int num = list_num_entries(&priv->inflight);
+	return snprintf(buf, PAGE_SIZE, "%u\n", num);
+}
+
+static ssize_t data_free_show(struct device *dev,
+			      struct device_attribute *attr, char *buf)
+{
+	struct fpga_device *priv = dev_get_drvdata(dev);
+	unsigned int num = list_num_entries(&priv->free);
+	return snprintf(buf, PAGE_SIZE, "%u\n", num);
+}
+
+static ssize_t data_used_show(struct device *dev,
+			      struct device_attribute *attr, char *buf)
+{
+	struct fpga_device *priv = dev_get_drvdata(dev);
+	unsigned int num = list_num_entries(&priv->used);
+	return snprintf(buf, PAGE_SIZE, "%u\n", num);
+}
+
+static ssize_t data_num_dropped_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->num_dropped);
+}
+
+static ssize_t data_en_show(struct device *dev, struct device_attribute *attr,
+			    char *buf)
+{
+	struct fpga_device *priv = dev_get_drvdata(dev);
+	ssize_t count;
+
+	count = mutex_lock_interruptible(&priv->mutex);
+	if (count)
+		return count;
+
+	count = snprintf(buf, PAGE_SIZE, "%u\n", priv->enabled);
+	mutex_unlock(&priv->mutex);
No locking needed.
quoted hunk ↗ jump to hunk
+	return count;
+}
+
+static ssize_t data_en_set(struct device *dev, struct device_attribute *attr,
+			   const char *buf, size_t count)
+{
+	struct fpga_device *priv = dev_get_drvdata(dev);
+	unsigned long enable;
+	int ret;
+
+	ret = strict_strtoul(buf, 0, &enable);
+	if (ret) {
+		dev_err(priv->dev, "unable to parse enable input\n");
+		return -EINVAL;
+	}
+
+	ret = mutex_lock_interruptible(&priv->mutex);
+	if (ret)
+		return ret;
+
+	if (enable)
+		ret = data_device_enable(priv);
+	else
+		ret = data_device_disable(priv);
+
Have you considered enabling the device when someone opens the device
node and closing it when last user drops off?

quoted hunk ↗ jump to hunk
+	if (ret) {
+		dev_err(priv->dev, "device %s failed\n",
+			enable ? "enable" : "disable");
+		count = ret;
+		goto out_unlock;
+	}
+
+out_unlock:
+	mutex_unlock(&priv->mutex);
+	return count;
+}
+
+static DEVICE_ATTR(num_buffers, S_IRUGO, data_num_buffers_show, NULL);
+static DEVICE_ATTR(buffer_size, S_IRUGO, data_bufsize_show, NULL);
+static DEVICE_ATTR(num_inflight, S_IRUGO, data_inflight_show, NULL);
+static DEVICE_ATTR(num_free, S_IRUGO, data_free_show, NULL);
+static DEVICE_ATTR(num_used, S_IRUGO, data_used_show, NULL);
+static DEVICE_ATTR(num_dropped, S_IRUGO, data_num_dropped_show, NULL);
+static DEVICE_ATTR(enable, S_IWUSR | S_IRUGO, data_en_show, data_en_set);
+
+static struct attribute *data_sysfs_attrs[] = {
+	&dev_attr_num_buffers.attr,
+	&dev_attr_buffer_size.attr,
+	&dev_attr_num_inflight.attr,
+	&dev_attr_num_free.attr,
+	&dev_attr_num_used.attr,
+	&dev_attr_num_dropped.attr,
+	&dev_attr_enable.attr,
+	NULL,
+};
+
+static const struct attribute_group rt_sysfs_attr_group = {
+	.attrs = data_sysfs_attrs,
+};
As I mentioned, consider switching to debugfs - you won't need multiple
attributes and will be able to get full picture (number of
free/used/in-flight/etc) in one file and will not be constrained with
ABI concerns.
quoted hunk ↗ jump to hunk
+
+/*
+ * FPGA Realtime Data Character Device
+ */
+
+static int data_open(struct inode *inode, struct file *filp)
+{
+	/*
+	 * The miscdevice layer puts our struct miscdevice into the
+	 * filp->private_data field. We use this to find our private
+	 * data and then overwrite it with our own private structure.
+	 */
+	struct fpga_device *priv = container_of(filp->private_data,
+						struct fpga_device, miscdev);
+	struct fpga_reader *reader;
+	int ret;
+
+	/* allocate private data */
+	reader = kzalloc(sizeof(*reader), GFP_KERNEL);
+	if (!reader)
+		return -ENOMEM;
+
+	reader->priv = priv;
+	reader->buf = NULL;
+
+	filp->private_data = reader;
+	ret = nonseekable_open(inode, filp);
+	if (ret) {
+		dev_err(priv->dev, "nonseekable-open failed\n");
+		kfree(reader);
+		return ret;
+	}
+
I wonder if the device should allow only exclusive open... Unless you
distribute copies of data between all readers.
quoted hunk ↗ jump to hunk
+	return 0;
+}
+
+static int data_release(struct inode *inode, struct file *filp)
+{
+	struct fpga_reader *reader = filp->private_data;
+
+	/* free the per-reader structure */
+	data_free_buffer(reader->buf);
+	kfree(reader);
+	filp->private_data = NULL;
+	return 0;
+}
+
+static ssize_t data_read(struct file *filp, char __user *ubuf, size_t count,
+			 loff_t *f_pos)
+{
+	struct fpga_reader *reader = filp->private_data;
+	struct fpga_device *priv = reader->priv;
+	struct list_head *used = &priv->used;
+	struct data_buf *dbuf;
+	size_t avail;
+	void *data;
+	int ret;
+
+	/* check if we already have a partial buffer */
+	if (reader->buf) {
+		dbuf = reader->buf;
+		goto have_buffer;
+	}
+
+	spin_lock_irq(&priv->lock);
+
+	/* Block until there is at least one buffer on the used list */
+	while (list_empty(used)) {
I believe we should stop and return error when device is disabled so the
condition should be:

	while (!reader->buf && list_empty() && priv->enabled)

quoted hunk ↗ jump to hunk
+		spin_unlock_irq(&priv->lock);
+
+		if (filp->f_flags & O_NONBLOCK)
+			return -EAGAIN;
+
+		ret = wait_event_interruptible(priv->wait, !list_empty(used));
		ret = wait_event_interruptible(priv->wait,
					!list_empty(used) || !priv->enabled);
quoted hunk ↗ jump to hunk
+		if (ret)
+			return ret;
+
+		spin_lock_irq(&priv->lock);
+	}
+
	if (!priv->enabled) {
		spin_unlock_irq(&priv->lock);
		return -ENXIO;
	}

	if (reader->buf) {
		dbuf = reader->buf;
	} else {
		dbuf = list_first_entry(used, struct data_buf, entry);
		list_del_init(&dbuf->entry);
	}
quoted hunk ↗ jump to hunk
+	/* Grab the first buffer off of the used list */
+	dbuf = list_first_entry(used, struct data_buf, entry);
+	list_del_init(&dbuf->entry);
+
+	spin_unlock_irq(&priv->lock);
+
+	/* Buffers are always mapped: unmap it */
+	data_unmap_buffer(priv->dev, dbuf);
+
+	/* save the buffer for later */
+	reader->buf = dbuf;
+	reader->buf_start = 0;
+
+	/* we removed a buffer from the used list: wake any waiters */
+	wake_up(&priv->wait);
I do not think anyone waits on this...
quoted hunk ↗ jump to hunk
+
+have_buffer:
+	/* Get the number of bytes available */
+	avail = dbuf->size - reader->buf_start;
+	data = dbuf->vb.vaddr + reader->buf_start;
+
+	/* Get the number of bytes we can transfer */
+	count = min(count, avail);
+
+	/* Copy the data to the userspace buffer */
+	if (copy_to_user(ubuf, data, count))
+		return -EFAULT;
+
+	/* Update the amount of available space */
+	avail -= count;
+
+	/* Lock against concurrent enable/disable */
+	ret = mutex_lock_interruptible(&priv->mutex);
+	if (ret)
+		return ret;
Mutex is not needed here, we shall rely on priv->lock. Map buffer
beforehand, take lock, check if the device is enabled and if it still is
put the buffer on free list. Release lock and exit if device was
enabled; otherwise dispose the buffer.
quoted hunk ↗ jump to hunk
+
+	/* Still some space available: save the buffer for later */
+	if (avail != 0) {
+		reader->buf_start += count;
+		reader->buf = dbuf;
+		goto out_unlock;
+	}
+
+	/*
+	 * No space is available in this buffer
+	 *
+	 * This is a complicated decision:
+	 * - if the device is not enabled: free the buffer
+	 * - if the buffer is too small: free the buffer
+	 */
+	if (!priv->enabled || dbuf->size != priv->bufsize) {
+		data_free_buffer(dbuf);
+		reader->buf = NULL;
+		goto out_unlock;
+	}
+
+	/*
+	 * The buffer is safe to recycle: remap it and finish
+	 *
+	 * If this fails, we pretend that the read never happened, and return
+	 * -EFAULT to userspace. They'll retry the read again.
+	 */
+	ret = data_map_buffer(priv->dev, dbuf);
+	if (ret) {
+		dev_err(priv->dev, "unable to remap buffer for DMA\n");
+		count = -EFAULT;
+		goto out_unlock;
+	}
+
+	/* Add the buffer back to the free list */
+	reader->buf = NULL;
+	spin_lock_irq(&priv->lock);
+	list_add_tail(&dbuf->entry, &priv->free);
+	spin_unlock_irq(&priv->lock);
+
+out_unlock:
+	mutex_unlock(&priv->mutex);
+	return count;
+}
+
+static unsigned int data_poll(struct file *filp, struct poll_table_struct *tbl)
+{
+	struct fpga_reader *reader = filp->private_data;
+	struct fpga_device *priv = reader->priv;
+	unsigned int mask = 0;
+
+	poll_wait(filp, &priv->wait, tbl);
+
+	spin_lock_irq(&priv->lock);
+
Like I mentioned, the spinlock is not useful here - all threads will get
woken up and will try to read since you are not resetting the wakeup
condition.
+	if (!list_empty(&priv->used))
+		mask |= POLLIN | POLLRDNORM;
I think you should also add:

	if (!priv->)
		mask |= POLLHUP | POLLERR;

to tell the readers that they should close their file descriptors. 

Thanks.

-- 
Dmitry
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help