[PATCH v2 1/5] iio: Add reference counting for buffers

[PATCH v2 1/5] iio: Add reference counting for buffers

[Lars-Peter Clausen]

Since the buffer is accessed by userspace we can not just free the buffers
memory once we are done with it in kernel space. There might still be open file
descriptors and userspace still might be accessing the buffer. This patch adds
support for reference counting to the IIO buffers. When a buffer is created and
initialized its initial reference count is set to 1. Instead of freeing the
memory of the buffer the buffer's _free() function will drop that reference
again. But only after the last reference to the buffer has been dropped the
buffer the buffer's memory will be freed. The IIO device will take a reference
to its primary buffer. The patch adds a small helper function for this called
iio_device_attach_buffer() which will get a reference to the buffer and assign
the buffer to the IIO device. This function must be used instead of assigning
the buffer to the device by hand. The reference is only dropped once the IIO
device is freed and we can be sure that there are no more open file handles. A
reference to a buffer will also be taken whenever the buffer is active to avoid
the buffer being freed while data is still being send to it.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>

---
Changes since v1:
	* Uninline iio_buffer_get()/iio_buffer_put()
	* Let iio_buffer_get()/iio_buffer_put() accept NULL pointers
	* Let iio_buffer_get() return the same buffer that was passed into it
---
 drivers/iio/buffer_cb.c                         | 21 +++++---
 drivers/iio/industrialio-buffer.c               | 66 +++++++++++++++++++++++--
 drivers/iio/industrialio-core.c                 |  3 ++
 drivers/iio/industrialio-triggered-buffer.c     |  7 ++-
 drivers/iio/kfifo_buf.c                         |  8 ++-
 drivers/staging/iio/accel/lis3l02dq_ring.c      |  2 +-
 drivers/staging/iio/accel/sca3000_ring.c        | 13 +++--
 drivers/staging/iio/iio_simple_dummy_buffer.c   |  2 +-
 drivers/staging/iio/impedance-analyzer/ad5933.c |  8 ++-
 drivers/staging/iio/meter/ade7758_ring.c        |  7 ++-
 include/linux/iio/buffer.h                      | 28 +++++++++++
 11 files changed, 141 insertions(+), 24 deletions(-)

diff --git a/drivers/iio/buffer_cb.c b/drivers/iio/buffer_cb.c
index 841fec1..2d9c6f8 100644
--- a/drivers/iio/buffer_cb.c
+++ b/drivers/iio/buffer_cb.c
@@ -12,17 +12,27 @@ struct iio_cb_buffer {
 	struct iio_channel *channels;
 };
 
-static int iio_buffer_cb_store_to(struct iio_buffer *buffer, const void *data)
+static struct iio_cb_buffer *buffer_to_cb_buffer(struct iio_buffer *buffer)
 {
-	struct iio_cb_buffer *cb_buff = container_of(buffer,
-						     struct iio_cb_buffer,
-						     buffer);
+	return container_of(buffer, struct iio_cb_buffer, buffer);
+}
 
+static int iio_buffer_cb_store_to(struct iio_buffer *buffer, const void *data)
+{
+	struct iio_cb_buffer *cb_buff = buffer_to_cb_buffer(buffer);
 	return cb_buff->cb(data, cb_buff->private);
 }
 
+static void iio_buffer_cb_release(struct iio_buffer *buffer)
+{
+	struct iio_cb_buffer *cb_buff = buffer_to_cb_buffer(buffer);
+	kfree(cb_buff->buffer.scan_mask);
+	kfree(cb_buff);
+}
+
 static const struct iio_buffer_access_funcs iio_cb_access = {
 	.store_to = &iio_buffer_cb_store_to,
+	.release = &iio_buffer_cb_release,
 };
 
 struct iio_cb_buffer *iio_channel_get_all_cb(struct device *dev,
@@ -104,9 +114,8 @@ EXPORT_SYMBOL_GPL(iio_channel_stop_all_cb);
 
 void iio_channel_release_all_cb(struct iio_cb_buffer *cb_buff)
 {
-	kfree(cb_buff->buffer.scan_mask);
 	iio_channel_release_all(cb_buff->channels);
-	kfree(cb_buff);
+	iio_buffer_put(&cb_buff->buffer);
 }
 EXPORT_SYMBOL_GPL(iio_channel_release_all_cb);
 
diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index e9f389b..36c39dc 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -74,6 +74,7 @@ void iio_buffer_init(struct iio_buffer *buffer)
 	INIT_LIST_HEAD(&buffer->demux_list);
 	INIT_LIST_HEAD(&buffer->buffer_list);
 	init_waitqueue_head(&buffer->pollq);
+	kref_init(&buffer->ref);
 }
 EXPORT_SYMBOL(iio_buffer_init);
 
@@ -454,6 +455,19 @@ static int iio_compute_scan_bytes(struct iio_dev *indio_dev,
 	return bytes;
 }
 
