Re: [PATCH v3 04/11] iio: core: rework iio device group creation

From: Alexandru Ardelean
Date: Fri Feb 05 2021 - 02:33:53 EST

Next message: Max Filippov: "Re: [PATCH] mm/memtest: Add ARCH_USE_MEMTEST"
Previous message: Ming Lei: "Re: [PATCH 2/2] block: avoid to drop & re-add partitions if partitions aren't changed"
In reply to: Jonathan Cameron: "Re: [PATCH v3 04/11] iio: core: rework iio device group creation"
Next in thread: Alexandru Ardelean: "[PATCH v3 02/11] iio: core: register chardev only if needed"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Thu, Feb 4, 2021 at 7:39 PM Jonathan Cameron
<Jonathan.Cameron@xxxxxxxxxx> wrote:
>
> On Mon, 1 Feb 2021 16:50:58 +0200
> Alexandru Ardelean <alexandru.ardelean@xxxxxxxxxx> wrote:
>
> > Up until now, the device groups that an IIO device had were limited to 6.
> > Two of these groups would account for buffer attributes (the buffer/ and
> > scan_elements/ directories).
> >
> > Since we want to add multiple buffers per IIO device, this number may not
> > be enough, when adding a second buffer. So, this change reallocates the
> > groups array whenever an IIO device group is added, via a
> > iio_device_register_sysfs_group() helper.
> >
> > This also means that the groups array should be assigned to
> > 'indio_dev.dev.groups' really late, right before {cdev_}device_add() is
> > called to do the entire setup.
> > And we also must take care to free this array when the sysfs resources are
> > being cleaned up.
> >
> > With this change we can also move the 'groups' & 'groupcounter' fields to
> > the iio_dev_opaque object. Up until now, this didn't make a whole lot of
> > sense (especially since we weren't sure how multibuffer support would look
> > like in the end).
> > But doing it now kills one birds with one stone.
> >
> > An alternative, would be to add a configurable Kconfig symbol
> > CONFIG_IIO_MAX_BUFFERS_PER_DEVICE (or something like that) and compute a
> > static maximum of the groups we can support per IIO device. But that would
> > probably annoy a few people since that would make the system less
> > configurable.
> >
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@xxxxxxxxxx>
> Nice change irrespective of the rest of the series needing it.
>
> Few comments below.
>
> Jonathan
>
> > ---
> > drivers/iio/iio_core.h | 3 +++
> > drivers/iio/industrialio-buffer.c | 12 +++++++++--
> > drivers/iio/industrialio-core.c | 32 +++++++++++++++++++++++++++---
> > drivers/iio/industrialio-event.c | 5 ++++-
> > drivers/iio/industrialio-trigger.c | 6 ++----
> > include/linux/iio/iio-opaque.h | 4 ++++
> > include/linux/iio/iio.h | 5 -----
> > 7 files changed, 52 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h
> > index fced02cadcc3..7d5b179c1fe7 100644
> > --- a/drivers/iio/iio_core.h
> > +++ b/drivers/iio/iio_core.h
> > @@ -46,6 +46,9 @@ int __iio_add_chan_devattr(const char *postfix,
> > struct list_head *attr_list);
> > void iio_free_chan_devattr_list(struct list_head *attr_list);
> >
> > +int iio_device_register_sysfs_group(struct iio_dev *indio_dev,
> > + const struct attribute_group *group);
> > +
> > ssize_t iio_format_value(char *buf, unsigned int type, int size, int *vals);
> >
> > /* Event interface flags */
> > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> > index 2f7426a2f47c..cc846988fdb9 100644
> > --- a/drivers/iio/industrialio-buffer.c
> > +++ b/drivers/iio/industrialio-buffer.c
> > @@ -1287,7 +1287,9 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
> > buffer->buffer_group.name = "buffer";
> > buffer->buffer_group.attrs = attr;
> >
> > - indio_dev->groups[indio_dev->groupcounter++] = &buffer->buffer_group;
> > + ret = iio_device_register_sysfs_group(indio_dev, &buffer->buffer_group);
> > + if (ret)
> > + goto error_free_buffer_attrs;
> >
> > attrcount = 0;
> > INIT_LIST_HEAD(&buffer->scan_el_dev_attr_list);
> > @@ -1330,14 +1332,20 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
> >
> > 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;
> > +
> > + ret = iio_device_register_sysfs_group(indio_dev, &buffer->scan_el_group);
> > + if (ret)
> > + goto error_free_scan_el_attrs;
> >
> > return 0;
> >
> > +error_free_scan_el_attrs:
> > + kfree(buffer->scan_el_group.attrs);
> > error_free_scan_mask:
> > bitmap_free(buffer->scan_mask);
> > error_cleanup_dynamic:
> > iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list);
> > +error_free_buffer_attrs:
> > kfree(buffer->buffer_group.attrs);
> >
> > return ret;
> > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> > index 0a6fd299a978..ccd7aaff6d13 100644
> > --- a/drivers/iio/industrialio-core.c
> > +++ b/drivers/iio/industrialio-core.c
> > @@ -1452,6 +1452,25 @@ static ssize_t iio_store_timestamp_clock(struct device *dev,
> > return len;
> > }
> >
> > +int iio_device_register_sysfs_group(struct iio_dev *indio_dev,
> > + const struct attribute_group *group)
> > +{
> > + struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
> > + const struct attribute_group **new, **old = iio_dev_opaque->groups;
> > + unsigned int cnt = iio_dev_opaque->groupcounter;
> > +
> > + new = krealloc(old, sizeof(*new) * (cnt + 2), GFP_KERNEL);
> > + if (!new)
> > + return -ENOMEM;
> > +
> > + new[iio_dev_opaque->groupcounter++] = group;
> > + new[iio_dev_opaque->groupcounter] = NULL;
> > +
> > + iio_dev_opaque->groups = new;
> > +
> > + return 0;
> > +}
> > +
> > static DEVICE_ATTR(current_timestamp_clock, S_IRUGO | S_IWUSR,
> > iio_show_timestamp_clock, iio_store_timestamp_clock);
> >
> > @@ -1525,8 +1544,10 @@ static int iio_device_register_sysfs(struct iio_dev *indio_dev)
> > if (clk)
> > iio_dev_opaque->chan_attr_group.attrs[attrn++] = clk;
> >
> > - indio_dev->groups[indio_dev->groupcounter++] =
> > - &iio_dev_opaque->chan_attr_group;
> > + ret = iio_device_register_sysfs_group(indio_dev,
> > + &iio_dev_opaque->chan_attr_group);
> > + if (ret)
> > + goto error_clear_attrs;
> >
> > return 0;
> >
> > @@ -1543,6 +1564,9 @@ static void iio_device_unregister_sysfs(struct iio_dev *indio_dev)
> > iio_free_chan_devattr_list(&iio_dev_opaque->channel_attr_list);
> > kfree(iio_dev_opaque->chan_attr_group.attrs);
> > iio_dev_opaque->chan_attr_group.attrs = NULL;
> > + kfree(iio_dev_opaque->groups);
> > + iio_dev_opaque->groups = NULL;
> I can see you are matching style above, but right now I can't see why
> we set chan_attr_group.attrs = NULL
> or this new case.
>
> > + iio_dev_opaque->groupcounter = 0;
>
> or indeed groupcounter.

