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

[Alexandru Ardelean]

On Mon, Jan 25, 2021 at 9:32 PM Greg Kroah-Hartman
<gregkh@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx>
> >
> > +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.


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?

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

I'm fine either way.

Thanks
Alex

>
> thanks,
>
> greg k-h