[PATCH 01/10] iio: Stop sampling when the device is removed

[PATCH 01/10] iio: Stop sampling when the device is removed

[Lars-Peter Clausen]

Make sure to stop sampling when the device is removed, otherwise it will
continue to sample forever.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/iio/iio_core.h            |  4 ++++
 drivers/iio/industrialio-buffer.c | 19 +++++++++++++++++++
 drivers/iio/industrialio-core.c   |  6 +++++-
 3 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h
index 6be5ab8..9209f47 100644
--- a/drivers/iio/iio_core.h
+++ b/drivers/iio/iio_core.h
@@ -49,11 +49,15 @@ ssize_t iio_buffer_read_first_n_outer(struct file *filp, char __user *buf,
 #define iio_buffer_poll_addr (&iio_buffer_poll)
 #define iio_buffer_read_first_n_outer_addr (&iio_buffer_read_first_n_outer)
 
+void iio_disable_all_buffers(struct iio_dev *indio_dev);
+
 #else
 
 #define iio_buffer_poll_addr NULL
 #define iio_buffer_read_first_n_outer_addr NULL
 
+static inline void iio_disable_all_buffers(struct iio_dev *indio_dev) {}
+
 #endif
 
 int iio_device_register_eventset(struct iio_dev *indio_dev);
diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index a7ac4b5..379721a 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -452,6 +452,25 @@ static int iio_compute_scan_bytes(struct iio_dev *indio_dev, const long *mask,
 	return bytes;
 }
 
+void iio_disable_all_buffers(struct iio_dev *indio_dev)
+{
+	struct iio_buffer *buffer, *_buffer;
+
+	if (list_empty(&indio_dev->buffer_list))
+		return;
+
+	if (indio_dev->setup_ops->predisable)
+		indio_dev->setup_ops->predisable(indio_dev);
+
+	list_for_each_entry_safe(buffer, _buffer,
+			&indio_dev->buffer_list, buffer_list)
+		list_del_init(&buffer->buffer_list);
+
+	indio_dev->currentmode = INDIO_DIRECT_MODE;
+	if (indio_dev->setup_ops->postdisable)
+		indio_dev->setup_ops->postdisable(indio_dev);
+}
+
 int iio_update_buffers(struct iio_dev *indio_dev,
 		       struct iio_buffer *insert_buffer,
 		       struct iio_buffer *remove_buffer)
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 24db185..96b35f0 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -1120,9 +1120,13 @@ EXPORT_SYMBOL(iio_device_register);
 void iio_device_unregister(struct iio_dev *indio_dev)
 {
 	mutex_lock(&indio_dev->info_exist_lock);
+
+	device_del(&indio_dev->dev);
+
+	iio_disable_all_buffers(indio_dev);
+
 	indio_dev->info = NULL;
 	mutex_unlock(&indio_dev->info_exist_lock);
-	device_del(&indio_dev->dev);
 }
 EXPORT_SYMBOL(iio_device_unregister);
 subsys_initcall(iio_init);
-- 
1.8.0

[PATCH 02/10] iio: Keep a reference to the IIO device for open file descriptors

[Lars-Peter Clausen]

Make sure that the IIO device is not freed while we still have file descriptors
for it.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/iio/industrialio-core.c  |  4 ++++
 drivers/iio/industrialio-event.c | 19 ++++++++++++++-----
 2 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 96b35f0..5ea741c 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -1012,6 +1012,8 @@ static int iio_chrdev_open(struct inode *inode, struct file *filp)
 	if (test_and_set_bit(IIO_BUSY_BIT_POS, &indio_dev->flags))
 		return -EBUSY;
 
+	iio_device_get(indio_dev);
+
 	filp->private_data = indio_dev;
 
 	return 0;
@@ -1025,6 +1027,8 @@ static int iio_chrdev_release(struct inode *inode, struct file *filp)
 	struct iio_dev *indio_dev = container_of(inode->i_cdev,
 						struct iio_dev, chrdev);
 	clear_bit(IIO_BUSY_BIT_POS, &indio_dev->flags);
+	iio_device_put(indio_dev);
+
 	return 0;
 }
 
diff --git a/drivers/iio/industrialio-event.c b/drivers/iio/industrialio-event.c
index 2390e3d..f89843b 100644
--- a/drivers/iio/industrialio-event.c
+++ b/drivers/iio/industrialio-event.c
@@ -72,7 +72,8 @@ EXPORT_SYMBOL(iio_push_event);
 static unsigned int iio_event_poll(struct file *filep,
 			     struct poll_table_struct *wait)
 {
-	struct iio_event_interface *ev_int = filep->private_data;
+	struct iio_dev *indio_dev = filep->private_data;
+	struct iio_event_interface *ev_int = indio_dev->event_interface;
 	unsigned int events = 0;
 
 	poll_wait(filep, &ev_int->wait, wait);
@@ -90,7 +91,8 @@ static ssize_t iio_event_chrdev_read(struct file *filep,
 				     size_t count,
 				     loff_t *f_ps)
 {
-	struct iio_event_interface *ev_int = filep->private_data;
+	struct iio_dev *indio_dev = filep->private_data;
+	struct iio_event_interface *ev_int = indio_dev->event_interface;
 	unsigned int copied;
 	int ret;
 
@@ -121,7 +123,8 @@ error_unlock:
 
 static int iio_event_chrdev_release(struct inode *inode, struct file *filep)
 {
-	struct iio_event_interface *ev_int = filep->private_data;
+	struct iio_dev *indio_dev = filep->private_data;
+	struct iio_event_interface *ev_int = indio_dev->event_interface;
 
 	spin_lock_irq(&ev_int->wait.lock);
 	__clear_bit(IIO_BUSY_BIT_POS, &ev_int->flags);
@@ -133,6 +136,8 @@ static int iio_event_chrdev_release(struct inode *inode, struct file *filep)
 	kfifo_reset_out(&ev_int->det_events);
 	spin_unlock_irq(&ev_int->wait.lock);
 
+	iio_device_put(indio_dev);
+
 	return 0;
 }
 
@@ -158,12 +163,16 @@ int iio_event_getfd(struct iio_dev *indio_dev)
 		return -EBUSY;
 	}
 	spin_unlock_irq(&ev_int->wait.lock);
-	fd = anon_inode_getfd("iio:event",
-				&iio_event_chrdev_fileops, ev_int, O_RDONLY | O_CLOEXEC);
+
+	iio_device_get(indio_dev);
+
+	fd = anon_inode_getfd("iio:event", &iio_event_chrdev_fileops,
+				indio_dev, O_RDONLY | O_CLOEXEC);
 	if (fd < 0) {
 		spin_lock_irq(&ev_int->wait.lock);
 		__clear_bit(IIO_BUSY_BIT_POS, &ev_int->flags);
 		spin_unlock_irq(&ev_int->wait.lock);
+		iio_device_put(indio_dev);
 	}
 	return fd;
 }
-- 
1.8.0

[PATCH 03/10] iio: Set the IIO device as the parent for the character device

[Lars-Peter Clausen]