I'll remove this.
At some point I was maybe thinking that iio_device_unregister_sysfs()
might be called to unregister and re-register.
But we're nowhere close to that.
And I had nothing in mind to do something similar.
Just that the name suggested that this might be an idea.

>
> > }
> >
> > static void iio_dev_release(struct device *device)
> > @@ -1592,7 +1616,6 @@ struct iio_dev *iio_device_alloc(struct device *parent, int sizeof_priv)
> > ALIGN(sizeof(struct iio_dev_opaque), IIO_ALIGN);
> >
> > dev->dev.parent = parent;
> > - dev->dev.groups = dev->groups;
>
> Hohum. Nothing to do with this patch but not sure why the iio_dev is called dev here...
> I (or someone else) should fix that at somepoint after this is in place as it confused me.

I also noticed this a while ago and wanted to fix it.
But with other patches to work on, I defered it, also because some of
them were overlapping.
Maybe I can squeeze this at the end of this series.

>
>
> > dev->dev.type = &iio_device_type;
> > dev->dev.bus = &iio_bus_type;
> > device_initialize(&dev->dev);
> > @@ -1853,6 +1876,9 @@ int __iio_device_register(struct iio_dev *indio_dev, struct module *this_mod)
> > indio_dev->chrdev.owner = this_mod;
> > }
> >
> > + /* assign device groups now; they should be all registered now */
> > + indio_dev->dev.groups = iio_dev_opaque->groups;
> > +
> > ret = cdev_device_add(&indio_dev->chrdev, &indio_dev->dev);
> > if (ret < 0)
> > goto error_unreg_eventset;
> > diff --git a/drivers/iio/industrialio-event.c b/drivers/iio/industrialio-event.c
> > index 7e532117ac55..ea8947cc21e4 100644
> > --- a/drivers/iio/industrialio-event.c
> > +++ b/drivers/iio/industrialio-event.c
> > @@ -544,7 +544,10 @@ int iio_device_register_eventset(struct iio_dev *indio_dev)
> > /* Add all elements from the list. */
> > list_for_each_entry(p, &ev_int->dev_attr_list, l)
> > ev_int->group.attrs[attrn++] = &p->dev_attr.attr;
> > - indio_dev->groups[indio_dev->groupcounter++] = &ev_int->group;
> > +
> > + ret = iio_device_register_sysfs_group(indio_dev, &ev_int->group);
> > + if (ret)
> > + goto error_free_setup_event_lines;
> >
> > ev_int->ioctl_handler.ioctl = iio_event_ioctl;
> > iio_device_ioctl_handler_register(&iio_dev_opaque->indio_dev,
> > diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c
> > index 438d5012e8b8..a035d5c2a445 100644
> > --- a/drivers/iio/industrialio-trigger.c
> > +++ b/drivers/iio/industrialio-trigger.c
> > @@ -694,10 +694,8 @@ EXPORT_SYMBOL(iio_trigger_validate_own_device);
> >
> > int iio_device_register_trigger_consumer(struct iio_dev *indio_dev)
> > {
> > - indio_dev->groups[indio_dev->groupcounter++] =
> > - &iio_trigger_consumer_attr_group;
> > -
> > - return 0;
> > + return iio_device_register_sysfs_group(indio_dev,
> > + &iio_trigger_consumer_attr_group);
> > }
> >
> > void iio_device_unregister_trigger_consumer(struct iio_dev *indio_dev)
> > diff --git a/include/linux/iio/iio-opaque.h b/include/linux/iio/iio-opaque.h
> > index 07c5a8e52ca8..8ba13a5c7af6 100644
> > --- a/include/linux/iio/iio-opaque.h
> > +++ b/include/linux/iio/iio-opaque.h
> > @@ -12,6 +12,8 @@
> > * attributes
> > * @chan_attr_group: group for all attrs in base directory
> > * @ioctl_handlers: ioctl handlers registered with the core handler
> > + * @groups: attribute groups
> > + * @groupcounter: index of next attribute group
> > * @debugfs_dentry: device specific debugfs dentry
> > * @cached_reg_addr: cached register address for debugfs reads
> > * @read_buf: read buffer to be used for the initial reg read
> > @@ -24,6 +26,8 @@ struct iio_dev_opaque {
> > struct list_head channel_attr_list;
> > struct attribute_group chan_attr_group;
> > struct list_head ioctl_handlers;
> > + const struct attribute_group **groups;
> > + int groupcounter;
> > #if defined(CONFIG_DEBUG_FS)
> > struct dentry *debugfs_dentry;
> > unsigned cached_reg_addr;
> > diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> > index e4a9822e6495..f8585d01fc76 100644
> > --- a/include/linux/iio/iio.h
> > +++ b/include/linux/iio/iio.h
> > @@ -518,8 +518,6 @@ struct iio_buffer_setup_ops {
> > * @setup_ops: [DRIVER] callbacks to call before and after buffer
> > * enable/disable
> > * @chrdev: [INTERN] associated character device
> > - * @groups: [INTERN] attribute groups
> > - * @groupcounter: [INTERN] index of next attribute group
> > * @flags: [INTERN] file ops related flags including busy flag.
> > * @priv: [DRIVER] reference to driver's private information
> > * **MUST** be accessed **ONLY** via iio_priv() helper
> > @@ -556,9 +554,6 @@ struct iio_dev {
> > struct mutex info_exist_lock;
> > const struct iio_buffer_setup_ops *setup_ops;
> > struct cdev chrdev;
> > -#define IIO_MAX_GROUPS 6
> > - const struct attribute_group *groups[IIO_MAX_GROUPS + 1];
> > - int groupcounter;
> >
> > unsigned long flags;
> > void *priv;
>

Next message: Max Filippov: "Re: [PATCH] mm/memtest: Add ARCH_USE_MEMTEST"
Previous message: Ming Lei: "Re: [PATCH 2/2] block: avoid to drop & re-add partitions if partitions aren't changed"
In reply to: Jonathan Cameron: "Re: [PATCH v3 04/11] iio: core: rework iio device group creation"
Next in thread: Alexandru Ardelean: "[PATCH v3 02/11] iio: core: register chardev only if needed"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]