[PATCH v2 00/12][RESEND] iio: core,buffer: add support for multiple IIO buffers per IIO device

[PATCH v2 00/12][RESEND] iio: core,buffer: add support for multiple IIO buffers per IIO device

[Alexandru Ardelean]

Continuing from:
 https://lore.kernel.org/linux-iio/20201117162340.43924-1-alexandru.ardelean@analog.com/

Changelog v1 -> v2:
* 'iio: buffer: rework buffer & scan_elements dir creation'
  add more doc-strings detailing the reasoning for this change
* 'iio: buffer: re-route scan_elements via it's kobj_type'
  move list_del() before the kfree()'s in the list destruction
* 'iio: buffer: introduce support for attaching more IIO buffers'
  - changed to 'cnt' variable vs re-using the 'i' for unwinding in
    iio_buffer_alloc_sysfs_and_mask()
  - removed kfree(old) in iio_device_attach_buffer()
  - made iio_device_attach_buffer() an int return; this means that some
    follow up patches are needed to make this return value be used;
* 'iio: buffer: add ioctl() to support opening extra buffers for IIO device'
  - tested ioctl() with a simple C program; attached to comment;
  - changed 'i' variable usage to 'sz' for alloc
  - changed logic for buffer0; returning FD 0; userspace should know
    that the IIO_BUFFER_GET_FD_IOCTL call returns 0 for buffer0;
    this is because I can't find a way to determine the FD of the
    ioctl() in the kernel; duplicating an ioctl() for buffer0 is also bad;

Alexandru Ardelean (12):
  iio: core: register chardev only if needed
  iio: buffer: add back-ref from iio_buffer to iio_dev
  iio: buffer: rework buffer & scan_elements dir creation
  iio: buffer: add index to the first IIO buffer dir and symlink it back
  iio: core: split __iio_device_attr_init() to init only the attr object
  iio: buffer: re-route scan_elements via it's kobj_type
  iio: buffer: re-route core buffer attributes via it's new kobj_type
  iio: buffer: add helper to get the IIO device to which a buffer
    belongs
  iio: re-route all buffer attributes through new buffer kobj_type
  iio: core: wrap iio device & buffer into struct for character devices
  iio: buffer: introduce support for attaching more IIO buffers
  iio: buffer: add ioctl() to support opening extra buffers for IIO
    device

 drivers/iio/accel/adxl372.c                   |  36 +-
 drivers/iio/accel/bmc150-accel-core.c         |  34 +-
 drivers/iio/adc/at91-sama5d2_adc.c            |  30 +-
 .../buffer/industrialio-buffer-dmaengine.c    |  13 +-
 .../cros_ec_sensors/cros_ec_sensors_core.c    |  30 +-
 .../common/hid-sensors/hid-sensor-trigger.c   |  32 +-
 drivers/iio/iio_core.h                        |  11 +
 drivers/iio/industrialio-buffer.c             | 647 ++++++++++++++----
 drivers/iio/industrialio-core.c               | 117 ++--
 include/linux/iio/buffer.h                    |   6 +-
 include/linux/iio/buffer_impl.h               |  25 +-
 include/linux/iio/iio-opaque.h                |   6 +
 include/linux/iio/iio.h                       |   2 +-
 include/linux/iio/sysfs.h                     |  50 ++
 include/uapi/linux/iio/buffer.h               |  10 +
 15 files changed, 790 insertions(+), 259 deletions(-)
 create mode 100644 include/uapi/linux/iio/buffer.h

-- 
2.17.1

[PATCH v2 01/12][RESEND] iio: core: register chardev only if needed

[Alexandru Ardelean]

We only need a chardev if we need to support buffers and/or events.

With this change, a chardev will be created only if an IIO buffer is
attached OR an event_interface is configured.

Otherwise, no chardev will be created, and the IIO device will get
registered with the 'device_add()' call.

Quite a lot of IIO devices don't really need a chardev, so this is a minor
improvement to the IIO core, as the IIO device will take up (slightly)
fewer resources.

In order to not create a chardev, we mostly just need to not initialize the
indio_dev->dev.devt field. If that is un-initialized, cdev_device_add()
behaves like device_add().

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/industrialio-core.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 7db761afa578..0a6fd299a978 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -1761,6 +1761,15 @@ static const struct file_operations iio_buffer_fileops = {
 	.release = iio_chrdev_release,
 };
 
+static const struct file_operations iio_event_fileops = {
+	.owner = THIS_MODULE,
+	.llseek = noop_llseek,
+	.unlocked_ioctl = iio_ioctl,
+	.compat_ioctl = compat_ptr_ioctl,
+	.open = iio_chrdev_open,
+	.release = iio_chrdev_release,
+};
+
 static int iio_check_unique_scan_index(struct iio_dev *indio_dev)
 {
 	int i, j;
@@ -1788,6 +1797,7 @@ static const struct iio_buffer_setup_ops noop_ring_setup_ops;
 
 int __iio_device_register(struct iio_dev *indio_dev, struct module *this_mod)
 {
+	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
 	int ret;
 
 	if (!indio_dev->info)
@@ -1805,9 +1815,6 @@ int __iio_device_register(struct iio_dev *indio_dev, struct module *this_mod)
 	if (ret < 0)
 		return ret;
 
-	/* configure elements for the chrdev */
-	indio_dev->dev.devt = MKDEV(MAJOR(iio_devt), indio_dev->id);
-
 	iio_device_register_debugfs(indio_dev);
 
 	ret = iio_buffer_alloc_sysfs_and_mask(indio_dev);
@@ -1836,9 +1843,15 @@ int __iio_device_register(struct iio_dev *indio_dev, struct module *this_mod)
 		indio_dev->setup_ops == NULL)
 		indio_dev->setup_ops = &noop_ring_setup_ops;
 
-	cdev_init(&indio_dev->chrdev, &iio_buffer_fileops);
+	if (indio_dev->buffer)
+		cdev_init(&indio_dev->chrdev, &iio_buffer_fileops);
+	else if (iio_dev_opaque->event_interface)
+		cdev_init(&indio_dev->chrdev, &iio_event_fileops);
 
-	indio_dev->chrdev.owner = this_mod;
+	if (indio_dev->buffer || iio_dev_opaque->event_interface) {
+		indio_dev->dev.devt = MKDEV(MAJOR(iio_devt), indio_dev->id);
+		indio_dev->chrdev.owner = this_mod;
+	}
 
 	ret = cdev_device_add(&indio_dev->chrdev, &indio_dev->dev);
 	if (ret < 0)
-- 
2.17.1

[PATCH v2 02/12][RESEND] iio: buffer: add back-ref from iio_buffer to iio_dev

[Alexandru Ardelean]

An IIO device will have multiple buffers, but it shouldn't be allowed that
an IIO buffer should belong to more than 1 IIO device.

Once things get moved more from IIO device to the IIO buffer, and an IIO
device will be able to have more than 1 buffer attached, there will be a
need for a back-ref to the IIO device [from the IIO buffer].

This change adds that.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/industrialio-buffer.c | 2 ++
 include/linux/iio/buffer_impl.h   | 3 +++
 2 files changed, 5 insertions(+)

diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index 2f7426a2f47c..0412c4fda4c1 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -1505,5 +1505,7 @@ void iio_device_attach_buffer(struct iio_dev *indio_dev,
 			      struct iio_buffer *buffer)
 {
 	indio_dev->buffer = iio_buffer_get(buffer);
+
+	indio_dev->buffer->indio_dev = indio_dev;
 }
 EXPORT_SYMBOL_GPL(iio_device_attach_buffer);
diff --git a/include/linux/iio/buffer_impl.h b/include/linux/iio/buffer_impl.h
index a63dc07b7350..67d73d465e02 100644
--- a/include/linux/iio/buffer_impl.h
+++ b/include/linux/iio/buffer_impl.h
@@ -69,6 +69,9 @@ struct iio_buffer_access_funcs {
  * those writing new buffer implementations.
  */
 struct iio_buffer {
+	/** @indio_dev: IIO device to which this buffer belongs to. */
+	struct iio_dev *indio_dev;
+
 	/** @length: Number of datums in buffer. */
 	unsigned int length;
 
-- 
2.17.1

[PATCH v2 03/12][RESEND] iio: buffer: rework buffer & scan_elements dir creation

[Alexandru Ardelean]

When adding more than one IIO buffer per IIO device, we will need to create
a buffer & scan_elements directory for each buffer.
We also want to move the 'scan_elements' to be a sub-directory of the
'buffer' folder.

The format we want to reach is, for a iio:device0 folder, for 2 buffers
[for example], we have a 'buffer0' and a 'buffer1' subfolder, and each with
it's own 'scan_elements' subfolder.

So, for example:
   iio:device0/buffer0
      scan_elements/

   iio:device0/buffer1
      scan_elements/

The other attributes under 'bufferX' would remain unchanged.

However, we would also need to symlink back to the old 'buffer' &
'scan_elements' folders, to keep backwards compatibility.

Doing all these, require that we maintain the kobjects for each 'bufferX'
and 'scan_elements' so that we can symlink them back. We also need to
implement the sysfs_ops for these folders.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/industrialio-buffer.c | 195 +++++++++++++++++++++++++++---
 drivers/iio/industrialio-core.c   |  24 ++--
 include/linux/iio/buffer_impl.h   |  14 ++-
 include/linux/iio/iio.h           |   2 +-
 4 files changed, 200 insertions(+), 35 deletions(-)

diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index 0412c4fda4c1..0f470d902790 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -1175,8 +1175,6 @@ static ssize_t iio_buffer_store_enable(struct device *dev,
 	return (ret < 0) ? ret : len;
 }
 
-static const char * const iio_scan_elements_group_name = "scan_elements";
-
 static ssize_t iio_buffer_show_watermark(struct device *dev,
 					 struct device_attribute *attr,
 					 char *buf)
@@ -1252,6 +1250,124 @@ static struct attribute *iio_buffer_attrs[] = {
 	&dev_attr_data_available.attr,
 };
 
+#define to_dev_attr(_attr) container_of(_attr, struct device_attribute, attr)
+
+static ssize_t iio_buffer_dir_attr_show(struct kobject *kobj,
+					struct attribute *attr,
+					char *buf)
+{
+	struct iio_buffer *buffer = container_of(kobj, struct iio_buffer, buffer_dir);
+	struct device_attribute *dattr;
+
+	dattr = to_dev_attr(attr);
+
+	return dattr->show(&buffer->indio_dev->dev, dattr, buf);
+}
+
+static ssize_t iio_buffer_dir_attr_store(struct kobject *kobj,
+					 struct attribute *attr,
+					 const char *buf,
+					 size_t len)
+{
+	struct iio_buffer *buffer = container_of(kobj, struct iio_buffer, buffer_dir);
+	struct device_attribute *dattr;
+
+	dattr = to_dev_attr(attr);
+
+	return dattr->store(&buffer->indio_dev->dev, dattr, buf, len);
+}
+
+static const struct sysfs_ops iio_buffer_dir_sysfs_ops = {
+	.show = iio_buffer_dir_attr_show,
+	.store = iio_buffer_dir_attr_store,
+};
+
+static struct kobj_type iio_buffer_dir_ktype = {
+	.sysfs_ops = &iio_buffer_dir_sysfs_ops,
+};
+
+static ssize_t iio_scan_el_dir_attr_show(struct kobject *kobj,
+					 struct attribute *attr,
+					 char *buf)
+{
+	struct iio_buffer *buffer = container_of(kobj, struct iio_buffer, scan_el_dir);
+	struct device_attribute *dattr = to_dev_attr(attr);
+
+	return dattr->show(&buffer->indio_dev->dev, dattr, buf);
+}
+
+static ssize_t iio_scan_el_dir_attr_store(struct kobject *kobj,
+					  struct attribute *attr,
+					  const char *buf,
+					  size_t len)
+{
+	struct iio_buffer *buffer = container_of(kobj, struct iio_buffer, scan_el_dir);
+	struct device_attribute *dattr = to_dev_attr(attr);
+
+	return dattr->store(&buffer->indio_dev->dev, dattr, buf, len);
+}
+
+static const struct sysfs_ops iio_scan_el_dir_sysfs_ops = {
+	.show = iio_scan_el_dir_attr_show,
+	.store = iio_scan_el_dir_attr_store,
+};
+
+static struct kobj_type iio_scan_el_dir_ktype = {
+	.sysfs_ops = &iio_scan_el_dir_sysfs_ops,
+};
+
+/*
+ * These iio_sysfs_{add,del}_attrs() are essentially re-implementations of
+ * sysfs_create_files() & sysfs_remove_files(), but they are meant to get
+ * around the const-pointer mismatch situation with using them.
+ *
+ * sysfs_{create,remove}_files() uses 'const struct attribute * const *ptr',
+ * while these are happy with just 'struct attribute **ptr'
+ */
+static int iio_sysfs_add_attrs(struct kobject *kobj, struct attribute **ptr)
+{
+	int err = 0;
+	int i;
+
+	for (i = 0; ptr[i] && !err; i++)
+		err = sysfs_create_file(kobj, ptr[i]);
+	if (err)
+		while (--i >= 0)
+			sysfs_remove_file(kobj, ptr[i]);
+	return err;
+}
+
+static void iio_sysfs_del_attrs(struct kobject *kobj, struct attribute **ptr)
+{
+	int i;
+
+	for (i = 0; ptr[i]; i++)
+		sysfs_remove_file(kobj, ptr[i]);
+}
+
+/**
+ * __iio_buffer_alloc_sysfs_and_mask() - Allocate sysfs attributes to an attached buffer
+ * @buffer: the buffer object for which the sysfs attributes are created for
+ * @indio_dev: the iio device to which the iio buffer belongs to
+ *
+ * Return 0, or negative for error.
+ *
+ * This function must be called for each single buffer. The sysfs attributes for that
+ * buffer will be created, and the IIO device object will be the parent kobject of that
+ * the kobjects created here.
+ * Because we need to redirect sysfs attribute to it's IIO buffer object, we need to
+ * implement our own sysfs_ops, and each IIO buffer will keep a kobject for the
+ * 'bufferX' directory and one for the 'scan_elements' directory.
+ * And in order to do this, this function must be called after the IIO device object
+ * has been added via device_add(). This fundamentally changes how sysfs attributes
+ * were created before (with one single IIO buffer per IIO device), where the
+ * sysfs attributes for the buffer were mapped as attribute groups on the IIO device
+ * groups object list.
+ * Using kobjects directly for the 'bufferX' and 'scan_elements' directories allows
+ * us to symlink them back to keep backwards compatibility for the old sysfs interface
+ * for IIO buffers while also allowing us to support multiple IIO buffers per one
+ * IIO device.
+ */
 static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
 					     struct iio_dev *indio_dev)
 {
@@ -1282,12 +1398,16 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
 		memcpy(&attr[ARRAY_SIZE(iio_buffer_attrs)], buffer->attrs,
 		       sizeof(struct attribute *) * attrcount);
 
-	attr[attrcount + ARRAY_SIZE(iio_buffer_attrs)] = NULL;
+	buffer->buffer_attrs = attr;
 
-	buffer->buffer_group.name = "buffer";
-	buffer->buffer_group.attrs = attr;
+	ret = kobject_init_and_add(&buffer->buffer_dir, &iio_buffer_dir_ktype,
+				   &indio_dev->dev.kobj, "buffer");
+	if (ret)
+		goto error_buffer_free_attrs;
 
-	indio_dev->groups[indio_dev->groupcounter++] = &buffer->buffer_group;
+	ret = iio_sysfs_add_attrs(&buffer->buffer_dir, buffer->buffer_attrs);
+	if (ret)
+		goto error_buffer_kobject_put;
 
 	attrcount = 0;
 	INIT_LIST_HEAD(&buffer->scan_el_dev_attr_list);
@@ -1317,32 +1437,57 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
 		}
 	}
 
-	buffer->scan_el_group.name = iio_scan_elements_group_name;
-
-	buffer->scan_el_group.attrs = kcalloc(attrcount + 1,
-					      sizeof(buffer->scan_el_group.attrs[0]),
-					      GFP_KERNEL);
-	if (buffer->scan_el_group.attrs == NULL) {
+	buffer->scan_el_attrs = kcalloc(attrcount + 1,
+					sizeof(buffer->scan_el_attrs[0]),
+					GFP_KERNEL);
+	if (buffer->scan_el_attrs == NULL) {
 		ret = -ENOMEM;
 		goto error_free_scan_mask;
 	}
-	attrn = 0;
 
+	ret = kobject_init_and_add(&buffer->scan_el_dir, &iio_scan_el_dir_ktype,
+				   &indio_dev->dev.kobj, "scan_elements");
+	if (ret)
+		goto error_free_scan_attrs;
+
+	attrn = 0;
 	list_for_each_entry(p, &buffer->scan_el_dev_attr_list, l)
-		buffer->scan_el_group.attrs[attrn++] = &p->dev_attr.attr;
-	indio_dev->groups[indio_dev->groupcounter++] = &buffer->scan_el_group;
+		buffer->scan_el_attrs[attrn++] = &p->dev_attr.attr;
+
+	ret = iio_sysfs_add_attrs(&buffer->scan_el_dir, buffer->scan_el_attrs);
+	if (ret)
+		goto error_scan_kobject_put;
 
 	return 0;
 
+error_scan_kobject_put:
+	kobject_put(&buffer->scan_el_dir);
+error_free_scan_attrs:
+	kfree(buffer->scan_el_attrs);
 error_free_scan_mask:
 	bitmap_free(buffer->scan_mask);
 error_cleanup_dynamic:
+	iio_sysfs_del_attrs(&buffer->buffer_dir, buffer->buffer_attrs);
 	iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list);
-	kfree(buffer->buffer_group.attrs);
+error_buffer_kobject_put:
+	kobject_put(&buffer->buffer_dir);
+error_buffer_free_attrs:
+	kfree(buffer->buffer_attrs);
 
 	return ret;
 }
 
+/**
+ * iio_buffer_alloc_sysfs_and_mask() - Allocate sysfs attributes to attached buffers
+ * @indio_dev: the iio device for which to create the buffer sysfs attributes
+ *
+ * Return 0, or negative for error.
+ *
+ * If the IIO device has no buffer attached, no sysfs attributes will be created.
+ * This function must be called after the IIO device object has been created and
+ * registered with device_add(). See __iio_buffer_alloc_sysfs_and_mask() for more
+ * details.
+ */
 int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
 {
 	struct iio_buffer *buffer = indio_dev->buffer;
@@ -1364,14 +1509,28 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
 	return __iio_buffer_alloc_sysfs_and_mask(buffer, indio_dev);
 }
 
+/**
+ * __iio_buffer_free_sysfs_and_mask() - Free sysfs objects for a single IIO buffer
+ * @buffer: the iio buffer for which to destroy the objects
+ */
 static void __iio_buffer_free_sysfs_and_mask(struct iio_buffer *buffer)
 {
+	iio_sysfs_del_attrs(&buffer->scan_el_dir, buffer->scan_el_attrs);
+	kobject_put(&buffer->scan_el_dir);
+	kfree(buffer->scan_el_attrs);
 	bitmap_free(buffer->scan_mask);
-	kfree(buffer->buffer_group.attrs);
-	kfree(buffer->scan_el_group.attrs);
+	iio_sysfs_del_attrs(&buffer->buffer_dir, buffer->buffer_attrs);
 	iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list);
+	kobject_put(&buffer->buffer_dir);
+	kfree(buffer->buffer_attrs);
 }
 
+/**
+ * iio_buffer_free_sysfs_and_mask() - Free sysfs objects for all IIO buffers
+ * @indio_dev: the iio device for which to destroy the objects
+ *
+ * If the IIO device has no buffer attached, nothing will be done.
+ */
 void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)
 {
 	struct iio_buffer *buffer = indio_dev->buffer;
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 0a6fd299a978..95d66745f118 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -1817,18 +1817,11 @@ int __iio_device_register(struct iio_dev *indio_dev, struct module *this_mod)
 
 	iio_device_register_debugfs(indio_dev);
 
-	ret = iio_buffer_alloc_sysfs_and_mask(indio_dev);
-	if (ret) {
-		dev_err(indio_dev->dev.parent,
-			"Failed to create buffer sysfs interfaces\n");
-		goto error_unreg_debugfs;
-	}
-
 	ret = iio_device_register_sysfs(indio_dev);
 	if (ret) {
 		dev_err(indio_dev->dev.parent,
 			"Failed to register sysfs interfaces\n");
-		goto error_buffer_free_sysfs;
+		goto error_unreg_debugfs;
 	}
 	ret = iio_device_register_eventset(indio_dev);
 	if (ret) {
@@ -1857,14 +1850,21 @@ int __iio_device_register(struct iio_dev *indio_dev, struct module *this_mod)
 	if (ret < 0)
 		goto error_unreg_eventset;
 
+	ret = iio_buffer_alloc_sysfs_and_mask(indio_dev);
+	if (ret) {
+		dev_err(indio_dev->dev.parent,
+			"Failed to create buffer sysfs interfaces\n");
+		goto error_device_del;
+	}
+
 	return 0;
 
+error_device_del:
+	cdev_device_del(&indio_dev->chrdev, &indio_dev->dev);
 error_unreg_eventset:
 	iio_device_unregister_eventset(indio_dev);
 error_free_sysfs:
 	iio_device_unregister_sysfs(indio_dev);
-error_buffer_free_sysfs:
-	iio_buffer_free_sysfs_and_mask(indio_dev);
 error_unreg_debugfs:
 	iio_device_unregister_debugfs(indio_dev);
 	return ret;
@@ -1880,6 +1880,8 @@ void iio_device_unregister(struct iio_dev *indio_dev)
 	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
 	struct iio_ioctl_handler *h, *t;
 
+	iio_buffer_free_sysfs_and_mask(indio_dev);
+
 	cdev_device_del(&indio_dev->chrdev, &indio_dev->dev);
 
 	mutex_lock(&indio_dev->info_exist_lock);
@@ -1897,8 +1899,6 @@ void iio_device_unregister(struct iio_dev *indio_dev)
 	iio_buffer_wakeup_poll(indio_dev);
 
 	mutex_unlock(&indio_dev->info_exist_lock);
-
-	iio_buffer_free_sysfs_and_mask(indio_dev);
 }
 EXPORT_SYMBOL(iio_device_unregister);
 
diff --git a/include/linux/iio/buffer_impl.h b/include/linux/iio/buffer_impl.h
index 67d73d465e02..77e169e51434 100644
--- a/include/linux/iio/buffer_impl.h
+++ b/include/linux/iio/buffer_impl.h
@@ -103,14 +103,20 @@ struct iio_buffer {
 	/* @scan_el_dev_attr_list: List of scan element related attributes. */
 	struct list_head scan_el_dev_attr_list;
 
-	/* @buffer_group: Attributes of the buffer group. */
-	struct attribute_group buffer_group;
+	/* @buffer_dir: kobject for the 'buffer' directory of this buffer */
+	struct kobject buffer_dir;
+
+	/* @buffer_attrs: Attributes of the buffer group. */
+	struct attribute **buffer_attrs;
+
+	/* @scan_el_dir: kobject for the 'scan_elements' directory of this buffer */
+	struct kobject scan_el_dir;
 
 	/*
-	 * @scan_el_group: Attribute group for those attributes not
+	 * @scan_el_attrs: Array of attributes for those attributes not
 	 * created from the iio_chan_info array.
 	 */
-	struct attribute_group scan_el_group;
+	struct attribute **scan_el_attrs;
 
 	/* @attrs: Standard attributes of the buffer. */
 	const struct attribute **attrs;
diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index e4a9822e6495..59b317dc45b8 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -556,7 +556,7 @@ struct iio_dev {
 	struct mutex			info_exist_lock;
 	const struct iio_buffer_setup_ops	*setup_ops;
 	struct cdev			chrdev;
-#define IIO_MAX_GROUPS 6
+#define IIO_MAX_GROUPS 4
 	const struct attribute_group	*groups[IIO_MAX_GROUPS + 1];
 	int				groupcounter;
 
-- 
2.17.1

[PATCH v2 04/12][RESEND] iio: buffer: add index to the first IIO buffer dir and symlink it back

[Alexandru Ardelean]

This change makes it so that the first buffer directory is named 'buffer0'
and moves the 'scan_elements' under it.

For backwards compatibility these folders are symlinked back to the
original folders.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/industrialio-buffer.c | 66 +++++++++++++++++++++----------
 1 file changed, 46 insertions(+), 20 deletions(-)

diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index 0f470d902790..628d78125126 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -1369,7 +1369,8 @@ static void iio_sysfs_del_attrs(struct kobject *kobj, struct attribute **ptr)
  * IIO device.
  */
 static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
-					     struct iio_dev *indio_dev)
+					     struct iio_dev *indio_dev,
+					     unsigned int idx)
 {
 	struct iio_dev_attr *p;
 	struct attribute **attr;
@@ -1401,7 +1402,7 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
 	buffer->buffer_attrs = attr;
 
 	ret = kobject_init_and_add(&buffer->buffer_dir, &iio_buffer_dir_ktype,
-				   &indio_dev->dev.kobj, "buffer");
+				   &indio_dev->dev.kobj, "buffer%u", idx);
 	if (ret)
 		goto error_buffer_free_attrs;
 
@@ -1446,7 +1447,7 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
 	}
 
 	ret = kobject_init_and_add(&buffer->scan_el_dir, &iio_scan_el_dir_ktype,
-				   &indio_dev->dev.kobj, "scan_elements");
+				   &buffer->buffer_dir, "scan_elements");
 	if (ret)
 		goto error_free_scan_attrs;
 
@@ -1477,6 +1478,22 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
 	return ret;
 }
 