We need to make sure that the IIO device is not freed while the character device
exists, otherwise the freeing of the IIO device might race against the file open
callback. Do this by setting the character device's parent to the IIO device,
this will cause the character device to grab a reference to the IIO device and
only release it once the character device itself has been removed.

Also move the registration of the character device before the registration of
the IIO device to avoid the (rather theoretical case) that the IIO device is
already freed again before we can add the character device and grab a reference
to the IIO device.

We also need to move the call to cdev_del() from iio_dev_release() to
iio_device_unregister() (where it should have been in the first place anyway) to
avoid a reference cycle. As iio_dev_release() is only called once all reference
are dropped, but the character device holds a reference to the IIO device.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/iio/industrialio-core.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 5ea741c..863aa01 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -890,8 +890,6 @@ static void iio_device_unregister_sysfs(struct iio_dev *indio_dev)
 static void iio_dev_release(struct device *device)
 {
 	struct iio_dev *indio_dev = dev_to_iio_dev(device);
-	if (indio_dev->chrdev.dev)
-		cdev_del(&indio_dev->chrdev);
 	if (indio_dev->modes & INDIO_BUFFER_TRIGGERED)
 		iio_device_unregister_trigger_consumer(indio_dev);
 	iio_device_unregister_eventset(indio_dev);
@@ -1098,18 +1096,20 @@ int iio_device_register(struct iio_dev *indio_dev)
 		indio_dev->setup_ops == NULL)
 		indio_dev->setup_ops = &noop_ring_setup_ops;
 
-	ret = device_add(&indio_dev->dev);
-	if (ret < 0)
-		goto error_unreg_eventset;
 	cdev_init(&indio_dev->chrdev, &iio_buffer_fileops);
 	indio_dev->chrdev.owner = indio_dev->info->driver_module;
+	indio_dev->chrdev.kobj.parent = &indio_dev->dev.kobj;
 	ret = cdev_add(&indio_dev->chrdev, indio_dev->dev.devt, 1);
 	if (ret < 0)
-		goto error_del_device;
-	return 0;
+		goto error_unreg_eventset;
 
-error_del_device:
-	device_del(&indio_dev->dev);
+	ret = device_add(&indio_dev->dev);
+	if (ret < 0)
+		goto error_cdev_del;
+
+	return 0;
+error_cdev_del:
+	cdev_del(&indio_dev->chrdev);
 error_unreg_eventset:
 	iio_device_unregister_eventset(indio_dev);
 error_free_sysfs:
@@ -1127,6 +1127,9 @@ void iio_device_unregister(struct iio_dev *indio_dev)
 
 	device_del(&indio_dev->dev);
 
+	if (indio_dev->chrdev.dev)
+		cdev_del(&indio_dev->chrdev);
+
 	iio_disable_all_buffers(indio_dev);
 
 	indio_dev->info = NULL;
-- 
1.8.0

[PATCH 04/10] iio:buffer_cb: Add missing iio_buffer_init()

[Lars-Peter Clausen]

Make sure to properly initialize the IIO buffer data structure.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/iio/buffer_cb.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/iio/buffer_cb.c b/drivers/iio/buffer_cb.c
index 578f719..841fec1 100644
--- a/drivers/iio/buffer_cb.c
+++ b/drivers/iio/buffer_cb.c
@@ -41,6 +41,8 @@ struct iio_cb_buffer *iio_channel_get_all_cb(struct device *dev,
 		goto error_ret;
 	}
 
+	iio_buffer_init(&cb_buff->buffer);
+
 	cb_buff->private = private;
 	cb_buff->cb = cb;
 	cb_buff->buffer.access = &iio_cb_access;
-- 
1.8.0

[PATCH 05/10] 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>
---
 drivers/iio/buffer_cb.c                         | 21 ++++++++----
 drivers/iio/industrialio-buffer.c               | 33 ++++++++++++++++---
 drivers/iio/industrialio-core.c                 |  4 +++
 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                      | 43 +++++++++++++++++++++++++
 11 files changed, 124 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 379721a..54d3b9b 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);
 
@@ -452,6 +453,20 @@ static int iio_compute_scan_bytes(struct iio_dev *indio_dev, const long *mask,
 	return bytes;
 }
 
+static void iio_buffer_enable(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_disable(struct iio_dev *indio_dev,
+	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;
@@ -464,7 +479,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_disable(indio_dev, buffer);
 
 	indio_dev->currentmode = INDIO_DIRECT_MODE;
 	if (indio_dev->setup_ops->postdisable)
@@ -501,9 +516,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_disable(indio_dev, remove_buffer);
 	if (insert_buffer)
-		list_add(&insert_buffer->buffer_list, &indio_dev->buffer_list);
+		iio_buffer_enable(indio_dev, insert_buffer);
 
 	/* If no buffers in list, we are done */
 	if (list_empty(&indio_dev->buffer_list)) {
@@ -538,7 +553,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_disable(indio_dev, insert_buffer);
 			indio_dev->active_scan_mask = old_mask;
 			success = -EINVAL;
 		}
@@ -622,7 +637,7 @@ error_run_postdisable:
 error_remove_inserted:
 
 	if (insert_buffer)
-		list_del_init(&insert_buffer->buffer_list);
+		iio_buffer_disable(indio_dev, insert_buffer);
 	indio_dev->active_scan_mask = old_mask;
 	kfree(compound_mask);
 error_ret:
@@ -942,3 +957,11 @@ error_clear_mux_table:
 	return ret;
 }
 EXPORT_SYMBOL_GPL(iio_update_demux);
+
+void __iio_buffer_release(struct kref *ref)
+{
+	struct iio_buffer *buffer = container_of(ref, struct iio_buffer, ref);
+
+	buffer->access->release(buffer);
+}
+EXPORT_SYMBOL_GPL(__iio_buffer_release);
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 863aa01..e4c8a6c 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,9 @@ static void iio_dev_release(struct device *device)
 	iio_device_unregister_sysfs(indio_dev);
 	iio_device_unregister_debugfs(indio_dev);
 
+	if (indio_dev->buffer)
+		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 36dcc7e..3ceea13 100644
--- a/drivers/staging/iio/accel/lis3l02dq_ring.c
+++ b/drivers/staging/iio/accel/lis3l02dq_ring.c
@@ -401,7 +401,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 3921865..e3342af 100644
--- a/drivers/staging/iio/iio_simple_dummy_buffer.c
+++ b/drivers/staging/iio/iio_simple_dummy_buffer.c
@@ -138,7 +138,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 3d8bc2d..b7b53b5 100644
--- a/drivers/staging/iio/meter/ade7758_ring.c
+++ b/drivers/staging/iio/meter/ade7758_ring.c
@@ -125,14 +125,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 e5507e9..feb3a1e 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
 
@@ -47,6 +48,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);
 };
 
 /**
@@ -83,6 +86,7 @@ struct iio_buffer {
 	struct list_head			demux_list;
 	void					*demux_bounce;
 	struct list_head			buffer_list;
+	struct kref				ref;
 };
 
 /**
@@ -179,6 +183,42 @@ 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);
 
+void __iio_buffer_release(struct kref *kref);
+
+/**
+ * iio_buffer_get() - Grab a reference to the buffer
+ * @buffer: The buffer to grab a reference for
+ */
+static inline void iio_buffer_get(struct iio_buffer *buffer)
+{
+	kref_get(&buffer->ref);
+}
+
+/**
+ * iio_buffer_put() - Release the reference to the buffer
+ * @buffer: The buffer to release the reference for
+ */
+static inline void iio_buffer_put(struct iio_buffer *buffer)
+{
+	kref_put(&buffer->ref, __iio_buffer_release);
+}
+
+/**
+ * iio_device_attach_buffer - Attach a buffer to a IIO device
+ * @device: 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)
+{
+	iio_buffer_get(buffer);
+	indio_dev->buffer = buffer;
+}
+
 #else /* CONFIG_IIO_BUFFER */
 
 static inline int iio_buffer_register(struct iio_dev *indio_dev,
@@ -191,6 +231,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 06/10] iio: Remove debugfs entries in iio_device_unregister()

