by Alexandru Ardelean

[permalink] [raw]

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

On Sat, Nov 21, 2020 at 8:46 PM Jonathan Cameron <[email protected]> wrote:
>
> On Tue, 17 Nov 2020 18:23:39 +0200
> Alexandru Ardelean <[email protected]> wrote:
>
> > 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.
> >
> > Signed-off-by: Alexandru Ardelean <[email protected]>
> A couple of minor things in here..
>
> Jonathan
>
> > ---
> > drivers/iio/industrialio-buffer.c | 58 +++++++++++++++++++++++++------
> > include/linux/iio/buffer_impl.h | 3 ++
> > include/linux/iio/iio-opaque.h | 4 +++
> > 3 files changed, 54 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> > index c83cec89eddf..daa68822cea7 100644
> > --- a/drivers/iio/industrialio-buffer.c
> > +++ b/drivers/iio/industrialio-buffer.c
> > @@ -1513,6 +1513,7 @@ 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;
> > @@ -1529,15 +1530,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,
> > @@ -1549,8 +1553,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);
> > + i = iio_dev_opaque->attached_buffers_cnt - 1;
>
> Perhaps just use a counter variable that is only for this then you won't need
> to set it again in this error path.

Ack.

>
> > +error_unwind_sysfs_and_mask:
> > + for (; 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;
> > return ret;
> > }
> >
> > @@ -1568,7 +1578,9 @@ static void __iio_buffer_free_sysfs_and_mask(struct iio_buffer *buffer)
> >
> > 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;
> > @@ -1576,7 +1588,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;
> > }
> >
> > /**
> > @@ -1709,14 +1727,32 @@ EXPORT_SYMBOL_GPL(iio_buffer_get_iio_dev);
> > * @buffer: The buffer to attach to the device
> > *
> > * This function attaches a buffer to a IIO device. The buffer stays attached to
> > - * the device until the device is freed. The function should only be called at
> > - * most once per device.
> > + * 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)
> > {
> > - 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) {
> > + kfree(old);
>
> Need a comment on why freeing old makes sense.

Hmm, maybe here I'd need to change this a bit.
I'm seeing a few potential issues.

>
> > + return;
> > + }
> > + 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;
> >
> > - indio_dev->buffer->indio_dev = indio_dev;
> > + iio_dev_opaque->attached_buffers[cnt - 1] = buffer;
> > + iio_dev_opaque->attached_buffers_cnt = cnt;
> > }
> > EXPORT_SYMBOL_GPL(iio_device_attach_buffer);
> > 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;
>

2020-11-23 23:32:27

by Alexandru Ardelean

[permalink] [raw]

Subject: Re: [RFC PATCH 01/12] iio: core: register chardev only if needed

On Sat, Nov 21, 2020 at 8:05 PM Jonathan Cameron <[email protected]> wrote:
>
> On Tue, 17 Nov 2020 18:23:29 +0200
> Alexandru Ardelean <[email protected]> wrote:
>
> > 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 <[email protected]>
> I'll be honest. I have no idea why I didn't do this in first place!
>
> I 'think' we are safe dropping this but I suppose it's possible some
> odd code checks for the chrdev presence?

So, libiio at least doesn't rely on this being there for any odd things.
But yeah, who knows what else is out there that might.
I was also thinking of sending this separately to have this earlier
out there in case it bothers other people.
I guess I got a little mixed by other patches and re-ordered things a
few times and this remained in this series.

>
> Hopefully not though.
>
> Jonathan
>
> > ---
> > 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 419d6f8acc13..ca8b11541477 100644
> > --- a/drivers/iio/industrialio-core.c
> > +++ b/drivers/iio/industrialio-core.c
> > @@ -1763,6 +1763,15 @@ static const struct file_operations iio_buffer_fileops = {
> > .compat_ioctl = compat_ptr_ioctl,
> > };
> >
> > +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;
> > @@ -1790,6 +1799,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)
> > @@ -1807,9 +1817,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);
> > @@ -1838,9 +1845,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)
>