| Message ID | 20210215104043.91251-15-alexandru.ardelean@analog.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iio: core,buffer: add support for multiple IIO buffers per IIO device | expand |
Hi On 15.02.2021 11:40, Alexandru Ardelean wrote: > This change wraps all buffer attributes into iio_dev_attr objects, and > assigns a reference to the IIO buffer they belong to. > > With the addition of multiple IIO buffers per one IIO device, we need a way > to know which IIO buffer is being enabled/disabled/controlled. > > We know that all buffer attributes are device_attributes. So we can wrap > them with a iio_dev_attr types. In the iio_dev_attr type, we can also hold > a reference to an IIO buffer. > So, we end up being able to allocate wrapped attributes for all buffer > attributes (even the one from other drivers). > > The neat part with this mechanism, is that we don't need to add any extra > cleanup, because these attributes are being added to a dynamic list that > will get cleaned up via iio_free_chan_devattr_list(). > > With this change, the 'buffer->scan_el_dev_attr_list' list is being renamed > to 'buffer->buffer_attr_list', effectively merging (or finalizing the > merge) of the buffer/ & scan_elements/ attributes internally. > > Accessing these new buffer attributes can now be done via > 'to_iio_dev_attr(attr)->buffer' inside the show/store handlers. > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> This patch landed recently in linux-next as commit 15097c7a1adc ("iio: buffer: wrap all buffer attributes into iio_dev_attr"). Sadly it causes a regression and triggers the lock debuging warning: ak8975 9-000c: mounting matrix not found: using identity... ak8975 9-000c: supply vdd not found, using dummy regulator ak8975 9-000c: supply vid not found, using dummy regulator BUG: key cf40d08c has not been registered! ------------[ cut here ]------------ WARNING: CPU: 2 PID: 1 at kernel/locking/lockdep.c:4686 __kernfs_create_file+0x7c/0xfc DEBUG_LOCKS_WARN_ON(1) Modules linked in: CPU: 2 PID: 1 Comm: swapper/0 Not tainted 5.12.0-rc2-00153-g15097c7a1adc #2828 Hardware name: Samsung Exynos (Flattened Device Tree) [<c011170c>] (unwind_backtrace) from [<c010cf74>] (show_stack+0x10/0x14) [<c010cf74>] (show_stack) from [<c0b48650>] (dump_stack+0xa4/0xc4) [<c0b48650>] (dump_stack) from [<c01274c8>] (__warn+0x118/0x11c) [<c01274c8>] (__warn) from [<c0127544>] (warn_slowpath_fmt+0x78/0xbc) [<c0127544>] (warn_slowpath_fmt) from [<c038db90>] (__kernfs_create_file+0x7c/0xfc) [<c038db90>] (__kernfs_create_file) from [<c038e870>] (sysfs_add_file_mode_ns+0xa0/0x1cc) [<c038e870>] (sysfs_add_file_mode_ns) from [<c038f4e0>] (internal_create_group+0x138/0x3f4) [<c038f4e0>] (internal_create_group) from [<c038fd64>] (internal_create_groups+0x48/0x88) [<c038fd64>] (internal_create_groups) from [<c06a8754>] (device_add+0x2e4/0x7ec) [<c06a8754>] (device_add) from [<c02e46a0>] (cdev_device_add+0x48/0x80) [<c02e46a0>] (cdev_device_add) from [<c08db1a4>] (__iio_device_register+0x670/0x7c0) [<c08db1a4>] (__iio_device_register) from [<c08e2efc>] (ak8975_probe+0x3a4/0x584) [<c08e2efc>] (ak8975_probe) from [<c082e120>] (i2c_device_probe+0x234/0x2a4) [<c082e120>] (i2c_device_probe) from [<c06abea4>] (really_probe+0x1d4/0x4ec) [<c06abea4>] (really_probe) from [<c06ac234>] (driver_probe_device+0x78/0x1d8) [<c06ac234>] (driver_probe_device) from [<c06ac74c>] (device_driver_attach+0x58/0x60) [<c06ac74c>] (device_driver_attach) from [<c06ac850>] (__driver_attach+0xfc/0x160) [<c06ac850>] (__driver_attach) from [<c06a9e58>] (bus_for_each_dev+0x6c/0xb8) [<c06a9e58>] (bus_for_each_dev) from [<c06aaf90>] (bus_add_driver+0x170/0x20c) [<c06aaf90>] (bus_add_driver) from [<c06ad6c8>] (driver_register+0x78/0x10c) [<c06ad6c8>] (driver_register) from [<c082f0d4>] (i2c_register_driver+0x3c/0xac) [<c082f0d4>] (i2c_register_driver) from [<c0102434>] (do_one_initcall+0x88/0x430) [<c0102434>] (do_one_initcall) from [<c11010d4>] (kernel_init_freeable+0x190/0x1e0) [<c11010d4>] (kernel_init_freeable) from [<c0b4c564>] (kernel_init+0x8/0x118) [<c0b4c564>] (kernel_init) from [<c010011c>] (ret_from_fork+0x14/0x38) Exception stack(0xc1d09fb0 to 0xc1d09ff8) 9fa0: 00000000 00000000 00000000 00000000 9fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 9fe0: 00000000 00000000 00000000 00000000 00000013 00000000 irq event stamp: 485619 hardirqs last enabled at (485619): [<c01a51fc>] console_unlock+0x500/0x648 hardirqs last disabled at (485618): [<c01a51e0>] console_unlock+0x4e4/0x648 softirqs last enabled at (484774): [<c0101790>] __do_softirq+0x528/0x63c softirqs last disabled at (484769): [<c0130a38>] irq_exit+0x1f4/0x1fc ---[ end trace 21850020cbfb3350 ]--- The above warning is probably caused by copying struct device_attribute objects in iio_buffer_wrap_attr() without re-initializing spinlocks/mutexes. Locks debuging depends on the spinlock/mutex initializers, which register them (as absolute value of the pointer to them) to the debugging engine. After copying the structures, the objects don't match the pointers they were registered. I didn't have time to analyze it further and find which object/lock is triggering this though. > --- > drivers/iio/industrialio-buffer.c | 76 ++++++++++++++++++++----------- > include/linux/iio/buffer_impl.h | 4 +- > 2 files changed, 52 insertions(+), 28 deletions(-) > > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c > index e6edec3bcb73..8dc140f13b99 100644 > --- a/drivers/iio/industrialio-buffer.c > +++ b/drivers/iio/industrialio-buffer.c > @@ -253,8 +253,7 @@ static ssize_t iio_scan_el_show(struct device *dev, > char *buf) > { > int ret; > - struct iio_dev *indio_dev = dev_to_iio_dev(dev); > - struct iio_buffer *buffer = indio_dev->buffer; > + struct iio_buffer *buffer = to_iio_dev_attr(attr)->buffer; > > /* Ensure ret is 0 or 1. */ > ret = !!test_bit(to_iio_dev_attr(attr)->address, > @@ -367,8 +366,8 @@ static ssize_t iio_scan_el_store(struct device *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); > + struct iio_buffer *buffer = this_attr->buffer; > > ret = strtobool(buf, &state); > if (ret < 0) > @@ -402,8 +401,7 @@ static ssize_t iio_scan_el_ts_show(struct device *dev, > struct device_attribute *attr, > char *buf) > { > - struct iio_dev *indio_dev = dev_to_iio_dev(dev); > - struct iio_buffer *buffer = indio_dev->buffer; > + struct iio_buffer *buffer = to_iio_dev_attr(attr)->buffer; > > return sprintf(buf, "%d\n", buffer->scan_timestamp); > } > @@ -415,7 +413,7 @@ static ssize_t iio_scan_el_ts_store(struct device *dev, > { > int ret; > struct iio_dev *indio_dev = dev_to_iio_dev(dev); > - struct iio_buffer *buffer = indio_dev->buffer; > + struct iio_buffer *buffer = to_iio_dev_attr(attr)->buffer; > bool state; > > ret = strtobool(buf, &state); > @@ -448,7 +446,7 @@ static int iio_buffer_add_channel_sysfs(struct iio_dev *indio_dev, > IIO_SEPARATE, > &indio_dev->dev, > buffer, > - &buffer->scan_el_dev_attr_list); > + &buffer->buffer_attr_list); > if (ret) > return ret; > attrcount++; > @@ -460,7 +458,7 @@ static int iio_buffer_add_channel_sysfs(struct iio_dev *indio_dev, > 0, > &indio_dev->dev, > buffer, > - &buffer->scan_el_dev_attr_list); > + &buffer->buffer_attr_list); > if (ret) > return ret; > attrcount++; > @@ -473,7 +471,7 @@ static int iio_buffer_add_channel_sysfs(struct iio_dev *indio_dev, > 0, > &indio_dev->dev, > buffer, > - &buffer->scan_el_dev_attr_list); > + &buffer->buffer_attr_list); > else > ret = __iio_add_chan_devattr("en", > chan, > @@ -483,7 +481,7 @@ static int iio_buffer_add_channel_sysfs(struct iio_dev *indio_dev, > 0, > &indio_dev->dev, > buffer, > - &buffer->scan_el_dev_attr_list); > + &buffer->buffer_attr_list); > if (ret) > return ret; > attrcount++; > @@ -495,8 +493,7 @@ static ssize_t iio_buffer_read_length(struct device *dev, > struct device_attribute *attr, > char *buf) > { > - struct iio_dev *indio_dev = dev_to_iio_dev(dev); > - struct iio_buffer *buffer = indio_dev->buffer; > + struct iio_buffer *buffer = to_iio_dev_attr(attr)->buffer; > > return sprintf(buf, "%d\n", buffer->length); > } > @@ -506,7 +503,7 @@ static ssize_t iio_buffer_write_length(struct device *dev, > 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_buffer *buffer = to_iio_dev_attr(attr)->buffer; > unsigned int val; > int ret; > > @@ -538,8 +535,7 @@ static ssize_t iio_buffer_show_enable(struct device *dev, > struct device_attribute *attr, > char *buf) > { > - struct iio_dev *indio_dev = dev_to_iio_dev(dev); > - struct iio_buffer *buffer = indio_dev->buffer; > + struct iio_buffer *buffer = to_iio_dev_attr(attr)->buffer; > > return sprintf(buf, "%d\n", iio_buffer_is_active(buffer)); > } > @@ -1154,7 +1150,7 @@ static ssize_t iio_buffer_store_enable(struct device *dev, > int ret; > bool requested_state; > struct iio_dev *indio_dev = dev_to_iio_dev(dev); > - struct iio_buffer *buffer = indio_dev->buffer; > + struct iio_buffer *buffer = to_iio_dev_attr(attr)->buffer; > bool inlist; > > ret = strtobool(buf, &requested_state); > @@ -1183,8 +1179,7 @@ static ssize_t iio_buffer_show_watermark(struct device *dev, > struct device_attribute *attr, > char *buf) > { > - struct iio_dev *indio_dev = dev_to_iio_dev(dev); > - struct iio_buffer *buffer = indio_dev->buffer; > + struct iio_buffer *buffer = to_iio_dev_attr(attr)->buffer; > > return sprintf(buf, "%u\n", buffer->watermark); > } > @@ -1195,7 +1190,7 @@ static ssize_t iio_buffer_store_watermark(struct device *dev, > size_t len) > { > struct iio_dev *indio_dev = dev_to_iio_dev(dev); > - struct iio_buffer *buffer = indio_dev->buffer; > + struct iio_buffer *buffer = to_iio_dev_attr(attr)->buffer; > unsigned int val; > int ret; > > @@ -1228,8 +1223,7 @@ static ssize_t iio_dma_show_data_available(struct device *dev, > struct device_attribute *attr, > char *buf) > { > - struct iio_dev *indio_dev = dev_to_iio_dev(dev); > - struct iio_buffer *buffer = indio_dev->buffer; > + struct iio_buffer *buffer = to_iio_dev_attr(attr)->buffer; > > return sprintf(buf, "%zu\n", iio_buffer_data_available(buffer)); > } > @@ -1254,6 +1248,26 @@ static struct attribute *iio_buffer_attrs[] = { > &dev_attr_data_available.attr, > }; > > +#define to_dev_attr(_attr) container_of(_attr, struct device_attribute, attr) > + > +static struct attribute *iio_buffer_wrap_attr(struct iio_buffer *buffer, > + struct attribute *attr) > +{ > + struct device_attribute *dattr = to_dev_attr(attr); > + struct iio_dev_attr *iio_attr; > + > + iio_attr = kzalloc(sizeof(*iio_attr), GFP_KERNEL); > + if (!iio_attr) > + return NULL; > + > + iio_attr->buffer = buffer; > + memcpy(&iio_attr->dev_attr, dattr, sizeof(iio_attr->dev_attr)); > + > + list_add(&iio_attr->l, &buffer->buffer_attr_list); > + > + return &iio_attr->dev_attr.attr; > +} > + > static int iio_buffer_register_legacy_sysfs_groups(struct iio_dev *indio_dev, > struct attribute **buffer_attrs, > int buffer_attrcount, > @@ -1329,7 +1343,7 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer, > } > > scan_el_attrcount = 0; > - INIT_LIST_HEAD(&buffer->scan_el_dev_attr_list); > + INIT_LIST_HEAD(&buffer->buffer_attr_list); > channels = indio_dev->channels; > if (channels) { > /* new magic */ > @@ -1376,9 +1390,19 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer, > > buffer_attrcount += ARRAY_SIZE(iio_buffer_attrs); > > - attrn = buffer_attrcount; > + for (i = 0; i < buffer_attrcount; i++) { > + struct attribute *wrapped; > + > + wrapped = iio_buffer_wrap_attr(buffer, attr[i]); > + if (!wrapped) { > + ret = -ENOMEM; > + goto error_free_scan_mask; > + } > + attr[i] = wrapped; > + } > > - list_for_each_entry(p, &buffer->scan_el_dev_attr_list, l) > + attrn = 0; > + list_for_each_entry(p, &buffer->buffer_attr_list, l) > attr[attrn++] = &p->dev_attr.attr; > > buffer->buffer_group.name = kasprintf(GFP_KERNEL, "buffer%d", index); > @@ -1412,7 +1436,7 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer, > error_free_scan_mask: > bitmap_free(buffer->scan_mask); > error_cleanup_dynamic: > - iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list); > + iio_free_chan_devattr_list(&buffer->buffer_attr_list); > > return ret; > } > @@ -1443,7 +1467,7 @@ static void __iio_buffer_free_sysfs_and_mask(struct iio_buffer *buffer) > bitmap_free(buffer->scan_mask); > kfree(buffer->buffer_group.name); > kfree(buffer->buffer_group.attrs); > - iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list); > + iio_free_chan_devattr_list(&buffer->buffer_attr_list); > } > > void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev) > diff --git a/include/linux/iio/buffer_impl.h b/include/linux/iio/buffer_impl.h > index 3e555e58475b..41044320e581 100644 > --- a/include/linux/iio/buffer_impl.h > +++ b/include/linux/iio/buffer_impl.h > @@ -97,8 +97,8 @@ struct iio_buffer { > /* @scan_timestamp: Does the scan mode include a timestamp. */ > bool scan_timestamp; > > - /* @scan_el_dev_attr_list: List of scan element related attributes. */ > - struct list_head scan_el_dev_attr_list; > + /* @buffer_attr_list: List of buffer attributes. */ > + struct list_head buffer_attr_list; > > /* > * @buffer_group: Attributes of the new buffer group. Best regards
On Thu, 1 Apr 2021 09:39:47 +0200 Marek Szyprowski <m.szyprowski@samsung.com> wrote: > Hi > > On 15.02.2021 11:40, Alexandru Ardelean wrote: > > This change wraps all buffer attributes into iio_dev_attr objects, and > > assigns a reference to the IIO buffer they belong to. > > > > With the addition of multiple IIO buffers per one IIO device, we need a way > > to know which IIO buffer is being enabled/disabled/controlled. > > > > We know that all buffer attributes are device_attributes. So we can wrap > > them with a iio_dev_attr types. In the iio_dev_attr type, we can also hold > > a reference to an IIO buffer. > > So, we end up being able to allocate wrapped attributes for all buffer > > attributes (even the one from other drivers). > > > > The neat part with this mechanism, is that we don't need to add any extra > > cleanup, because these attributes are being added to a dynamic list that > > will get cleaned up via iio_free_chan_devattr_list(). > > > > With this change, the 'buffer->scan_el_dev_attr_list' list is being renamed > > to 'buffer->buffer_attr_list', effectively merging (or finalizing the > > merge) of the buffer/ & scan_elements/ attributes internally. > > > > Accessing these new buffer attributes can now be done via > > 'to_iio_dev_attr(attr)->buffer' inside the show/store handlers. > > > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> > > This patch landed recently in linux-next as commit 15097c7a1adc ("iio: > buffer: wrap all buffer attributes into iio_dev_attr"). Sadly it causes > a regression and triggers the lock debuging warning: > > ak8975 9-000c: mounting matrix not found: using identity... > ak8975 9-000c: supply vdd not found, using dummy regulator > ak8975 9-000c: supply vid not found, using dummy regulator > BUG: key cf40d08c has not been registered! > ------------[ cut here ]------------ > WARNING: CPU: 2 PID: 1 at kernel/locking/lockdep.c:4686 > __kernfs_create_file+0x7c/0xfc > DEBUG_LOCKS_WARN_ON(1) > Modules linked in: > CPU: 2 PID: 1 Comm: swapper/0 Not tainted 5.12.0-rc2-00153-g15097c7a1adc > #2828 > Hardware name: Samsung Exynos (Flattened Device Tree) > [<c011170c>] (unwind_backtrace) from [<c010cf74>] (show_stack+0x10/0x14) > [<c010cf74>] (show_stack) from [<c0b48650>] (dump_stack+0xa4/0xc4) > [<c0b48650>] (dump_stack) from [<c01274c8>] (__warn+0x118/0x11c) > [<c01274c8>] (__warn) from [<c0127544>] (warn_slowpath_fmt+0x78/0xbc) > [<c0127544>] (warn_slowpath_fmt) from [<c038db90>] > (__kernfs_create_file+0x7c/0xfc) > [<c038db90>] (__kernfs_create_file) from [<c038e870>] > (sysfs_add_file_mode_ns+0xa0/0x1cc) > [<c038e870>] (sysfs_add_file_mode_ns) from [<c038f4e0>] > (internal_create_group+0x138/0x3f4) > [<c038f4e0>] (internal_create_group) from [<c038fd64>] > (internal_create_groups+0x48/0x88) > [<c038fd64>] (internal_create_groups) from [<c06a8754>] > (device_add+0x2e4/0x7ec) > [<c06a8754>] (device_add) from [<c02e46a0>] (cdev_device_add+0x48/0x80) > [<c02e46a0>] (cdev_device_add) from [<c08db1a4>] > (__iio_device_register+0x670/0x7c0) > [<c08db1a4>] (__iio_device_register) from [<c08e2efc>] > (ak8975_probe+0x3a4/0x584) > [<c08e2efc>] (ak8975_probe) from [<c082e120>] (i2c_device_probe+0x234/0x2a4) > [<c082e120>] (i2c_device_probe) from [<c06abea4>] (really_probe+0x1d4/0x4ec) > [<c06abea4>] (really_probe) from [<c06ac234>] > (driver_probe_device+0x78/0x1d8) > [<c06ac234>] (driver_probe_device) from [<c06ac74c>] > (device_driver_attach+0x58/0x60) > [<c06ac74c>] (device_driver_attach) from [<c06ac850>] > (__driver_attach+0xfc/0x160) > [<c06ac850>] (__driver_attach) from [<c06a9e58>] > (bus_for_each_dev+0x6c/0xb8) > [<c06a9e58>] (bus_for_each_dev) from [<c06aaf90>] > (bus_add_driver+0x170/0x20c) > [<c06aaf90>] (bus_add_driver) from [<c06ad6c8>] (driver_register+0x78/0x10c) > [<c06ad6c8>] (driver_register) from [<c082f0d4>] > (i2c_register_driver+0x3c/0xac) > [<c082f0d4>] (i2c_register_driver) from [<c0102434>] > (do_one_initcall+0x88/0x430) > [<c0102434>] (do_one_initcall) from [<c11010d4>] > (kernel_init_freeable+0x190/0x1e0) > [<c11010d4>] (kernel_init_freeable) from [<c0b4c564>] > (kernel_init+0x8/0x118) > [<c0b4c564>] (kernel_init) from [<c010011c>] (ret_from_fork+0x14/0x38) > Exception stack(0xc1d09fb0 to 0xc1d09ff8) > 9fa0: 00000000 00000000 00000000 > 00000000 > 9fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 > 00000000 > 9fe0: 00000000 00000000 00000000 00000000 00000013 00000000 > irq event stamp: 485619 > hardirqs last enabled at (485619): [<c01a51fc>] console_unlock+0x500/0x648 > hardirqs last disabled at (485618): [<c01a51e0>] console_unlock+0x4e4/0x648 > softirqs last enabled at (484774): [<c0101790>] __do_softirq+0x528/0x63c > softirqs last disabled at (484769): [<c0130a38>] irq_exit+0x1f4/0x1fc > ---[ end trace 21850020cbfb3350 ]--- > > The above warning is probably caused by copying struct device_attribute > objects in iio_buffer_wrap_attr() without re-initializing > spinlocks/mutexes. Locks debuging depends on the spinlock/mutex > initializers, which register them (as absolute value of the pointer to > them) to the debugging engine. After copying the structures, the objects > don't match the pointers they were registered. I didn't have time to > analyze it further and find which object/lock is triggering this though. Thanks, interestingly there aren't any spinlocks or mutexes in attributes, but there are lock_class_keys. Looks like we need to call sysfs_attr_init() and I guess we are currently missing it on this path. Alex, can you take a look? If not I'll get to it sometime over the weekend most likely. Jonathan > > > --- > > drivers/iio/industrialio-buffer.c | 76 ++++++++++++++++++++----------- > > include/linux/iio/buffer_impl.h | 4 +- > > 2 files changed, 52 insertions(+), 28 deletions(-) > > > > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c > > index e6edec3bcb73..8dc140f13b99 100644 > > --- a/drivers/iio/industrialio-buffer.c > > +++ b/drivers/iio/industrialio-buffer.c > > @@ -253,8 +253,7 @@ static ssize_t iio_scan_el_show(struct device *dev, > > char *buf) > > { > > int ret; > > - struct iio_dev *indio_dev = dev_to_iio_dev(dev); > > - struct iio_buffer *buffer = indio_dev->buffer; > > + struct iio_buffer *buffer = to_iio_dev_attr(attr)->buffer; > > > > /* Ensure ret is 0 or 1. */ > > ret = !!test_bit(to_iio_dev_attr(attr)->address, > > @@ -367,8 +366,8 @@ static ssize_t iio_scan_el_store(struct device *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); > > + struct iio_buffer *buffer = this_attr->buffer; > > > > ret = strtobool(buf, &state); > > if (ret < 0) > > @@ -402,8 +401,7 @@ static ssize_t iio_scan_el_ts_show(struct device *dev, > > struct device_attribute *attr, > > char *buf) > > { > > - struct iio_dev *indio_dev = dev_to_iio_dev(dev); > > - struct iio_buffer *buffer = indio_dev->buffer; > > + struct iio_buffer *buffer = to_iio_dev_attr(attr)->buffer; > > > > return sprintf(buf, "%d\n", buffer->scan_timestamp); > > } > > @@ -415,7 +413,7 @@ static ssize_t iio_scan_el_ts_store(struct device *dev, > > { > > int ret; > > struct iio_dev *indio_dev = dev_to_iio_dev(dev); > > - struct iio_buffer *buffer = indio_dev->buffer; > > + struct iio_buffer *buffer = to_iio_dev_attr(attr)->buffer; > > bool state; > > > > ret = strtobool(buf, &state); > > @@ -448,7 +446,7 @@ static int iio_buffer_add_channel_sysfs(struct iio_dev *indio_dev, > > IIO_SEPARATE, > > &indio_dev->dev, > > buffer, > > - &buffer->scan_el_dev_attr_list); > > + &buffer->buffer_attr_list); > > if (ret) > > return ret; > > attrcount++; > > @@ -460,7 +458,7 @@ static int iio_buffer_add_channel_sysfs(struct iio_dev *indio_dev, > > 0, > > &indio_dev->dev, > > buffer, > > - &buffer->scan_el_dev_attr_list); > > + &buffer->buffer_attr_list); > > if (ret) > > return ret; > > attrcount++; > > @@ -473,7 +471,7 @@ static int iio_buffer_add_channel_sysfs(struct iio_dev *indio_dev, > > 0, > > &indio_dev->dev, > > buffer, > > - &buffer->scan_el_dev_attr_list); > > + &buffer->buffer_attr_list); > > else > > ret = __iio_add_chan_devattr("en", > > chan, > > @@ -483,7 +481,7 @@ static int iio_buffer_add_channel_sysfs(struct iio_dev *indio_dev, > > 0, > > &indio_dev->dev, > > buffer, > > - &buffer->scan_el_dev_attr_list); > > + &buffer->buffer_attr_list); > > if (ret) > > return ret; > > attrcount++; > > @@ -495,8 +493,7 @@ static ssize_t iio_buffer_read_length(struct device *dev, > > struct device_attribute *attr, > > char *buf) > > { > > - struct iio_dev *indio_dev = dev_to_iio_dev(dev); > > - struct iio_buffer *buffer = indio_dev->buffer; > > + struct iio_buffer *buffer = to_iio_dev_attr(attr)->buffer; > > > > return sprintf(buf, "%d\n", buffer->length); > > } > > @@ -506,7 +503,7 @@ static ssize_t iio_buffer_write_length(struct device *dev, > > 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_buffer *buffer = to_iio_dev_attr(attr)->buffer; > > unsigned int val; > > int ret; > > > > @@ -538,8 +535,7 @@ static ssize_t iio_buffer_show_enable(struct device *dev, > > struct device_attribute *attr, > > char *buf) > > { > > - struct iio_dev *indio_dev = dev_to_iio_dev(dev); > > - struct iio_buffer *buffer = indio_dev->buffer; > > + struct iio_buffer *buffer = to_iio_dev_attr(attr)->buffer; > > > > return sprintf(buf, "%d\n", iio_buffer_is_active(buffer)); > > } > > @@ -1154,7 +1150,7 @@ static ssize_t iio_buffer_store_enable(struct device *dev, > > int ret; > > bool requested_state; > > struct iio_dev *indio_dev = dev_to_iio_dev(dev); > > - struct iio_buffer *buffer = indio_dev->buffer; > > + struct iio_buffer *buffer = to_iio_dev_attr(attr)->buffer; > > bool inlist; > > > > ret = strtobool(buf, &requested_state); > > @@ -1183,8 +1179,7 @@ static ssize_t iio_buffer_show_watermark(struct device *dev, > > struct device_attribute *attr, > > char *buf) > > { > > - struct iio_dev *indio_dev = dev_to_iio_dev(dev); > > - struct iio_buffer *buffer = indio_dev->buffer; > > + struct iio_buffer *buffer = to_iio_dev_attr(attr)->buffer; > > > > return sprintf(buf, "%u\n", buffer->watermark); > > } > > @@ -1195,7 +1190,7 @@ static ssize_t iio_buffer_store_watermark(struct device *dev, > > size_t len) > > { > > struct iio_dev *indio_dev = dev_to_iio_dev(dev); > > - struct iio_buffer *buffer = indio_dev->buffer; > > + struct iio_buffer *buffer = to_iio_dev_attr(attr)->buffer; > > unsigned int val; > > int ret; > > > > @@ -1228,8 +1223,7 @@ static ssize_t iio_dma_show_data_available(struct device *dev, > > struct device_attribute *attr, > > char *buf) > > { > > - struct iio_dev *indio_dev = dev_to_iio_dev(dev); > > - struct iio_buffer *buffer = indio_dev->buffer; > > + struct iio_buffer *buffer = to_iio_dev_attr(attr)->buffer; > > > > return sprintf(buf, "%zu\n", iio_buffer_data_available(buffer)); > > } > > @@ -1254,6 +1248,26 @@ static struct attribute *iio_buffer_attrs[] = { > > &dev_attr_data_available.attr, > > }; > > > > +#define to_dev_attr(_attr) container_of(_attr, struct device_attribute, attr) > > + > > +static struct attribute *iio_buffer_wrap_attr(struct iio_buffer *buffer, > > + struct attribute *attr) > > +{ > > + struct device_attribute *dattr = to_dev_attr(attr); > > + struct iio_dev_attr *iio_attr; > > + > > + iio_attr = kzalloc(sizeof(*iio_attr), GFP_KERNEL); > > + if (!iio_attr) > > + return NULL; > > + > > + iio_attr->buffer = buffer; > > + memcpy(&iio_attr->dev_attr, dattr, sizeof(iio_attr->dev_attr)); > > + > > + list_add(&iio_attr->l, &buffer->buffer_attr_list); > > + > > + return &iio_attr->dev_attr.attr; > > +} > > + > > static int iio_buffer_register_legacy_sysfs_groups(struct iio_dev *indio_dev, > > struct attribute **buffer_attrs, > > int buffer_attrcount, > > @@ -1329,7 +1343,7 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer, > > } > > > > scan_el_attrcount = 0; > > - INIT_LIST_HEAD(&buffer->scan_el_dev_attr_list); > > + INIT_LIST_HEAD(&buffer->buffer_attr_list); > > channels = indio_dev->channels; > > if (channels) { > > /* new magic */ > > @@ -1376,9 +1390,19 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer, > > > > buffer_attrcount += ARRAY_SIZE(iio_buffer_attrs); > > > > - attrn = buffer_attrcount; > > + for (i = 0; i < buffer_attrcount; i++) { > > + struct attribute *wrapped; > > + > > + wrapped = iio_buffer_wrap_attr(buffer, attr[i]); > > + if (!wrapped) { > > + ret = -ENOMEM; > > + goto error_free_scan_mask; > > + } > > + attr[i] = wrapped; > > + } > > > > - list_for_each_entry(p, &buffer->scan_el_dev_attr_list, l) > > + attrn = 0; > > + list_for_each_entry(p, &buffer->buffer_attr_list, l) > > attr[attrn++] = &p->dev_attr.attr; > > > > buffer->buffer_group.name = kasprintf(GFP_KERNEL, "buffer%d", index); > > @@ -1412,7 +1436,7 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer, > > error_free_scan_mask: > > bitmap_free(buffer->scan_mask); > > error_cleanup_dynamic: > > - iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list); > > + iio_free_chan_devattr_list(&buffer->buffer_attr_list); > > > > return ret; > > } > > @@ -1443,7 +1467,7 @@ static void __iio_buffer_free_sysfs_and_mask(struct iio_buffer *buffer) > > bitmap_free(buffer->scan_mask); > > kfree(buffer->buffer_group.name); > > kfree(buffer->buffer_group.attrs); > > - iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list); > > + iio_free_chan_devattr_list(&buffer->buffer_attr_list); > > } > > > > void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev) > > diff --git a/include/linux/iio/buffer_impl.h b/include/linux/iio/buffer_impl.h > > index 3e555e58475b..41044320e581 100644 > > --- a/include/linux/iio/buffer_impl.h > > +++ b/include/linux/iio/buffer_impl.h > > @@ -97,8 +97,8 @@ struct iio_buffer { > > /* @scan_timestamp: Does the scan mode include a timestamp. */ > > bool scan_timestamp; > > > > - /* @scan_el_dev_attr_list: List of scan element related attributes. */ > > - struct list_head scan_el_dev_attr_list; > > + /* @buffer_attr_list: List of buffer attributes. */ > > + struct list_head buffer_attr_list; > > > > /* > > * @buffer_group: Attributes of the new buffer group. > > Best regards
On Thu, Apr 1, 2021 at 11:29 AM Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote: > > On Thu, 1 Apr 2021 09:39:47 +0200 > Marek Szyprowski <m.szyprowski@samsung.com> wrote: > > > Hi > > > > On 15.02.2021 11:40, Alexandru Ardelean wrote: > > > This change wraps all buffer attributes into iio_dev_attr objects, and > > > assigns a reference to the IIO buffer they belong to. > > > > > > With the addition of multiple IIO buffers per one IIO device, we need a way > > > to know which IIO buffer is being enabled/disabled/controlled. > > > > > > We know that all buffer attributes are device_attributes. So we can wrap > > > them with a iio_dev_attr types. In the iio_dev_attr type, we can also hold > > > a reference to an IIO buffer. > > > So, we end up being able to allocate wrapped attributes for all buffer > > > attributes (even the one from other drivers). > > > > > > The neat part with this mechanism, is that we don't need to add any extra > > > cleanup, because these attributes are being added to a dynamic list that > > > will get cleaned up via iio_free_chan_devattr_list(). > > > > > > With this change, the 'buffer->scan_el_dev_attr_list' list is being renamed > > > to 'buffer->buffer_attr_list', effectively merging (or finalizing the > > > merge) of the buffer/ & scan_elements/ attributes internally. > > > > > > Accessing these new buffer attributes can now be done via > > > 'to_iio_dev_attr(attr)->buffer' inside the show/store handlers. > > > > > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> > > > > This patch landed recently in linux-next as commit 15097c7a1adc ("iio: > > buffer: wrap all buffer attributes into iio_dev_attr"). Sadly it causes > > a regression and triggers the lock debuging warning: > > > > ak8975 9-000c: mounting matrix not found: using identity... > > ak8975 9-000c: supply vdd not found, using dummy regulator > > ak8975 9-000c: supply vid not found, using dummy regulator > > BUG: key cf40d08c has not been registered! > > ------------[ cut here ]------------ > > WARNING: CPU: 2 PID: 1 at kernel/locking/lockdep.c:4686 > > __kernfs_create_file+0x7c/0xfc > > DEBUG_LOCKS_WARN_ON(1) > > Modules linked in: > > CPU: 2 PID: 1 Comm: swapper/0 Not tainted 5.12.0-rc2-00153-g15097c7a1adc > > #2828 > > Hardware name: Samsung Exynos (Flattened Device Tree) > > [<c011170c>] (unwind_backtrace) from [<c010cf74>] (show_stack+0x10/0x14) > > [<c010cf74>] (show_stack) from [<c0b48650>] (dump_stack+0xa4/0xc4) > > [<c0b48650>] (dump_stack) from [<c01274c8>] (__warn+0x118/0x11c) > > [<c01274c8>] (__warn) from [<c0127544>] (warn_slowpath_fmt+0x78/0xbc) > > [<c0127544>] (warn_slowpath_fmt) from [<c038db90>] > > (__kernfs_create_file+0x7c/0xfc) > > [<c038db90>] (__kernfs_create_file) from [<c038e870>] > > (sysfs_add_file_mode_ns+0xa0/0x1cc) > > [<c038e870>] (sysfs_add_file_mode_ns) from [<c038f4e0>] > > (internal_create_group+0x138/0x3f4) > > [<c038f4e0>] (internal_create_group) from [<c038fd64>] > > (internal_create_groups+0x48/0x88) > > [<c038fd64>] (internal_create_groups) from [<c06a8754>] > > (device_add+0x2e4/0x7ec) > > [<c06a8754>] (device_add) from [<c02e46a0>] (cdev_device_add+0x48/0x80) > > [<c02e46a0>] (cdev_device_add) from [<c08db1a4>] > > (__iio_device_register+0x670/0x7c0) > > [<c08db1a4>] (__iio_device_register) from [<c08e2efc>] > > (ak8975_probe+0x3a4/0x584) > > [<c08e2efc>] (ak8975_probe) from [<c082e120>] (i2c_device_probe+0x234/0x2a4) > > [<c082e120>] (i2c_device_probe) from [<c06abea4>] (really_probe+0x1d4/0x4ec) > > [<c06abea4>] (really_probe) from [<c06ac234>] > > (driver_probe_device+0x78/0x1d8) > > [<c06ac234>] (driver_probe_device) from [<c06ac74c>] > > (device_driver_attach+0x58/0x60) > > [<c06ac74c>] (device_driver_attach) from [<c06ac850>] > > (__driver_attach+0xfc/0x160) > > [<c06ac850>] (__driver_attach) from [<c06a9e58>] > > (bus_for_each_dev+0x6c/0xb8) > > [<c06a9e58>] (bus_for_each_dev) from [<c06aaf90>] > > (bus_add_driver+0x170/0x20c) > > [<c06aaf90>] (bus_add_driver) from [<c06ad6c8>] (driver_register+0x78/0x10c) > > [<c06ad6c8>] (driver_register) from [<c082f0d4>] > > (i2c_register_driver+0x3c/0xac) > > [<c082f0d4>] (i2c_register_driver) from [<c0102434>] > > (do_one_initcall+0x88/0x430) > > [<c0102434>] (do_one_initcall) from [<c11010d4>] > > (kernel_init_freeable+0x190/0x1e0) > > [<c11010d4>] (kernel_init_freeable) from [<c0b4c564>] > > (kernel_init+0x8/0x118) > > [<c0b4c564>] (kernel_init) from [<c010011c>] (ret_from_fork+0x14/0x38) > > Exception stack(0xc1d09fb0 to 0xc1d09ff8) > > 9fa0: 00000000 00000000 00000000 > > 00000000 > > 9fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 > > 00000000 > > 9fe0: 00000000 00000000 00000000 00000000 00000013 00000000 > > irq event stamp: 485619 > > hardirqs last enabled at (485619): [<c01a51fc>] console_unlock+0x500/0x648 > > hardirqs last disabled at (485618): [<c01a51e0>] console_unlock+0x4e4/0x648 > > softirqs last enabled at (484774): [<c0101790>] __do_softirq+0x528/0x63c > > softirqs last disabled at (484769): [<c0130a38>] irq_exit+0x1f4/0x1fc > > ---[ end trace 21850020cbfb3350 ]--- > > > > The above warning is probably caused by copying struct device_attribute > > objects in iio_buffer_wrap_attr() without re-initializing > > spinlocks/mutexes. Locks debuging depends on the spinlock/mutex > > initializers, which register them (as absolute value of the pointer to > > them) to the debugging engine. After copying the structures, the objects > > don't match the pointers they were registered. I didn't have time to > > analyze it further and find which object/lock is triggering this though. > > Thanks, interestingly there aren't any spinlocks or mutexes in attributes, > but there are lock_class_keys. Looks like we need to call sysfs_attr_init() > and I guess we are currently missing it on this path. > > Alex, can you take a look? If not I'll get to it sometime over the weekend > most likely. Will take a look. > > Jonathan > > > > > > > --- > > > drivers/iio/industrialio-buffer.c | 76 ++++++++++++++++++++----------- > > > include/linux/iio/buffer_impl.h | 4 +- > > > 2 files changed, 52 insertions(+), 28 deletions(-) > > > > > > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c > > > index e6edec3bcb73..8dc140f13b99 100644 > > > --- a/drivers/iio/industrialio-buffer.c > > > +++ b/drivers/iio/industrialio-buffer.c > > > @@ -253,8 +253,7 @@ static ssize_t iio_scan_el_show(struct device *dev, > > > char *buf) > > > { > > > int ret; > > > - struct iio_dev *indio_dev = dev_to_iio_dev(dev); > > > - struct iio_buffer *buffer = indio_dev->buffer; > > > + struct iio_buffer *buffer = to_iio_dev_attr(attr)->buffer; > > > > > > /* Ensure ret is 0 or 1. */ > > > ret = !!test_bit(to_iio_dev_attr(attr)->address, > > > @@ -367,8 +366,8 @@ static ssize_t iio_scan_el_store(struct device *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); > > > + struct iio_buffer *buffer = this_attr->buffer; > > > > > > ret = strtobool(buf, &state); > > > if (ret < 0) > > > @@ -402,8 +401,7 @@ static ssize_t iio_scan_el_ts_show(struct device *dev, > > > struct device_attribute *attr, > > > char *buf) > > > { > > > - struct iio_dev *indio_dev = dev_to_iio_dev(dev); > > > - struct iio_buffer *buffer = indio_dev->buffer; > > > + struct iio_buffer *buffer = to_iio_dev_attr(attr)->buffer; > > > > > > return sprintf(buf, "%d\n", buffer->scan_timestamp); > > > } > > > @@ -415,7 +413,7 @@ static ssize_t iio_scan_el_ts_store(struct device *dev, > > > { > > > int ret; > > > struct iio_dev *indio_dev = dev_to_iio_dev(dev); > > > - struct iio_buffer *buffer = indio_dev->buffer; > > > + struct iio_buffer *buffer = to_iio_dev_attr(attr)->buffer; > > > bool state; > > > > > > ret = strtobool(buf, &state); > > > @@ -448,7 +446,7 @@ static int iio_buffer_add_channel_sysfs(struct iio_dev *indio_dev, > > > IIO_SEPARATE, > > > &indio_dev->dev, > > > buffer, > > > - &buffer->scan_el_dev_attr_list); > > > + &buffer->buffer_attr_list); > > > if (ret) > > > return ret; > > > attrcount++; > > > @@ -460,7 +458,7 @@ static int iio_buffer_add_channel_sysfs(struct iio_dev *indio_dev, > > > 0, > > > &indio_dev->dev, > > > buffer, > > > - &buffer->scan_el_dev_attr_list); > > > + &buffer->buffer_attr_list); > > > if (ret) > > > return ret; > > > attrcount++; > > > @@ -473,7 +471,7 @@ static int iio_buffer_add_channel_sysfs(struct iio_dev *indio_dev, > > > 0, > > > &indio_dev->dev, > > > buffer, > > > - &buffer->scan_el_dev_attr_list); > > > + &buffer->buffer_attr_list); > > > else > > > ret = __iio_add_chan_devattr("en", > > > chan, > > > @@ -483,7 +481,7 @@ static int iio_buffer_add_channel_sysfs(struct iio_dev *indio_dev, > > > 0, > > > &indio_dev->dev, > > > buffer, > > > - &buffer->scan_el_dev_attr_list); > > > + &buffer->buffer_attr_list); > > > if (ret) > > > return ret; > > > attrcount++; > > > @@ -495,8 +493,7 @@ static ssize_t iio_buffer_read_length(struct device *dev, > > > struct device_attribute *attr, > > > char *buf) > > > { > > > - struct iio_dev *indio_dev = dev_to_iio_dev(dev); > > > - struct iio_buffer *buffer = indio_dev->buffer; > > > + struct iio_buffer *buffer = to_iio_dev_attr(attr)->buffer; > > > > > > return sprintf(buf, "%d\n", buffer->length); > > > } > > > @@ -506,7 +503,7 @@ static ssize_t iio_buffer_write_length(struct device *dev, > > > 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_buffer *buffer = to_iio_dev_attr(attr)->buffer; > > > unsigned int val; > > > int ret; > > > > > > @@ -538,8 +535,7 @@ static ssize_t iio_buffer_show_enable(struct device *dev, > > > struct device_attribute *attr, > > > char *buf) > > > { > > > - struct iio_dev *indio_dev = dev_to_iio_dev(dev); > > > - struct iio_buffer *buffer = indio_dev->buffer; > > > + struct iio_buffer *buffer = to_iio_dev_attr(attr)->buffer; > > > > > > return sprintf(buf, "%d\n", iio_buffer_is_active(buffer)); > > > } > > > @@ -1154,7 +1150,7 @@ static ssize_t iio_buffer_store_enable(struct device *dev, > > > int ret; > > > bool requested_state; > > > struct iio_dev *indio_dev = dev_to_iio_dev(dev); > > > - struct iio_buffer *buffer = indio_dev->buffer; > > > + struct iio_buffer *buffer = to_iio_dev_attr(attr)->buffer; > > > bool inlist; > > > > > > ret = strtobool(buf, &requested_state); > > > @@ -1183,8 +1179,7 @@ static ssize_t iio_buffer_show_watermark(struct device *dev, > > > struct device_attribute *attr, > > > char *buf) > > > { > > > - struct iio_dev *indio_dev = dev_to_iio_dev(dev); > > > - struct iio_buffer *buffer = indio_dev->buffer; > > > + struct iio_buffer *buffer = to_iio_dev_attr(attr)->buffer; > > > > > > return sprintf(buf, "%u\n", buffer->watermark); > > > } > > > @@ -1195,7 +1190,7 @@ static ssize_t iio_buffer_store_watermark(struct device *dev, > > > size_t len) > > > { > > > struct iio_dev *indio_dev = dev_to_iio_dev(dev); > > > - struct iio_buffer *buffer = indio_dev->buffer; > > > + struct iio_buffer *buffer = to_iio_dev_attr(attr)->buffer; > > > unsigned int val; > > > int ret; > > > > > > @@ -1228,8 +1223,7 @@ static ssize_t iio_dma_show_data_available(struct device *dev, > > > struct device_attribute *attr, > > > char *buf) > > > { > > > - struct iio_dev *indio_dev = dev_to_iio_dev(dev); > > > - struct iio_buffer *buffer = indio_dev->buffer; > > > + struct iio_buffer *buffer = to_iio_dev_attr(attr)->buffer; > > > > > > return sprintf(buf, "%zu\n", iio_buffer_data_available(buffer)); > > > } > > > @@ -1254,6 +1248,26 @@ static struct attribute *iio_buffer_attrs[] = { > > > &dev_attr_data_available.attr, > > > }; > > > > > > +#define to_dev_attr(_attr) container_of(_attr, struct device_attribute, attr) > > > + > > > +static struct attribute *iio_buffer_wrap_attr(struct iio_buffer *buffer, > > > + struct attribute *attr) > > > +{ > > > + struct device_attribute *dattr = to_dev_attr(attr); > > > + struct iio_dev_attr *iio_attr; > > > + > > > + iio_attr = kzalloc(sizeof(*iio_attr), GFP_KERNEL); > > > + if (!iio_attr) > > > + return NULL; > > > + > > > + iio_attr->buffer = buffer; > > > + memcpy(&iio_attr->dev_attr, dattr, sizeof(iio_attr->dev_attr)); > > > + > > > + list_add(&iio_attr->l, &buffer->buffer_attr_list); > > > + > > > + return &iio_attr->dev_attr.attr; > > > +} > > > + > > > static int iio_buffer_register_legacy_sysfs_groups(struct iio_dev *indio_dev, > > > struct attribute **buffer_attrs, > > > int buffer_attrcount, > > > @@ -1329,7 +1343,7 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer, > > > } > > > > > > scan_el_attrcount = 0; > > > - INIT_LIST_HEAD(&buffer->scan_el_dev_attr_list); > > > + INIT_LIST_HEAD(&buffer->buffer_attr_list); > > > channels = indio_dev->channels; > > > if (channels) { > > > /* new magic */ > > > @@ -1376,9 +1390,19 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer, > > > > > > buffer_attrcount += ARRAY_SIZE(iio_buffer_attrs); > > > > > > - attrn = buffer_attrcount; > > > + for (i = 0; i < buffer_attrcount; i++) { > > > + struct attribute *wrapped; > > > + > > > + wrapped = iio_buffer_wrap_attr(buffer, attr[i]); > > > + if (!wrapped) { > > > + ret = -ENOMEM; > > > + goto error_free_scan_mask; > > > + } > > > + attr[i] = wrapped; > > > + } > > > > > > - list_for_each_entry(p, &buffer->scan_el_dev_attr_list, l) > > > + attrn = 0; > > > + list_for_each_entry(p, &buffer->buffer_attr_list, l) > > > attr[attrn++] = &p->dev_attr.attr; > > > > > > buffer->buffer_group.name = kasprintf(GFP_KERNEL, "buffer%d", index); > > > @@ -1412,7 +1436,7 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer, > > > error_free_scan_mask: > > > bitmap_free(buffer->scan_mask); > > > error_cleanup_dynamic: > > > - iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list); > > > + iio_free_chan_devattr_list(&buffer->buffer_attr_list); > > > > > > return ret; > > > } > > > @@ -1443,7 +1467,7 @@ static void __iio_buffer_free_sysfs_and_mask(struct iio_buffer *buffer) > > > bitmap_free(buffer->scan_mask); > > > kfree(buffer->buffer_group.name); > > > kfree(buffer->buffer_group.attrs); > > > - iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list); > > > + iio_free_chan_devattr_list(&buffer->buffer_attr_list); > > > } > > > > > > void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev) > > > diff --git a/include/linux/iio/buffer_impl.h b/include/linux/iio/buffer_impl.h > > > index 3e555e58475b..41044320e581 100644 > > > --- a/include/linux/iio/buffer_impl.h > > > +++ b/include/linux/iio/buffer_impl.h > > > @@ -97,8 +97,8 @@ struct iio_buffer { > > > /* @scan_timestamp: Does the scan mode include a timestamp. */ > > > bool scan_timestamp; > > > > > > - /* @scan_el_dev_attr_list: List of scan element related attributes. */ > > > - struct list_head scan_el_dev_attr_list; > > > + /* @buffer_attr_list: List of buffer attributes. */ > > > + struct list_head buffer_attr_list; > > > > > > /* > > > * @buffer_group: Attributes of the new buffer group. > > > > Best regards >
Hi dee Ho peeps! Disclaimer - I have no HW to test this using real in-tree drivers. If someone has a device with a variant of bmc150 or adxl372 or - it'd be nice to see if reading hwfifo_watermark_max or hwfifo_watermark_min works with the v6.0-rc4. Maybe I am misreading code and have my own issues - in which case I apologize already now and go to the corner while being deeply ashamed :) On 2/15/21 12:40, Alexandru Ardelean wrote: > This change wraps all buffer attributes into iio_dev_attr objects, and > assigns a reference to the IIO buffer they belong to. > > With the addition of multiple IIO buffers per one IIO device, we need a way > to know which IIO buffer is being enabled/disabled/controlled. > > We know that all buffer attributes are device_attributes. I think this assumption is slightly unsafe. I see few drivers adding IIO_CONST_ATTRs in attribute groups. For example the bmc150 and adxl372 add the hwfifo_watermark_min and hwfifo_watermark_max. Long story short (or the other way around?) I was developing a driver for ROHM/Kionix KX022A accelerometer. I did a random pick and chose the bmc150-core as a reference how others have implemented the accel drivers. During the testing I noticed that using IIO_CONST_ATTRs for triggered buffers seem to cause access to somewhere it shouldn't... Oops. Reading the code allows me to assume the problem is wrapping the attributes to IIO_DEV_ATTRs. static struct attribute *iio_buffer_wrap_attr(struct iio_buffer *buffer, + struct attribute *attr) +{ + struct device_attribute *dattr = to_dev_attr(attr); + struct iio_dev_attr *iio_attr; + + iio_attr = kzalloc(sizeof(*iio_attr), GFP_KERNEL); + if (!iio_attr) + return NULL; + + iio_attr->buffer = buffer; + memcpy(&iio_attr->dev_attr, dattr, sizeof(iio_attr->dev_attr)); This copy does assume all attributes are device_attrs, and does not take into account that IIO_CONST_ATTRS have the string stored in a struct iio_const_attr which is containing the dev_attr. Eg, copying in the iio_buffer_wrap_attr() does not copy the string - and later invoking the 'show' callback goes reading something else than the mentioned string because the pointer is not copied. I believe problem has been there for a while now - introduced by this: 15097c7a1adc ("iio: buffer: wrap all buffer attributes into iio_dev_attr") at Feb 2021. I don't have a fix for you as I am not sure what would be the correct fix. We _could_ go through all the IIO drivers and ensure none used IIO_CONST_ATTRs for triggered buffers. This would IMHO be just a workaround to the real problem - which is assumption that all attributes in the attribute group are dev_attrs. Such silent assumptions are fragile as we see :) If we opt treating all attributes in the group as device_attrs also in the future, then (in my not always so humble opinion) the APIs should force this - but I'm not sure how to do that. Best regards -- Matti
On 9/9/22 11:12, Vaittinen, Matti wrote: > Hi dee Ho peeps! > > Disclaimer - I have no HW to test this using real in-tree drivers. If > someone has a device with a variant of bmc150 or adxl372 or - it'd be > nice to see if reading hwfifo_watermark_max or hwfifo_watermark_min > works with the v6.0-rc4. Maybe I am misreading code and have my own > issues - in which case I apologize already now and go to the corner > while being deeply ashamed :) I would like to add at least the at91-sama5d2_adc (conditonally registers the IIO_CONST_ATTR for triggered-buffer) to the list of devices that could be potentially tested. I hope some of these devices had a user who could either make us worried and verify my assumption - or make me ashamed but rest of us relieved :) Eg - I second my request for testing this - and add potential owners of at91-sama5d2_adc to the list. > On 2/15/21 12:40, Alexandru Ardelean wrote: >> This change wraps all buffer attributes into iio_dev_attr objects, and >> assigns a reference to the IIO buffer they belong to. >> >> With the addition of multiple IIO buffers per one IIO device, we need a way >> to know which IIO buffer is being enabled/disabled/controlled. >> >> We know that all buffer attributes are device_attributes. > > I think this assumption is slightly unsafe. I see few drivers adding > IIO_CONST_ATTRs in attribute groups. For example the bmc150 and adxl372 > add the hwfifo_watermark_min and hwfifo_watermark_max. > and at91-sama5d2_adc //snip >I noticed that using > IIO_CONST_ATTRs for triggered buffers seem to cause access to somewhere > it shouldn't... Oops. > > Reading the code allows me to assume the problem is wrapping the > attributes to IIO_DEV_ATTRs. > > static struct attribute *iio_buffer_wrap_attr(struct iio_buffer *buffer, > + struct attribute *attr) > +{ > + struct device_attribute *dattr = to_dev_attr(attr); > + struct iio_dev_attr *iio_attr; > + > + iio_attr = kzalloc(sizeof(*iio_attr), GFP_KERNEL); > + if (!iio_attr) > + return NULL; > + > + iio_attr->buffer = buffer; > + memcpy(&iio_attr->dev_attr, dattr, sizeof(iio_attr->dev_attr)); > > This copy does assume all attributes are device_attrs, and does not take > into account that IIO_CONST_ATTRS have the string stored in a struct > iio_const_attr which is containing the dev_attr. Eg, copying in the > iio_buffer_wrap_attr() does not copy the string - and later invoking the > 'show' callback goes reading something else than the mentioned string > because the pointer is not copied. Yours, -- Matti
On Mon, 19 Sep 2022 08:52:38 +0000 "Vaittinen, Matti" <Matti.Vaittinen@fi.rohmeurope.com> wrote: > On 9/9/22 11:12, Vaittinen, Matti wrote: > > Hi dee Ho peeps! > > > > Disclaimer - I have no HW to test this using real in-tree drivers. If > > someone has a device with a variant of bmc150 or adxl372 or - it'd be > > nice to see if reading hwfifo_watermark_max or hwfifo_watermark_min > > works with the v6.0-rc4. Maybe I am misreading code and have my own > > issues - in which case I apologize already now and go to the corner > > while being deeply ashamed :) > > I would like to add at least the at91-sama5d2_adc (conditonally > registers the IIO_CONST_ATTR for triggered-buffer) to the list of > devices that could be potentially tested. I hope some of these devices > had a user who could either make us worried and verify my assumption - > or make me ashamed but rest of us relieved :) Eg - I second my request > for testing this - and add potential owners of at91-sama5d2_adc to the list. > > > On 2/15/21 12:40, Alexandru Ardelean wrote: > >> This change wraps all buffer attributes into iio_dev_attr objects, and > >> assigns a reference to the IIO buffer they belong to. > >> > >> With the addition of multiple IIO buffers per one IIO device, we need a way > >> to know which IIO buffer is being enabled/disabled/controlled. > >> > >> We know that all buffer attributes are device_attributes. > > > > I think this assumption is slightly unsafe. I see few drivers adding > > IIO_CONST_ATTRs in attribute groups. For example the bmc150 and adxl372 > > add the hwfifo_watermark_min and hwfifo_watermark_max. > > > > and at91-sama5d2_adc > > //snip > > >I noticed that using > > IIO_CONST_ATTRs for triggered buffers seem to cause access to somewhere > > it shouldn't... Oops. > > > > Reading the code allows me to assume the problem is wrapping the > > attributes to IIO_DEV_ATTRs. > > > > static struct attribute *iio_buffer_wrap_attr(struct iio_buffer *buffer, > > + struct attribute *attr) > > +{ > > + struct device_attribute *dattr = to_dev_attr(attr); > > + struct iio_dev_attr *iio_attr; > > + > > + iio_attr = kzalloc(sizeof(*iio_attr), GFP_KERNEL); > > + if (!iio_attr) > > + return NULL; > > + > > + iio_attr->buffer = buffer; > > + memcpy(&iio_attr->dev_attr, dattr, sizeof(iio_attr->dev_attr)); > > > > This copy does assume all attributes are device_attrs, and does not take > > into account that IIO_CONST_ATTRS have the string stored in a struct > > iio_const_attr which is containing the dev_attr. Eg, copying in the > > iio_buffer_wrap_attr() does not copy the string - and later invoking the > > 'show' callback goes reading something else than the mentioned string > > because the pointer is not copied. > > Yours, > -- Matti Hi Matti, +CC Alexandru on a current email address. I saw this whilst travelling and completely forgot about when I was back to normal - so great you sent a follow up! Anyhow, your reasoning seems correct and it would be easy enough to add such a case to iio/dummy/iio_simple_dummy_buffer.c and provide a clear test for the problem. As to solutions. The quickest is probably to switch these const attrs over to a non const form and add a comment to the header to say they are unsuitable for use with buffers. An alternative would be to make it 'safe' by making the data layouts match up. struct iio_attr { struct device_attribute dev_attr; union { u64 address; const char *string; }; struct list_head l; struct iio_chan_spec const *c; struct iio_buffer *buffer; }; #define iio_dev_attr iio_attr #define iio_const_attr iio_attr Looking at this raises another potential problem. Where is the address copied over for attributes using IIO_DEVICE_ATTR()? Maybe I'm just missing it somewhere. Grepping suggests we've been lucky and there are no users of that field in buffer attributes. Detecting the problem you found is going to be inherently tricky - though maybe could rely on the naming of the attributes passed in (iio_const...) and some scripting magic. Longer term, it's this sort of thing that motivates protections / runnable CI self tests with, for example, the roadtest framework that I'm hoping will be available upstream soonish! Would you like to send patches given you identified the problem? If not I'm happy to fix these up. My grepping identified the same 3 cases you found. Jonathan
On Mon, 19 Sep 2022 16:32:14 +0100 Jonathan Cameron <jic23@kernel.org> wrote: > On Mon, 19 Sep 2022 08:52:38 +0000 > "Vaittinen, Matti" <Matti.Vaittinen@fi.rohmeurope.com> wrote: > > > On 9/9/22 11:12, Vaittinen, Matti wrote: > > > Hi dee Ho peeps! > > > > > > Disclaimer - I have no HW to test this using real in-tree drivers. If > > > someone has a device with a variant of bmc150 or adxl372 or - it'd be > > > nice to see if reading hwfifo_watermark_max or hwfifo_watermark_min > > > works with the v6.0-rc4. Maybe I am misreading code and have my own > > > issues - in which case I apologize already now and go to the corner > > > while being deeply ashamed :) > > > > I would like to add at least the at91-sama5d2_adc (conditonally > > registers the IIO_CONST_ATTR for triggered-buffer) to the list of > > devices that could be potentially tested. I hope some of these devices > > had a user who could either make us worried and verify my assumption - > > or make me ashamed but rest of us relieved :) Eg - I second my request > > for testing this - and add potential owners of at91-sama5d2_adc to the list. > > > > > On 2/15/21 12:40, Alexandru Ardelean wrote: > > >> This change wraps all buffer attributes into iio_dev_attr objects, and > > >> assigns a reference to the IIO buffer they belong to. > > >> > > >> With the addition of multiple IIO buffers per one IIO device, we need a way > > >> to know which IIO buffer is being enabled/disabled/controlled. > > >> > > >> We know that all buffer attributes are device_attributes. > > > > > > I think this assumption is slightly unsafe. I see few drivers adding > > > IIO_CONST_ATTRs in attribute groups. For example the bmc150 and adxl372 > > > add the hwfifo_watermark_min and hwfifo_watermark_max. > > > > > > > and at91-sama5d2_adc > > > > //snip > > > > >I noticed that using > > > IIO_CONST_ATTRs for triggered buffers seem to cause access to somewhere > > > it shouldn't... Oops. > > > > > > Reading the code allows me to assume the problem is wrapping the > > > attributes to IIO_DEV_ATTRs. > > > > > > static struct attribute *iio_buffer_wrap_attr(struct iio_buffer *buffer, > > > + struct attribute *attr) > > > +{ > > > + struct device_attribute *dattr = to_dev_attr(attr); > > > + struct iio_dev_attr *iio_attr; > > > + > > > + iio_attr = kzalloc(sizeof(*iio_attr), GFP_KERNEL); > > > + if (!iio_attr) > > > + return NULL; > > > + > > > + iio_attr->buffer = buffer; > > > + memcpy(&iio_attr->dev_attr, dattr, sizeof(iio_attr->dev_attr)); > > > > > > This copy does assume all attributes are device_attrs, and does not take > > > into account that IIO_CONST_ATTRS have the string stored in a struct > > > iio_const_attr which is containing the dev_attr. Eg, copying in the > > > iio_buffer_wrap_attr() does not copy the string - and later invoking the > > > 'show' callback goes reading something else than the mentioned string > > > because the pointer is not copied. > > > > Yours, > > -- Matti > Hi Matti, > > +CC Alexandru on a current email address. > > I saw this whilst travelling and completely forgot about when > I was back to normal - so great you sent a follow up! > > Anyhow, your reasoning seems correct and it would be easy enough > to add such a case to iio/dummy/iio_simple_dummy_buffer.c and > provide a clear test for the problem. > > As to solutions. The quickest is probably to switch these const attrs > over to a non const form and add a comment to the header to say they are > unsuitable for use with buffers. Thinking a little more on this - all / (most?) of the users pass a null terminated array of struct device_attribute * to *iio_triggered_buffer_setup_ext() That's then assigned to buffer->attrs. We could add an additional pointer to the struct iio_buffer to take a null terminated array of struct iio_dev_attr * and change the signature of that function to take one of those, thus preventing us using iio_const_attr structures for this. Then we can wrap those just fine in the code you highlighted and assign the result into buffer->attrs. We'd need to precede that change with fixes that just switch the iio_const_attr uses over to iio_dev_attr but changing this would ensure no accidental reintroductions of the problem in future drivers (typically as a result of someone forward porting a driver that is out of tree). I think this combination of fix then prevent future problems is what I would prefer. Jonathan > > An alternative would be to make it 'safe' by making the data layouts > match up. > > struct iio_attr { > struct device_attribute dev_attr; > union { > u64 address; > const char *string; > }; > struct list_head l; > struct iio_chan_spec const *c; > struct iio_buffer *buffer; > }; > > #define iio_dev_attr iio_attr > #define iio_const_attr iio_attr > > Looking at this raises another potential problem. > Where is the address copied over for attributes using IIO_DEVICE_ATTR()? > Maybe I'm just missing it somewhere. Grepping suggests we've been > lucky and there are no users of that field in buffer attributes. > > Detecting the problem you found is going to be inherently tricky - though maybe > could rely on the naming of the attributes passed in (iio_const...) > and some scripting magic. > > Longer term, it's this sort of thing that motivates protections / runnable > CI self tests with, for example, the roadtest framework that I'm hoping > will be available upstream soonish! > > Would you like to send patches given you identified the problem? > > If not I'm happy to fix these up. My grepping identified the same 3 cases > you found. > > Jonathan >
On 9/19/22 20:18, Jonathan Cameron wrote: > On Mon, 19 Sep 2022 16:32:14 +0100 > Jonathan Cameron <jic23@kernel.org> wrote: > >> On Mon, 19 Sep 2022 08:52:38 +0000 >> "Vaittinen, Matti" <Matti.Vaittinen@fi.rohmeurope.com> wrote: >> >>> On 9/9/22 11:12, Vaittinen, Matti wrote: >>>> Hi dee Ho peeps! >>>> >>>> Disclaimer - I have no HW to test this using real in-tree drivers. If >>>> someone has a device with a variant of bmc150 or adxl372 or - it'd be >>>> nice to see if reading hwfifo_watermark_max or hwfifo_watermark_min >>>> works with the v6.0-rc4. Maybe I am misreading code and have my own >>>> issues - in which case I apologize already now and go to the corner >>>> while being deeply ashamed :) >>> >>> I would like to add at least the at91-sama5d2_adc (conditonally >>> registers the IIO_CONST_ATTR for triggered-buffer) to the list of >>> devices that could be potentially tested. I hope some of these devices >>> had a user who could either make us worried and verify my assumption - >>> or make me ashamed but rest of us relieved :) Eg - I second my request >>> for testing this - and add potential owners of at91-sama5d2_adc to the list. >>> >>>> On 2/15/21 12:40, Alexandru Ardelean wrote: >>>>> This change wraps all buffer attributes into iio_dev_attr objects, and >>>>> assigns a reference to the IIO buffer they belong to. >>>>> >>>>> With the addition of multiple IIO buffers per one IIO device, we need a way >>>>> to know which IIO buffer is being enabled/disabled/controlled. >>>>> >>>>> We know that all buffer attributes are device_attributes. >>>> >>>> I think this assumption is slightly unsafe. I see few drivers adding >>>> IIO_CONST_ATTRs in attribute groups. For example the bmc150 and adxl372 >>>> add the hwfifo_watermark_min and hwfifo_watermark_max. >>>> >>> >>> and at91-sama5d2_adc >>> >>> //snip >>> >>>> I noticed that using >>>> IIO_CONST_ATTRs for triggered buffers seem to cause access to somewhere >>>> it shouldn't... Oops. >>>> >>>> Reading the code allows me to assume the problem is wrapping the >>>> attributes to IIO_DEV_ATTRs. >>>> >>>> static struct attribute *iio_buffer_wrap_attr(struct iio_buffer *buffer, >>>> + struct attribute *attr) >>>> +{ >>>> + struct device_attribute *dattr = to_dev_attr(attr); >>>> + struct iio_dev_attr *iio_attr; >>>> + >>>> + iio_attr = kzalloc(sizeof(*iio_attr), GFP_KERNEL); >>>> + if (!iio_attr) >>>> + return NULL; >>>> + >>>> + iio_attr->buffer = buffer; >>>> + memcpy(&iio_attr->dev_attr, dattr, sizeof(iio_attr->dev_attr)); >>>> >>>> This copy does assume all attributes are device_attrs, and does not take >>>> into account that IIO_CONST_ATTRS have the string stored in a struct >>>> iio_const_attr which is containing the dev_attr. Eg, copying in the >>>> iio_buffer_wrap_attr() does not copy the string - and later invoking the >>>> 'show' callback goes reading something else than the mentioned string >>>> because the pointer is not copied. >>> >>> Yours, >>> -- Matti >> Hi Matti, >> >> +CC Alexandru on a current email address. >> >> I saw this whilst travelling and completely forgot about when >> I was back to normal - so great you sent a follow up! I was also participating at ELCE last week so didn't do much of emails/code. >> >> Anyhow, your reasoning seems correct and it would be easy enough >> to add such a case to iio/dummy/iio_simple_dummy_buffer.c and >> provide a clear test for the problem. >> >> As to solutions. The quickest is probably to switch these const attrs >> over to a non const form and add a comment to the header to say they are >> unsuitable for use with buffers. > > Thinking a little more on this - all / (most?) of the users pass a null terminated > array of struct device_attribute * to *iio_triggered_buffer_setup_ext() > > That's then assigned to buffer->attrs. > We could add an additional pointer to the struct iio_buffer to take > a null terminated array of struct iio_dev_attr * > and change the signature of that function to take one of those, thus > preventing us using iio_const_attr structures for this. Yes. I would also rather see pointer to array of struct iio_dev_attr * if we continue keeping the assumption that attrs are of type iio_dev_attr. > > Then we can wrap those just fine in the code you highlighted and assign the > result into buffer->attrs. > > We'd need to precede that change with fixes that just switch the > iio_const_attr uses over to iio_dev_attr but changing this would ensure no > accidental reintroductions of the problem in future drivers (typically > as a result of someone forward porting a driver that is out of tree). Again I do agree. Besides change of const_attrs is necessary in any case if we don't change the wrapping. >> >> Would you like to send patches given you identified the problem? I am in any case about to send couple of patches to IIO. The devm-helper usage (v2 - I sent v1 from my other email address (mazziesaccount) - but I am the same person :] ) and a new accelerometer driver. So, I can look also at this change while I am at it if you're busy). >> If not I'm happy to fix these up. My grepping identified the same 3 cases >> you found. Feel free to patch this if you wish. Just please let me know if you take care of this so we don't do double the work :) Yours -- Matti
On Mon, 19 Sep 2022 18:06:37 +0000 "Vaittinen, Matti" <Matti.Vaittinen@fi.rohmeurope.com> wrote: > On 9/19/22 20:18, Jonathan Cameron wrote: > > On Mon, 19 Sep 2022 16:32:14 +0100 > > Jonathan Cameron <jic23@kernel.org> wrote: > > > >> On Mon, 19 Sep 2022 08:52:38 +0000 > >> "Vaittinen, Matti" <Matti.Vaittinen@fi.rohmeurope.com> wrote: > >> > >>> On 9/9/22 11:12, Vaittinen, Matti wrote: > >>>> Hi dee Ho peeps! > >>>> > >>>> Disclaimer - I have no HW to test this using real in-tree drivers. If > >>>> someone has a device with a variant of bmc150 or adxl372 or - it'd be > >>>> nice to see if reading hwfifo_watermark_max or hwfifo_watermark_min > >>>> works with the v6.0-rc4. Maybe I am misreading code and have my own > >>>> issues - in which case I apologize already now and go to the corner > >>>> while being deeply ashamed :) > >>> > >>> I would like to add at least the at91-sama5d2_adc (conditonally > >>> registers the IIO_CONST_ATTR for triggered-buffer) to the list of > >>> devices that could be potentially tested. I hope some of these devices > >>> had a user who could either make us worried and verify my assumption - > >>> or make me ashamed but rest of us relieved :) Eg - I second my request > >>> for testing this - and add potential owners of at91-sama5d2_adc to the list. > >>> > >>>> On 2/15/21 12:40, Alexandru Ardelean wrote: > >>>>> This change wraps all buffer attributes into iio_dev_attr objects, and > >>>>> assigns a reference to the IIO buffer they belong to. > >>>>> > >>>>> With the addition of multiple IIO buffers per one IIO device, we need a way > >>>>> to know which IIO buffer is being enabled/disabled/controlled. > >>>>> > >>>>> We know that all buffer attributes are device_attributes. > >>>> > >>>> I think this assumption is slightly unsafe. I see few drivers adding > >>>> IIO_CONST_ATTRs in attribute groups. For example the bmc150 and adxl372 > >>>> add the hwfifo_watermark_min and hwfifo_watermark_max. > >>>> > >>> > >>> and at91-sama5d2_adc > >>> > >>> //snip > >>> > >>>> I noticed that using > >>>> IIO_CONST_ATTRs for triggered buffers seem to cause access to somewhere > >>>> it shouldn't... Oops. > >>>> > >>>> Reading the code allows me to assume the problem is wrapping the > >>>> attributes to IIO_DEV_ATTRs. > >>>> > >>>> static struct attribute *iio_buffer_wrap_attr(struct iio_buffer *buffer, > >>>> + struct attribute *attr) > >>>> +{ > >>>> + struct device_attribute *dattr = to_dev_attr(attr); > >>>> + struct iio_dev_attr *iio_attr; > >>>> + > >>>> + iio_attr = kzalloc(sizeof(*iio_attr), GFP_KERNEL); > >>>> + if (!iio_attr) > >>>> + return NULL; > >>>> + > >>>> + iio_attr->buffer = buffer; > >>>> + memcpy(&iio_attr->dev_attr, dattr, sizeof(iio_attr->dev_attr)); > >>>> > >>>> This copy does assume all attributes are device_attrs, and does not take > >>>> into account that IIO_CONST_ATTRS have the string stored in a struct > >>>> iio_const_attr which is containing the dev_attr. Eg, copying in the > >>>> iio_buffer_wrap_attr() does not copy the string - and later invoking the > >>>> 'show' callback goes reading something else than the mentioned string > >>>> because the pointer is not copied. > >>> > >>> Yours, > >>> -- Matti > >> Hi Matti, > >> > >> +CC Alexandru on a current email address. > >> > >> I saw this whilst travelling and completely forgot about when > >> I was back to normal - so great you sent a follow up! > > I was also participating at ELCE last week so didn't do much of emails/code. > > >> > >> Anyhow, your reasoning seems correct and it would be easy enough > >> to add such a case to iio/dummy/iio_simple_dummy_buffer.c and > >> provide a clear test for the problem. > >> > >> As to solutions. The quickest is probably to switch these const attrs > >> over to a non const form and add a comment to the header to say they are > >> unsuitable for use with buffers. > > > > Thinking a little more on this - all / (most?) of the users pass a null terminated > > array of struct device_attribute * to *iio_triggered_buffer_setup_ext() > > > > That's then assigned to buffer->attrs. > > We could add an additional pointer to the struct iio_buffer to take > > a null terminated array of struct iio_dev_attr * > > and change the signature of that function to take one of those, thus > > preventing us using iio_const_attr structures for this. > > Yes. I would also rather see pointer to array of struct iio_dev_attr * > if we continue keeping the assumption that attrs are of type iio_dev_attr. > > > > > Then we can wrap those just fine in the code you highlighted and assign the > > result into buffer->attrs. > > > > We'd need to precede that change with fixes that just switch the > > iio_const_attr uses over to iio_dev_attr but changing this would ensure no > > accidental reintroductions of the problem in future drivers (typically > > as a result of someone forward porting a driver that is out of tree). > > Again I do agree. Besides change of const_attrs is necessary in any case > if we don't change the wrapping. > > >> > >> Would you like to send patches given you identified the problem? > > I am in any case about to send couple of patches to IIO. The devm-helper > usage (v2 - I sent v1 from my other email address (mazziesaccount) - but > I am the same person :] ) and a new accelerometer driver. So, I can look > also at this change while I am at it if you're busy). > > >> If not I'm happy to fix these up. My grepping identified the same 3 cases > >> you found. > > Feel free to patch this if you wish. Just please let me know if you take > care of this so we don't do double the work :) I'm never one to turn down a volunteer, so I'll leave these for you :) Plenty of other things on the todo list that I can be getting on with. Jonathan > > Yours > -- Matti > >
On Sat, Sep 24, 2022 at 4:49 PM Jonathan Cameron <jic23@kernel.org> wrote: > > On Mon, 19 Sep 2022 18:06:37 +0000 > "Vaittinen, Matti" <Matti.Vaittinen@fi.rohmeurope.com> wrote: > > > On 9/19/22 20:18, Jonathan Cameron wrote: > > > On Mon, 19 Sep 2022 16:32:14 +0100 > > > Jonathan Cameron <jic23@kernel.org> wrote: > > > > > >> On Mon, 19 Sep 2022 08:52:38 +0000 > > >> "Vaittinen, Matti" <Matti.Vaittinen@fi.rohmeurope.com> wrote: > > >> > > >>> On 9/9/22 11:12, Vaittinen, Matti wrote: > > >>>> Hi dee Ho peeps! > > >>>> > > >>>> Disclaimer - I have no HW to test this using real in-tree drivers. If > > >>>> someone has a device with a variant of bmc150 or adxl372 or - it'd be > > >>>> nice to see if reading hwfifo_watermark_max or hwfifo_watermark_min > > >>>> works with the v6.0-rc4. Maybe I am misreading code and have my own > > >>>> issues - in which case I apologize already now and go to the corner > > >>>> while being deeply ashamed :) > > >>> > > >>> I would like to add at least the at91-sama5d2_adc (conditonally > > >>> registers the IIO_CONST_ATTR for triggered-buffer) to the list of > > >>> devices that could be potentially tested. I hope some of these devices > > >>> had a user who could either make us worried and verify my assumption - > > >>> or make me ashamed but rest of us relieved :) Eg - I second my request > > >>> for testing this - and add potential owners of at91-sama5d2_adc to the list. > > >>> > > >>>> On 2/15/21 12:40, Alexandru Ardelean wrote: > > >>>>> This change wraps all buffer attributes into iio_dev_attr objects, and > > >>>>> assigns a reference to the IIO buffer they belong to. > > >>>>> > > >>>>> With the addition of multiple IIO buffers per one IIO device, we need a way > > >>>>> to know which IIO buffer is being enabled/disabled/controlled. > > >>>>> > > >>>>> We know that all buffer attributes are device_attributes. > > >>>> > > >>>> I think this assumption is slightly unsafe. I see few drivers adding > > >>>> IIO_CONST_ATTRs in attribute groups. For example the bmc150 and adxl372 > > >>>> add the hwfifo_watermark_min and hwfifo_watermark_max. Took me a while to get to this and read in-depth. Yep. Apologies. I omitted the IIO_CONST_ATTRs when I did that change. > > >>>> > > >>> > > >>> and at91-sama5d2_adc > > >>> > > >>> //snip > > >>> > > >>>> I noticed that using > > >>>> IIO_CONST_ATTRs for triggered buffers seem to cause access to somewhere > > >>>> it shouldn't... Oops. > > >>>> > > >>>> Reading the code allows me to assume the problem is wrapping the > > >>>> attributes to IIO_DEV_ATTRs. > > >>>> > > >>>> static struct attribute *iio_buffer_wrap_attr(struct iio_buffer *buffer, > > >>>> + struct attribute *attr) > > >>>> +{ > > >>>> + struct device_attribute *dattr = to_dev_attr(attr); > > >>>> + struct iio_dev_attr *iio_attr; > > >>>> + > > >>>> + iio_attr = kzalloc(sizeof(*iio_attr), GFP_KERNEL); > > >>>> + if (!iio_attr) > > >>>> + return NULL; > > >>>> + > > >>>> + iio_attr->buffer = buffer; > > >>>> + memcpy(&iio_attr->dev_attr, dattr, sizeof(iio_attr->dev_attr)); > > >>>> > > >>>> This copy does assume all attributes are device_attrs, and does not take > > >>>> into account that IIO_CONST_ATTRS have the string stored in a struct > > >>>> iio_const_attr which is containing the dev_attr. Eg, copying in the > > >>>> iio_buffer_wrap_attr() does not copy the string - and later invoking the > > >>>> 'show' callback goes reading something else than the mentioned string > > >>>> because the pointer is not copied. > > >>> > > >>> Yours, > > >>> -- Matti > > >> Hi Matti, > > >> > > >> +CC Alexandru on a current email address. > > >> > > >> I saw this whilst travelling and completely forgot about when > > >> I was back to normal - so great you sent a follow up! > > > > I was also participating at ELCE last week so didn't do much of emails/code. > > > > >> > > >> Anyhow, your reasoning seems correct and it would be easy enough > > >> to add such a case to iio/dummy/iio_simple_dummy_buffer.c and > > >> provide a clear test for the problem. > > >> > > >> As to solutions. The quickest is probably to switch these const attrs > > >> over to a non const form and add a comment to the header to say they are > > >> unsuitable for use with buffers. > > > > > > Thinking a little more on this - all / (most?) of the users pass a null terminated > > > array of struct device_attribute * to *iio_triggered_buffer_setup_ext() > > > > > > That's then assigned to buffer->attrs. > > > We could add an additional pointer to the struct iio_buffer to take > > > a null terminated array of struct iio_dev_attr * > > > and change the signature of that function to take one of those, thus > > > preventing us using iio_const_attr structures for this. > > > > Yes. I would also rather see pointer to array of struct iio_dev_attr * > > if we continue keeping the assumption that attrs are of type iio_dev_attr. > > > > > > > > Then we can wrap those just fine in the code you highlighted and assign the > > > result into buffer->attrs. > > > > > > We'd need to precede that change with fixes that just switch the > > > iio_const_attr uses over to iio_dev_attr but changing this would ensure no > > > accidental reintroductions of the problem in future drivers (typically > > > as a result of someone forward porting a driver that is out of tree). > > > > Again I do agree. Besides change of const_attrs is necessary in any case > > if we don't change the wrapping. > > > > >> > > >> Would you like to send patches given you identified the problem? > > > > I am in any case about to send couple of patches to IIO. The devm-helper > > usage (v2 - I sent v1 from my other email address (mazziesaccount) - but > > I am the same person :] ) and a new accelerometer driver. So, I can look > > also at this change while I am at it if you're busy). > > > > >> If not I'm happy to fix these up. My grepping identified the same 3 cases > > >> you found. > > > > Feel free to patch this if you wish. Just please let me know if you take > > care of this so we don't do double the work :) > > I'm never one to turn down a volunteer, so I'll leave these for you :) > > Plenty of other things on the todo list that I can be getting on with. > > Jonathan > > > > > Yours > > -- Matti > > > > >
On 19.09.2022 11:52, Vaittinen, Matti wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On 9/9/22 11:12, Vaittinen, Matti wrote: >> Hi dee Ho peeps! >> >> Disclaimer - I have no HW to test this using real in-tree drivers. If >> someone has a device with a variant of bmc150 or adxl372 or - it'd be >> nice to see if reading hwfifo_watermark_max or hwfifo_watermark_min >> works with the v6.0-rc4. I've checked it on sama5d2_xplained board on v6.0 and it returns (null) for both hwfifo_watermark_max and hwfifo_watermark_min: # cat hwfifo_watermark_max (null) # cat hwfifo_watermark_min (null) With your series at [1] I have: # cat hwfifo_watermark_max 128 # cat hwfifo_watermark_min 2 Thank you, Claudiu Beznea [1] https://patchwork.kernel.org/project/linux-iio/list/?series=682707 > Maybe I am misreading code and have my own >> issues - in which case I apologize already now and go to the corner >> while being deeply ashamed :) > > I would like to add at least the at91-sama5d2_adc (conditonally > registers the IIO_CONST_ATTR for triggered-buffer) to the list of > devices that could be potentially tested. I hope some of these devices > had a user who could either make us worried and verify my assumption - > or make me ashamed but rest of us relieved :) Eg - I second my request > for testing this - and add potential owners of at91-sama5d2_adc to the list. > >> On 2/15/21 12:40, Alexandru Ardelean wrote: >>> This change wraps all buffer attributes into iio_dev_attr objects, and >>> assigns a reference to the IIO buffer they belong to. >>> >>> With the addition of multiple IIO buffers per one IIO device, we need a way >>> to know which IIO buffer is being enabled/disabled/controlled. >>> >>> We know that all buffer attributes are device_attributes. >> >> I think this assumption is slightly unsafe. I see few drivers adding >> IIO_CONST_ATTRs in attribute groups. For example the bmc150 and adxl372 >> add the hwfifo_watermark_min and hwfifo_watermark_max. >> > > and at91-sama5d2_adc > > //snip > >> I noticed that using >> IIO_CONST_ATTRs for triggered buffers seem to cause access to somewhere >> it shouldn't... Oops. >> >> Reading the code allows me to assume the problem is wrapping the >> attributes to IIO_DEV_ATTRs. >> >> static struct attribute *iio_buffer_wrap_attr(struct iio_buffer *buffer, >> + struct attribute *attr) >> +{ >> + struct device_attribute *dattr = to_dev_attr(attr); >> + struct iio_dev_attr *iio_attr; >> + >> + iio_attr = kzalloc(sizeof(*iio_attr), GFP_KERNEL); >> + if (!iio_attr) >> + return NULL; >> + >> + iio_attr->buffer = buffer; >> + memcpy(&iio_attr->dev_attr, dattr, sizeof(iio_attr->dev_attr)); >> >> This copy does assume all attributes are device_attrs, and does not take >> into account that IIO_CONST_ATTRS have the string stored in a struct >> iio_const_attr which is containing the dev_attr. Eg, copying in the >> iio_buffer_wrap_attr() does not copy the string - and later invoking the >> 'show' callback goes reading something else than the mentioned string >> because the pointer is not copied. > > Yours, > -- Matti > > -- > Matti Vaittinen > Linux kernel developer at ROHM Semiconductors > Oulu Finland > > ~~ When things go utterly wrong vim users can always type :help! ~~
diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c index e6edec3bcb73..8dc140f13b99 100644 --- a/drivers/iio/industrialio-buffer.c +++ b/drivers/iio/industrialio-buffer.c @@ -253,8 +253,7 @@ static ssize_t iio_scan_el_show(struct device *dev, char *buf) { int ret; - struct iio_dev *indio_dev = dev_to_iio_dev(dev); - struct iio_buffer *buffer = indio_dev->buffer; + struct iio_buffer *buffer = to_iio_dev_attr(attr)->buffer; /* Ensure ret is 0 or 1. */ ret = !!test_bit(to_iio_dev_attr(attr)->address, @@ -367,8 +366,8 @@ static ssize_t iio_scan_el_store(struct device *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); + struct iio_buffer *buffer = this_attr->buffer; ret = strtobool(buf, &state); if (ret < 0) @@ -402,8 +401,7 @@ static ssize_t iio_scan_el_ts_show(struct device *dev, struct device_attribute *attr, char *buf) { - struct iio_dev *indio_dev = dev_to_iio_dev(dev); - struct iio_buffer *buffer = indio_dev->buffer; + struct iio_buffer *buffer = to_iio_dev_attr(attr)->buffer; return sprintf(buf, "%d\n", buffer->scan_timestamp); } @@ -415,7 +413,7 @@ static ssize_t iio_scan_el_ts_store(struct device *dev, { int ret; struct iio_dev *indio_dev = dev_to_iio_dev(dev); - struct iio_buffer *buffer = indio_dev->buffer; + struct iio_buffer *buffer = to_iio_dev_attr(attr)->buffer; bool state; ret = strtobool(buf, &state); @@ -448,7 +446,7 @@ static int iio_buffer_add_channel_sysfs(struct iio_dev *indio_dev, IIO_SEPARATE, &indio_dev->dev, buffer, - &buffer->scan_el_dev_attr_list); + &buffer->buffer_attr_list); if (ret) return ret; attrcount++; @@ -460,7 +458,7 @@ static int iio_buffer_add_channel_sysfs(struct iio_dev *indio_dev, 0, &indio_dev->dev, buffer, - &buffer->scan_el_dev_attr_list); + &buffer->buffer_attr_list); if (ret) return ret; attrcount++; @@ -473,7 +471,7 @@ static int iio_buffer_add_channel_sysfs(struct iio_dev *indio_dev, 0, &indio_dev->dev, buffer, - &buffer->scan_el_dev_attr_list); + &buffer->buffer_attr_list); else ret = __iio_add_chan_devattr("en", chan, @@ -483,7 +481,7 @@ static int iio_buffer_add_channel_sysfs(struct iio_dev *indio_dev, 0, &indio_dev->dev, buffer, - &buffer->scan_el_dev_attr_list); + &buffer->buffer_attr_list); if (ret) return ret; attrcount++; @@ -495,8 +493,7 @@ static ssize_t iio_buffer_read_length(struct device *dev, struct device_attribute *attr, char *buf) { - struct iio_dev *indio_dev = dev_to_iio_dev(dev); - struct iio_buffer *buffer = indio_dev->buffer; + struct iio_buffer *buffer = to_iio_dev_attr(attr)->buffer; return sprintf(buf, "%d\n", buffer->length); } @@ -506,7 +503,7 @@ static ssize_t iio_buffer_write_length(struct device *dev, 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_buffer *buffer = to_iio_dev_attr(attr)->buffer; unsigned int val; int ret; @@ -538,8 +535,7 @@ static ssize_t iio_buffer_show_enable(struct device *dev, struct device_attribute *attr, char *buf) { - struct iio_dev *indio_dev = dev_to_iio_dev(dev); - struct iio_buffer *buffer = indio_dev->buffer; + struct iio_buffer *buffer = to_iio_dev_attr(attr)->buffer; return sprintf(buf, "%d\n", iio_buffer_is_active(buffer)); } @@ -1154,7 +1150,7 @@ static ssize_t iio_buffer_store_enable(struct device *dev, int ret; bool requested_state; struct iio_dev *indio_dev = dev_to_iio_dev(dev); - struct iio_buffer *buffer = indio_dev->buffer; + struct iio_buffer *buffer = to_iio_dev_attr(attr)->buffer; bool inlist; ret = strtobool(buf, &requested_state); @@ -1183,8 +1179,7 @@ static ssize_t iio_buffer_show_watermark(struct device *dev, struct device_attribute *attr, char *buf) { - struct iio_dev *indio_dev = dev_to_iio_dev(dev); - struct iio_buffer *buffer = indio_dev->buffer; + struct iio_buffer *buffer = to_iio_dev_attr(attr)->buffer; return sprintf(buf, "%u\n", buffer->watermark); } @@ -1195,7 +1190,7 @@ static ssize_t iio_buffer_store_watermark(struct device *dev, size_t len) { struct iio_dev *indio_dev = dev_to_iio_dev(dev); - struct iio_buffer *buffer = indio_dev->buffer; + struct iio_buffer *buffer = to_iio_dev_attr(attr)->buffer; unsigned int val; int ret; @@ -1228,8 +1223,7 @@ static ssize_t iio_dma_show_data_available(struct device *dev, struct device_attribute *attr, char *buf) { - struct iio_dev *indio_dev = dev_to_iio_dev(dev); - struct iio_buffer *buffer = indio_dev->buffer; + struct iio_buffer *buffer = to_iio_dev_attr(attr)->buffer; return sprintf(buf, "%zu\n", iio_buffer_data_available(buffer)); } @@ -1254,6 +1248,26 @@ static struct attribute *iio_buffer_attrs[] = { &dev_attr_data_available.attr, }; +#define to_dev_attr(_attr) container_of(_attr, struct device_attribute, attr) + +static struct attribute *iio_buffer_wrap_attr(struct iio_buffer *buffer, + struct attribute *attr) +{ + struct device_attribute *dattr = to_dev_attr(attr); + struct iio_dev_attr *iio_attr; + + iio_attr = kzalloc(sizeof(*iio_attr), GFP_KERNEL); + if (!iio_attr) + return NULL; + + iio_attr->buffer = buffer; + memcpy(&iio_attr->dev_attr, dattr, sizeof(iio_attr->dev_attr)); + + list_add(&iio_attr->l, &buffer->buffer_attr_list); + + return &iio_attr->dev_attr.attr; +} + static int iio_buffer_register_legacy_sysfs_groups(struct iio_dev *indio_dev, struct attribute **buffer_attrs, int buffer_attrcount, @@ -1329,7 +1343,7 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer, } scan_el_attrcount = 0; - INIT_LIST_HEAD(&buffer->scan_el_dev_attr_list); + INIT_LIST_HEAD(&buffer->buffer_attr_list); channels = indio_dev->channels; if (channels) { /* new magic */ @@ -1376,9 +1390,19 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer, buffer_attrcount += ARRAY_SIZE(iio_buffer_attrs); - attrn = buffer_attrcount; + for (i = 0; i < buffer_attrcount; i++) { + struct attribute *wrapped; + + wrapped = iio_buffer_wrap_attr(buffer, attr[i]); + if (!wrapped) { + ret = -ENOMEM; + goto error_free_scan_mask; + } + attr[i] = wrapped; + } - list_for_each_entry(p, &buffer->scan_el_dev_attr_list, l) + attrn = 0; + list_for_each_entry(p, &buffer->buffer_attr_list, l) attr[attrn++] = &p->dev_attr.attr; buffer->buffer_group.name = kasprintf(GFP_KERNEL, "buffer%d", index); @@ -1412,7 +1436,7 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer, error_free_scan_mask: bitmap_free(buffer->scan_mask); error_cleanup_dynamic: - iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list); + iio_free_chan_devattr_list(&buffer->buffer_attr_list); return ret; } @@ -1443,7 +1467,7 @@ static void __iio_buffer_free_sysfs_and_mask(struct iio_buffer *buffer) bitmap_free(buffer->scan_mask); kfree(buffer->buffer_group.name); kfree(buffer->buffer_group.attrs); - iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list); + iio_free_chan_devattr_list(&buffer->buffer_attr_list); } void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev) diff --git a/include/linux/iio/buffer_impl.h b/include/linux/iio/buffer_impl.h index 3e555e58475b..41044320e581 100644 --- a/include/linux/iio/buffer_impl.h +++ b/include/linux/iio/buffer_impl.h @@ -97,8 +97,8 @@ struct iio_buffer { /* @scan_timestamp: Does the scan mode include a timestamp. */ bool scan_timestamp; - /* @scan_el_dev_attr_list: List of scan element related attributes. */ - struct list_head scan_el_dev_attr_list; + /* @buffer_attr_list: List of buffer attributes. */ + struct list_head buffer_attr_list; /* * @buffer_group: Attributes of the new buffer group.
This change wraps all buffer attributes into iio_dev_attr objects, and assigns a reference to the IIO buffer they belong to. With the addition of multiple IIO buffers per one IIO device, we need a way to know which IIO buffer is being enabled/disabled/controlled. We know that all buffer attributes are device_attributes. So we can wrap them with a iio_dev_attr types. In the iio_dev_attr type, we can also hold a reference to an IIO buffer. So, we end up being able to allocate wrapped attributes for all buffer attributes (even the one from other drivers). The neat part with this mechanism, is that we don't need to add any extra cleanup, because these attributes are being added to a dynamic list that will get cleaned up via iio_free_chan_devattr_list(). With this change, the 'buffer->scan_el_dev_attr_list' list is being renamed to 'buffer->buffer_attr_list', effectively merging (or finalizing the merge) of the buffer/ & scan_elements/ attributes internally. Accessing these new buffer attributes can now be done via 'to_iio_dev_attr(attr)->buffer' inside the show/store handlers. Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> --- drivers/iio/industrialio-buffer.c | 76 ++++++++++++++++++++----------- include/linux/iio/buffer_impl.h | 4 +- 2 files changed, 52 insertions(+), 28 deletions(-)