[Lars-Peter Clausen]

Remove the the debugfs entries in iio_device_unregister(). Otherwise the debugfs
entries might still be accessible even though the device used in the debugfs
callback has already been freed.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/iio/industrialio-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index e4c8a6c..3e581a76 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -895,7 +895,6 @@ static void iio_dev_release(struct device *device)
 		iio_device_unregister_trigger_consumer(indio_dev);
 	iio_device_unregister_eventset(indio_dev);
 	iio_device_unregister_sysfs(indio_dev);
-	iio_device_unregister_debugfs(indio_dev);
 
 	if (indio_dev->buffer)
 		iio_buffer_put(indio_dev->buffer);
@@ -1133,6 +1132,7 @@ void iio_device_unregister(struct iio_dev *indio_dev)
 
 	if (indio_dev->chrdev.dev)
 		cdev_del(&indio_dev->chrdev);
+	iio_device_unregister_debugfs(indio_dev);
 
 	iio_disable_all_buffers(indio_dev);
 
-- 
1.8.0

[PATCH 07/10] 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>
---
 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 54d3b9b..e9cbde3 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 3e581a76..dd7b601 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 f89843b..3843abf 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 08/10] 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>
---
 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 e9cbde3..c28625a 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 dd7b601..88a77d9 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -1140,6 +1140,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 3843abf..6aace88 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 */
 	}
 
@@ -455,6 +460,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 09/10] 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>
---
 drivers/iio/industrialio-buffer.c | 26 +++++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index c28625a..8ceaaaf 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -508,7 +508,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)
 {
@@ -666,6 +666,26 @@ 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)
+		return -ENODEV;
+
+	ret = __iio_update_buffers(indio_dev, insert_buffer, remove_buffer);
+
+	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,
@@ -691,10 +711,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 10/10] 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>
---
 drivers/iio/industrialio-buffer.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index 8ceaaaf..2b36cea 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -673,9 +673,21 @@ 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)
+		return 0;
+
 	if (indio_dev->info == NULL)
 		return -ENODEV;
 
-- 
1.8.0

Re: [PATCH 09/10] iio:buffer: Add proper locking for iio_update_buffers()

[Lars-Peter Clausen]

[...]
> +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)
> +		return -ENODEV;

Yea, ok that happens when send patches way to late in the evening. Of course we 
still need to unlock here...

> +	ret = __iio_update_buffers(indio_dev, insert_buffer, remove_buffer);
> +
> +	mutex_unlock(&indio_dev->mlock);
> +	mutex_unlock(&indio_dev->info_exist_lock);
> +
> +	return ret;
> +}

Re: [PATCH 01/10] iio: Stop sampling when the device is removed

[Lars-Peter Clausen]

On 09/19/2013 12:00 AM, Jonathan Cameron wrote:
> On 09/18/13 21:02, Lars-Peter Clausen wrote:
>> Make sure to stop sampling when the device is removed, otherwise it will
>> continue to sample forever.
>
> The intent is that you can't remove a device if there is a buffer
> enabled. I thought we had the reference counting correct to prevent
> this happening.  Perhaps not!
>

You can't prevent a device from being removed, that's a basic property of the 
Linux device driver model. Device drivers and subsystems need to be able to 
deal with hot unplug.

- Lars

