| Message ID | 20200427131100.50845-1-alexandru.ardelean@analog.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | iio: core,buffer: re-organize chardev creation | expand |
On Mon, 27 Apr 2020 16:10:54 +0300 Alexandru Ardelean <alexandru.ardelean@analog.com> wrote: > The main intent is to be able to add more chardevs per IIO device, one > for each buffer. To get there, some rework is needed. > This tries to re-organize the init of the chardev. Hmm. I'd like this set to sit and ideally gather a few acks before I move ahead with it. The protections against problems around remove have always been somewhat fiddly and I suspect don't cover everything. I'm fairly sure taking the exist lock is 'sufficient' but I'm not actually sure it's necessary. We only otherwise take it for place where the inkern interface is in use so we can race against removal of a provider driver. We don't have such heavy weight protections in the buffer code and I'll be honest I can't remember why. Original patch mentions that it was about avoiding taking additional new references to the struct iio_dev. We aren't doing that as such here so perhaps we don't need to take the lock.. Lars, I suspect you may have been involved in that stuff originally so I'd appreciate you taking a quick look at this if you have time! Thanks, Jonathan > > > Changelog v5 -> v6: > - patch 'iio: core: register chardev only if needed' > - sort file_operations fields for iio_event_fileops > - patch 'iio: buffer,event: duplicate chardev creation for buffers & events' > - fixed-up '**/' -> '*/' for 2 block comments > - sorted file_operations for iio_buffer_fileops, after move > - removed 'indio_dev->chrdev = NULL' on IIO device unregister > - added comment about 'indio_dev->info' NULL check in > iio_device_event_ioctl() > - patch 'iio: core: add simple centralized mechanism for ioctl() handlers' > - re-using lock 'indio_dev->info_exist_lock' for new ioctl register > mechanism in iio_device_ioctl() > - simplified exit condition from the loop; only need to check > `ret != IIO_IOCTL_UNHANDLED` to continue looping; > everything else is just return/break > - patch 'iio: core: use new common ioctl() mechanism' > - the comment for 'indio_dev->info' NULL check is being moved here to > highlight why the null-check is being removed; or where it's being > moved > > Changelog v4 -> v5: > - dropped patch 'iio: Use an early return in iio_device_alloc to simplify code.' > is applied upstream > > Changelog v3 -> v4: > - added patch [1] 'iio: Use an early return in iio_device_alloc to simplify code.' > it's main purpose is so that this patch applies: > [2]'iio: core: add simple centralized mechanism for ioctl() handlers' > depending on the final version of patch [1], patch [2] needs some > minor fixup > - added patch 'iio: core,buffer: wrap iio_buffer_put() call into iio_buffers_put()' > - patch 'iio: core: register buffer fileops only if buffer present' > is now: 'iio: core: register chardev only if needed' > - dropped 'iio: buffer: move sysfs alloc/free in industrialio-buffer.c' > it's likely we won't be doing this patch anymore > - patches: > 'iio: buffer: move iio buffer chrdev in industrialio-buffer.c' > 'iio: event: move event-only chardev in industrialio-event.c' > have been merged into 'iio: buffer,event: duplicate chardev creation for buffers & events' > since now, the logic is a bit different, and 'indio_dev->chrdev' is > now a reference to either the buffer's chrdev & or the events-only > chrdev > - added simple mechanism to register ioctl() handlers for IIO device > which is currently used only by events mechanism > > Changelog v2 -> v3: > * removed double init in > 'iio: event: move event-only chardev in industrialio-event.c' > > Changelog v1 -> v2: > * re-reviewed some exit-paths and cleanup some potential leaks on those > exit paths: > - for 'iio: buffer: move iio buffer chrdev in industrialio-buffer.c' > add iio_device_buffers_put() helper and calling iio_buffers_uninit() > on device un-regsiter > - for 'move sysfs alloc/free in industrialio-buffer.c' > call 'iio_buffer_free_sysfs_and_mask()' on exit path if > cdev_device_add() fails > - for 'move event-only chardev in industrialio-event.c' > check if event_interface is NULL in > iio_device_unregister_event_chrdev() > > Alexandru Ardelean (6): > iio: buffer: add back-ref from iio_buffer to iio_dev > iio: core,buffer: wrap iio_buffer_put() call into iio_buffers_put() > iio: core: register chardev only if needed > iio: buffer,event: duplicate chardev creation for buffers & events > iio: core: add simple centralized mechanism for ioctl() handlers > iio: core: use new common ioctl() mechanism > > drivers/iio/iio_core.h | 29 +++++--- > drivers/iio/industrialio-buffer.c | 102 ++++++++++++++++++++++++-- > drivers/iio/industrialio-core.c | 116 +++++++++++++----------------- > drivers/iio/industrialio-event.c | 100 +++++++++++++++++++++++++- > include/linux/iio/buffer_impl.h | 10 +++ > include/linux/iio/iio.h | 8 +-- > 6 files changed, 276 insertions(+), 89 deletions(-) >
On Sun, 2020-05-03 at 16:51 +0100, Jonathan Cameron wrote: > [External] > > On Mon, 27 Apr 2020 16:10:54 +0300 > Alexandru Ardelean <alexandru.ardelean@analog.com> wrote: > > > The main intent is to be able to add more chardevs per IIO device, one > > for each buffer. To get there, some rework is needed. > > This tries to re-organize the init of the chardev. > > Hmm. I'd like this set to sit and ideally gather a few acks before > I move ahead with it. > > The protections against problems around remove have always been > somewhat fiddly and I suspect don't cover everything. > > I'm fairly sure taking the exist lock is 'sufficient' but I'm not > actually sure it's necessary. We only otherwise take it for > place where the inkern interface is in use so we can race > against removal of a provider driver. > > We don't have such heavy weight protections in the buffer code > and I'll be honest I can't remember why. Original patch mentions > that it was about avoiding taking additional new references to the > struct iio_dev. We aren't doing that as such here so perhaps > we don't need to take the lock.. > > Lars, I suspect you may have been involved in that stuff originally > so I'd appreciate you taking a quick look at this if you have > time! > Well, it will take me a bit to re-do this series. I've got to a point where some of these patches will need some re-tuning. . Adding multiple buffers per IIO device seems a bit more difficult than I initially thought. > Thanks, > > Jonathan > > > > > Changelog v5 -> v6: > > - patch 'iio: core: register chardev only if needed' > > - sort file_operations fields for iio_event_fileops > > - patch 'iio: buffer,event: duplicate chardev creation for buffers & events' > > - fixed-up '**/' -> '*/' for 2 block comments > > - sorted file_operations for iio_buffer_fileops, after move > > - removed 'indio_dev->chrdev = NULL' on IIO device unregister > > - added comment about 'indio_dev->info' NULL check in > > iio_device_event_ioctl() > > - patch 'iio: core: add simple centralized mechanism for ioctl() handlers' > > - re-using lock 'indio_dev->info_exist_lock' for new ioctl register > > mechanism in iio_device_ioctl() > > - simplified exit condition from the loop; only need to check > > `ret != IIO_IOCTL_UNHANDLED` to continue looping; > > everything else is just return/break > > - patch 'iio: core: use new common ioctl() mechanism' > > - the comment for 'indio_dev->info' NULL check is being moved here to > > highlight why the null-check is being removed; or where it's being > > moved > > > > Changelog v4 -> v5: > > - dropped patch 'iio: Use an early return in iio_device_alloc to simplify > > code.' > > is applied upstream > > > > Changelog v3 -> v4: > > - added patch [1] 'iio: Use an early return in iio_device_alloc to simplify > > code.' > > it's main purpose is so that this patch applies: > > [2]'iio: core: add simple centralized mechanism for ioctl() handlers' > > depending on the final version of patch [1], patch [2] needs some > > minor fixup > > - added patch 'iio: core,buffer: wrap iio_buffer_put() call into > > iio_buffers_put()' > > - patch 'iio: core: register buffer fileops only if buffer present' > > is now: 'iio: core: register chardev only if needed' > > - dropped 'iio: buffer: move sysfs alloc/free in industrialio-buffer.c' > > it's likely we won't be doing this patch anymore > > - patches: > > 'iio: buffer: move iio buffer chrdev in industrialio-buffer.c' > > 'iio: event: move event-only chardev in industrialio-event.c' > > have been merged into 'iio: buffer,event: duplicate chardev creation for > > buffers & events' > > since now, the logic is a bit different, and 'indio_dev->chrdev' is > > now a reference to either the buffer's chrdev & or the events-only > > chrdev > > - added simple mechanism to register ioctl() handlers for IIO device > > which is currently used only by events mechanism > > > > Changelog v2 -> v3: > > * removed double init in > > 'iio: event: move event-only chardev in industrialio-event.c' > > > > Changelog v1 -> v2: > > * re-reviewed some exit-paths and cleanup some potential leaks on those > > exit paths: > > - for 'iio: buffer: move iio buffer chrdev in industrialio-buffer.c' > > add iio_device_buffers_put() helper and calling iio_buffers_uninit() > > on device un-regsiter > > - for 'move sysfs alloc/free in industrialio-buffer.c' > > call 'iio_buffer_free_sysfs_and_mask()' on exit path if > > cdev_device_add() fails > > - for 'move event-only chardev in industrialio-event.c' > > check if event_interface is NULL in > > iio_device_unregister_event_chrdev() > > > > Alexandru Ardelean (6): > > iio: buffer: add back-ref from iio_buffer to iio_dev > > iio: core,buffer: wrap iio_buffer_put() call into iio_buffers_put() > > iio: core: register chardev only if needed > > iio: buffer,event: duplicate chardev creation for buffers & events > > iio: core: add simple centralized mechanism for ioctl() handlers > > iio: core: use new common ioctl() mechanism > > > > drivers/iio/iio_core.h | 29 +++++--- > > drivers/iio/industrialio-buffer.c | 102 ++++++++++++++++++++++++-- > > drivers/iio/industrialio-core.c | 116 +++++++++++++----------------- > > drivers/iio/industrialio-event.c | 100 +++++++++++++++++++++++++- > > include/linux/iio/buffer_impl.h | 10 +++ > > include/linux/iio/iio.h | 8 +-- > > 6 files changed, 276 insertions(+), 89 deletions(-) > >
The main intent is to be able to add more chardevs per IIO device, one for each buffer. To get there, some rework is needed. This tries to re-organize the init of the chardev. Changelog v5 -> v6: - patch 'iio: core: register chardev only if needed' - sort file_operations fields for iio_event_fileops - patch 'iio: buffer,event: duplicate chardev creation for buffers & events' - fixed-up '**/' -> '*/' for 2 block comments - sorted file_operations for iio_buffer_fileops, after move - removed 'indio_dev->chrdev = NULL' on IIO device unregister - added comment about 'indio_dev->info' NULL check in iio_device_event_ioctl() - patch 'iio: core: add simple centralized mechanism for ioctl() handlers' - re-using lock 'indio_dev->info_exist_lock' for new ioctl register mechanism in iio_device_ioctl() - simplified exit condition from the loop; only need to check `ret != IIO_IOCTL_UNHANDLED` to continue looping; everything else is just return/break - patch 'iio: core: use new common ioctl() mechanism' - the comment for 'indio_dev->info' NULL check is being moved here to highlight why the null-check is being removed; or where it's being moved Changelog v4 -> v5: - dropped patch 'iio: Use an early return in iio_device_alloc to simplify code.' is applied upstream Changelog v3 -> v4: - added patch [1] 'iio: Use an early return in iio_device_alloc to simplify code.' it's main purpose is so that this patch applies: [2]'iio: core: add simple centralized mechanism for ioctl() handlers' depending on the final version of patch [1], patch [2] needs some minor fixup - added patch 'iio: core,buffer: wrap iio_buffer_put() call into iio_buffers_put()' - patch 'iio: core: register buffer fileops only if buffer present' is now: 'iio: core: register chardev only if needed' - dropped 'iio: buffer: move sysfs alloc/free in industrialio-buffer.c' it's likely we won't be doing this patch anymore - patches: 'iio: buffer: move iio buffer chrdev in industrialio-buffer.c' 'iio: event: move event-only chardev in industrialio-event.c' have been merged into 'iio: buffer,event: duplicate chardev creation for buffers & events' since now, the logic is a bit different, and 'indio_dev->chrdev' is now a reference to either the buffer's chrdev & or the events-only chrdev - added simple mechanism to register ioctl() handlers for IIO device which is currently used only by events mechanism Changelog v2 -> v3: * removed double init in 'iio: event: move event-only chardev in industrialio-event.c' Changelog v1 -> v2: * re-reviewed some exit-paths and cleanup some potential leaks on those exit paths: - for 'iio: buffer: move iio buffer chrdev in industrialio-buffer.c' add iio_device_buffers_put() helper and calling iio_buffers_uninit() on device un-regsiter - for 'move sysfs alloc/free in industrialio-buffer.c' call 'iio_buffer_free_sysfs_and_mask()' on exit path if cdev_device_add() fails - for 'move event-only chardev in industrialio-event.c' check if event_interface is NULL in iio_device_unregister_event_chrdev() Alexandru Ardelean (6): iio: buffer: add back-ref from iio_buffer to iio_dev iio: core,buffer: wrap iio_buffer_put() call into iio_buffers_put() iio: core: register chardev only if needed iio: buffer,event: duplicate chardev creation for buffers & events iio: core: add simple centralized mechanism for ioctl() handlers iio: core: use new common ioctl() mechanism drivers/iio/iio_core.h | 29 +++++--- drivers/iio/industrialio-buffer.c | 102 ++++++++++++++++++++++++-- drivers/iio/industrialio-core.c | 116 +++++++++++++----------------- drivers/iio/industrialio-event.c | 100 +++++++++++++++++++++++++- include/linux/iio/buffer_impl.h | 10 +++ include/linux/iio/iio.h | 8 +-- 6 files changed, 276 insertions(+), 89 deletions(-)