+static void iio_buffer_activate(struct iio_dev *indio_dev,
+	struct iio_buffer *buffer)
+{
+	iio_buffer_get(buffer);
+	list_add(&buffer->buffer_list, &indio_dev->buffer_list);
+}
+
+static void iio_buffer_deactivate(struct iio_buffer *buffer)
+{
+	list_del_init(&buffer->buffer_list);
+	iio_buffer_put(buffer);
+}
+
 void iio_disable_all_buffers(struct iio_dev *indio_dev)
 {
 	struct iio_buffer *buffer, *_buffer;
@@ -466,7 +480,7 @@ void iio_disable_all_buffers(struct iio_dev *indio_dev)
 
 	list_for_each_entry_safe(buffer, _buffer,
 			&indio_dev->buffer_list, buffer_list)
-		list_del_init(&buffer->buffer_list);
+		iio_buffer_deactivate(buffer);
 
 	indio_dev->currentmode = INDIO_DIRECT_MODE;
 	if (indio_dev->setup_ops->postdisable)
@@ -503,9 +517,9 @@ int iio_update_buffers(struct iio_dev *indio_dev,
 		indio_dev->active_scan_mask = NULL;
 
 	if (remove_buffer)
-		list_del_init(&remove_buffer->buffer_list);
+		iio_buffer_deactivate(remove_buffer);
 	if (insert_buffer)
-		list_add(&insert_buffer->buffer_list, &indio_dev->buffer_list);
+		iio_buffer_activate(indio_dev, insert_buffer);
 
 	/* If no buffers in list, we are done */
 	if (list_empty(&indio_dev->buffer_list)) {
@@ -540,7 +554,7 @@ int iio_update_buffers(struct iio_dev *indio_dev,
 			 * Roll back.
 			 * Note can only occur when adding a buffer.
 			 */
-			list_del_init(&insert_buffer->buffer_list);
+			iio_buffer_deactivate(insert_buffer);
 			if (old_mask) {
 				indio_dev->active_scan_mask = old_mask;
 				success = -EINVAL;
@@ -631,7 +645,7 @@ error_run_postdisable:
 error_remove_inserted:
 
 	if (insert_buffer)
-		list_del_init(&insert_buffer->buffer_list);
+		iio_buffer_deactivate(insert_buffer);
 	indio_dev->active_scan_mask = old_mask;
 	kfree(compound_mask);
 error_ret:
@@ -952,3 +966,45 @@ error_clear_mux_table:
 	return ret;
 }
 EXPORT_SYMBOL_GPL(iio_update_demux);
+
+/**
+ * iio_buffer_release() - Free a buffer's resources
+ * @ref: Pointer to the kref embedded in the iio_buffer struct
+ *
+ * This function is called when the last reference to the buffer has been
+ * dropped. It will typically free all resources allocated by the buffer. Do not
+ * call this function manually, always use iio_buffer_put() when done using a
+ * buffer.
+ */
+static void iio_buffer_release(struct kref *ref)
+{
+	struct iio_buffer *buffer = container_of(ref, struct iio_buffer, ref);
+
+	buffer->access->release(buffer);
+}
+
+/**
+ * iio_buffer_get() - Grab a reference to the buffer
+ * @buffer: The buffer to grab a reference for, may be NULL
+ *
+ * Returns the pointer to the buffer that was passed into the function.
+ */
+struct iio_buffer *iio_buffer_get(struct iio_buffer *buffer)
+{
+	if (buffer)
+		kref_get(&buffer->ref);
+
+	return buffer;
+}
+EXPORT_SYMBOL_GPL(iio_buffer_get);
+
+/**
+ * iio_buffer_put() - Release the reference to the buffer
+ * @buffer: The buffer to release the reference for, may be NULL
+ */
+void iio_buffer_put(struct iio_buffer *buffer)
+{
+	if (buffer)
+		kref_put(&buffer->ref, iio_buffer_release);
+}
+EXPORT_SYMBOL_GPL(iio_buffer_put);
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 863aa01..0dfaf9e 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -28,6 +28,7 @@
 #include "iio_core_trigger.h"
 #include <linux/iio/sysfs.h>
 #include <linux/iio/events.h>
+#include <linux/iio/buffer.h>
 
 /* IDA to assign each registered device a unique id */
 static DEFINE_IDA(iio_ida);
@@ -896,6 +897,8 @@ static void iio_dev_release(struct device *device)
 	iio_device_unregister_sysfs(indio_dev);
 	iio_device_unregister_debugfs(indio_dev);
 
+	iio_buffer_put(indio_dev->buffer);
+
 	ida_simple_remove(&iio_ida, indio_dev->id);
 	kfree(indio_dev);
 }
diff --git a/drivers/iio/industrialio-triggered-buffer.c b/drivers/iio/industrialio-triggered-buffer.c
index 46c619b..c1cb1f9 100644
--- a/drivers/iio/industrialio-triggered-buffer.c
+++ b/drivers/iio/industrialio-triggered-buffer.c
@@ -47,14 +47,17 @@ int iio_triggered_buffer_setup(struct iio_dev *indio_dev,
 	irqreturn_t (*pollfunc_th)(int irq, void *p),
 	const struct iio_buffer_setup_ops *setup_ops)
 {
+	struct iio_buffer *buffer;
 	int ret;
 
-	indio_dev->buffer = iio_kfifo_allocate(indio_dev);
-	if (!indio_dev->buffer) {
+	buffer = iio_kfifo_allocate(indio_dev);
+	if (!buffer) {
 		ret = -ENOMEM;
 		goto error_ret;
 	}
 
+	iio_device_attach_buffer(indio_dev, buffer);
+
 	indio_dev->pollfunc = iio_alloc_pollfunc(pollfunc_bh,
 						 pollfunc_th,
 						 IRQF_ONESHOT,
diff --git a/drivers/iio/kfifo_buf.c b/drivers/iio/kfifo_buf.c
index 538d461..b4ac55a 100644
--- a/drivers/iio/kfifo_buf.c
+++ b/drivers/iio/kfifo_buf.c
@@ -130,6 +130,11 @@ static int iio_read_first_n_kfifo(struct iio_buffer *r,
 	return copied;
 }
 
+static void iio_kfifo_buffer_release(struct iio_buffer *buffer)
+{
+	kfree(iio_to_kfifo(buffer));
+}
+
 static const struct iio_buffer_access_funcs kfifo_access_funcs = {
 	.store_to = &iio_store_to_kfifo,
 	.read_first_n = &iio_read_first_n_kfifo,
@@ -138,6 +143,7 @@ static const struct iio_buffer_access_funcs kfifo_access_funcs = {
 	.set_bytes_per_datum = &iio_set_bytes_per_datum_kfifo,
 	.get_length = &iio_get_length_kfifo,
 	.set_length = &iio_set_length_kfifo,
+	.release = &iio_kfifo_buffer_release,
 };
 
 struct iio_buffer *iio_kfifo_allocate(struct iio_dev *indio_dev)
@@ -158,7 +164,7 @@ EXPORT_SYMBOL(iio_kfifo_allocate);
 
 void iio_kfifo_free(struct iio_buffer *r)
 {
-	kfree(iio_to_kfifo(r));
+	iio_buffer_put(r);
 }
 EXPORT_SYMBOL(iio_kfifo_free);
 
diff --git a/drivers/staging/iio/accel/lis3l02dq_ring.c b/drivers/staging/iio/accel/lis3l02dq_ring.c
index aae86dd..99b233f 100644
--- a/drivers/staging/iio/accel/lis3l02dq_ring.c
+++ b/drivers/staging/iio/accel/lis3l02dq_ring.c
@@ -397,7 +397,7 @@ int lis3l02dq_configure_buffer(struct iio_dev *indio_dev)
 	if (!buffer)
 		return -ENOMEM;
 
-	indio_dev->buffer = buffer;
+	iio_device_attach_buffer(indio_dev, buffer);
 
 	buffer->scan_timestamp = true;
 	indio_dev->setup_ops = &lis3l02dq_buffer_setup_ops;
diff --git a/drivers/staging/iio/accel/sca3000_ring.c b/drivers/staging/iio/accel/sca3000_ring.c
index 0f2ee33..4a27beb 100644
--- a/drivers/staging/iio/accel/sca3000_ring.c
+++ b/drivers/staging/iio/accel/sca3000_ring.c
@@ -265,7 +265,7 @@ static struct iio_buffer *sca3000_rb_allocate(struct iio_dev *indio_dev)
 	return buf;
 }
 
-static inline void sca3000_rb_free(struct iio_buffer *r)
+static void sca3000_ring_release(struct iio_buffer *r)
 {
 	kfree(iio_to_hw_buf(r));
 }
@@ -274,23 +274,28 @@ static const struct iio_buffer_access_funcs sca3000_ring_access_funcs = {
 	.read_first_n = &sca3000_read_first_n_hw_rb,
 	.get_length = &sca3000_ring_get_length,
 	.get_bytes_per_datum = &sca3000_ring_get_bytes_per_datum,
+	.release = sca3000_ring_release,
 };
 
 int sca3000_configure_ring(struct iio_dev *indio_dev)
 {
-	indio_dev->buffer = sca3000_rb_allocate(indio_dev);
-	if (indio_dev->buffer == NULL)
+	struct iio_buffer *buffer;
+
+	buffer = sca3000_rb_allocate(indio_dev);
+	if (buffer == NULL)
 		return -ENOMEM;
 	indio_dev->modes |= INDIO_BUFFER_HARDWARE;
 
 	indio_dev->buffer->access = &sca3000_ring_access_funcs;
 
+	iio_device_attach_buffer(indio_dev, buffer);
+
 	return 0;
 }
 
 void sca3000_unconfigure_ring(struct iio_dev *indio_dev)
 {
-	sca3000_rb_free(indio_dev->buffer);
+	iio_buffer_put(indio_dev->buffer);
 }
 
 static inline
diff --git a/drivers/staging/iio/iio_simple_dummy_buffer.c b/drivers/staging/iio/iio_simple_dummy_buffer.c
index 09c93ac..2612e87 100644
--- a/drivers/staging/iio/iio_simple_dummy_buffer.c
+++ b/drivers/staging/iio/iio_simple_dummy_buffer.c
@@ -135,7 +135,7 @@ int iio_simple_dummy_configure_buffer(struct iio_dev *indio_dev,
 		goto error_ret;
 	}
 
-	indio_dev->buffer = buffer;
+	iio_device_attach_buffer(indio_dev, buffer);
 
 	/* Enable timestamps by default */
 	buffer->scan_timestamp = true;
diff --git a/drivers/staging/iio/impedance-analyzer/ad5933.c b/drivers/staging/iio/impedance-analyzer/ad5933.c
index 712f3c2..f570115 100644
--- a/drivers/staging/iio/impedance-analyzer/ad5933.c
+++ b/drivers/staging/iio/impedance-analyzer/ad5933.c
@@ -630,10 +630,14 @@ static const struct iio_buffer_setup_ops ad5933_ring_setup_ops = {
 
 static int ad5933_register_ring_funcs_and_init(struct iio_dev *indio_dev)
 {
-	indio_dev->buffer = iio_kfifo_allocate(indio_dev);
-	if (!indio_dev->buffer)
+	struct iio_buffer *buffer;
+
+	buffer = iio_kfifo_allocate(indio_dev);
+	if (buffer)
 		return -ENOMEM;
 
+	iio_device_attach_buffer(indio_dev, buffer);
+
 	/* Ring buffer functions - here trigger setup related */
 	indio_dev->setup_ops = &ad5933_ring_setup_ops;
 
diff --git a/drivers/staging/iio/meter/ade7758_ring.c b/drivers/staging/iio/meter/ade7758_ring.c
index 4080995..7a448ff 100644
--- a/drivers/staging/iio/meter/ade7758_ring.c
+++ b/drivers/staging/iio/meter/ade7758_ring.c
@@ -121,14 +121,17 @@ void ade7758_unconfigure_ring(struct iio_dev *indio_dev)
 int ade7758_configure_ring(struct iio_dev *indio_dev)
 {
 	struct ade7758_state *st = iio_priv(indio_dev);
+	struct iio_buffer *buffer;
 	int ret = 0;
 
-	indio_dev->buffer = iio_kfifo_allocate(indio_dev);
-	if (!indio_dev->buffer) {
+	buffer = iio_kfifo_allocate(indio_dev);
+	if (!buffer) {
 		ret = -ENOMEM;
 		return ret;
 	}
 
+	iio_device_attach_buffer(indio_dev, buffer);
+
 	indio_dev->setup_ops = &ade7758_ring_setup_ops;
 
 	indio_dev->pollfunc = iio_alloc_pollfunc(&iio_pollfunc_store_time,
diff --git a/include/linux/iio/buffer.h b/include/linux/iio/buffer.h
index a1124bd..6e428d9 100644
--- a/include/linux/iio/buffer.h
+++ b/include/linux/iio/buffer.h
@@ -11,6 +11,7 @@
 #define _IIO_BUFFER_GENERIC_H_
 #include <linux/sysfs.h>
 #include <linux/iio/iio.h>
+#include <linux/kref.h>
 
 #ifdef CONFIG_IIO_BUFFER
 
@@ -26,6 +27,8 @@ struct iio_buffer;
  * @set_bytes_per_datum:set number of bytes per datum
  * @get_length:		get number of datums in buffer
  * @set_length:		set number of datums in buffer
+ * @release:		called when the last reference to the buffer is dropped,
+ *			should free all resources allocated by the buffer.
  *
  * The purpose of this structure is to make the buffer element
  * modular as event for a given driver, different usecases may require
@@ -47,6 +50,8 @@ struct iio_buffer_access_funcs {
 	int (*set_bytes_per_datum)(struct iio_buffer *buffer, size_t bpd);
 	int (*get_length)(struct iio_buffer *buffer);
 	int (*set_length)(struct iio_buffer *buffer, int length);
+
+	void (*release)(struct iio_buffer *buffer);
 };
 
 /**
@@ -67,6 +72,7 @@ struct iio_buffer_access_funcs {
  * @demux_list:		[INTERN] list of operations required to demux the scan.
  * @demux_bounce:	[INTERN] buffer for doing gather from incoming scan.
  * @buffer_list:	[INTERN] entry in the devices list of current buffers.
+ * @ref:		[INTERN] reference count of the buffer.
  */
 struct iio_buffer {
 	int					length;
@@ -83,6 +89,7 @@ struct iio_buffer {
 	struct list_head			demux_list;
 	void					*demux_bounce;
 	struct list_head			buffer_list;
+	struct kref				ref;
 };
 
 /**
@@ -204,6 +211,24 @@ int iio_sw_buffer_preenable(struct iio_dev *indio_dev);
 bool iio_validate_scan_mask_onehot(struct iio_dev *indio_dev,
 	const unsigned long *mask);
 
+struct iio_buffer *iio_buffer_get(struct iio_buffer *buffer);
+void iio_buffer_put(struct iio_buffer *buffer);
+
+/**
+ * iio_device_attach_buffer - Attach a buffer to a IIO device
+ * @indio_dev: The device the buffer should be attached to
+ * @buffer: The buffer to attach to the device
+ *
+ * This function attaches a buffer to a IIO device. The buffer stays attached to
+ * the device until the device is freed. The function should only be called at
+ * most once per device.
+ */
+static inline void iio_device_attach_buffer(struct iio_dev *indio_dev,
+	struct iio_buffer *buffer)
+{
+	indio_dev->buffer = iio_buffer_get(buffer);
+}
+
 #else /* CONFIG_IIO_BUFFER */
 
 static inline int iio_buffer_register(struct iio_dev *indio_dev,
@@ -216,6 +241,9 @@ static inline int iio_buffer_register(struct iio_dev *indio_dev,
 static inline void iio_buffer_unregister(struct iio_dev *indio_dev)
 {}
 
+static inline void iio_buffer_get(struct iio_buffer *buffer) {}
+static inline void iio_buffer_put(struct iio_buffer *buffer) {}
+
 #endif /* CONFIG_IIO_BUFFER */
 
 #endif /* _IIO_BUFFER_GENERIC_H_ */
-- 
1.8.0

[PATCH v2 2/5] iio: Return -ENODEV for file operations if the device has been unregistered

[Lars-Peter Clausen]

If the IIO device has been unregistered return -ENODEV for any further file
operations like read() and ioctl(). This avoids userspace being able to grab new
references to the device.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
No changes since v1
---
 drivers/iio/industrialio-buffer.c | 6 ++++++
 drivers/iio/industrialio-core.c   | 3 +++
 drivers/iio/industrialio-event.c  | 6 ++++++
 3 files changed, 15 insertions(+)

diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index 36c39dc..6c7a9c5 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -48,6 +48,9 @@ ssize_t iio_buffer_read_first_n_outer(struct file *filp, char __user *buf,
 	struct iio_dev *indio_dev = filp->private_data;
 	struct iio_buffer *rb = indio_dev->buffer;
 
+	if (!indio_dev->info)
+		return -ENODEV;
+
 	if (!rb || !rb->access->read_first_n)
 		return -EINVAL;
 	return rb->access->read_first_n(rb, n, buf);
@@ -62,6 +65,9 @@ unsigned int iio_buffer_poll(struct file *filp,
 	struct iio_dev *indio_dev = filp->private_data;
 	struct iio_buffer *rb = indio_dev->buffer;
 
+	if (!indio_dev->info)
+		return -ENODEV;
+
 	poll_wait(filp, &rb->pollq, wait);
 	if (rb->stufftoread)
 		return POLLIN | POLLRDNORM;
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 0dfaf9e..c9c6558 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -1041,6 +1041,9 @@ static long iio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 	int __user *ip = (int __user *)arg;
 	int fd;
 
+	if (!indio_dev->info)
+		return -ENODEV;
+
 	if (cmd == IIO_GET_EVENT_FD_IOCTL) {
 		fd = iio_event_getfd(indio_dev);
 		if (copy_to_user(ip, &fd, sizeof(fd)))
diff --git a/drivers/iio/industrialio-event.c b/drivers/iio/industrialio-event.c
index 36f0c8e..837d450 100644
--- a/drivers/iio/industrialio-event.c
+++ b/drivers/iio/industrialio-event.c
@@ -76,6 +76,9 @@ static unsigned int iio_event_poll(struct file *filep,
 	struct iio_event_interface *ev_int = indio_dev->event_interface;
 	unsigned int events = 0;
 
+	if (!indio_dev->info)
+		return -ENODEV;
+
 	poll_wait(filep, &ev_int->wait, wait);
 
 	spin_lock_irq(&ev_int->wait.lock);
@@ -96,6 +99,9 @@ static ssize_t iio_event_chrdev_read(struct file *filep,
 	unsigned int copied;
 	int ret;
 
+	if (!indio_dev->info)
+		return -ENODEV;
+
 	if (count < sizeof(struct iio_event_data))
 		return -EINVAL;
 
-- 
1.8.0

[PATCH v2 3/5] iio: Wakeup poll and blocking reads when the device is unregistered

[Lars-Peter Clausen]

Once the device has been unregistered there won't be any new data no matter how
long a userspace application waits, so we might as well wake them up and let
them know.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
No changes since v1
---
 drivers/iio/iio_core.h            |  3 +++
 drivers/iio/industrialio-buffer.c | 16 ++++++++++++++++
 drivers/iio/industrialio-core.c   |  4 ++++
 drivers/iio/industrialio-event.c  | 21 ++++++++++++++++++++-
 4 files changed, 43 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h
index 9209f47..7512cf7 100644
--- a/drivers/iio/iio_core.h
+++ b/drivers/iio/iio_core.h
@@ -50,6 +50,7 @@ ssize_t iio_buffer_read_first_n_outer(struct file *filp, char __user *buf,
 #define iio_buffer_read_first_n_outer_addr (&iio_buffer_read_first_n_outer)
 
 void iio_disable_all_buffers(struct iio_dev *indio_dev);
+void iio_buffer_wakeup_poll(struct iio_dev *indio_dev);
 
 #else
 
@@ -57,11 +58,13 @@ void iio_disable_all_buffers(struct iio_dev *indio_dev);
 #define iio_buffer_read_first_n_outer_addr NULL
 
 static inline void iio_disable_all_buffers(struct iio_dev *indio_dev) {}
+static inline void iio_buffer_wakeup_poll(struct iio_dev *indio_dev) {}
 
 #endif
 
 int iio_device_register_eventset(struct iio_dev *indio_dev);
 void iio_device_unregister_eventset(struct iio_dev *indio_dev);
+void iio_device_wakeup_eventset(struct iio_dev *indio_dev);
 int iio_event_getfd(struct iio_dev *indio_dev);
 
 #endif
diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index 6c7a9c5..5a46c57 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -20,6 +20,7 @@
 #include <linux/cdev.h>
 #include <linux/slab.h>
 #include <linux/poll.h>
+#include <linux/sched.h>
 
 #include <linux/iio/iio.h>
 #include "iio_core.h"
@@ -75,6 +76,21 @@ unsigned int iio_buffer_poll(struct file *filp,
 	return 0;
 }
 
+/**
+ * iio_buffer_wakeup_poll - Wakes up the buffer waitqueue
+ * @indio_dev: The IIO device
+ *
+ * Wakes up the event waitqueue used for poll(). Should usually
+ * be called when the device is unregistered.
+ */
+void iio_buffer_wakeup_poll(struct iio_dev *indio_dev)
+{
+	if (!indio_dev->buffer)
+		return;
+
+	wake_up(&indio_dev->buffer->pollq);
+}
+
 void iio_buffer_init(struct iio_buffer *buffer)
 {
 	INIT_LIST_HEAD(&buffer->demux_list);
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index c9c6558..81650e1 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -1139,6 +1139,10 @@ void iio_device_unregister(struct iio_dev *indio_dev)
 	iio_disable_all_buffers(indio_dev);
 
 	indio_dev->info = NULL;
+
+	iio_device_wakeup_eventset(indio_dev);
+	iio_buffer_wakeup_poll(indio_dev);
+
 	mutex_unlock(&indio_dev->info_exist_lock);
 }
 EXPORT_SYMBOL(iio_device_unregister);
diff --git a/drivers/iio/industrialio-event.c b/drivers/iio/industrialio-event.c
index 837d450..d251f30 100644
--- a/drivers/iio/industrialio-event.c
+++ b/drivers/iio/industrialio-event.c
@@ -113,9 +113,14 @@ static ssize_t iio_event_chrdev_read(struct file *filep,
 		}
 		/* Blocking on device; waiting for something to be there */
 		ret = wait_event_interruptible_locked_irq(ev_int->wait,
-					!kfifo_is_empty(&ev_int->det_events));
+					!kfifo_is_empty(&ev_int->det_events) ||
+					indio_dev->info == NULL);
 		if (ret)
 			goto error_unlock;
+		if (indio_dev->info == NULL) {
+			ret = -ENODEV;
+			goto error_unlock;
+		}
 		/* Single access device so no one else can get the data */
 	}
 
@@ -454,6 +459,20 @@ error_ret:
 	return ret;
 }
 
+/**
+ * iio_device_wakeup_eventset - Wakes up the event waitqueue
+ * @indio_dev: The IIO device
+ *
+ * Wakes up the event waitqueue used for poll() and blocking read().
+ * Should usually be called when the device is unregistered.
+ */
+void iio_device_wakeup_eventset(struct iio_dev *indio_dev)
+{
+	if (indio_dev->event_interface == NULL)
+		return;
+	wake_up(&indio_dev->event_interface->wait);
+}
+
 void iio_device_unregister_eventset(struct iio_dev *indio_dev)
 {
 	if (indio_dev->event_interface == NULL)
-- 
1.8.0

[PATCH v2 4/5] iio:buffer: Add proper locking for iio_update_buffers()

[Lars-Peter Clausen]

We need to make sure that in-kernel users of iio_update_buffers() do not race
against each other or against unregistration of the device. So we need to take
both the mlock and the info_exist_lock when calling iio_update_buffers() from a
in-kernel consumer.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
No changes since v1
---
 drivers/iio/industrialio-buffer.c | 29 ++++++++++++++++++++++++++---
 1 file changed, 26 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index 5a46c57..d6a5455 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -509,7 +509,7 @@ void iio_disable_all_buffers(struct iio_dev *indio_dev)
 		indio_dev->setup_ops->postdisable(indio_dev);
 }
 
-int iio_update_buffers(struct iio_dev *indio_dev,
+static int __iio_update_buffers(struct iio_dev *indio_dev,
 		       struct iio_buffer *insert_buffer,
 		       struct iio_buffer *remove_buffer)
 {
@@ -674,6 +674,29 @@ error_ret:
 
 	return ret;
 }
+
+int iio_update_buffers(struct iio_dev *indio_dev,
+		       struct iio_buffer *insert_buffer,
+		       struct iio_buffer *remove_buffer)
+{
+	int ret;
+
+	mutex_lock(&indio_dev->info_exist_lock);
+	mutex_lock(&indio_dev->mlock);
+
+	if (indio_dev->info == NULL) {
+		ret = -ENODEV;
+		goto out_unlock;
+	}
+
+	ret = __iio_update_buffers(indio_dev, insert_buffer, remove_buffer);
+
+out_unlock:
+	mutex_unlock(&indio_dev->mlock);
+	mutex_unlock(&indio_dev->info_exist_lock);
+
+	return ret;
+}
 EXPORT_SYMBOL_GPL(iio_update_buffers);
 
 ssize_t iio_buffer_store_enable(struct device *dev,
@@ -699,10 +722,10 @@ ssize_t iio_buffer_store_enable(struct device *dev,
 		goto done;
 
 	if (requested_state)
-		ret = iio_update_buffers(indio_dev,
+		ret = __iio_update_buffers(indio_dev,
 					 indio_dev->buffer, NULL);
 	else
-		ret = iio_update_buffers(indio_dev,
+		ret = __iio_update_buffers(indio_dev,
 					 NULL, indio_dev->buffer);
 
 	if (ret < 0)
-- 
1.8.0

[PATCH v2 5/5] iio:buffer: Ignore noop requests for iio_update_buffers()

[Lars-Peter Clausen]

Since the kernel now disables all buffers when a device is unregistered it might
happen that a in-kernel consumer tries to disable that buffer again. So ignore
requests where the buffer already is in the desired state.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
No changes since v1
---
 drivers/iio/industrialio-buffer.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index d6a5455..fd3f3af 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -681,9 +681,23 @@ int iio_update_buffers(struct iio_dev *indio_dev,
 {
 	int ret;
 
+	if (insert_buffer == remove_buffer)
+		return 0;
+
 	mutex_lock(&indio_dev->info_exist_lock);
 	mutex_lock(&indio_dev->mlock);
 
+	if (insert_buffer && iio_buffer_is_active(insert_buffer))
+		insert_buffer = NULL;
+
+	if (remove_buffer && !iio_buffer_is_active(remove_buffer))
+		remove_buffer = NULL;
+
+	if (!insert_buffer && !remove_buffer) {
+		ret = 0;
+		goto out_unlock;
+	}
+
 	if (indio_dev->info == NULL) {
 		ret = -ENODEV;
 		goto out_unlock;
-- 
1.8.0

Re: [PATCH v2 5/5] iio:buffer: Ignore noop requests for iio_update_buffers()

[Lars-Peter Clausen]

On 10/12/2013 01:10 PM, Jonathan Cameron wrote:
> On 10/04/13 12:07, Lars-Peter Clausen wrote:
>> Since the kernel now disables all buffers when a device is unregistered it might
>> happen that a in-kernel consumer tries to disable that buffer again. So ignore
>> requests where the buffer already is in the desired state.
>>
>> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
>> ---
>> No changes since v1
>> ---
>>  drivers/iio/industrialio-buffer.c | 14 ++++++++++++++
>>  1 file changed, 14 insertions(+)
>>
>> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
>> index d6a5455..fd3f3af 100644
>> --- a/drivers/iio/industrialio-buffer.c
>> +++ b/drivers/iio/industrialio-buffer.c
>> @@ -681,9 +681,23 @@ int iio_update_buffers(struct iio_dev *indio_dev,
>>  {
>>  	int ret;
>>  
>> +	if (insert_buffer == remove_buffer)
>> +		return 0;
>> +
>>  	mutex_lock(&indio_dev->info_exist_lock);
>>  	mutex_lock(&indio_dev->mlock);
>>  
>> +	if (insert_buffer && iio_buffer_is_active(insert_buffer))
>> +		insert_buffer = NULL;
>> +
>> +	if (remove_buffer && !iio_buffer_is_active(remove_buffer))
>> +		remove_buffer = NULL;
>> +
> So this condition will occur iff insert_buffer = 0 and remove buffer = 0?
> 
> If so, then insert_buffer == remove_buffer and you'll have already returned 0 above??

This is to catch the case where we've set one (or both) of them to NULL
because it was already in the requested state.

> 
> Entirely possible I'm needing more coffee this morning...
>> +	if (!insert_buffer && !remove_buffer) {
>> +		ret = 0;
>> +		goto out_unlock;
>> +	}
>> +
>>  	if (indio_dev->info == NULL) {
>>  		ret = -ENODEV;
>>  		goto out_unlock;
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v2 1/5] iio: Add reference counting for buffers

[Jonathan Cameron]

On 10/04/13 12:06, Lars-Peter Clausen wrote:
> Since the buffer is accessed by userspace we can not just free the buffers
> memory once we are done with it in kernel space. There might still be open file
> descriptors and userspace still might be accessing the buffer. This patch adds
> support for reference counting to the IIO buffers. When a buffer is created and
> initialized its initial reference count is set to 1. Instead of freeing the
> memory of the buffer the buffer's _free() function will drop that reference
> again. But only after the last reference to the buffer has been dropped the
> buffer the buffer's memory will be freed. The IIO device will take a reference
> to its primary buffer. The patch adds a small helper function for this called
> iio_device_attach_buffer() which will get a reference to the buffer and assign
> the buffer to the IIO device. This function must be used instead of assigning
> the buffer to the device by hand. The reference is only dropped once the IIO
> device is freed and we can be sure that there are no more open file handles. A
> reference to a buffer will also be taken whenever the buffer is active to avoid
> the buffer being freed while data is still being send to it.
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
Lars, you have forgotten to cc GregKH given he raised the issues you have fixed.
I've added him now.  As far as I can see you have addressed all the points
raised and the code is a little bit nicer as a result (and more consistent!)

Applied to the togreg branch of iio.git.

Thanks for fixing this little mess that I made ;)

Jonathan
> 
> ---
> Changes since v1:
> 	* Uninline iio_buffer_get()/iio_buffer_put()
> 	* Let iio_buffer_get()/iio_buffer_put() accept NULL pointers
> 	* Let iio_buffer_get() return the same buffer that was passed into it
> ---
>  drivers/iio/buffer_cb.c                         | 21 +++++---
>  drivers/iio/industrialio-buffer.c               | 66 +++++++++++++++++++++++--
>  drivers/iio/industrialio-core.c                 |  3 ++
>  drivers/iio/industrialio-triggered-buffer.c     |  7 ++-
>  drivers/iio/kfifo_buf.c                         |  8 ++-
>  drivers/staging/iio/accel/lis3l02dq_ring.c      |  2 +-
>  drivers/staging/iio/accel/sca3000_ring.c        | 13 +++--
>  drivers/staging/iio/iio_simple_dummy_buffer.c   |  2 +-
>  drivers/staging/iio/impedance-analyzer/ad5933.c |  8 ++-
>  drivers/staging/iio/meter/ade7758_ring.c        |  7 ++-
>  include/linux/iio/buffer.h                      | 28 +++++++++++
>  11 files changed, 141 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/iio/buffer_cb.c b/drivers/iio/buffer_cb.c
> index 841fec1..2d9c6f8 100644
> --- a/drivers/iio/buffer_cb.c
> +++ b/drivers/iio/buffer_cb.c
> @@ -12,17 +12,27 @@ struct iio_cb_buffer {
>  	struct iio_channel *channels;
>  };
>  
> -static int iio_buffer_cb_store_to(struct iio_buffer *buffer, const void *data)
> +static struct iio_cb_buffer *buffer_to_cb_buffer(struct iio_buffer *buffer)
>  {
> -	struct iio_cb_buffer *cb_buff = container_of(buffer,
> -						     struct iio_cb_buffer,
> -						     buffer);
> +	return container_of(buffer, struct iio_cb_buffer, buffer);
> +}
>  
> +static int iio_buffer_cb_store_to(struct iio_buffer *buffer, const void *data)
> +{
> +	struct iio_cb_buffer *cb_buff = buffer_to_cb_buffer(buffer);
>  	return cb_buff->cb(data, cb_buff->private);
>  }
>  
> +static void iio_buffer_cb_release(struct iio_buffer *buffer)
> +{
> +	struct iio_cb_buffer *cb_buff = buffer_to_cb_buffer(buffer);
> +	kfree(cb_buff->buffer.scan_mask);
> +	kfree(cb_buff);
> +}
> +
>  static const struct iio_buffer_access_funcs iio_cb_access = {
>  	.store_to = &iio_buffer_cb_store_to,
> +	.release = &iio_buffer_cb_release,
>  };
>  
>  struct iio_cb_buffer *iio_channel_get_all_cb(struct device *dev,
> @@ -104,9 +114,8 @@ EXPORT_SYMBOL_GPL(iio_channel_stop_all_cb);
>  
>  void iio_channel_release_all_cb(struct iio_cb_buffer *cb_buff)
>  {
> -	kfree(cb_buff->buffer.scan_mask);
>  	iio_channel_release_all(cb_buff->channels);
> -	kfree(cb_buff);
> +	iio_buffer_put(&cb_buff->buffer);
>  }
>  EXPORT_SYMBOL_GPL(iio_channel_release_all_cb);
>  
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index e9f389b..36c39dc 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -74,6 +74,7 @@ void iio_buffer_init(struct iio_buffer *buffer)
>  	INIT_LIST_HEAD(&buffer->demux_list);
>  	INIT_LIST_HEAD(&buffer->buffer_list);
>  	init_waitqueue_head(&buffer->pollq);
> +	kref_init(&buffer->ref);
>  }
>  EXPORT_SYMBOL(iio_buffer_init);
>  
> @@ -454,6 +455,19 @@ static int iio_compute_scan_bytes(struct iio_dev *indio_dev,
>  	return bytes;
>  }
>  
> +static void iio_buffer_activate(struct iio_dev *indio_dev,
> +	struct iio_buffer *buffer)
> +{
> +	iio_buffer_get(buffer);
> +	list_add(&buffer->buffer_list, &indio_dev->buffer_list);
> +}
> +
> +static void iio_buffer_deactivate(struct iio_buffer *buffer)
> +{
> +	list_del_init(&buffer->buffer_list);
> +	iio_buffer_put(buffer);
> +}
> +
>  void iio_disable_all_buffers(struct iio_dev *indio_dev)
>  {
>  	struct iio_buffer *buffer, *_buffer;
> @@ -466,7 +480,7 @@ void iio_disable_all_buffers(struct iio_dev *indio_dev)
>  
>  	list_for_each_entry_safe(buffer, _buffer,
>  			&indio_dev->buffer_list, buffer_list)
> -		list_del_init(&buffer->buffer_list);
> +		iio_buffer_deactivate(buffer);
>  
>  	indio_dev->currentmode = INDIO_DIRECT_MODE;
>  	if (indio_dev->setup_ops->postdisable)
> @@ -503,9 +517,9 @@ int iio_update_buffers(struct iio_dev *indio_dev,
>  		indio_dev->active_scan_mask = NULL;
>  
>  	if (remove_buffer)
> -		list_del_init(&remove_buffer->buffer_list);
> +		iio_buffer_deactivate(remove_buffer);
>  	if (insert_buffer)
> -		list_add(&insert_buffer->buffer_list, &indio_dev->buffer_list);
> +		iio_buffer_activate(indio_dev, insert_buffer);
>  
>  	/* If no buffers in list, we are done */
>  	if (list_empty(&indio_dev->buffer_list)) {
> @@ -540,7 +554,7 @@ int iio_update_buffers(struct iio_dev *indio_dev,
>  			 * Roll back.
>  			 * Note can only occur when adding a buffer.
>  			 */
> -			list_del_init(&insert_buffer->buffer_list);
> +			iio_buffer_deactivate(insert_buffer);
>  			if (old_mask) {
>  				indio_dev->active_scan_mask = old_mask;
>  				success = -EINVAL;
> @@ -631,7 +645,7 @@ error_run_postdisable:
>  error_remove_inserted:
>  
>  	if (insert_buffer)
> -		list_del_init(&insert_buffer->buffer_list);
> +		iio_buffer_deactivate(insert_buffer);
>  	indio_dev->active_scan_mask = old_mask;
>  	kfree(compound_mask);
>  error_ret:
> @@ -952,3 +966,45 @@ error_clear_mux_table:
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(iio_update_demux);
> +
> +/**
> + * iio_buffer_release() - Free a buffer's resources
> + * @ref: Pointer to the kref embedded in the iio_buffer struct
> + *
> + * This function is called when the last reference to the buffer has been
> + * dropped. It will typically free all resources allocated by the buffer. Do not
> + * call this function manually, always use iio_buffer_put() when done using a
> + * buffer.
> + */
> +static void iio_buffer_release(struct kref *ref)
> +{
> +	struct iio_buffer *buffer = container_of(ref, struct iio_buffer, ref);
> +
> +	buffer->access->release(buffer);
> +}
> +
> +/**
> + * iio_buffer_get() - Grab a reference to the buffer
> + * @buffer: The buffer to grab a reference for, may be NULL
> + *
> + * Returns the pointer to the buffer that was passed into the function.
> + */
> +struct iio_buffer *iio_buffer_get(struct iio_buffer *buffer)
> +{
> +	if (buffer)
> +		kref_get(&buffer->ref);
> +
> +	return buffer;
> +}
> +EXPORT_SYMBOL_GPL(iio_buffer_get);
> +
> +/**
> + * iio_buffer_put() - Release the reference to the buffer
> + * @buffer: The buffer to release the reference for, may be NULL
> + */
> +void iio_buffer_put(struct iio_buffer *buffer)
> +{
> +	if (buffer)
> +		kref_put(&buffer->ref, iio_buffer_release);
> +}
> +EXPORT_SYMBOL_GPL(iio_buffer_put);
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index 863aa01..0dfaf9e 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -28,6 +28,7 @@
>  #include "iio_core_trigger.h"
>  #include <linux/iio/sysfs.h>
>  #include <linux/iio/events.h>
> +#include <linux/iio/buffer.h>
>  
>  /* IDA to assign each registered device a unique id */
>  static DEFINE_IDA(iio_ida);
> @@ -896,6 +897,8 @@ static void iio_dev_release(struct device *device)
>  	iio_device_unregister_sysfs(indio_dev);
>  	iio_device_unregister_debugfs(indio_dev);
>  
> +	iio_buffer_put(indio_dev->buffer);
> +
>  	ida_simple_remove(&iio_ida, indio_dev->id);
>  	kfree(indio_dev);
>  }
> diff --git a/drivers/iio/industrialio-triggered-buffer.c b/drivers/iio/industrialio-triggered-buffer.c
> index 46c619b..c1cb1f9 100644
> --- a/drivers/iio/industrialio-triggered-buffer.c
> +++ b/drivers/iio/industrialio-triggered-buffer.c
> @@ -47,14 +47,17 @@ int iio_triggered_buffer_setup(struct iio_dev *indio_dev,
>  	irqreturn_t (*pollfunc_th)(int irq, void *p),
>  	const struct iio_buffer_setup_ops *setup_ops)
>  {
> +	struct iio_buffer *buffer;
>  	int ret;
>  
> -	indio_dev->buffer = iio_kfifo_allocate(indio_dev);
> -	if (!indio_dev->buffer) {
> +	buffer = iio_kfifo_allocate(indio_dev);
> +	if (!buffer) {
>  		ret = -ENOMEM;
>  		goto error_ret;
>  	}
>  
> +	iio_device_attach_buffer(indio_dev, buffer);
> +
>  	indio_dev->pollfunc = iio_alloc_pollfunc(pollfunc_bh,
>  						 pollfunc_th,
>  						 IRQF_ONESHOT,
> diff --git a/drivers/iio/kfifo_buf.c b/drivers/iio/kfifo_buf.c
> index 538d461..b4ac55a 100644
> --- a/drivers/iio/kfifo_buf.c
> +++ b/drivers/iio/kfifo_buf.c
> @@ -130,6 +130,11 @@ static int iio_read_first_n_kfifo(struct iio_buffer *r,
>  	return copied;
>  }
>  
> +static void iio_kfifo_buffer_release(struct iio_buffer *buffer)
> +{
> +	kfree(iio_to_kfifo(buffer));
> +}
> +
>  static const struct iio_buffer_access_funcs kfifo_access_funcs = {
>  	.store_to = &iio_store_to_kfifo,
>  	.read_first_n = &iio_read_first_n_kfifo,
> @@ -138,6 +143,7 @@ static const struct iio_buffer_access_funcs kfifo_access_funcs = {
>  	.set_bytes_per_datum = &iio_set_bytes_per_datum_kfifo,
>  	.get_length = &iio_get_length_kfifo,
>  	.set_length = &iio_set_length_kfifo,
> +	.release = &iio_kfifo_buffer_release,
>  };
>  
>  struct iio_buffer *iio_kfifo_allocate(struct iio_dev *indio_dev)
> @@ -158,7 +164,7 @@ EXPORT_SYMBOL(iio_kfifo_allocate);
>  
>  void iio_kfifo_free(struct iio_buffer *r)
>  {
> -	kfree(iio_to_kfifo(r));
> +	iio_buffer_put(r);
>  }
>  EXPORT_SYMBOL(iio_kfifo_free);
>  
> diff --git a/drivers/staging/iio/accel/lis3l02dq_ring.c b/drivers/staging/iio/accel/lis3l02dq_ring.c
> index aae86dd..99b233f 100644
> --- a/drivers/staging/iio/accel/lis3l02dq_ring.c
> +++ b/drivers/staging/iio/accel/lis3l02dq_ring.c
> @@ -397,7 +397,7 @@ int lis3l02dq_configure_buffer(struct iio_dev *indio_dev)
>  	if (!buffer)
>  		return -ENOMEM;
>  
> -	indio_dev->buffer = buffer;
> +	iio_device_attach_buffer(indio_dev, buffer);
>  
>  	buffer->scan_timestamp = true;
>  	indio_dev->setup_ops = &lis3l02dq_buffer_setup_ops;
> diff --git a/drivers/staging/iio/accel/sca3000_ring.c b/drivers/staging/iio/accel/sca3000_ring.c
> index 0f2ee33..4a27beb 100644
> --- a/drivers/staging/iio/accel/sca3000_ring.c
> +++ b/drivers/staging/iio/accel/sca3000_ring.c
> @@ -265,7 +265,7 @@ static struct iio_buffer *sca3000_rb_allocate(struct iio_dev *indio_dev)
>  	return buf;
>  }
>  
> -static inline void sca3000_rb_free(struct iio_buffer *r)
> +static void sca3000_ring_release(struct iio_buffer *r)
>  {
>  	kfree(iio_to_hw_buf(r));
>  }
> @@ -274,23 +274,28 @@ static const struct iio_buffer_access_funcs sca3000_ring_access_funcs = {
>  	.read_first_n = &sca3000_read_first_n_hw_rb,
>  	.get_length = &sca3000_ring_get_length,
>  	.get_bytes_per_datum = &sca3000_ring_get_bytes_per_datum,
> +	.release = sca3000_ring_release,
>  };
>  
>  int sca3000_configure_ring(struct iio_dev *indio_dev)
>  {
> -	indio_dev->buffer = sca3000_rb_allocate(indio_dev);
> -	if (indio_dev->buffer == NULL)
> +	struct iio_buffer *buffer;
> +
> +	buffer = sca3000_rb_allocate(indio_dev);
> +	if (buffer == NULL)
>  		return -ENOMEM;
>  	indio_dev->modes |= INDIO_BUFFER_HARDWARE;
>  
>  	indio_dev->buffer->access = &sca3000_ring_access_funcs;
>  
> +	iio_device_attach_buffer(indio_dev, buffer);
> +
>  	return 0;
>  }
>  
>  void sca3000_unconfigure_ring(struct iio_dev *indio_dev)
>  {
> -	sca3000_rb_free(indio_dev->buffer);
> +	iio_buffer_put(indio_dev->buffer);
>  }
>  
>  static inline
> diff --git a/drivers/staging/iio/iio_simple_dummy_buffer.c b/drivers/staging/iio/iio_simple_dummy_buffer.c
> index 09c93ac..2612e87 100644
> --- a/drivers/staging/iio/iio_simple_dummy_buffer.c
> +++ b/drivers/staging/iio/iio_simple_dummy_buffer.c
> @@ -135,7 +135,7 @@ int iio_simple_dummy_configure_buffer(struct iio_dev *indio_dev,
>  		goto error_ret;
>  	}
>  
> -	indio_dev->buffer = buffer;
> +	iio_device_attach_buffer(indio_dev, buffer);
>  
>  	/* Enable timestamps by default */
>  	buffer->scan_timestamp = true;
> diff --git a/drivers/staging/iio/impedance-analyzer/ad5933.c b/drivers/staging/iio/impedance-analyzer/ad5933.c
> index 712f3c2..f570115 100644
> --- a/drivers/staging/iio/impedance-analyzer/ad5933.c
> +++ b/drivers/staging/iio/impedance-analyzer/ad5933.c
> @@ -630,10 +630,14 @@ static const struct iio_buffer_setup_ops ad5933_ring_setup_ops = {
>  
>  static int ad5933_register_ring_funcs_and_init(struct iio_dev *indio_dev)
>  {
> -	indio_dev->buffer = iio_kfifo_allocate(indio_dev);
> -	if (!indio_dev->buffer)
> +	struct iio_buffer *buffer;
> +
> +	buffer = iio_kfifo_allocate(indio_dev);
> +	if (buffer)
>  		return -ENOMEM;
>  
> +	iio_device_attach_buffer(indio_dev, buffer);
> +
>  	/* Ring buffer functions - here trigger setup related */
>  	indio_dev->setup_ops = &ad5933_ring_setup_ops;
>  
> diff --git a/drivers/staging/iio/meter/ade7758_ring.c b/drivers/staging/iio/meter/ade7758_ring.c
> index 4080995..7a448ff 100644
> --- a/drivers/staging/iio/meter/ade7758_ring.c
> +++ b/drivers/staging/iio/meter/ade7758_ring.c
> @@ -121,14 +121,17 @@ void ade7758_unconfigure_ring(struct iio_dev *indio_dev)
>  int ade7758_configure_ring(struct iio_dev *indio_dev)
>  {
>  	struct ade7758_state *st = iio_priv(indio_dev);
> +	struct iio_buffer *buffer;
>  	int ret = 0;
>  
> -	indio_dev->buffer = iio_kfifo_allocate(indio_dev);
> -	if (!indio_dev->buffer) {
> +	buffer = iio_kfifo_allocate(indio_dev);
> +	if (!buffer) {
>  		ret = -ENOMEM;
>  		return ret;
>  	}
>  
> +	iio_device_attach_buffer(indio_dev, buffer);
> +
>  	indio_dev->setup_ops = &ade7758_ring_setup_ops;
>  
>  	indio_dev->pollfunc = iio_alloc_pollfunc(&iio_pollfunc_store_time,
> diff --git a/include/linux/iio/buffer.h b/include/linux/iio/buffer.h
> index a1124bd..6e428d9 100644
> --- a/include/linux/iio/buffer.h
> +++ b/include/linux/iio/buffer.h
> @@ -11,6 +11,7 @@
>  #define _IIO_BUFFER_GENERIC_H_
>  #include <linux/sysfs.h>
>  #include <linux/iio/iio.h>
> +#include <linux/kref.h>
>  
>  #ifdef CONFIG_IIO_BUFFER
>  
> @@ -26,6 +27,8 @@ struct iio_buffer;
>   * @set_bytes_per_datum:set number of bytes per datum
>   * @get_length:		get number of datums in buffer
>   * @set_length:		set number of datums in buffer
> + * @release:		called when the last reference to the buffer is dropped,
> + *			should free all resources allocated by the buffer.
>   *
>   * The purpose of this structure is to make the buffer element
>   * modular as event for a given driver, different usecases may require
> @@ -47,6 +50,8 @@ struct iio_buffer_access_funcs {
>  	int (*set_bytes_per_datum)(struct iio_buffer *buffer, size_t bpd);
>  	int (*get_length)(struct iio_buffer *buffer);
>  	int (*set_length)(struct iio_buffer *buffer, int length);
> +
> +	void (*release)(struct iio_buffer *buffer);
>  };
>  
>  /**
> @@ -67,6 +72,7 @@ struct iio_buffer_access_funcs {
>   * @demux_list:		[INTERN] list of operations required to demux the scan.
>   * @demux_bounce:	[INTERN] buffer for doing gather from incoming scan.
>   * @buffer_list:	[INTERN] entry in the devices list of current buffers.
> + * @ref:		[INTERN] reference count of the buffer.
>   */
>  struct iio_buffer {
>  	int					length;
> @@ -83,6 +89,7 @@ struct iio_buffer {
>  	struct list_head			demux_list;
>  	void					*demux_bounce;
>  	struct list_head			buffer_list;
> +	struct kref				ref;
>  };
>  
>  /**
> @@ -204,6 +211,24 @@ int iio_sw_buffer_preenable(struct iio_dev *indio_dev);
>  bool iio_validate_scan_mask_onehot(struct iio_dev *indio_dev,
>  	const unsigned long *mask);
>  
> +struct iio_buffer *iio_buffer_get(struct iio_buffer *buffer);
> +void iio_buffer_put(struct iio_buffer *buffer);
> +
> +/**
> + * iio_device_attach_buffer - Attach a buffer to a IIO device
> + * @indio_dev: The device the buffer should be attached to
> + * @buffer: The buffer to attach to the device
> + *
> + * This function attaches a buffer to a IIO device. The buffer stays attached to
> + * the device until the device is freed. The function should only be called at
> + * most once per device.
> + */
> +static inline void iio_device_attach_buffer(struct iio_dev *indio_dev,
> +	struct iio_buffer *buffer)
> +{
> +	indio_dev->buffer = iio_buffer_get(buffer);
> +}
> +
>  #else /* CONFIG_IIO_BUFFER */
>  
>  static inline int iio_buffer_register(struct iio_dev *indio_dev,
> @@ -216,6 +241,9 @@ static inline int iio_buffer_register(struct iio_dev *indio_dev,
>  static inline void iio_buffer_unregister(struct iio_dev *indio_dev)
>  {}
>  
> +static inline void iio_buffer_get(struct iio_buffer *buffer) {}
> +static inline void iio_buffer_put(struct iio_buffer *buffer) {}
> +
>  #endif /* CONFIG_IIO_BUFFER */
>  
>  #endif /* _IIO_BUFFER_GENERIC_H_ */
> 

Re: [PATCH v2 2/5] iio: Return -ENODEV for file operations if the device has been unregistered

[Jonathan Cameron]

On 10/04/13 12:06, Lars-Peter Clausen wrote:
> If the IIO device has been unregistered return -ENODEV for any further file
> operations like read() and ioctl(). This avoids userspace being able to grab new
> references to the device.
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
Applied to the togreg branch of iio.git

Thanks
> ---
> No changes since v1
> ---
>  drivers/iio/industrialio-buffer.c | 6 ++++++
>  drivers/iio/industrialio-core.c   | 3 +++
>  drivers/iio/industrialio-event.c  | 6 ++++++
>  3 files changed, 15 insertions(+)
> 
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index 36c39dc..6c7a9c5 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -48,6 +48,9 @@ ssize_t iio_buffer_read_first_n_outer(struct file *filp, char __user *buf,
>  	struct iio_dev *indio_dev = filp->private_data;
>  	struct iio_buffer *rb = indio_dev->buffer;
>  
> +	if (!indio_dev->info)
> +		return -ENODEV;
> +
>  	if (!rb || !rb->access->read_first_n)
>  		return -EINVAL;
>  	return rb->access->read_first_n(rb, n, buf);
> @@ -62,6 +65,9 @@ unsigned int iio_buffer_poll(struct file *filp,
>  	struct iio_dev *indio_dev = filp->private_data;
>  	struct iio_buffer *rb = indio_dev->buffer;
>  
> +	if (!indio_dev->info)
> +		return -ENODEV;
> +
>  	poll_wait(filp, &rb->pollq, wait);
>  	if (rb->stufftoread)
>  		return POLLIN | POLLRDNORM;
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index 0dfaf9e..c9c6558 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -1041,6 +1041,9 @@ static long iio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  	int __user *ip = (int __user *)arg;
>  	int fd;
>  
> +	if (!indio_dev->info)
> +		return -ENODEV;
> +
>  	if (cmd == IIO_GET_EVENT_FD_IOCTL) {
>  		fd = iio_event_getfd(indio_dev);
>  		if (copy_to_user(ip, &fd, sizeof(fd)))
> diff --git a/drivers/iio/industrialio-event.c b/drivers/iio/industrialio-event.c
> index 36f0c8e..837d450 100644
> --- a/drivers/iio/industrialio-event.c
> +++ b/drivers/iio/industrialio-event.c
> @@ -76,6 +76,9 @@ static unsigned int iio_event_poll(struct file *filep,
>  	struct iio_event_interface *ev_int = indio_dev->event_interface;
>  	unsigned int events = 0;
>  
> +	if (!indio_dev->info)
> +		return -ENODEV;
> +
>  	poll_wait(filep, &ev_int->wait, wait);
>  
>  	spin_lock_irq(&ev_int->wait.lock);
> @@ -96,6 +99,9 @@ static ssize_t iio_event_chrdev_read(struct file *filep,
>  	unsigned int copied;
>  	int ret;
>  
> +	if (!indio_dev->info)
> +		return -ENODEV;
> +
>  	if (count < sizeof(struct iio_event_data))
>  		return -EINVAL;
>  
> 

Re: [PATCH v2 3/5] iio: Wakeup poll and blocking reads when the device is unregistered

[Jonathan Cameron]

On 10/04/13 12:07, Lars-Peter Clausen wrote:
> Once the device has been unregistered there won't be any new data no matter how
> long a userspace application waits, so we might as well wake them up and let
> them know.
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
I guess this makes sense given they'll then do a read and get an error to indicate
that the device is no longer there.

Applied to the togreg branch of iio.git

Thanks,

Jonathan
> ---
> No changes since v1
> ---
>  drivers/iio/iio_core.h            |  3 +++
>  drivers/iio/industrialio-buffer.c | 16 ++++++++++++++++
>  drivers/iio/industrialio-core.c   |  4 ++++
>  drivers/iio/industrialio-event.c  | 21 ++++++++++++++++++++-
>  4 files changed, 43 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h
> index 9209f47..7512cf7 100644
> --- a/drivers/iio/iio_core.h
> +++ b/drivers/iio/iio_core.h
> @@ -50,6 +50,7 @@ ssize_t iio_buffer_read_first_n_outer(struct file *filp, char __user *buf,
>  #define iio_buffer_read_first_n_outer_addr (&iio_buffer_read_first_n_outer)
>  
>  void iio_disable_all_buffers(struct iio_dev *indio_dev);
> +void iio_buffer_wakeup_poll(struct iio_dev *indio_dev);
>  
>  #else
>  
> @@ -57,11 +58,13 @@ void iio_disable_all_buffers(struct iio_dev *indio_dev);
>  #define iio_buffer_read_first_n_outer_addr NULL
>  
>  static inline void iio_disable_all_buffers(struct iio_dev *indio_dev) {}
> +static inline void iio_buffer_wakeup_poll(struct iio_dev *indio_dev) {}
>  
>  #endif
>  
>  int iio_device_register_eventset(struct iio_dev *indio_dev);
>  void iio_device_unregister_eventset(struct iio_dev *indio_dev);
> +void iio_device_wakeup_eventset(struct iio_dev *indio_dev);
>  int iio_event_getfd(struct iio_dev *indio_dev);
>  
>  #endif
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index 6c7a9c5..5a46c57 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -20,6 +20,7 @@
>  #include <linux/cdev.h>
>  #include <linux/slab.h>
>  #include <linux/poll.h>
> +#include <linux/sched.h>
>  
>  #include <linux/iio/iio.h>
>  #include "iio_core.h"
> @@ -75,6 +76,21 @@ unsigned int iio_buffer_poll(struct file *filp,
>  	return 0;
>  }
>  
> +/**
> + * iio_buffer_wakeup_poll - Wakes up the buffer waitqueue
> + * @indio_dev: The IIO device
> + *
> + * Wakes up the event waitqueue used for poll(). Should usually
> + * be called when the device is unregistered.
> + */
> +void iio_buffer_wakeup_poll(struct iio_dev *indio_dev)
> +{
> +	if (!indio_dev->buffer)
> +		return;
> +
> +	wake_up(&indio_dev->buffer->pollq);
> +}
> +
>  void iio_buffer_init(struct iio_buffer *buffer)
>  {
>  	INIT_LIST_HEAD(&buffer->demux_list);
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index c9c6558..81650e1 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -1139,6 +1139,10 @@ void iio_device_unregister(struct iio_dev *indio_dev)
>  	iio_disable_all_buffers(indio_dev);
>  
>  	indio_dev->info = NULL;
> +
> +	iio_device_wakeup_eventset(indio_dev);
> +	iio_buffer_wakeup_poll(indio_dev);
> +
>  	mutex_unlock(&indio_dev->info_exist_lock);
>  }
>  EXPORT_SYMBOL(iio_device_unregister);
> diff --git a/drivers/iio/industrialio-event.c b/drivers/iio/industrialio-event.c
> index 837d450..d251f30 100644
> --- a/drivers/iio/industrialio-event.c
> +++ b/drivers/iio/industrialio-event.c
> @@ -113,9 +113,14 @@ static ssize_t iio_event_chrdev_read(struct file *filep,
>  		}
>  		/* Blocking on device; waiting for something to be there */
>  		ret = wait_event_interruptible_locked_irq(ev_int->wait,
> -					!kfifo_is_empty(&ev_int->det_events));
> +					!kfifo_is_empty(&ev_int->det_events) ||
> +					indio_dev->info == NULL);
>  		if (ret)
>  			goto error_unlock;
> +		if (indio_dev->info == NULL) {
> +			ret = -ENODEV;
> +			goto error_unlock;
> +		}
>  		/* Single access device so no one else can get the data */
>  	}
>  
> @@ -454,6 +459,20 @@ error_ret:
>  	return ret;
>  }
>  
> +/**
> + * iio_device_wakeup_eventset - Wakes up the event waitqueue
> + * @indio_dev: The IIO device
> + *
> + * Wakes up the event waitqueue used for poll() and blocking read().
> + * Should usually be called when the device is unregistered.
> + */
> +void iio_device_wakeup_eventset(struct iio_dev *indio_dev)
> +{
> +	if (indio_dev->event_interface == NULL)
> +		return;
> +	wake_up(&indio_dev->event_interface->wait);
> +}
> +
>  void iio_device_unregister_eventset(struct iio_dev *indio_dev)
>  {
>  	if (indio_dev->event_interface == NULL)
> 

Re: [PATCH v2 4/5] iio:buffer: Add proper locking for iio_update_buffers()

[Jonathan Cameron]

On 10/04/13 12:07, Lars-Peter Clausen wrote:
> We need to make sure that in-kernel users of iio_update_buffers() do not race
> against each other or against unregistration of the device. So we need to take
> both the mlock and the info_exist_lock when calling iio_update_buffers() from a
> in-kernel consumer.
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
Applied to the togreg branch of iio.git

Thanks,

Jonathan
> ---
> No changes since v1
> ---
>  drivers/iio/industrialio-buffer.c | 29 ++++++++++++++++++++++++++---
>  1 file changed, 26 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index 5a46c57..d6a5455 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -509,7 +509,7 @@ void iio_disable_all_buffers(struct iio_dev *indio_dev)
>  		indio_dev->setup_ops->postdisable(indio_dev);
>  }
>  
> -int iio_update_buffers(struct iio_dev *indio_dev,
> +static int __iio_update_buffers(struct iio_dev *indio_dev,
>  		       struct iio_buffer *insert_buffer,
>  		       struct iio_buffer *remove_buffer)
>  {
> @@ -674,6 +674,29 @@ error_ret:
>  
>  	return ret;
>  }
> +
> +int iio_update_buffers(struct iio_dev *indio_dev,
> +		       struct iio_buffer *insert_buffer,
> +		       struct iio_buffer *remove_buffer)
> +{
> +	int ret;
> +
> +	mutex_lock(&indio_dev->info_exist_lock);
> +	mutex_lock(&indio_dev->mlock);
> +
> +	if (indio_dev->info == NULL) {
> +		ret = -ENODEV;
> +		goto out_unlock;
> +	}
> +
> +	ret = __iio_update_buffers(indio_dev, insert_buffer, remove_buffer);
> +
> +out_unlock:
> +	mutex_unlock(&indio_dev->mlock);
> +	mutex_unlock(&indio_dev->info_exist_lock);
> +
> +	return ret;
> +}
>  EXPORT_SYMBOL_GPL(iio_update_buffers);
>  
>  ssize_t iio_buffer_store_enable(struct device *dev,
> @@ -699,10 +722,10 @@ ssize_t iio_buffer_store_enable(struct device *dev,
>  		goto done;
>  
>  	if (requested_state)
> -		ret = iio_update_buffers(indio_dev,
> +		ret = __iio_update_buffers(indio_dev,
>  					 indio_dev->buffer, NULL);
>  	else
> -		ret = iio_update_buffers(indio_dev,
> +		ret = __iio_update_buffers(indio_dev,
>  					 NULL, indio_dev->buffer);
>  
>  	if (ret < 0)
> 

Re: [PATCH v2 5/5] iio:buffer: Ignore noop requests for iio_update_buffers()

[Jonathan Cameron]

On 10/04/13 12:07, Lars-Peter Clausen wrote:
> Since the kernel now disables all buffers when a device is unregistered it might
> happen that a in-kernel consumer tries to disable that buffer again. So ignore
> requests where the buffer already is in the desired state.
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> ---
> No changes since v1
> ---
>  drivers/iio/industrialio-buffer.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index d6a5455..fd3f3af 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -681,9 +681,23 @@ int iio_update_buffers(struct iio_dev *indio_dev,
>  {
>  	int ret;
>  
> +	if (insert_buffer == remove_buffer)
> +		return 0;
> +
>  	mutex_lock(&indio_dev->info_exist_lock);
>  	mutex_lock(&indio_dev->mlock);
>  
> +	if (insert_buffer && iio_buffer_is_active(insert_buffer))
> +		insert_buffer = NULL;
> +
> +	if (remove_buffer && !iio_buffer_is_active(remove_buffer))
> +		remove_buffer = NULL;
> +
So this condition will occur iff insert_buffer = 0 and remove buffer = 0?

If so, then insert_buffer == remove_buffer and you'll have already returned 0 above??

Entirely possible I'm needing more coffee this morning...
> +	if (!insert_buffer && !remove_buffer) {
> +		ret = 0;
> +		goto out_unlock;
> +	}
> +
>  	if (indio_dev->info == NULL) {
>  		ret = -ENODEV;
>  		goto out_unlock;
> 

Re: [PATCH v2 5/5] iio:buffer: Ignore noop requests for iio_update_buffers()

[Jonathan Cameron]

On 10/12/13 11:19, Lars-Peter Clausen wrote:
> On 10/12/2013 01:10 PM, Jonathan Cameron wrote:
>> On 10/04/13 12:07, Lars-Peter Clausen wrote:
>>> Since the kernel now disables all buffers when a device is unregistered it might
>>> happen that a in-kernel consumer tries to disable that buffer again. So ignore
>>> requests where the buffer already is in the desired state.
>>>
>>> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
>>> ---
>>> No changes since v1
>>> ---
>>>  drivers/iio/industrialio-buffer.c | 14 ++++++++++++++
>>>  1 file changed, 14 insertions(+)
>>>
>>> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
>>> index d6a5455..fd3f3af 100644
>>> --- a/drivers/iio/industrialio-buffer.c
>>> +++ b/drivers/iio/industrialio-buffer.c
>>> @@ -681,9 +681,23 @@ int iio_update_buffers(struct iio_dev *indio_dev,
>>>  {
>>>  	int ret;
>>>  
>>> +	if (insert_buffer == remove_buffer)
>>> +		return 0;
>>> +
>>>  	mutex_lock(&indio_dev->info_exist_lock);
>>>  	mutex_lock(&indio_dev->mlock);
>>>  
>>> +	if (insert_buffer && iio_buffer_is_active(insert_buffer))
>>> +		insert_buffer = NULL;
>>> +
>>> +	if (remove_buffer && !iio_buffer_is_active(remove_buffer))
>>> +		remove_buffer = NULL;
>>> +
>> So this condition will occur iff insert_buffer = 0 and remove buffer = 0?
>>
>> If so, then insert_buffer == remove_buffer and you'll have already returned 0 above??
> 
> This is to catch the case where we've set one (or both) of them to NULL
> because it was already in the requested state.
> 
Ah, as I said. Not enough coffee ;)
>>
>> Entirely possible I'm needing more coffee this morning...
>>> +	if (!insert_buffer && !remove_buffer) {
>>> +		ret = 0;
>>> +		goto out_unlock;
>>> +	}
>>> +
>>>  	if (indio_dev->info == NULL) {
>>>  		ret = -ENODEV;
>>>  		goto out_unlock;
>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
> 

Re: [PATCH v2 5/5] iio:buffer: Ignore noop requests for iio_update_buffers()

[Jonathan Cameron]

On 10/12/13 13:06, Jonathan Cameron wrote:
> On 10/12/13 11:19, Lars-Peter Clausen wrote:
>> On 10/12/2013 01:10 PM, Jonathan Cameron wrote:
>>> On 10/04/13 12:07, Lars-Peter Clausen wrote:
>>>> Since the kernel now disables all buffers when a device is unregistered it might
>>>> happen that a in-kernel consumer tries to disable that buffer again. So ignore
>>>> requests where the buffer already is in the desired state.
>>>>
>>>> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
Applied to the togreg branch of iio.git

Thanks,
>>>> ---
>>>> No changes since v1
>>>> ---
>>>>  drivers/iio/industrialio-buffer.c | 14 ++++++++++++++
>>>>  1 file changed, 14 insertions(+)
>>>>
>>>> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
>>>> index d6a5455..fd3f3af 100644
>>>> --- a/drivers/iio/industrialio-buffer.c
>>>> +++ b/drivers/iio/industrialio-buffer.c
>>>> @@ -681,9 +681,23 @@ int iio_update_buffers(struct iio_dev *indio_dev,
>>>>  {
>>>>  	int ret;
>>>>  
>>>> +	if (insert_buffer == remove_buffer)
>>>> +		return 0;
>>>> +
>>>>  	mutex_lock(&indio_dev->info_exist_lock);
>>>>  	mutex_lock(&indio_dev->mlock);
>>>>  
>>>> +	if (insert_buffer && iio_buffer_is_active(insert_buffer))
>>>> +		insert_buffer = NULL;
>>>> +
>>>> +	if (remove_buffer && !iio_buffer_is_active(remove_buffer))
>>>> +		remove_buffer = NULL;
>>>> +
>>> So this condition will occur iff insert_buffer = 0 and remove buffer = 0?
>>>
>>> If so, then insert_buffer == remove_buffer and you'll have already returned 0 above??
>>
>> This is to catch the case where we've set one (or both) of them to NULL
>> because it was already in the requested state.
>>
> Ah, as I said. Not enough coffee ;)
>>>
>>> Entirely possible I'm needing more coffee this morning...
>>>> +	if (!insert_buffer && !remove_buffer) {
>>>> +		ret = 0;
>>>> +		goto out_unlock;
>>>> +	}
>>>> +
>>>>  	if (indio_dev->info == NULL) {
>>>>  		ret = -ENODEV;
>>>>  		goto out_unlock;
>>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>