>>
>> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
>> ---
>>   drivers/iio/iio_core.h            |  4 ++++
>>   drivers/iio/industrialio-buffer.c | 19 +++++++++++++++++++
>>   drivers/iio/industrialio-core.c   |  6 +++++-
>>   3 files changed, 28 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h
>> index 6be5ab8..9209f47 100644
>> --- a/drivers/iio/iio_core.h
>> +++ b/drivers/iio/iio_core.h
>> @@ -49,11 +49,15 @@ ssize_t iio_buffer_read_first_n_outer(struct file *filp, char __user *buf,
>>   #define iio_buffer_poll_addr (&iio_buffer_poll)
>>   #define iio_buffer_read_first_n_outer_addr (&iio_buffer_read_first_n_outer)
>>
>> +void iio_disable_all_buffers(struct iio_dev *indio_dev);
>> +
>>   #else
>>
>>   #define iio_buffer_poll_addr NULL
>>   #define iio_buffer_read_first_n_outer_addr NULL
>>
>> +static inline void iio_disable_all_buffers(struct iio_dev *indio_dev) {}
>> +
>>   #endif
>>
>>   int iio_device_register_eventset(struct iio_dev *indio_dev);
>> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
>> index a7ac4b5..379721a 100644
>> --- a/drivers/iio/industrialio-buffer.c
>> +++ b/drivers/iio/industrialio-buffer.c
>> @@ -452,6 +452,25 @@ static int iio_compute_scan_bytes(struct iio_dev *indio_dev, const long *mask,
>>   	return bytes;
>>   }
>>
>> +void iio_disable_all_buffers(struct iio_dev *indio_dev)
>> +{
>> +	struct iio_buffer *buffer, *_buffer;
>> +
>> +	if (list_empty(&indio_dev->buffer_list))
>> +		return;
>> +
>> +	if (indio_dev->setup_ops->predisable)
>> +		indio_dev->setup_ops->predisable(indio_dev);
>> +
>> +	list_for_each_entry_safe(buffer, _buffer,
>> +			&indio_dev->buffer_list, buffer_list)
>> +		list_del_init(&buffer->buffer_list);
>> +
>> +	indio_dev->currentmode = INDIO_DIRECT_MODE;
>> +	if (indio_dev->setup_ops->postdisable)
>> +		indio_dev->setup_ops->postdisable(indio_dev);
>> +}
>> +
>>   int iio_update_buffers(struct iio_dev *indio_dev,
>>   		       struct iio_buffer *insert_buffer,
>>   		       struct iio_buffer *remove_buffer)
>> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
>> index 24db185..96b35f0 100644
>> --- a/drivers/iio/industrialio-core.c
>> +++ b/drivers/iio/industrialio-core.c
>> @@ -1120,9 +1120,13 @@ EXPORT_SYMBOL(iio_device_register);
>>   void iio_device_unregister(struct iio_dev *indio_dev)
>>   {
>>   	mutex_lock(&indio_dev->info_exist_lock);
>> +
>> +	device_del(&indio_dev->dev);
>> +
>> +	iio_disable_all_buffers(indio_dev);
>> +
>>   	indio_dev->info = NULL;
>>   	mutex_unlock(&indio_dev->info_exist_lock);
>> -	device_del(&indio_dev->dev);
>>   }
>>   EXPORT_SYMBOL(iio_device_unregister);
>>   subsys_initcall(iio_init);
>>

Re: [PATCH 01/10] iio: Stop sampling when the device is removed

[Jonathan Cameron]

On 09/18/13 21:02, Lars-Peter Clausen wrote:
> Make sure to stop sampling when the device is removed, otherwise it will
> continue to sample forever.

The intent is that you can't remove a device if there is a buffer
enabled. I thought we had the reference counting correct to prevent
this happening.  Perhaps not!

> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> ---
>  drivers/iio/iio_core.h            |  4 ++++
>  drivers/iio/industrialio-buffer.c | 19 +++++++++++++++++++
>  drivers/iio/industrialio-core.c   |  6 +++++-
>  3 files changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h
> index 6be5ab8..9209f47 100644
> --- a/drivers/iio/iio_core.h
> +++ b/drivers/iio/iio_core.h
> @@ -49,11 +49,15 @@ ssize_t iio_buffer_read_first_n_outer(struct file *filp, char __user *buf,
>  #define iio_buffer_poll_addr (&iio_buffer_poll)
>  #define iio_buffer_read_first_n_outer_addr (&iio_buffer_read_first_n_outer)
>  
> +void iio_disable_all_buffers(struct iio_dev *indio_dev);
> +
>  #else
>  
>  #define iio_buffer_poll_addr NULL
>  #define iio_buffer_read_first_n_outer_addr NULL
>  
> +static inline void iio_disable_all_buffers(struct iio_dev *indio_dev) {}
> +
>  #endif
>  
>  int iio_device_register_eventset(struct iio_dev *indio_dev);
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index a7ac4b5..379721a 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -452,6 +452,25 @@ static int iio_compute_scan_bytes(struct iio_dev *indio_dev, const long *mask,
>  	return bytes;
>  }
>  
> +void iio_disable_all_buffers(struct iio_dev *indio_dev)
> +{
> +	struct iio_buffer *buffer, *_buffer;
> +
> +	if (list_empty(&indio_dev->buffer_list))
> +		return;
> +
> +	if (indio_dev->setup_ops->predisable)
> +		indio_dev->setup_ops->predisable(indio_dev);
> +
> +	list_for_each_entry_safe(buffer, _buffer,
> +			&indio_dev->buffer_list, buffer_list)
> +		list_del_init(&buffer->buffer_list);
> +
> +	indio_dev->currentmode = INDIO_DIRECT_MODE;
> +	if (indio_dev->setup_ops->postdisable)
> +		indio_dev->setup_ops->postdisable(indio_dev);
> +}
> +
>  int iio_update_buffers(struct iio_dev *indio_dev,
>  		       struct iio_buffer *insert_buffer,
>  		       struct iio_buffer *remove_buffer)
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index 24db185..96b35f0 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -1120,9 +1120,13 @@ EXPORT_SYMBOL(iio_device_register);
>  void iio_device_unregister(struct iio_dev *indio_dev)
>  {
>  	mutex_lock(&indio_dev->info_exist_lock);
> +
> +	device_del(&indio_dev->dev);
> +
> +	iio_disable_all_buffers(indio_dev);
> +
>  	indio_dev->info = NULL;
>  	mutex_unlock(&indio_dev->info_exist_lock);
> -	device_del(&indio_dev->dev);
>  }
>  EXPORT_SYMBOL(iio_device_unregister);
>  subsys_initcall(iio_init);
> 

Re: [PATCH 01/10] iio: Stop sampling when the device is removed

[Jonathan Cameron]

On 09/18/13 22:08, Lars-Peter Clausen wrote:
> On 09/19/2013 12:00 AM, Jonathan Cameron wrote:
>> On 09/18/13 21:02, Lars-Peter Clausen wrote:
>>> Make sure to stop sampling when the device is removed, otherwise it will
>>> continue to sample forever.
>>
>> The intent is that you can't remove a device if there is a buffer
>> enabled. I thought we had the reference counting correct to prevent
>> this happening.  Perhaps not!
>>
> 
> You can't prevent a device from being removed, that's a basic property of the Linux device driver model. Device drivers
> and subsystems need to be able to deal with hot unplug.

Hmm.. I hadn't thought that through. So in normal operation we still need
any buffers to be disabled, but if someone actualy unplugs the hardware
then it will all get nicely cleaned up. I'm way to used to nice i2c / spi
devices without any of this modern hotplug stuff ;)

Fair enough.  Thanks for cleaning this stuff up.  Still some uggly
corners out there for the observant to find ;)

Jonathan
> 
> - Lars
> 
>>>
>>> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
>>> ---
>>>   drivers/iio/iio_core.h            |  4 ++++
>>>   drivers/iio/industrialio-buffer.c | 19 +++++++++++++++++++
>>>   drivers/iio/industrialio-core.c   |  6 +++++-
>>>   3 files changed, 28 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h
>>> index 6be5ab8..9209f47 100644
>>> --- a/drivers/iio/iio_core.h
>>> +++ b/drivers/iio/iio_core.h
>>> @@ -49,11 +49,15 @@ ssize_t iio_buffer_read_first_n_outer(struct file *filp, char __user *buf,
>>>   #define iio_buffer_poll_addr (&iio_buffer_poll)
>>>   #define iio_buffer_read_first_n_outer_addr (&iio_buffer_read_first_n_outer)
>>>
>>> +void iio_disable_all_buffers(struct iio_dev *indio_dev);
>>> +
>>>   #else
>>>
>>>   #define iio_buffer_poll_addr NULL
>>>   #define iio_buffer_read_first_n_outer_addr NULL
>>>
>>> +static inline void iio_disable_all_buffers(struct iio_dev *indio_dev) {}
>>> +
>>>   #endif
>>>
>>>   int iio_device_register_eventset(struct iio_dev *indio_dev);
>>> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
>>> index a7ac4b5..379721a 100644
>>> --- a/drivers/iio/industrialio-buffer.c
>>> +++ b/drivers/iio/industrialio-buffer.c
>>> @@ -452,6 +452,25 @@ static int iio_compute_scan_bytes(struct iio_dev *indio_dev, const long *mask,
>>>       return bytes;
>>>   }
>>>
>>> +void iio_disable_all_buffers(struct iio_dev *indio_dev)
>>> +{
>>> +    struct iio_buffer *buffer, *_buffer;
>>> +
>>> +    if (list_empty(&indio_dev->buffer_list))
>>> +        return;
>>> +
>>> +    if (indio_dev->setup_ops->predisable)
>>> +        indio_dev->setup_ops->predisable(indio_dev);
>>> +
>>> +    list_for_each_entry_safe(buffer, _buffer,
>>> +            &indio_dev->buffer_list, buffer_list)
>>> +        list_del_init(&buffer->buffer_list);
>>> +
>>> +    indio_dev->currentmode = INDIO_DIRECT_MODE;
>>> +    if (indio_dev->setup_ops->postdisable)
>>> +        indio_dev->setup_ops->postdisable(indio_dev);
>>> +}
>>> +
>>>   int iio_update_buffers(struct iio_dev *indio_dev,
>>>                  struct iio_buffer *insert_buffer,
>>>                  struct iio_buffer *remove_buffer)
>>> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
>>> index 24db185..96b35f0 100644
>>> --- a/drivers/iio/industrialio-core.c
>>> +++ b/drivers/iio/industrialio-core.c
>>> @@ -1120,9 +1120,13 @@ EXPORT_SYMBOL(iio_device_register);
>>>   void iio_device_unregister(struct iio_dev *indio_dev)
>>>   {
>>>       mutex_lock(&indio_dev->info_exist_lock);
>>> +
>>> +    device_del(&indio_dev->dev);
>>> +
>>> +    iio_disable_all_buffers(indio_dev);
>>> +
>>>       indio_dev->info = NULL;
>>>       mutex_unlock(&indio_dev->info_exist_lock);
>>> -    device_del(&indio_dev->dev);
>>>   }
>>>   EXPORT_SYMBOL(iio_device_unregister);
>>>   subsys_initcall(iio_init);
>>>
> 

Re: [PATCH 01/10] iio: Stop sampling when the device is removed

[Lars-Peter Clausen]

On 09/21/2013 01:36 PM, Jonathan Cameron wrote:
> On 09/18/13 21:02, Lars-Peter Clausen wrote:
>> Make sure to stop sampling when the device is removed, otherwise it will
>> continue to sample forever.
>>
>> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> Applied to the fixes togreg branch of iio.git
>

1-4 can go in as is, I've reworked and made some fixes the others though.

- Lars

Re: [PATCH 09/10] iio:buffer: Add proper locking for iio_update_buffers()

[Lars-Peter Clausen]

On 09/21/2013 01:59 PM, Jonathan Cameron wrote:
> On 09/18/13 21:27, Lars-Peter Clausen wrote:
>> [...]
>>> +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)
>>> +        return -ENODEV;
>>
>> Yea, ok that happens when send patches way to late in the evening. Of course we still need to unlock here...
> Any reason not to do the indio_dev->info test before locking in the first place?

