Re: [RFC PATCH 00/14] iio: buffer: add support for multiple buffers

[Jonathan Cameron]

On Sun, 17 May 2020 06:26:17 +0000
"Ardelean, Alexandru" <alexandru.Ardelean@xxxxxxxxxx> wrote:

> On Sat, 2020-05-16 at 17:24 +0100, Jonathan Cameron wrote:
> > [External]
> > 
> > On Sat, 16 May 2020 13:08:46 +0000
> > "Ardelean, Alexandru" <alexandru.Ardelean@xxxxxxxxxx> wrote:
> >   
> > > On Tue, 2020-05-12 at 06:26 +0000, Ardelean, Alexandru wrote:  
> > > > [External]
> > > > 
> > > > On Mon, 2020-05-11 at 21:56 +0200, Lars-Peter Clausen wrote:    
> > > > > [External]
> > > > > 
> > > > > On 5/11/20 4:56 PM, Ardelean, Alexandru wrote:    
> > > > > > On Mon, 2020-05-11 at 15:58 +0200, Lars-Peter Clausen wrote:    
> > > > > > > [External]
> > > > > > > 
> > > > > > > On 5/11/20 3:24 PM, Ardelean, Alexandru wrote:    
> > > > > > > > On Mon, 2020-05-11 at 13:03 +0000, Ardelean, Alexandru wrote:    
> > > > > > > > > [External]
> > > > > > > > > 
> > > > > > > > > On Mon, 2020-05-11 at 12:37 +0200, Lars-Peter Clausen wrote:    
> > > > > > > > > > [External]
> > > > > > > > > > 
> > > > > > > > > > On 5/11/20 12:33 PM, Ardelean, Alexandru wrote:    
> > > > > > > > > > > On Sun, 2020-05-10 at 11:09 +0100, Jonathan Cameron wrote:    
> > > > > > > > > > > > [External]
> > > > > > > > > > > > 
> > > > > > > > > > > > On Sat, 9 May 2020 10:52:14 +0200
> > > > > > > > > > > > Lars-Peter Clausen <lars@xxxxxxxxxx> wrote:
> > > > > > > > > > > >     
> > > > > > > > > > > > > On 5/8/20 3:53 PM, Alexandru Ardelean wrote:    
> > > > > > > > > > > > > > [...]
> > > > > > > > > > > > > > What I don't like, is that iio:device3 has
> > > > > > > > > > > > > > iio:buffer3:0
> > > > > > > > > > > > > > (to
> > > > > > > > > > > > > > 3).
> > > > > > > > > > > > > > This is because the 'buffer->dev.parent =
> > > > > > > > > > > > > > &indio_dev-    
> > > > > > > > > > > > > > > dev'.    
> > > > > > > > > > > > > > But I do feel this is correct.
> > > > > > > > > > > > > > So, now I don't know whether to leave it like that or
> > > > > > > > > > > > > > symlink to
> > > > > > > > > > > > > > shorter
> > > > > > > > > > > > > > versions like 'iio:buffer3:Y' ->
> > > > > > > > > > > > > > 'iio:device3/bufferY'.
> > > > > > > > > > > > > > The reason for naming the IIO buffer devices to
> > > > > > > > > > > > > > 'iio:bufferX:Y'
> > > > > > > > > > > > > > is
> > > > > > > > > > > > > > mostly to make the names unique. It would have looked
> > > > > > > > > > > > > > weird
> > > > > > > > > > > > > > to
> > > > > > > > > > > > > > do
> > > > > > > > > > > > > > '/dev/buffer1' if I would have named the buffer
> > > > > > > > > > > > > > devices
> > > > > > > > > > > > > > 'bufferX'.
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > So, now I'm thinking of whether all this is
> > > > > > > > > > > > > > acceptable.
> > > > > > > > > > > > > > Or what is acceptable?
> > > > > > > > > > > > > > Should I symlink 'iio:device3/iio:buffer3:0' ->
> > > > > > > > > > > > > > 'iio:device3/buffer0'?
> > > > > > > > > > > > > > What else should I consider moving forward?
> > > > > > > > > > > > > > What means forward?
> > > > > > > > > > > > > > Where did I leave my beer?    
> > > > > > > > > > > > > Looking at how the /dev/ devices are named I think we
> > > > > > > > > > > > > can
> > > > > > > > > > > > > provide
> > > > > > > > > > > > > a
> > > > > > > > > > > > > name
> > > > > > > > > > > > > that is different from the dev_name() of the device.
> > > > > > > > > > > > > Have a
> > > > > > > > > > > > > look
> > > > > > > > > > > > > at
> > > > > > > > > > > > > device_get_devnode() in drivers/base/core.c. We should
> > > > > > > > > > > > > be
> > > > > > > > > > > > > able
> > > > > > > > > > > > > to
> > > > > > > > > > > > > provide the name for the chardev through the devnode()
> > > > > > > > > > > > > callback.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > While we are at this, do we want to move the new devices
> > > > > > > > > > > > > into
> > > > > > > > > > > > > an
> > > > > > > > > > > > > iio
> > > > > > > > > > > > > subfolder? So iio/buffer0:0 instead of iio:buffer0:0?    
> > > > > > > > > > > > Possibly on the folder.  I can't for the life of me
> > > > > > > > > > > > remember
> > > > > > > > > > > > why
> > > > > > > > > > > > I
> > > > > > > > > > > > decided
> > > > > > > > > > > > not to do that the first time around - I'll leave it at
> > > > > > > > > > > > the
> > > > > > > > > > > > mysterious "it may turn out to be harder than you'd
> > > > > > > > > > > > think..."
> > > > > > > > > > > > Hopefully not ;)    
> > > > > > > > > > > I was also thinking about the /dev/iio subfolder while doing
> > > > > > > > > > > this.
> > > > > > > > > > > I can copy that from /dev/input
> > > > > > > > > > > They seem to do it already.
> > > > > > > > > > > I don't know how difficult it would be. But it looks like a
> > > > > > > > > > > good
> > > > > > > > > > > precedent.    
> > > > > > > > > > All you have to do is return "iio/..." from the devnode()
> > > > > > > > > > callback.    
> > > > > > > > > I admit I did not look closely into drivers/input/input.c before
> > > > > > > > > mentioning
> > > > > > > > > this
> > > > > > > > > as as good precedent.
> > > > > > > > > 
> > > > > > > > > But, I looks like /dev/inpput is a class.
> > > > > > > > > While IIO devices are a bus_type devices.
> > > > > > > > > Should we start implementing an IIO class? or?    
> > > > > > > > What I should have highlighted [before] with this, is that there
> > > > > > > > is no
> > > > > > > > devnode()
> > > > > > > > callback for the bus_type [type].    
> > > > > > > But there is one in device_type :)    
> > > > > > Many thanks :)
> > > > > > That worked nicely.
> > > > > > 
> > > > > > I now have:
> > > > > > 
> > > > > > root@analog:~# ls /dev/iio/*
> > > > > > /dev/iio/iio:device0  /dev/iio/iio:device1
> > > > > > 
> > > > > > /dev/iio/device3:
> > > > > > buffer0  buffer1  buffer2  buffer3
> > > > > > 
> > > > > > /dev/iio/device4:
> > > > > > buffer0
> > > > > > 
> > > > > > 
> > > > > > It looks like I can shift these around as needed.
> > > > > > This is just an experiment.
> > > > > > I managed to move the iio devices under /dev/iio, though probably the
> > > > > > IIO
> > > > > > devices will still be around as /dev/iio:deviceX for legacy reasons.
> > > > > > 
> > > > > > Two things remain unresolved.
> > > > > > 1. The name of the IIO buffer device.
> > > > > > 
> > > > > > root@analog:/sys/bus/iio/devices# ls iio\:device3/
> > > > > > buffer          in_voltage0_test_mode           name
> > > > > > events          in_voltage1_test_mode           of_node
> > > > > > iio:buffer:3:0  in_voltage_sampling_frequency   power
> > > > > > iio:buffer:3:1  in_voltage_scale                scan_elements
> > > > > > iio:buffer:3:2  in_voltage_scale_available      subsystem
> > > > > > iio:buffer:3:3  in_voltage_test_mode_available  uevent
> > > > > > 
> > > > > > 
> > > > > > Right now, each buffer device is named 'iio:buffer:X:Y'.
> > > > > > One suggesttion was  'iio:deviceX:bufferY'
> > > > > > I'm suspecting the latter is preferred as when you sort the folders,
> > > > > > buffers
> > > > > > come right after the iio:deviceX folders in /sys/bus/iio/devices.
> > > > > > 
> > > > > > I don't feel it matters much the device name of the IIO buffer if we
> > > > > > symlink
> > > > > > it
> > > > > > to a shorter form.
> > > > > >   
> > > > > > I'm guessing, we symlink these devices to short-hand 'bufferY' folders
> > > > > > in
> > > > > > each
> > > > > > 'iio:deviceX'?    
> > > > > 
> > > > > I think that would be a bit excessive. Only for the legacy buffer we 
> > > > > need to have a symlink.
> > > > >     
> > > > > > [...]
> > > > > > 2. I know this is [still] stupid now; but any suggestions one how to
> > > > > > symlink
> > > > > > /dev/iio:device3 -> /dev/iio/device3/buffer0 ?
> > > > > >     
> > > > > Does not seem to be possible. Userspace will have to take care of it. 
> > > > > This means we need to keep legacy devices in /dev/ and only new buffers 
> > > > > in /dev/iio/.    
> > > > 
> > > > One thought about this, was that we keep the chardev for the IIO device
> > > > for
> > > > this.
> > > > i.e.  /dev/iio:deviceX and /dev/iio/deviceX/buffer0 open the same buffer.
> > > > This means that for a device with 4 buffers, you get 5 chardevs.
> > > > This also seems a bit much/excessive. Maybe also in terms of source-code.
> > > > It would at least mean not moving the event-only chardev to 'industrialio-
> > > > event.c', OR move it, and have the same chardev in 3 places
> > > > ['industrialio-
> > > > event.c', 'industrialio-buffer.c' & 'industrialio-buffer.c'
> > > > 
> > > > Maybe this sort-of makes sense to have for a few years/kernel-revisions
> > > > until
> > > > things clean-up.
> > > > 
> > > > I guess at this point, the maintainer should have the final say about
> > > > this.    
> > > 
> > > Another 'compromise' idea, is that we make this '/dev/iio/deviceX/bufferY'
> > > thing
> > > a feature for new devices, and leave '/dev/iio:deviceX' devices [for
> > > buffers] a
> > > thing for current devices.
> > > It would mean adding a 'new' iio_device_attach_buffer(); no idea on a name
> > > [for
> > > this yet].  
> > 
> > Definitely a no to that.  If we make this transition it needs to be
> > automatic and subsystem wide.  At some point we could have a kconfig option
> > to disable the legacy interface subsystem wise as a precursor to eventually
> > dropping it.  
> >   
> > > Over time, people can convert existing drivers to the new IIO-buffer format,
> > > if
> > > they want to. That also gives them a bit better control over symlinking
> > > '/dev/iio:deviceX' -> '/dev/iio/deviceX/bufferY' [or symlinking in reverse
> > > if
> > > they want to].
> > > 
> > > That may create confusion I guess during a transition period.
> > > And it would [ideally] have a mechanism [preferably at build/compile time]
> > > to
> > > notify users to use the new IIO buffer mechanism [vs the old one] when
> > > adding
> > > new drivers.
> > > Otherwise, there's the risk of people copying the old IIO buffer mechanism.
> > > This can be brought-up at review, but Â\_(ã)_/Â ; it can be annoying.  
> > 
> > If we can't do this in a transparent fashion we need to rethink.
> > The existing interface 'has' to remain and do something sensible.
> > Realistically
> > we need to keep it in place for 3-5 years at least.
> > 
> > I'm not yet convinced the complexity is worthwhile.  We 'could' fallback to
> > the same trick used for events and use an ioctl to access all buffers
> > other than the first one...  Then we retain one chardev per iio device
> > and still get the flexibility we need to have multiple buffers.
> > In some ways it is tidier, even if a bit less intuitive...
> > If we can't build the symlinks we were all kind of assuming we could
> > we may need to rethink the overall path.
> > 
> > Anyhow, you are doing great work exploring the options!  
> 
> I wonder if you got to read the idea about adding more chardevs.
> 
> The one where /dev/iio:deviceX & /dev/iio/deviceX/buffer0 open the same buffer
> object. I'm not sure about any race issues here.
> The bad-part [I feel] is that we get more duplication on chardev file_operations
> (open, release, ioctl, etc).
> We need to re-wrap code-paths so that they open the same buffer.
> And the number of chardevs per IIO device increases by 1 (for a device with 4
> buffers == 4 chardevs + 1 legacy)

I read that one and it strikes me as a hack.  Two interfaces to the same
thing is always a bad idea if we can possibly avoid it.  I was happy enough
with a link if that was doable because then they are the same device with
same dev-node etc.  Though thinking more on that it will be hard to get
all the relevant links in place.


> 
> I kinda like the idea of the /dev/iio/ folder. But I'm not strongly opinionated
> towards it either.
> This also allows some /dev/iio/deviceX/eventY chardevs.
> And some other types of chardevs /dev/iio/deviceX/somethingNewY

That expansion of chrdevs is kind of what worries me.  We end up with
more and more of them.

> 
> But, if I think about it, the only benefit of this [over anon inodes for
> chardevs] is that it allows us to do direct access via cat/echo to the actual
> chardev of the buffer.
> Then, there's also the fact that adding more chardevs increases complexity to
> userspace, so it won't matter much. People would probably prefer some userspace
> IIO library to do the data read/write.
> 
> I'm getting the feeling now that the final debathe is 'anon inodes or not'

Yes, I think that is the big question.  Without your work to explore
the options we wouldn't really know what the other options were. 

I'm far from sure what the 'right' answer is to this.

So there is another corner case we should think about.  What do we
do about the devices that current expose multiple buffers by having multiple
IIO devices?  Usually that occurs for devices that can do weird interleaving
in hardware fifos - hence can produce some channels at N times the frequency of
others - or where we have one shared stream with tagged data.

I suspect it's going to be really hard to map those across to the new interface
with any sort of backwards compatibility.  They expose complete iio devices
with all the infrastructure that entails.  Maybe we just leave them alone?

J

> 
> Thoughts here?
> 
> 
> > 
> > Thanks,
> > 
> > Jonathan
> > 
> >   
> > >   
> > > >     
> > > > >