+/**
+ * __iio_buffer_free_sysfs_and_mask() - Free sysfs objects for a single IIO buffer
+ * @buffer: the iio buffer for which to destroy the objects
+ */
+static void __iio_buffer_free_sysfs_and_mask(struct iio_buffer *buffer)
+{
+	iio_sysfs_del_attrs(&buffer->scan_el_dir, buffer->scan_el_attrs);
+	kobject_put(&buffer->scan_el_dir);
+	kfree(buffer->scan_el_attrs);
+	bitmap_free(buffer->scan_mask);
+	iio_sysfs_del_attrs(&buffer->buffer_dir, buffer->buffer_attrs);
+	iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list);
+	kobject_put(&buffer->buffer_dir);
+	kfree(buffer->buffer_attrs);
+}
+
 /**
  * iio_buffer_alloc_sysfs_and_mask() - Allocate sysfs attributes to attached buffers
  * @indio_dev: the iio device for which to create the buffer sysfs attributes
@@ -1492,7 +1509,7 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
 {
 	struct iio_buffer *buffer = indio_dev->buffer;
 	const struct iio_chan_spec *channels;
-	int i;
+	int i, ret;
 
 	channels = indio_dev->channels;
 	if (channels) {
@@ -1506,23 +1523,29 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
 	if (!buffer)
 		return 0;
 
-	return __iio_buffer_alloc_sysfs_and_mask(buffer, indio_dev);
-}
+	ret = __iio_buffer_alloc_sysfs_and_mask(buffer, indio_dev, 0);
+	if (ret)
+		return ret;
 
-/**
- * __iio_buffer_free_sysfs_and_mask() - Free sysfs objects for a single IIO buffer
- * @buffer: the iio buffer for which to destroy the objects
- */
-static void __iio_buffer_free_sysfs_and_mask(struct iio_buffer *buffer)
-{
-	iio_sysfs_del_attrs(&buffer->scan_el_dir, buffer->scan_el_attrs);
-	kobject_put(&buffer->scan_el_dir);
-	kfree(buffer->scan_el_attrs);
-	bitmap_free(buffer->scan_mask);
-	iio_sysfs_del_attrs(&buffer->buffer_dir, buffer->buffer_attrs);
-	iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list);
-	kobject_put(&buffer->buffer_dir);
-	kfree(buffer->buffer_attrs);
+	ret = sysfs_create_link(&indio_dev->dev.kobj,
+				&indio_dev->buffer->buffer_dir,
+				"buffer");
+	if (ret)
+		goto error_free_sysfs_and_mask;
+
+	ret = sysfs_create_link(&indio_dev->dev.kobj,
+				&indio_dev->buffer->scan_el_dir,
+				"scan_elements");
+	if (ret)
+		goto error_remove_buffer_dir_link;
+
+	return 0;
+
+error_remove_buffer_dir_link:
+	sysfs_remove_link(&indio_dev->dev.kobj, "buffer");
+error_free_sysfs_and_mask:
+	__iio_buffer_free_sysfs_and_mask(buffer);
+	return ret;
 }
 
 /**
@@ -1538,6 +1561,9 @@ void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)
 	if (!buffer)
 		return;
 
+	sysfs_remove_link(&indio_dev->dev.kobj, "scan_elements");
+	sysfs_remove_link(&indio_dev->dev.kobj, "buffer");
+
 	__iio_buffer_free_sysfs_and_mask(buffer);
 }
 
-- 
2.17.1

[PATCH v2 05/12][RESEND] iio: core: split __iio_device_attr_init() to init only the attr object

[Alexandru Ardelean]

The __iio_device_attr_init() function initializes a device_attribute
object, but mostly it just does a lot of name creation logic.

We will want to re-use this logic for name-creation, so this change
re-purposes the __iio_device_attr_init() to be a __iio_attr_init() function
which just handles the creation of the attribute name.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/industrialio-core.c | 43 +++++++++++++--------------------
 1 file changed, 17 insertions(+), 26 deletions(-)

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 95d66745f118..b8f7261945f5 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -968,22 +968,15 @@ static ssize_t iio_write_channel_info(struct device *dev,
 }
 
 static
-int __iio_device_attr_init(struct device_attribute *dev_attr,
-			   const char *postfix,
-			   struct iio_chan_spec const *chan,
-			   ssize_t (*readfunc)(struct device *dev,
-					       struct device_attribute *attr,
-					       char *buf),
-			   ssize_t (*writefunc)(struct device *dev,
-						struct device_attribute *attr,
-						const char *buf,
-						size_t len),
-			   enum iio_shared_by shared_by)
+int iio_attr_init(struct attribute *attr,
+		  const char *postfix,
+		  struct iio_chan_spec const *chan,
+		  enum iio_shared_by shared_by)
 {
 	int ret = 0;
 	char *name = NULL;
 	char *full_postfix;
-	sysfs_attr_init(&dev_attr->attr);
+	sysfs_attr_init(attr);
 
 	/* Build up postfix of <extend_name>_<modifier>_postfix */
 	if (chan->modified && (shared_by == IIO_SEPARATE)) {
@@ -1079,17 +1072,7 @@ int __iio_device_attr_init(struct device_attribute *dev_attr,
 		ret = -ENOMEM;
 		goto error_free_full_postfix;
 	}
-	dev_attr->attr.name = name;
-
-	if (readfunc) {
-		dev_attr->attr.mode |= S_IRUGO;
-		dev_attr->show = readfunc;
-	}
-
-	if (writefunc) {
-		dev_attr->attr.mode |= S_IWUSR;
-		dev_attr->store = writefunc;
-	}
+	attr->name = name;
 
 error_free_full_postfix:
 	kfree(full_postfix);
@@ -1122,9 +1105,7 @@ int __iio_add_chan_devattr(const char *postfix,
 	iio_attr = kzalloc(sizeof(*iio_attr), GFP_KERNEL);
 	if (iio_attr == NULL)
 		return -ENOMEM;
-	ret = __iio_device_attr_init(&iio_attr->dev_attr,
-				     postfix, chan,
-				     readfunc, writefunc, shared_by);
+	ret = iio_attr_init(&iio_attr->dev_attr.attr, postfix, chan, shared_by);
 	if (ret)
 		goto error_iio_dev_attr_free;
 	iio_attr->c = chan;
@@ -1140,6 +1121,16 @@ int __iio_add_chan_devattr(const char *postfix,
 		}
 	list_add(&iio_attr->l, attr_list);
 
+	if (readfunc) {
+		iio_attr->dev_attr.attr.mode |= S_IRUGO;
+		iio_attr->dev_attr.show = readfunc;
+	}
+
+	if (writefunc) {
+		iio_attr->dev_attr.attr.mode |= S_IWUSR;
+		iio_attr->dev_attr.store = writefunc;
+	}
+
 	return 0;
 
 error_device_attr_deinit:
-- 
2.17.1

[PATCH v2 06/12][RESEND] iio: buffer: re-route scan_elements via it's kobj_type

[Alexandru Ardelean]

The scan_elements attributes are solely located inside
'industrialio-buffer-sysfs.c'. In order to support more than one buffer per
IIO device, we need to expand scan_elements attributes directly to IIO
buffer object, and not the IIO device.

This also requires that a new 'iio_buffer_attr' type be added which is
mostly a copy of 'iio_dev_attr', but this expands to an IIO buffer object.

The 'iio_dev_attr' type could have been re-used here, but managing 'device'
objects is a bit more tricky (than it looks at first). A 'device' object
needs to be initialized & managed and we only need to the 'kobj' to expand
from the 'bufferX' directory back to an IIO buffer.
kobjects are simpler to manage.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/iio_core.h            |   5 +
 drivers/iio/industrialio-buffer.c | 160 +++++++++++++++++++++++-------
 drivers/iio/industrialio-core.c   |   1 -
 3 files changed, 128 insertions(+), 38 deletions(-)

diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h
index fced02cadcc3..43d44ec92781 100644
--- a/drivers/iio/iio_core.h
+++ b/drivers/iio/iio_core.h
@@ -31,6 +31,11 @@ void iio_device_ioctl_handler_register(struct iio_dev *indio_dev,
 				       struct iio_ioctl_handler *h);
 void iio_device_ioctl_handler_unregister(struct iio_ioctl_handler *h);
 
+int iio_attr_init(struct attribute *attr,
+		  const char *postfix,
+		  struct iio_chan_spec const *chan,
+		  enum iio_shared_by shared_by);
+
 int __iio_add_chan_devattr(const char *postfix,
 			   struct iio_chan_spec const *chan,
 			   ssize_t (*func)(struct device *dev,
diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index 628d78125126..524b897a1877 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -26,6 +26,26 @@
 #include <linux/iio/buffer.h>
 #include <linux/iio/buffer_impl.h>
 
+/**
+ * struct iio_buf_attr - iio buffer specific attribute
+ * @attr:	underlying attribute
+ * @address:	associated register address
+ * @l:		list head for maintaining list of dynamically created attrs
+ * @c:		specification for the underlying channel
+ * @show:	sysfs show hook for this attribute
+ * @store:	sysfs store hook for this attribute
+ */
+struct iio_buf_attr {
+	struct attribute attr;
+	u64 address;
+	struct list_head l;
+	struct iio_chan_spec const *c;
+	ssize_t (*show)(struct iio_buffer *buffer, struct iio_buf_attr *attr,
+			char *buf);
+	ssize_t (*store)(struct iio_buffer *buffer, struct iio_buf_attr *attr,
+			 const char *buf, size_t count);
+};
+
 static const char * const iio_endian_prefix[] = {
 	[IIO_BE] = "be",
 	[IIO_LE] = "le",
@@ -210,18 +230,17 @@ void iio_buffer_init(struct iio_buffer *buffer)
 }
 EXPORT_SYMBOL(iio_buffer_init);
 
-static ssize_t iio_show_scan_index(struct device *dev,
-				   struct device_attribute *attr,
+static ssize_t iio_show_scan_index(struct iio_buffer *buffer,
+				   struct iio_buf_attr *attr,
 				   char *buf)
 {
-	return sprintf(buf, "%u\n", to_iio_dev_attr(attr)->c->scan_index);
+	return sprintf(buf, "%u\n", attr->c->scan_index);
 }
 
-static ssize_t iio_show_fixed_type(struct device *dev,
-				   struct device_attribute *attr,
+static ssize_t iio_show_fixed_type(struct iio_buffer *buffer,
+				   struct iio_buf_attr *this_attr,
 				   char *buf)
 {
-	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
 	u8 type = this_attr->c->scan_type.endianness;
 
 	if (type == IIO_CPU) {
@@ -248,17 +267,14 @@ static ssize_t iio_show_fixed_type(struct device *dev,
 		       this_attr->c->scan_type.shift);
 }
 
-static ssize_t iio_scan_el_show(struct device *dev,
-				struct device_attribute *attr,
+static ssize_t iio_scan_el_show(struct iio_buffer *buffer,
+				struct iio_buf_attr *attr,
 				char *buf)
 {
 	int ret;
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	struct iio_buffer *buffer = indio_dev->buffer;
 
 	/* Ensure ret is 0 or 1. */
-	ret = !!test_bit(to_iio_dev_attr(attr)->address,
-		       buffer->scan_mask);
+	ret = !!test_bit(attr->address, buffer->scan_mask);
 
 	return sprintf(buf, "%d\n", ret);
 }
@@ -359,16 +375,14 @@ static int iio_scan_mask_query(struct iio_dev *indio_dev,
 	return !!test_bit(bit, buffer->scan_mask);
 };
 
-static ssize_t iio_scan_el_store(struct device *dev,
-				 struct device_attribute *attr,
+static ssize_t iio_scan_el_store(struct iio_buffer *buffer,
+				 struct iio_buf_attr *this_attr,
 				 const char *buf,
 				 size_t len)
 {
+	struct iio_dev *indio_dev = buffer->indio_dev;
 	int ret;
 	bool state;
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	struct iio_buffer *buffer = indio_dev->buffer;
-	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
 
 	ret = strtobool(buf, &state);
 	if (ret < 0)
@@ -398,24 +412,20 @@ static ssize_t iio_scan_el_store(struct device *dev,
 
 }
 
-static ssize_t iio_scan_el_ts_show(struct device *dev,
-				   struct device_attribute *attr,
+static ssize_t iio_scan_el_ts_show(struct iio_buffer *buffer,
+				   struct iio_buf_attr *attr,
 				   char *buf)
 {
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	struct iio_buffer *buffer = indio_dev->buffer;
-
 	return sprintf(buf, "%d\n", buffer->scan_timestamp);
 }
 
-static ssize_t iio_scan_el_ts_store(struct device *dev,
-				    struct device_attribute *attr,
+static ssize_t iio_scan_el_ts_store(struct iio_buffer *buffer,
+				    struct iio_buf_attr *attr,
 				    const char *buf,
 				    size_t len)
 {
+	struct iio_dev *indio_dev = buffer->indio_dev;
 	int ret;
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	struct iio_buffer *buffer = indio_dev->buffer;
 	bool state;
 
 	ret = strtobool(buf, &state);
@@ -434,13 +444,88 @@ static ssize_t iio_scan_el_ts_store(struct device *dev,
 	return ret ? ret : len;
 }
 
+static int __iio_add_chan_bufattr(const char *postfix,
+				  struct iio_chan_spec const *chan,
+				  ssize_t (*readfunc)(struct iio_buffer *buffer,
+						      struct iio_buf_attr *attr,
+						      char *buf),
+				  ssize_t (*writefunc)(struct iio_buffer *buffer,
+						       struct iio_buf_attr *attr,
+						       const char *buf,
+						       size_t len),
+				  u64 mask,
+				  enum iio_shared_by shared_by,
+				  struct device *dev,
+				  struct list_head *attr_list)
+{
+	struct iio_buf_attr *iio_attr, *t;
+	int ret;
+
+	iio_attr = kzalloc(sizeof(*iio_attr), GFP_KERNEL);
+	if (iio_attr == NULL)
+		return -ENOMEM;
+
+	ret = iio_attr_init(&iio_attr->attr, postfix, chan, shared_by);
+	if (ret)
+		goto error_iio_buf_attr_free;
+
+	iio_attr->c = chan;
+	iio_attr->address = mask;
+	list_for_each_entry(t, attr_list, l) {
+		if (strcmp(t->attr.name, iio_attr->attr.name) == 0) {
+			if (shared_by == IIO_SEPARATE)
+				dev_err(dev, "tried to double register : %s\n",
+					t->attr.name);
+			ret = -EBUSY;
+			goto error_iio_buf_attr_deinit;
+		}
+	}
+	list_add(&iio_attr->l, attr_list);
+
+	if (readfunc) {
+		iio_attr->attr.mode |= S_IRUGO;
+		iio_attr->show = readfunc;
+	}
+
+	if (writefunc) {
+		iio_attr->attr.mode |= S_IWUSR;
+		iio_attr->store = writefunc;
+	}
+
+	return 0;
+
+error_iio_buf_attr_deinit:
+	kfree(iio_attr->attr.name);
+error_iio_buf_attr_free:
+	kfree(iio_attr);
+	return ret;
+}
+
+/**
+ * iio_free_chan_bufattr_list() - Free a list of IIO buffer attributes
+ * @attr_list: List of IIO buffer attributes
+ *
+ * This function frees the memory allocated for each of the IIO buffer
+ * attributes in the list.
+ */
+static void iio_free_chan_bufattr_list(struct list_head *attr_list)
+{
+	struct iio_buf_attr *p, *n;
+
+	list_for_each_entry_safe(p, n, attr_list, l) {
+		list_del(&p->l);
+		kfree(p->attr.name);
+		kfree(p);
+	}
+}
+
 static int iio_buffer_add_channel_sysfs(struct iio_dev *indio_dev,
 					struct iio_buffer *buffer,
 					const struct iio_chan_spec *chan)
 {
 	int ret, attrcount = 0;
 
-	ret = __iio_add_chan_devattr("index",
+	ret = __iio_add_chan_bufattr("index",
 				     chan,
 				     &iio_show_scan_index,
 				     NULL,
@@ -451,7 +536,7 @@ static int iio_buffer_add_channel_sysfs(struct iio_dev *indio_dev,
 	if (ret)
 		return ret;
 	attrcount++;
-	ret = __iio_add_chan_devattr("type",
+	ret = __iio_add_chan_bufattr("type",
 				     chan,
 				     &iio_show_fixed_type,
 				     NULL,
@@ -463,7 +548,7 @@ static int iio_buffer_add_channel_sysfs(struct iio_dev *indio_dev,
 		return ret;
 	attrcount++;
 	if (chan->type != IIO_TIMESTAMP)
-		ret = __iio_add_chan_devattr("en",
+		ret = __iio_add_chan_bufattr("en",
 					     chan,
 					     &iio_scan_el_show,
 					     &iio_scan_el_store,
@@ -472,7 +557,7 @@ static int iio_buffer_add_channel_sysfs(struct iio_dev *indio_dev,
 					     &indio_dev->dev,
 					     &buffer->scan_el_dev_attr_list);
 	else
-		ret = __iio_add_chan_devattr("en",
+		ret = __iio_add_chan_bufattr("en",
 					     chan,
 					     &iio_scan_el_ts_show,
 					     &iio_scan_el_ts_store,
@@ -1251,6 +1336,7 @@ static struct attribute *iio_buffer_attrs[] = {
 };
 
 #define to_dev_attr(_attr) container_of(_attr, struct device_attribute, attr)
+#define to_iio_buf_attr(_attr) container_of(_attr, struct iio_buf_attr, attr)
 
 static ssize_t iio_buffer_dir_attr_show(struct kobject *kobj,
 					struct attribute *attr,
@@ -1291,9 +1377,9 @@ static ssize_t iio_scan_el_dir_attr_show(struct kobject *kobj,
 					 char *buf)
 {
 	struct iio_buffer *buffer = container_of(kobj, struct iio_buffer, scan_el_dir);
-	struct device_attribute *dattr = to_dev_attr(attr);
+	struct iio_buf_attr *battr = to_iio_buf_attr(attr);
 
-	return dattr->show(&buffer->indio_dev->dev, dattr, buf);
+	return battr->show(buffer, battr, buf);
 }
 
 static ssize_t iio_scan_el_dir_attr_store(struct kobject *kobj,
@@ -1302,9 +1388,9 @@ static ssize_t iio_scan_el_dir_attr_store(struct kobject *kobj,
 					  size_t len)
 {
 	struct iio_buffer *buffer = container_of(kobj, struct iio_buffer, scan_el_dir);
-	struct device_attribute *dattr = to_dev_attr(attr);
+	struct iio_buf_attr *battr = to_iio_buf_attr(attr);
 
-	return dattr->store(&buffer->indio_dev->dev, dattr, buf, len);
+	return battr->store(buffer, battr, buf, len);
 }
 
 static const struct sysfs_ops iio_scan_el_dir_sysfs_ops = {
@@ -1372,7 +1458,7 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
 					     struct iio_dev *indio_dev,
 					     unsigned int idx)
 {
-	struct iio_dev_attr *p;
+	struct iio_buf_attr *p;
 	struct attribute **attr;
 	int ret, i, attrn, attrcount;
 	const struct iio_chan_spec *channels;
@@ -1453,7 +1539,7 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
 
 	attrn = 0;
 	list_for_each_entry(p, &buffer->scan_el_dev_attr_list, l)
-		buffer->scan_el_attrs[attrn++] = &p->dev_attr.attr;
+		buffer->scan_el_attrs[attrn++] = &p->attr;
 
 	ret = iio_sysfs_add_attrs(&buffer->scan_el_dir, buffer->scan_el_attrs);
 	if (ret)
@@ -1469,7 +1555,7 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
 	bitmap_free(buffer->scan_mask);
 error_cleanup_dynamic:
 	iio_sysfs_del_attrs(&buffer->buffer_dir, buffer->buffer_attrs);
-	iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list);
+	iio_free_chan_bufattr_list(&buffer->scan_el_dev_attr_list);
 error_buffer_kobject_put:
 	kobject_put(&buffer->buffer_dir);
 error_buffer_free_attrs:
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index b8f7261945f5..088e59042226 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -967,7 +967,6 @@ static ssize_t iio_write_channel_info(struct device *dev,
 	return len;
 }
 
-static
 int iio_attr_init(struct attribute *attr,
 		  const char *postfix,
 		  struct iio_chan_spec const *chan,
-- 
2.17.1

[PATCH v2 07/12][RESEND] iio: buffer: re-route core buffer attributes via it's new kobj_type

[Alexandru Ardelean]

For the buffer attributes that are present inside the IIO core buffer logic
we can re-route them to expand the attribute into iio_buffer objects.

The rest, will still expand to device_attributes.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/industrialio-buffer.c | 113 +++++++++++++++++-------------
 1 file changed, 64 insertions(+), 49 deletions(-)

diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index 524b897a1877..8470921cf2fa 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -572,22 +572,18 @@ static int iio_buffer_add_channel_sysfs(struct iio_dev *indio_dev,
 	return ret;
 }
 
-static ssize_t iio_buffer_read_length(struct device *dev,
-				      struct device_attribute *attr,
+static ssize_t iio_buffer_read_length(struct iio_buffer *buffer,
+				      struct iio_buf_attr *attr,
 				      char *buf)
 {
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	struct iio_buffer *buffer = indio_dev->buffer;
-
 	return sprintf(buf, "%d\n", buffer->length);
 }
 
-static ssize_t iio_buffer_write_length(struct device *dev,
-				       struct device_attribute *attr,
+static ssize_t iio_buffer_write_length(struct iio_buffer *buffer,
+				       struct iio_buf_attr *attr,
 				       const char *buf, size_t len)
 {
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	struct iio_buffer *buffer = indio_dev->buffer;
+	struct iio_dev *indio_dev = buffer->indio_dev;
 	unsigned int val;
 	int ret;
 
@@ -615,13 +611,10 @@ static ssize_t iio_buffer_write_length(struct device *dev,
 	return ret ? ret : len;
 }
 
-static ssize_t iio_buffer_show_enable(struct device *dev,
-				      struct device_attribute *attr,
+static ssize_t iio_buffer_show_enable(struct iio_buffer *buffer,
+				      struct iio_buf_attr *attr,
 				      char *buf)
 {
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	struct iio_buffer *buffer = indio_dev->buffer;
-
 	return sprintf(buf, "%d\n", iio_buffer_is_active(buffer));
 }
 
@@ -1227,15 +1220,14 @@ void iio_disable_all_buffers(struct iio_dev *indio_dev)
 	iio_buffer_deactivate_all(indio_dev);
 }
 
-static ssize_t iio_buffer_store_enable(struct device *dev,
-				       struct device_attribute *attr,
+static ssize_t iio_buffer_store_enable(struct iio_buffer *buffer,
+				       struct iio_buf_attr *attr,
 				       const char *buf,
 				       size_t len)
 {
 	int ret;
 	bool requested_state;
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	struct iio_buffer *buffer = indio_dev->buffer;
+	struct iio_dev *indio_dev = buffer->indio_dev;
 	bool inlist;
 
 	ret = strtobool(buf, &requested_state);
@@ -1260,23 +1252,19 @@ static ssize_t iio_buffer_store_enable(struct device *dev,
 	return (ret < 0) ? ret : len;
 }
 
-static ssize_t iio_buffer_show_watermark(struct device *dev,
-					 struct device_attribute *attr,
+static ssize_t iio_buffer_show_watermark(struct iio_buffer *buffer,
+					 struct iio_buf_attr *attr,
 					 char *buf)
 {
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	struct iio_buffer *buffer = indio_dev->buffer;
-
 	return sprintf(buf, "%u\n", buffer->watermark);
 }
 
-static ssize_t iio_buffer_store_watermark(struct device *dev,
-					  struct device_attribute *attr,
+static ssize_t iio_buffer_store_watermark(struct iio_buffer *buffer,
+					  struct iio_buf_attr *attr,
 					  const char *buf,
 					  size_t len)
 {
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	struct iio_buffer *buffer = indio_dev->buffer;
+	struct iio_dev *indio_dev = buffer->indio_dev;
 	unsigned int val;
 	int ret;
 
@@ -1305,36 +1293,51 @@ static ssize_t iio_buffer_store_watermark(struct device *dev,
 	return ret ? ret : len;
 }
 
-static ssize_t iio_dma_show_data_available(struct device *dev,
-						struct device_attribute *attr,
-						char *buf)
+static ssize_t iio_dma_show_data_available(struct iio_buffer *buffer,
+					   struct iio_buf_attr *attr,
+					   char *buf)
 {
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	struct iio_buffer *buffer = indio_dev->buffer;
-
 	return sprintf(buf, "%zu\n", iio_buffer_data_available(buffer));
 }
 
-static DEVICE_ATTR(length, S_IRUGO | S_IWUSR, iio_buffer_read_length,
-		   iio_buffer_write_length);
-static struct device_attribute dev_attr_length_ro = __ATTR(length,
+#define IIO_BUF_ATTR(_name, _mode, _show, _store)	\
+	struct iio_buf_attr iio_buf_attr_##_name =	\
+		__ATTR(_name, _mode, _show, _store)
+
+static IIO_BUF_ATTR(length, S_IRUGO | S_IWUSR,
+		    iio_buffer_read_length, iio_buffer_write_length);
+static struct iio_buf_attr buf_attr_length_ro = __ATTR(length,
 	S_IRUGO, iio_buffer_read_length, NULL);
-static DEVICE_ATTR(enable, S_IRUGO | S_IWUSR,
-		   iio_buffer_show_enable, iio_buffer_store_enable);
-static DEVICE_ATTR(watermark, S_IRUGO | S_IWUSR,
-		   iio_buffer_show_watermark, iio_buffer_store_watermark);
-static struct device_attribute dev_attr_watermark_ro = __ATTR(watermark,
+static IIO_BUF_ATTR(enable, S_IRUGO | S_IWUSR,
+		    iio_buffer_show_enable, iio_buffer_store_enable);
+static IIO_BUF_ATTR(watermark, S_IRUGO | S_IWUSR,
+		    iio_buffer_show_watermark, iio_buffer_store_watermark);
+static struct iio_buf_attr buf_attr_watermark_ro = __ATTR(watermark,
 	S_IRUGO, iio_buffer_show_watermark, NULL);
-static DEVICE_ATTR(data_available, S_IRUGO,
-		iio_dma_show_data_available, NULL);
+static IIO_BUF_ATTR(data_available, S_IRUGO,
+		    iio_dma_show_data_available, NULL);
 
 static struct attribute *iio_buffer_attrs[] = {
-	&dev_attr_length.attr,
-	&dev_attr_enable.attr,
-	&dev_attr_watermark.attr,
-	&dev_attr_data_available.attr,
+	&iio_buf_attr_length.attr,
+	&iio_buf_attr_enable.attr,
+	&iio_buf_attr_watermark.attr,
+	&iio_buf_attr_data_available.attr,
 };
 
+static bool iio_buffer_attr_is_core(struct attribute *attr)
+{
+	struct attribute *a;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(iio_buffer_attrs); i++) {
+		a = iio_buffer_attrs[i];
+		if (!strcmp(attr->name, a->name))
+			return true;
+	}
+
+	return false;
+}
+
 #define to_dev_attr(_attr) container_of(_attr, struct device_attribute, attr)
 #define to_iio_buf_attr(_attr) container_of(_attr, struct iio_buf_attr, attr)
 
@@ -1344,6 +1347,12 @@ static ssize_t iio_buffer_dir_attr_show(struct kobject *kobj,
 {
 	struct iio_buffer *buffer = container_of(kobj, struct iio_buffer, buffer_dir);
 	struct device_attribute *dattr;
+	struct iio_buf_attr *battr;
+
+	if (iio_buffer_attr_is_core(attr)) {
+		battr = to_iio_buf_attr(attr);
+		return battr->show(buffer, battr, buf);
+	}
 
 	dattr = to_dev_attr(attr);
 
@@ -1357,6 +1366,12 @@ static ssize_t iio_buffer_dir_attr_store(struct kobject *kobj,
 {
 	struct iio_buffer *buffer = container_of(kobj, struct iio_buffer, buffer_dir);
 	struct device_attribute *dattr;
+	struct iio_buf_attr *battr;
+
+	if (iio_buffer_attr_is_core(attr)) {
+		battr = to_iio_buf_attr(attr);
+		return battr->store(buffer, battr, buf, len);
+	}
 
 	dattr = to_dev_attr(attr);
 
@@ -1476,10 +1491,10 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
 
 	memcpy(attr, iio_buffer_attrs, sizeof(iio_buffer_attrs));
 	if (!buffer->access->set_length)
-		attr[0] = &dev_attr_length_ro.attr;
+		attr[0] = &buf_attr_length_ro.attr;
 
 	if (buffer->access->flags & INDIO_BUFFER_FLAG_FIXED_WATERMARK)
-		attr[2] = &dev_attr_watermark_ro.attr;
+		attr[2] = &buf_attr_watermark_ro.attr;
 
 	if (buffer->attrs)
 		memcpy(&attr[ARRAY_SIZE(iio_buffer_attrs)], buffer->attrs,
-- 
2.17.1

[PATCH v2 08/12][RESEND] iio: buffer: add helper to get the IIO device to which a buffer belongs

[Alexandru Ardelean]

This is useful for drivers that may have a reference to an IIO buffer, to
be able to get a reference back to the IIO device.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/industrialio-buffer.c | 14 ++++++++++++++
 include/linux/iio/buffer.h        |  2 ++
 2 files changed, 16 insertions(+)

diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index 8470921cf2fa..a2dd30567072 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -1778,6 +1778,20 @@ void iio_buffer_put(struct iio_buffer *buffer)
 }
 EXPORT_SYMBOL_GPL(iio_buffer_put);
 
+/**
+ * iio_buffer_get_iio_dev - Get the IIO device to which this buffer belongs to
+ * @buffer: The buffer for which to retrieve the IIO device
+ *
+ * This function retrieves the IIO device to which this IIO buffer is attached
+ * to. Given that an IIO device may have multiple IIO buffers, it's useful
+ * for some drivers to obtain a reference back to the IIO device.
+ */
+struct iio_dev *iio_buffer_get_iio_dev(struct iio_buffer *buffer)
+{
+	return buffer->indio_dev;
+}
+EXPORT_SYMBOL_GPL(iio_buffer_get_iio_dev);
+
 /**
  * iio_device_attach_buffer - Attach a buffer to a IIO device
  * @indio_dev: The device the buffer should be attached to
diff --git a/include/linux/iio/buffer.h b/include/linux/iio/buffer.h
index 8febc23f5f26..b27d8c81f32c 100644
--- a/include/linux/iio/buffer.h
+++ b/include/linux/iio/buffer.h
@@ -11,6 +11,8 @@
 
 struct iio_buffer;
 
+struct iio_dev *iio_buffer_get_iio_dev(struct iio_buffer *buffer);
+
 int iio_push_to_buffers(struct iio_dev *indio_dev, const void *data);
 
 /**
-- 
2.17.1

[PATCH v2 09/12][RESEND] iio: re-route all buffer attributes through new buffer kobj_type

[Alexandru Ardelean]

Now that the iio_buffer_set_attrs() has been removed, we can be sure that
no accidents can happen with drivers that try to provide extra buffer
attributes that expand to iio_dev objects.

So, we can convert all remaining buffer attributes to expand to iio_buffer
objects.
These will look a bit weird at first, as most of them will just pass back
their reference to the IIO device.
But this can also allow for newer (maybe more interesting) uses.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/accel/adxl372.c                   | 36 ++++-----
 drivers/iio/accel/bmc150-accel-core.c         | 34 ++++-----
 drivers/iio/adc/at91-sama5d2_adc.c            | 30 ++++----
 .../buffer/industrialio-buffer-dmaengine.c    | 13 ++--
 .../cros_ec_sensors/cros_ec_sensors_core.c    | 30 ++++----
 .../common/hid-sensors/hid-sensor-trigger.c   | 32 ++++----
 drivers/iio/industrialio-buffer.c             | 73 +++----------------
 include/linux/iio/sysfs.h                     | 50 +++++++++++++
 8 files changed, 148 insertions(+), 150 deletions(-)

diff --git a/drivers/iio/accel/adxl372.c b/drivers/iio/accel/adxl372.c
index 8ba1453b8dbf..a90aaa518816 100644
--- a/drivers/iio/accel/adxl372.c
+++ b/drivers/iio/accel/adxl372.c
@@ -978,39 +978,39 @@ static ssize_t adxl372_show_filter_freq_avail(struct device *dev,
 	return len;
 }
 
-static ssize_t adxl372_get_fifo_enabled(struct device *dev,
-					  struct device_attribute *attr,
-					  char *buf)
+static ssize_t adxl372_get_fifo_enabled(struct iio_buffer *buffer,
+					struct iio_buf_attr *attr,
+					char *buf)
 {
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct iio_dev *indio_dev = iio_buffer_get_iio_dev(buffer);
 	struct adxl372_state *st = iio_priv(indio_dev);
 
 	return sprintf(buf, "%d\n", st->fifo_mode);
 }
 
-static ssize_t adxl372_get_fifo_watermark(struct device *dev,
-					  struct device_attribute *attr,
+static ssize_t adxl372_get_fifo_watermark(struct iio_buffer *buffer,
+					  struct iio_buf_attr *attr,
 					  char *buf)
 {
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct iio_dev *indio_dev = iio_buffer_get_iio_dev(buffer);
 	struct adxl372_state *st = iio_priv(indio_dev);
 
 	return sprintf(buf, "%d\n", st->watermark);
 }
 
-static IIO_CONST_ATTR(hwfifo_watermark_min, "1");
-static IIO_CONST_ATTR(hwfifo_watermark_max,
-		      __stringify(ADXL372_FIFO_SIZE));
-static IIO_DEVICE_ATTR(hwfifo_watermark, 0444,
-		       adxl372_get_fifo_watermark, NULL, 0);
-static IIO_DEVICE_ATTR(hwfifo_enabled, 0444,
-		       adxl372_get_fifo_enabled, NULL, 0);
+static IIO_BUF_CONST_ATTR(hwfifo_watermark_min, "1");
+static IIO_BUF_CONST_ATTR(hwfifo_watermark_max,
+			  __stringify(ADXL372_FIFO_SIZE));
+static IIO_BUF_ATTR(hwfifo_watermark, 0444,
+		    adxl372_get_fifo_watermark, NULL);
+static IIO_BUF_ATTR(hwfifo_enabled, 0444,
+		    adxl372_get_fifo_enabled, NULL);
 
 static const struct attribute *adxl372_fifo_attributes[] = {
-	&iio_const_attr_hwfifo_watermark_min.dev_attr.attr,
-	&iio_const_attr_hwfifo_watermark_max.dev_attr.attr,
-	&iio_dev_attr_hwfifo_watermark.dev_attr.attr,
-	&iio_dev_attr_hwfifo_enabled.dev_attr.attr,
+	&iio_buf_const_attr_hwfifo_watermark_min.buf_attr.attr,
+	&iio_buf_const_attr_hwfifo_watermark_max.buf_attr.attr,
+	&iio_buf_attr_hwfifo_watermark.attr,
+	&iio_buf_attr_hwfifo_enabled.attr,
 	NULL,
 };
 
diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c
index 7e425ebcd7ea..87b1ad523106 100644
--- a/drivers/iio/accel/bmc150-accel-core.c
+++ b/drivers/iio/accel/bmc150-accel-core.c
@@ -867,11 +867,11 @@ static int bmc150_accel_validate_trigger(struct iio_dev *indio_dev,
 	return -EINVAL;
 }
 
-static ssize_t bmc150_accel_get_fifo_watermark(struct device *dev,
-					       struct device_attribute *attr,
+static ssize_t bmc150_accel_get_fifo_watermark(struct iio_buffer *buffer,
+					       struct iio_buf_attr *attr,
 					       char *buf)
 {
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct iio_dev *indio_dev = iio_buffer_get_iio_dev(buffer);
 	struct bmc150_accel_data *data = iio_priv(indio_dev);
 	int wm;
 
@@ -882,11 +882,11 @@ static ssize_t bmc150_accel_get_fifo_watermark(struct device *dev,
 	return sprintf(buf, "%d\n", wm);
 }
 
-static ssize_t bmc150_accel_get_fifo_state(struct device *dev,
-					   struct device_attribute *attr,
+static ssize_t bmc150_accel_get_fifo_state(struct iio_buffer *buffer,
+					   struct iio_buf_attr *attr,
 					   char *buf)
 {
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct iio_dev *indio_dev = iio_buffer_get_iio_dev(buffer);
 	struct bmc150_accel_data *data = iio_priv(indio_dev);
 	bool state;
 
@@ -911,19 +911,19 @@ static const struct iio_chan_spec_ext_info bmc150_accel_ext_info[] = {
 	{ }
 };
 
-static IIO_CONST_ATTR(hwfifo_watermark_min, "1");
-static IIO_CONST_ATTR(hwfifo_watermark_max,
-		      __stringify(BMC150_ACCEL_FIFO_LENGTH));
-static IIO_DEVICE_ATTR(hwfifo_enabled, S_IRUGO,
-		       bmc150_accel_get_fifo_state, NULL, 0);
-static IIO_DEVICE_ATTR(hwfifo_watermark, S_IRUGO,
-		       bmc150_accel_get_fifo_watermark, NULL, 0);
+static IIO_BUF_CONST_ATTR(hwfifo_watermark_min, "1");
+static IIO_BUF_CONST_ATTR(hwfifo_watermark_max,
+			  __stringify(BMC150_ACCEL_FIFO_LENGTH));
+static IIO_BUF_ATTR(hwfifo_enabled, S_IRUGO,
+		    bmc150_accel_get_fifo_state, NULL);
+static IIO_BUF_ATTR(hwfifo_watermark, S_IRUGO,
+		    bmc150_accel_get_fifo_watermark, NULL);
 
 static const struct attribute *bmc150_accel_fifo_attributes[] = {
-	&iio_const_attr_hwfifo_watermark_min.dev_attr.attr,
-	&iio_const_attr_hwfifo_watermark_max.dev_attr.attr,
-	&iio_dev_attr_hwfifo_watermark.dev_attr.attr,
-	&iio_dev_attr_hwfifo_enabled.dev_attr.attr,
+	&iio_buf_const_attr_hwfifo_watermark_min.buf_attr.attr,
+	&iio_buf_const_attr_hwfifo_watermark_max.buf_attr.attr,
+	&iio_buf_attr_hwfifo_watermark.attr,
+	&iio_buf_attr_hwfifo_enabled.attr,
 	NULL,
 };
 
diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
index a7826f097b95..a573f0d5d0d8 100644
--- a/drivers/iio/adc/at91-sama5d2_adc.c
+++ b/drivers/iio/adc/at91-sama5d2_adc.c
@@ -1610,31 +1610,29 @@ static void at91_adc_hw_init(struct iio_dev *indio_dev)
 	at91_adc_config_emr(st);
 }
 
-static ssize_t at91_adc_get_fifo_state(struct device *dev,
-				       struct device_attribute *attr, char *buf)
+static ssize_t at91_adc_get_fifo_state(struct iio_buffer *buffer,
+				       struct iio_buf_attr *attr, char *buf)
 {
-	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct iio_dev *indio_dev = iio_buffer_get_iio_dev(buffer);
 	struct at91_adc_state *st = iio_priv(indio_dev);
 
 	return scnprintf(buf, PAGE_SIZE, "%d\n", !!st->dma_st.dma_chan);
 }
 
-static ssize_t at91_adc_get_watermark(struct device *dev,
-				      struct device_attribute *attr, char *buf)
+static ssize_t at91_adc_get_watermark(struct iio_buffer *buffer,
+				      struct iio_buf_attr *attr, char *buf)
 {
-	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct iio_dev *indio_dev = iio_buffer_get_iio_dev(buffer);
 	struct at91_adc_state *st = iio_priv(indio_dev);
 
 	return scnprintf(buf, PAGE_SIZE, "%d\n", st->dma_st.watermark);
 }
 
-static IIO_DEVICE_ATTR(hwfifo_enabled, 0444,
-		       at91_adc_get_fifo_state, NULL, 0);
-static IIO_DEVICE_ATTR(hwfifo_watermark, 0444,
-		       at91_adc_get_watermark, NULL, 0);
+static IIO_BUF_ATTR(hwfifo_enabled, 0444, at91_adc_get_fifo_state, NULL);
+static IIO_BUF_ATTR(hwfifo_watermark, 0444, at91_adc_get_watermark, NULL);
 
-static IIO_CONST_ATTR(hwfifo_watermark_min, "2");
-static IIO_CONST_ATTR(hwfifo_watermark_max, AT91_HWFIFO_MAX_SIZE_STR);
+static IIO_BUF_CONST_ATTR(hwfifo_watermark_min, "2");
+static IIO_BUF_CONST_ATTR(hwfifo_watermark_max, AT91_HWFIFO_MAX_SIZE_STR);
 
 static IIO_CONST_ATTR(oversampling_ratio_available,
 		      __stringify(AT91_OSR_1SAMPLES) " "
@@ -1651,10 +1649,10 @@ static const struct attribute_group at91_adc_attribute_group = {
 };
 
 static const struct attribute *at91_adc_fifo_attributes[] = {
-	&iio_const_attr_hwfifo_watermark_min.dev_attr.attr,
-	&iio_const_attr_hwfifo_watermark_max.dev_attr.attr,
-	&iio_dev_attr_hwfifo_watermark.dev_attr.attr,
-	&iio_dev_attr_hwfifo_enabled.dev_attr.attr,
+	&iio_buf_const_attr_hwfifo_watermark_min.buf_attr.attr,
+	&iio_buf_const_attr_hwfifo_watermark_max.buf_attr.attr,
+	&iio_buf_attr_hwfifo_watermark.attr,
+	&iio_buf_attr_hwfifo_enabled.attr,
 	NULL,
 };
 
diff --git a/drivers/iio/buffer/industrialio-buffer-dmaengine.c b/drivers/iio/buffer/industrialio-buffer-dmaengine.c
index b0cb9a35f5cd..2cd5fd3fe191 100644
--- a/drivers/iio/buffer/industrialio-buffer-dmaengine.c
+++ b/drivers/iio/buffer/industrialio-buffer-dmaengine.c
@@ -129,21 +129,20 @@ static const struct iio_dma_buffer_ops iio_dmaengine_default_ops = {
 	.abort = iio_dmaengine_buffer_abort,
 };
 
-static ssize_t iio_dmaengine_buffer_get_length_align(struct device *dev,
-	struct device_attribute *attr, char *buf)
+static ssize_t iio_dmaengine_buffer_get_length_align(struct iio_buffer *buffer,
+	struct iio_buf_attr *attr, char *buf)
 {
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
 	struct dmaengine_buffer *dmaengine_buffer =
-		iio_buffer_to_dmaengine_buffer(indio_dev->buffer);
+		iio_buffer_to_dmaengine_buffer(buffer);
 
 	return sprintf(buf, "%zu\n", dmaengine_buffer->align);
 }
 
-static IIO_DEVICE_ATTR(length_align_bytes, 0444,
-		       iio_dmaengine_buffer_get_length_align, NULL, 0);
+static IIO_BUF_ATTR(length_align_bytes, 0444,
+		    iio_dmaengine_buffer_get_length_align, NULL);
 
 static const struct attribute *iio_dmaengine_buffer_attrs[] = {
-	&iio_dev_attr_length_align_bytes.dev_attr.attr,
+	&iio_buf_attr_length_align_bytes.attr,
 	NULL,
 };
 
diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
index 4e7b3bc187c9..99f2f9146fab 100644
--- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
+++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
@@ -116,11 +116,11 @@ static int cros_ec_sensor_set_ec_rate(struct cros_ec_sensors_core_state *st,
 	return ret;
 }
 
-static ssize_t cros_ec_sensor_set_report_latency(struct device *dev,
-						 struct device_attribute *attr,
+static ssize_t cros_ec_sensor_set_report_latency(struct iio_buffer *buffer,
+						 struct iio_buf_attr *attr,
 						 const char *buf, size_t len)
 {
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct iio_dev *indio_dev = iio_buffer_get_iio_dev(buffer);
 	struct cros_ec_sensors_core_state *st = iio_priv(indio_dev);
 	int integer, fract, ret;
 	int latency;
@@ -138,11 +138,11 @@ static ssize_t cros_ec_sensor_set_report_latency(struct device *dev,
 	return len;
 }
 
-static ssize_t cros_ec_sensor_get_report_latency(struct device *dev,
-						 struct device_attribute *attr,
+static ssize_t cros_ec_sensor_get_report_latency(struct iio_buffer *buffer,
+						 struct iio_buf_attr *attr,
 						 char *buf)
 {
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct iio_dev *indio_dev = iio_buffer_get_iio_dev(buffer);
 	struct cros_ec_sensors_core_state *st = iio_priv(indio_dev);
 	int latency, ret;
 
@@ -161,25 +161,25 @@ static ssize_t cros_ec_sensor_get_report_latency(struct device *dev,
 		       (latency % 1000) * 1000);
 }
 
-static IIO_DEVICE_ATTR(hwfifo_timeout, 0644,
-		       cros_ec_sensor_get_report_latency,
-		       cros_ec_sensor_set_report_latency, 0);
+static IIO_BUF_ATTR(hwfifo_timeout, 0644,
+		    cros_ec_sensor_get_report_latency,
+		    cros_ec_sensor_set_report_latency);
 
-static ssize_t hwfifo_watermark_max_show(struct device *dev,
-					 struct device_attribute *attr,
+static ssize_t hwfifo_watermark_max_show(struct iio_buffer *buffer,
+					 struct iio_buf_attr *attr,
 					 char *buf)
 {
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct iio_dev *indio_dev = iio_buffer_get_iio_dev(buffer);
 	struct cros_ec_sensors_core_state *st = iio_priv(indio_dev);
 
 	return sprintf(buf, "%d\n", st->fifo_max_event_count);
 }
 
-static IIO_DEVICE_ATTR_RO(hwfifo_watermark_max, 0);
+static IIO_BUF_ATTR(hwfifo_watermark_max, 0444, hwfifo_watermark_max_show, NULL);
 
 static const struct attribute *cros_ec_sensor_fifo_attributes[] = {
-	&iio_dev_attr_hwfifo_timeout.dev_attr.attr,
-	&iio_dev_attr_hwfifo_watermark_max.dev_attr.attr,
+	&iio_buf_attr_hwfifo_timeout.attr,
+	&iio_buf_attr_hwfifo_watermark_max.attr,
 	NULL,
 };
 
diff --git a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
index 064c32bec9c7..c04dca7a457b 100644
--- a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
+++ b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
@@ -19,11 +19,11 @@
 #include <linux/iio/sysfs.h>
 #include "hid-sensor-trigger.h"
 
-static ssize_t _hid_sensor_set_report_latency(struct device *dev,
-					      struct device_attribute *attr,
+static ssize_t _hid_sensor_set_report_latency(struct iio_buffer *buffer,
+					      struct iio_buf_attr *attr,
 					      const char *buf, size_t len)
 {
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct iio_dev *indio_dev = iio_buffer_get_iio_dev(buffer);
 	struct hid_sensor_common *attrb = iio_device_get_drvdata(indio_dev);
 	int integer, fract, ret;
 	int latency;
@@ -42,11 +42,11 @@ static ssize_t _hid_sensor_set_report_latency(struct device *dev,
 	return len;
 }
 
-static ssize_t _hid_sensor_get_report_latency(struct device *dev,
-					      struct device_attribute *attr,
+static ssize_t _hid_sensor_get_report_latency(struct iio_buffer *buffer,
+					      struct iio_buf_attr *attr,
 					      char *buf)
 {
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct iio_dev *indio_dev = iio_buffer_get_iio_dev(buffer);
 	struct hid_sensor_common *attrb = iio_device_get_drvdata(indio_dev);
 	int latency;
 
@@ -57,11 +57,11 @@ static ssize_t _hid_sensor_get_report_latency(struct device *dev,
 	return sprintf(buf, "%d.%06u\n", latency / 1000, (latency % 1000) * 1000);
 }
 
-static ssize_t _hid_sensor_get_fifo_state(struct device *dev,
-					  struct device_attribute *attr,
+static ssize_t _hid_sensor_get_fifo_state(struct iio_buffer *buffer,
+					  struct iio_buf_attr *attr,
 					  char *buf)
 {
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct iio_dev *indio_dev = iio_buffer_get_iio_dev(buffer);
 	struct hid_sensor_common *attrb = iio_device_get_drvdata(indio_dev);
 	int latency;
 
@@ -72,15 +72,15 @@ static ssize_t _hid_sensor_get_fifo_state(struct device *dev,
 	return sprintf(buf, "%d\n", !!latency);
 }
 
-static IIO_DEVICE_ATTR(hwfifo_timeout, 0644,
-		       _hid_sensor_get_report_latency,
-		       _hid_sensor_set_report_latency, 0);
-static IIO_DEVICE_ATTR(hwfifo_enabled, 0444,
-		       _hid_sensor_get_fifo_state, NULL, 0);
+static IIO_BUF_ATTR(hwfifo_timeout, 0644,
+		    _hid_sensor_get_report_latency,
+		    _hid_sensor_set_report_latency);
+static IIO_BUF_ATTR(hwfifo_enabled, 0444,
+		    _hid_sensor_get_fifo_state, NULL);
 
 static const struct attribute *hid_sensor_fifo_attributes[] = {
-	&iio_dev_attr_hwfifo_timeout.dev_attr.attr,
-	&iio_dev_attr_hwfifo_enabled.dev_attr.attr,
+	&iio_buf_attr_hwfifo_timeout.attr,
+	&iio_buf_attr_hwfifo_enabled.attr,
 	NULL,
 };
 
diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index a2dd30567072..887c71272e9c 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -26,31 +26,19 @@
 #include <linux/iio/buffer.h>
 #include <linux/iio/buffer_impl.h>
 
-/**
- * struct iio_buf_attr - iio buffer specific attribute
- * @attr:	underlying attribute
- * @address:	associated register address
- * @l:		list head for maintaining list of dynamically created attrs
- * @c:		specification for the underlying channel
- * @show:	sysfs show hook for this attribute
- * @store:	sysfs store hook for this attribute
- */
-struct iio_buf_attr {
-	struct attribute attr;
-	u64 address;
-	struct list_head l;
-	struct iio_chan_spec const *c;
-	ssize_t (*show)(struct iio_buffer *buffer, struct iio_buf_attr *attr,
-			char *buf);
-	ssize_t (*store)(struct iio_buffer *buffer, struct iio_buf_attr *attr,
-			 const char *buf, size_t count);
-};
-
 static const char * const iio_endian_prefix[] = {
 	[IIO_BE] = "be",
 	[IIO_LE] = "le",
 };
 
+ssize_t iio_read_buf_const_attr(struct iio_buffer *buffer,
+				struct iio_buf_attr *attr,
+				char *buf)
+{
+	return sprintf(buf, "%s\n", to_iio_buf_const_attr(attr)->string);
+}
+EXPORT_SYMBOL(iio_read_buf_const_attr);
+
 static bool iio_buffer_is_active(struct iio_buffer *buf)
 {
 	return !list_empty(&buf->buffer_list);
@@ -1300,10 +1288,6 @@ static ssize_t iio_dma_show_data_available(struct iio_buffer *buffer,
 	return sprintf(buf, "%zu\n", iio_buffer_data_available(buffer));
 }
 
-#define IIO_BUF_ATTR(_name, _mode, _show, _store)	\
-	struct iio_buf_attr iio_buf_attr_##_name =	\
-		__ATTR(_name, _mode, _show, _store)
-
 static IIO_BUF_ATTR(length, S_IRUGO | S_IWUSR,
 		    iio_buffer_read_length, iio_buffer_write_length);
 static struct iio_buf_attr buf_attr_length_ro = __ATTR(length,
@@ -1324,39 +1308,14 @@ static struct attribute *iio_buffer_attrs[] = {
 	&iio_buf_attr_data_available.attr,
 };
 
-static bool iio_buffer_attr_is_core(struct attribute *attr)
-{
-	struct attribute *a;
-	int i;
-
-	for (i = 0; i < ARRAY_SIZE(iio_buffer_attrs); i++) {
-		a = iio_buffer_attrs[i];
-		if (!strcmp(attr->name, a->name))
-			return true;
-	}
-
-	return false;
-}
-
-#define to_dev_attr(_attr) container_of(_attr, struct device_attribute, attr)
-#define to_iio_buf_attr(_attr) container_of(_attr, struct iio_buf_attr, attr)
-
 static ssize_t iio_buffer_dir_attr_show(struct kobject *kobj,
 					struct attribute *attr,
 					char *buf)
 {
 	struct iio_buffer *buffer = container_of(kobj, struct iio_buffer, buffer_dir);
-	struct device_attribute *dattr;
-	struct iio_buf_attr *battr;
-
-	if (iio_buffer_attr_is_core(attr)) {
-		battr = to_iio_buf_attr(attr);
-		return battr->show(buffer, battr, buf);
-	}
-
-	dattr = to_dev_attr(attr);
+	struct iio_buf_attr *battr = to_iio_buf_attr(attr);
 
-	return dattr->show(&buffer->indio_dev->dev, dattr, buf);
+	return battr->show(buffer, battr, buf);
 }
 
 static ssize_t iio_buffer_dir_attr_store(struct kobject *kobj,
@@ -1365,17 +1324,9 @@ static ssize_t iio_buffer_dir_attr_store(struct kobject *kobj,
 					 size_t len)
 {
 	struct iio_buffer *buffer = container_of(kobj, struct iio_buffer, buffer_dir);
-	struct device_attribute *dattr;
-	struct iio_buf_attr *battr;
-
-	if (iio_buffer_attr_is_core(attr)) {
-		battr = to_iio_buf_attr(attr);
-		return battr->store(buffer, battr, buf, len);
-	}
-
-	dattr = to_dev_attr(attr);
+	struct iio_buf_attr *battr = to_iio_buf_attr(attr);
 
-	return dattr->store(&buffer->indio_dev->dev, dattr, buf, len);
+	return battr->store(buffer, battr, buf, len);
 }
 
 static const struct sysfs_ops iio_buffer_dir_sysfs_ops = {
diff --git a/include/linux/iio/sysfs.h b/include/linux/iio/sysfs.h
index b532c875bc24..d25b47971c09 100644
--- a/include/linux/iio/sysfs.h
+++ b/include/linux/iio/sysfs.h
@@ -9,6 +9,7 @@
 #ifndef _INDUSTRIAL_IO_SYSFS_H_
 #define _INDUSTRIAL_IO_SYSFS_H_
 
+struct iio_buffer;
 struct iio_chan_spec;
 
 /**
@@ -28,6 +29,55 @@ struct iio_dev_attr {
 #define to_iio_dev_attr(_dev_attr)				\
 	container_of(_dev_attr, struct iio_dev_attr, dev_attr)
 
+/**
+ * struct iio_buf_attr - iio buffer specific attribute
+ * @attr:	underlying attribute
+ * @address:	associated register address
+ * @l:		list head for maintaining list of dynamically created attrs
+ * @c:		specification for the underlying channel
+ * @show:	sysfs show hook for this attribute
+ * @store:	sysfs store hook for this attribute
+ */
+struct iio_buf_attr {
+	struct attribute attr;
+	u64 address;
+	struct list_head l;
+	struct iio_chan_spec const *c;
+	ssize_t (*show)(struct iio_buffer *buffer, struct iio_buf_attr *attr,
+			char *buf);
+	ssize_t (*store)(struct iio_buffer *buffer, struct iio_buf_attr *attr,
+			 const char *buf, size_t count);
+};
+
+/**
+ * struct iio_buf_const_attr - constant buffer specific attribute
+ *                         often used for things constant parameters of buffers
+ * @string:	attribute string
+ * @buf_attr:	underlying buffer attribute
+ */
+struct iio_buf_const_attr {
+	const char *string;
+	struct iio_buf_attr buf_attr;
+};
+
+#define to_iio_buf_attr(_attr) container_of(_attr, struct iio_buf_attr, attr)
+
+#define IIO_BUF_ATTR(_name, _mode, _show, _store)	\
+	struct iio_buf_attr iio_buf_attr_##_name =	\
+		__ATTR(_name, _mode, _show, _store)
+
+#define IIO_BUF_CONST_ATTR(_name, _string)				\
+	struct iio_buf_const_attr iio_buf_const_attr_##_name		\
+	= { .string = _string,						\
+	    .buf_attr = __ATTR(_name, 0444, iio_read_buf_const_attr, NULL)}
+
+#define to_iio_buf_const_attr(_buf_attr) \
+	container_of(_buf_attr, struct iio_buf_const_attr, buf_attr)
+
+ssize_t iio_read_buf_const_attr(struct iio_buffer *buffer,
+				struct iio_buf_attr *attr,
+				char *len);
+
 ssize_t iio_read_const_attr(struct device *dev,
 			    struct device_attribute *attr,
 			    char *len);
-- 
2.17.1

[PATCH v2 10/12][RESEND] iio: core: wrap iio device & buffer into struct for character devices

[Alexandru Ardelean]

In order to keep backwards compatibility with the current chardev
mechanism, and in order to add support for multiple buffers per IIO device,
we need to pass both the IIO device & IIO buffer to the chardev.

This is particularly needed for the iio_buffer_read_outer() function, where
we need to pass another buffer object than 'indio_dev->buffer'.

Since we'll also open some chardevs via anon inodes, we can pass extra
buffers in that function by assigning another object to the
iio_dev_buffer_pair object.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/iio_core.h            |  6 ++++++
 drivers/iio/industrialio-buffer.c | 10 ++++++----
 drivers/iio/industrialio-core.c   | 18 ++++++++++++++++--
 3 files changed, 28 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h
index 43d44ec92781..ad6716fe1a93 100644
--- a/drivers/iio/iio_core.h
+++ b/drivers/iio/iio_core.h
@@ -12,11 +12,17 @@
 #include <linux/kernel.h>
 #include <linux/device.h>
 
+struct iio_buffer;
 struct iio_chan_spec;
 struct iio_dev;
 
 extern struct device_type iio_device_type;
 
+struct iio_dev_buffer_pair {
+	struct iio_dev		*indio_dev;
+	struct iio_buffer	*buffer;
+};
+
 #define IIO_IOCTL_UNHANDLED	1
 struct iio_ioctl_handler {
 	struct list_head entry;
diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index 887c71272e9c..62cb90385246 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -112,8 +112,9 @@ static bool iio_buffer_ready(struct iio_dev *indio_dev, struct iio_buffer *buf,
 ssize_t iio_buffer_read_outer(struct file *filp, char __user *buf,
 			      size_t n, loff_t *f_ps)
 {
-	struct iio_dev *indio_dev = filp->private_data;
-	struct iio_buffer *rb = indio_dev->buffer;
+	struct iio_dev_buffer_pair *ib = filp->private_data;
+	struct iio_buffer *rb = ib->buffer;
+	struct iio_dev *indio_dev = ib->indio_dev;
 	DEFINE_WAIT_FUNC(wait, woken_wake_function);
 	size_t datum_size;
 	size_t to_wait;
@@ -178,8 +179,9 @@ ssize_t iio_buffer_read_outer(struct file *filp, char __user *buf,
 __poll_t iio_buffer_poll(struct file *filp,
 			     struct poll_table_struct *wait)
 {
-	struct iio_dev *indio_dev = filp->private_data;
-	struct iio_buffer *rb = indio_dev->buffer;
+	struct iio_dev_buffer_pair *ib = filp->private_data;
+	struct iio_buffer *rb = ib->buffer;
+	struct iio_dev *indio_dev = ib->indio_dev;
 
 	if (!indio_dev->info || rb == NULL)
 		return 0;
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 088e59042226..0d1880837776 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -1666,13 +1666,24 @@ static int iio_chrdev_open(struct inode *inode, struct file *filp)
 {
 	struct iio_dev *indio_dev = container_of(inode->i_cdev,
 						struct iio_dev, chrdev);
+	struct iio_dev_buffer_pair *ib;
 
 	if (test_and_set_bit(IIO_BUSY_BIT_POS, &indio_dev->flags))
 		return -EBUSY;
 
 	iio_device_get(indio_dev);
 
-	filp->private_data = indio_dev;
+	ib = kmalloc(sizeof(*ib), GFP_KERNEL);
+	if (!ib) {
+		iio_device_put(indio_dev);
+		clear_bit(IIO_BUSY_BIT_POS, &indio_dev->flags);
+		return -ENOMEM;
+	}
+
+	ib->indio_dev = indio_dev;
+	ib->buffer = indio_dev->buffer;
+
+	filp->private_data = ib;
 
 	return 0;
 }
@@ -1686,10 +1697,12 @@ static int iio_chrdev_open(struct inode *inode, struct file *filp)
  */
 static int iio_chrdev_release(struct inode *inode, struct file *filp)
 {
+	struct iio_dev_buffer_pair *ib = filp->private_data;
 	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);
+	kfree(ib);
 
 	return 0;
 }
@@ -1709,7 +1722,8 @@ void iio_device_ioctl_handler_unregister(struct iio_ioctl_handler *h)
 
 static long iio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 {
-	struct iio_dev *indio_dev = filp->private_data;
+	struct iio_dev_buffer_pair *ib = filp->private_data;
+	struct iio_dev *indio_dev = ib->indio_dev;
 	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
 	struct iio_ioctl_handler *h;
 	int ret = -ENODEV;
-- 
2.17.1

[PATCH v2 11/12][RESEND] iio: buffer: introduce support for attaching more IIO buffers

[Alexandru Ardelean]

With this change, calling iio_device_attach_buffer() will actually attach
more buffers.
Right now this doesn't do any validation of whether a buffer is attached
twice; maybe that can be added later (if needed). Attaching a buffer more
than once should yield noticeably bad results.

The first buffer is the legacy buffer, so a reference is kept to it.

At this point, accessing the data for the extra buffers (that are added
after the first one) isn't possible yet.

The iio_device_attach_buffer() is also changed to return an error code,
which for now is -ENOMEM if the array could not be realloc-ed for more
buffers.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/industrialio-buffer.c | 67 ++++++++++++++++++++++++-------
 include/linux/iio/buffer.h        |  4 +-
 include/linux/iio/buffer_impl.h   |  3 ++
 include/linux/iio/iio-opaque.h    |  4 ++
 4 files changed, 62 insertions(+), 16 deletions(-)

diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index 62cb90385246..2f429616e998 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -1561,9 +1561,10 @@ static void __iio_buffer_free_sysfs_and_mask(struct iio_buffer *buffer)
  */
 int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
 {
+	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
 	struct iio_buffer *buffer = indio_dev->buffer;
 	const struct iio_chan_spec *channels;
-	int i, ret;
+	int i, cnt, ret;
 
 	channels = indio_dev->channels;
 	if (channels) {
@@ -1577,15 +1578,18 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
 	if (!buffer)
 		return 0;
 
-	ret = __iio_buffer_alloc_sysfs_and_mask(buffer, indio_dev, 0);
-	if (ret)
-		return ret;
+	for (i = 0; i < iio_dev_opaque->attached_buffers_cnt; i++) {
+		buffer = iio_dev_opaque->attached_buffers[i];
+		ret = __iio_buffer_alloc_sysfs_and_mask(buffer, indio_dev, i);
+		if (ret)
+			goto error_unwind_sysfs_and_mask;
+	}
 
 	ret = sysfs_create_link(&indio_dev->dev.kobj,
 				&indio_dev->buffer->buffer_dir,
 				"buffer");
 	if (ret)
-		goto error_free_sysfs_and_mask;
+		goto error_unwind_sysfs_and_mask;
 
 	ret = sysfs_create_link(&indio_dev->dev.kobj,
 				&indio_dev->buffer->scan_el_dir,
@@ -1597,8 +1601,14 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
 
 error_remove_buffer_dir_link:
 	sysfs_remove_link(&indio_dev->dev.kobj, "buffer");
-error_free_sysfs_and_mask:
-	__iio_buffer_free_sysfs_and_mask(buffer);
+	cnt = iio_dev_opaque->attached_buffers_cnt - 1;
+error_unwind_sysfs_and_mask:
+	for (; cnt >= 0; cnt--) {
+		buffer = iio_dev_opaque->attached_buffers[cnt];
+		__iio_buffer_free_sysfs_and_mask(buffer);
+	}
+	kfree(iio_dev_opaque->attached_buffers);
+	iio_dev_opaque->attached_buffers = NULL;
 	return ret;
 }
 
@@ -1610,7 +1620,9 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
  */
 void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)
 {
+	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
 	struct iio_buffer *buffer = indio_dev->buffer;
+	int i;
 
 	if (!buffer)
 		return;
@@ -1618,7 +1630,13 @@ void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)
 	sysfs_remove_link(&indio_dev->dev.kobj, "scan_elements");
 	sysfs_remove_link(&indio_dev->dev.kobj, "buffer");
 
-	__iio_buffer_free_sysfs_and_mask(buffer);
+	for (i = iio_dev_opaque->attached_buffers_cnt - 1; i >= 0; i--) {
+		buffer = iio_dev_opaque->attached_buffers[i];
+		__iio_buffer_free_sysfs_and_mask(buffer);
+	}
+
+	kfree(iio_dev_opaque->attached_buffers);
+	iio_dev_opaque->attached_buffers = NULL;
 }
 
 /**
@@ -1750,15 +1768,36 @@ EXPORT_SYMBOL_GPL(iio_buffer_get_iio_dev);
  * @indio_dev: The device the buffer should be attached to
  * @buffer: The buffer to attach to the device
  *
+ * Return 0 if buffer attached, or negative if error occured.
+ *
  * 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.
+ * the device until the device is freed. For legacy reasons, the first attached
+ * buffer will also be assigned to 'indio_dev->buffer'.
  */
-void iio_device_attach_buffer(struct iio_dev *indio_dev,
-			      struct iio_buffer *buffer)
+int iio_device_attach_buffer(struct iio_dev *indio_dev,
+			     struct iio_buffer *buffer)
 {
-	indio_dev->buffer = iio_buffer_get(buffer);
+	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
+	struct iio_buffer **new, **old = iio_dev_opaque->attached_buffers;
+	unsigned int cnt = iio_dev_opaque->attached_buffers_cnt;
+
+	cnt++;
+
+	new = krealloc(old, sizeof(*new) * cnt, GFP_KERNEL);
+	if (!new)
+		return -ENOMEM;
 
-	indio_dev->buffer->indio_dev = indio_dev;
+	iio_dev_opaque->attached_buffers = new;
+
+	/* first buffer is legacy; attach it to the IIO device directly */
+	if (!indio_dev->buffer)
+		indio_dev->buffer = iio_buffer_get(buffer);
+
+	buffer->indio_dev = indio_dev;
+
+	iio_dev_opaque->attached_buffers[cnt - 1] = buffer;
+	iio_dev_opaque->attached_buffers_cnt = cnt;
+
+	return 0;
 }
 EXPORT_SYMBOL_GPL(iio_device_attach_buffer);
diff --git a/include/linux/iio/buffer.h b/include/linux/iio/buffer.h
index b27d8c81f32c..5532eddb7808 100644
--- a/include/linux/iio/buffer.h
+++ b/include/linux/iio/buffer.h
@@ -43,7 +43,7 @@ static inline int iio_push_to_buffers_with_timestamp(struct iio_dev *indio_dev,
 bool iio_validate_scan_mask_onehot(struct iio_dev *indio_dev,
 				   const unsigned long *mask);
 
-void iio_device_attach_buffer(struct iio_dev *indio_dev,
-			      struct iio_buffer *buffer);
+int iio_device_attach_buffer(struct iio_dev *indio_dev,
+			     struct iio_buffer *buffer);
 
 #endif /* _IIO_BUFFER_GENERIC_H_ */
diff --git a/include/linux/iio/buffer_impl.h b/include/linux/iio/buffer_impl.h
index 77e169e51434..e25d26a7f601 100644
--- a/include/linux/iio/buffer_impl.h
+++ b/include/linux/iio/buffer_impl.h
@@ -124,6 +124,9 @@ struct iio_buffer {
 	/* @demux_bounce: Buffer for doing gather from incoming scan. */
 	void *demux_bounce;
 
+	/* @attached_entry: Entry in the devices list of buffers attached by the driver. */
+	struct list_head attached_entry;
+
 	/* @buffer_list: Entry in the devices list of current buffers. */
 	struct list_head buffer_list;
 
diff --git a/include/linux/iio/iio-opaque.h b/include/linux/iio/iio-opaque.h
index 07c5a8e52ca8..1db0ea09520e 100644
--- a/include/linux/iio/iio-opaque.h
+++ b/include/linux/iio/iio-opaque.h
@@ -7,6 +7,8 @@
  * struct iio_dev_opaque - industrial I/O device opaque information
  * @indio_dev:			public industrial I/O device information
  * @event_interface:		event chrdevs associated with interrupt lines
+ * @attached_buffers:		array of buffers statically attached by the driver
+ * @attached_buffers_cnt:	number of buffers in the array of statically attached buffers
  * @buffer_list:		list of all buffers currently attached
  * @channel_attr_list:		keep track of automatically created channel
  *				attributes
@@ -20,6 +22,8 @@
 struct iio_dev_opaque {
 	struct iio_dev			indio_dev;
 	struct iio_event_interface	*event_interface;
+	struct iio_buffer		**attached_buffers;
+	unsigned int			attached_buffers_cnt;
 	struct list_head		buffer_list;
 	struct list_head		channel_attr_list;
 	struct attribute_group		chan_attr_group;
-- 
2.17.1

[PATCH v2 12/12][RESEND] iio: buffer: add ioctl() to support opening extra buffers for IIO device

[Alexandru Ardelean]

With this change, an ioctl() call is added to open a character device for a
buffer.
The ioctl() will return a 0 FD for the first buffer, as that FD for buffer0
is the same FD as the one used for this ioctl().

For any other extra buffer, this ioctl() will return an anon inode FD that
would access any extra buffer.

Right now, there doesn't seem to be (or I couldn't find) a way for this
ioctl() to return the FD for buffer0 (i.e. to return the same FD as used
for the ioctl()).
So, usespace would need to know that  ioctl(fd,
IIO_BUFFER_GET_BUFFER_COUNT, 0) will return FD 0.
We could also return another FD for buffer 0, but duplicating FDs for the
same IIO buffer sounds problematic.

Also, there is no IIO_BUFFER_GET_BUFFER_COUNT ioctl() implemented, as the
index for each buffer (and the count) can be deduced from the
'/sys/bus/iio/devices/iio:deviceX/bufferY' folders (i.e the number of
bufferY folders).

Used following C code to test this:
-------------------------------------------------------------------

 #include <stdio.h>
 #include <stdlib.h>
 #include <unistd.h>
 #include <sys/ioctl.h>
 #include <fcntl.h"
 #include <errno.h>

 #define IIO_BUFFER_GET_FD_IOCTL      _IOWR('i', 0xb0, int)

int main(int argc, char *argv[])
{
        int fd;
        int fd1;
        int ret;

        if ((fd = open("/dev/iio:device0", O_RDWR))<0) {
                fprintf(stderr, "Error open() %d errno %d\n",fd, errno);
                return -1;
        }

        fprintf(stderr, "Using FD %d\n", fd);

        fd1 = atoi(argv[1]);

        ret = ioctl(fd, IIO_BUFFER_GET_FD_IOCTL, &fd1);
        if (ret < 0) {
                fprintf(stderr, "Error for buffer %d ioctl() %d errno %d\n", fd1, ret, errno);
                close(fd);
                return -1;
        }

        fprintf(stderr, "Got FD %d\n", fd1);

        close(fd1);
        close(fd);

        return 0;
}
-------------------------------------------------------------------

Results are:
-------------------------------------------------------------------
 # ./test 0
 Using FD 3
 Got FD 0

 # ./test 1
 Using FD 3
 Got FD 4

 # ./test 2
 Using FD 3
 Got FD 4

 # ./test 3
 Using FD 3
 Got FD 4

 # ls /sys/bus/iio/devices/iio\:device0
 buffer  buffer0  buffer1  buffer2  buffer3  dev
 in_voltage_sampling_frequency  in_voltage_scale
 in_voltage_scale_available
 name  of_node  power  scan_elements  subsystem  uevent
-------------------------------------------------------------------

iio:device0 has some fake kfifo buffers attached to an IIO device.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/industrialio-buffer.c | 119 ++++++++++++++++++++++++++++++
 drivers/iio/industrialio-core.c   |   8 ++
 include/linux/iio/buffer_impl.h   |   5 ++
 include/linux/iio/iio-opaque.h    |   2 +
 include/uapi/linux/iio/buffer.h   |  10 +++
 5 files changed, 144 insertions(+)
 create mode 100644 include/uapi/linux/iio/buffer.h

diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index 2f429616e998..e619ec35c431 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -9,6 +9,7 @@
  * - Better memory allocation techniques?
  * - Alternative access techniques?
  */
+#include <linux/anon_inodes.h>
 #include <linux/kernel.h>
 #include <linux/export.h>
 #include <linux/device.h>
@@ -1399,6 +1400,106 @@ static void iio_sysfs_del_attrs(struct kobject *kobj, struct attribute **ptr)
 		sysfs_remove_file(kobj, ptr[i]);
 }
 
+static int iio_buffer_chrdev_release(struct inode *inode, struct file *filep)
+{
+	struct iio_dev_buffer_pair *ib = filep->private_data;
+	struct iio_dev *indio_dev = ib->indio_dev;
+	struct iio_buffer *buffer = ib->buffer;
+
+	clear_bit(IIO_BUSY_BIT_POS, &buffer->flags);
+	iio_device_put(indio_dev);
+	kfree(ib);
+
+	return 0;
+}
+
+static const struct file_operations iio_buffer_chrdev_fileops = {
+	.owner = THIS_MODULE,
+	.llseek = noop_llseek,
+	.read = iio_buffer_read_outer_addr,
+	.poll = iio_buffer_poll_addr,
+	.compat_ioctl = compat_ptr_ioctl,
+	.release = iio_buffer_chrdev_release,
+};
+
+static long iio_device_buffer_getfd(struct iio_dev *indio_dev, unsigned long arg)
+{
+	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
+	int __user *ival = (int __user *)arg;
+	char buf_name[sizeof("iio:buffer:xxx")];
+	struct iio_dev_buffer_pair *ib;
+	struct iio_buffer *buffer;
+	int fd, idx;
+
+	if (copy_from_user(&idx, ival, sizeof(idx)))
+		return -EFAULT;
+
+	if (idx == 0) {
+		fd = 0;
+		if (copy_to_user(ival, &fd, sizeof(fd)))
+			return -EFAULT;
+		return 0;
+	}
+
+	if (idx >= iio_dev_opaque->attached_buffers_cnt)
+		return -ENOENT;
+
+	fd = mutex_lock_interruptible(&indio_dev->mlock);
+	if (fd)
+		return fd;
+
+	buffer = iio_dev_opaque->attached_buffers[idx];
+
+	if (test_and_set_bit(IIO_BUSY_BIT_POS, &buffer->flags)) {
+		fd = -EBUSY;
+		goto error_unlock;
+	}
+
+	iio_device_get(indio_dev);
+
+	ib = kzalloc(sizeof(*ib), GFP_KERNEL);
+	if (!ib) {
+		fd = -ENOMEM;
+		goto error_iio_dev_put;
+	}
+
+	ib->indio_dev = indio_dev;
+	ib->buffer = buffer;
+
+	fd = anon_inode_getfd(buf_name, &iio_buffer_chrdev_fileops,
+			      ib, O_RDWR | O_CLOEXEC);
+	if (fd < 0)
+		goto error_free_ib;
+
+	if (copy_to_user(ival, &fd, sizeof(fd))) {
+		fd = -EFAULT;
+		goto error_free_ib;
+	}
+
+	mutex_unlock(&indio_dev->mlock);
+	return fd;
+
+error_free_ib:
+	kfree(ib);
+error_iio_dev_put:
+	iio_device_put(indio_dev);
+	clear_bit(IIO_BUSY_BIT_POS, &buffer->flags);
+error_unlock:
+	mutex_unlock(&indio_dev->mlock);
+	return fd;
+}
+
+static long iio_device_buffer_ioctl(struct iio_dev *indio_dev, struct file *filp,
+				    unsigned int cmd, unsigned long arg)
+{
+	switch (cmd) {
+	case IIO_BUFFER_GET_FD_IOCTL:
+		return iio_device_buffer_getfd(indio_dev, arg);
+	default:
+		return IIO_IOCTL_UNHANDLED;
+	}
+}
+
 /**
  * __iio_buffer_alloc_sysfs_and_mask() - Allocate sysfs attributes to an attached buffer
  * @buffer: the buffer object for which the sysfs attributes are created for
@@ -1565,6 +1666,7 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
 	struct iio_buffer *buffer = indio_dev->buffer;
 	const struct iio_chan_spec *channels;
 	int i, cnt, ret;
+	size_t sz;
 
 	channels = indio_dev->channels;
 	if (channels) {
@@ -1597,8 +1699,21 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
 	if (ret)
 		goto error_remove_buffer_dir_link;
 
+	sz = sizeof(*(iio_dev_opaque->buffer_ioctl_handler));
+	iio_dev_opaque->buffer_ioctl_handler = kzalloc(sz, GFP_KERNEL);
+	if (!iio_dev_opaque->buffer_ioctl_handler) {
+		ret = -ENOMEM;
+		goto error_remove_scan_el_dir;
+	}
+
+	iio_dev_opaque->buffer_ioctl_handler->ioctl = iio_device_buffer_ioctl;
+	iio_device_ioctl_handler_register(indio_dev,
+					  iio_dev_opaque->buffer_ioctl_handler);
+
 	return 0;
 
+error_remove_scan_el_dir:
+	sysfs_remove_link(&indio_dev->dev.kobj, "scan_elements");
 error_remove_buffer_dir_link:
 	sysfs_remove_link(&indio_dev->dev.kobj, "buffer");
 	cnt = iio_dev_opaque->attached_buffers_cnt - 1;
@@ -1627,6 +1742,10 @@ void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)
 	if (!buffer)
 		return;
 
+	iio_device_ioctl_handler_unregister(iio_dev_opaque->buffer_ioctl_handler);
+	kfree(iio_dev_opaque->buffer_ioctl_handler);
+	iio_dev_opaque->buffer_ioctl_handler = NULL;
+
 	sysfs_remove_link(&indio_dev->dev.kobj, "scan_elements");
 	sysfs_remove_link(&indio_dev->dev.kobj, "buffer");
 
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 0d1880837776..898992fd33c3 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -1683,6 +1683,9 @@ static int iio_chrdev_open(struct inode *inode, struct file *filp)
 	ib->indio_dev = indio_dev;
 	ib->buffer = indio_dev->buffer;
 
+	if (indio_dev->buffer)
+		test_and_set_bit(IIO_BUSY_BIT_POS, &indio_dev->buffer->flags);
+
 	filp->private_data = ib;
 
 	return 0;
@@ -1700,6 +1703,11 @@ static int iio_chrdev_release(struct inode *inode, struct file *filp)
 	struct iio_dev_buffer_pair *ib = filp->private_data;
 	struct iio_dev *indio_dev = container_of(inode->i_cdev,
 						struct iio_dev, chrdev);
+	struct iio_buffer *buffer = ib->buffer;
+
+	if (buffer)
+		clear_bit(IIO_BUSY_BIT_POS, &buffer->flags);
+
 	clear_bit(IIO_BUSY_BIT_POS, &indio_dev->flags);
 	iio_device_put(indio_dev);
 	kfree(ib);
diff --git a/include/linux/iio/buffer_impl.h b/include/linux/iio/buffer_impl.h
index e25d26a7f601..78da590b5607 100644
--- a/include/linux/iio/buffer_impl.h
+++ b/include/linux/iio/buffer_impl.h
@@ -6,6 +6,8 @@
 
 #ifdef CONFIG_IIO_BUFFER
 
+#include <uapi/linux/iio/buffer.h>
+
 struct iio_dev;
 struct iio_buffer;
 
@@ -75,6 +77,9 @@ struct iio_buffer {
 	/** @length: Number of datums in buffer. */
 	unsigned int length;
 
+	/** @flags: File ops flags including busy flag. */
+	unsigned long flags;
+
 	/**  @bytes_per_datum: Size of individual datum including timestamp. */
 	size_t bytes_per_datum;
 
diff --git a/include/linux/iio/iio-opaque.h b/include/linux/iio/iio-opaque.h
index 1db0ea09520e..d0429b13afa8 100644
--- a/include/linux/iio/iio-opaque.h
+++ b/include/linux/iio/iio-opaque.h
@@ -9,6 +9,7 @@
  * @event_interface:		event chrdevs associated with interrupt lines
  * @attached_buffers:		array of buffers statically attached by the driver
  * @attached_buffers_cnt:	number of buffers in the array of statically attached buffers
+ * @buffer_ioctl_handler:	ioctl() handler for this IIO device's buffer interface
  * @buffer_list:		list of all buffers currently attached
  * @channel_attr_list:		keep track of automatically created channel
  *				attributes
@@ -24,6 +25,7 @@ struct iio_dev_opaque {
 	struct iio_event_interface	*event_interface;
 	struct iio_buffer		**attached_buffers;
 	unsigned int			attached_buffers_cnt;
+	struct iio_ioctl_handler	*buffer_ioctl_handler;
 	struct list_head		buffer_list;
 	struct list_head		channel_attr_list;
 	struct attribute_group		chan_attr_group;
diff --git a/include/uapi/linux/iio/buffer.h b/include/uapi/linux/iio/buffer.h
new file mode 100644
index 000000000000..5455f7ff143d
--- /dev/null
+++ b/include/uapi/linux/iio/buffer.h
@@ -0,0 +1,10 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+/* industrial I/O buffer definitions needed both in and out of kernel
+ */
+
+#ifndef _UAPI_IIO_BUFFER_H_
+#define _UAPI_IIO_BUFFER_H_
+
+#define IIO_BUFFER_GET_FD_IOCTL		_IOWR('i', 0xb0, int)
+
+#endif /* _UAPI_IIO_BUFFER_H_ */
-- 
2.17.1

Re: [PATCH v2 03/12][RESEND] iio: buffer: rework buffer & scan_elements dir creation

[Jonathan Cameron]

On Fri, 22 Jan 2021 18:25:20 +0200
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> When adding more than one IIO buffer per IIO device, we will need to create
> a buffer & scan_elements directory for each buffer.
> We also want to move the 'scan_elements' to be a sub-directory of the
> 'buffer' folder.
> 
> The format we want to reach is, for a iio:device0 folder, for 2 buffers
> [for example], we have a 'buffer0' and a 'buffer1' subfolder, and each with
> it's own 'scan_elements' subfolder.
> 
> So, for example:
>    iio:device0/buffer0
>       scan_elements/
> 
>    iio:device0/buffer1
>       scan_elements/
> 
> The other attributes under 'bufferX' would remain unchanged.
> 
> However, we would also need to symlink back to the old 'buffer' &
> 'scan_elements' folders, to keep backwards compatibility.
> 
> Doing all these, require that we maintain the kobjects for each 'bufferX'
> and 'scan_elements' so that we can symlink them back. We also need to
> implement the sysfs_ops for these folders.
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>

+CC GregKH and Rafael W for feedback on various things inline.

It might be that this is the neatest solution that we can come up with but
more eyes would be good!

Whilst I think this looks fine, I'm less confident than I'd like to be.

Jonathan

> ---
>  drivers/iio/industrialio-buffer.c | 195 +++++++++++++++++++++++++++---
>  drivers/iio/industrialio-core.c   |  24 ++--
>  include/linux/iio/buffer_impl.h   |  14 ++-
>  include/linux/iio/iio.h           |   2 +-
>  4 files changed, 200 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index 0412c4fda4c1..0f470d902790 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -1175,8 +1175,6 @@ static ssize_t iio_buffer_store_enable(struct device *dev,
>  	return (ret < 0) ? ret : len;
>  }
>  
> -static const char * const iio_scan_elements_group_name = "scan_elements";
> -
>  static ssize_t iio_buffer_show_watermark(struct device *dev,
>  					 struct device_attribute *attr,
>  					 char *buf)
> @@ -1252,6 +1250,124 @@ static struct attribute *iio_buffer_attrs[] = {
>  	&dev_attr_data_available.attr,
>  };
>  
> +#define to_dev_attr(_attr) container_of(_attr, struct device_attribute, attr)
> +
> +static ssize_t iio_buffer_dir_attr_show(struct kobject *kobj,
> +					struct attribute *attr,
> +					char *buf)
> +{
> +	struct iio_buffer *buffer = container_of(kobj, struct iio_buffer, buffer_dir);
> +	struct device_attribute *dattr;
> +
> +	dattr = to_dev_attr(attr);
> +
> +	return dattr->show(&buffer->indio_dev->dev, dattr, buf);
> +}
> +
> +static ssize_t iio_buffer_dir_attr_store(struct kobject *kobj,
> +					 struct attribute *attr,
> +					 const char *buf,
> +					 size_t len)
> +{
> +	struct iio_buffer *buffer = container_of(kobj, struct iio_buffer, buffer_dir);
> +	struct device_attribute *dattr;
> +
> +	dattr = to_dev_attr(attr);
> +
> +	return dattr->store(&buffer->indio_dev->dev, dattr, buf, len);
> +}
> +
> +static const struct sysfs_ops iio_buffer_dir_sysfs_ops = {
> +	.show = iio_buffer_dir_attr_show,
> +	.store = iio_buffer_dir_attr_store,
> +};
> +
> +static struct kobj_type iio_buffer_dir_ktype = {
> +	.sysfs_ops = &iio_buffer_dir_sysfs_ops,
> +};
> +
> +static ssize_t iio_scan_el_dir_attr_show(struct kobject *kobj,
> +					 struct attribute *attr,
> +					 char *buf)
> +{
> +	struct iio_buffer *buffer = container_of(kobj, struct iio_buffer, scan_el_dir);
> +	struct device_attribute *dattr = to_dev_attr(attr);
> +
> +	return dattr->show(&buffer->indio_dev->dev, dattr, buf);
> +}
> +
> +static ssize_t iio_scan_el_dir_attr_store(struct kobject *kobj,
> +					  struct attribute *attr,
> +					  const char *buf,
> +					  size_t len)
> +{
> +	struct iio_buffer *buffer = container_of(kobj, struct iio_buffer, scan_el_dir);
> +	struct device_attribute *dattr = to_dev_attr(attr);
> +
> +	return dattr->store(&buffer->indio_dev->dev, dattr, buf, len);
> +}
> +
> +static const struct sysfs_ops iio_scan_el_dir_sysfs_ops = {
> +	.show = iio_scan_el_dir_attr_show,
> +	.store = iio_scan_el_dir_attr_store,
> +};
> +
> +static struct kobj_type iio_scan_el_dir_ktype = {
> +	.sysfs_ops = &iio_scan_el_dir_sysfs_ops,
> +};
> +
> +/*
> + * These iio_sysfs_{add,del}_attrs() are essentially re-implementations of
> + * sysfs_create_files() & sysfs_remove_files(), but they are meant to get
> + * around the const-pointer mismatch situation with using them.
> + *
> + * sysfs_{create,remove}_files() uses 'const struct attribute * const *ptr',
> + * while these are happy with just 'struct attribute **ptr'
> + */

Then to my mind the question becomes why sysfs_create_files() etc requires
the second level of const.  If there is justification for that relaxation here
we should make it more generally.

> +static int iio_sysfs_add_attrs(struct kobject *kobj, struct attribute **ptr)
> +{
> +	int err = 0;
> +	int i;
> +
> +	for (i = 0; ptr[i] && !err; i++)
> +		err = sysfs_create_file(kobj, ptr[i]);
> +	if (err)
> +		while (--i >= 0)
> +			sysfs_remove_file(kobj, ptr[i]);
> +	return err;
> +}
> +
> +static void iio_sysfs_del_attrs(struct kobject *kobj, struct attribute **ptr)
> +{
> +	int i;
> +
> +	for (i = 0; ptr[i]; i++)
> +		sysfs_remove_file(kobj, ptr[i]);
> +}
> +
> +/**
> + * __iio_buffer_alloc_sysfs_and_mask() - Allocate sysfs attributes to an attached buffer
> + * @buffer: the buffer object for which the sysfs attributes are created for
> + * @indio_dev: the iio device to which the iio buffer belongs to
> + *
> + * Return 0, or negative for error.
> + *
> + * This function must be called for each single buffer. The sysfs attributes for that
> + * buffer will be created, and the IIO device object will be the parent kobject of that
> + * the kobjects created here.
> + * Because we need to redirect sysfs attribute to it's IIO buffer object, we need to
> + * implement our own sysfs_ops, and each IIO buffer will keep a kobject for the
> + * 'bufferX' directory and one for the 'scan_elements' directory.
> + * And in order to do this, this function must be called after the IIO device object
> + * has been added via device_add(). This fundamentally changes how sysfs attributes
> + * were created before (with one single IIO buffer per IIO device), where the
> + * sysfs attributes for the buffer were mapped as attribute groups on the IIO device
> + * groups object list.

Been a while, so I can't recall the exact reasons this can cause problems.
I've +CC Greg and Rafael for comments.

For their reference, the aim of this set is undo an old restriction on IIO that devices
only had one buffer.  To do that we need to keep a iio:deviceX/buffer0 directory
also exposed as iio:deviceX/buffer/* and
iio:deviceX/buffer0/scan_elements/ as iio:deviceX/scan_elements.
to maintain backwards compatibility.


> + * Using kobjects directly for the 'bufferX' and 'scan_elements' directories allows
> + * us to symlink them back to keep backwards compatibility for the old sysfs interface
> + * for IIO buffers while also allowing us to support multiple IIO buffers per one
> + * IIO device.
> + */
>  static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
>  					     struct iio_dev *indio_dev)
>  {
> @@ -1282,12 +1398,16 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
>  		memcpy(&attr[ARRAY_SIZE(iio_buffer_attrs)], buffer->attrs,
>  		       sizeof(struct attribute *) * attrcount);
>  
> -	attr[attrcount + ARRAY_SIZE(iio_buffer_attrs)] = NULL;
> +	buffer->buffer_attrs = attr;
>  
> -	buffer->buffer_group.name = "buffer";
> -	buffer->buffer_group.attrs = attr;
> +	ret = kobject_init_and_add(&buffer->buffer_dir, &iio_buffer_dir_ktype,
> +				   &indio_dev->dev.kobj, "buffer");
> +	if (ret)
> +		goto error_buffer_free_attrs;
>  

Do we potentially want kobject_uevent calls for these?
(based on looking at similar code in edac).

> -	indio_dev->groups[indio_dev->groupcounter++] = &buffer->buffer_group;
> +	ret = iio_sysfs_add_attrs(&buffer->buffer_dir, buffer->buffer_attrs);
> +	if (ret)
> +		goto error_buffer_kobject_put;
>  
>  	attrcount = 0;
>  	INIT_LIST_HEAD(&buffer->scan_el_dev_attr_list);
> @@ -1317,32 +1437,57 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
>  		}
>  	}
>  
> -	buffer->scan_el_group.name = iio_scan_elements_group_name;
> -
> -	buffer->scan_el_group.attrs = kcalloc(attrcount + 1,
> -					      sizeof(buffer->scan_el_group.attrs[0]),
> -					      GFP_KERNEL);
> -	if (buffer->scan_el_group.attrs == NULL) {
> +	buffer->scan_el_attrs = kcalloc(attrcount + 1,
> +					sizeof(buffer->scan_el_attrs[0]),
> +					GFP_KERNEL);
> +	if (buffer->scan_el_attrs == NULL) {
>  		ret = -ENOMEM;
>  		goto error_free_scan_mask;
>  	}
> -	attrn = 0;
>  
> +	ret = kobject_init_and_add(&buffer->scan_el_dir, &iio_scan_el_dir_ktype,
> +				   &indio_dev->dev.kobj, "scan_elements");
> +	if (ret)
> +		goto error_free_scan_attrs;
> +
> +	attrn = 0;
>  	list_for_each_entry(p, &buffer->scan_el_dev_attr_list, l)
> -		buffer->scan_el_group.attrs[attrn++] = &p->dev_attr.attr;
> -	indio_dev->groups[indio_dev->groupcounter++] = &buffer->scan_el_group;
> +		buffer->scan_el_attrs[attrn++] = &p->dev_attr.attr;
> +
> +	ret = iio_sysfs_add_attrs(&buffer->scan_el_dir, buffer->scan_el_attrs);
> +	if (ret)
> +		goto error_scan_kobject_put;
>  
>  	return 0;
>  
> +error_scan_kobject_put:
> +	kobject_put(&buffer->scan_el_dir);
> +error_free_scan_attrs:
> +	kfree(buffer->scan_el_attrs);
>  error_free_scan_mask:
>  	bitmap_free(buffer->scan_mask);
>  error_cleanup_dynamic:
> +	iio_sysfs_del_attrs(&buffer->buffer_dir, buffer->buffer_attrs);
>  	iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list);
> -	kfree(buffer->buffer_group.attrs);
> +error_buffer_kobject_put:
> +	kobject_put(&buffer->buffer_dir);
> +error_buffer_free_attrs:
> +	kfree(buffer->buffer_attrs);
>  
>  	return ret;
>  }
>  
> +/**
> + * iio_buffer_alloc_sysfs_and_mask() - Allocate sysfs attributes to attached buffers
> + * @indio_dev: the iio device for which to create the buffer sysfs attributes
> + *
> + * Return 0, or negative for error.
> + *
> + * If the IIO device has no buffer attached, no sysfs attributes will be created.
> + * This function must be called after the IIO device object has been created and
> + * registered with device_add(). See __iio_buffer_alloc_sysfs_and_mask() for more
> + * details.
> + */
>  int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
>  {
>  	struct iio_buffer *buffer = indio_dev->buffer;
> @@ -1364,14 +1509,28 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
>  	return __iio_buffer_alloc_sysfs_and_mask(buffer, indio_dev);
>  }
>  
> +/**
> + * __iio_buffer_free_sysfs_and_mask() - Free sysfs objects for a single IIO buffer
> + * @buffer: the iio buffer for which to destroy the objects
> + */
>  static void __iio_buffer_free_sysfs_and_mask(struct iio_buffer *buffer)
>  {
> +	iio_sysfs_del_attrs(&buffer->scan_el_dir, buffer->scan_el_attrs);
> +	kobject_put(&buffer->scan_el_dir);
> +	kfree(buffer->scan_el_attrs);
>  	bitmap_free(buffer->scan_mask);
> -	kfree(buffer->buffer_group.attrs);
> -	kfree(buffer->scan_el_group.attrs);
> +	iio_sysfs_del_attrs(&buffer->buffer_dir, buffer->buffer_attrs);
>  	iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list);
> +	kobject_put(&buffer->buffer_dir);
> +	kfree(buffer->buffer_attrs);
>  }
>  
> +/**
> + * iio_buffer_free_sysfs_and_mask() - Free sysfs objects for all IIO buffers
> + * @indio_dev: the iio device for which to destroy the objects
> + *
> + * If the IIO device has no buffer attached, nothing will be done.
> + */
>  void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)
>  {
>  	struct iio_buffer *buffer = indio_dev->buffer;
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index 0a6fd299a978..95d66745f118 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -1817,18 +1817,11 @@ int __iio_device_register(struct iio_dev *indio_dev, struct module *this_mod)
>  
>  	iio_device_register_debugfs(indio_dev);
>  
> -	ret = iio_buffer_alloc_sysfs_and_mask(indio_dev);
> -	if (ret) {
> -		dev_err(indio_dev->dev.parent,
> -			"Failed to create buffer sysfs interfaces\n");
> -		goto error_unreg_debugfs;
> -	}
> -
>  	ret = iio_device_register_sysfs(indio_dev);
>  	if (ret) {
>  		dev_err(indio_dev->dev.parent,
>  			"Failed to register sysfs interfaces\n");
> -		goto error_buffer_free_sysfs;
> +		goto error_unreg_debugfs;
>  	}
>  	ret = iio_device_register_eventset(indio_dev);
>  	if (ret) {
> @@ -1857,14 +1850,21 @@ int __iio_device_register(struct iio_dev *indio_dev, struct module *this_mod)
>  	if (ret < 0)
>  		goto error_unreg_eventset;
>  
> +	ret = iio_buffer_alloc_sysfs_and_mask(indio_dev);
> +	if (ret) {
> +		dev_err(indio_dev->dev.parent,
> +			"Failed to create buffer sysfs interfaces\n");
> +		goto error_device_del;
> +	}
> +
>  	return 0;
>  
> +error_device_del:
> +	cdev_device_del(&indio_dev->chrdev, &indio_dev->dev);
>  error_unreg_eventset:
>  	iio_device_unregister_eventset(indio_dev);
>  error_free_sysfs:
>  	iio_device_unregister_sysfs(indio_dev);
> -error_buffer_free_sysfs:
> -	iio_buffer_free_sysfs_and_mask(indio_dev);
>  error_unreg_debugfs:
>  	iio_device_unregister_debugfs(indio_dev);
>  	return ret;
> @@ -1880,6 +1880,8 @@ void iio_device_unregister(struct iio_dev *indio_dev)
>  	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
>  	struct iio_ioctl_handler *h, *t;
>  
> +	iio_buffer_free_sysfs_and_mask(indio_dev);
> +
>  	cdev_device_del(&indio_dev->chrdev, &indio_dev->dev);
>  
>  	mutex_lock(&indio_dev->info_exist_lock);
> @@ -1897,8 +1899,6 @@ void iio_device_unregister(struct iio_dev *indio_dev)
>  	iio_buffer_wakeup_poll(indio_dev);
>  
>  	mutex_unlock(&indio_dev->info_exist_lock);
> -
> -	iio_buffer_free_sysfs_and_mask(indio_dev);
>  }
>  EXPORT_SYMBOL(iio_device_unregister);
>  
> diff --git a/include/linux/iio/buffer_impl.h b/include/linux/iio/buffer_impl.h
> index 67d73d465e02..77e169e51434 100644
> --- a/include/linux/iio/buffer_impl.h
> +++ b/include/linux/iio/buffer_impl.h
> @@ -103,14 +103,20 @@ struct iio_buffer {
>  	/* @scan_el_dev_attr_list: List of scan element related attributes. */
>  	struct list_head scan_el_dev_attr_list;
>  
> -	/* @buffer_group: Attributes of the buffer group. */
> -	struct attribute_group buffer_group;
> +	/* @buffer_dir: kobject for the 'buffer' directory of this buffer */
> +	struct kobject buffer_dir;
> +
> +	/* @buffer_attrs: Attributes of the buffer group. */
> +	struct attribute **buffer_attrs;
> +
> +	/* @scan_el_dir: kobject for the 'scan_elements' directory of this buffer */
> +	struct kobject scan_el_dir;
>  
>  	/*
> -	 * @scan_el_group: Attribute group for those attributes not
> +	 * @scan_el_attrs: Array of attributes for those attributes not
>  	 * created from the iio_chan_info array.
>  	 */
> -	struct attribute_group scan_el_group;
> +	struct attribute **scan_el_attrs;
>  
>  	/* @attrs: Standard attributes of the buffer. */
>  	const struct attribute **attrs;
> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> index e4a9822e6495..59b317dc45b8 100644
> --- a/include/linux/iio/iio.h
> +++ b/include/linux/iio/iio.h
> @@ -556,7 +556,7 @@ struct iio_dev {
>  	struct mutex			info_exist_lock;
>  	const struct iio_buffer_setup_ops	*setup_ops;
>  	struct cdev			chrdev;
> -#define IIO_MAX_GROUPS 6
> +#define IIO_MAX_GROUPS 4
>  	const struct attribute_group	*groups[IIO_MAX_GROUPS + 1];
>  	int				groupcounter;
>  

Re: [PATCH v2 12/12][RESEND] iio: buffer: add ioctl() to support opening extra buffers for IIO device

[Jonathan Cameron]

On Fri, 22 Jan 2021 18:25:29 +0200
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> With this change, an ioctl() call is added to open a character device for a
> buffer.
> The ioctl() will return a 0 FD for the first buffer, as that FD for buffer0
> is the same FD as the one used for this ioctl().
> 
> For any other extra buffer, this ioctl() will return an anon inode FD that
> would access any extra buffer.
> 
> Right now, there doesn't seem to be (or I couldn't find) a way for this
> ioctl() to return the FD for buffer0 (i.e. to return the same FD as used
> for the ioctl()).
> So, usespace would need to know that  ioctl(fd,
> IIO_BUFFER_GET_BUFFER_COUNT, 0) will return FD 0.
> We could also return another FD for buffer 0, but duplicating FDs for the
> same IIO buffer sounds problematic.

Agreed. For now this is fine.  Userspace doing that request is a bit
odd in general even if I can see it might be convenient.

Hohum. Now in theory ioctl numbers should be added to 
Documentation/userspace-api/ioctl/ioctl-number.rst but oops
seems I never added the one we already use.

Seems no one is using the block we are though so should be fine.
If you fancy adding this one and the event one we already have that would be great.
Perhaps we are allowing a bit too much space though given the
event one is down at 'i', 0x90 and this is 'i', 0xB0

We'll probably hit something undocumented anyway ;)

So from a fairly light level of read through this seems to have come together
nicely. I'm a bit nervous about the sysfs kobject stuff, but it seems there
are other places where similar stuff happens so if Rafael and Greg don't come
back with major comments that should be fine.

Jonathan

> 
> Also, there is no IIO_BUFFER_GET_BUFFER_COUNT ioctl() implemented, as the
> index for each buffer (and the count) can be deduced from the
> '/sys/bus/iio/devices/iio:deviceX/bufferY' folders (i.e the number of
> bufferY folders).
> 
> Used following C code to test this:
> -------------------------------------------------------------------
> 
>  #include <stdio.h>
>  #include <stdlib.h>
>  #include <unistd.h>
>  #include <sys/ioctl.h>
>  #include <fcntl.h"
>  #include <errno.h>
> 
>  #define IIO_BUFFER_GET_FD_IOCTL      _IOWR('i', 0xb0, int)
> 
> int main(int argc, char *argv[])
> {
>         int fd;
>         int fd1;
>         int ret;
> 
>         if ((fd = open("/dev/iio:device0", O_RDWR))<0) {
>                 fprintf(stderr, "Error open() %d errno %d\n",fd, errno);
>                 return -1;
>         }
> 
>         fprintf(stderr, "Using FD %d\n", fd);
> 
>         fd1 = atoi(argv[1]);
> 
>         ret = ioctl(fd, IIO_BUFFER_GET_FD_IOCTL, &fd1);
>         if (ret < 0) {
>                 fprintf(stderr, "Error for buffer %d ioctl() %d errno %d\n", fd1, ret, errno);
>                 close(fd);
>                 return -1;
>         }
> 
>         fprintf(stderr, "Got FD %d\n", fd1);
> 
>         close(fd1);
>         close(fd);
> 
>         return 0;
> }
> -------------------------------------------------------------------
> 
> Results are:
> -------------------------------------------------------------------
>  # ./test 0
>  Using FD 3
>  Got FD 0
> 
>  # ./test 1
>  Using FD 3
>  Got FD 4
> 
>  # ./test 2
>  Using FD 3
>  Got FD 4
> 
>  # ./test 3
>  Using FD 3
>  Got FD 4
> 
>  # ls /sys/bus/iio/devices/iio\:device0
>  buffer  buffer0  buffer1  buffer2  buffer3  dev
>  in_voltage_sampling_frequency  in_voltage_scale
>  in_voltage_scale_available
>  name  of_node  power  scan_elements  subsystem  uevent
> -------------------------------------------------------------------
> 
> iio:device0 has some fake kfifo buffers attached to an IIO device.
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> ---
>  drivers/iio/industrialio-buffer.c | 119 ++++++++++++++++++++++++++++++
>  drivers/iio/industrialio-core.c   |   8 ++
>  include/linux/iio/buffer_impl.h   |   5 ++
>  include/linux/iio/iio-opaque.h    |   2 +
>  include/uapi/linux/iio/buffer.h   |  10 +++
>  5 files changed, 144 insertions(+)
>  create mode 100644 include/uapi/linux/iio/buffer.h
> 
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index 2f429616e998..e619ec35c431 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -9,6 +9,7 @@
>   * - Better memory allocation techniques?
>   * - Alternative access techniques?
>   */
> +#include <linux/anon_inodes.h>
>  #include <linux/kernel.h>
>  #include <linux/export.h>
>  #include <linux/device.h>
> @@ -1399,6 +1400,106 @@ static void iio_sysfs_del_attrs(struct kobject *kobj, struct attribute **ptr)
>  		sysfs_remove_file(kobj, ptr[i]);
>  }
>  
> +static int iio_buffer_chrdev_release(struct inode *inode, struct file *filep)
> +{
> +	struct iio_dev_buffer_pair *ib = filep->private_data;
> +	struct iio_dev *indio_dev = ib->indio_dev;
> +	struct iio_buffer *buffer = ib->buffer;
> +
> +	clear_bit(IIO_BUSY_BIT_POS, &buffer->flags);
> +	iio_device_put(indio_dev);
> +	kfree(ib);
> +
> +	return 0;
> +}
> +
> +static const struct file_operations iio_buffer_chrdev_fileops = {
> +	.owner = THIS_MODULE,
> +	.llseek = noop_llseek,
> +	.read = iio_buffer_read_outer_addr,
> +	.poll = iio_buffer_poll_addr,
> +	.compat_ioctl = compat_ptr_ioctl,
> +	.release = iio_buffer_chrdev_release,
> +};
> +
> +static long iio_device_buffer_getfd(struct iio_dev *indio_dev, unsigned long arg)
> +{
> +	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
> +	int __user *ival = (int __user *)arg;
> +	char buf_name[sizeof("iio:buffer:xxx")];
> +	struct iio_dev_buffer_pair *ib;
> +	struct iio_buffer *buffer;
> +	int fd, idx;
> +
> +	if (copy_from_user(&idx, ival, sizeof(idx)))
> +		return -EFAULT;
> +
> +	if (idx == 0) {
> +		fd = 0;
> +		if (copy_to_user(ival, &fd, sizeof(fd)))
> +			return -EFAULT;
> +		return 0;
> +	}
> +
> +	if (idx >= iio_dev_opaque->attached_buffers_cnt)
> +		return -ENOENT;
> +
> +	fd = mutex_lock_interruptible(&indio_dev->mlock);
> +	if (fd)
> +		return fd;
> +
> +	buffer = iio_dev_opaque->attached_buffers[idx];
> +
> +	if (test_and_set_bit(IIO_BUSY_BIT_POS, &buffer->flags)) {
> +		fd = -EBUSY;
> +		goto error_unlock;
> +	}
> +
> +	iio_device_get(indio_dev);
> +
> +	ib = kzalloc(sizeof(*ib), GFP_KERNEL);
> +	if (!ib) {
> +		fd = -ENOMEM;
> +		goto error_iio_dev_put;
> +	}
> +
> +	ib->indio_dev = indio_dev;
> +	ib->buffer = buffer;
> +
> +	fd = anon_inode_getfd(buf_name, &iio_buffer_chrdev_fileops,
> +			      ib, O_RDWR | O_CLOEXEC);
> +	if (fd < 0)
> +		goto error_free_ib;
> +
> +	if (copy_to_user(ival, &fd, sizeof(fd))) {
> +		fd = -EFAULT;
> +		goto error_free_ib;
> +	}
> +
> +	mutex_unlock(&indio_dev->mlock);
> +	return fd;
> +
> +error_free_ib:
> +	kfree(ib);
> +error_iio_dev_put:
> +	iio_device_put(indio_dev);
> +	clear_bit(IIO_BUSY_BIT_POS, &buffer->flags);
> +error_unlock:
> +	mutex_unlock(&indio_dev->mlock);
> +	return fd;
> +}
> +
> +static long iio_device_buffer_ioctl(struct iio_dev *indio_dev, struct file *filp,
> +				    unsigned int cmd, unsigned long arg)
> +{
> +	switch (cmd) {
> +	case IIO_BUFFER_GET_FD_IOCTL:
> +		return iio_device_buffer_getfd(indio_dev, arg);
> +	default:
> +		return IIO_IOCTL_UNHANDLED;
> +	}
> +}
> +
>  /**
>   * __iio_buffer_alloc_sysfs_and_mask() - Allocate sysfs attributes to an attached buffer
>   * @buffer: the buffer object for which the sysfs attributes are created for
> @@ -1565,6 +1666,7 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
>  	struct iio_buffer *buffer = indio_dev->buffer;
>  	const struct iio_chan_spec *channels;
>  	int i, cnt, ret;
> +	size_t sz;
>  
>  	channels = indio_dev->channels;
>  	if (channels) {
> @@ -1597,8 +1699,21 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
>  	if (ret)
>  		goto error_remove_buffer_dir_link;
>  
> +	sz = sizeof(*(iio_dev_opaque->buffer_ioctl_handler));
> +	iio_dev_opaque->buffer_ioctl_handler = kzalloc(sz, GFP_KERNEL);
> +	if (!iio_dev_opaque->buffer_ioctl_handler) {
> +		ret = -ENOMEM;
> +		goto error_remove_scan_el_dir;
> +	}
> +
> +	iio_dev_opaque->buffer_ioctl_handler->ioctl = iio_device_buffer_ioctl;
> +	iio_device_ioctl_handler_register(indio_dev,
> +					  iio_dev_opaque->buffer_ioctl_handler);
> +
>  	return 0;
>  
> +error_remove_scan_el_dir:
> +	sysfs_remove_link(&indio_dev->dev.kobj, "scan_elements");
>  error_remove_buffer_dir_link:
>  	sysfs_remove_link(&indio_dev->dev.kobj, "buffer");
>  	cnt = iio_dev_opaque->attached_buffers_cnt - 1;
> @@ -1627,6 +1742,10 @@ void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)
>  	if (!buffer)
>  		return;
>  
> +	iio_device_ioctl_handler_unregister(iio_dev_opaque->buffer_ioctl_handler);
> +	kfree(iio_dev_opaque->buffer_ioctl_handler);
> +	iio_dev_opaque->buffer_ioctl_handler = NULL;
> +
>  	sysfs_remove_link(&indio_dev->dev.kobj, "scan_elements");
>  	sysfs_remove_link(&indio_dev->dev.kobj, "buffer");
>  
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index 0d1880837776..898992fd33c3 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -1683,6 +1683,9 @@ static int iio_chrdev_open(struct inode *inode, struct file *filp)
>  	ib->indio_dev = indio_dev;
>  	ib->buffer = indio_dev->buffer;
>  
> +	if (indio_dev->buffer)
> +		test_and_set_bit(IIO_BUSY_BIT_POS, &indio_dev->buffer->flags);
> +
>  	filp->private_data = ib;
>  
>  	return 0;
> @@ -1700,6 +1703,11 @@ static int iio_chrdev_release(struct inode *inode, struct file *filp)
>  	struct iio_dev_buffer_pair *ib = filp->private_data;
>  	struct iio_dev *indio_dev = container_of(inode->i_cdev,
>  						struct iio_dev, chrdev);
> +	struct iio_buffer *buffer = ib->buffer;
> +
> +	if (buffer)
> +		clear_bit(IIO_BUSY_BIT_POS, &buffer->flags);
> +
>  	clear_bit(IIO_BUSY_BIT_POS, &indio_dev->flags);
>  	iio_device_put(indio_dev);
>  	kfree(ib);
> diff --git a/include/linux/iio/buffer_impl.h b/include/linux/iio/buffer_impl.h
> index e25d26a7f601..78da590b5607 100644
> --- a/include/linux/iio/buffer_impl.h
> +++ b/include/linux/iio/buffer_impl.h
> @@ -6,6 +6,8 @@
>  
>  #ifdef CONFIG_IIO_BUFFER
>  
> +#include <uapi/linux/iio/buffer.h>
> +
>  struct iio_dev;
>  struct iio_buffer;
>  
> @@ -75,6 +77,9 @@ struct iio_buffer {
>  	/** @length: Number of datums in buffer. */
>  	unsigned int length;
>  
> +	/** @flags: File ops flags including busy flag. */
> +	unsigned long flags;
> +
>  	/**  @bytes_per_datum: Size of individual datum including timestamp. */
>  	size_t bytes_per_datum;
>  
> diff --git a/include/linux/iio/iio-opaque.h b/include/linux/iio/iio-opaque.h
> index 1db0ea09520e..d0429b13afa8 100644
> --- a/include/linux/iio/iio-opaque.h
> +++ b/include/linux/iio/iio-opaque.h
> @@ -9,6 +9,7 @@
>   * @event_interface:		event chrdevs associated with interrupt lines
>   * @attached_buffers:		array of buffers statically attached by the driver
>   * @attached_buffers_cnt:	number of buffers in the array of statically attached buffers
> + * @buffer_ioctl_handler:	ioctl() handler for this IIO device's buffer interface
>   * @buffer_list:		list of all buffers currently attached
>   * @channel_attr_list:		keep track of automatically created channel
>   *				attributes
> @@ -24,6 +25,7 @@ struct iio_dev_opaque {
>  	struct iio_event_interface	*event_interface;
>  	struct iio_buffer		**attached_buffers;
>  	unsigned int			attached_buffers_cnt;
> +	struct iio_ioctl_handler	*buffer_ioctl_handler;
>  	struct list_head		buffer_list;
>  	struct list_head		channel_attr_list;
>  	struct attribute_group		chan_attr_group;
> diff --git a/include/uapi/linux/iio/buffer.h b/include/uapi/linux/iio/buffer.h
> new file mode 100644
> index 000000000000..5455f7ff143d
> --- /dev/null
> +++ b/include/uapi/linux/iio/buffer.h
> @@ -0,0 +1,10 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +/* industrial I/O buffer definitions needed both in and out of kernel
> + */
> +
> +#ifndef _UAPI_IIO_BUFFER_H_
> +#define _UAPI_IIO_BUFFER_H_
> +
> +#define IIO_BUFFER_GET_FD_IOCTL		_IOWR('i', 0xb0, int)
> +
> +#endif /* _UAPI_IIO_BUFFER_H_ */

Re: [PATCH v2 03/12][RESEND] iio: buffer: rework buffer & scan_elements dir creation

[Alexandru Ardelean]

On Sun, Jan 24, 2021 at 8:13 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Fri, 22 Jan 2021 18:25:20 +0200
> Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
>
> > When adding more than one IIO buffer per IIO device, we will need to create
> > a buffer & scan_elements directory for each buffer.
> > We also want to move the 'scan_elements' to be a sub-directory of the
> > 'buffer' folder.
> >
> > The format we want to reach is, for a iio:device0 folder, for 2 buffers
> > [for example], we have a 'buffer0' and a 'buffer1' subfolder, and each with
> > it's own 'scan_elements' subfolder.
> >
> > So, for example:
> >    iio:device0/buffer0
> >       scan_elements/
> >
> >    iio:device0/buffer1
> >       scan_elements/
> >
> > The other attributes under 'bufferX' would remain unchanged.
> >
> > However, we would also need to symlink back to the old 'buffer' &
> > 'scan_elements' folders, to keep backwards compatibility.
> >
> > Doing all these, require that we maintain the kobjects for each 'bufferX'
> > and 'scan_elements' so that we can symlink them back. We also need to
> > implement the sysfs_ops for these folders.
> >
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
>
> +CC GregKH and Rafael W for feedback on various things inline.
>
> It might be that this is the neatest solution that we can come up with but
> more eyes would be good!
>
> Whilst I think this looks fine, I'm less confident than I'd like to be.
>
> Jonathan
>
> > ---
> >  drivers/iio/industrialio-buffer.c | 195 +++++++++++++++++++++++++++---
> >  drivers/iio/industrialio-core.c   |  24 ++--
> >  include/linux/iio/buffer_impl.h   |  14 ++-
> >  include/linux/iio/iio.h           |   2 +-
> >  4 files changed, 200 insertions(+), 35 deletions(-)
> >
> > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> > index 0412c4fda4c1..0f470d902790 100644
> > --- a/drivers/iio/industrialio-buffer.c
> > +++ b/drivers/iio/industrialio-buffer.c
> > @@ -1175,8 +1175,6 @@ static ssize_t iio_buffer_store_enable(struct device *dev,
> >       return (ret < 0) ? ret : len;
> >  }
> >
> > -static const char * const iio_scan_elements_group_name = "scan_elements";
> > -
> >  static ssize_t iio_buffer_show_watermark(struct device *dev,
> >                                        struct device_attribute *attr,
> >                                        char *buf)
> > @@ -1252,6 +1250,124 @@ static struct attribute *iio_buffer_attrs[] = {
> >       &dev_attr_data_available.attr,
> >  };
> >
> > +#define to_dev_attr(_attr) container_of(_attr, struct device_attribute, attr)
> > +
> > +static ssize_t iio_buffer_dir_attr_show(struct kobject *kobj,
> > +                                     struct attribute *attr,
> > +                                     char *buf)
> > +{
> > +     struct iio_buffer *buffer = container_of(kobj, struct iio_buffer, buffer_dir);
> > +     struct device_attribute *dattr;
> > +
> > +     dattr = to_dev_attr(attr);
> > +
> > +     return dattr->show(&buffer->indio_dev->dev, dattr, buf);
> > +}
> > +
> > +static ssize_t iio_buffer_dir_attr_store(struct kobject *kobj,
> > +                                      struct attribute *attr,
> > +                                      const char *buf,
> > +                                      size_t len)
> > +{
> > +     struct iio_buffer *buffer = container_of(kobj, struct iio_buffer, buffer_dir);
> > +     struct device_attribute *dattr;
> > +
> > +     dattr = to_dev_attr(attr);
> > +
> > +     return dattr->store(&buffer->indio_dev->dev, dattr, buf, len);
> > +}
> > +
> > +static const struct sysfs_ops iio_buffer_dir_sysfs_ops = {
> > +     .show = iio_buffer_dir_attr_show,
> > +     .store = iio_buffer_dir_attr_store,
> > +};
> > +
> > +static struct kobj_type iio_buffer_dir_ktype = {
> > +     .sysfs_ops = &iio_buffer_dir_sysfs_ops,
> > +};
> > +
> > +static ssize_t iio_scan_el_dir_attr_show(struct kobject *kobj,
> > +                                      struct attribute *attr,
> > +                                      char *buf)
> > +{
> > +     struct iio_buffer *buffer = container_of(kobj, struct iio_buffer, scan_el_dir);
> > +     struct device_attribute *dattr = to_dev_attr(attr);
> > +
> > +     return dattr->show(&buffer->indio_dev->dev, dattr, buf);
> > +}
> > +
> > +static ssize_t iio_scan_el_dir_attr_store(struct kobject *kobj,
> > +                                       struct attribute *attr,
> > +                                       const char *buf,
> > +                                       size_t len)
> > +{
> > +     struct iio_buffer *buffer = container_of(kobj, struct iio_buffer, scan_el_dir);
> > +     struct device_attribute *dattr = to_dev_attr(attr);
> > +
> > +     return dattr->store(&buffer->indio_dev->dev, dattr, buf, len);
> > +}
> > +
> > +static const struct sysfs_ops iio_scan_el_dir_sysfs_ops = {
> > +     .show = iio_scan_el_dir_attr_show,
> > +     .store = iio_scan_el_dir_attr_store,
> > +};
> > +
> > +static struct kobj_type iio_scan_el_dir_ktype = {
> > +     .sysfs_ops = &iio_scan_el_dir_sysfs_ops,
> > +};
> > +
> > +/*
> > + * These iio_sysfs_{add,del}_attrs() are essentially re-implementations of
> > + * sysfs_create_files() & sysfs_remove_files(), but they are meant to get
> > + * around the const-pointer mismatch situation with using them.
> > + *
> > + * sysfs_{create,remove}_files() uses 'const struct attribute * const *ptr',
> > + * while these are happy with just 'struct attribute **ptr'
> > + */
>
> Then to my mind the question becomes why sysfs_create_files() etc requires
> the second level of const.  If there is justification for that relaxation here
> we should make it more generally.
>
> > +static int iio_sysfs_add_attrs(struct kobject *kobj, struct attribute **ptr)
> > +{
> > +     int err = 0;
> > +     int i;
> > +
> > +     for (i = 0; ptr[i] && !err; i++)
> > +             err = sysfs_create_file(kobj, ptr[i]);
> > +     if (err)
> > +             while (--i >= 0)
> > +                     sysfs_remove_file(kobj, ptr[i]);
> > +     return err;
> > +}
> > +
> > +static void iio_sysfs_del_attrs(struct kobject *kobj, struct attribute **ptr)
> > +{
> > +     int i;
> > +
> > +     for (i = 0; ptr[i]; i++)
> > +             sysfs_remove_file(kobj, ptr[i]);
> > +}
> > +
> > +/**
> > + * __iio_buffer_alloc_sysfs_and_mask() - Allocate sysfs attributes to an attached buffer
> > + * @buffer: the buffer object for which the sysfs attributes are created for
> > + * @indio_dev: the iio device to which the iio buffer belongs to
> > + *
> > + * Return 0, or negative for error.
> > + *
> > + * This function must be called for each single buffer. The sysfs attributes for that
> > + * buffer will be created, and the IIO device object will be the parent kobject of that
> > + * the kobjects created here.
> > + * Because we need to redirect sysfs attribute to it's IIO buffer object, we need to
> > + * implement our own sysfs_ops, and each IIO buffer will keep a kobject for the
> > + * 'bufferX' directory and one for the 'scan_elements' directory.
> > + * And in order to do this, this function must be called after the IIO device object
> > + * has been added via device_add(). This fundamentally changes how sysfs attributes
> > + * were created before (with one single IIO buffer per IIO device), where the
> > + * sysfs attributes for the buffer were mapped as attribute groups on the IIO device
> > + * groups object list.
>
> Been a while, so I can't recall the exact reasons this can cause problems.
> I've +CC Greg and Rafael for comments.
>
> For their reference, the aim of this set is undo an old restriction on IIO that devices
> only had one buffer.  To do that we need to keep a iio:deviceX/buffer0 directory
> also exposed as iio:deviceX/buffer/* and
> iio:deviceX/buffer0/scan_elements/ as iio:deviceX/scan_elements.
> to maintain backwards compatibility.
>
>
> > + * Using kobjects directly for the 'bufferX' and 'scan_elements' directories allows
> > + * us to symlink them back to keep backwards compatibility for the old sysfs interface
> > + * for IIO buffers while also allowing us to support multiple IIO buffers per one
> > + * IIO device.
> > + */
> >  static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
> >                                            struct iio_dev *indio_dev)
> >  {
> > @@ -1282,12 +1398,16 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
> >               memcpy(&attr[ARRAY_SIZE(iio_buffer_attrs)], buffer->attrs,
> >                      sizeof(struct attribute *) * attrcount);
> >
> > -     attr[attrcount + ARRAY_SIZE(iio_buffer_attrs)] = NULL;
> > +     buffer->buffer_attrs = attr;
> >
> > -     buffer->buffer_group.name = "buffer";
> > -     buffer->buffer_group.attrs = attr;
> > +     ret = kobject_init_and_add(&buffer->buffer_dir, &iio_buffer_dir_ktype,
> > +                                &indio_dev->dev.kobj, "buffer");
> > +     if (ret)
> > +             goto error_buffer_free_attrs;
> >
>
> Do we potentially want kobject_uevent calls for these?
> (based on looking at similar code in edac).

Not sure if this question is also for me.
I can take a look.
Hopefully I can make some sense of it.
But I don't know of a way to see if it actually does anything by adding it.
So, suggestions/ideas are welcome.


>
> > -     indio_dev->groups[indio_dev->groupcounter++] = &buffer->buffer_group;
> > +     ret = iio_sysfs_add_attrs(&buffer->buffer_dir, buffer->buffer_attrs);
> > +     if (ret)
> > +             goto error_buffer_kobject_put;
> >
> >       attrcount = 0;
> >       INIT_LIST_HEAD(&buffer->scan_el_dev_attr_list);
> > @@ -1317,32 +1437,57 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
> >               }
> >       }
> >
> > -     buffer->scan_el_group.name = iio_scan_elements_group_name;
> > -
> > -     buffer->scan_el_group.attrs = kcalloc(attrcount + 1,
> > -                                           sizeof(buffer->scan_el_group.attrs[0]),
> > -                                           GFP_KERNEL);
> > -     if (buffer->scan_el_group.attrs == NULL) {
> > +     buffer->scan_el_attrs = kcalloc(attrcount + 1,
> > +                                     sizeof(buffer->scan_el_attrs[0]),
> > +                                     GFP_KERNEL);
> > +     if (buffer->scan_el_attrs == NULL) {
> >               ret = -ENOMEM;
> >               goto error_free_scan_mask;
> >       }
> > -     attrn = 0;
> >
> > +     ret = kobject_init_and_add(&buffer->scan_el_dir, &iio_scan_el_dir_ktype,
> > +                                &indio_dev->dev.kobj, "scan_elements");
> > +     if (ret)
> > +             goto error_free_scan_attrs;
> > +
> > +     attrn = 0;
> >       list_for_each_entry(p, &buffer->scan_el_dev_attr_list, l)
> > -             buffer->scan_el_group.attrs[attrn++] = &p->dev_attr.attr;
> > -     indio_dev->groups[indio_dev->groupcounter++] = &buffer->scan_el_group;
> > +             buffer->scan_el_attrs[attrn++] = &p->dev_attr.attr;
> > +
> > +     ret = iio_sysfs_add_attrs(&buffer->scan_el_dir, buffer->scan_el_attrs);
> > +     if (ret)
> > +             goto error_scan_kobject_put;
> >
> >       return 0;
> >
> > +error_scan_kobject_put:
> > +     kobject_put(&buffer->scan_el_dir);
> > +error_free_scan_attrs:
> > +     kfree(buffer->scan_el_attrs);
> >  error_free_scan_mask:
> >       bitmap_free(buffer->scan_mask);
> >  error_cleanup_dynamic:
> > +     iio_sysfs_del_attrs(&buffer->buffer_dir, buffer->buffer_attrs);
> >       iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list);
> > -     kfree(buffer->buffer_group.attrs);
> > +error_buffer_kobject_put:
> > +     kobject_put(&buffer->buffer_dir);
> > +error_buffer_free_attrs:
> > +     kfree(buffer->buffer_attrs);
> >
> >       return ret;
> >  }
> >
> > +/**
> > + * iio_buffer_alloc_sysfs_and_mask() - Allocate sysfs attributes to attached buffers
> > + * @indio_dev: the iio device for which to create the buffer sysfs attributes
> > + *
> > + * Return 0, or negative for error.
> > + *
> > + * If the IIO device has no buffer attached, no sysfs attributes will be created.
> > + * This function must be called after the IIO device object has been created and
> > + * registered with device_add(). See __iio_buffer_alloc_sysfs_and_mask() for more
> > + * details.
> > + */
> >  int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
> >  {
> >       struct iio_buffer *buffer = indio_dev->buffer;
> > @@ -1364,14 +1509,28 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
> >       return __iio_buffer_alloc_sysfs_and_mask(buffer, indio_dev);
> >  }
> >
> > +/**
> > + * __iio_buffer_free_sysfs_and_mask() - Free sysfs objects for a single IIO buffer
> > + * @buffer: the iio buffer for which to destroy the objects
> > + */
> >  static void __iio_buffer_free_sysfs_and_mask(struct iio_buffer *buffer)
> >  {
> > +     iio_sysfs_del_attrs(&buffer->scan_el_dir, buffer->scan_el_attrs);
> > +     kobject_put(&buffer->scan_el_dir);
> > +     kfree(buffer->scan_el_attrs);
> >       bitmap_free(buffer->scan_mask);
> > -     kfree(buffer->buffer_group.attrs);
> > -     kfree(buffer->scan_el_group.attrs);
> > +     iio_sysfs_del_attrs(&buffer->buffer_dir, buffer->buffer_attrs);
> >       iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list);
> > +     kobject_put(&buffer->buffer_dir);
> > +     kfree(buffer->buffer_attrs);
> >  }
> >
> > +/**
> > + * iio_buffer_free_sysfs_and_mask() - Free sysfs objects for all IIO buffers
> > + * @indio_dev: the iio device for which to destroy the objects
> > + *
> > + * If the IIO device has no buffer attached, nothing will be done.
> > + */
> >  void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)
> >  {
> >       struct iio_buffer *buffer = indio_dev->buffer;
> > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> > index 0a6fd299a978..95d66745f118 100644
> > --- a/drivers/iio/industrialio-core.c
> > +++ b/drivers/iio/industrialio-core.c
> > @@ -1817,18 +1817,11 @@ int __iio_device_register(struct iio_dev *indio_dev, struct module *this_mod)
> >
> >       iio_device_register_debugfs(indio_dev);
> >
> > -     ret = iio_buffer_alloc_sysfs_and_mask(indio_dev);
> > -     if (ret) {
> > -             dev_err(indio_dev->dev.parent,
> > -                     "Failed to create buffer sysfs interfaces\n");
> > -             goto error_unreg_debugfs;
> > -     }
> > -
> >       ret = iio_device_register_sysfs(indio_dev);
> >       if (ret) {
> >               dev_err(indio_dev->dev.parent,
> >                       "Failed to register sysfs interfaces\n");
> > -             goto error_buffer_free_sysfs;
> > +             goto error_unreg_debugfs;
> >       }
> >       ret = iio_device_register_eventset(indio_dev);
> >       if (ret) {
> > @@ -1857,14 +1850,21 @@ int __iio_device_register(struct iio_dev *indio_dev, struct module *this_mod)
> >       if (ret < 0)
> >               goto error_unreg_eventset;
> >
> > +     ret = iio_buffer_alloc_sysfs_and_mask(indio_dev);
> > +     if (ret) {
> > +             dev_err(indio_dev->dev.parent,
> > +                     "Failed to create buffer sysfs interfaces\n");
> > +             goto error_device_del;
> > +     }
> > +
> >       return 0;
> >
> > +error_device_del:
> > +     cdev_device_del(&indio_dev->chrdev, &indio_dev->dev);
> >  error_unreg_eventset:
> >       iio_device_unregister_eventset(indio_dev);
> >  error_free_sysfs:
> >       iio_device_unregister_sysfs(indio_dev);
> > -error_buffer_free_sysfs:
> > -     iio_buffer_free_sysfs_and_mask(indio_dev);
> >  error_unreg_debugfs:
> >       iio_device_unregister_debugfs(indio_dev);
> >       return ret;
> > @@ -1880,6 +1880,8 @@ void iio_device_unregister(struct iio_dev *indio_dev)
> >       struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
> >       struct iio_ioctl_handler *h, *t;
> >
> > +     iio_buffer_free_sysfs_and_mask(indio_dev);
> > +
> >       cdev_device_del(&indio_dev->chrdev, &indio_dev->dev);
> >
> >       mutex_lock(&indio_dev->info_exist_lock);
> > @@ -1897,8 +1899,6 @@ void iio_device_unregister(struct iio_dev *indio_dev)
> >       iio_buffer_wakeup_poll(indio_dev);
> >
> >       mutex_unlock(&indio_dev->info_exist_lock);
> > -
> > -     iio_buffer_free_sysfs_and_mask(indio_dev);
> >  }
> >  EXPORT_SYMBOL(iio_device_unregister);
> >
> > diff --git a/include/linux/iio/buffer_impl.h b/include/linux/iio/buffer_impl.h
> > index 67d73d465e02..77e169e51434 100644
> > --- a/include/linux/iio/buffer_impl.h
> > +++ b/include/linux/iio/buffer_impl.h
> > @@ -103,14 +103,20 @@ struct iio_buffer {
> >       /* @scan_el_dev_attr_list: List of scan element related attributes. */
> >       struct list_head scan_el_dev_attr_list;
> >
> > -     /* @buffer_group: Attributes of the buffer group. */
> > -     struct attribute_group buffer_group;
> > +     /* @buffer_dir: kobject for the 'buffer' directory of this buffer */
> > +     struct kobject buffer_dir;
> > +
> > +     /* @buffer_attrs: Attributes of the buffer group. */
> > +     struct attribute **buffer_attrs;
> > +
> > +     /* @scan_el_dir: kobject for the 'scan_elements' directory of this buffer */
> > +     struct kobject scan_el_dir;
> >
> >       /*
> > -      * @scan_el_group: Attribute group for those attributes not
> > +      * @scan_el_attrs: Array of attributes for those attributes not
> >        * created from the iio_chan_info array.
> >        */
> > -     struct attribute_group scan_el_group;
> > +     struct attribute **scan_el_attrs;
> >
> >       /* @attrs: Standard attributes of the buffer. */
> >       const struct attribute **attrs;
> > diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> > index e4a9822e6495..59b317dc45b8 100644
> > --- a/include/linux/iio/iio.h
> > +++ b/include/linux/iio/iio.h
> > @@ -556,7 +556,7 @@ struct iio_dev {
> >       struct mutex                    info_exist_lock;
> >       const struct iio_buffer_setup_ops       *setup_ops;
> >       struct cdev                     chrdev;
> > -#define IIO_MAX_GROUPS 6
> > +#define IIO_MAX_GROUPS 4
> >       const struct attribute_group    *groups[IIO_MAX_GROUPS + 1];
> >       int                             groupcounter;
> >
>

Re: [PATCH v2 12/12][RESEND] iio: buffer: add ioctl() to support opening extra buffers for IIO device

[Alexandru Ardelean]

On Sun, Jan 24, 2021 at 8:41 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Fri, 22 Jan 2021 18:25:29 +0200
> Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
>
> > With this change, an ioctl() call is added to open a character device for a
> > buffer.
> > The ioctl() will return a 0 FD for the first buffer, as that FD for buffer0
> > is the same FD as the one used for this ioctl().
> >
> > For any other extra buffer, this ioctl() will return an anon inode FD that
> > would access any extra buffer.
> >
> > Right now, there doesn't seem to be (or I couldn't find) a way for this
> > ioctl() to return the FD for buffer0 (i.e. to return the same FD as used
> > for the ioctl()).
> > So, usespace would need to know that  ioctl(fd,
> > IIO_BUFFER_GET_BUFFER_COUNT, 0) will return FD 0.

Just noticed a copy+paste error here.
This should be IIO_BUFFER_GET_FD_IOCTL

> > We could also return another FD for buffer 0, but duplicating FDs for the
> > same IIO buffer sounds problematic.
>
> Agreed. For now this is fine.  Userspace doing that request is a bit
> odd in general even if I can see it might be convenient.

I fully agree that this would make userspace convenient.
It was my initial thought when doing the v1 (or RFC) but I could not
find a sane way to do it.

>
> Hohum. Now in theory ioctl numbers should be added to
> Documentation/userspace-api/ioctl/ioctl-number.rst but oops
> seems I never added the one we already use.
>
> Seems no one is using the block we are though so should be fine.
> If you fancy adding this one and the event one we already have that would be great.

Normally I don't shy away from doing requests, but I may defer this one.
Will see if I can squeeze in this one though.

> Perhaps we are allowing a bit too much space though given the
> event one is down at 'i', 0x90 and this is 'i', 0xB0

So, the only reason for jumping to 0xb0 is because internally ADI, is
using 0xa0 (and a few) for some non-upstream buffer ioctls:
See
https://github.com/analogdevicesinc/linux/blob/master/include/linux/iio/buffer_impl.h#L12
and
https://github.com/analogdevicesinc/libiio/blob/master/local.c#L51

So, libiio is dependent on these non-mainline ioctls.
I am fine to change these addresses to anything else that doesn't
overlap with 0xa0.
GNU Radio is using those ioctls (0xa0) for some AD9361 stuff.
As well as other ADI customers using other high-speed
ADC/DAC/transceiver boards (usually from ADI, because they're using
the ADI kernel).
Well, bringing up GNU Radio makes for a stronger case :)

>
> We'll probably hit something undocumented anyway ;)
>
> So from a fairly light level of read through this seems to have come together
> nicely. I'm a bit nervous about the sysfs kobject stuff, but it seems there
> are other places where similar stuff happens so if Rafael and Greg don't come
> back with major comments that should be fine.

[1] My preference would have been to have a "/dev/iio/deviceX/bufferY"
similar to the input subsystem.
I think I am with Lars (on this; well, I hope I am not wrong) that
using dd/cat/echo directly on the buffer chardev would be interesting.

I am also fine with this.
And from a backwards compatibility perspective this is pretty good (a
bit better than [1]).
I agree that the sysfs kobject stuff is tricky; and also the stuff
about getting the FD for buffer 0 via this new ioctl() is also quirky.
But I heard an interesting quote from a colleague (it may be his or
maybe he also heard it from somewhere else):
"Darn compromises... but that's engineering. If you don't want to make
compromises in your career - theology or pure mathematics are better
choices :)"  - Mark Thoren

[i enjoyed that comment when i first heard it :) ]

>
> Jonathan
>
> >
> > Also, there is no IIO_BUFFER_GET_BUFFER_COUNT ioctl() implemented, as the
> > index for each buffer (and the count) can be deduced from the
> > '/sys/bus/iio/devices/iio:deviceX/bufferY' folders (i.e the number of
> > bufferY folders).
> >
> > Used following C code to test this:
> > -------------------------------------------------------------------
> >
> >  #include <stdio.h>
> >  #include <stdlib.h>
> >  #include <unistd.h>
> >  #include <sys/ioctl.h>
> >  #include <fcntl.h"
> >  #include <errno.h>
> >
> >  #define IIO_BUFFER_GET_FD_IOCTL      _IOWR('i', 0xb0, int)
> >
> > int main(int argc, char *argv[])
> > {
> >         int fd;
> >         int fd1;
> >         int ret;
> >
> >         if ((fd = open("/dev/iio:device0", O_RDWR))<0) {
> >                 fprintf(stderr, "Error open() %d errno %d\n",fd, errno);
> >                 return -1;
> >         }
> >
> >         fprintf(stderr, "Using FD %d\n", fd);
> >
> >         fd1 = atoi(argv[1]);
> >
> >         ret = ioctl(fd, IIO_BUFFER_GET_FD_IOCTL, &fd1);
> >         if (ret < 0) {
> >                 fprintf(stderr, "Error for buffer %d ioctl() %d errno %d\n", fd1, ret, errno);
> >                 close(fd);
> >                 return -1;
> >         }
> >
> >         fprintf(stderr, "Got FD %d\n", fd1);
> >
> >         close(fd1);
> >         close(fd);
> >
> >         return 0;
> > }
> > -------------------------------------------------------------------
> >
> > Results are:
> > -------------------------------------------------------------------
> >  # ./test 0
> >  Using FD 3
> >  Got FD 0
> >
> >  # ./test 1
> >  Using FD 3
> >  Got FD 4
> >
> >  # ./test 2
> >  Using FD 3
> >  Got FD 4
> >
> >  # ./test 3
> >  Using FD 3
> >  Got FD 4
> >
> >  # ls /sys/bus/iio/devices/iio\:device0
> >  buffer  buffer0  buffer1  buffer2  buffer3  dev
> >  in_voltage_sampling_frequency  in_voltage_scale
> >  in_voltage_scale_available
> >  name  of_node  power  scan_elements  subsystem  uevent
> > -------------------------------------------------------------------
> >
> > iio:device0 has some fake kfifo buffers attached to an IIO device.
> >
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> > ---
> >  drivers/iio/industrialio-buffer.c | 119 ++++++++++++++++++++++++++++++
> >  drivers/iio/industrialio-core.c   |   8 ++
> >  include/linux/iio/buffer_impl.h   |   5 ++
> >  include/linux/iio/iio-opaque.h    |   2 +
> >  include/uapi/linux/iio/buffer.h   |  10 +++
> >  5 files changed, 144 insertions(+)
> >  create mode 100644 include/uapi/linux/iio/buffer.h
> >
> > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> > index 2f429616e998..e619ec35c431 100644
> > --- a/drivers/iio/industrialio-buffer.c
> > +++ b/drivers/iio/industrialio-buffer.c
> > @@ -9,6 +9,7 @@
> >   * - Better memory allocation techniques?
> >   * - Alternative access techniques?
> >   */
> > +#include <linux/anon_inodes.h>
> >  #include <linux/kernel.h>
> >  #include <linux/export.h>
> >  #include <linux/device.h>
> > @@ -1399,6 +1400,106 @@ static void iio_sysfs_del_attrs(struct kobject *kobj, struct attribute **ptr)
> >               sysfs_remove_file(kobj, ptr[i]);
> >  }
> >
> > +static int iio_buffer_chrdev_release(struct inode *inode, struct file *filep)
> > +{
> > +     struct iio_dev_buffer_pair *ib = filep->private_data;
> > +     struct iio_dev *indio_dev = ib->indio_dev;
> > +     struct iio_buffer *buffer = ib->buffer;
> > +
> > +     clear_bit(IIO_BUSY_BIT_POS, &buffer->flags);
> > +     iio_device_put(indio_dev);
> > +     kfree(ib);
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct file_operations iio_buffer_chrdev_fileops = {
> > +     .owner = THIS_MODULE,
> > +     .llseek = noop_llseek,
> > +     .read = iio_buffer_read_outer_addr,
> > +     .poll = iio_buffer_poll_addr,
> > +     .compat_ioctl = compat_ptr_ioctl,
> > +     .release = iio_buffer_chrdev_release,
> > +};
> > +
> > +static long iio_device_buffer_getfd(struct iio_dev *indio_dev, unsigned long arg)
> > +{
> > +     struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
> > +     int __user *ival = (int __user *)arg;
> > +     char buf_name[sizeof("iio:buffer:xxx")];
> > +     struct iio_dev_buffer_pair *ib;
> > +     struct iio_buffer *buffer;
> > +     int fd, idx;
> > +
> > +     if (copy_from_user(&idx, ival, sizeof(idx)))
> > +             return -EFAULT;
> > +
> > +     if (idx == 0) {
> > +             fd = 0;
> > +             if (copy_to_user(ival, &fd, sizeof(fd)))
> > +                     return -EFAULT;
> > +             return 0;
> > +     }
> > +
> > +     if (idx >= iio_dev_opaque->attached_buffers_cnt)
> > +             return -ENOENT;
> > +
> > +     fd = mutex_lock_interruptible(&indio_dev->mlock);
> > +     if (fd)
> > +             return fd;
> > +
> > +     buffer = iio_dev_opaque->attached_buffers[idx];
> > +
> > +     if (test_and_set_bit(IIO_BUSY_BIT_POS, &buffer->flags)) {
> > +             fd = -EBUSY;
> > +             goto error_unlock;
> > +     }
> > +
> > +     iio_device_get(indio_dev);
> > +
> > +     ib = kzalloc(sizeof(*ib), GFP_KERNEL);
> > +     if (!ib) {
> > +             fd = -ENOMEM;
> > +             goto error_iio_dev_put;
> > +     }
> > +
> > +     ib->indio_dev = indio_dev;
> > +     ib->buffer = buffer;
> > +
> > +     fd = anon_inode_getfd(buf_name, &iio_buffer_chrdev_fileops,
> > +                           ib, O_RDWR | O_CLOEXEC);
> > +     if (fd < 0)
> > +             goto error_free_ib;
> > +
> > +     if (copy_to_user(ival, &fd, sizeof(fd))) {
> > +             fd = -EFAULT;
> > +             goto error_free_ib;
> > +     }
> > +
> > +     mutex_unlock(&indio_dev->mlock);
> > +     return fd;
> > +
> > +error_free_ib:
> > +     kfree(ib);
> > +error_iio_dev_put:
> > +     iio_device_put(indio_dev);
> > +     clear_bit(IIO_BUSY_BIT_POS, &buffer->flags);
> > +error_unlock:
> > +     mutex_unlock(&indio_dev->mlock);
> > +     return fd;
> > +}
> > +
> > +static long iio_device_buffer_ioctl(struct iio_dev *indio_dev, struct file *filp,
> > +                                 unsigned int cmd, unsigned long arg)
> > +{
> > +     switch (cmd) {
> > +     case IIO_BUFFER_GET_FD_IOCTL:
> > +             return iio_device_buffer_getfd(indio_dev, arg);
> > +     default:
> > +             return IIO_IOCTL_UNHANDLED;
> > +     }
> > +}
> > +
> >  /**
> >   * __iio_buffer_alloc_sysfs_and_mask() - Allocate sysfs attributes to an attached buffer
> >   * @buffer: the buffer object for which the sysfs attributes are created for
> > @@ -1565,6 +1666,7 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
> >       struct iio_buffer *buffer = indio_dev->buffer;
> >       const struct iio_chan_spec *channels;
> >       int i, cnt, ret;
> > +     size_t sz;
> >
> >       channels = indio_dev->channels;
> >       if (channels) {
> > @@ -1597,8 +1699,21 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
> >       if (ret)
> >               goto error_remove_buffer_dir_link;
> >
> > +     sz = sizeof(*(iio_dev_opaque->buffer_ioctl_handler));
> > +     iio_dev_opaque->buffer_ioctl_handler = kzalloc(sz, GFP_KERNEL);
> > +     if (!iio_dev_opaque->buffer_ioctl_handler) {
> > +             ret = -ENOMEM;
> > +             goto error_remove_scan_el_dir;
> > +     }
> > +
> > +     iio_dev_opaque->buffer_ioctl_handler->ioctl = iio_device_buffer_ioctl;
> > +     iio_device_ioctl_handler_register(indio_dev,
> > +                                       iio_dev_opaque->buffer_ioctl_handler);
> > +
> >       return 0;
> >
> > +error_remove_scan_el_dir:
> > +     sysfs_remove_link(&indio_dev->dev.kobj, "scan_elements");
> >  error_remove_buffer_dir_link:
> >       sysfs_remove_link(&indio_dev->dev.kobj, "buffer");
> >       cnt = iio_dev_opaque->attached_buffers_cnt - 1;
> > @@ -1627,6 +1742,10 @@ void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)
> >       if (!buffer)
> >               return;
> >
> > +     iio_device_ioctl_handler_unregister(iio_dev_opaque->buffer_ioctl_handler);
> > +     kfree(iio_dev_opaque->buffer_ioctl_handler);
> > +     iio_dev_opaque->buffer_ioctl_handler = NULL;
> > +
> >       sysfs_remove_link(&indio_dev->dev.kobj, "scan_elements");
> >       sysfs_remove_link(&indio_dev->dev.kobj, "buffer");
> >
> > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> > index 0d1880837776..898992fd33c3 100644
> > --- a/drivers/iio/industrialio-core.c
> > +++ b/drivers/iio/industrialio-core.c
> > @@ -1683,6 +1683,9 @@ static int iio_chrdev_open(struct inode *inode, struct file *filp)
> >       ib->indio_dev = indio_dev;
> >       ib->buffer = indio_dev->buffer;
> >
> > +     if (indio_dev->buffer)
> > +             test_and_set_bit(IIO_BUSY_BIT_POS, &indio_dev->buffer->flags);
> > +
> >       filp->private_data = ib;
> >
> >       return 0;
> > @@ -1700,6 +1703,11 @@ static int iio_chrdev_release(struct inode *inode, struct file *filp)
> >       struct iio_dev_buffer_pair *ib = filp->private_data;
> >       struct iio_dev *indio_dev = container_of(inode->i_cdev,
> >                                               struct iio_dev, chrdev);
> > +     struct iio_buffer *buffer = ib->buffer;
> > +
> > +     if (buffer)
> > +             clear_bit(IIO_BUSY_BIT_POS, &buffer->flags);
> > +
> >       clear_bit(IIO_BUSY_BIT_POS, &indio_dev->flags);
> >       iio_device_put(indio_dev);
> >       kfree(ib);
> > diff --git a/include/linux/iio/buffer_impl.h b/include/linux/iio/buffer_impl.h
> > index e25d26a7f601..78da590b5607 100644
> > --- a/include/linux/iio/buffer_impl.h
> > +++ b/include/linux/iio/buffer_impl.h
> > @@ -6,6 +6,8 @@
> >
> >  #ifdef CONFIG_IIO_BUFFER
> >
> > +#include <uapi/linux/iio/buffer.h>
> > +
> >  struct iio_dev;
> >  struct iio_buffer;
> >
> > @@ -75,6 +77,9 @@ struct iio_buffer {
> >       /** @length: Number of datums in buffer. */
> >       unsigned int length;
> >
> > +     /** @flags: File ops flags including busy flag. */
> > +     unsigned long flags;
> > +
> >       /**  @bytes_per_datum: Size of individual datum including timestamp. */
> >       size_t bytes_per_datum;
> >
> > diff --git a/include/linux/iio/iio-opaque.h b/include/linux/iio/iio-opaque.h
> > index 1db0ea09520e..d0429b13afa8 100644
> > --- a/include/linux/iio/iio-opaque.h
> > +++ b/include/linux/iio/iio-opaque.h
> > @@ -9,6 +9,7 @@
> >   * @event_interface:         event chrdevs associated with interrupt lines
> >   * @attached_buffers:                array of buffers statically attached by the driver
> >   * @attached_buffers_cnt:    number of buffers in the array of statically attached buffers
> > + * @buffer_ioctl_handler:    ioctl() handler for this IIO device's buffer interface
> >   * @buffer_list:             list of all buffers currently attached
> >   * @channel_attr_list:               keep track of automatically created channel
> >   *                           attributes
> > @@ -24,6 +25,7 @@ struct iio_dev_opaque {
> >       struct iio_event_interface      *event_interface;
> >       struct iio_buffer               **attached_buffers;
> >       unsigned int                    attached_buffers_cnt;
> > +     struct iio_ioctl_handler        *buffer_ioctl_handler;
> >       struct list_head                buffer_list;
> >       struct list_head                channel_attr_list;
> >       struct attribute_group          chan_attr_group;
> > diff --git a/include/uapi/linux/iio/buffer.h b/include/uapi/linux/iio/buffer.h
> > new file mode 100644
> > index 000000000000..5455f7ff143d
> > --- /dev/null
> > +++ b/include/uapi/linux/iio/buffer.h
> > @@ -0,0 +1,10 @@
> > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> > +/* industrial I/O buffer definitions needed both in and out of kernel
> > + */
> > +
> > +#ifndef _UAPI_IIO_BUFFER_H_
> > +#define _UAPI_IIO_BUFFER_H_
> > +
> > +#define IIO_BUFFER_GET_FD_IOCTL              _IOWR('i', 0xb0, int)
> > +
> > +#endif /* _UAPI_IIO_BUFFER_H_ */
>

[kbuild] Re: [PATCH v2 11/12][RESEND] iio: buffer: introduce support for attaching more IIO buffers

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 5186 bytes --]

Hi Alexandru,

I love your patch! Perhaps something to improve:

url:    https://github.com/0day-ci/linux/commits/Alexandru-Ardelean/iio-core-buffer-add-support-for-multiple-IIO-buffers-per-IIO-device/20210125-053419
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git  togreg
config: i386-randconfig-m021-20210125 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-20) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

smatch warnings:
drivers/iio/industrialio-buffer.c:1606 iio_buffer_alloc_sysfs_and_mask() error: uninitialized symbol 'cnt'.

vim +/cnt +1606 drivers/iio/industrialio-buffer.c

e16e0a778fec8ac1 Alexandru Ardelean 2020-09-17  1562  int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
d967cb6bd4e79c0c Lars-Peter Clausen 2014-11-26  1563  {
8b70c93a9d04de1e Alexandru Ardelean 2021-01-22  1564  	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
ff3f7e049aef9202 Alexandru Ardelean 2020-04-24  1565  	struct iio_buffer *buffer = indio_dev->buffer;
e16e0a778fec8ac1 Alexandru Ardelean 2020-09-17  1566  	const struct iio_chan_spec *channels;
8b70c93a9d04de1e Alexandru Ardelean 2021-01-22  1567  	int i, cnt, ret;
e16e0a778fec8ac1 Alexandru Ardelean 2020-09-17  1568  
e16e0a778fec8ac1 Alexandru Ardelean 2020-09-17  1569  	channels = indio_dev->channels;
e16e0a778fec8ac1 Alexandru Ardelean 2020-09-17  1570  	if (channels) {
e16e0a778fec8ac1 Alexandru Ardelean 2020-09-17  1571  		int ml = indio_dev->masklength;
e16e0a778fec8ac1 Alexandru Ardelean 2020-09-17  1572  
e16e0a778fec8ac1 Alexandru Ardelean 2020-09-17  1573  		for (i = 0; i < indio_dev->num_channels; i++)
e16e0a778fec8ac1 Alexandru Ardelean 2020-09-17  1574  			ml = max(ml, channels[i].scan_index + 1);
e16e0a778fec8ac1 Alexandru Ardelean 2020-09-17  1575  		indio_dev->masklength = ml;
e16e0a778fec8ac1 Alexandru Ardelean 2020-09-17  1576  	}
ff3f7e049aef9202 Alexandru Ardelean 2020-04-24  1577  
ff3f7e049aef9202 Alexandru Ardelean 2020-04-24  1578  	if (!buffer)
e16e0a778fec8ac1 Alexandru Ardelean 2020-09-17  1579  		return 0;
e16e0a778fec8ac1 Alexandru Ardelean 2020-09-17  1580  
8b70c93a9d04de1e Alexandru Ardelean 2021-01-22  1581  	for (i = 0; i < iio_dev_opaque->attached_buffers_cnt; i++) {
8b70c93a9d04de1e Alexandru Ardelean 2021-01-22  1582  		buffer = iio_dev_opaque->attached_buffers[i];
8b70c93a9d04de1e Alexandru Ardelean 2021-01-22  1583  		ret = __iio_buffer_alloc_sysfs_and_mask(buffer, indio_dev, i);
e2eb176de4748db8 Alexandru Ardelean 2021-01-22  1584  		if (ret)
8b70c93a9d04de1e Alexandru Ardelean 2021-01-22  1585  			goto error_unwind_sysfs_and_mask;

"cnt" is not set here.

8b70c93a9d04de1e Alexandru Ardelean 2021-01-22  1586  	}
d967cb6bd4e79c0c Lars-Peter Clausen 2014-11-26  1587  
e2eb176de4748db8 Alexandru Ardelean 2021-01-22  1588  	ret = sysfs_create_link(&indio_dev->dev.kobj,
e2eb176de4748db8 Alexandru Ardelean 2021-01-22  1589  				&indio_dev->buffer->buffer_dir,
e2eb176de4748db8 Alexandru Ardelean 2021-01-22  1590  				"buffer");
e2eb176de4748db8 Alexandru Ardelean 2021-01-22  1591  	if (ret)
8b70c93a9d04de1e Alexandru Ardelean 2021-01-22  1592  		goto error_unwind_sysfs_and_mask;

Oh here.

e2eb176de4748db8 Alexandru Ardelean 2021-01-22  1593  
e2eb176de4748db8 Alexandru Ardelean 2021-01-22  1594  	ret = sysfs_create_link(&indio_dev->dev.kobj,
e2eb176de4748db8 Alexandru Ardelean 2021-01-22  1595  				&indio_dev->buffer->scan_el_dir,
e2eb176de4748db8 Alexandru Ardelean 2021-01-22  1596  				"scan_elements");
e2eb176de4748db8 Alexandru Ardelean 2021-01-22  1597  	if (ret)
e2eb176de4748db8 Alexandru Ardelean 2021-01-22  1598  		goto error_remove_buffer_dir_link;
e2eb176de4748db8 Alexandru Ardelean 2021-01-22  1599  
e2eb176de4748db8 Alexandru Ardelean 2021-01-22  1600  	return 0;
e2eb176de4748db8 Alexandru Ardelean 2021-01-22  1601  
e2eb176de4748db8 Alexandru Ardelean 2021-01-22  1602  error_remove_buffer_dir_link:
e2eb176de4748db8 Alexandru Ardelean 2021-01-22  1603  	sysfs_remove_link(&indio_dev->dev.kobj, "buffer");
8b70c93a9d04de1e Alexandru Ardelean 2021-01-22  1604  	cnt = iio_dev_opaque->attached_buffers_cnt - 1;
8b70c93a9d04de1e Alexandru Ardelean 2021-01-22  1605  error_unwind_sysfs_and_mask:
8b70c93a9d04de1e Alexandru Ardelean 2021-01-22 @1606  	for (; cnt >= 0; cnt--) {
                                                               ^^^^^^^^
Uninitialized.

8b70c93a9d04de1e Alexandru Ardelean 2021-01-22  1607  		buffer = iio_dev_opaque->attached_buffers[cnt];
e2eb176de4748db8 Alexandru Ardelean 2021-01-22  1608  		__iio_buffer_free_sysfs_and_mask(buffer);
8b70c93a9d04de1e Alexandru Ardelean 2021-01-22  1609  	}
8b70c93a9d04de1e Alexandru Ardelean 2021-01-22  1610  	kfree(iio_dev_opaque->attached_buffers);
8b70c93a9d04de1e Alexandru Ardelean 2021-01-22  1611  	iio_dev_opaque->attached_buffers = NULL;
e2eb176de4748db8 Alexandru Ardelean 2021-01-22  1612  	return ret;
d967cb6bd4e79c0c Lars-Peter Clausen 2014-11-26  1613  }

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org 

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 30447 bytes --]

[-- Attachment #3: Type: text/plain, Size: 149 bytes --]

_______________________________________________
kbuild mailing list -- kbuild@lists.01.org
To unsubscribe send an email to kbuild-leave@lists.01.org

Re: [PATCH v2 11/12][RESEND] iio: buffer: introduce support for attaching more IIO buffers

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 5429 bytes --]

Hi Alexandru,

I love your patch! Perhaps something to improve:

url:    https://github.com/0day-ci/linux/commits/Alexandru-Ardelean/iio-core-buffer-add-support-for-multiple-IIO-buffers-per-IIO-device/20210125-053419
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git  togreg
config: i386-randconfig-m021-20210125 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-20) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

smatch warnings:
drivers/iio/industrialio-buffer.c:1606 iio_buffer_alloc_sysfs_and_mask() error: uninitialized symbol 'cnt'.

vim +/cnt +1606 drivers/iio/industrialio-buffer.c

e16e0a778fec8ac1 Alexandru Ardelean 2020-09-17  1562  int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
d967cb6bd4e79c0c Lars-Peter Clausen 2014-11-26  1563  {
8b70c93a9d04de1e Alexandru Ardelean 2021-01-22  1564  	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
ff3f7e049aef9202 Alexandru Ardelean 2020-04-24  1565  	struct iio_buffer *buffer = indio_dev->buffer;
e16e0a778fec8ac1 Alexandru Ardelean 2020-09-17  1566  	const struct iio_chan_spec *channels;
8b70c93a9d04de1e Alexandru Ardelean 2021-01-22  1567  	int i, cnt, ret;
e16e0a778fec8ac1 Alexandru Ardelean 2020-09-17  1568  
e16e0a778fec8ac1 Alexandru Ardelean 2020-09-17  1569  	channels = indio_dev->channels;
e16e0a778fec8ac1 Alexandru Ardelean 2020-09-17  1570  	if (channels) {
e16e0a778fec8ac1 Alexandru Ardelean 2020-09-17  1571  		int ml = indio_dev->masklength;
e16e0a778fec8ac1 Alexandru Ardelean 2020-09-17  1572  
e16e0a778fec8ac1 Alexandru Ardelean 2020-09-17  1573  		for (i = 0; i < indio_dev->num_channels; i++)
e16e0a778fec8ac1 Alexandru Ardelean 2020-09-17  1574  			ml = max(ml, channels[i].scan_index + 1);
e16e0a778fec8ac1 Alexandru Ardelean 2020-09-17  1575  		indio_dev->masklength = ml;
e16e0a778fec8ac1 Alexandru Ardelean 2020-09-17  1576  	}
ff3f7e049aef9202 Alexandru Ardelean 2020-04-24  1577  
ff3f7e049aef9202 Alexandru Ardelean 2020-04-24  1578  	if (!buffer)
e16e0a778fec8ac1 Alexandru Ardelean 2020-09-17  1579  		return 0;
e16e0a778fec8ac1 Alexandru Ardelean 2020-09-17  1580  
8b70c93a9d04de1e Alexandru Ardelean 2021-01-22  1581  	for (i = 0; i < iio_dev_opaque->attached_buffers_cnt; i++) {
8b70c93a9d04de1e Alexandru Ardelean 2021-01-22  1582  		buffer = iio_dev_opaque->attached_buffers[i];
8b70c93a9d04de1e Alexandru Ardelean 2021-01-22  1583  		ret = __iio_buffer_alloc_sysfs_and_mask(buffer, indio_dev, i);
e2eb176de4748db8 Alexandru Ardelean 2021-01-22  1584  		if (ret)
8b70c93a9d04de1e Alexandru Ardelean 2021-01-22  1585  			goto error_unwind_sysfs_and_mask;

"cnt" is not set here.

8b70c93a9d04de1e Alexandru Ardelean 2021-01-22  1586  	}
d967cb6bd4e79c0c Lars-Peter Clausen 2014-11-26  1587  
e2eb176de4748db8 Alexandru Ardelean 2021-01-22  1588  	ret = sysfs_create_link(&indio_dev->dev.kobj,
e2eb176de4748db8 Alexandru Ardelean 2021-01-22  1589  				&indio_dev->buffer->buffer_dir,
e2eb176de4748db8 Alexandru Ardelean 2021-01-22  1590  				"buffer");
e2eb176de4748db8 Alexandru Ardelean 2021-01-22  1591  	if (ret)
8b70c93a9d04de1e Alexandru Ardelean 2021-01-22  1592  		goto error_unwind_sysfs_and_mask;

Oh here.

e2eb176de4748db8 Alexandru Ardelean 2021-01-22  1593  
e2eb176de4748db8 Alexandru Ardelean 2021-01-22  1594  	ret = sysfs_create_link(&indio_dev->dev.kobj,
e2eb176de4748db8 Alexandru Ardelean 2021-01-22  1595  				&indio_dev->buffer->scan_el_dir,
e2eb176de4748db8 Alexandru Ardelean 2021-01-22  1596  				"scan_elements");
e2eb176de4748db8 Alexandru Ardelean 2021-01-22  1597  	if (ret)
e2eb176de4748db8 Alexandru Ardelean 2021-01-22  1598  		goto error_remove_buffer_dir_link;
e2eb176de4748db8 Alexandru Ardelean 2021-01-22  1599  
e2eb176de4748db8 Alexandru Ardelean 2021-01-22  1600  	return 0;
e2eb176de4748db8 Alexandru Ardelean 2021-01-22  1601  
e2eb176de4748db8 Alexandru Ardelean 2021-01-22  1602  error_remove_buffer_dir_link:
e2eb176de4748db8 Alexandru Ardelean 2021-01-22  1603  	sysfs_remove_link(&indio_dev->dev.kobj, "buffer");
8b70c93a9d04de1e Alexandru Ardelean 2021-01-22  1604  	cnt = iio_dev_opaque->attached_buffers_cnt - 1;
8b70c93a9d04de1e Alexandru Ardelean 2021-01-22  1605  error_unwind_sysfs_and_mask:
8b70c93a9d04de1e Alexandru Ardelean 2021-01-22 @1606  	for (; cnt >= 0; cnt--) {
                                                               ^^^^^^^^
Uninitialized.

8b70c93a9d04de1e Alexandru Ardelean 2021-01-22  1607  		buffer = iio_dev_opaque->attached_buffers[cnt];
e2eb176de4748db8 Alexandru Ardelean 2021-01-22  1608  		__iio_buffer_free_sysfs_and_mask(buffer);
8b70c93a9d04de1e Alexandru Ardelean 2021-01-22  1609  	}
8b70c93a9d04de1e Alexandru Ardelean 2021-01-22  1610  	kfree(iio_dev_opaque->attached_buffers);
8b70c93a9d04de1e Alexandru Ardelean 2021-01-22  1611  	iio_dev_opaque->attached_buffers = NULL;
e2eb176de4748db8 Alexandru Ardelean 2021-01-22  1612  	return ret;
d967cb6bd4e79c0c Lars-Peter Clausen 2014-11-26  1613  }

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org 

_______________________________________________
kbuild mailing list -- kbuild(a)lists.01.org
To unsubscribe send an email to kbuild-leave(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 30447 bytes --]

[kbuild] Re: [PATCH v2 11/12][RESEND] iio: buffer: introduce support for attaching more IIO buffers

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 5429 bytes --]

Hi Alexandru,

I love your patch! Perhaps something to improve:

url:    https://github.com/0day-ci/linux/commits/Alexandru-Ardelean/iio-core-buffer-add-support-for-multiple-IIO-buffers-per-IIO-device/20210125-053419
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git  togreg
config: i386-randconfig-m021-20210125 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-20) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

smatch warnings:
drivers/iio/industrialio-buffer.c:1606 iio_buffer_alloc_sysfs_and_mask() error: uninitialized symbol 'cnt'.

vim +/cnt +1606 drivers/iio/industrialio-buffer.c

e16e0a778fec8ac1 Alexandru Ardelean 2020-09-17  1562  int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
d967cb6bd4e79c0c Lars-Peter Clausen 2014-11-26  1563  {
8b70c93a9d04de1e Alexandru Ardelean 2021-01-22  1564  	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
ff3f7e049aef9202 Alexandru Ardelean 2020-04-24  1565  	struct iio_buffer *buffer = indio_dev->buffer;
e16e0a778fec8ac1 Alexandru Ardelean 2020-09-17  1566  	const struct iio_chan_spec *channels;
8b70c93a9d04de1e Alexandru Ardelean 2021-01-22  1567  	int i, cnt, ret;
e16e0a778fec8ac1 Alexandru Ardelean 2020-09-17  1568  
e16e0a778fec8ac1 Alexandru Ardelean 2020-09-17  1569  	channels = indio_dev->channels;
e16e0a778fec8ac1 Alexandru Ardelean 2020-09-17  1570  	if (channels) {
e16e0a778fec8ac1 Alexandru Ardelean 2020-09-17  1571  		int ml = indio_dev->masklength;
e16e0a778fec8ac1 Alexandru Ardelean 2020-09-17  1572  
e16e0a778fec8ac1 Alexandru Ardelean 2020-09-17  1573  		for (i = 0; i < indio_dev->num_channels; i++)
e16e0a778fec8ac1 Alexandru Ardelean 2020-09-17  1574  			ml = max(ml, channels[i].scan_index + 1);
e16e0a778fec8ac1 Alexandru Ardelean 2020-09-17  1575  		indio_dev->masklength = ml;
e16e0a778fec8ac1 Alexandru Ardelean 2020-09-17  1576  	}
ff3f7e049aef9202 Alexandru Ardelean 2020-04-24  1577  
ff3f7e049aef9202 Alexandru Ardelean 2020-04-24  1578  	if (!buffer)
e16e0a778fec8ac1 Alexandru Ardelean 2020-09-17  1579  		return 0;
e16e0a778fec8ac1 Alexandru Ardelean 2020-09-17  1580  
8b70c93a9d04de1e Alexandru Ardelean 2021-01-22  1581  	for (i = 0; i < iio_dev_opaque->attached_buffers_cnt; i++) {
8b70c93a9d04de1e Alexandru Ardelean 2021-01-22  1582  		buffer = iio_dev_opaque->attached_buffers[i];
8b70c93a9d04de1e Alexandru Ardelean 2021-01-22  1583  		ret = __iio_buffer_alloc_sysfs_and_mask(buffer, indio_dev, i);
e2eb176de4748db8 Alexandru Ardelean 2021-01-22  1584  		if (ret)
8b70c93a9d04de1e Alexandru Ardelean 2021-01-22  1585  			goto error_unwind_sysfs_and_mask;

"cnt" is not set here.

8b70c93a9d04de1e Alexandru Ardelean 2021-01-22  1586  	}
d967cb6bd4e79c0c Lars-Peter Clausen 2014-11-26  1587  
e2eb176de4748db8 Alexandru Ardelean 2021-01-22  1588  	ret = sysfs_create_link(&indio_dev->dev.kobj,
e2eb176de4748db8 Alexandru Ardelean 2021-01-22  1589  				&indio_dev->buffer->buffer_dir,
e2eb176de4748db8 Alexandru Ardelean 2021-01-22  1590  				"buffer");
e2eb176de4748db8 Alexandru Ardelean 2021-01-22  1591  	if (ret)
8b70c93a9d04de1e Alexandru Ardelean 2021-01-22  1592  		goto error_unwind_sysfs_and_mask;

Oh here.

e2eb176de4748db8 Alexandru Ardelean 2021-01-22  1593  
e2eb176de4748db8 Alexandru Ardelean 2021-01-22  1594  	ret = sysfs_create_link(&indio_dev->dev.kobj,
e2eb176de4748db8 Alexandru Ardelean 2021-01-22  1595  				&indio_dev->buffer->scan_el_dir,
e2eb176de4748db8 Alexandru Ardelean 2021-01-22  1596  				"scan_elements");
e2eb176de4748db8 Alexandru Ardelean 2021-01-22  1597  	if (ret)
e2eb176de4748db8 Alexandru Ardelean 2021-01-22  1598  		goto error_remove_buffer_dir_link;
e2eb176de4748db8 Alexandru Ardelean 2021-01-22  1599  
e2eb176de4748db8 Alexandru Ardelean 2021-01-22  1600  	return 0;
e2eb176de4748db8 Alexandru Ardelean 2021-01-22  1601  
e2eb176de4748db8 Alexandru Ardelean 2021-01-22  1602  error_remove_buffer_dir_link:
e2eb176de4748db8 Alexandru Ardelean 2021-01-22  1603  	sysfs_remove_link(&indio_dev->dev.kobj, "buffer");
8b70c93a9d04de1e Alexandru Ardelean 2021-01-22  1604  	cnt = iio_dev_opaque->attached_buffers_cnt - 1;
8b70c93a9d04de1e Alexandru Ardelean 2021-01-22  1605  error_unwind_sysfs_and_mask:
8b70c93a9d04de1e Alexandru Ardelean 2021-01-22 @1606  	for (; cnt >= 0; cnt--) {
                                                               ^^^^^^^^
Uninitialized.

8b70c93a9d04de1e Alexandru Ardelean 2021-01-22  1607  		buffer = iio_dev_opaque->attached_buffers[cnt];
e2eb176de4748db8 Alexandru Ardelean 2021-01-22  1608  		__iio_buffer_free_sysfs_and_mask(buffer);
8b70c93a9d04de1e Alexandru Ardelean 2021-01-22  1609  	}
8b70c93a9d04de1e Alexandru Ardelean 2021-01-22  1610  	kfree(iio_dev_opaque->attached_buffers);
8b70c93a9d04de1e Alexandru Ardelean 2021-01-22  1611  	iio_dev_opaque->attached_buffers = NULL;
e2eb176de4748db8 Alexandru Ardelean 2021-01-22  1612  	return ret;
d967cb6bd4e79c0c Lars-Peter Clausen 2014-11-26  1613  }

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org 

_______________________________________________
kbuild mailing list -- kbuild(a)lists.01.org
To unsubscribe send an email to kbuild-leave(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 30447 bytes --]

Re: [PATCH v2 03/12][RESEND] iio: buffer: rework buffer & scan_elements dir creation

[Greg Kroah-Hartman]

On Sun, Jan 24, 2021 at 06:11:26PM +0000, Jonathan Cameron wrote:
> On Fri, 22 Jan 2021 18:25:20 +0200
> Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
> 
> > When adding more than one IIO buffer per IIO device, we will need to create
> > a buffer & scan_elements directory for each buffer.
> > We also want to move the 'scan_elements' to be a sub-directory of the
> > 'buffer' folder.
> > 
> > The format we want to reach is, for a iio:device0 folder, for 2 buffers
> > [for example], we have a 'buffer0' and a 'buffer1' subfolder, and each with
> > it's own 'scan_elements' subfolder.
> > 
> > So, for example:
> >    iio:device0/buffer0
> >       scan_elements/
> > 
> >    iio:device0/buffer1
> >       scan_elements/
> > 
> > The other attributes under 'bufferX' would remain unchanged.
> > 
> > However, we would also need to symlink back to the old 'buffer' &
> > 'scan_elements' folders, to keep backwards compatibility.
> > 
> > Doing all these, require that we maintain the kobjects for each 'bufferX'
> > and 'scan_elements' so that we can symlink them back. We also need to
> > implement the sysfs_ops for these folders.
> > 
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> 
> +CC GregKH and Rafael W for feedback on various things inline.
> 
> It might be that this is the neatest solution that we can come up with but
> more eyes would be good!

In short, please do NOT do this.

At all.

no.

{sigh}

> 
> Whilst I think this looks fine, I'm less confident than I'd like to be.
> 
> Jonathan
> 
> > ---
> >  drivers/iio/industrialio-buffer.c | 195 +++++++++++++++++++++++++++---
> >  drivers/iio/industrialio-core.c   |  24 ++--
> >  include/linux/iio/buffer_impl.h   |  14 ++-
> >  include/linux/iio/iio.h           |   2 +-
> >  4 files changed, 200 insertions(+), 35 deletions(-)
> > 
> > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> > index 0412c4fda4c1..0f470d902790 100644
> > --- a/drivers/iio/industrialio-buffer.c
> > +++ b/drivers/iio/industrialio-buffer.c
> > @@ -1175,8 +1175,6 @@ static ssize_t iio_buffer_store_enable(struct device *dev,
> >  	return (ret < 0) ? ret : len;
> >  }
> >  
> > -static const char * const iio_scan_elements_group_name = "scan_elements";
> > -
> >  static ssize_t iio_buffer_show_watermark(struct device *dev,
> >  					 struct device_attribute *attr,
> >  					 char *buf)
> > @@ -1252,6 +1250,124 @@ static struct attribute *iio_buffer_attrs[] = {
> >  	&dev_attr_data_available.attr,
> >  };
> >  
> > +#define to_dev_attr(_attr) container_of(_attr, struct device_attribute, attr)
> > +
> > +static ssize_t iio_buffer_dir_attr_show(struct kobject *kobj,
> > +					struct attribute *attr,
> > +					char *buf)
> > +{
> > +	struct iio_buffer *buffer = container_of(kobj, struct iio_buffer, buffer_dir);
> > +	struct device_attribute *dattr;
> > +
> > +	dattr = to_dev_attr(attr);
> > +
> > +	return dattr->show(&buffer->indio_dev->dev, dattr, buf);
> > +}


First off, you are dealing with "raw" kobjects here, below a 'struct
device' in the device tree, which means that suddenly userspace does not
know what in the world is going on, and you lost events and lots of
other stuff.

Never do this.  It should not be needed, and you are just trying to
paper over one odd decision of an api with another one you will be stuck
with for forever.

Remember the driver core can create subdirectories for your attributes
automatically if you want them to be in a subdir, but that's it, no
further than that.  Just name the attribute group.

But yes, you can not create a symlink to there, because (surprise), you
don't want to!

So please, just rethink your naming, create a totally new naming scheme
for multiple entities, and just drop the old one (or keep a single
value if you really want to.)  Don't make it harder than it has to be
please, you can never remove the "compatible symlinks", just make a new
api and move on.

thanks,

greg k-h

Re: [PATCH v2 03/12][RESEND] iio: buffer: rework buffer & scan_elements dir creation

[Greg Kroah-Hartman]

On Sun, Jan 24, 2021 at 09:07:52PM +0200, Alexandru Ardelean wrote:
> On Sun, Jan 24, 2021 at 8:13 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Fri, 22 Jan 2021 18:25:20 +0200
> > Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
> >
> > > When adding more than one IIO buffer per IIO device, we will need to create
> > > a buffer & scan_elements directory for each buffer.
> > > We also want to move the 'scan_elements' to be a sub-directory of the
> > > 'buffer' folder.
> > >
> > > The format we want to reach is, for a iio:device0 folder, for 2 buffers
> > > [for example], we have a 'buffer0' and a 'buffer1' subfolder, and each with
> > > it's own 'scan_elements' subfolder.
> > >
> > > So, for example:
> > >    iio:device0/buffer0
> > >       scan_elements/
> > >
> > >    iio:device0/buffer1
> > >       scan_elements/
> > >
> > > The other attributes under 'bufferX' would remain unchanged.
> > >
> > > However, we would also need to symlink back to the old 'buffer' &
> > > 'scan_elements' folders, to keep backwards compatibility.
> > >
> > > Doing all these, require that we maintain the kobjects for each 'bufferX'
> > > and 'scan_elements' so that we can symlink them back. We also need to
> > > implement the sysfs_ops for these folders.
> > >
> > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> >
> > +CC GregKH and Rafael W for feedback on various things inline.
> >
> > It might be that this is the neatest solution that we can come up with but
> > more eyes would be good!
> >
> > Whilst I think this looks fine, I'm less confident than I'd like to be.
> >
> > Jonathan
> >
> > > ---
> > >  drivers/iio/industrialio-buffer.c | 195 +++++++++++++++++++++++++++---
> > >  drivers/iio/industrialio-core.c   |  24 ++--
> > >  include/linux/iio/buffer_impl.h   |  14 ++-
> > >  include/linux/iio/iio.h           |   2 +-
> > >  4 files changed, 200 insertions(+), 35 deletions(-)
> > >
> > > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> > > index 0412c4fda4c1..0f470d902790 100644
> > > --- a/drivers/iio/industrialio-buffer.c
> > > +++ b/drivers/iio/industrialio-buffer.c
> > > @@ -1175,8 +1175,6 @@ static ssize_t iio_buffer_store_enable(struct device *dev,
> > >       return (ret < 0) ? ret : len;
> > >  }
> > >
> > > -static const char * const iio_scan_elements_group_name = "scan_elements";
> > > -
> > >  static ssize_t iio_buffer_show_watermark(struct device *dev,
> > >                                        struct device_attribute *attr,
> > >                                        char *buf)
> > > @@ -1252,6 +1250,124 @@ static struct attribute *iio_buffer_attrs[] = {
> > >       &dev_attr_data_available.attr,
> > >  };
> > >
> > > +#define to_dev_attr(_attr) container_of(_attr, struct device_attribute, attr)
> > > +
> > > +static ssize_t iio_buffer_dir_attr_show(struct kobject *kobj,
> > > +                                     struct attribute *attr,
> > > +                                     char *buf)
> > > +{
> > > +     struct iio_buffer *buffer = container_of(kobj, struct iio_buffer, buffer_dir);
> > > +     struct device_attribute *dattr;
> > > +
> > > +     dattr = to_dev_attr(attr);
> > > +
> > > +     return dattr->show(&buffer->indio_dev->dev, dattr, buf);
> > > +}
> > > +
> > > +static ssize_t iio_buffer_dir_attr_store(struct kobject *kobj,
> > > +                                      struct attribute *attr,
> > > +                                      const char *buf,
> > > +                                      size_t len)
> > > +{
> > > +     struct iio_buffer *buffer = container_of(kobj, struct iio_buffer, buffer_dir);
> > > +     struct device_attribute *dattr;
> > > +
> > > +     dattr = to_dev_attr(attr);
> > > +
> > > +     return dattr->store(&buffer->indio_dev->dev, dattr, buf, len);
> > > +}
> > > +
> > > +static const struct sysfs_ops iio_buffer_dir_sysfs_ops = {
> > > +     .show = iio_buffer_dir_attr_show,
> > > +     .store = iio_buffer_dir_attr_store,
> > > +};
> > > +
> > > +static struct kobj_type iio_buffer_dir_ktype = {
> > > +     .sysfs_ops = &iio_buffer_dir_sysfs_ops,
> > > +};
> > > +
> > > +static ssize_t iio_scan_el_dir_attr_show(struct kobject *kobj,
> > > +                                      struct attribute *attr,
> > > +                                      char *buf)
> > > +{
> > > +     struct iio_buffer *buffer = container_of(kobj, struct iio_buffer, scan_el_dir);
> > > +     struct device_attribute *dattr = to_dev_attr(attr);
> > > +
> > > +     return dattr->show(&buffer->indio_dev->dev, dattr, buf);
> > > +}
> > > +
> > > +static ssize_t iio_scan_el_dir_attr_store(struct kobject *kobj,
> > > +                                       struct attribute *attr,
> > > +                                       const char *buf,
> > > +                                       size_t len)
> > > +{
> > > +     struct iio_buffer *buffer = container_of(kobj, struct iio_buffer, scan_el_dir);
> > > +     struct device_attribute *dattr = to_dev_attr(attr);
> > > +
> > > +     return dattr->store(&buffer->indio_dev->dev, dattr, buf, len);
> > > +}
> > > +
> > > +static const struct sysfs_ops iio_scan_el_dir_sysfs_ops = {
> > > +     .show = iio_scan_el_dir_attr_show,
> > > +     .store = iio_scan_el_dir_attr_store,
> > > +};
> > > +
> > > +static struct kobj_type iio_scan_el_dir_ktype = {
> > > +     .sysfs_ops = &iio_scan_el_dir_sysfs_ops,
> > > +};
> > > +
> > > +/*
> > > + * These iio_sysfs_{add,del}_attrs() are essentially re-implementations of
> > > + * sysfs_create_files() & sysfs_remove_files(), but they are meant to get
> > > + * around the const-pointer mismatch situation with using them.
> > > + *
> > > + * sysfs_{create,remove}_files() uses 'const struct attribute * const *ptr',
> > > + * while these are happy with just 'struct attribute **ptr'
> > > + */
> >
> > Then to my mind the question becomes why sysfs_create_files() etc requires
> > the second level of const.  If there is justification for that relaxation here
> > we should make it more generally.
> >
> > > +static int iio_sysfs_add_attrs(struct kobject *kobj, struct attribute **ptr)
> > > +{
> > > +     int err = 0;
> > > +     int i;
> > > +
> > > +     for (i = 0; ptr[i] && !err; i++)
> > > +             err = sysfs_create_file(kobj, ptr[i]);
> > > +     if (err)
> > > +             while (--i >= 0)
> > > +                     sysfs_remove_file(kobj, ptr[i]);
> > > +     return err;
> > > +}
> > > +
> > > +static void iio_sysfs_del_attrs(struct kobject *kobj, struct attribute **ptr)
> > > +{
> > > +     int i;
> > > +
> > > +     for (i = 0; ptr[i]; i++)
> > > +             sysfs_remove_file(kobj, ptr[i]);
> > > +}
> > > +
> > > +/**
> > > + * __iio_buffer_alloc_sysfs_and_mask() - Allocate sysfs attributes to an attached buffer
> > > + * @buffer: the buffer object for which the sysfs attributes are created for
> > > + * @indio_dev: the iio device to which the iio buffer belongs to
> > > + *
> > > + * Return 0, or negative for error.
> > > + *
> > > + * This function must be called for each single buffer. The sysfs attributes for that
> > > + * buffer will be created, and the IIO device object will be the parent kobject of that
> > > + * the kobjects created here.
> > > + * Because we need to redirect sysfs attribute to it's IIO buffer object, we need to
> > > + * implement our own sysfs_ops, and each IIO buffer will keep a kobject for the
> > > + * 'bufferX' directory and one for the 'scan_elements' directory.
> > > + * And in order to do this, this function must be called after the IIO device object
> > > + * has been added via device_add(). This fundamentally changes how sysfs attributes
> > > + * were created before (with one single IIO buffer per IIO device), where the
> > > + * sysfs attributes for the buffer were mapped as attribute groups on the IIO device
> > > + * groups object list.
> >
> > Been a while, so I can't recall the exact reasons this can cause problems.
> > I've +CC Greg and Rafael for comments.
> >
> > For their reference, the aim of this set is undo an old restriction on IIO that devices
> > only had one buffer.  To do that we need to keep a iio:deviceX/buffer0 directory
> > also exposed as iio:deviceX/buffer/* and
> > iio:deviceX/buffer0/scan_elements/ as iio:deviceX/scan_elements.
> > to maintain backwards compatibility.
> >
> >
> > > + * Using kobjects directly for the 'bufferX' and 'scan_elements' directories allows
> > > + * us to symlink them back to keep backwards compatibility for the old sysfs interface
> > > + * for IIO buffers while also allowing us to support multiple IIO buffers per one
> > > + * IIO device.
> > > + */
> > >  static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
> > >                                            struct iio_dev *indio_dev)
> > >  {
> > > @@ -1282,12 +1398,16 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
> > >               memcpy(&attr[ARRAY_SIZE(iio_buffer_attrs)], buffer->attrs,
> > >                      sizeof(struct attribute *) * attrcount);
> > >
> > > -     attr[attrcount + ARRAY_SIZE(iio_buffer_attrs)] = NULL;
> > > +     buffer->buffer_attrs = attr;
> > >
> > > -     buffer->buffer_group.name = "buffer";
> > > -     buffer->buffer_group.attrs = attr;
> > > +     ret = kobject_init_and_add(&buffer->buffer_dir, &iio_buffer_dir_ktype,
> > > +                                &indio_dev->dev.kobj, "buffer");
> > > +     if (ret)
> > > +             goto error_buffer_free_attrs;
> > >
> >
> > Do we potentially want kobject_uevent calls for these?
> > (based on looking at similar code in edac).

Never use edac as a good example of how to use sysfs or the driver model
please.  It's just not right, but can't really be changed as it has been
there for too long...

And yes, the edac maintainer agrees with me :)

thanks,

greg k-h

Re: [PATCH v2 03/12][RESEND] iio: buffer: rework buffer & scan_elements dir creation

[Alexandru Ardelean]

On Mon, Jan 25, 2021 at 9:32 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Sun, Jan 24, 2021 at 06:11:26PM +0000, Jonathan Cameron wrote:
> > On Fri, 22 Jan 2021 18:25:20 +0200
> > Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
> >
> > > When adding more than one IIO buffer per IIO device, we will need to create
> > > a buffer & scan_elements directory for each buffer.
> > > We also want to move the 'scan_elements' to be a sub-directory of the
> > > 'buffer' folder.
> > >
> > > The format we want to reach is, for a iio:device0 folder, for 2 buffers
> > > [for example], we have a 'buffer0' and a 'buffer1' subfolder, and each with
> > > it's own 'scan_elements' subfolder.
> > >
> > > So, for example:
> > >    iio:device0/buffer0
> > >       scan_elements/
> > >
> > >    iio:device0/buffer1
> > >       scan_elements/
> > >
> > > The other attributes under 'bufferX' would remain unchanged.
> > >
> > > However, we would also need to symlink back to the old 'buffer' &
> > > 'scan_elements' folders, to keep backwards compatibility.
> > >
> > > Doing all these, require that we maintain the kobjects for each 'bufferX'
> > > and 'scan_elements' so that we can symlink them back. We also need to
> > > implement the sysfs_ops for these folders.
> > >
> > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> >
> > +CC GregKH and Rafael W for feedback on various things inline.
> >
> > It might be that this is the neatest solution that we can come up with but
> > more eyes would be good!
>
> In short, please do NOT do this.
>
> At all.
>
> no.
>
> {sigh}
>
> >
> > Whilst I think this looks fine, I'm less confident than I'd like to be.
> >
> > Jonathan
> >
> > > ---
> > >  drivers/iio/industrialio-buffer.c | 195 +++++++++++++++++++++++++++---
> > >  drivers/iio/industrialio-core.c   |  24 ++--
> > >  include/linux/iio/buffer_impl.h   |  14 ++-
> > >  include/linux/iio/iio.h           |   2 +-
> > >  4 files changed, 200 insertions(+), 35 deletions(-)
> > >
> > > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> > > index 0412c4fda4c1..0f470d902790 100644
> > > --- a/drivers/iio/industrialio-buffer.c
> > > +++ b/drivers/iio/industrialio-buffer.c
> > > @@ -1175,8 +1175,6 @@ static ssize_t iio_buffer_store_enable(struct device *dev,
> > >     return (ret < 0) ? ret : len;
> > >  }
> > >
> > > -static const char * const iio_scan_elements_group_name = "scan_elements";
> > > -
> > >  static ssize_t iio_buffer_show_watermark(struct device *dev,
> > >                                      struct device_attribute *attr,
> > >                                      char *buf)
> > > @@ -1252,6 +1250,124 @@ static struct attribute *iio_buffer_attrs[] = {
> > >     &dev_attr_data_available.attr,
> > >  };
> > >
> > > +#define to_dev_attr(_attr) container_of(_attr, struct device_attribute, attr)
> > > +
> > > +static ssize_t iio_buffer_dir_attr_show(struct kobject *kobj,
> > > +                                   struct attribute *attr,
> > > +                                   char *buf)
> > > +{
> > > +   struct iio_buffer *buffer = container_of(kobj, struct iio_buffer, buffer_dir);
> > > +   struct device_attribute *dattr;
> > > +
> > > +   dattr = to_dev_attr(attr);
> > > +
> > > +   return dattr->show(&buffer->indio_dev->dev, dattr, buf);
> > > +}
>
>
> First off, you are dealing with "raw" kobjects here, below a 'struct
> device' in the device tree, which means that suddenly userspace does not
> know what in the world is going on, and you lost events and lots of
> other stuff.
>
> Never do this.  It should not be needed, and you are just trying to
> paper over one odd decision of an api with another one you will be stuck
> with for forever.
>
> Remember the driver core can create subdirectories for your attributes
> automatically if you want them to be in a subdir, but that's it, no
> further than that.  Just name the attribute group.
>
> But yes, you can not create a symlink to there, because (surprise), you
> don't want to!
>
> So please, just rethink your naming, create a totally new naming scheme
> for multiple entities, and just drop the old one (or keep a single
> value if you really want to.)  Don't make it harder than it has to be
> please, you can never remove the "compatible symlinks", just make a new
> api and move on.


So, coming back to Jonathan.
Any thoughts on how to proceed?

We could merge the files 'buffer & scan_elements' [from in the
/sys/bus/iio/devices/iio:deviceX/{buffer,scan_elements}

So, essentially:
# ls /sys/bus/iio/devices/iio:deviceX/bufferY
data_available       length              watermark
enable                   length_align_bytes
in_voltage0_en      in_voltage0_type   in_voltage1_index
in_voltage0_index  in_voltage1_en     in_voltage1_type

Where:
# ls  /sys/bus/iio/devices/iio:deviceX/scan_elements
in_voltage0_en     in_voltage0_type   in_voltage1_index
in_voltage0_index  in_voltage1_en     in_voltage1_typ

# ls  /sys/bus/iio/devices/iio:deviceX/buffer
data_available      length              watermark
enable              length_align_bytes

I don't think we need to add any prefixes for the scan_elements/buffer
files, or?

Do we still do this new ioctl() for buffer0, 1, 2, N being accessed
via anon inodes?
Or do we go [back] via the route of each buffer with it's own chardev?
i.e.  introduce a "/dev/iio/deviceX/bufferY" structure

I'm fine either way.

Thanks
Alex

>
> thanks,
>
> greg k-h

Re: [PATCH v2 03/12][RESEND] iio: buffer: rework buffer & scan_elements dir creation

[Jonathan Cameron]

On Tue, 26 Jan 2021 11:45:04 +0200
Alexandru Ardelean <ardeleanalex@gmail.com> wrote:

> On Mon, Jan 25, 2021 at 9:32 PM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Sun, Jan 24, 2021 at 06:11:26PM +0000, Jonathan Cameron wrote:  
> > > On Fri, 22 Jan 2021 18:25:20 +0200
> > > Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
> > >  
> > > > When adding more than one IIO buffer per IIO device, we will need to create
> > > > a buffer & scan_elements directory for each buffer.
> > > > We also want to move the 'scan_elements' to be a sub-directory of the
> > > > 'buffer' folder.
> > > >
> > > > The format we want to reach is, for a iio:device0 folder, for 2 buffers
> > > > [for example], we have a 'buffer0' and a 'buffer1' subfolder, and each with
> > > > it's own 'scan_elements' subfolder.
> > > >
> > > > So, for example:
> > > >    iio:device0/buffer0
> > > >       scan_elements/
> > > >
> > > >    iio:device0/buffer1
> > > >       scan_elements/
> > > >
> > > > The other attributes under 'bufferX' would remain unchanged.
> > > >
> > > > However, we would also need to symlink back to the old 'buffer' &
> > > > 'scan_elements' folders, to keep backwards compatibility.
> > > >
> > > > Doing all these, require that we maintain the kobjects for each 'bufferX'
> > > > and 'scan_elements' so that we can symlink them back. We also need to
> > > > implement the sysfs_ops for these folders.
> > > >
> > > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>  
> > >
> > > +CC GregKH and Rafael W for feedback on various things inline.
> > >
> > > It might be that this is the neatest solution that we can come up with but
> > > more eyes would be good!  
> >
> > In short, please do NOT do this.
> >
> > At all.
> >
> > no.
> >
> > {sigh}
> >  
> > >
> > > Whilst I think this looks fine, I'm less confident than I'd like to be.
> > >
> > > Jonathan
> > >  
> > > > ---
> > > >  drivers/iio/industrialio-buffer.c | 195 +++++++++++++++++++++++++++---
> > > >  drivers/iio/industrialio-core.c   |  24 ++--
> > > >  include/linux/iio/buffer_impl.h   |  14 ++-
> > > >  include/linux/iio/iio.h           |   2 +-
> > > >  4 files changed, 200 insertions(+), 35 deletions(-)
> > > >
> > > > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> > > > index 0412c4fda4c1..0f470d902790 100644
> > > > --- a/drivers/iio/industrialio-buffer.c
> > > > +++ b/drivers/iio/industrialio-buffer.c
> > > > @@ -1175,8 +1175,6 @@ static ssize_t iio_buffer_store_enable(struct device *dev,
> > > >     return (ret < 0) ? ret : len;
> > > >  }
> > > >
> > > > -static const char * const iio_scan_elements_group_name = "scan_elements";
> > > > -
> > > >  static ssize_t iio_buffer_show_watermark(struct device *dev,
> > > >                                      struct device_attribute *attr,
> > > >                                      char *buf)
> > > > @@ -1252,6 +1250,124 @@ static struct attribute *iio_buffer_attrs[] = {
> > > >     &dev_attr_data_available.attr,
> > > >  };
> > > >
> > > > +#define to_dev_attr(_attr) container_of(_attr, struct device_attribute, attr)
> > > > +
> > > > +static ssize_t iio_buffer_dir_attr_show(struct kobject *kobj,
> > > > +                                   struct attribute *attr,
> > > > +                                   char *buf)
> > > > +{
> > > > +   struct iio_buffer *buffer = container_of(kobj, struct iio_buffer, buffer_dir);
> > > > +   struct device_attribute *dattr;
> > > > +
> > > > +   dattr = to_dev_attr(attr);
> > > > +
> > > > +   return dattr->show(&buffer->indio_dev->dev, dattr, buf);
> > > > +}  
> >
> >
> > First off, you are dealing with "raw" kobjects here, below a 'struct
> > device' in the device tree, which means that suddenly userspace does not
> > know what in the world is going on, and you lost events and lots of
> > other stuff.
> >
> > Never do this.  It should not be needed, and you are just trying to
> > paper over one odd decision of an api with another one you will be stuck
> > with for forever.
> >
> > Remember the driver core can create subdirectories for your attributes
> > automatically if you want them to be in a subdir, but that's it, no
> > further than that.  Just name the attribute group.
> >
> > But yes, you can not create a symlink to there, because (surprise), you
> > don't want to!
> >
> > So please, just rethink your naming, create a totally new naming scheme
> > for multiple entities, and just drop the old one (or keep a single
> > value if you really want to.)  Don't make it harder than it has to be
> > please, you can never remove the "compatible symlinks", just make a new
> > api and move on.  

Thanks Greg. I had a feeling we were pushing things too far in an
ugly direction :(

> 
> 
> So, coming back to Jonathan.
> Any thoughts on how to proceed?
> 
> We could merge the files 'buffer & scan_elements' [from in the
> /sys/bus/iio/devices/iio:deviceX/{buffer,scan_elements}
> 
> So, essentially:
> # ls /sys/bus/iio/devices/iio:deviceX/bufferY
> data_available       length              watermark
> enable                   length_align_bytes
> in_voltage0_en      in_voltage0_type   in_voltage1_index
> in_voltage0_index  in_voltage1_en     in_voltage1_type
> 
> Where:
> # ls  /sys/bus/iio/devices/iio:deviceX/scan_elements
> in_voltage0_en     in_voltage0_type   in_voltage1_index
> in_voltage0_index  in_voltage1_en     in_voltage1_typ
> 
> # ls  /sys/bus/iio/devices/iio:deviceX/buffer
> data_available      length              watermark
> enable              length_align_bytes
> 
> I don't think we need to add any prefixes for the scan_elements/buffer
> files, or?

Hmm. I guess this works. The only alternative I can think of would
be bufferY and bufferY_scan_elements directories, but that is probably
worse than what you suggest.

I'm not keen on the lost of grouping between of scan elements but it
may just be a price we have to pay.  Probably not too bad as only
in_ elements (_out shortly I guess) exist in scan_elements.

To maintain backwards compatibility we'll need to also register the
old attributes, but that shouldn't be too painful beyond a bunch
of near duplicated code.

> 
> Do we still do this new ioctl() for buffer0, 1, 2, N being accessed
> via anon inodes?
> Or do we go [back] via the route of each buffer with it's own chardev?
> i.e.  introduce a "/dev/iio/deviceX/bufferY" structure
> 

Reality is most of our devices are going to remain single buffered, so
whilst I'm not overly keen on proliferation of chardevs the cost should
be fairly small.

For separate chardevs what would the naming in /dev/ look like?
Gut feeling is stay with the anon approach, at least partly for
consistency with the event interface.

Jonathan


> I'm fine either way.
> 
> Thanks
> Alex
> 
> >
> > thanks,
> >
> > greg k-h