The check and the updating needs to happen in one atomic step, otherwise the 
updating might race against the iio_disable_all_buffers in the 
iio_device_unregister function.

>>
>>> +    ret = __iio_update_buffers(indio_dev, insert_buffer, remove_buffer);
>>> +
>>> +    mutex_unlock(&indio_dev->mlock);
>>> +    mutex_unlock(&indio_dev->info_exist_lock);
>>> +
>>> +    return ret;
>>> +}
>>
> --
> 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 01/10] iio: Stop sampling when the device is removed

[Jonathan Cameron]

On 09/18/13 21:02, Lars-Peter Clausen wrote:
> Make sure to stop sampling when the device is removed, otherwise it will
> continue to sample forever.
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
Applied to the fixes togreg branch of iio.git

Thanks,
> ---
>  drivers/iio/iio_core.h            |  4 ++++
>  drivers/iio/industrialio-buffer.c | 19 +++++++++++++++++++
>  drivers/iio/industrialio-core.c   |  6 +++++-
>  3 files changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h
> index 6be5ab8..9209f47 100644
> --- a/drivers/iio/iio_core.h
> +++ b/drivers/iio/iio_core.h
> @@ -49,11 +49,15 @@ ssize_t iio_buffer_read_first_n_outer(struct file *filp, char __user *buf,
>  #define iio_buffer_poll_addr (&iio_buffer_poll)
>  #define iio_buffer_read_first_n_outer_addr (&iio_buffer_read_first_n_outer)
>  
> +void iio_disable_all_buffers(struct iio_dev *indio_dev);
> +
>  #else
>  
>  #define iio_buffer_poll_addr NULL
>  #define iio_buffer_read_first_n_outer_addr NULL
>  
> +static inline void iio_disable_all_buffers(struct iio_dev *indio_dev) {}
> +
>  #endif
>  
>  int iio_device_register_eventset(struct iio_dev *indio_dev);
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index a7ac4b5..379721a 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -452,6 +452,25 @@ static int iio_compute_scan_bytes(struct iio_dev *indio_dev, const long *mask,
>  	return bytes;
>  }
>  
> +void iio_disable_all_buffers(struct iio_dev *indio_dev)
> +{
> +	struct iio_buffer *buffer, *_buffer;
> +
> +	if (list_empty(&indio_dev->buffer_list))
> +		return;
> +
> +	if (indio_dev->setup_ops->predisable)
> +		indio_dev->setup_ops->predisable(indio_dev);
> +
> +	list_for_each_entry_safe(buffer, _buffer,
> +			&indio_dev->buffer_list, buffer_list)
> +		list_del_init(&buffer->buffer_list);
> +
> +	indio_dev->currentmode = INDIO_DIRECT_MODE;
> +	if (indio_dev->setup_ops->postdisable)
> +		indio_dev->setup_ops->postdisable(indio_dev);
> +}
> +
>  int iio_update_buffers(struct iio_dev *indio_dev,
>  		       struct iio_buffer *insert_buffer,
>  		       struct iio_buffer *remove_buffer)
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index 24db185..96b35f0 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -1120,9 +1120,13 @@ EXPORT_SYMBOL(iio_device_register);
>  void iio_device_unregister(struct iio_dev *indio_dev)
>  {
>  	mutex_lock(&indio_dev->info_exist_lock);
> +
> +	device_del(&indio_dev->dev);
> +
> +	iio_disable_all_buffers(indio_dev);
> +
>  	indio_dev->info = NULL;
>  	mutex_unlock(&indio_dev->info_exist_lock);
> -	device_del(&indio_dev->dev);
>  }
>  EXPORT_SYMBOL(iio_device_unregister);
>  subsys_initcall(iio_init);
> 

Re: [PATCH 08/10] iio: Wakeup poll and blocking reads when the device is unregistered

[Lars-Peter Clausen]

On 09/21/2013 01:56 PM, Jonathan Cameron wrote:
> On 09/18/13 21:02, 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>
> This is probably the only one in the set that isn't technically a 'fix' so could you
> reorder so this is at the end.  I'll then push it out once one the other patches
> have made there way into the staging-next tree.
>

While the last two patches are fixes, there is currently no way to trigger the 
bug they fix since there are no users of the cb_buffer yet, so strictly 
speaking they don't have to go into the fixes branch.

> Thanks,
>> ---
>>   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 e9cbde3..c28625a 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 dd7b601..88a77d9 100644
>> --- a/drivers/iio/industrialio-core.c
>> +++ b/drivers/iio/industrialio-core.c
>> @@ -1140,6 +1140,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 3843abf..6aace88 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 */
>>   	}
>>
>> @@ -455,6 +460,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 02/10] iio: Keep a reference to the IIO device for open file descriptors

[Jonathan Cameron]

On 09/18/13 21:02, Lars-Peter Clausen wrote:
> Make sure that the IIO device is not freed while we still have file descriptors
> for it.
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
Back ported to the fixes-togreg branch and applied.

