Message-ID: <dd67c3500beb67ba40488d5ef49db7ee934dfe99.camel@analog.com>
Date:   Wed, 6 May 2020 12:52:08 +0000
From:   "Ardelean, Alexandru" <alexandru.Ardelean@...log.com>
To:     "jic23@...nel.org" <jic23@...nel.org>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
        "pmeerw@...erw.net" <pmeerw@...erw.net>,
        "lars@...afoo.de" <lars@...afoo.de>
Subject: Re: [PATCH v6 0/6] iio: core,buffer: re-organize chardev creation

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@...log.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(-)
> > 

Powered by blists - more mailing lists