Re: [PATCH v2 03/12][RESEND] iio: buffer: rework buffer & scan_elements dir creation

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Tue, 26 Jan 2021 11:45:04 +0200
Alexandru Ardelean <ardeleanalex@gmail.com> wrote:

> On Mon, Jan 25, 2021 at 9:32 PM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Sun, Jan 24, 2021 at 06:11:26PM +0000, Jonathan Cameron wrote:  
> > > On Fri, 22 Jan 2021 18:25:20 +0200
> > > Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
> > >  
> > > > When adding more than one IIO buffer per IIO device, we will need to create
> > > > a buffer & scan_elements directory for each buffer.
> > > > We also want to move the 'scan_elements' to be a sub-directory of the
> > > > 'buffer' folder.
> > > >
> > > > The format we want to reach is, for a iio:device0 folder, for 2 buffers
> > > > [for example], we have a 'buffer0' and a 'buffer1' subfolder, and each with
> > > > it's own 'scan_elements' subfolder.
> > > >
> > > > So, for example:
> > > >    iio:device0/buffer0
> > > >       scan_elements/
> > > >
> > > >    iio:device0/buffer1
> > > >       scan_elements/
> > > >
> > > > The other attributes under 'bufferX' would remain unchanged.
> > > >
> > > > However, we would also need to symlink back to the old 'buffer' &
> > > > 'scan_elements' folders, to keep backwards compatibility.
> > > >
> > > > Doing all these, require that we maintain the kobjects for each 'bufferX'
> > > > and 'scan_elements' so that we can symlink them back. We also need to
> > > > implement the sysfs_ops for these folders.
> > > >
> > > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>  
> > >
> > > +CC GregKH and Rafael W for feedback on various things inline.
> > >
> > > It might be that this is the neatest solution that we can come up with but
> > > more eyes would be good!  
> >
> > In short, please do NOT do this.
> >
> > At all.
> >
> > no.
> >
> > {sigh}
> >  
> > >
> > > Whilst I think this looks fine, I'm less confident than I'd like to be.
> > >
> > > Jonathan
> > >  
> > > > ---
> > > >  drivers/iio/industrialio-buffer.c | 195 +++++++++++++++++++++++++++---
> > > >  drivers/iio/industrialio-core.c   |  24 ++--
> > > >  include/linux/iio/buffer_impl.h   |  14 ++-
> > > >  include/linux/iio/iio.h           |   2 +-
> > > >  4 files changed, 200 insertions(+), 35 deletions(-)
> > > >
> > > > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> > > > index 0412c4fda4c1..0f470d902790 100644
> > > > --- a/drivers/iio/industrialio-buffer.c
> > > > +++ b/drivers/iio/industrialio-buffer.c
> > > > @@ -1175,8 +1175,6 @@ static ssize_t iio_buffer_store_enable(struct device *dev,
> > > >     return (ret < 0) ? ret : len;
> > > >  }
> > > >
> > > > -static const char * const iio_scan_elements_group_name = "scan_elements";
> > > > -
> > > >  static ssize_t iio_buffer_show_watermark(struct device *dev,
> > > >                                      struct device_attribute *attr,
> > > >                                      char *buf)
> > > > @@ -1252,6 +1250,124 @@ static struct attribute *iio_buffer_attrs[] = {
> > > >     &dev_attr_data_available.attr,
> > > >  };
> > > >
> > > > +#define to_dev_attr(_attr) container_of(_attr, struct device_attribute, attr)
> > > > +
> > > > +static ssize_t iio_buffer_dir_attr_show(struct kobject *kobj,
> > > > +                                   struct attribute *attr,
> > > > +                                   char *buf)
> > > > +{
> > > > +   struct iio_buffer *buffer = container_of(kobj, struct iio_buffer, buffer_dir);
> > > > +   struct device_attribute *dattr;
> > > > +
> > > > +   dattr = to_dev_attr(attr);
> > > > +
> > > > +   return dattr->show(&buffer->indio_dev->dev, dattr, buf);
> > > > +}  
> >
> >
> > First off, you are dealing with "raw" kobjects here, below a 'struct
> > device' in the device tree, which means that suddenly userspace does not
> > know what in the world is going on, and you lost events and lots of
> > other stuff.
> >
> > Never do this.  It should not be needed, and you are just trying to
> > paper over one odd decision of an api with another one you will be stuck
> > with for forever.
> >
> > Remember the driver core can create subdirectories for your attributes
> > automatically if you want them to be in a subdir, but that's it, no
> > further than that.  Just name the attribute group.
> >
> > But yes, you can not create a symlink to there, because (surprise), you
> > don't want to!
> >
> > So please, just rethink your naming, create a totally new naming scheme
> > for multiple entities, and just drop the old one (or keep a single
> > value if you really want to.)  Don't make it harder than it has to be
> > please, you can never remove the "compatible symlinks", just make a new
> > api and move on.  

Thanks Greg. I had a feeling we were pushing things too far in an
ugly direction :(

> 
> 
> So, coming back to Jonathan.
> Any thoughts on how to proceed?
> 
> We could merge the files 'buffer & scan_elements' [from in the
> /sys/bus/iio/devices/iio:deviceX/{buffer,scan_elements}
> 
> So, essentially:
> # ls /sys/bus/iio/devices/iio:deviceX/bufferY
> data_available       length              watermark
> enable                   length_align_bytes
> in_voltage0_en      in_voltage0_type   in_voltage1_index
> in_voltage0_index  in_voltage1_en     in_voltage1_type
> 
> Where:
> # ls  /sys/bus/iio/devices/iio:deviceX/scan_elements
> in_voltage0_en     in_voltage0_type   in_voltage1_index
> in_voltage0_index  in_voltage1_en     in_voltage1_typ
> 
> # ls  /sys/bus/iio/devices/iio:deviceX/buffer
> data_available      length              watermark
> enable              length_align_bytes
> 
> I don't think we need to add any prefixes for the scan_elements/buffer
> files, or?

Hmm. I guess this works. The only alternative I can think of would
be bufferY and bufferY_scan_elements directories, but that is probably
worse than what you suggest.

I'm not keen on the lost of grouping between of scan elements but it
may just be a price we have to pay.  Probably not too bad as only
in_ elements (_out shortly I guess) exist in scan_elements.

To maintain backwards compatibility we'll need to also register the
old attributes, but that shouldn't be too painful beyond a bunch
of near duplicated code.

> 
> Do we still do this new ioctl() for buffer0, 1, 2, N being accessed
> via anon inodes?
> Or do we go [back] via the route of each buffer with it's own chardev?
> i.e.  introduce a "/dev/iio/deviceX/bufferY" structure
> 

Reality is most of our devices are going to remain single buffered, so
whilst I'm not overly keen on proliferation of chardevs the cost should
be fairly small.

For separate chardevs what would the naming in /dev/ look like?
Gut feeling is stay with the anon approach, at least partly for
consistency with the event interface.

Jonathan


> I'm fine either way.
> 
> Thanks
> Alex
> 
> >
> > thanks,
> >
> > greg k-h