Lars.  Either these should have been based on that branch, or
you should have sent a good reason why not.  Whilst these are
rather large and invasive, they are fixing real bugs so
should go into mainline asap as far as I can see.

Jonathan
> ---
>  drivers/iio/industrialio-core.c  |  4 ++++
>  drivers/iio/industrialio-event.c | 19 ++++++++++++++-----
>  2 files changed, 18 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index 96b35f0..5ea741c 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -1012,6 +1012,8 @@ static int iio_chrdev_open(struct inode *inode, struct file *filp)
>  	if (test_and_set_bit(IIO_BUSY_BIT_POS, &indio_dev->flags))
>  		return -EBUSY;
>  
> +	iio_device_get(indio_dev);
> +
>  	filp->private_data = indio_dev;
>  
>  	return 0;
> @@ -1025,6 +1027,8 @@ static int iio_chrdev_release(struct inode *inode, struct file *filp)
>  	struct iio_dev *indio_dev = container_of(inode->i_cdev,
>  						struct iio_dev, chrdev);
>  	clear_bit(IIO_BUSY_BIT_POS, &indio_dev->flags);
> +	iio_device_put(indio_dev);
> +
>  	return 0;
>  }
>  
> diff --git a/drivers/iio/industrialio-event.c b/drivers/iio/industrialio-event.c
> index 2390e3d..f89843b 100644
> --- a/drivers/iio/industrialio-event.c
> +++ b/drivers/iio/industrialio-event.c
> @@ -72,7 +72,8 @@ EXPORT_SYMBOL(iio_push_event);
>  static unsigned int iio_event_poll(struct file *filep,
>  			     struct poll_table_struct *wait)
>  {
> -	struct iio_event_interface *ev_int = filep->private_data;
> +	struct iio_dev *indio_dev = filep->private_data;
> +	struct iio_event_interface *ev_int = indio_dev->event_interface;
>  	unsigned int events = 0;
>  
>  	poll_wait(filep, &ev_int->wait, wait);
> @@ -90,7 +91,8 @@ static ssize_t iio_event_chrdev_read(struct file *filep,
>  				     size_t count,
>  				     loff_t *f_ps)
>  {
> -	struct iio_event_interface *ev_int = filep->private_data;
> +	struct iio_dev *indio_dev = filep->private_data;
> +	struct iio_event_interface *ev_int = indio_dev->event_interface;
>  	unsigned int copied;
>  	int ret;
>  
> @@ -121,7 +123,8 @@ error_unlock:
>  
>  static int iio_event_chrdev_release(struct inode *inode, struct file *filep)
>  {
> -	struct iio_event_interface *ev_int = filep->private_data;
> +	struct iio_dev *indio_dev = filep->private_data;
> +	struct iio_event_interface *ev_int = indio_dev->event_interface;
>  
>  	spin_lock_irq(&ev_int->wait.lock);
>  	__clear_bit(IIO_BUSY_BIT_POS, &ev_int->flags);
> @@ -133,6 +136,8 @@ static int iio_event_chrdev_release(struct inode *inode, struct file *filep)
>  	kfifo_reset_out(&ev_int->det_events);
>  	spin_unlock_irq(&ev_int->wait.lock);
>  
> +	iio_device_put(indio_dev);
> +
>  	return 0;
>  }
>  
> @@ -158,12 +163,16 @@ int iio_event_getfd(struct iio_dev *indio_dev)
>  		return -EBUSY;
>  	}
>  	spin_unlock_irq(&ev_int->wait.lock);
> -	fd = anon_inode_getfd("iio:event",
> -				&iio_event_chrdev_fileops, ev_int, O_RDONLY | O_CLOEXEC);
> +
> +	iio_device_get(indio_dev);
> +
> +	fd = anon_inode_getfd("iio:event", &iio_event_chrdev_fileops,
> +				indio_dev, O_RDONLY | O_CLOEXEC);
>  	if (fd < 0) {
>  		spin_lock_irq(&ev_int->wait.lock);
>  		__clear_bit(IIO_BUSY_BIT_POS, &ev_int->flags);
>  		spin_unlock_irq(&ev_int->wait.lock);
> +		iio_device_put(indio_dev);
>  	}
>  	return fd;
>  }
> 

Re: [PATCH 02/10] iio: Keep a reference to the IIO device for open file descriptors

[Jonathan Cameron]

On 09/18/13 21:02, Lars-Peter Clausen wrote:
> Make sure that the IIO device is not freed while we still have file descriptors
> for it.
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
Applied to the togreg branch of iio.git

Thanks,
> ---
>  drivers/iio/industrialio-core.c  |  4 ++++
>  drivers/iio/industrialio-event.c | 19 ++++++++++++++-----
>  2 files changed, 18 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index 96b35f0..5ea741c 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -1012,6 +1012,8 @@ static int iio_chrdev_open(struct inode *inode, struct file *filp)
>  	if (test_and_set_bit(IIO_BUSY_BIT_POS, &indio_dev->flags))
>  		return -EBUSY;
>  
> +	iio_device_get(indio_dev);
> +
>  	filp->private_data = indio_dev;
>  
>  	return 0;
> @@ -1025,6 +1027,8 @@ static int iio_chrdev_release(struct inode *inode, struct file *filp)
>  	struct iio_dev *indio_dev = container_of(inode->i_cdev,
>  						struct iio_dev, chrdev);
>  	clear_bit(IIO_BUSY_BIT_POS, &indio_dev->flags);
> +	iio_device_put(indio_dev);
> +
>  	return 0;
>  }
>  
> diff --git a/drivers/iio/industrialio-event.c b/drivers/iio/industrialio-event.c
> index 2390e3d..f89843b 100644
> --- a/drivers/iio/industrialio-event.c
> +++ b/drivers/iio/industrialio-event.c
> @@ -72,7 +72,8 @@ EXPORT_SYMBOL(iio_push_event);
>  static unsigned int iio_event_poll(struct file *filep,
>  			     struct poll_table_struct *wait)
>  {
> -	struct iio_event_interface *ev_int = filep->private_data;
> +	struct iio_dev *indio_dev = filep->private_data;
> +	struct iio_event_interface *ev_int = indio_dev->event_interface;
>  	unsigned int events = 0;
>  
>  	poll_wait(filep, &ev_int->wait, wait);
> @@ -90,7 +91,8 @@ static ssize_t iio_event_chrdev_read(struct file *filep,
>  				     size_t count,
>  				     loff_t *f_ps)
>  {
> -	struct iio_event_interface *ev_int = filep->private_data;
> +	struct iio_dev *indio_dev = filep->private_data;
> +	struct iio_event_interface *ev_int = indio_dev->event_interface;
>  	unsigned int copied;
>  	int ret;
>  
> @@ -121,7 +123,8 @@ error_unlock:
>  
>  static int iio_event_chrdev_release(struct inode *inode, struct file *filep)
>  {
> -	struct iio_event_interface *ev_int = filep->private_data;
> +	struct iio_dev *indio_dev = filep->private_data;
> +	struct iio_event_interface *ev_int = indio_dev->event_interface;
>  
>  	spin_lock_irq(&ev_int->wait.lock);
>  	__clear_bit(IIO_BUSY_BIT_POS, &ev_int->flags);
> @@ -133,6 +136,8 @@ static int iio_event_chrdev_release(struct inode *inode, struct file *filep)
>  	kfifo_reset_out(&ev_int->det_events);
>  	spin_unlock_irq(&ev_int->wait.lock);
>  
> +	iio_device_put(indio_dev);
> +
>  	return 0;
>  }
>  
> @@ -158,12 +163,16 @@ int iio_event_getfd(struct iio_dev *indio_dev)
>  		return -EBUSY;
>  	}
>  	spin_unlock_irq(&ev_int->wait.lock);
> -	fd = anon_inode_getfd("iio:event",
> -				&iio_event_chrdev_fileops, ev_int, O_RDONLY | O_CLOEXEC);
> +
> +	iio_device_get(indio_dev);
> +
> +	fd = anon_inode_getfd("iio:event", &iio_event_chrdev_fileops,
> +				indio_dev, O_RDONLY | O_CLOEXEC);
>  	if (fd < 0) {
>  		spin_lock_irq(&ev_int->wait.lock);
>  		__clear_bit(IIO_BUSY_BIT_POS, &ev_int->flags);
>  		spin_unlock_irq(&ev_int->wait.lock);
> +		iio_device_put(indio_dev);
>  	}
>  	return fd;
>  }
> 

Re: [PATCH 03/10] iio: Set the IIO device as the parent for the character device

[Jonathan Cameron]

On 09/18/13 21:02, Lars-Peter Clausen wrote:
> We need to make sure that the IIO device is not freed while the character device
> exists, otherwise the freeing of the IIO device might race against the file open
> callback. Do this by setting the character device's parent to the IIO device,
> this will cause the character device to grab a reference to the IIO device and
> only release it once the character device itself has been removed.
> 
> Also move the registration of the character device before the registration of
> the IIO device to avoid the (rather theoretical case) that the IIO device is
> already freed again before we can add the character device and grab a reference
> to the IIO device.
> 
> We also need to move the call to cdev_del() from iio_dev_release() to
> iio_device_unregister() (where it should have been in the first place anyway) to
> avoid a reference cycle. As iio_dev_release() is only called once all reference
> are dropped, but the character device holds a reference to the IIO device.
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
Applied to the fixes to greg branch of iio.git

Have changed the patch title to make it more apparent that this is a fix.

Now called:
iio: Prevent race between IIO chardev opening and IIO device free

with the original title as the first line of the description.

Good fix but you need to be sensational in the patch titles!


> ---
>  drivers/iio/industrialio-core.c | 21 ++++++++++++---------
>  1 file changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index 5ea741c..863aa01 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -890,8 +890,6 @@ static void iio_device_unregister_sysfs(struct iio_dev *indio_dev)
>  static void iio_dev_release(struct device *device)
>  {
>  	struct iio_dev *indio_dev = dev_to_iio_dev(device);
> -	if (indio_dev->chrdev.dev)
> -		cdev_del(&indio_dev->chrdev);
>  	if (indio_dev->modes & INDIO_BUFFER_TRIGGERED)
>  		iio_device_unregister_trigger_consumer(indio_dev);
>  	iio_device_unregister_eventset(indio_dev);
> @@ -1098,18 +1096,20 @@ int iio_device_register(struct iio_dev *indio_dev)
>  		indio_dev->setup_ops == NULL)
>  		indio_dev->setup_ops = &noop_ring_setup_ops;
>  
> -	ret = device_add(&indio_dev->dev);
> -	if (ret < 0)
> -		goto error_unreg_eventset;
>  	cdev_init(&indio_dev->chrdev, &iio_buffer_fileops);
>  	indio_dev->chrdev.owner = indio_dev->info->driver_module;
> +	indio_dev->chrdev.kobj.parent = &indio_dev->dev.kobj;
>  	ret = cdev_add(&indio_dev->chrdev, indio_dev->dev.devt, 1);
>  	if (ret < 0)
> -		goto error_del_device;
> -	return 0;
> +		goto error_unreg_eventset;
>  
> -error_del_device:
> -	device_del(&indio_dev->dev);
> +	ret = device_add(&indio_dev->dev);
> +	if (ret < 0)
> +		goto error_cdev_del;
> +
> +	return 0;
> +error_cdev_del:
> +	cdev_del(&indio_dev->chrdev);
>  error_unreg_eventset:
>  	iio_device_unregister_eventset(indio_dev);
>  error_free_sysfs:
> @@ -1127,6 +1127,9 @@ void iio_device_unregister(struct iio_dev *indio_dev)
>  
>  	device_del(&indio_dev->dev);
>  
> +	if (indio_dev->chrdev.dev)
> +		cdev_del(&indio_dev->chrdev);
> +
>  	iio_disable_all_buffers(indio_dev);
>  
>  	indio_dev->info = NULL;
> 

Re: [PATCH 04/10] iio:buffer_cb: Add missing iio_buffer_init()

[Jonathan Cameron]

On 09/18/13 21:02, Lars-Peter Clausen wrote:
> Make sure to properly initialize the IIO buffer data structure.
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
Applied to the fixes togreg branch of iio.git

As requested, stopping here in the series for now.
> ---
>  drivers/iio/buffer_cb.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/iio/buffer_cb.c b/drivers/iio/buffer_cb.c
> index 578f719..841fec1 100644
> --- a/drivers/iio/buffer_cb.c
> +++ b/drivers/iio/buffer_cb.c
> @@ -41,6 +41,8 @@ struct iio_cb_buffer *iio_channel_get_all_cb(struct device *dev,
>  		goto error_ret;
>  	}
>  
> +	iio_buffer_init(&cb_buff->buffer);
> +
>  	cb_buff->private = private;
>  	cb_buff->cb = cb;
>  	cb_buff->buffer.access = &iio_cb_access;
> 

Re: [PATCH 08/10] iio: Wakeup poll and blocking reads when the device is unregistered

[Jonathan Cameron]

On 09/18/13 21:02, 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>
This is probably the only one in the set that isn't technically a 'fix' so could you
reorder so this is at the end.  I'll then push it out once one the other patches
have made there way into the staging-next tree.

Thanks,
> ---
>  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 e9cbde3..c28625a 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 dd7b601..88a77d9 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -1140,6 +1140,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 3843abf..6aace88 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 */
>  	}
>  
> @@ -455,6 +460,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 10/10] iio:buffer: Ignore noop requests for iio_update_buffers()

[Jonathan Cameron]

On 09/18/13 21:02, 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>
> ---
>  drivers/iio/industrialio-buffer.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index 8ceaaaf..2b36cea 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -673,9 +673,21 @@ int iio_update_buffers(struct iio_dev *indio_dev,
>  {
>  	int ret;
>  
> +	if (insert_buffer == remove_buffer)
> +		return 0;
> +
Can this first case occur without a bug in the calling function?

The rest are fine.
>  	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)
> +		return 0;
> +
>  	if (indio_dev->info == NULL)
>  		return -ENODEV;
>  
> 

Re: [PATCH 09/10] iio:buffer: Add proper locking for iio_update_buffers()

[Jonathan Cameron]

On 09/18/13 21:27, Lars-Peter Clausen wrote:
> [...]
>> +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)
>> +        return -ENODEV;
> 
> Yea, ok that happens when send patches way to late in the evening. Of course we still need to unlock here...
Any reason not to do the indio_dev->info test before locking in the first place?
> 
>> +    ret = __iio_update_buffers(indio_dev, insert_buffer, remove_buffer);
>> +
>> +    mutex_unlock(&indio_dev->mlock);
>> +    mutex_unlock(&indio_dev->info_exist_lock);
>> +
>> +    return ret;
>> +}
> 

Re: [PATCH 09/10] iio:buffer: Add proper locking for iio_update_buffers()

[Jonathan Cameron]

On 09/21/13 12:05, Lars-Peter Clausen wrote:
> On 09/21/2013 01:59 PM, Jonathan Cameron wrote:
>> On 09/18/13 21:27, Lars-Peter Clausen wrote:
>>> [...]
>>>> +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)
>>>> +        return -ENODEV;
>>>
>>> Yea, ok that happens when send patches way to late in the evening. Of course we still need to unlock here...
>> Any reason not to do the indio_dev->info test before locking in the first place?
> 
> The check and the updating needs to happen in one atomic step, otherwise the updating might race against the
> iio_disable_all_buffers in the iio_device_unregister function.
Ah, gotcha.  Could you add a trivial comment for idiots like me pointing that out :)
> 
>>>
>>>> +    ret = __iio_update_buffers(indio_dev, insert_buffer, remove_buffer);
>>>> +
>>>> +    mutex_unlock(&indio_dev->mlock);
>>>> +    mutex_unlock(&indio_dev->info_exist_lock);
>>>> +
>>>> +    return ret;
>>>> +}
>>>
>> -- 
>> 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 08/10] iio: Wakeup poll and blocking reads when the device is unregistered

[Jonathan Cameron]

On 09/21/13 12:43, Lars-Peter Clausen wrote:
> On 09/21/2013 01:56 PM, Jonathan Cameron wrote:
>> On 09/18/13 21:02, 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>
>> This is probably the only one in the set that isn't technically a 'fix' so could you
>> reorder so this is at the end.  I'll then push it out once one the other patches
>> have made there way into the staging-next tree.
>>
> 
> While the last two patches are fixes, there is currently no way to trigger the bug they fix since there are no users of
> the cb_buffer yet, so strictly speaking they don't have to go into the fixes branch.

Good point.  I really ought to finish that input-iio driver sometime.
I keep hoping some keen person who actually has a use for it will pick
it up and do it for me :)

None of my boards have a screen so input devices aren't all that useful.

> 
>> Thanks,
>>> ---
>>>   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 e9cbde3..c28625a 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 dd7b601..88a77d9 100644
>>> --- a/drivers/iio/industrialio-core.c
>>> +++ b/drivers/iio/industrialio-core.c
>>> @@ -1140,6 +1140,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 3843abf..6aace88 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 */
>>>       }
>>>
>>> @@ -455,6 +460,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 08/10] iio: Wakeup poll and blocking reads when the device is unregistered

[Lars-Peter Clausen]

On 09/21/2013 02:45 PM, Jonathan Cameron wrote:
> On 09/21/13 12:43, Lars-Peter Clausen wrote:
>> On 09/21/2013 01:56 PM, Jonathan Cameron wrote:
>>> On 09/18/13 21:02, 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>
>>> This is probably the only one in the set that isn't technically a 'fix' so could you
>>> reorder so this is at the end.  I'll then push it out once one the other patches
>>> have made there way into the staging-next tree.
>>>
>>
>> While the last two patches are fixes, there is currently no way to trigger the bug they fix since there are no users of
>> the cb_buffer yet, so strictly speaking they don't have to go into the fixes branch.
> 
> Good point.  I really ought to finish that input-iio driver sometime.
> I keep hoping some keen person who actually has a use for it will pick
> it up and do it for me :)
> 
> None of my boards have a screen so input devices aren't all that useful.
> 

Actually 7/10 ("iio: Return -ENODEV for file operations if the device has
been unregistered") is not critical either. It's more in the same category
than this one, let userspace know that the device is gone and make sure no
new references can be grabbed. So only leaves the buffer reference counting
and the debugfs unregister patch. Will send those shortly and then the
remaining four patches once fixes-togreg as rippled back into togreg. Since
the last 4 patch depend on some of the earlier changes.

- Lars

>>
>>> Thanks,
>>>> ---
>>>>   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 e9cbde3..c28625a 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 dd7b601..88a77d9 100644
>>>> --- a/drivers/iio/industrialio-core.c
>>>> +++ b/drivers/iio/industrialio-core.c
>>>> @@ -1140,6 +1140,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 3843abf..6aace88 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 */
>>>>       }
>>>>
>>>> @@ -455,6 +460,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 08/10] iio: Wakeup poll and blocking reads when the device is unregistered

[Jonathan Cameron]

On 09/21/13 16:16, Lars-Peter Clausen wrote:
> On 09/21/2013 02:45 PM, Jonathan Cameron wrote:
>> On 09/21/13 12:43, Lars-Peter Clausen wrote:
>>> On 09/21/2013 01:56 PM, Jonathan Cameron wrote:
>>>> On 09/18/13 21:02, 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>
>>>> This is probably the only one in the set that isn't technically a 'fix' so could you
>>>> reorder so this is at the end.  I'll then push it out once one the other patches
>>>> have made there way into the staging-next tree.
>>>>
>>>
>>> While the last two patches are fixes, there is currently no way to trigger the bug they fix since there are no users of
>>> the cb_buffer yet, so strictly speaking they don't have to go into the fixes branch.
>>
>> Good point.  I really ought to finish that input-iio driver sometime.
>> I keep hoping some keen person who actually has a use for it will pick
>> it up and do it for me :)
>>
>> None of my boards have a screen so input devices aren't all that useful.
>>
> 
> Actually 7/10 ("iio: Return -ENODEV for file operations if the device has
> been unregistered") is not critical either. It's more in the same category
> than this one, let userspace know that the device is gone and make sure no
> new references can be grabbed. So only leaves the buffer reference counting
> and the debugfs unregister patch. Will send those shortly and then the
> remaining four patches once fixes-togreg as rippled back into togreg. Since
> the last 4 patch depend on some of the earlier changes.
> 
> - Lars
The two you just sent will have to wait for a day or so as the pull request
has gone out. Depending on when Greg pulls that one, I'lll try and get
these to him before the end of the week.

Jonathan
> 
>>>
>>>> Thanks,
>>>>> ---
>>>>>   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 e9cbde3..c28625a 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 dd7b601..88a77d9 100644
>>>>> --- a/drivers/iio/industrialio-core.c
>>>>> +++ b/drivers/iio/industrialio-core.c
>>>>> @@ -1140,6 +1140,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 3843abf..6aace88 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 */
>>>>>       }
>>>>>
>>>>> @@ -455,6 +460,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)
>>>>>
>>>
>