diff mbox series

[v6,20/24] iio: buffer: add ioctl() to support opening extra buffers for IIO device

Message ID 20210215104043.91251-21-alexandru.ardelean@analog.com (mailing list archive)
State New
Headers show
Series iio: core,buffer: add support for multiple IIO buffers per IIO device | expand

Commit Message

Alexandru Ardelean Feb. 15, 2021, 10:40 a.m. UTC
With this change, an ioctl() call is added to open a character device for a
buffer. The ioctl() number is 'i' 0x91, which follows the
IIO_GET_EVENT_FD_IOCTL ioctl.

The ioctl() will return an FD for the requested buffer index. The indexes
are the same from the /sys/iio/devices/iio:deviceX/bufferY (i.e. the Y
variable).

Since there doesn't seem to be a sane way to return the FD for buffer0 to
be the same FD for the /dev/iio:deviceX, this ioctl() will return another
FD for buffer0 (or the first buffer). This duplicate FD will be able to
access the same buffer object (for buffer0) as accessing directly the
/dev/iio:deviceX chardev.

Also, there is no IIO_BUFFER_GET_BUFFER_COUNT ioctl() implemented, as the
index for each buffer (and the count) can be deduced from the
'/sys/bus/iio/devices/iio:deviceX/bufferY' folders (i.e the number of
bufferY folders).

Used following C code to test this:
-------------------------------------------------------------------

 #include <stdio.h>
 #include <stdlib.h>
 #include <unistd.h>
 #include <sys/ioctl.h>
 #include <fcntl.h"
 #include <errno.h>

 #define IIO_BUFFER_GET_FD_IOCTL      _IOWR('i', 0x91, int)

int main(int argc, char *argv[])
{
        int fd;
        int fd1;
        int ret;

        if ((fd = open("/dev/iio:device0", O_RDWR))<0) {
                fprintf(stderr, "Error open() %d errno %d\n",fd, errno);
                return -1;
        }

        fprintf(stderr, "Using FD %d\n", fd);

        fd1 = atoi(argv[1]);

        ret = ioctl(fd, IIO_BUFFER_GET_FD_IOCTL, &fd1);
        if (ret < 0) {
                fprintf(stderr, "Error for buffer %d ioctl() %d errno %d\n", fd1, ret, errno);
                close(fd);
                return -1;
        }

        fprintf(stderr, "Got FD %d\n", fd1);

        close(fd1);
        close(fd);

        return 0;
}
-------------------------------------------------------------------

Results are:
-------------------------------------------------------------------
 # ./test 0
 Using FD 3
 Got FD 4

 # ./test 1
 Using FD 3
 Got FD 4

 # ./test 2
 Using FD 3
 Got FD 4

 # ./test 3
 Using FD 3
 Got FD 4

 # ls /sys/bus/iio/devices/iio\:device0
 buffer  buffer0  buffer1  buffer2  buffer3  dev
 in_voltage_sampling_frequency  in_voltage_scale
 in_voltage_scale_available
 name  of_node  power  scan_elements  subsystem  uevent
-------------------------------------------------------------------

iio:device0 has some fake kfifo buffers attached to an IIO device.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/iio_core.h            |  12 +--
 drivers/iio/industrialio-buffer.c | 144 ++++++++++++++++++++++++++++--
 include/linux/iio/buffer_impl.h   |   5 ++
 include/linux/iio/iio-opaque.h    |   2 +
 include/uapi/linux/iio/buffer.h   |  10 +++
 5 files changed, 162 insertions(+), 11 deletions(-)
 create mode 100644 include/uapi/linux/iio/buffer.h

Comments

Lars-Peter Clausen Feb. 28, 2021, 7:57 a.m. UTC | #1
On 2/15/21 11:40 AM, Alexandru Ardelean wrote:
> [...]
>   /**
>    * iio_buffer_wakeup_poll - Wakes up the buffer waitqueue
>    * @indio_dev: The IIO device
> @@ -1343,6 +1371,96 @@ static void iio_buffer_unregister_legacy_sysfs_groups(struct iio_dev *indio_dev)
>   	kfree(iio_dev_opaque->legacy_scan_el_group.attrs);
>   }
>   
> [...]
> +static long iio_device_buffer_getfd(struct iio_dev *indio_dev, unsigned long arg)
> +{
> +	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
> +	int __user *ival = (int __user *)arg;
> +	struct iio_dev_buffer_pair *ib;
> +	struct iio_buffer *buffer;
> +	int fd, idx, ret;
> +
> +	if (copy_from_user(&idx, ival, sizeof(idx)))
> +		return -EFAULT;

If we only want to pass an int, we can pass that directly, no need to 
pass it as a pointer.

int fd = arg;

> +
> +	if (idx >= iio_dev_opaque->attached_buffers_cnt)
> +		return -ENODEV;
> +
> +	iio_device_get(indio_dev);
> +
> +	buffer = iio_dev_opaque->attached_buffers[idx];
> +
> +	if (test_and_set_bit(IIO_BUSY_BIT_POS, &buffer->flags)) {
> +		ret = -EBUSY;
> +		goto error_iio_dev_put;
> +	}
> +
> +	ib = kzalloc(sizeof(*ib), GFP_KERNEL);
> +	if (!ib) {
> +		ret = -ENOMEM;
> +		goto error_clear_busy_bit;
> +	}
> +
> +	ib->indio_dev = indio_dev;
> +	ib->buffer = buffer;
> +
> +	fd = anon_inode_getfd("iio:buffer", &iio_buffer_chrdev_fileops,
> +			      ib, O_RDWR | O_CLOEXEC);

I wonder if we need to allow to pass flags, like e.g. O_NONBLOCK.

Something like 
https://elixir.bootlin.com/linux/latest/source/fs/signalfd.c#L288

> +	if (fd < 0) {
> +		ret = fd;
> +		goto error_free_ib;
> +	}
> +
> +	if (copy_to_user(ival, &fd, sizeof(fd))) {
> +		put_unused_fd(fd);
> +		ret = -EFAULT;
> +		goto error_free_ib;
> +	}

Here we copy back the fd, but also return it. Just return is probably 
enough.

> +
> +	return fd;
> +
> +error_free_ib:
> +	kfree(ib);
> +error_clear_busy_bit:
> +	clear_bit(IIO_BUSY_BIT_POS, &buffer->flags);
> +error_iio_dev_put:
> +	iio_device_put(indio_dev);
> +	return ret;
> +}
> [...]
> diff --git a/include/linux/iio/iio-opaque.h b/include/linux/iio/iio-opaque.h
> index b6ebc04af3e7..32addd5e790e 100644
> --- a/include/linux/iio/iio-opaque.h
> +++ b/include/linux/iio/iio-opaque.h
> @@ -9,6 +9,7 @@
>    * @event_interface:		event chrdevs associated with interrupt lines
>    * @attached_buffers:		array of buffers statically attached by the driver
>    * @attached_buffers_cnt:	number of buffers in the array of statically attached buffers
> + * @buffer_ioctl_handler:	ioctl() handler for this IIO device's buffer interface
>    * @buffer_list:		list of all buffers currently attached
>    * @channel_attr_list:		keep track of automatically created channel
>    *				attributes
> @@ -28,6 +29,7 @@ struct iio_dev_opaque {
>   	struct iio_event_interface	*event_interface;
>   	struct iio_buffer		**attached_buffers;
>   	unsigned int			attached_buffers_cnt;
> +	struct iio_ioctl_handler	*buffer_ioctl_handler;

Can we just embedded this struct so we do not have to 
allocate/deallocate it?

>   	struct list_head		buffer_list;
>   	struct list_head		channel_attr_list;
>   	struct attribute_group		chan_attr_group;
Lars-Peter Clausen Feb. 28, 2021, 8:51 a.m. UTC | #2
On 2/15/21 11:40 AM, Alexandru Ardelean wrote:
> With this change, an ioctl() call is added to open a character device for a
> buffer. The ioctl() number is 'i' 0x91, which follows the
> IIO_GET_EVENT_FD_IOCTL ioctl.
>
> The ioctl() will return an FD for the requested buffer index. The indexes
> are the same from the /sys/iio/devices/iio:deviceX/bufferY (i.e. the Y
> variable).
>
> Since there doesn't seem to be a sane way to return the FD for buffer0 to
> be the same FD for the /dev/iio:deviceX, this ioctl() will return another
> FD for buffer0 (or the first buffer). This duplicate FD will be able to
> access the same buffer object (for buffer0) as accessing directly the
> /dev/iio:deviceX chardev.
>
> Also, there is no IIO_BUFFER_GET_BUFFER_COUNT ioctl() implemented, as the
> index for each buffer (and the count) can be deduced from the
> '/sys/bus/iio/devices/iio:deviceX/bufferY' folders (i.e the number of
> bufferY folders).
>
> Used following C code to test this:
> -------------------------------------------------------------------
>
>   #include <stdio.h>
>   #include <stdlib.h>
>   #include <unistd.h>
>   #include <sys/ioctl.h>
>   #include <fcntl.h"
>   #include <errno.h>
>
>   #define IIO_BUFFER_GET_FD_IOCTL      _IOWR('i', 0x91, int)
>
> int main(int argc, char *argv[])
> {
>          int fd;
>          int fd1;
>          int ret;
>
>          if ((fd = open("/dev/iio:device0", O_RDWR))<0) {
>                  fprintf(stderr, "Error open() %d errno %d\n",fd, errno);
>                  return -1;
>          }
>
>          fprintf(stderr, "Using FD %d\n", fd);
>
>          fd1 = atoi(argv[1]);
>
>          ret = ioctl(fd, IIO_BUFFER_GET_FD_IOCTL, &fd1);
>          if (ret < 0) {
>                  fprintf(stderr, "Error for buffer %d ioctl() %d errno %d\n", fd1, ret, errno);
>                  close(fd);
>                  return -1;
>          }
>
>          fprintf(stderr, "Got FD %d\n", fd1);
>
>          close(fd1);
>          close(fd);
>
>          return 0;
> }
> -------------------------------------------------------------------
>
> Results are:
> -------------------------------------------------------------------
>   # ./test 0
>   Using FD 3
>   Got FD 4
>
>   # ./test 1
>   Using FD 3
>   Got FD 4
>
>   # ./test 2
>   Using FD 3
>   Got FD 4
>
>   # ./test 3
>   Using FD 3
>   Got FD 4
>
>   # ls /sys/bus/iio/devices/iio\:device0
>   buffer  buffer0  buffer1  buffer2  buffer3  dev
>   in_voltage_sampling_frequency  in_voltage_scale
>   in_voltage_scale_available
>   name  of_node  power  scan_elements  subsystem  uevent
> -------------------------------------------------------------------
>
> iio:device0 has some fake kfifo buffers attached to an IIO device.

For me there is one major problem with this approach. We only allow one 
application to open /dev/iio:deviceX at a time. This means we can't have 
different applications access different buffers of the same device. I 
believe this is a circuital feature.

It is possible to open the chardev, get the annonfd, close the chardev 
and keep the annonfd open. Then the next application can do the same and 
get access to a different buffer. But this has room for race conditions 
when two applications try this at the very same time.

We need to somehow address this.

I'm also not much of a fan of using ioctls to create annon fds. In part 
because all the standard mechanisms for access control no longer work.
Jonathan Cameron Feb. 28, 2021, 2:34 p.m. UTC | #3
On Sun, 28 Feb 2021 09:51:38 +0100
Lars-Peter Clausen <lars@metafoo.de> wrote:

> On 2/15/21 11:40 AM, Alexandru Ardelean wrote:
> > With this change, an ioctl() call is added to open a character device for a
> > buffer. The ioctl() number is 'i' 0x91, which follows the
> > IIO_GET_EVENT_FD_IOCTL ioctl.
> >
> > The ioctl() will return an FD for the requested buffer index. The indexes
> > are the same from the /sys/iio/devices/iio:deviceX/bufferY (i.e. the Y
> > variable).
> >
> > Since there doesn't seem to be a sane way to return the FD for buffer0 to
> > be the same FD for the /dev/iio:deviceX, this ioctl() will return another
> > FD for buffer0 (or the first buffer). This duplicate FD will be able to
> > access the same buffer object (for buffer0) as accessing directly the
> > /dev/iio:deviceX chardev.
> >
> > Also, there is no IIO_BUFFER_GET_BUFFER_COUNT ioctl() implemented, as the
> > index for each buffer (and the count) can be deduced from the
> > '/sys/bus/iio/devices/iio:deviceX/bufferY' folders (i.e the number of
> > bufferY folders).
> >
> > Used following C code to test this:
> > -------------------------------------------------------------------
> >
> >   #include <stdio.h>
> >   #include <stdlib.h>
> >   #include <unistd.h>
> >   #include <sys/ioctl.h>
> >   #include <fcntl.h"
> >   #include <errno.h>
> >
> >   #define IIO_BUFFER_GET_FD_IOCTL      _IOWR('i', 0x91, int)
> >
> > int main(int argc, char *argv[])
> > {
> >          int fd;
> >          int fd1;
> >          int ret;
> >
> >          if ((fd = open("/dev/iio:device0", O_RDWR))<0) {
> >                  fprintf(stderr, "Error open() %d errno %d\n",fd, errno);
> >                  return -1;
> >          }
> >
> >          fprintf(stderr, "Using FD %d\n", fd);
> >
> >          fd1 = atoi(argv[1]);
> >
> >          ret = ioctl(fd, IIO_BUFFER_GET_FD_IOCTL, &fd1);
> >          if (ret < 0) {
> >                  fprintf(stderr, "Error for buffer %d ioctl() %d errno %d\n", fd1, ret, errno);
> >                  close(fd);
> >                  return -1;
> >          }
> >
> >          fprintf(stderr, "Got FD %d\n", fd1);
> >
> >          close(fd1);
> >          close(fd);
> >
> >          return 0;
> > }
> > -------------------------------------------------------------------
> >
> > Results are:
> > -------------------------------------------------------------------
> >   # ./test 0
> >   Using FD 3
> >   Got FD 4
> >
> >   # ./test 1
> >   Using FD 3
> >   Got FD 4
> >
> >   # ./test 2
> >   Using FD 3
> >   Got FD 4
> >
> >   # ./test 3
> >   Using FD 3
> >   Got FD 4
> >
> >   # ls /sys/bus/iio/devices/iio\:device0
> >   buffer  buffer0  buffer1  buffer2  buffer3  dev
> >   in_voltage_sampling_frequency  in_voltage_scale
> >   in_voltage_scale_available
> >   name  of_node  power  scan_elements  subsystem  uevent
> > -------------------------------------------------------------------
> >
> > iio:device0 has some fake kfifo buffers attached to an IIO device.  
> 
> For me there is one major problem with this approach. We only allow one 
> application to open /dev/iio:deviceX at a time. This means we can't have 
> different applications access different buffers of the same device. I 
> believe this is a circuital feature.

Thats not quite true (I think - though I've not tested it).  What we don't
allow is for multiple processes to access them in an unaware fashion.
My assumption is we can rely on fork + fd passing via appropriate sockets.

> 
> It is possible to open the chardev, get the annonfd, close the chardev 
> and keep the annonfd open. Then the next application can do the same and 
> get access to a different buffer. But this has room for race conditions 
> when two applications try this at the very same time.
> 
> We need to somehow address this.

I'd count this as a bug :).  It could be safely done in a particular custom
system but in general it opens a can of worm.

> 
> I'm also not much of a fan of using ioctls to create annon fds. In part 
> because all the standard mechanisms for access control no longer work.

The inability to trivially have multiple processes open the anon fds
without care is one of the things I like most about them.

IIO drivers and interfaces really aren't designed for multiple unaware
processes to access them.  We don't have per process controls for device
wide sysfs attributes etc.  In general, it would be hard to
do due to the complexity of modeling all the interactions between the
different interfaces (events / buffers / sysfs access) in a generic fashion.

As such, the model, in my head at least, is that we only want a single
process to ever be responsible for access control.  That process can then
assign access to children or via a deliberate action (I think passing the
anon fd over a unix socket should work for example).  The intent being
that it is also responsible for mediating access to infrastructure that
multiple child processes all want to access.

As such, having one chrdev isn't a disadvantage because only one process
should ever open it at a time.  This same process also handles the
resource / control mediation.  Therefore we should only have one file
exposed for all the standard access control mechanisms.

Jonathan
Lars-Peter Clausen Feb. 28, 2021, 3:51 p.m. UTC | #4
On 2/28/21 3:34 PM, Jonathan Cameron wrote:
> On Sun, 28 Feb 2021 09:51:38 +0100
> Lars-Peter Clausen <lars@metafoo.de> wrote:
>
>> On 2/15/21 11:40 AM, Alexandru Ardelean wrote:
>>> With this change, an ioctl() call is added to open a character device for a
>>> buffer. The ioctl() number is 'i' 0x91, which follows the
>>> IIO_GET_EVENT_FD_IOCTL ioctl.
>>>
>>> The ioctl() will return an FD for the requested buffer index. The indexes
>>> are the same from the /sys/iio/devices/iio:deviceX/bufferY (i.e. the Y
>>> variable).
>>>
>>> Since there doesn't seem to be a sane way to return the FD for buffer0 to
>>> be the same FD for the /dev/iio:deviceX, this ioctl() will return another
>>> FD for buffer0 (or the first buffer). This duplicate FD will be able to
>>> access the same buffer object (for buffer0) as accessing directly the
>>> /dev/iio:deviceX chardev.
>>>
>>> Also, there is no IIO_BUFFER_GET_BUFFER_COUNT ioctl() implemented, as the
>>> index for each buffer (and the count) can be deduced from the
>>> '/sys/bus/iio/devices/iio:deviceX/bufferY' folders (i.e the number of
>>> bufferY folders).
>>>
>>> Used following C code to test this:
>>> -------------------------------------------------------------------
>>>
>>>    #include <stdio.h>
>>>    #include <stdlib.h>
>>>    #include <unistd.h>
>>>    #include <sys/ioctl.h>
>>>    #include <fcntl.h"
>>>    #include <errno.h>
>>>
>>>    #define IIO_BUFFER_GET_FD_IOCTL      _IOWR('i', 0x91, int)
>>>
>>> int main(int argc, char *argv[])
>>> {
>>>           int fd;
>>>           int fd1;
>>>           int ret;
>>>
>>>           if ((fd = open("/dev/iio:device0", O_RDWR))<0) {
>>>                   fprintf(stderr, "Error open() %d errno %d\n",fd, errno);
>>>                   return -1;
>>>           }
>>>
>>>           fprintf(stderr, "Using FD %d\n", fd);
>>>
>>>           fd1 = atoi(argv[1]);
>>>
>>>           ret = ioctl(fd, IIO_BUFFER_GET_FD_IOCTL, &fd1);
>>>           if (ret < 0) {
>>>                   fprintf(stderr, "Error for buffer %d ioctl() %d errno %d\n", fd1, ret, errno);
>>>                   close(fd);
>>>                   return -1;
>>>           }
>>>
>>>           fprintf(stderr, "Got FD %d\n", fd1);
>>>
>>>           close(fd1);
>>>           close(fd);
>>>
>>>           return 0;
>>> }
>>> -------------------------------------------------------------------
>>>
>>> Results are:
>>> -------------------------------------------------------------------
>>>    # ./test 0
>>>    Using FD 3
>>>    Got FD 4
>>>
>>>    # ./test 1
>>>    Using FD 3
>>>    Got FD 4
>>>
>>>    # ./test 2
>>>    Using FD 3
>>>    Got FD 4
>>>
>>>    # ./test 3
>>>    Using FD 3
>>>    Got FD 4
>>>
>>>    # ls /sys/bus/iio/devices/iio\:device0
>>>    buffer  buffer0  buffer1  buffer2  buffer3  dev
>>>    in_voltage_sampling_frequency  in_voltage_scale
>>>    in_voltage_scale_available
>>>    name  of_node  power  scan_elements  subsystem  uevent
>>> -------------------------------------------------------------------
>>>
>>> iio:device0 has some fake kfifo buffers attached to an IIO device.
>> For me there is one major problem with this approach. We only allow one
>> application to open /dev/iio:deviceX at a time. This means we can't have
>> different applications access different buffers of the same device. I
>> believe this is a circuital feature.
> Thats not quite true (I think - though I've not tested it).  What we don't
> allow is for multiple processes to access them in an unaware fashion.
> My assumption is we can rely on fork + fd passing via appropriate sockets.
>
>> It is possible to open the chardev, get the annonfd, close the chardev
>> and keep the annonfd open. Then the next application can do the same and
>> get access to a different buffer. But this has room for race conditions
>> when two applications try this at the very same time.
>>
>> We need to somehow address this.
> I'd count this as a bug :).  It could be safely done in a particular custom
> system but in general it opens a can of worm.
>
>> I'm also not much of a fan of using ioctls to create annon fds. In part
>> because all the standard mechanisms for access control no longer work.
> The inability to trivially have multiple processes open the anon fds
> without care is one of the things I like most about them.
>
> IIO drivers and interfaces really aren't designed for multiple unaware
> processes to access them.  We don't have per process controls for device
> wide sysfs attributes etc.  In general, it would be hard to
> do due to the complexity of modeling all the interactions between the
> different interfaces (events / buffers / sysfs access) in a generic fashion.
>
> As such, the model, in my head at least, is that we only want a single
> process to ever be responsible for access control.  That process can then
> assign access to children or via a deliberate action (I think passing the
> anon fd over a unix socket should work for example).  The intent being
> that it is also responsible for mediating access to infrastructure that
> multiple child processes all want to access.
>
> As such, having one chrdev isn't a disadvantage because only one process
> should ever open it at a time.  This same process also handles the
> resource / control mediation.  Therefore we should only have one file
> exposed for all the standard access control mechanisms.
>
Hm, I see your point, but I'm not convinced.

Having to have explicit synchronization makes it difficult to mix and 
match. E.g. at ADI a popular use case for testing was to run some signal 
generator application on the TX buffer and some signal analyzer 
application on the RX buffer.

Both can be launched independently and there can be different types of 
generator and analyzer applications. Having to have a 3rd application to 
arbitrate access makes this quite cumbersome. And I'm afraid that in 
reality people might just stick with the two devices model just to avoid 
this restriction.

- Lars
Jonathan Cameron Feb. 28, 2021, 5:27 p.m. UTC | #5
On Sun, 28 Feb 2021 16:51:51 +0100
Lars-Peter Clausen <lars@metafoo.de> wrote:

> On 2/28/21 3:34 PM, Jonathan Cameron wrote:
> > On Sun, 28 Feb 2021 09:51:38 +0100
> > Lars-Peter Clausen <lars@metafoo.de> wrote:
> >  
> >> On 2/15/21 11:40 AM, Alexandru Ardelean wrote:  
> >>> With this change, an ioctl() call is added to open a character device for a
> >>> buffer. The ioctl() number is 'i' 0x91, which follows the
> >>> IIO_GET_EVENT_FD_IOCTL ioctl.
> >>>
> >>> The ioctl() will return an FD for the requested buffer index. The indexes
> >>> are the same from the /sys/iio/devices/iio:deviceX/bufferY (i.e. the Y
> >>> variable).
> >>>
> >>> Since there doesn't seem to be a sane way to return the FD for buffer0 to
> >>> be the same FD for the /dev/iio:deviceX, this ioctl() will return another
> >>> FD for buffer0 (or the first buffer). This duplicate FD will be able to
> >>> access the same buffer object (for buffer0) as accessing directly the
> >>> /dev/iio:deviceX chardev.
> >>>
> >>> Also, there is no IIO_BUFFER_GET_BUFFER_COUNT ioctl() implemented, as the
> >>> index for each buffer (and the count) can be deduced from the
> >>> '/sys/bus/iio/devices/iio:deviceX/bufferY' folders (i.e the number of
> >>> bufferY folders).
> >>>
> >>> Used following C code to test this:
> >>> -------------------------------------------------------------------
> >>>
> >>>    #include <stdio.h>
> >>>    #include <stdlib.h>
> >>>    #include <unistd.h>
> >>>    #include <sys/ioctl.h>
> >>>    #include <fcntl.h"
> >>>    #include <errno.h>
> >>>
> >>>    #define IIO_BUFFER_GET_FD_IOCTL      _IOWR('i', 0x91, int)
> >>>
> >>> int main(int argc, char *argv[])
> >>> {
> >>>           int fd;
> >>>           int fd1;
> >>>           int ret;
> >>>
> >>>           if ((fd = open("/dev/iio:device0", O_RDWR))<0) {
> >>>                   fprintf(stderr, "Error open() %d errno %d\n",fd, errno);
> >>>                   return -1;
> >>>           }
> >>>
> >>>           fprintf(stderr, "Using FD %d\n", fd);
> >>>
> >>>           fd1 = atoi(argv[1]);
> >>>
> >>>           ret = ioctl(fd, IIO_BUFFER_GET_FD_IOCTL, &fd1);
> >>>           if (ret < 0) {
> >>>                   fprintf(stderr, "Error for buffer %d ioctl() %d errno %d\n", fd1, ret, errno);
> >>>                   close(fd);
> >>>                   return -1;
> >>>           }
> >>>
> >>>           fprintf(stderr, "Got FD %d\n", fd1);
> >>>
> >>>           close(fd1);
> >>>           close(fd);
> >>>
> >>>           return 0;
> >>> }
> >>> -------------------------------------------------------------------
> >>>
> >>> Results are:
> >>> -------------------------------------------------------------------
> >>>    # ./test 0
> >>>    Using FD 3
> >>>    Got FD 4
> >>>
> >>>    # ./test 1
> >>>    Using FD 3
> >>>    Got FD 4
> >>>
> >>>    # ./test 2
> >>>    Using FD 3
> >>>    Got FD 4
> >>>
> >>>    # ./test 3
> >>>    Using FD 3
> >>>    Got FD 4
> >>>
> >>>    # ls /sys/bus/iio/devices/iio\:device0
> >>>    buffer  buffer0  buffer1  buffer2  buffer3  dev
> >>>    in_voltage_sampling_frequency  in_voltage_scale
> >>>    in_voltage_scale_available
> >>>    name  of_node  power  scan_elements  subsystem  uevent
> >>> -------------------------------------------------------------------
> >>>
> >>> iio:device0 has some fake kfifo buffers attached to an IIO device.  
> >> For me there is one major problem with this approach. We only allow one
> >> application to open /dev/iio:deviceX at a time. This means we can't have
> >> different applications access different buffers of the same device. I
> >> believe this is a circuital feature.  
> > Thats not quite true (I think - though I've not tested it).  What we don't
> > allow is for multiple processes to access them in an unaware fashion.
> > My assumption is we can rely on fork + fd passing via appropriate sockets.
> >  
> >> It is possible to open the chardev, get the annonfd, close the chardev
> >> and keep the annonfd open. Then the next application can do the same and
> >> get access to a different buffer. But this has room for race conditions
> >> when two applications try this at the very same time.
> >>
> >> We need to somehow address this.  
> > I'd count this as a bug :).  It could be safely done in a particular custom
> > system but in general it opens a can of worm.
> >  
> >> I'm also not much of a fan of using ioctls to create annon fds. In part
> >> because all the standard mechanisms for access control no longer work.  
> > The inability to trivially have multiple processes open the anon fds
> > without care is one of the things I like most about them.
> >
> > IIO drivers and interfaces really aren't designed for multiple unaware
> > processes to access them.  We don't have per process controls for device
> > wide sysfs attributes etc.  In general, it would be hard to
> > do due to the complexity of modeling all the interactions between the
> > different interfaces (events / buffers / sysfs access) in a generic fashion.
> >
> > As such, the model, in my head at least, is that we only want a single
> > process to ever be responsible for access control.  That process can then
> > assign access to children or via a deliberate action (I think passing the
> > anon fd over a unix socket should work for example).  The intent being
> > that it is also responsible for mediating access to infrastructure that
> > multiple child processes all want to access.
> >
> > As such, having one chrdev isn't a disadvantage because only one process
> > should ever open it at a time.  This same process also handles the
> > resource / control mediation.  Therefore we should only have one file
> > exposed for all the standard access control mechanisms.
> >  
> Hm, I see your point, but I'm not convinced.
> 
> Having to have explicit synchronization makes it difficult to mix and 
> match. E.g. at ADI a popular use case for testing was to run some signal 
> generator application on the TX buffer and some signal analyzer 
> application on the RX buffer.
> 
> Both can be launched independently and there can be different types of 
> generator and analyzer applications. Having to have a 3rd application to 
> arbitrate access makes this quite cumbersome. And I'm afraid that in 
> reality people might just stick with the two devices model just to avoid 
> this restriction.

I'd argue that's a problem best tackled in a library - though it's a bit
fiddly.  It ought to be possible to make it invisible that this level
of sharing is going on.   The management process you describe would probably
be a thread running inside the first process to try and access a given device.
A second process failing to open the file with -EBUSY then connects to
appropriate socket (via path in /tmp or similar) and asks for the FD.
There are race conditions that might make it fail, but a retry loop should
deal with those.

I agree people might just stick to a two device model and if the devices
are independent enough I'm not sure that is the wrong way to approach the
problem.  It represents the independence and that the driver is being careful
that it both can and is safely handle independent simultaneous accessors.
We are always going to have some drivers doing that anyway because they've
already been doing that for years.

J

> 
> - Lars
>
Alexandru Ardelean Feb. 28, 2021, 6:04 p.m. UTC | #6
On Sun, Feb 28, 2021 at 9:58 AM Lars-Peter Clausen <lars@metafoo.de> wrote:
>
> On 2/15/21 11:40 AM, Alexandru Ardelean wrote:
> > [...]
> >   /**
> >    * iio_buffer_wakeup_poll - Wakes up the buffer waitqueue
> >    * @indio_dev: The IIO device
> > @@ -1343,6 +1371,96 @@ static void iio_buffer_unregister_legacy_sysfs_groups(struct iio_dev *indio_dev)
> >       kfree(iio_dev_opaque->legacy_scan_el_group.attrs);
> >   }
> >
> > [...]
> > +static long iio_device_buffer_getfd(struct iio_dev *indio_dev, unsigned long arg)
> > +{
> > +     struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
> > +     int __user *ival = (int __user *)arg;
> > +     struct iio_dev_buffer_pair *ib;
> > +     struct iio_buffer *buffer;
> > +     int fd, idx, ret;
> > +
> > +     if (copy_from_user(&idx, ival, sizeof(idx)))
> > +             return -EFAULT;
>
> If we only want to pass an int, we can pass that directly, no need to
> pass it as a pointer.
>
> int fd = arg;

I guess I can simplify this a bit.

>
> > +
> > +     if (idx >= iio_dev_opaque->attached_buffers_cnt)
> > +             return -ENODEV;
> > +
> > +     iio_device_get(indio_dev);
> > +
> > +     buffer = iio_dev_opaque->attached_buffers[idx];
> > +
> > +     if (test_and_set_bit(IIO_BUSY_BIT_POS, &buffer->flags)) {
> > +             ret = -EBUSY;
> > +             goto error_iio_dev_put;
> > +     }
> > +
> > +     ib = kzalloc(sizeof(*ib), GFP_KERNEL);
> > +     if (!ib) {
> > +             ret = -ENOMEM;
> > +             goto error_clear_busy_bit;
> > +     }
> > +
> > +     ib->indio_dev = indio_dev;
> > +     ib->buffer = buffer;
> > +
> > +     fd = anon_inode_getfd("iio:buffer", &iio_buffer_chrdev_fileops,
> > +                           ib, O_RDWR | O_CLOEXEC);
>
> I wonder if we need to allow to pass flags, like e.g. O_NONBLOCK.
>
> Something like
> https://elixir.bootlin.com/linux/latest/source/fs/signalfd.c#L288

Umm, no idea.
I guess we could.
Would a syscall make more sense than an ioctl() here?
I guess for the ioctl() case we would need to change the type (of the
data) to some sort of

struct iio_buffer_get_fd {
      unsigned int buffer_idx;
      unsigned int fd_flags;
};

Or do we just let userspace use fcntl() to change flags to O_NONBLOCK ?

>
> > +     if (fd < 0) {
> > +             ret = fd;
> > +             goto error_free_ib;
> > +     }
> > +
> > +     if (copy_to_user(ival, &fd, sizeof(fd))) {
> > +             put_unused_fd(fd);
> > +             ret = -EFAULT;
> > +             goto error_free_ib;
> > +     }
>
> Here we copy back the fd, but also return it. Just return is probably
> enough.

Hmm.
Yes, it is a bit duplicate.
But it is a good point. Maybe we should return 0 to userspace.

>
> > +
> > +     return fd;
> > +
> > +error_free_ib:
> > +     kfree(ib);
> > +error_clear_busy_bit:
> > +     clear_bit(IIO_BUSY_BIT_POS, &buffer->flags);
> > +error_iio_dev_put:
> > +     iio_device_put(indio_dev);
> > +     return ret;
> > +}
> > [...]
> > diff --git a/include/linux/iio/iio-opaque.h b/include/linux/iio/iio-opaque.h
> > index b6ebc04af3e7..32addd5e790e 100644
> > --- a/include/linux/iio/iio-opaque.h
> > +++ b/include/linux/iio/iio-opaque.h
> > @@ -9,6 +9,7 @@
> >    * @event_interface:                event chrdevs associated with interrupt lines
> >    * @attached_buffers:               array of buffers statically attached by the driver
> >    * @attached_buffers_cnt:   number of buffers in the array of statically attached buffers
> > + * @buffer_ioctl_handler:    ioctl() handler for this IIO device's buffer interface
> >    * @buffer_list:            list of all buffers currently attached
> >    * @channel_attr_list:              keep track of automatically created channel
> >    *                          attributes
> > @@ -28,6 +29,7 @@ struct iio_dev_opaque {
> >       struct iio_event_interface      *event_interface;
> >       struct iio_buffer               **attached_buffers;
> >       unsigned int                    attached_buffers_cnt;
> > +     struct iio_ioctl_handler        *buffer_ioctl_handler;
>
> Can we just embedded this struct so we do not have to
> allocate/deallocate it?

Unfortunately we can't.
The type of ' struct iio_ioctl_handler ' is stored in iio_core.h.
So, we don't know the size here. So we need to keep it as a pointer
and allocate it.
It is a bit of an unfortunate consequence of the design of this
generic ioctl() registering.

>
> >       struct list_head                buffer_list;
> >       struct list_head                channel_attr_list;
> >       struct attribute_group          chan_attr_group;
>
>
Alexandru Ardelean Feb. 28, 2021, 6:09 p.m. UTC | #7
On Sun, Feb 28, 2021 at 5:54 PM Lars-Peter Clausen <lars@metafoo.de> wrote:
>
> On 2/28/21 3:34 PM, Jonathan Cameron wrote:
> > On Sun, 28 Feb 2021 09:51:38 +0100
> > Lars-Peter Clausen <lars@metafoo.de> wrote:
> >
> >> On 2/15/21 11:40 AM, Alexandru Ardelean wrote:
> >>> With this change, an ioctl() call is added to open a character device for a
> >>> buffer. The ioctl() number is 'i' 0x91, which follows the
> >>> IIO_GET_EVENT_FD_IOCTL ioctl.
> >>>
> >>> The ioctl() will return an FD for the requested buffer index. The indexes
> >>> are the same from the /sys/iio/devices/iio:deviceX/bufferY (i.e. the Y
> >>> variable).
> >>>
> >>> Since there doesn't seem to be a sane way to return the FD for buffer0 to
> >>> be the same FD for the /dev/iio:deviceX, this ioctl() will return another
> >>> FD for buffer0 (or the first buffer). This duplicate FD will be able to
> >>> access the same buffer object (for buffer0) as accessing directly the
> >>> /dev/iio:deviceX chardev.
> >>>
> >>> Also, there is no IIO_BUFFER_GET_BUFFER_COUNT ioctl() implemented, as the
> >>> index for each buffer (and the count) can be deduced from the
> >>> '/sys/bus/iio/devices/iio:deviceX/bufferY' folders (i.e the number of
> >>> bufferY folders).
> >>>
> >>> Used following C code to test this:
> >>> -------------------------------------------------------------------
> >>>
> >>>    #include <stdio.h>
> >>>    #include <stdlib.h>
> >>>    #include <unistd.h>
> >>>    #include <sys/ioctl.h>
> >>>    #include <fcntl.h"
> >>>    #include <errno.h>
> >>>
> >>>    #define IIO_BUFFER_GET_FD_IOCTL      _IOWR('i', 0x91, int)
> >>>
> >>> int main(int argc, char *argv[])
> >>> {
> >>>           int fd;
> >>>           int fd1;
> >>>           int ret;
> >>>
> >>>           if ((fd = open("/dev/iio:device0", O_RDWR))<0) {
> >>>                   fprintf(stderr, "Error open() %d errno %d\n",fd, errno);
> >>>                   return -1;
> >>>           }
> >>>
> >>>           fprintf(stderr, "Using FD %d\n", fd);
> >>>
> >>>           fd1 = atoi(argv[1]);
> >>>
> >>>           ret = ioctl(fd, IIO_BUFFER_GET_FD_IOCTL, &fd1);
> >>>           if (ret < 0) {
> >>>                   fprintf(stderr, "Error for buffer %d ioctl() %d errno %d\n", fd1, ret, errno);
> >>>                   close(fd);
> >>>                   return -1;
> >>>           }
> >>>
> >>>           fprintf(stderr, "Got FD %d\n", fd1);
> >>>
> >>>           close(fd1);
> >>>           close(fd);
> >>>
> >>>           return 0;
> >>> }
> >>> -------------------------------------------------------------------
> >>>
> >>> Results are:
> >>> -------------------------------------------------------------------
> >>>    # ./test 0
> >>>    Using FD 3
> >>>    Got FD 4
> >>>
> >>>    # ./test 1
> >>>    Using FD 3
> >>>    Got FD 4
> >>>
> >>>    # ./test 2
> >>>    Using FD 3
> >>>    Got FD 4
> >>>
> >>>    # ./test 3
> >>>    Using FD 3
> >>>    Got FD 4
> >>>
> >>>    # ls /sys/bus/iio/devices/iio\:device0
> >>>    buffer  buffer0  buffer1  buffer2  buffer3  dev
> >>>    in_voltage_sampling_frequency  in_voltage_scale
> >>>    in_voltage_scale_available
> >>>    name  of_node  power  scan_elements  subsystem  uevent
> >>> -------------------------------------------------------------------
> >>>
> >>> iio:device0 has some fake kfifo buffers attached to an IIO device.
> >> For me there is one major problem with this approach. We only allow one
> >> application to open /dev/iio:deviceX at a time. This means we can't have
> >> different applications access different buffers of the same device. I
> >> believe this is a circuital feature.
> > Thats not quite true (I think - though I've not tested it).  What we don't
> > allow is for multiple processes to access them in an unaware fashion.
> > My assumption is we can rely on fork + fd passing via appropriate sockets.
> >
> >> It is possible to open the chardev, get the annonfd, close the chardev
> >> and keep the annonfd open. Then the next application can do the same and
> >> get access to a different buffer. But this has room for race conditions
> >> when two applications try this at the very same time.
> >>
> >> We need to somehow address this.
> > I'd count this as a bug :).  It could be safely done in a particular custom
> > system but in general it opens a can of worm.

I'll take a look at this.

> >
> >> I'm also not much of a fan of using ioctls to create annon fds. In part
> >> because all the standard mechanisms for access control no longer work.
> > The inability to trivially have multiple processes open the anon fds
> > without care is one of the things I like most about them.
> >
> > IIO drivers and interfaces really aren't designed for multiple unaware
> > processes to access them.  We don't have per process controls for device
> > wide sysfs attributes etc.  In general, it would be hard to
> > do due to the complexity of modeling all the interactions between the
> > different interfaces (events / buffers / sysfs access) in a generic fashion.
> >
> > As such, the model, in my head at least, is that we only want a single
> > process to ever be responsible for access control.  That process can then
> > assign access to children or via a deliberate action (I think passing the
> > anon fd over a unix socket should work for example).  The intent being
> > that it is also responsible for mediating access to infrastructure that
> > multiple child processes all want to access.
> >
> > As such, having one chrdev isn't a disadvantage because only one process
> > should ever open it at a time.  This same process also handles the
> > resource / control mediation.  Therefore we should only have one file
> > exposed for all the standard access control mechanisms.
> >
> Hm, I see your point, but I'm not convinced.
>
> Having to have explicit synchronization makes it difficult to mix and
> match. E.g. at ADI a popular use case for testing was to run some signal
> generator application on the TX buffer and some signal analyzer
> application on the RX buffer.
>
> Both can be launched independently and there can be different types of
> generator and analyzer applications. Having to have a 3rd application to
> arbitrate access makes this quite cumbersome. And I'm afraid that in
> reality people might just stick with the two devices model just to avoid
> this restriction.

I'm neutral on this part of the debate.
I feel this may be some older discussion being refreshed here (it's
just a personal feeling).

I can try to accommodate a solution if something (else) is agreed.
Though at this point it may be a little slower.
I'm no longer an ADI employee, so it may take me a little longer to
test some things.

>
> - Lars
>
Alexandru Ardelean March 6, 2021, 5 p.m. UTC | #8
On Sun, Feb 28, 2021 at 9:00 PM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> On Sun, 28 Feb 2021 16:51:51 +0100
> Lars-Peter Clausen <lars@metafoo.de> wrote:
>
> > On 2/28/21 3:34 PM, Jonathan Cameron wrote:
> > > On Sun, 28 Feb 2021 09:51:38 +0100
> > > Lars-Peter Clausen <lars@metafoo.de> wrote:
> > >
> > >> On 2/15/21 11:40 AM, Alexandru Ardelean wrote:
> > >>> With this change, an ioctl() call is added to open a character device for a
> > >>> buffer. The ioctl() number is 'i' 0x91, which follows the
> > >>> IIO_GET_EVENT_FD_IOCTL ioctl.
> > >>>
> > >>> The ioctl() will return an FD for the requested buffer index. The indexes
> > >>> are the same from the /sys/iio/devices/iio:deviceX/bufferY (i.e. the Y
> > >>> variable).
> > >>>
> > >>> Since there doesn't seem to be a sane way to return the FD for buffer0 to
> > >>> be the same FD for the /dev/iio:deviceX, this ioctl() will return another
> > >>> FD for buffer0 (or the first buffer). This duplicate FD will be able to
> > >>> access the same buffer object (for buffer0) as accessing directly the
> > >>> /dev/iio:deviceX chardev.
> > >>>
> > >>> Also, there is no IIO_BUFFER_GET_BUFFER_COUNT ioctl() implemented, as the
> > >>> index for each buffer (and the count) can be deduced from the
> > >>> '/sys/bus/iio/devices/iio:deviceX/bufferY' folders (i.e the number of
> > >>> bufferY folders).
> > >>>
> > >>> Used following C code to test this:
> > >>> -------------------------------------------------------------------
> > >>>
> > >>>    #include <stdio.h>
> > >>>    #include <stdlib.h>
> > >>>    #include <unistd.h>
> > >>>    #include <sys/ioctl.h>
> > >>>    #include <fcntl.h"
> > >>>    #include <errno.h>
> > >>>
> > >>>    #define IIO_BUFFER_GET_FD_IOCTL      _IOWR('i', 0x91, int)
> > >>>
> > >>> int main(int argc, char *argv[])
> > >>> {
> > >>>           int fd;
> > >>>           int fd1;
> > >>>           int ret;
> > >>>
> > >>>           if ((fd = open("/dev/iio:device0", O_RDWR))<0) {
> > >>>                   fprintf(stderr, "Error open() %d errno %d\n",fd, errno);
> > >>>                   return -1;
> > >>>           }
> > >>>
> > >>>           fprintf(stderr, "Using FD %d\n", fd);
> > >>>
> > >>>           fd1 = atoi(argv[1]);
> > >>>
> > >>>           ret = ioctl(fd, IIO_BUFFER_GET_FD_IOCTL, &fd1);
> > >>>           if (ret < 0) {
> > >>>                   fprintf(stderr, "Error for buffer %d ioctl() %d errno %d\n", fd1, ret, errno);
> > >>>                   close(fd);
> > >>>                   return -1;
> > >>>           }
> > >>>
> > >>>           fprintf(stderr, "Got FD %d\n", fd1);
> > >>>
> > >>>           close(fd1);
> > >>>           close(fd);
> > >>>
> > >>>           return 0;
> > >>> }
> > >>> -------------------------------------------------------------------
> > >>>
> > >>> Results are:
> > >>> -------------------------------------------------------------------
> > >>>    # ./test 0
> > >>>    Using FD 3
> > >>>    Got FD 4
> > >>>
> > >>>    # ./test 1
> > >>>    Using FD 3
> > >>>    Got FD 4
> > >>>
> > >>>    # ./test 2
> > >>>    Using FD 3
> > >>>    Got FD 4
> > >>>
> > >>>    # ./test 3
> > >>>    Using FD 3
> > >>>    Got FD 4
> > >>>
> > >>>    # ls /sys/bus/iio/devices/iio\:device0
> > >>>    buffer  buffer0  buffer1  buffer2  buffer3  dev
> > >>>    in_voltage_sampling_frequency  in_voltage_scale
> > >>>    in_voltage_scale_available
> > >>>    name  of_node  power  scan_elements  subsystem  uevent
> > >>> -------------------------------------------------------------------
> > >>>
> > >>> iio:device0 has some fake kfifo buffers attached to an IIO device.
> > >> For me there is one major problem with this approach. We only allow one
> > >> application to open /dev/iio:deviceX at a time. This means we can't have
> > >> different applications access different buffers of the same device. I
> > >> believe this is a circuital feature.
> > > Thats not quite true (I think - though I've not tested it).  What we don't
> > > allow is for multiple processes to access them in an unaware fashion.
> > > My assumption is we can rely on fork + fd passing via appropriate sockets.
> > >
> > >> It is possible to open the chardev, get the annonfd, close the chardev
> > >> and keep the annonfd open. Then the next application can do the same and
> > >> get access to a different buffer. But this has room for race conditions
> > >> when two applications try this at the very same time.
> > >>
> > >> We need to somehow address this.
> > > I'd count this as a bug :).  It could be safely done in a particular custom
> > > system but in general it opens a can of worm.
> > >
> > >> I'm also not much of a fan of using ioctls to create annon fds. In part
> > >> because all the standard mechanisms for access control no longer work.
> > > The inability to trivially have multiple processes open the anon fds
> > > without care is one of the things I like most about them.
> > >
> > > IIO drivers and interfaces really aren't designed for multiple unaware
> > > processes to access them.  We don't have per process controls for device
> > > wide sysfs attributes etc.  In general, it would be hard to
> > > do due to the complexity of modeling all the interactions between the
> > > different interfaces (events / buffers / sysfs access) in a generic fashion.
> > >
> > > As such, the model, in my head at least, is that we only want a single
> > > process to ever be responsible for access control.  That process can then
> > > assign access to children or via a deliberate action (I think passing the
> > > anon fd over a unix socket should work for example).  The intent being
> > > that it is also responsible for mediating access to infrastructure that
> > > multiple child processes all want to access.
> > >
> > > As such, having one chrdev isn't a disadvantage because only one process
> > > should ever open it at a time.  This same process also handles the
> > > resource / control mediation.  Therefore we should only have one file
> > > exposed for all the standard access control mechanisms.
> > >
> > Hm, I see your point, but I'm not convinced.
> >
> > Having to have explicit synchronization makes it difficult to mix and
> > match. E.g. at ADI a popular use case for testing was to run some signal
> > generator application on the TX buffer and some signal analyzer
> > application on the RX buffer.
> >
> > Both can be launched independently and there can be different types of
> > generator and analyzer applications. Having to have a 3rd application to
> > arbitrate access makes this quite cumbersome. And I'm afraid that in
> > reality people might just stick with the two devices model just to avoid
> > this restriction.
>
> I'd argue that's a problem best tackled in a library - though it's a bit
> fiddly.  It ought to be possible to make it invisible that this level
> of sharing is going on.   The management process you describe would probably
> be a thread running inside the first process to try and access a given device.
> A second process failing to open the file with -EBUSY then connects to
> appropriate socket (via path in /tmp or similar) and asks for the FD.
> There are race conditions that might make it fail, but a retry loop should
> deal with those.
>
> I agree people might just stick to a two device model and if the devices
> are independent enough I'm not sure that is the wrong way to approach the
> problem.  It represents the independence and that the driver is being careful
> that it both can and is safely handle independent simultaneous accessors.
> We are always going to have some drivers doing that anyway because they've
> already been doing that for years.
>

This is the last of the 3 patches that I need to re-spin after Lars' review.
I have a good handle on the small stuff.

I'm not sure about the race-condition about which Lars was talking about.
I mean, I get the problem, but is it a problem that we should fix in the kernel?

I'm sensing that Jonathan's preference is to keep things mostly as the
current implementation.
I'll probably leave this alone for a few days.
And I'll prepare some patches for the tweaks Lars suggested (adding
O_NONBLOCK and doing things a bit differently with the FD).
I'll send those in the next few days.

> J
>
> >
> > - Lars
> >
>
Jonathan Cameron March 7, 2021, 12:13 p.m. UTC | #9
On Sat, 6 Mar 2021 19:00:52 +0200
Alexandru Ardelean <ardeleanalex@gmail.com> wrote:

> On Sun, Feb 28, 2021 at 9:00 PM Jonathan Cameron
> <Jonathan.Cameron@huawei.com> wrote:
> >
> > On Sun, 28 Feb 2021 16:51:51 +0100
> > Lars-Peter Clausen <lars@metafoo.de> wrote:
> >  
> > > On 2/28/21 3:34 PM, Jonathan Cameron wrote:  
> > > > On Sun, 28 Feb 2021 09:51:38 +0100
> > > > Lars-Peter Clausen <lars@metafoo.de> wrote:
> > > >  
> > > >> On 2/15/21 11:40 AM, Alexandru Ardelean wrote:  
> > > >>> With this change, an ioctl() call is added to open a character device for a
> > > >>> buffer. The ioctl() number is 'i' 0x91, which follows the
> > > >>> IIO_GET_EVENT_FD_IOCTL ioctl.
> > > >>>
> > > >>> The ioctl() will return an FD for the requested buffer index. The indexes
> > > >>> are the same from the /sys/iio/devices/iio:deviceX/bufferY (i.e. the Y
> > > >>> variable).
> > > >>>
> > > >>> Since there doesn't seem to be a sane way to return the FD for buffer0 to
> > > >>> be the same FD for the /dev/iio:deviceX, this ioctl() will return another
> > > >>> FD for buffer0 (or the first buffer). This duplicate FD will be able to
> > > >>> access the same buffer object (for buffer0) as accessing directly the
> > > >>> /dev/iio:deviceX chardev.
> > > >>>
> > > >>> Also, there is no IIO_BUFFER_GET_BUFFER_COUNT ioctl() implemented, as the
> > > >>> index for each buffer (and the count) can be deduced from the
> > > >>> '/sys/bus/iio/devices/iio:deviceX/bufferY' folders (i.e the number of
> > > >>> bufferY folders).
> > > >>>
> > > >>> Used following C code to test this:
> > > >>> -------------------------------------------------------------------
> > > >>>
> > > >>>    #include <stdio.h>
> > > >>>    #include <stdlib.h>
> > > >>>    #include <unistd.h>
> > > >>>    #include <sys/ioctl.h>
> > > >>>    #include <fcntl.h"
> > > >>>    #include <errno.h>
> > > >>>
> > > >>>    #define IIO_BUFFER_GET_FD_IOCTL      _IOWR('i', 0x91, int)
> > > >>>
> > > >>> int main(int argc, char *argv[])
> > > >>> {
> > > >>>           int fd;
> > > >>>           int fd1;
> > > >>>           int ret;
> > > >>>
> > > >>>           if ((fd = open("/dev/iio:device0", O_RDWR))<0) {
> > > >>>                   fprintf(stderr, "Error open() %d errno %d\n",fd, errno);
> > > >>>                   return -1;
> > > >>>           }
> > > >>>
> > > >>>           fprintf(stderr, "Using FD %d\n", fd);
> > > >>>
> > > >>>           fd1 = atoi(argv[1]);
> > > >>>
> > > >>>           ret = ioctl(fd, IIO_BUFFER_GET_FD_IOCTL, &fd1);
> > > >>>           if (ret < 0) {
> > > >>>                   fprintf(stderr, "Error for buffer %d ioctl() %d errno %d\n", fd1, ret, errno);
> > > >>>                   close(fd);
> > > >>>                   return -1;
> > > >>>           }
> > > >>>
> > > >>>           fprintf(stderr, "Got FD %d\n", fd1);
> > > >>>
> > > >>>           close(fd1);
> > > >>>           close(fd);
> > > >>>
> > > >>>           return 0;
> > > >>> }
> > > >>> -------------------------------------------------------------------
> > > >>>
> > > >>> Results are:
> > > >>> -------------------------------------------------------------------
> > > >>>    # ./test 0
> > > >>>    Using FD 3
> > > >>>    Got FD 4
> > > >>>
> > > >>>    # ./test 1
> > > >>>    Using FD 3
> > > >>>    Got FD 4
> > > >>>
> > > >>>    # ./test 2
> > > >>>    Using FD 3
> > > >>>    Got FD 4
> > > >>>
> > > >>>    # ./test 3
> > > >>>    Using FD 3
> > > >>>    Got FD 4
> > > >>>
> > > >>>    # ls /sys/bus/iio/devices/iio\:device0
> > > >>>    buffer  buffer0  buffer1  buffer2  buffer3  dev
> > > >>>    in_voltage_sampling_frequency  in_voltage_scale
> > > >>>    in_voltage_scale_available
> > > >>>    name  of_node  power  scan_elements  subsystem  uevent
> > > >>> -------------------------------------------------------------------
> > > >>>
> > > >>> iio:device0 has some fake kfifo buffers attached to an IIO device.  
> > > >> For me there is one major problem with this approach. We only allow one
> > > >> application to open /dev/iio:deviceX at a time. This means we can't have
> > > >> different applications access different buffers of the same device. I
> > > >> believe this is a circuital feature.  
> > > > Thats not quite true (I think - though I've not tested it).  What we don't
> > > > allow is for multiple processes to access them in an unaware fashion.
> > > > My assumption is we can rely on fork + fd passing via appropriate sockets.
> > > >  
> > > >> It is possible to open the chardev, get the annonfd, close the chardev
> > > >> and keep the annonfd open. Then the next application can do the same and
> > > >> get access to a different buffer. But this has room for race conditions
> > > >> when two applications try this at the very same time.
> > > >>
> > > >> We need to somehow address this.  
> > > > I'd count this as a bug :).  It could be safely done in a particular custom
> > > > system but in general it opens a can of worm.
> > > >  
> > > >> I'm also not much of a fan of using ioctls to create annon fds. In part
> > > >> because all the standard mechanisms for access control no longer work.  
> > > > The inability to trivially have multiple processes open the anon fds
> > > > without care is one of the things I like most about them.
> > > >
> > > > IIO drivers and interfaces really aren't designed for multiple unaware
> > > > processes to access them.  We don't have per process controls for device
> > > > wide sysfs attributes etc.  In general, it would be hard to
> > > > do due to the complexity of modeling all the interactions between the
> > > > different interfaces (events / buffers / sysfs access) in a generic fashion.
> > > >
> > > > As such, the model, in my head at least, is that we only want a single
> > > > process to ever be responsible for access control.  That process can then
> > > > assign access to children or via a deliberate action (I think passing the
> > > > anon fd over a unix socket should work for example).  The intent being
> > > > that it is also responsible for mediating access to infrastructure that
> > > > multiple child processes all want to access.
> > > >
> > > > As such, having one chrdev isn't a disadvantage because only one process
> > > > should ever open it at a time.  This same process also handles the
> > > > resource / control mediation.  Therefore we should only have one file
> > > > exposed for all the standard access control mechanisms.
> > > >  
> > > Hm, I see your point, but I'm not convinced.
> > >
> > > Having to have explicit synchronization makes it difficult to mix and
> > > match. E.g. at ADI a popular use case for testing was to run some signal
> > > generator application on the TX buffer and some signal analyzer
> > > application on the RX buffer.
> > >
> > > Both can be launched independently and there can be different types of
> > > generator and analyzer applications. Having to have a 3rd application to
> > > arbitrate access makes this quite cumbersome. And I'm afraid that in
> > > reality people might just stick with the two devices model just to avoid
> > > this restriction.  
> >
> > I'd argue that's a problem best tackled in a library - though it's a bit
> > fiddly.  It ought to be possible to make it invisible that this level
> > of sharing is going on.   The management process you describe would probably
> > be a thread running inside the first process to try and access a given device.
> > A second process failing to open the file with -EBUSY then connects to
> > appropriate socket (via path in /tmp or similar) and asks for the FD.
> > There are race conditions that might make it fail, but a retry loop should
> > deal with those.
> >
> > I agree people might just stick to a two device model and if the devices
> > are independent enough I'm not sure that is the wrong way to approach the
> > problem.  It represents the independence and that the driver is being careful
> > that it both can and is safely handle independent simultaneous accessors.
> > We are always going to have some drivers doing that anyway because they've
> > already been doing that for years.
> >  
> 
> This is the last of the 3 patches that I need to re-spin after Lars' review.
> I have a good handle on the small stuff.
> 
> I'm not sure about the race-condition about which Lars was talking about.
> I mean, I get the problem, but is it a problem that we should fix in the kernel?
> 
> I'm sensing that Jonathan's preference is to keep things mostly as the
> current implementation.
> I'll probably leave this alone for a few days.

Wise move :)  Whilst I'm currently falling on the side of leaving the
current situation fo the ioctl, I'm not sure the discussion has completely
finished. 

I'm not keen to delay this series too much because other stuff will back
up behind it.  For now I'm in two minds on whether to back out the series
(on basis it's easy enough to bring back until churn gets us) or rely
on us being able to make any tweaks in a safe fashion.

Note we will have a little time to tweak the interface either way
as there aren't any mainline drivers using it yet.  As a result
I'm open to other proposals until this becomes ABI that we have to
support for ever.

Ideally I'd have lots of time and mock up the userspace library handling
that I think would hid most of the magic needed, but who knows when I'll
get time for that...

> And I'll prepare some patches for the tweaks Lars suggested (adding
> O_NONBLOCK and doing things a bit differently with the FD).
> I'll send those in the next few days.

Great.

Jonathan

> 
> > J
> >  
> > >
> > > - Lars
> > >  
> >
Jonathan March 13, 2021, 6:46 p.m. UTC | #10
On Sun, 7 Mar 2021 12:13:08 +0000
Jonathan Cameron <jic23@kernel.org> wrote:

> On Sat, 6 Mar 2021 19:00:52 +0200
> Alexandru Ardelean <ardeleanalex@gmail.com> wrote:
> 
> > On Sun, Feb 28, 2021 at 9:00 PM Jonathan Cameron
> > <Jonathan.Cameron@huawei.com> wrote:  
> > >
> > > On Sun, 28 Feb 2021 16:51:51 +0100
> > > Lars-Peter Clausen <lars@metafoo.de> wrote:
> > >    
> > > > On 2/28/21 3:34 PM, Jonathan Cameron wrote:    
> > > > > On Sun, 28 Feb 2021 09:51:38 +0100
> > > > > Lars-Peter Clausen <lars@metafoo.de> wrote:
> > > > >    
> > > > >> On 2/15/21 11:40 AM, Alexandru Ardelean wrote:    
> > > > >>> With this change, an ioctl() call is added to open a character device for a
> > > > >>> buffer. The ioctl() number is 'i' 0x91, which follows the
> > > > >>> IIO_GET_EVENT_FD_IOCTL ioctl.
> > > > >>>
> > > > >>> The ioctl() will return an FD for the requested buffer index. The indexes
> > > > >>> are the same from the /sys/iio/devices/iio:deviceX/bufferY (i.e. the Y
> > > > >>> variable).
> > > > >>>
> > > > >>> Since there doesn't seem to be a sane way to return the FD for buffer0 to
> > > > >>> be the same FD for the /dev/iio:deviceX, this ioctl() will return another
> > > > >>> FD for buffer0 (or the first buffer). This duplicate FD will be able to
> > > > >>> access the same buffer object (for buffer0) as accessing directly the
> > > > >>> /dev/iio:deviceX chardev.
> > > > >>>
> > > > >>> Also, there is no IIO_BUFFER_GET_BUFFER_COUNT ioctl() implemented, as the
> > > > >>> index for each buffer (and the count) can be deduced from the
> > > > >>> '/sys/bus/iio/devices/iio:deviceX/bufferY' folders (i.e the number of
> > > > >>> bufferY folders).
> > > > >>>
> > > > >>> Used following C code to test this:
> > > > >>> -------------------------------------------------------------------
> > > > >>>
> > > > >>>    #include <stdio.h>
> > > > >>>    #include <stdlib.h>
> > > > >>>    #include <unistd.h>
> > > > >>>    #include <sys/ioctl.h>
> > > > >>>    #include <fcntl.h"
> > > > >>>    #include <errno.h>
> > > > >>>
> > > > >>>    #define IIO_BUFFER_GET_FD_IOCTL      _IOWR('i', 0x91, int)
> > > > >>>
> > > > >>> int main(int argc, char *argv[])
> > > > >>> {
> > > > >>>           int fd;
> > > > >>>           int fd1;
> > > > >>>           int ret;
> > > > >>>
> > > > >>>           if ((fd = open("/dev/iio:device0", O_RDWR))<0) {
> > > > >>>                   fprintf(stderr, "Error open() %d errno %d\n",fd, errno);
> > > > >>>                   return -1;
> > > > >>>           }
> > > > >>>
> > > > >>>           fprintf(stderr, "Using FD %d\n", fd);
> > > > >>>
> > > > >>>           fd1 = atoi(argv[1]);
> > > > >>>
> > > > >>>           ret = ioctl(fd, IIO_BUFFER_GET_FD_IOCTL, &fd1);
> > > > >>>           if (ret < 0) {
> > > > >>>                   fprintf(stderr, "Error for buffer %d ioctl() %d errno %d\n", fd1, ret, errno);
> > > > >>>                   close(fd);
> > > > >>>                   return -1;
> > > > >>>           }
> > > > >>>
> > > > >>>           fprintf(stderr, "Got FD %d\n", fd1);
> > > > >>>
> > > > >>>           close(fd1);
> > > > >>>           close(fd);
> > > > >>>
> > > > >>>           return 0;
> > > > >>> }
> > > > >>> -------------------------------------------------------------------
> > > > >>>
> > > > >>> Results are:
> > > > >>> -------------------------------------------------------------------
> > > > >>>    # ./test 0
> > > > >>>    Using FD 3
> > > > >>>    Got FD 4
> > > > >>>
> > > > >>>    # ./test 1
> > > > >>>    Using FD 3
> > > > >>>    Got FD 4
> > > > >>>
> > > > >>>    # ./test 2
> > > > >>>    Using FD 3
> > > > >>>    Got FD 4
> > > > >>>
> > > > >>>    # ./test 3
> > > > >>>    Using FD 3
> > > > >>>    Got FD 4
> > > > >>>
> > > > >>>    # ls /sys/bus/iio/devices/iio\:device0
> > > > >>>    buffer  buffer0  buffer1  buffer2  buffer3  dev
> > > > >>>    in_voltage_sampling_frequency  in_voltage_scale
> > > > >>>    in_voltage_scale_available
> > > > >>>    name  of_node  power  scan_elements  subsystem  uevent
> > > > >>> -------------------------------------------------------------------
> > > > >>>
> > > > >>> iio:device0 has some fake kfifo buffers attached to an IIO device.    
> > > > >> For me there is one major problem with this approach. We only allow one
> > > > >> application to open /dev/iio:deviceX at a time. This means we can't have
> > > > >> different applications access different buffers of the same device. I
> > > > >> believe this is a circuital feature.    
> > > > > Thats not quite true (I think - though I've not tested it).  What we don't
> > > > > allow is for multiple processes to access them in an unaware fashion.
> > > > > My assumption is we can rely on fork + fd passing via appropriate sockets.
> > > > >    
> > > > >> It is possible to open the chardev, get the annonfd, close the chardev
> > > > >> and keep the annonfd open. Then the next application can do the same and
> > > > >> get access to a different buffer. But this has room for race conditions
> > > > >> when two applications try this at the very same time.
> > > > >>
> > > > >> We need to somehow address this.    
> > > > > I'd count this as a bug :).  It could be safely done in a particular custom
> > > > > system but in general it opens a can of worm.
> > > > >    
> > > > >> I'm also not much of a fan of using ioctls to create annon fds. In part
> > > > >> because all the standard mechanisms for access control no longer work.    
> > > > > The inability to trivially have multiple processes open the anon fds
> > > > > without care is one of the things I like most about them.
> > > > >
> > > > > IIO drivers and interfaces really aren't designed for multiple unaware
> > > > > processes to access them.  We don't have per process controls for device
> > > > > wide sysfs attributes etc.  In general, it would be hard to
> > > > > do due to the complexity of modeling all the interactions between the
> > > > > different interfaces (events / buffers / sysfs access) in a generic fashion.
> > > > >
> > > > > As such, the model, in my head at least, is that we only want a single
> > > > > process to ever be responsible for access control.  That process can then
> > > > > assign access to children or via a deliberate action (I think passing the
> > > > > anon fd over a unix socket should work for example).  The intent being
> > > > > that it is also responsible for mediating access to infrastructure that
> > > > > multiple child processes all want to access.
> > > > >
> > > > > As such, having one chrdev isn't a disadvantage because only one process
> > > > > should ever open it at a time.  This same process also handles the
> > > > > resource / control mediation.  Therefore we should only have one file
> > > > > exposed for all the standard access control mechanisms.
> > > > >    
> > > > Hm, I see your point, but I'm not convinced.
> > > >
> > > > Having to have explicit synchronization makes it difficult to mix and
> > > > match. E.g. at ADI a popular use case for testing was to run some signal
> > > > generator application on the TX buffer and some signal analyzer
> > > > application on the RX buffer.
> > > >
> > > > Both can be launched independently and there can be different types of
> > > > generator and analyzer applications. Having to have a 3rd application to
> > > > arbitrate access makes this quite cumbersome. And I'm afraid that in
> > > > reality people might just stick with the two devices model just to avoid
> > > > this restriction.    
> > >
> > > I'd argue that's a problem best tackled in a library - though it's a bit
> > > fiddly.  It ought to be possible to make it invisible that this level
> > > of sharing is going on.   The management process you describe would probably
> > > be a thread running inside the first process to try and access a given device.
> > > A second process failing to open the file with -EBUSY then connects to
> > > appropriate socket (via path in /tmp or similar) and asks for the FD.
> > > There are race conditions that might make it fail, but a retry loop should
> > > deal with those.
> > >
> > > I agree people might just stick to a two device model and if the devices
> > > are independent enough I'm not sure that is the wrong way to approach the
> > > problem.  It represents the independence and that the driver is being careful
> > > that it both can and is safely handle independent simultaneous accessors.
> > > We are always going to have some drivers doing that anyway because they've
> > > already been doing that for years.
> > >    
> > 
> > This is the last of the 3 patches that I need to re-spin after Lars' review.
> > I have a good handle on the small stuff.
> > 
> > I'm not sure about the race-condition about which Lars was talking about.
> > I mean, I get the problem, but is it a problem that we should fix in the kernel?
> > 
> > I'm sensing that Jonathan's preference is to keep things mostly as the
> > current implementation.
> > I'll probably leave this alone for a few days.  
> 
> Wise move :)  Whilst I'm currently falling on the side of leaving the
> current situation fo the ioctl, I'm not sure the discussion has completely
> finished. 
> 
> I'm not keen to delay this series too much because other stuff will back
> up behind it.  For now I'm in two minds on whether to back out the series
> (on basis it's easy enough to bring back until churn gets us) or rely
> on us being able to make any tweaks in a safe fashion.
> 
> Note we will have a little time to tweak the interface either way
> as there aren't any mainline drivers using it yet.  As a result
> I'm open to other proposals until this becomes ABI that we have to
> support for ever.
> 
> Ideally I'd have lots of time and mock up the userspace library handling
> that I think would hid most of the magic needed, but who knows when I'll
> get time for that...
So just to update. My current plan is to leave this series in place as it
is (with fixes etc as they come).

If we need to we can always block off the ioctl later this cycle and rethink.

Jonathan

> 
> > And I'll prepare some patches for the tweaks Lars suggested (adding
> > O_NONBLOCK and doing things a bit differently with the FD).
> > I'll send those in the next few days.  
> 
> Great.
> 
> Jonathan
> 
> >   
> > > J
> > >    
> > > >
> > > > - Lars
> > > >    
> > >    
>
Sa, Nuno March 15, 2021, 9:58 a.m. UTC | #11
> -----Original Message-----
> From: Alexandru Ardelean <ardeleanalex@gmail.com>
> Sent: Saturday, March 6, 2021 6:01 PM
> To: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Lars-Peter Clausen <lars@metafoo.de>; zzzzArdelean,
> zzzzAlexandru <alexandru.Ardelean@analog.com>; LKML <linux-
> kernel@vger.kernel.org>; linux-iio <linux-iio@vger.kernel.org>;
> Hennerich, Michael <Michael.Hennerich@analog.com>; Jonathan
> Cameron <jic23@kernel.org>; Sa, Nuno <Nuno.Sa@analog.com>;
> Bogdan, Dragos <Dragos.Bogdan@analog.com>
> Subject: Re: [PATCH v6 20/24] iio: buffer: add ioctl() to support opening
> extra buffers for IIO device
> 
> [External]
> 
> On Sun, Feb 28, 2021 at 9:00 PM Jonathan Cameron
> <Jonathan.Cameron@huawei.com> wrote:
> >
> > On Sun, 28 Feb 2021 16:51:51 +0100
> > Lars-Peter Clausen <lars@metafoo.de> wrote:
> >
> > > On 2/28/21 3:34 PM, Jonathan Cameron wrote:
> > > > On Sun, 28 Feb 2021 09:51:38 +0100
> > > > Lars-Peter Clausen <lars@metafoo.de> wrote:
> > > >
> > > >> On 2/15/21 11:40 AM, Alexandru Ardelean wrote:
> > > >>> With this change, an ioctl() call is added to open a character
> device for a
> > > >>> buffer. The ioctl() number is 'i' 0x91, which follows the
> > > >>> IIO_GET_EVENT_FD_IOCTL ioctl.
> > > >>>
> > > >>> The ioctl() will return an FD for the requested buffer index.
> The indexes
> > > >>> are the same from the /sys/iio/devices/iio:deviceX/bufferY
> (i.e. the Y
> > > >>> variable).
> > > >>>
> > > >>> Since there doesn't seem to be a sane way to return the FD for
> buffer0 to
> > > >>> be the same FD for the /dev/iio:deviceX, this ioctl() will return
> another
> > > >>> FD for buffer0 (or the first buffer). This duplicate FD will be
> able to
> > > >>> access the same buffer object (for buffer0) as accessing
> directly the
> > > >>> /dev/iio:deviceX chardev.
> > > >>>
> > > >>> Also, there is no IIO_BUFFER_GET_BUFFER_COUNT ioctl()
> implemented, as the
> > > >>> index for each buffer (and the count) can be deduced from
> the
> > > >>> '/sys/bus/iio/devices/iio:deviceX/bufferY' folders (i.e the
> number of
> > > >>> bufferY folders).
> > > >>>
> > > >>> Used following C code to test this:
> > > >>> -------------------------------------------------------------------
> > > >>>
> > > >>>    #include <stdio.h>
> > > >>>    #include <stdlib.h>
> > > >>>    #include <unistd.h>
> > > >>>    #include <sys/ioctl.h>
> > > >>>    #include <fcntl.h"
> > > >>>    #include <errno.h>
> > > >>>
> > > >>>    #define IIO_BUFFER_GET_FD_IOCTL      _IOWR('i', 0x91, int)
> > > >>>
> > > >>> int main(int argc, char *argv[])
> > > >>> {
> > > >>>           int fd;
> > > >>>           int fd1;
> > > >>>           int ret;
> > > >>>
> > > >>>           if ((fd = open("/dev/iio:device0", O_RDWR))<0) {
> > > >>>                   fprintf(stderr, "Error open() %d errno %d\n",fd,
> errno);
> > > >>>                   return -1;
> > > >>>           }
> > > >>>
> > > >>>           fprintf(stderr, "Using FD %d\n", fd);
> > > >>>
> > > >>>           fd1 = atoi(argv[1]);
> > > >>>
> > > >>>           ret = ioctl(fd, IIO_BUFFER_GET_FD_IOCTL, &fd1);
> > > >>>           if (ret < 0) {
> > > >>>                   fprintf(stderr, "Error for buffer %d ioctl() %d errno
> %d\n", fd1, ret, errno);
> > > >>>                   close(fd);
> > > >>>                   return -1;
> > > >>>           }
> > > >>>
> > > >>>           fprintf(stderr, "Got FD %d\n", fd1);
> > > >>>
> > > >>>           close(fd1);
> > > >>>           close(fd);
> > > >>>
> > > >>>           return 0;
> > > >>> }
> > > >>> -------------------------------------------------------------------
> > > >>>
> > > >>> Results are:
> > > >>> -------------------------------------------------------------------
> > > >>>    # ./test 0
> > > >>>    Using FD 3
> > > >>>    Got FD 4
> > > >>>
> > > >>>    # ./test 1
> > > >>>    Using FD 3
> > > >>>    Got FD 4
> > > >>>
> > > >>>    # ./test 2
> > > >>>    Using FD 3
> > > >>>    Got FD 4
> > > >>>
> > > >>>    # ./test 3
> > > >>>    Using FD 3
> > > >>>    Got FD 4
> > > >>>
> > > >>>    # ls /sys/bus/iio/devices/iio\:device0
> > > >>>    buffer  buffer0  buffer1  buffer2  buffer3  dev
> > > >>>    in_voltage_sampling_frequency  in_voltage_scale
> > > >>>    in_voltage_scale_available
> > > >>>    name  of_node  power  scan_elements  subsystem  uevent
> > > >>> -------------------------------------------------------------------
> > > >>>
> > > >>> iio:device0 has some fake kfifo buffers attached to an IIO
> device.
> > > >> For me there is one major problem with this approach. We only
> allow one
> > > >> application to open /dev/iio:deviceX at a time. This means we
> can't have
> > > >> different applications access different buffers of the same
> device. I
> > > >> believe this is a circuital feature.
> > > > Thats not quite true (I think - though I've not tested it).  What we
> don't
> > > > allow is for multiple processes to access them in an unaware
> fashion.
> > > > My assumption is we can rely on fork + fd passing via appropriate
> sockets.
> > > >
> > > >> It is possible to open the chardev, get the annonfd, close the
> chardev
> > > >> and keep the annonfd open. Then the next application can do
> the same and
> > > >> get access to a different buffer. But this has room for race
> conditions
> > > >> when two applications try this at the very same time.
> > > >>
> > > >> We need to somehow address this.
> > > > I'd count this as a bug :).  It could be safely done in a particular
> custom
> > > > system but in general it opens a can of worm.
> > > >
> > > >> I'm also not much of a fan of using ioctls to create annon fds. In
> part
> > > >> because all the standard mechanisms for access control no
> longer work.
> > > > The inability to trivially have multiple processes open the anon
> fds
> > > > without care is one of the things I like most about them.
> > > >
> > > > IIO drivers and interfaces really aren't designed for multiple
> unaware
> > > > processes to access them.  We don't have per process controls
> for device
> > > > wide sysfs attributes etc.  In general, it would be hard to
> > > > do due to the complexity of modeling all the interactions
> between the
> > > > different interfaces (events / buffers / sysfs access) in a generic
> fashion.
> > > >
> > > > As such, the model, in my head at least, is that we only want a
> single
> > > > process to ever be responsible for access control.  That process
> can then
> > > > assign access to children or via a deliberate action (I think passing
> the
> > > > anon fd over a unix socket should work for example).  The intent
> being
> > > > that it is also responsible for mediating access to infrastructure
> that
> > > > multiple child processes all want to access.
> > > >
> > > > As such, having one chrdev isn't a disadvantage because only one
> process
> > > > should ever open it at a time.  This same process also handles the
> > > > resource / control mediation.  Therefore we should only have
> one file
> > > > exposed for all the standard access control mechanisms.
> > > >
> > > Hm, I see your point, but I'm not convinced.
> > >
> > > Having to have explicit synchronization makes it difficult to mix and
> > > match. E.g. at ADI a popular use case for testing was to run some
> signal
> > > generator application on the TX buffer and some signal analyzer
> > > application on the RX buffer.
> > >
> > > Both can be launched independently and there can be different
> types of
> > > generator and analyzer applications. Having to have a 3rd
> application to
> > > arbitrate access makes this quite cumbersome. And I'm afraid that
> in
> > > reality people might just stick with the two devices model just to
> avoid
> > > this restriction.
> >
> > I'd argue that's a problem best tackled in a library - though it's a bit
> > fiddly.  It ought to be possible to make it invisible that this level
> > of sharing is going on.   The management process you describe would
> probably
> > be a thread running inside the first process to try and access a given
> device.
> > A second process failing to open the file with -EBUSY then connects
> to
> > appropriate socket (via path in /tmp or similar) and asks for the FD.
> > There are race conditions that might make it fail, but a retry loop
> should
> > deal with those.
> >
> > I agree people might just stick to a two device model and if the
> devices
> > are independent enough I'm not sure that is the wrong way to
> approach the
> > problem.  It represents the independence and that the driver is
> being careful
> > that it both can and is safely handle independent simultaneous
> accessors.
> > We are always going to have some drivers doing that anyway
> because they've
> > already been doing that for years.
> >
> 
> This is the last of the 3 patches that I need to re-spin after Lars' review.
> I have a good handle on the small stuff.
> 
> I'm not sure about the race-condition about which Lars was talking
> about.
> I mean, I get the problem, but is it a problem that we should fix in the
> kernel?

Hi all,

FWIW, I think that this really depends on the chosen ABI. If we do use
the ioctl to return the buffer fd and just allow one app to hold the chardev
at a time, I agree with Alex that this is not really a race and is just something
that userspace needs to deal with....

That said and giving my superficial (I did not really read the full series) piece on this,
I get both Lars and Jonathan points and, personally, it feels that the most natural thing
would be to have a chardev per buffer...

On the other hand, AFAIC, events are also being handled in the same chardev as
buffers, which makes things harder in terms of consistency... Events are per device
and not per buffers right? My point is that, to have a chardev per buffer, it would make
sense to detach events from the buffer stuff and that seems to be not doable without
breaking ABI (we would probably need to assume that events and buffer0 are on the
same chardev).

- Nuno Sá
Jonathan Cameron March 20, 2021, 5:41 p.m. UTC | #12
On Mon, 15 Mar 2021 09:58:08 +0000
"Sa, Nuno" <Nuno.Sa@analog.com> wrote:

> > -----Original Message-----
> > From: Alexandru Ardelean <ardeleanalex@gmail.com>
> > Sent: Saturday, March 6, 2021 6:01 PM
> > To: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Cc: Lars-Peter Clausen <lars@metafoo.de>; zzzzArdelean,
> > zzzzAlexandru <alexandru.Ardelean@analog.com>; LKML <linux-  
> > kernel@vger.kernel.org>; linux-iio <linux-iio@vger.kernel.org>;  
> > Hennerich, Michael <Michael.Hennerich@analog.com>; Jonathan
> > Cameron <jic23@kernel.org>; Sa, Nuno <Nuno.Sa@analog.com>;
> > Bogdan, Dragos <Dragos.Bogdan@analog.com>
> > Subject: Re: [PATCH v6 20/24] iio: buffer: add ioctl() to support opening
> > extra buffers for IIO device
> > 
> > [External]
> > 
> > On Sun, Feb 28, 2021 at 9:00 PM Jonathan Cameron
> > <Jonathan.Cameron@huawei.com> wrote:  
> > >
> > > On Sun, 28 Feb 2021 16:51:51 +0100
> > > Lars-Peter Clausen <lars@metafoo.de> wrote:
> > >  
> > > > On 2/28/21 3:34 PM, Jonathan Cameron wrote:  
> > > > > On Sun, 28 Feb 2021 09:51:38 +0100
> > > > > Lars-Peter Clausen <lars@metafoo.de> wrote:
> > > > >  
> > > > >> On 2/15/21 11:40 AM, Alexandru Ardelean wrote:  
> > > > >>> With this change, an ioctl() call is added to open a character  
> > device for a  
> > > > >>> buffer. The ioctl() number is 'i' 0x91, which follows the
> > > > >>> IIO_GET_EVENT_FD_IOCTL ioctl.
> > > > >>>
> > > > >>> The ioctl() will return an FD for the requested buffer index.  
> > The indexes  
> > > > >>> are the same from the /sys/iio/devices/iio:deviceX/bufferY  
> > (i.e. the Y  
> > > > >>> variable).
> > > > >>>
> > > > >>> Since there doesn't seem to be a sane way to return the FD for  
> > buffer0 to  
> > > > >>> be the same FD for the /dev/iio:deviceX, this ioctl() will return  
> > another  
> > > > >>> FD for buffer0 (or the first buffer). This duplicate FD will be  
> > able to  
> > > > >>> access the same buffer object (for buffer0) as accessing  
> > directly the  
> > > > >>> /dev/iio:deviceX chardev.
> > > > >>>
> > > > >>> Also, there is no IIO_BUFFER_GET_BUFFER_COUNT ioctl()  
> > implemented, as the  
> > > > >>> index for each buffer (and the count) can be deduced from  
> > the  
> > > > >>> '/sys/bus/iio/devices/iio:deviceX/bufferY' folders (i.e the  
> > number of  
> > > > >>> bufferY folders).
> > > > >>>
> > > > >>> Used following C code to test this:
> > > > >>> -------------------------------------------------------------------
> > > > >>>
> > > > >>>    #include <stdio.h>
> > > > >>>    #include <stdlib.h>
> > > > >>>    #include <unistd.h>
> > > > >>>    #include <sys/ioctl.h>
> > > > >>>    #include <fcntl.h"
> > > > >>>    #include <errno.h>
> > > > >>>
> > > > >>>    #define IIO_BUFFER_GET_FD_IOCTL      _IOWR('i', 0x91, int)
> > > > >>>
> > > > >>> int main(int argc, char *argv[])
> > > > >>> {
> > > > >>>           int fd;
> > > > >>>           int fd1;
> > > > >>>           int ret;
> > > > >>>
> > > > >>>           if ((fd = open("/dev/iio:device0", O_RDWR))<0) {
> > > > >>>                   fprintf(stderr, "Error open() %d errno %d\n",fd,  
> > errno);  
> > > > >>>                   return -1;
> > > > >>>           }
> > > > >>>
> > > > >>>           fprintf(stderr, "Using FD %d\n", fd);
> > > > >>>
> > > > >>>           fd1 = atoi(argv[1]);
> > > > >>>
> > > > >>>           ret = ioctl(fd, IIO_BUFFER_GET_FD_IOCTL, &fd1);
> > > > >>>           if (ret < 0) {
> > > > >>>                   fprintf(stderr, "Error for buffer %d ioctl() %d errno  
> > %d\n", fd1, ret, errno);  
> > > > >>>                   close(fd);
> > > > >>>                   return -1;
> > > > >>>           }
> > > > >>>
> > > > >>>           fprintf(stderr, "Got FD %d\n", fd1);
> > > > >>>
> > > > >>>           close(fd1);
> > > > >>>           close(fd);
> > > > >>>
> > > > >>>           return 0;
> > > > >>> }
> > > > >>> -------------------------------------------------------------------
> > > > >>>
> > > > >>> Results are:
> > > > >>> -------------------------------------------------------------------
> > > > >>>    # ./test 0
> > > > >>>    Using FD 3
> > > > >>>    Got FD 4
> > > > >>>
> > > > >>>    # ./test 1
> > > > >>>    Using FD 3
> > > > >>>    Got FD 4
> > > > >>>
> > > > >>>    # ./test 2
> > > > >>>    Using FD 3
> > > > >>>    Got FD 4
> > > > >>>
> > > > >>>    # ./test 3
> > > > >>>    Using FD 3
> > > > >>>    Got FD 4
> > > > >>>
> > > > >>>    # ls /sys/bus/iio/devices/iio\:device0
> > > > >>>    buffer  buffer0  buffer1  buffer2  buffer3  dev
> > > > >>>    in_voltage_sampling_frequency  in_voltage_scale
> > > > >>>    in_voltage_scale_available
> > > > >>>    name  of_node  power  scan_elements  subsystem  uevent
> > > > >>> -------------------------------------------------------------------
> > > > >>>
> > > > >>> iio:device0 has some fake kfifo buffers attached to an IIO  
> > device.  
> > > > >> For me there is one major problem with this approach. We only  
> > allow one  
> > > > >> application to open /dev/iio:deviceX at a time. This means we  
> > can't have  
> > > > >> different applications access different buffers of the same  
> > device. I  
> > > > >> believe this is a circuital feature.  
> > > > > Thats not quite true (I think - though I've not tested it).  What we  
> > don't  
> > > > > allow is for multiple processes to access them in an unaware  
> > fashion.  
> > > > > My assumption is we can rely on fork + fd passing via appropriate  
> > sockets.  
> > > > >  
> > > > >> It is possible to open the chardev, get the annonfd, close the  
> > chardev  
> > > > >> and keep the annonfd open. Then the next application can do  
> > the same and  
> > > > >> get access to a different buffer. But this has room for race  
> > conditions  
> > > > >> when two applications try this at the very same time.
> > > > >>
> > > > >> We need to somehow address this.  
> > > > > I'd count this as a bug :).  It could be safely done in a particular  
> > custom  
> > > > > system but in general it opens a can of worm.
> > > > >  
> > > > >> I'm also not much of a fan of using ioctls to create annon fds. In  
> > part  
> > > > >> because all the standard mechanisms for access control no  
> > longer work.  
> > > > > The inability to trivially have multiple processes open the anon  
> > fds  
> > > > > without care is one of the things I like most about them.
> > > > >
> > > > > IIO drivers and interfaces really aren't designed for multiple  
> > unaware  
> > > > > processes to access them.  We don't have per process controls  
> > for device  
> > > > > wide sysfs attributes etc.  In general, it would be hard to
> > > > > do due to the complexity of modeling all the interactions  
> > between the  
> > > > > different interfaces (events / buffers / sysfs access) in a generic  
> > fashion.  
> > > > >
> > > > > As such, the model, in my head at least, is that we only want a  
> > single  
> > > > > process to ever be responsible for access control.  That process  
> > can then  
> > > > > assign access to children or via a deliberate action (I think passing  
> > the  
> > > > > anon fd over a unix socket should work for example).  The intent  
> > being  
> > > > > that it is also responsible for mediating access to infrastructure  
> > that  
> > > > > multiple child processes all want to access.
> > > > >
> > > > > As such, having one chrdev isn't a disadvantage because only one  
> > process  
> > > > > should ever open it at a time.  This same process also handles the
> > > > > resource / control mediation.  Therefore we should only have  
> > one file  
> > > > > exposed for all the standard access control mechanisms.
> > > > >  
> > > > Hm, I see your point, but I'm not convinced.
> > > >
> > > > Having to have explicit synchronization makes it difficult to mix and
> > > > match. E.g. at ADI a popular use case for testing was to run some  
> > signal  
> > > > generator application on the TX buffer and some signal analyzer
> > > > application on the RX buffer.
> > > >
> > > > Both can be launched independently and there can be different  
> > types of  
> > > > generator and analyzer applications. Having to have a 3rd  
> > application to  
> > > > arbitrate access makes this quite cumbersome. And I'm afraid that  
> > in  
> > > > reality people might just stick with the two devices model just to  
> > avoid  
> > > > this restriction.  
> > >
> > > I'd argue that's a problem best tackled in a library - though it's a bit
> > > fiddly.  It ought to be possible to make it invisible that this level
> > > of sharing is going on.   The management process you describe would  
> > probably  
> > > be a thread running inside the first process to try and access a given  
> > device.  
> > > A second process failing to open the file with -EBUSY then connects  
> > to  
> > > appropriate socket (via path in /tmp or similar) and asks for the FD.
> > > There are race conditions that might make it fail, but a retry loop  
> > should  
> > > deal with those.
> > >
> > > I agree people might just stick to a two device model and if the  
> > devices  
> > > are independent enough I'm not sure that is the wrong way to  
> > approach the  
> > > problem.  It represents the independence and that the driver is  
> > being careful  
> > > that it both can and is safely handle independent simultaneous  
> > accessors.  
> > > We are always going to have some drivers doing that anyway  
> > because they've  
> > > already been doing that for years.
> > >  
> > 
> > This is the last of the 3 patches that I need to re-spin after Lars' review.
> > I have a good handle on the small stuff.
> > 
> > I'm not sure about the race-condition about which Lars was talking
> > about.
> > I mean, I get the problem, but is it a problem that we should fix in the
> > kernel?  
> 
> Hi all,
> 
> FWIW, I think that this really depends on the chosen ABI. If we do use
> the ioctl to return the buffer fd and just allow one app to hold the chardev
> at a time, I agree with Alex that this is not really a race and is just something
> that userspace needs to deal with....
> 
> That said and giving my superficial (I did not really read the full series) piece on this,
> I get both Lars and Jonathan points and, personally, it feels that the most natural thing
> would be to have a chardev per buffer...
> 
> On the other hand, AFAIC, events are also being handled in the same chardev as
> buffers, which makes things harder in terms of consistency... Events are per device
> and not per buffers right? My point is that, to have a chardev per buffer, it would make
> sense to detach events from the buffer stuff and that seems to be not doable without
> breaking ABI (we would probably need to assume that events and buffer0 are on the
> same chardev).

Events are interesting as there is no particular reason to assume the driver
handling buffer0 is the right one to deal with them.  It might just as easily
be the case that they are of interest to a process that is concerned with buffer1.

To add a bit more flavour to my earlier comments.

I'm still concerned that if we did do multiple /dev/* files it would allow code
to think it has complete control over the device when it really doesn't.
Events are just one aspect of that.

We have had discussions in the past about allowing multiple userspace consumers
for a single buffer, but the conclusion there was that was a job for userspace
(daemon or similar) software which can deal with control inter dependencies etc.

There are already potential messy corners we don't handle for userspace
iio buffers vs in kernel users (what happens if they both try to control the
sampling frequency?)  I'm not keen to broaden this problem set.
If a device genuinely has separate control and pipelines for different
buffers then we are probably better representing that cleanly as
an mfd type layer and two separate IIO devices. Its effectively the
same a multi chip package.

A more classic multibuffer usecase is the one where you have related
datastreams that run at different rates (often happens in devices with
tagged FIFO elements). These are tightly coupled but we need to split
the data stream (or add tagging to our FIFOs.). Another case would be
DMA based device that puts channels into buffers that are entirely
separate in memory address rather than interleaved.

So I still need to put together a PoC, but it feels like there are various
software models that will give the illusion of there being separate
/dev/* files, but with an aspect of control being possible.

1. Daemon, if present that can hand off chardevs to who needs them
2. Library to make the first user of the buffer responsible for providing
   service to other users.  Yes there are races, but I don't think they
   are hard to deal in normal usecases.  (retry loops)

Jonathan


> 
> - Nuno Sá
Jonathan March 21, 2021, 5:37 p.m. UTC | #13
On Sat, 20 Mar 2021 17:41:00 +0000
Jonathan Cameron <jic23@kernel.org> wrote:

> On Mon, 15 Mar 2021 09:58:08 +0000
> "Sa, Nuno" <Nuno.Sa@analog.com> wrote:
> 
> > > -----Original Message-----
> > > From: Alexandru Ardelean <ardeleanalex@gmail.com>
> > > Sent: Saturday, March 6, 2021 6:01 PM
> > > To: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > Cc: Lars-Peter Clausen <lars@metafoo.de>; zzzzArdelean,
> > > zzzzAlexandru <alexandru.Ardelean@analog.com>; LKML <linux-    
> > > kernel@vger.kernel.org>; linux-iio <linux-iio@vger.kernel.org>;    
> > > Hennerich, Michael <Michael.Hennerich@analog.com>; Jonathan
> > > Cameron <jic23@kernel.org>; Sa, Nuno <Nuno.Sa@analog.com>;
> > > Bogdan, Dragos <Dragos.Bogdan@analog.com>
> > > Subject: Re: [PATCH v6 20/24] iio: buffer: add ioctl() to support opening
> > > extra buffers for IIO device
> > > 
> > > [External]
> > > 
> > > On Sun, Feb 28, 2021 at 9:00 PM Jonathan Cameron
> > > <Jonathan.Cameron@huawei.com> wrote:    
> > > >
> > > > On Sun, 28 Feb 2021 16:51:51 +0100
> > > > Lars-Peter Clausen <lars@metafoo.de> wrote:
> > > >    
> > > > > On 2/28/21 3:34 PM, Jonathan Cameron wrote:    
> > > > > > On Sun, 28 Feb 2021 09:51:38 +0100
> > > > > > Lars-Peter Clausen <lars@metafoo.de> wrote:
> > > > > >    
> > > > > >> On 2/15/21 11:40 AM, Alexandru Ardelean wrote:    
> > > > > >>> With this change, an ioctl() call is added to open a character    
> > > device for a    
> > > > > >>> buffer. The ioctl() number is 'i' 0x91, which follows the
> > > > > >>> IIO_GET_EVENT_FD_IOCTL ioctl.
> > > > > >>>
> > > > > >>> The ioctl() will return an FD for the requested buffer index.    
> > > The indexes    
> > > > > >>> are the same from the /sys/iio/devices/iio:deviceX/bufferY    
> > > (i.e. the Y    
> > > > > >>> variable).
> > > > > >>>
> > > > > >>> Since there doesn't seem to be a sane way to return the FD for    
> > > buffer0 to    
> > > > > >>> be the same FD for the /dev/iio:deviceX, this ioctl() will return    
> > > another    
> > > > > >>> FD for buffer0 (or the first buffer). This duplicate FD will be    
> > > able to    
> > > > > >>> access the same buffer object (for buffer0) as accessing    
> > > directly the    
> > > > > >>> /dev/iio:deviceX chardev.
> > > > > >>>
> > > > > >>> Also, there is no IIO_BUFFER_GET_BUFFER_COUNT ioctl()    
> > > implemented, as the    
> > > > > >>> index for each buffer (and the count) can be deduced from    
> > > the    
> > > > > >>> '/sys/bus/iio/devices/iio:deviceX/bufferY' folders (i.e the    
> > > number of    
> > > > > >>> bufferY folders).
> > > > > >>>
> > > > > >>> Used following C code to test this:
> > > > > >>> -------------------------------------------------------------------
> > > > > >>>
> > > > > >>>    #include <stdio.h>
> > > > > >>>    #include <stdlib.h>
> > > > > >>>    #include <unistd.h>
> > > > > >>>    #include <sys/ioctl.h>
> > > > > >>>    #include <fcntl.h"
> > > > > >>>    #include <errno.h>
> > > > > >>>
> > > > > >>>    #define IIO_BUFFER_GET_FD_IOCTL      _IOWR('i', 0x91, int)
> > > > > >>>
> > > > > >>> int main(int argc, char *argv[])
> > > > > >>> {
> > > > > >>>           int fd;
> > > > > >>>           int fd1;
> > > > > >>>           int ret;
> > > > > >>>
> > > > > >>>           if ((fd = open("/dev/iio:device0", O_RDWR))<0) {
> > > > > >>>                   fprintf(stderr, "Error open() %d errno %d\n",fd,    
> > > errno);    
> > > > > >>>                   return -1;
> > > > > >>>           }
> > > > > >>>
> > > > > >>>           fprintf(stderr, "Using FD %d\n", fd);
> > > > > >>>
> > > > > >>>           fd1 = atoi(argv[1]);
> > > > > >>>
> > > > > >>>           ret = ioctl(fd, IIO_BUFFER_GET_FD_IOCTL, &fd1);
> > > > > >>>           if (ret < 0) {
> > > > > >>>                   fprintf(stderr, "Error for buffer %d ioctl() %d errno    
> > > %d\n", fd1, ret, errno);    
> > > > > >>>                   close(fd);
> > > > > >>>                   return -1;
> > > > > >>>           }
> > > > > >>>
> > > > > >>>           fprintf(stderr, "Got FD %d\n", fd1);
> > > > > >>>
> > > > > >>>           close(fd1);
> > > > > >>>           close(fd);
> > > > > >>>
> > > > > >>>           return 0;
> > > > > >>> }
> > > > > >>> -------------------------------------------------------------------
> > > > > >>>
> > > > > >>> Results are:
> > > > > >>> -------------------------------------------------------------------
> > > > > >>>    # ./test 0
> > > > > >>>    Using FD 3
> > > > > >>>    Got FD 4
> > > > > >>>
> > > > > >>>    # ./test 1
> > > > > >>>    Using FD 3
> > > > > >>>    Got FD 4
> > > > > >>>
> > > > > >>>    # ./test 2
> > > > > >>>    Using FD 3
> > > > > >>>    Got FD 4
> > > > > >>>
> > > > > >>>    # ./test 3
> > > > > >>>    Using FD 3
> > > > > >>>    Got FD 4
> > > > > >>>
> > > > > >>>    # ls /sys/bus/iio/devices/iio\:device0
> > > > > >>>    buffer  buffer0  buffer1  buffer2  buffer3  dev
> > > > > >>>    in_voltage_sampling_frequency  in_voltage_scale
> > > > > >>>    in_voltage_scale_available
> > > > > >>>    name  of_node  power  scan_elements  subsystem  uevent
> > > > > >>> -------------------------------------------------------------------
> > > > > >>>
> > > > > >>> iio:device0 has some fake kfifo buffers attached to an IIO    
> > > device.    
> > > > > >> For me there is one major problem with this approach. We only    
> > > allow one    
> > > > > >> application to open /dev/iio:deviceX at a time. This means we    
> > > can't have    
> > > > > >> different applications access different buffers of the same    
> > > device. I    
> > > > > >> believe this is a circuital feature.    
> > > > > > Thats not quite true (I think - though I've not tested it).  What we    
> > > don't    
> > > > > > allow is for multiple processes to access them in an unaware    
> > > fashion.    
> > > > > > My assumption is we can rely on fork + fd passing via appropriate    
> > > sockets.    
> > > > > >    
> > > > > >> It is possible to open the chardev, get the annonfd, close the    
> > > chardev    
> > > > > >> and keep the annonfd open. Then the next application can do    
> > > the same and    
> > > > > >> get access to a different buffer. But this has room for race    
> > > conditions    
> > > > > >> when two applications try this at the very same time.
> > > > > >>
> > > > > >> We need to somehow address this.    
> > > > > > I'd count this as a bug :).  It could be safely done in a particular    
> > > custom    
> > > > > > system but in general it opens a can of worm.
> > > > > >    
> > > > > >> I'm also not much of a fan of using ioctls to create annon fds. In    
> > > part    
> > > > > >> because all the standard mechanisms for access control no    
> > > longer work.    
> > > > > > The inability to trivially have multiple processes open the anon    
> > > fds    
> > > > > > without care is one of the things I like most about them.
> > > > > >
> > > > > > IIO drivers and interfaces really aren't designed for multiple    
> > > unaware    
> > > > > > processes to access them.  We don't have per process controls    
> > > for device    
> > > > > > wide sysfs attributes etc.  In general, it would be hard to
> > > > > > do due to the complexity of modeling all the interactions    
> > > between the    
> > > > > > different interfaces (events / buffers / sysfs access) in a generic    
> > > fashion.    
> > > > > >
> > > > > > As such, the model, in my head at least, is that we only want a    
> > > single    
> > > > > > process to ever be responsible for access control.  That process    
> > > can then    
> > > > > > assign access to children or via a deliberate action (I think passing    
> > > the    
> > > > > > anon fd over a unix socket should work for example).  The intent    
> > > being    
> > > > > > that it is also responsible for mediating access to infrastructure    
> > > that    
> > > > > > multiple child processes all want to access.
> > > > > >
> > > > > > As such, having one chrdev isn't a disadvantage because only one    
> > > process    
> > > > > > should ever open it at a time.  This same process also handles the
> > > > > > resource / control mediation.  Therefore we should only have    
> > > one file    
> > > > > > exposed for all the standard access control mechanisms.
> > > > > >    
> > > > > Hm, I see your point, but I'm not convinced.
> > > > >
> > > > > Having to have explicit synchronization makes it difficult to mix and
> > > > > match. E.g. at ADI a popular use case for testing was to run some    
> > > signal    
> > > > > generator application on the TX buffer and some signal analyzer
> > > > > application on the RX buffer.
> > > > >
> > > > > Both can be launched independently and there can be different    
> > > types of    
> > > > > generator and analyzer applications. Having to have a 3rd    
> > > application to    
> > > > > arbitrate access makes this quite cumbersome. And I'm afraid that    
> > > in    
> > > > > reality people might just stick with the two devices model just to    
> > > avoid    
> > > > > this restriction.    
> > > >
> > > > I'd argue that's a problem best tackled in a library - though it's a bit
> > > > fiddly.  It ought to be possible to make it invisible that this level
> > > > of sharing is going on.   The management process you describe would    
> > > probably    
> > > > be a thread running inside the first process to try and access a given    
> > > device.    
> > > > A second process failing to open the file with -EBUSY then connects    
> > > to    
> > > > appropriate socket (via path in /tmp or similar) and asks for the FD.
> > > > There are race conditions that might make it fail, but a retry loop    
> > > should    
> > > > deal with those.
> > > >
> > > > I agree people might just stick to a two device model and if the    
> > > devices    
> > > > are independent enough I'm not sure that is the wrong way to    
> > > approach the    
> > > > problem.  It represents the independence and that the driver is    
> > > being careful    
> > > > that it both can and is safely handle independent simultaneous    
> > > accessors.    
> > > > We are always going to have some drivers doing that anyway    
> > > because they've    
> > > > already been doing that for years.
> > > >    
> > > 
> > > This is the last of the 3 patches that I need to re-spin after Lars' review.
> > > I have a good handle on the small stuff.
> > > 
> > > I'm not sure about the race-condition about which Lars was talking
> > > about.
> > > I mean, I get the problem, but is it a problem that we should fix in the
> > > kernel?    
> > 
> > Hi all,
> > 
> > FWIW, I think that this really depends on the chosen ABI. If we do use
> > the ioctl to return the buffer fd and just allow one app to hold the chardev
> > at a time, I agree with Alex that this is not really a race and is just something
> > that userspace needs to deal with....
> > 
> > That said and giving my superficial (I did not really read the full series) piece on this,
> > I get both Lars and Jonathan points and, personally, it feels that the most natural thing
> > would be to have a chardev per buffer...
> > 
> > On the other hand, AFAIC, events are also being handled in the same chardev as
> > buffers, which makes things harder in terms of consistency... Events are per device
> > and not per buffers right? My point is that, to have a chardev per buffer, it would make
> > sense to detach events from the buffer stuff and that seems to be not doable without
> > breaking ABI (we would probably need to assume that events and buffer0 are on the
> > same chardev).  
> 
> Events are interesting as there is no particular reason to assume the driver
> handling buffer0 is the right one to deal with them.  It might just as easily
> be the case that they are of interest to a process that is concerned with buffer1.
> 
> To add a bit more flavour to my earlier comments.
> 
> I'm still concerned that if we did do multiple /dev/* files it would allow code
> to think it has complete control over the device when it really doesn't.
> Events are just one aspect of that.
> 
> We have had discussions in the past about allowing multiple userspace consumers
> for a single buffer, but the conclusion there was that was a job for userspace
> (daemon or similar) software which can deal with control inter dependencies etc.
> 
> There are already potential messy corners we don't handle for userspace
> iio buffers vs in kernel users (what happens if they both try to control the
> sampling frequency?)  I'm not keen to broaden this problem set.
> If a device genuinely has separate control and pipelines for different
> buffers then we are probably better representing that cleanly as
> an mfd type layer and two separate IIO devices. Its effectively the
> same a multi chip package.
> 
> A more classic multibuffer usecase is the one where you have related
> datastreams that run at different rates (often happens in devices with
> tagged FIFO elements). These are tightly coupled but we need to split
> the data stream (or add tagging to our FIFOs.). Another case would be
> DMA based device that puts channels into buffers that are entirely
> separate in memory address rather than interleaved.
> 
> So I still need to put together a PoC, but it feels like there are various
> software models that will give the illusion of there being separate
> /dev/* files, but with an aspect of control being possible.
> 
> 1. Daemon, if present that can hand off chardevs to who needs them
> 2. Library to make the first user of the buffer responsible for providing
>    service to other users.  Yes there are races, but I don't think they
>    are hard to deal in normal usecases.  (retry loops)

Hi Nuno / Others,

Nuno's mention of things being similar for the event anon
FD to the situation for the buffer anon FDs made me realise there was
a horrible short cut to a proof of concept that didn't require me
to wire up a multiple buffer device.

Upshot, is that I've just sent out a (definitely not for merging)
hacked up version of the iio_event_monitor that can act as server
or client.  The idea is that the socket handling looks a bit
like what I'd expect to see hidden away in a library so as to
allow

1) Client 1 is after buffer 3.
   It tries to open the /dev/iio\:deviceX chrdev and succeeds.
   It spins up a thread with a listening socket for /tmp/iio\:deviceX-magic
   Continues in main thread to request buffer 3.
2) Client 2 is after buffer 2
   I tries to open the /dev/iio\:deviceX chrdev and fails.
   It sleeps a moment (reduces chance of race with client 1)
   It opens a connection to the socket via /tmp/iio\:deviceX-magic
   Sends a request for the buffer 2 FD.
   Thread in Client 1 calls the ioctl to get the buffer 2 FD which
   it then sends on to Client 2 which can use it as if it had
   requested it directly.

We might want to have a generic server version as well that doesn't
itself make use of any of the buffers as keeps the model more symmetric
and reduce common corner cases.

Anyhow the code I put together is terrible, but I wasn't 100% sure
there weren't any issues passing anon fd file handles and this shows
that at least in theory the approach I proposed above works.

Test is something like
./iio_events_network /dev/iio\:device1
./iio_events_network -c

Then make some events happen (I was using the dummy driver and
the event generator associated with that).
The server in this PoC just quits after handling off the FD.

Jonathan

> 
> Jonathan
> 
> 
> > 
> > - Nuno Sá  
>
Alexandru Ardelean March 23, 2021, 9:51 a.m. UTC | #14
On Sun, Mar 21, 2021 at 7:37 PM Jonathan Cameron
<jic23@jic23.retrosnub.co.uk> wrote:
>
> On Sat, 20 Mar 2021 17:41:00 +0000
> Jonathan Cameron <jic23@kernel.org> wrote:
>
> > On Mon, 15 Mar 2021 09:58:08 +0000
> > "Sa, Nuno" <Nuno.Sa@analog.com> wrote:
> >
> > > > -----Original Message-----
> > > > From: Alexandru Ardelean <ardeleanalex@gmail.com>
> > > > Sent: Saturday, March 6, 2021 6:01 PM
> > > > To: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > Cc: Lars-Peter Clausen <lars@metafoo.de>; zzzzArdelean,
> > > > zzzzAlexandru <alexandru.Ardelean@analog.com>; LKML <linux-
> > > > kernel@vger.kernel.org>; linux-iio <linux-iio@vger.kernel.org>;
> > > > Hennerich, Michael <Michael.Hennerich@analog.com>; Jonathan
> > > > Cameron <jic23@kernel.org>; Sa, Nuno <Nuno.Sa@analog.com>;
> > > > Bogdan, Dragos <Dragos.Bogdan@analog.com>
> > > > Subject: Re: [PATCH v6 20/24] iio: buffer: add ioctl() to support opening
> > > > extra buffers for IIO device
> > > >
> > > > [External]
> > > >
> > > > On Sun, Feb 28, 2021 at 9:00 PM Jonathan Cameron
> > > > <Jonathan.Cameron@huawei.com> wrote:
> > > > >
> > > > > On Sun, 28 Feb 2021 16:51:51 +0100
> > > > > Lars-Peter Clausen <lars@metafoo.de> wrote:
> > > > >
> > > > > > On 2/28/21 3:34 PM, Jonathan Cameron wrote:
> > > > > > > On Sun, 28 Feb 2021 09:51:38 +0100
> > > > > > > Lars-Peter Clausen <lars@metafoo.de> wrote:
> > > > > > >
> > > > > > >> On 2/15/21 11:40 AM, Alexandru Ardelean wrote:
> > > > > > >>> With this change, an ioctl() call is added to open a character
> > > > device for a
> > > > > > >>> buffer. The ioctl() number is 'i' 0x91, which follows the
> > > > > > >>> IIO_GET_EVENT_FD_IOCTL ioctl.
> > > > > > >>>
> > > > > > >>> The ioctl() will return an FD for the requested buffer index.
> > > > The indexes
> > > > > > >>> are the same from the /sys/iio/devices/iio:deviceX/bufferY
> > > > (i.e. the Y
> > > > > > >>> variable).
> > > > > > >>>
> > > > > > >>> Since there doesn't seem to be a sane way to return the FD for
> > > > buffer0 to
> > > > > > >>> be the same FD for the /dev/iio:deviceX, this ioctl() will return
> > > > another
> > > > > > >>> FD for buffer0 (or the first buffer). This duplicate FD will be
> > > > able to
> > > > > > >>> access the same buffer object (for buffer0) as accessing
> > > > directly the
> > > > > > >>> /dev/iio:deviceX chardev.
> > > > > > >>>
> > > > > > >>> Also, there is no IIO_BUFFER_GET_BUFFER_COUNT ioctl()
> > > > implemented, as the
> > > > > > >>> index for each buffer (and the count) can be deduced from
> > > > the
> > > > > > >>> '/sys/bus/iio/devices/iio:deviceX/bufferY' folders (i.e the
> > > > number of
> > > > > > >>> bufferY folders).
> > > > > > >>>
> > > > > > >>> Used following C code to test this:
> > > > > > >>> -------------------------------------------------------------------
> > > > > > >>>
> > > > > > >>>    #include <stdio.h>
> > > > > > >>>    #include <stdlib.h>
> > > > > > >>>    #include <unistd.h>
> > > > > > >>>    #include <sys/ioctl.h>
> > > > > > >>>    #include <fcntl.h"
> > > > > > >>>    #include <errno.h>
> > > > > > >>>
> > > > > > >>>    #define IIO_BUFFER_GET_FD_IOCTL      _IOWR('i', 0x91, int)
> > > > > > >>>
> > > > > > >>> int main(int argc, char *argv[])
> > > > > > >>> {
> > > > > > >>>           int fd;
> > > > > > >>>           int fd1;
> > > > > > >>>           int ret;
> > > > > > >>>
> > > > > > >>>           if ((fd = open("/dev/iio:device0", O_RDWR))<0) {
> > > > > > >>>                   fprintf(stderr, "Error open() %d errno %d\n",fd,
> > > > errno);
> > > > > > >>>                   return -1;
> > > > > > >>>           }
> > > > > > >>>
> > > > > > >>>           fprintf(stderr, "Using FD %d\n", fd);
> > > > > > >>>
> > > > > > >>>           fd1 = atoi(argv[1]);
> > > > > > >>>
> > > > > > >>>           ret = ioctl(fd, IIO_BUFFER_GET_FD_IOCTL, &fd1);
> > > > > > >>>           if (ret < 0) {
> > > > > > >>>                   fprintf(stderr, "Error for buffer %d ioctl() %d errno
> > > > %d\n", fd1, ret, errno);
> > > > > > >>>                   close(fd);
> > > > > > >>>                   return -1;
> > > > > > >>>           }
> > > > > > >>>
> > > > > > >>>           fprintf(stderr, "Got FD %d\n", fd1);
> > > > > > >>>
> > > > > > >>>           close(fd1);
> > > > > > >>>           close(fd);
> > > > > > >>>
> > > > > > >>>           return 0;
> > > > > > >>> }
> > > > > > >>> -------------------------------------------------------------------
> > > > > > >>>
> > > > > > >>> Results are:
> > > > > > >>> -------------------------------------------------------------------
> > > > > > >>>    # ./test 0
> > > > > > >>>    Using FD 3
> > > > > > >>>    Got FD 4
> > > > > > >>>
> > > > > > >>>    # ./test 1
> > > > > > >>>    Using FD 3
> > > > > > >>>    Got FD 4
> > > > > > >>>
> > > > > > >>>    # ./test 2
> > > > > > >>>    Using FD 3
> > > > > > >>>    Got FD 4
> > > > > > >>>
> > > > > > >>>    # ./test 3
> > > > > > >>>    Using FD 3
> > > > > > >>>    Got FD 4
> > > > > > >>>
> > > > > > >>>    # ls /sys/bus/iio/devices/iio\:device0
> > > > > > >>>    buffer  buffer0  buffer1  buffer2  buffer3  dev
> > > > > > >>>    in_voltage_sampling_frequency  in_voltage_scale
> > > > > > >>>    in_voltage_scale_available
> > > > > > >>>    name  of_node  power  scan_elements  subsystem  uevent
> > > > > > >>> -------------------------------------------------------------------
> > > > > > >>>
> > > > > > >>> iio:device0 has some fake kfifo buffers attached to an IIO
> > > > device.
> > > > > > >> For me there is one major problem with this approach. We only
> > > > allow one
> > > > > > >> application to open /dev/iio:deviceX at a time. This means we
> > > > can't have
> > > > > > >> different applications access different buffers of the same
> > > > device. I
> > > > > > >> believe this is a circuital feature.
> > > > > > > Thats not quite true (I think - though I've not tested it).  What we
> > > > don't
> > > > > > > allow is for multiple processes to access them in an unaware
> > > > fashion.
> > > > > > > My assumption is we can rely on fork + fd passing via appropriate
> > > > sockets.
> > > > > > >
> > > > > > >> It is possible to open the chardev, get the annonfd, close the
> > > > chardev
> > > > > > >> and keep the annonfd open. Then the next application can do
> > > > the same and
> > > > > > >> get access to a different buffer. But this has room for race
> > > > conditions
> > > > > > >> when two applications try this at the very same time.
> > > > > > >>
> > > > > > >> We need to somehow address this.
> > > > > > > I'd count this as a bug :).  It could be safely done in a particular
> > > > custom
> > > > > > > system but in general it opens a can of worm.
> > > > > > >
> > > > > > >> I'm also not much of a fan of using ioctls to create annon fds. In
> > > > part
> > > > > > >> because all the standard mechanisms for access control no
> > > > longer work.
> > > > > > > The inability to trivially have multiple processes open the anon
> > > > fds
> > > > > > > without care is one of the things I like most about them.
> > > > > > >
> > > > > > > IIO drivers and interfaces really aren't designed for multiple
> > > > unaware
> > > > > > > processes to access them.  We don't have per process controls
> > > > for device
> > > > > > > wide sysfs attributes etc.  In general, it would be hard to
> > > > > > > do due to the complexity of modeling all the interactions
> > > > between the
> > > > > > > different interfaces (events / buffers / sysfs access) in a generic
> > > > fashion.
> > > > > > >
> > > > > > > As such, the model, in my head at least, is that we only want a
> > > > single
> > > > > > > process to ever be responsible for access control.  That process
> > > > can then
> > > > > > > assign access to children or via a deliberate action (I think passing
> > > > the
> > > > > > > anon fd over a unix socket should work for example).  The intent
> > > > being
> > > > > > > that it is also responsible for mediating access to infrastructure
> > > > that
> > > > > > > multiple child processes all want to access.
> > > > > > >
> > > > > > > As such, having one chrdev isn't a disadvantage because only one
> > > > process
> > > > > > > should ever open it at a time.  This same process also handles the
> > > > > > > resource / control mediation.  Therefore we should only have
> > > > one file
> > > > > > > exposed for all the standard access control mechanisms.
> > > > > > >
> > > > > > Hm, I see your point, but I'm not convinced.
> > > > > >
> > > > > > Having to have explicit synchronization makes it difficult to mix and
> > > > > > match. E.g. at ADI a popular use case for testing was to run some
> > > > signal
> > > > > > generator application on the TX buffer and some signal analyzer
> > > > > > application on the RX buffer.
> > > > > >
> > > > > > Both can be launched independently and there can be different
> > > > types of
> > > > > > generator and analyzer applications. Having to have a 3rd
> > > > application to
> > > > > > arbitrate access makes this quite cumbersome. And I'm afraid that
> > > > in
> > > > > > reality people might just stick with the two devices model just to
> > > > avoid
> > > > > > this restriction.
> > > > >
> > > > > I'd argue that's a problem best tackled in a library - though it's a bit
> > > > > fiddly.  It ought to be possible to make it invisible that this level
> > > > > of sharing is going on.   The management process you describe would
> > > > probably
> > > > > be a thread running inside the first process to try and access a given
> > > > device.
> > > > > A second process failing to open the file with -EBUSY then connects
> > > > to
> > > > > appropriate socket (via path in /tmp or similar) and asks for the FD.
> > > > > There are race conditions that might make it fail, but a retry loop
> > > > should
> > > > > deal with those.
> > > > >
> > > > > I agree people might just stick to a two device model and if the
> > > > devices
> > > > > are independent enough I'm not sure that is the wrong way to
> > > > approach the
> > > > > problem.  It represents the independence and that the driver is
> > > > being careful
> > > > > that it both can and is safely handle independent simultaneous
> > > > accessors.
> > > > > We are always going to have some drivers doing that anyway
> > > > because they've
> > > > > already been doing that for years.
> > > > >
> > > >
> > > > This is the last of the 3 patches that I need to re-spin after Lars' review.
> > > > I have a good handle on the small stuff.
> > > >
> > > > I'm not sure about the race-condition about which Lars was talking
> > > > about.
> > > > I mean, I get the problem, but is it a problem that we should fix in the
> > > > kernel?
> > >
> > > Hi all,
> > >
> > > FWIW, I think that this really depends on the chosen ABI. If we do use
> > > the ioctl to return the buffer fd and just allow one app to hold the chardev
> > > at a time, I agree with Alex that this is not really a race and is just something
> > > that userspace needs to deal with....
> > >
> > > That said and giving my superficial (I did not really read the full series) piece on this,
> > > I get both Lars and Jonathan points and, personally, it feels that the most natural thing
> > > would be to have a chardev per buffer...
> > >
> > > On the other hand, AFAIC, events are also being handled in the same chardev as
> > > buffers, which makes things harder in terms of consistency... Events are per device
> > > and not per buffers right? My point is that, to have a chardev per buffer, it would make
> > > sense to detach events from the buffer stuff and that seems to be not doable without
> > > breaking ABI (we would probably need to assume that events and buffer0 are on the
> > > same chardev).
> >
> > Events are interesting as there is no particular reason to assume the driver
> > handling buffer0 is the right one to deal with them.  It might just as easily
> > be the case that they are of interest to a process that is concerned with buffer1.
> >
> > To add a bit more flavour to my earlier comments.
> >
> > I'm still concerned that if we did do multiple /dev/* files it would allow code
> > to think it has complete control over the device when it really doesn't.
> > Events are just one aspect of that.
> >
> > We have had discussions in the past about allowing multiple userspace consumers
> > for a single buffer, but the conclusion there was that was a job for userspace
> > (daemon or similar) software which can deal with control inter dependencies etc.
> >
> > There are already potential messy corners we don't handle for userspace
> > iio buffers vs in kernel users (what happens if they both try to control the
> > sampling frequency?)  I'm not keen to broaden this problem set.
> > If a device genuinely has separate control and pipelines for different
> > buffers then we are probably better representing that cleanly as
> > an mfd type layer and two separate IIO devices. Its effectively the
> > same a multi chip package.
> >
> > A more classic multibuffer usecase is the one where you have related
> > datastreams that run at different rates (often happens in devices with
> > tagged FIFO elements). These are tightly coupled but we need to split
> > the data stream (or add tagging to our FIFOs.). Another case would be
> > DMA based device that puts channels into buffers that are entirely
> > separate in memory address rather than interleaved.
> >
> > So I still need to put together a PoC, but it feels like there are various
> > software models that will give the illusion of there being separate
> > /dev/* files, but with an aspect of control being possible.
> >
> > 1. Daemon, if present that can hand off chardevs to who needs them
> > 2. Library to make the first user of the buffer responsible for providing
> >    service to other users.  Yes there are races, but I don't think they
> >    are hard to deal in normal usecases.  (retry loops)
>
> Hi Nuno / Others,
>
> Nuno's mention of things being similar for the event anon
> FD to the situation for the buffer anon FDs made me realise there was
> a horrible short cut to a proof of concept that didn't require me
> to wire up a multiple buffer device.
>
> Upshot, is that I've just sent out a (definitely not for merging)
> hacked up version of the iio_event_monitor that can act as server
> or client.  The idea is that the socket handling looks a bit
> like what I'd expect to see hidden away in a library so as to
> allow
>
> 1) Client 1 is after buffer 3.
>    It tries to open the /dev/iio\:deviceX chrdev and succeeds.
>    It spins up a thread with a listening socket for /tmp/iio\:deviceX-magic
>    Continues in main thread to request buffer 3.
> 2) Client 2 is after buffer 2
>    I tries to open the /dev/iio\:deviceX chrdev and fails.
>    It sleeps a moment (reduces chance of race with client 1)
>    It opens a connection to the socket via /tmp/iio\:deviceX-magic
>    Sends a request for the buffer 2 FD.
>    Thread in Client 1 calls the ioctl to get the buffer 2 FD which
>    it then sends on to Client 2 which can use it as if it had
>    requested it directly.
>
> We might want to have a generic server version as well that doesn't
> itself make use of any of the buffers as keeps the model more symmetric
> and reduce common corner cases.
>
> Anyhow the code I put together is terrible, but I wasn't 100% sure
> there weren't any issues passing anon fd file handles and this shows
> that at least in theory the approach I proposed above works.
>
> Test is something like
> ./iio_events_network /dev/iio\:device1
> ./iio_events_network -c
>
> Then make some events happen (I was using the dummy driver and
> the event generator associated with that).
> The server in this PoC just quits after handling off the FD.

The whole code looks good functionally.
If there are any race issues [as discussed here], they can be handled
in the server code.
And if this is the model we try to enforce/propose in userspace, then
all should be fine.


Continuing a bit with the original IIO buffer ioctl(), I talked to
Lars a bit over IRC.
And there was an idea/suggestion to maybe use a struct to pass more
information to the buffer FD.

So, right now the ioctl() just returns an FD.
Would it be worth to extend this to a struct?
What I'm worried about is that it opens the discussion to add more
stuff to that struct.

so now, it would be:

struct iio_buffer_ioctl_data {
            __u32 fd;
            __u32 flags;   // flags for the new FD, which maybe we
could also pass via fcntl()
}

anything else that we would need?

>
> Jonathan
>
> >
> > Jonathan
> >
> >
> > >
> > > - Nuno Sá
> >
>
Jonathan Cameron March 23, 2021, 11:34 a.m. UTC | #15
On Tue, 23 Mar 2021 11:51:04 +0200
Alexandru Ardelean <ardeleanalex@gmail.com> wrote:

> On Sun, Mar 21, 2021 at 7:37 PM Jonathan Cameron
> <jic23@jic23.retrosnub.co.uk> wrote:
> >
> > On Sat, 20 Mar 2021 17:41:00 +0000
> > Jonathan Cameron <jic23@kernel.org> wrote:
> >  
> > > On Mon, 15 Mar 2021 09:58:08 +0000
> > > "Sa, Nuno" <Nuno.Sa@analog.com> wrote:
> > >  
> > > > > -----Original Message-----
> > > > > From: Alexandru Ardelean <ardeleanalex@gmail.com>
> > > > > Sent: Saturday, March 6, 2021 6:01 PM
> > > > > To: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > > Cc: Lars-Peter Clausen <lars@metafoo.de>; zzzzArdelean,
> > > > > zzzzAlexandru <alexandru.Ardelean@analog.com>; LKML <linux-  
> > > > > kernel@vger.kernel.org>; linux-iio <linux-iio@vger.kernel.org>;  
> > > > > Hennerich, Michael <Michael.Hennerich@analog.com>; Jonathan
> > > > > Cameron <jic23@kernel.org>; Sa, Nuno <Nuno.Sa@analog.com>;
> > > > > Bogdan, Dragos <Dragos.Bogdan@analog.com>
> > > > > Subject: Re: [PATCH v6 20/24] iio: buffer: add ioctl() to support opening
> > > > > extra buffers for IIO device
> > > > >
> > > > > [External]
> > > > >
> > > > > On Sun, Feb 28, 2021 at 9:00 PM Jonathan Cameron
> > > > > <Jonathan.Cameron@huawei.com> wrote:  
> > > > > >
> > > > > > On Sun, 28 Feb 2021 16:51:51 +0100
> > > > > > Lars-Peter Clausen <lars@metafoo.de> wrote:
> > > > > >  
> > > > > > > On 2/28/21 3:34 PM, Jonathan Cameron wrote:  
> > > > > > > > On Sun, 28 Feb 2021 09:51:38 +0100
> > > > > > > > Lars-Peter Clausen <lars@metafoo.de> wrote:
> > > > > > > >  
> > > > > > > >> On 2/15/21 11:40 AM, Alexandru Ardelean wrote:  
> > > > > > > >>> With this change, an ioctl() call is added to open a character  
> > > > > device for a  
> > > > > > > >>> buffer. The ioctl() number is 'i' 0x91, which follows the
> > > > > > > >>> IIO_GET_EVENT_FD_IOCTL ioctl.
> > > > > > > >>>
> > > > > > > >>> The ioctl() will return an FD for the requested buffer index.  
> > > > > The indexes  
> > > > > > > >>> are the same from the /sys/iio/devices/iio:deviceX/bufferY  
> > > > > (i.e. the Y  
> > > > > > > >>> variable).
> > > > > > > >>>
> > > > > > > >>> Since there doesn't seem to be a sane way to return the FD for  
> > > > > buffer0 to  
> > > > > > > >>> be the same FD for the /dev/iio:deviceX, this ioctl() will return  
> > > > > another  
> > > > > > > >>> FD for buffer0 (or the first buffer). This duplicate FD will be  
> > > > > able to  
> > > > > > > >>> access the same buffer object (for buffer0) as accessing  
> > > > > directly the  
> > > > > > > >>> /dev/iio:deviceX chardev.
> > > > > > > >>>
> > > > > > > >>> Also, there is no IIO_BUFFER_GET_BUFFER_COUNT ioctl()  
> > > > > implemented, as the  
> > > > > > > >>> index for each buffer (and the count) can be deduced from  
> > > > > the  
> > > > > > > >>> '/sys/bus/iio/devices/iio:deviceX/bufferY' folders (i.e the  
> > > > > number of  
> > > > > > > >>> bufferY folders).
> > > > > > > >>>
> > > > > > > >>> Used following C code to test this:
> > > > > > > >>> -------------------------------------------------------------------
> > > > > > > >>>
> > > > > > > >>>    #include <stdio.h>
> > > > > > > >>>    #include <stdlib.h>
> > > > > > > >>>    #include <unistd.h>
> > > > > > > >>>    #include <sys/ioctl.h>
> > > > > > > >>>    #include <fcntl.h"
> > > > > > > >>>    #include <errno.h>
> > > > > > > >>>
> > > > > > > >>>    #define IIO_BUFFER_GET_FD_IOCTL      _IOWR('i', 0x91, int)
> > > > > > > >>>
> > > > > > > >>> int main(int argc, char *argv[])
> > > > > > > >>> {
> > > > > > > >>>           int fd;
> > > > > > > >>>           int fd1;
> > > > > > > >>>           int ret;
> > > > > > > >>>
> > > > > > > >>>           if ((fd = open("/dev/iio:device0", O_RDWR))<0) {
> > > > > > > >>>                   fprintf(stderr, "Error open() %d errno %d\n",fd,  
> > > > > errno);  
> > > > > > > >>>                   return -1;
> > > > > > > >>>           }
> > > > > > > >>>
> > > > > > > >>>           fprintf(stderr, "Using FD %d\n", fd);
> > > > > > > >>>
> > > > > > > >>>           fd1 = atoi(argv[1]);
> > > > > > > >>>
> > > > > > > >>>           ret = ioctl(fd, IIO_BUFFER_GET_FD_IOCTL, &fd1);
> > > > > > > >>>           if (ret < 0) {
> > > > > > > >>>                   fprintf(stderr, "Error for buffer %d ioctl() %d errno  
> > > > > %d\n", fd1, ret, errno);  
> > > > > > > >>>                   close(fd);
> > > > > > > >>>                   return -1;
> > > > > > > >>>           }
> > > > > > > >>>
> > > > > > > >>>           fprintf(stderr, "Got FD %d\n", fd1);
> > > > > > > >>>
> > > > > > > >>>           close(fd1);
> > > > > > > >>>           close(fd);
> > > > > > > >>>
> > > > > > > >>>           return 0;
> > > > > > > >>> }
> > > > > > > >>> -------------------------------------------------------------------
> > > > > > > >>>
> > > > > > > >>> Results are:
> > > > > > > >>> -------------------------------------------------------------------
> > > > > > > >>>    # ./test 0
> > > > > > > >>>    Using FD 3
> > > > > > > >>>    Got FD 4
> > > > > > > >>>
> > > > > > > >>>    # ./test 1
> > > > > > > >>>    Using FD 3
> > > > > > > >>>    Got FD 4
> > > > > > > >>>
> > > > > > > >>>    # ./test 2
> > > > > > > >>>    Using FD 3
> > > > > > > >>>    Got FD 4
> > > > > > > >>>
> > > > > > > >>>    # ./test 3
> > > > > > > >>>    Using FD 3
> > > > > > > >>>    Got FD 4
> > > > > > > >>>
> > > > > > > >>>    # ls /sys/bus/iio/devices/iio\:device0
> > > > > > > >>>    buffer  buffer0  buffer1  buffer2  buffer3  dev
> > > > > > > >>>    in_voltage_sampling_frequency  in_voltage_scale
> > > > > > > >>>    in_voltage_scale_available
> > > > > > > >>>    name  of_node  power  scan_elements  subsystem  uevent
> > > > > > > >>> -------------------------------------------------------------------
> > > > > > > >>>
> > > > > > > >>> iio:device0 has some fake kfifo buffers attached to an IIO  
> > > > > device.  
> > > > > > > >> For me there is one major problem with this approach. We only  
> > > > > allow one  
> > > > > > > >> application to open /dev/iio:deviceX at a time. This means we  
> > > > > can't have  
> > > > > > > >> different applications access different buffers of the same  
> > > > > device. I  
> > > > > > > >> believe this is a circuital feature.  
> > > > > > > > Thats not quite true (I think - though I've not tested it).  What we  
> > > > > don't  
> > > > > > > > allow is for multiple processes to access them in an unaware  
> > > > > fashion.  
> > > > > > > > My assumption is we can rely on fork + fd passing via appropriate  
> > > > > sockets.  
> > > > > > > >  
> > > > > > > >> It is possible to open the chardev, get the annonfd, close the  
> > > > > chardev  
> > > > > > > >> and keep the annonfd open. Then the next application can do  
> > > > > the same and  
> > > > > > > >> get access to a different buffer. But this has room for race  
> > > > > conditions  
> > > > > > > >> when two applications try this at the very same time.
> > > > > > > >>
> > > > > > > >> We need to somehow address this.  
> > > > > > > > I'd count this as a bug :).  It could be safely done in a particular  
> > > > > custom  
> > > > > > > > system but in general it opens a can of worm.
> > > > > > > >  
> > > > > > > >> I'm also not much of a fan of using ioctls to create annon fds. In  
> > > > > part  
> > > > > > > >> because all the standard mechanisms for access control no  
> > > > > longer work.  
> > > > > > > > The inability to trivially have multiple processes open the anon  
> > > > > fds  
> > > > > > > > without care is one of the things I like most about them.
> > > > > > > >
> > > > > > > > IIO drivers and interfaces really aren't designed for multiple  
> > > > > unaware  
> > > > > > > > processes to access them.  We don't have per process controls  
> > > > > for device  
> > > > > > > > wide sysfs attributes etc.  In general, it would be hard to
> > > > > > > > do due to the complexity of modeling all the interactions  
> > > > > between the  
> > > > > > > > different interfaces (events / buffers / sysfs access) in a generic  
> > > > > fashion.  
> > > > > > > >
> > > > > > > > As such, the model, in my head at least, is that we only want a  
> > > > > single  
> > > > > > > > process to ever be responsible for access control.  That process  
> > > > > can then  
> > > > > > > > assign access to children or via a deliberate action (I think passing  
> > > > > the  
> > > > > > > > anon fd over a unix socket should work for example).  The intent  
> > > > > being  
> > > > > > > > that it is also responsible for mediating access to infrastructure  
> > > > > that  
> > > > > > > > multiple child processes all want to access.
> > > > > > > >
> > > > > > > > As such, having one chrdev isn't a disadvantage because only one  
> > > > > process  
> > > > > > > > should ever open it at a time.  This same process also handles the
> > > > > > > > resource / control mediation.  Therefore we should only have  
> > > > > one file  
> > > > > > > > exposed for all the standard access control mechanisms.
> > > > > > > >  
> > > > > > > Hm, I see your point, but I'm not convinced.
> > > > > > >
> > > > > > > Having to have explicit synchronization makes it difficult to mix and
> > > > > > > match. E.g. at ADI a popular use case for testing was to run some  
> > > > > signal  
> > > > > > > generator application on the TX buffer and some signal analyzer
> > > > > > > application on the RX buffer.
> > > > > > >
> > > > > > > Both can be launched independently and there can be different  
> > > > > types of  
> > > > > > > generator and analyzer applications. Having to have a 3rd  
> > > > > application to  
> > > > > > > arbitrate access makes this quite cumbersome. And I'm afraid that  
> > > > > in  
> > > > > > > reality people might just stick with the two devices model just to  
> > > > > avoid  
> > > > > > > this restriction.  
> > > > > >
> > > > > > I'd argue that's a problem best tackled in a library - though it's a bit
> > > > > > fiddly.  It ought to be possible to make it invisible that this level
> > > > > > of sharing is going on.   The management process you describe would  
> > > > > probably  
> > > > > > be a thread running inside the first process to try and access a given  
> > > > > device.  
> > > > > > A second process failing to open the file with -EBUSY then connects  
> > > > > to  
> > > > > > appropriate socket (via path in /tmp or similar) and asks for the FD.
> > > > > > There are race conditions that might make it fail, but a retry loop  
> > > > > should  
> > > > > > deal with those.
> > > > > >
> > > > > > I agree people might just stick to a two device model and if the  
> > > > > devices  
> > > > > > are independent enough I'm not sure that is the wrong way to  
> > > > > approach the  
> > > > > > problem.  It represents the independence and that the driver is  
> > > > > being careful  
> > > > > > that it both can and is safely handle independent simultaneous  
> > > > > accessors.  
> > > > > > We are always going to have some drivers doing that anyway  
> > > > > because they've  
> > > > > > already been doing that for years.
> > > > > >  
> > > > >
> > > > > This is the last of the 3 patches that I need to re-spin after Lars' review.
> > > > > I have a good handle on the small stuff.
> > > > >
> > > > > I'm not sure about the race-condition about which Lars was talking
> > > > > about.
> > > > > I mean, I get the problem, but is it a problem that we should fix in the
> > > > > kernel?  
> > > >
> > > > Hi all,
> > > >
> > > > FWIW, I think that this really depends on the chosen ABI. If we do use
> > > > the ioctl to return the buffer fd and just allow one app to hold the chardev
> > > > at a time, I agree with Alex that this is not really a race and is just something
> > > > that userspace needs to deal with....
> > > >
> > > > That said and giving my superficial (I did not really read the full series) piece on this,
> > > > I get both Lars and Jonathan points and, personally, it feels that the most natural thing
> > > > would be to have a chardev per buffer...
> > > >
> > > > On the other hand, AFAIC, events are also being handled in the same chardev as
> > > > buffers, which makes things harder in terms of consistency... Events are per device
> > > > and not per buffers right? My point is that, to have a chardev per buffer, it would make
> > > > sense to detach events from the buffer stuff and that seems to be not doable without
> > > > breaking ABI (we would probably need to assume that events and buffer0 are on the
> > > > same chardev).  
> > >
> > > Events are interesting as there is no particular reason to assume the driver
> > > handling buffer0 is the right one to deal with them.  It might just as easily
> > > be the case that they are of interest to a process that is concerned with buffer1.
> > >
> > > To add a bit more flavour to my earlier comments.
> > >
> > > I'm still concerned that if we did do multiple /dev/* files it would allow code
> > > to think it has complete control over the device when it really doesn't.
> > > Events are just one aspect of that.
> > >
> > > We have had discussions in the past about allowing multiple userspace consumers
> > > for a single buffer, but the conclusion there was that was a job for userspace
> > > (daemon or similar) software which can deal with control inter dependencies etc.
> > >
> > > There are already potential messy corners we don't handle for userspace
> > > iio buffers vs in kernel users (what happens if they both try to control the
> > > sampling frequency?)  I'm not keen to broaden this problem set.
> > > If a device genuinely has separate control and pipelines for different
> > > buffers then we are probably better representing that cleanly as
> > > an mfd type layer and two separate IIO devices. Its effectively the
> > > same a multi chip package.
> > >
> > > A more classic multibuffer usecase is the one where you have related
> > > datastreams that run at different rates (often happens in devices with
> > > tagged FIFO elements). These are tightly coupled but we need to split
> > > the data stream (or add tagging to our FIFOs.). Another case would be
> > > DMA based device that puts channels into buffers that are entirely
> > > separate in memory address rather than interleaved.
> > >
> > > So I still need to put together a PoC, but it feels like there are various
> > > software models that will give the illusion of there being separate
> > > /dev/* files, but with an aspect of control being possible.
> > >
> > > 1. Daemon, if present that can hand off chardevs to who needs them
> > > 2. Library to make the first user of the buffer responsible for providing
> > >    service to other users.  Yes there are races, but I don't think they
> > >    are hard to deal in normal usecases.  (retry loops)  
> >
> > Hi Nuno / Others,
> >
> > Nuno's mention of things being similar for the event anon
> > FD to the situation for the buffer anon FDs made me realise there was
> > a horrible short cut to a proof of concept that didn't require me
> > to wire up a multiple buffer device.
> >
> > Upshot, is that I've just sent out a (definitely not for merging)
> > hacked up version of the iio_event_monitor that can act as server
> > or client.  The idea is that the socket handling looks a bit
> > like what I'd expect to see hidden away in a library so as to
> > allow
> >
> > 1) Client 1 is after buffer 3.
> >    It tries to open the /dev/iio\:deviceX chrdev and succeeds.
> >    It spins up a thread with a listening socket for /tmp/iio\:deviceX-magic
> >    Continues in main thread to request buffer 3.
> > 2) Client 2 is after buffer 2
> >    I tries to open the /dev/iio\:deviceX chrdev and fails.
> >    It sleeps a moment (reduces chance of race with client 1)
> >    It opens a connection to the socket via /tmp/iio\:deviceX-magic
> >    Sends a request for the buffer 2 FD.
> >    Thread in Client 1 calls the ioctl to get the buffer 2 FD which
> >    it then sends on to Client 2 which can use it as if it had
> >    requested it directly.
> >
> > We might want to have a generic server version as well that doesn't
> > itself make use of any of the buffers as keeps the model more symmetric
> > and reduce common corner cases.
> >
> > Anyhow the code I put together is terrible, but I wasn't 100% sure
> > there weren't any issues passing anon fd file handles and this shows
> > that at least in theory the approach I proposed above works.
> >
> > Test is something like
> > ./iio_events_network /dev/iio\:device1
> > ./iio_events_network -c
> >
> > Then make some events happen (I was using the dummy driver and
> > the event generator associated with that).
> > The server in this PoC just quits after handling off the FD.  
> 
> The whole code looks good functionally.
> If there are any race issues [as discussed here], they can be handled
> in the server code.
> And if this is the model we try to enforce/propose in userspace, then
> all should be fine.
> 
> 
> Continuing a bit with the original IIO buffer ioctl(), I talked to
> Lars a bit over IRC.
> And there was an idea/suggestion to maybe use a struct to pass more
> information to the buffer FD.
> 
> So, right now the ioctl() just returns an FD.
> Would it be worth to extend this to a struct?
> What I'm worried about is that it opens the discussion to add more
> stuff to that struct.
> 
> so now, it would be:
> 
> struct iio_buffer_ioctl_data {
>             __u32 fd;
>             __u32 flags;   // flags for the new FD, which maybe we
> could also pass via fcntl()
> }
> 
> anything else that we would need?

I have a vague recollection that is is almost always worth adding
some padding to such ioctl data coming out of the kernel.  Gives
flexibility to safely add more stuff later without userspace
failing to allocate enough space etc.

I'm curious though, because this feels backwards. I'd expect the
flags to be more useful passed into the ioctl? i.e. request
a non blocking FD?  Might want to mirror that back again of course.

Jonathan


> 
> >
> > Jonathan
> >  
> > >
> > > Jonathan
> > >
> > >  
> > > >
> > > > - Nuno Sá  
> > >  
> >
Alexandru Ardelean March 24, 2021, 9:10 a.m. UTC | #16
On Tue, Mar 23, 2021 at 1:35 PM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> On Tue, 23 Mar 2021 11:51:04 +0200
> Alexandru Ardelean <ardeleanalex@gmail.com> wrote:
>
> > On Sun, Mar 21, 2021 at 7:37 PM Jonathan Cameron
> > <jic23@jic23.retrosnub.co.uk> wrote:
> > >
> > > On Sat, 20 Mar 2021 17:41:00 +0000
> > > Jonathan Cameron <jic23@kernel.org> wrote:
> > >
> > > > On Mon, 15 Mar 2021 09:58:08 +0000
> > > > "Sa, Nuno" <Nuno.Sa@analog.com> wrote:
> > > >
> > > > > > -----Original Message-----
> > > > > > From: Alexandru Ardelean <ardeleanalex@gmail.com>
> > > > > > Sent: Saturday, March 6, 2021 6:01 PM
> > > > > > To: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > > > Cc: Lars-Peter Clausen <lars@metafoo.de>; zzzzArdelean,
> > > > > > zzzzAlexandru <alexandru.Ardelean@analog.com>; LKML <linux-
> > > > > > kernel@vger.kernel.org>; linux-iio <linux-iio@vger.kernel.org>;
> > > > > > Hennerich, Michael <Michael.Hennerich@analog.com>; Jonathan
> > > > > > Cameron <jic23@kernel.org>; Sa, Nuno <Nuno.Sa@analog.com>;
> > > > > > Bogdan, Dragos <Dragos.Bogdan@analog.com>
> > > > > > Subject: Re: [PATCH v6 20/24] iio: buffer: add ioctl() to support opening
> > > > > > extra buffers for IIO device
> > > > > >
> > > > > > [External]
> > > > > >
> > > > > > On Sun, Feb 28, 2021 at 9:00 PM Jonathan Cameron
> > > > > > <Jonathan.Cameron@huawei.com> wrote:
> > > > > > >
> > > > > > > On Sun, 28 Feb 2021 16:51:51 +0100
> > > > > > > Lars-Peter Clausen <lars@metafoo.de> wrote:
> > > > > > >
> > > > > > > > On 2/28/21 3:34 PM, Jonathan Cameron wrote:
> > > > > > > > > On Sun, 28 Feb 2021 09:51:38 +0100
> > > > > > > > > Lars-Peter Clausen <lars@metafoo.de> wrote:
> > > > > > > > >
> > > > > > > > >> On 2/15/21 11:40 AM, Alexandru Ardelean wrote:
> > > > > > > > >>> With this change, an ioctl() call is added to open a character
> > > > > > device for a
> > > > > > > > >>> buffer. The ioctl() number is 'i' 0x91, which follows the
> > > > > > > > >>> IIO_GET_EVENT_FD_IOCTL ioctl.
> > > > > > > > >>>
> > > > > > > > >>> The ioctl() will return an FD for the requested buffer index.
> > > > > > The indexes
> > > > > > > > >>> are the same from the /sys/iio/devices/iio:deviceX/bufferY
> > > > > > (i.e. the Y
> > > > > > > > >>> variable).
> > > > > > > > >>>
> > > > > > > > >>> Since there doesn't seem to be a sane way to return the FD for
> > > > > > buffer0 to
> > > > > > > > >>> be the same FD for the /dev/iio:deviceX, this ioctl() will return
> > > > > > another
> > > > > > > > >>> FD for buffer0 (or the first buffer). This duplicate FD will be
> > > > > > able to
> > > > > > > > >>> access the same buffer object (for buffer0) as accessing
> > > > > > directly the
> > > > > > > > >>> /dev/iio:deviceX chardev.
> > > > > > > > >>>
> > > > > > > > >>> Also, there is no IIO_BUFFER_GET_BUFFER_COUNT ioctl()
> > > > > > implemented, as the
> > > > > > > > >>> index for each buffer (and the count) can be deduced from
> > > > > > the
> > > > > > > > >>> '/sys/bus/iio/devices/iio:deviceX/bufferY' folders (i.e the
> > > > > > number of
> > > > > > > > >>> bufferY folders).
> > > > > > > > >>>
> > > > > > > > >>> Used following C code to test this:
> > > > > > > > >>> -------------------------------------------------------------------
> > > > > > > > >>>
> > > > > > > > >>>    #include <stdio.h>
> > > > > > > > >>>    #include <stdlib.h>
> > > > > > > > >>>    #include <unistd.h>
> > > > > > > > >>>    #include <sys/ioctl.h>
> > > > > > > > >>>    #include <fcntl.h"
> > > > > > > > >>>    #include <errno.h>
> > > > > > > > >>>
> > > > > > > > >>>    #define IIO_BUFFER_GET_FD_IOCTL      _IOWR('i', 0x91, int)
> > > > > > > > >>>
> > > > > > > > >>> int main(int argc, char *argv[])
> > > > > > > > >>> {
> > > > > > > > >>>           int fd;
> > > > > > > > >>>           int fd1;
> > > > > > > > >>>           int ret;
> > > > > > > > >>>
> > > > > > > > >>>           if ((fd = open("/dev/iio:device0", O_RDWR))<0) {
> > > > > > > > >>>                   fprintf(stderr, "Error open() %d errno %d\n",fd,
> > > > > > errno);
> > > > > > > > >>>                   return -1;
> > > > > > > > >>>           }
> > > > > > > > >>>
> > > > > > > > >>>           fprintf(stderr, "Using FD %d\n", fd);
> > > > > > > > >>>
> > > > > > > > >>>           fd1 = atoi(argv[1]);
> > > > > > > > >>>
> > > > > > > > >>>           ret = ioctl(fd, IIO_BUFFER_GET_FD_IOCTL, &fd1);
> > > > > > > > >>>           if (ret < 0) {
> > > > > > > > >>>                   fprintf(stderr, "Error for buffer %d ioctl() %d errno
> > > > > > %d\n", fd1, ret, errno);
> > > > > > > > >>>                   close(fd);
> > > > > > > > >>>                   return -1;
> > > > > > > > >>>           }
> > > > > > > > >>>
> > > > > > > > >>>           fprintf(stderr, "Got FD %d\n", fd1);
> > > > > > > > >>>
> > > > > > > > >>>           close(fd1);
> > > > > > > > >>>           close(fd);
> > > > > > > > >>>
> > > > > > > > >>>           return 0;
> > > > > > > > >>> }
> > > > > > > > >>> -------------------------------------------------------------------
> > > > > > > > >>>
> > > > > > > > >>> Results are:
> > > > > > > > >>> -------------------------------------------------------------------
> > > > > > > > >>>    # ./test 0
> > > > > > > > >>>    Using FD 3
> > > > > > > > >>>    Got FD 4
> > > > > > > > >>>
> > > > > > > > >>>    # ./test 1
> > > > > > > > >>>    Using FD 3
> > > > > > > > >>>    Got FD 4
> > > > > > > > >>>
> > > > > > > > >>>    # ./test 2
> > > > > > > > >>>    Using FD 3
> > > > > > > > >>>    Got FD 4
> > > > > > > > >>>
> > > > > > > > >>>    # ./test 3
> > > > > > > > >>>    Using FD 3
> > > > > > > > >>>    Got FD 4
> > > > > > > > >>>
> > > > > > > > >>>    # ls /sys/bus/iio/devices/iio\:device0
> > > > > > > > >>>    buffer  buffer0  buffer1  buffer2  buffer3  dev
> > > > > > > > >>>    in_voltage_sampling_frequency  in_voltage_scale
> > > > > > > > >>>    in_voltage_scale_available
> > > > > > > > >>>    name  of_node  power  scan_elements  subsystem  uevent
> > > > > > > > >>> -------------------------------------------------------------------
> > > > > > > > >>>
> > > > > > > > >>> iio:device0 has some fake kfifo buffers attached to an IIO
> > > > > > device.
> > > > > > > > >> For me there is one major problem with this approach. We only
> > > > > > allow one
> > > > > > > > >> application to open /dev/iio:deviceX at a time. This means we
> > > > > > can't have
> > > > > > > > >> different applications access different buffers of the same
> > > > > > device. I
> > > > > > > > >> believe this is a circuital feature.
> > > > > > > > > Thats not quite true (I think - though I've not tested it).  What we
> > > > > > don't
> > > > > > > > > allow is for multiple processes to access them in an unaware
> > > > > > fashion.
> > > > > > > > > My assumption is we can rely on fork + fd passing via appropriate
> > > > > > sockets.
> > > > > > > > >
> > > > > > > > >> It is possible to open the chardev, get the annonfd, close the
> > > > > > chardev
> > > > > > > > >> and keep the annonfd open. Then the next application can do
> > > > > > the same and
> > > > > > > > >> get access to a different buffer. But this has room for race
> > > > > > conditions
> > > > > > > > >> when two applications try this at the very same time.
> > > > > > > > >>
> > > > > > > > >> We need to somehow address this.
> > > > > > > > > I'd count this as a bug :).  It could be safely done in a particular
> > > > > > custom
> > > > > > > > > system but in general it opens a can of worm.
> > > > > > > > >
> > > > > > > > >> I'm also not much of a fan of using ioctls to create annon fds. In
> > > > > > part
> > > > > > > > >> because all the standard mechanisms for access control no
> > > > > > longer work.
> > > > > > > > > The inability to trivially have multiple processes open the anon
> > > > > > fds
> > > > > > > > > without care is one of the things I like most about them.
> > > > > > > > >
> > > > > > > > > IIO drivers and interfaces really aren't designed for multiple
> > > > > > unaware
> > > > > > > > > processes to access them.  We don't have per process controls
> > > > > > for device
> > > > > > > > > wide sysfs attributes etc.  In general, it would be hard to
> > > > > > > > > do due to the complexity of modeling all the interactions
> > > > > > between the
> > > > > > > > > different interfaces (events / buffers / sysfs access) in a generic
> > > > > > fashion.
> > > > > > > > >
> > > > > > > > > As such, the model, in my head at least, is that we only want a
> > > > > > single
> > > > > > > > > process to ever be responsible for access control.  That process
> > > > > > can then
> > > > > > > > > assign access to children or via a deliberate action (I think passing
> > > > > > the
> > > > > > > > > anon fd over a unix socket should work for example).  The intent
> > > > > > being
> > > > > > > > > that it is also responsible for mediating access to infrastructure
> > > > > > that
> > > > > > > > > multiple child processes all want to access.
> > > > > > > > >
> > > > > > > > > As such, having one chrdev isn't a disadvantage because only one
> > > > > > process
> > > > > > > > > should ever open it at a time.  This same process also handles the
> > > > > > > > > resource / control mediation.  Therefore we should only have
> > > > > > one file
> > > > > > > > > exposed for all the standard access control mechanisms.
> > > > > > > > >
> > > > > > > > Hm, I see your point, but I'm not convinced.
> > > > > > > >
> > > > > > > > Having to have explicit synchronization makes it difficult to mix and
> > > > > > > > match. E.g. at ADI a popular use case for testing was to run some
> > > > > > signal
> > > > > > > > generator application on the TX buffer and some signal analyzer
> > > > > > > > application on the RX buffer.
> > > > > > > >
> > > > > > > > Both can be launched independently and there can be different
> > > > > > types of
> > > > > > > > generator and analyzer applications. Having to have a 3rd
> > > > > > application to
> > > > > > > > arbitrate access makes this quite cumbersome. And I'm afraid that
> > > > > > in
> > > > > > > > reality people might just stick with the two devices model just to
> > > > > > avoid
> > > > > > > > this restriction.
> > > > > > >
> > > > > > > I'd argue that's a problem best tackled in a library - though it's a bit
> > > > > > > fiddly.  It ought to be possible to make it invisible that this level
> > > > > > > of sharing is going on.   The management process you describe would
> > > > > > probably
> > > > > > > be a thread running inside the first process to try and access a given
> > > > > > device.
> > > > > > > A second process failing to open the file with -EBUSY then connects
> > > > > > to
> > > > > > > appropriate socket (via path in /tmp or similar) and asks for the FD.
> > > > > > > There are race conditions that might make it fail, but a retry loop
> > > > > > should
> > > > > > > deal with those.
> > > > > > >
> > > > > > > I agree people might just stick to a two device model and if the
> > > > > > devices
> > > > > > > are independent enough I'm not sure that is the wrong way to
> > > > > > approach the
> > > > > > > problem.  It represents the independence and that the driver is
> > > > > > being careful
> > > > > > > that it both can and is safely handle independent simultaneous
> > > > > > accessors.
> > > > > > > We are always going to have some drivers doing that anyway
> > > > > > because they've
> > > > > > > already been doing that for years.
> > > > > > >
> > > > > >
> > > > > > This is the last of the 3 patches that I need to re-spin after Lars' review.
> > > > > > I have a good handle on the small stuff.
> > > > > >
> > > > > > I'm not sure about the race-condition about which Lars was talking
> > > > > > about.
> > > > > > I mean, I get the problem, but is it a problem that we should fix in the
> > > > > > kernel?
> > > > >
> > > > > Hi all,
> > > > >
> > > > > FWIW, I think that this really depends on the chosen ABI. If we do use
> > > > > the ioctl to return the buffer fd and just allow one app to hold the chardev
> > > > > at a time, I agree with Alex that this is not really a race and is just something
> > > > > that userspace needs to deal with....
> > > > >
> > > > > That said and giving my superficial (I did not really read the full series) piece on this,
> > > > > I get both Lars and Jonathan points and, personally, it feels that the most natural thing
> > > > > would be to have a chardev per buffer...
> > > > >
> > > > > On the other hand, AFAIC, events are also being handled in the same chardev as
> > > > > buffers, which makes things harder in terms of consistency... Events are per device
> > > > > and not per buffers right? My point is that, to have a chardev per buffer, it would make
> > > > > sense to detach events from the buffer stuff and that seems to be not doable without
> > > > > breaking ABI (we would probably need to assume that events and buffer0 are on the
> > > > > same chardev).
> > > >
> > > > Events are interesting as there is no particular reason to assume the driver
> > > > handling buffer0 is the right one to deal with them.  It might just as easily
> > > > be the case that they are of interest to a process that is concerned with buffer1.
> > > >
> > > > To add a bit more flavour to my earlier comments.
> > > >
> > > > I'm still concerned that if we did do multiple /dev/* files it would allow code
> > > > to think it has complete control over the device when it really doesn't.
> > > > Events are just one aspect of that.
> > > >
> > > > We have had discussions in the past about allowing multiple userspace consumers
> > > > for a single buffer, but the conclusion there was that was a job for userspace
> > > > (daemon or similar) software which can deal with control inter dependencies etc.
> > > >
> > > > There are already potential messy corners we don't handle for userspace
> > > > iio buffers vs in kernel users (what happens if they both try to control the
> > > > sampling frequency?)  I'm not keen to broaden this problem set.
> > > > If a device genuinely has separate control and pipelines for different
> > > > buffers then we are probably better representing that cleanly as
> > > > an mfd type layer and two separate IIO devices. Its effectively the
> > > > same a multi chip package.
> > > >
> > > > A more classic multibuffer usecase is the one where you have related
> > > > datastreams that run at different rates (often happens in devices with
> > > > tagged FIFO elements). These are tightly coupled but we need to split
> > > > the data stream (or add tagging to our FIFOs.). Another case would be
> > > > DMA based device that puts channels into buffers that are entirely
> > > > separate in memory address rather than interleaved.
> > > >
> > > > So I still need to put together a PoC, but it feels like there are various
> > > > software models that will give the illusion of there being separate
> > > > /dev/* files, but with an aspect of control being possible.
> > > >
> > > > 1. Daemon, if present that can hand off chardevs to who needs them
> > > > 2. Library to make the first user of the buffer responsible for providing
> > > >    service to other users.  Yes there are races, but I don't think they
> > > >    are hard to deal in normal usecases.  (retry loops)
> > >
> > > Hi Nuno / Others,
> > >
> > > Nuno's mention of things being similar for the event anon
> > > FD to the situation for the buffer anon FDs made me realise there was
> > > a horrible short cut to a proof of concept that didn't require me
> > > to wire up a multiple buffer device.
> > >
> > > Upshot, is that I've just sent out a (definitely not for merging)
> > > hacked up version of the iio_event_monitor that can act as server
> > > or client.  The idea is that the socket handling looks a bit
> > > like what I'd expect to see hidden away in a library so as to
> > > allow
> > >
> > > 1) Client 1 is after buffer 3.
> > >    It tries to open the /dev/iio\:deviceX chrdev and succeeds.
> > >    It spins up a thread with a listening socket for /tmp/iio\:deviceX-magic
> > >    Continues in main thread to request buffer 3.
> > > 2) Client 2 is after buffer 2
> > >    I tries to open the /dev/iio\:deviceX chrdev and fails.
> > >    It sleeps a moment (reduces chance of race with client 1)
> > >    It opens a connection to the socket via /tmp/iio\:deviceX-magic
> > >    Sends a request for the buffer 2 FD.
> > >    Thread in Client 1 calls the ioctl to get the buffer 2 FD which
> > >    it then sends on to Client 2 which can use it as if it had
> > >    requested it directly.
> > >
> > > We might want to have a generic server version as well that doesn't
> > > itself make use of any of the buffers as keeps the model more symmetric
> > > and reduce common corner cases.
> > >
> > > Anyhow the code I put together is terrible, but I wasn't 100% sure
> > > there weren't any issues passing anon fd file handles and this shows
> > > that at least in theory the approach I proposed above works.
> > >
> > > Test is something like
> > > ./iio_events_network /dev/iio\:device1
> > > ./iio_events_network -c
> > >
> > > Then make some events happen (I was using the dummy driver and
> > > the event generator associated with that).
> > > The server in this PoC just quits after handling off the FD.
> >
> > The whole code looks good functionally.
> > If there are any race issues [as discussed here], they can be handled
> > in the server code.
> > And if this is the model we try to enforce/propose in userspace, then
> > all should be fine.
> >
> >
> > Continuing a bit with the original IIO buffer ioctl(), I talked to
> > Lars a bit over IRC.
> > And there was an idea/suggestion to maybe use a struct to pass more
> > information to the buffer FD.
> >
> > So, right now the ioctl() just returns an FD.
> > Would it be worth to extend this to a struct?
> > What I'm worried about is that it opens the discussion to add more
> > stuff to that struct.
> >
> > so now, it would be:
> >
> > struct iio_buffer_ioctl_data {
> >             __u32 fd;
> >             __u32 flags;   // flags for the new FD, which maybe we
> > could also pass via fcntl()
> > }
> >
> > anything else that we would need?
>
> I have a vague recollection that is is almost always worth adding
> some padding to such ioctl data coming out of the kernel.  Gives
> flexibility to safely add more stuff later without userspace
> failing to allocate enough space etc.
>
> I'm curious though, because this feels backwards. I'd expect the
> flags to be more useful passed into the ioctl? i.e. request
> a non blocking FD?  Might want to mirror that back again of course.

Personally, I don't know.
I don't have any experiences on this.

So, then I'll do a change to this ioctl() to use a struct.
We can probably add some reserved space?

struct iio_buffer_ioctl_data {
            __u32 fd;
            __u32 flags;
            __u32 reserved1;
            __u32 reserved2;
}

Lars was giving me some articles about ioctls.
One idea was to maybe consider making them multiples of 64 bits.

But reading through one of the docs here:
     https://www.kernel.org/doc/html/latest/driver-api/ioctl.html#interface-versions
it discourages to do interface versions.

But I guess if we plan ahead with some reserved space, it might be
somewhat fine.

I'm still a little green on this stuff.

>
> Jonathan
>
>
> >
> > >
> > > Jonathan
> > >
> > > >
> > > > Jonathan
> > > >
> > > >
> > > > >
> > > > > - Nuno Sá
> > > >
> > >
>
Lars-Peter Clausen March 27, 2021, noon UTC | #17
On 3/24/21 10:10 AM, Alexandru Ardelean wrote:
> On Tue, Mar 23, 2021 at 1:35 PM Jonathan Cameron
> <Jonathan.Cameron@huawei.com> wrote:
>>
[..]
>>>
>>> Continuing a bit with the original IIO buffer ioctl(), I talked to
>>> Lars a bit over IRC.
>>> And there was an idea/suggestion to maybe use a struct to pass more
>>> information to the buffer FD.
>>>
>>> So, right now the ioctl() just returns an FD.
>>> Would it be worth to extend this to a struct?
>>> What I'm worried about is that it opens the discussion to add more
>>> stuff to that struct.
>>>
>>> so now, it would be:
>>>
>>> struct iio_buffer_ioctl_data {
>>>              __u32 fd;
>>>              __u32 flags;   // flags for the new FD, which maybe we
>>> could also pass via fcntl()
>>> }
>>>
>>> anything else that we would need?
>>
>> I have a vague recollection that is is almost always worth adding
>> some padding to such ioctl data coming out of the kernel.  Gives
>> flexibility to safely add more stuff later without userspace
>> failing to allocate enough space etc.
>>
>> I'm curious though, because this feels backwards. I'd expect the
>> flags to be more useful passed into the ioctl? i.e. request
>> a non blocking FD?  Might want to mirror that back again of course.
> 

The struct can be used for both. Passing flags and buffer number in and fd out.

> Personally, I don't know.
> I don't have any experiences on this.
> 
> So, then I'll do a change to this ioctl() to use a struct.
> We can probably add some reserved space?
> 
> struct iio_buffer_ioctl_data {
>              __u32 fd;
>              __u32 flags;
>              __u32 reserved1;
>              __u32 reserved2;
> }

What to make sure of when using reserved fields is to check that they are 0. 
And reject the ioctl if they are not. This is the only way to ensure that old 
applications will continue to work if the struct is updated.

> 
> Lars was giving me some articles about ioctls.
> One idea was to maybe consider making them multiples of 64 bits.
> 
> But reading through one of the docs here:
>       https://www.kernel.org/doc/html/latest/driver-api/ioctl.html#interface-versions
> it discourages to do interface versions.
> 
> But I guess if we plan ahead with some reserved space, it might be
> somewhat fine.
> 
> I'm still a little green on this stuff.
> 
>>
diff mbox series

Patch

diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h
index 7990c759f1f5..062fe16c6c49 100644
--- a/drivers/iio/iio_core.h
+++ b/drivers/iio/iio_core.h
@@ -64,16 +64,16 @@  ssize_t iio_format_value(char *buf, unsigned int type, int size, int *vals);
 #ifdef CONFIG_IIO_BUFFER
 struct poll_table_struct;
 
-__poll_t iio_buffer_poll(struct file *filp,
-			     struct poll_table_struct *wait);
-ssize_t iio_buffer_read_outer(struct file *filp, char __user *buf,
-			      size_t n, loff_t *f_ps);
+__poll_t iio_buffer_poll_wrapper(struct file *filp,
+				 struct poll_table_struct *wait);
+ssize_t iio_buffer_read_wrapper(struct file *filp, char __user *buf,
+				size_t n, loff_t *f_ps);
 
 int iio_buffers_alloc_sysfs_and_mask(struct iio_dev *indio_dev);
 void iio_buffers_free_sysfs_and_mask(struct iio_dev *indio_dev);
 
-#define iio_buffer_poll_addr (&iio_buffer_poll)
-#define iio_buffer_read_outer_addr (&iio_buffer_read_outer)
+#define iio_buffer_poll_addr (&iio_buffer_poll_wrapper)
+#define iio_buffer_read_outer_addr (&iio_buffer_read_wrapper)
 
 void iio_disable_all_buffers(struct iio_dev *indio_dev);
 void iio_buffer_wakeup_poll(struct iio_dev *indio_dev);
diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index 7b5827b91280..4848932d4394 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -9,9 +9,11 @@ 
  * - Better memory allocation techniques?
  * - Alternative access techniques?
  */
+#include <linux/anon_inodes.h>
 #include <linux/kernel.h>
 #include <linux/export.h>
 #include <linux/device.h>
+#include <linux/file.h>
 #include <linux/fs.h>
 #include <linux/cdev.h>
 #include <linux/slab.h>
@@ -89,7 +91,7 @@  static bool iio_buffer_ready(struct iio_dev *indio_dev, struct iio_buffer *buf,
 }
 
 /**
- * iio_buffer_read_outer() - chrdev read for buffer access
+ * iio_buffer_read() - chrdev read for buffer access
  * @filp:	File structure pointer for the char device
  * @buf:	Destination buffer for iio buffer read
  * @n:		First n bytes to read
@@ -101,8 +103,8 @@  static bool iio_buffer_ready(struct iio_dev *indio_dev, struct iio_buffer *buf,
  * Return: negative values corresponding to error codes or ret != 0
  *	   for ending the reading activity
  **/
-ssize_t iio_buffer_read_outer(struct file *filp, char __user *buf,
-			      size_t n, loff_t *f_ps)
+static ssize_t iio_buffer_read(struct file *filp, char __user *buf,
+			       size_t n, loff_t *f_ps)
 {
 	struct iio_dev_buffer_pair *ib = filp->private_data;
 	struct iio_buffer *rb = ib->buffer;
@@ -168,8 +170,8 @@  ssize_t iio_buffer_read_outer(struct file *filp, char __user *buf,
  * Return: (EPOLLIN | EPOLLRDNORM) if data is available for reading
  *	   or 0 for other cases
  */
-__poll_t iio_buffer_poll(struct file *filp,
-			     struct poll_table_struct *wait)
+static __poll_t iio_buffer_poll(struct file *filp,
+				struct poll_table_struct *wait)
 {
 	struct iio_dev_buffer_pair *ib = filp->private_data;
 	struct iio_buffer *rb = ib->buffer;
@@ -184,6 +186,32 @@  __poll_t iio_buffer_poll(struct file *filp,
 	return 0;
 }
 
+ssize_t iio_buffer_read_wrapper(struct file *filp, char __user *buf,
+				size_t n, loff_t *f_ps)
+{
+	struct iio_dev_buffer_pair *ib = filp->private_data;
+	struct iio_buffer *rb = ib->buffer;
+
+	/* check if buffer was opened through new API */
+	if (test_bit(IIO_BUSY_BIT_POS, &rb->flags))
+		return -EBUSY;
+
+	return iio_buffer_read(filp, buf, n, f_ps);
+}
+
+__poll_t iio_buffer_poll_wrapper(struct file *filp,
+				 struct poll_table_struct *wait)
+{
+	struct iio_dev_buffer_pair *ib = filp->private_data;
+	struct iio_buffer *rb = ib->buffer;
+
+	/* check if buffer was opened through new API */
+	if (test_bit(IIO_BUSY_BIT_POS, &rb->flags))
+		return 0;
+
+	return iio_buffer_poll(filp, wait);
+}
+
 /**
  * iio_buffer_wakeup_poll - Wakes up the buffer waitqueue
  * @indio_dev: The IIO device
@@ -1343,6 +1371,96 @@  static void iio_buffer_unregister_legacy_sysfs_groups(struct iio_dev *indio_dev)
 	kfree(iio_dev_opaque->legacy_scan_el_group.attrs);
 }
 
+static int iio_buffer_chrdev_release(struct inode *inode, struct file *filep)
+{
+	struct iio_dev_buffer_pair *ib = filep->private_data;
+	struct iio_dev *indio_dev = ib->indio_dev;
+	struct iio_buffer *buffer = ib->buffer;
+
+	wake_up(&buffer->pollq);
+
+	kfree(ib);
+	clear_bit(IIO_BUSY_BIT_POS, &buffer->flags);
+	iio_device_put(indio_dev);
+
+	return 0;
+}
+
+static const struct file_operations iio_buffer_chrdev_fileops = {
+	.owner = THIS_MODULE,
+	.llseek = noop_llseek,
+	.read = iio_buffer_read,
+	.poll = iio_buffer_poll,
+	.release = iio_buffer_chrdev_release,
+};
+
+static long iio_device_buffer_getfd(struct iio_dev *indio_dev, unsigned long arg)
+{
+	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
+	int __user *ival = (int __user *)arg;
+	struct iio_dev_buffer_pair *ib;
+	struct iio_buffer *buffer;
+	int fd, idx, ret;
+
+	if (copy_from_user(&idx, ival, sizeof(idx)))
+		return -EFAULT;
+
+	if (idx >= iio_dev_opaque->attached_buffers_cnt)
+		return -ENODEV;
+
+	iio_device_get(indio_dev);
+
+	buffer = iio_dev_opaque->attached_buffers[idx];
+
+	if (test_and_set_bit(IIO_BUSY_BIT_POS, &buffer->flags)) {
+		ret = -EBUSY;
+		goto error_iio_dev_put;
+	}
+
+	ib = kzalloc(sizeof(*ib), GFP_KERNEL);
+	if (!ib) {
+		ret = -ENOMEM;
+		goto error_clear_busy_bit;
+	}
+
+	ib->indio_dev = indio_dev;
+	ib->buffer = buffer;
+
+	fd = anon_inode_getfd("iio:buffer", &iio_buffer_chrdev_fileops,
+			      ib, O_RDWR | O_CLOEXEC);
+	if (fd < 0) {
+		ret = fd;
+		goto error_free_ib;
+	}
+
+	if (copy_to_user(ival, &fd, sizeof(fd))) {
+		put_unused_fd(fd);
+		ret = -EFAULT;
+		goto error_free_ib;
+	}
+
+	return fd;
+
+error_free_ib:
+	kfree(ib);
+error_clear_busy_bit:
+	clear_bit(IIO_BUSY_BIT_POS, &buffer->flags);
+error_iio_dev_put:
+	iio_device_put(indio_dev);
+	return ret;
+}
+
+static long iio_device_buffer_ioctl(struct iio_dev *indio_dev, struct file *filp,
+				    unsigned int cmd, unsigned long arg)
+{
+	switch (cmd) {
+	case IIO_BUFFER_GET_FD_IOCTL:
+		return iio_device_buffer_getfd(indio_dev, arg);
+	default:
+		return IIO_IOCTL_UNHANDLED;
+	}
+}
+
 static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
 					     struct iio_dev *indio_dev,
 					     int index)
@@ -1472,6 +1590,7 @@  int iio_buffers_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
 	struct iio_buffer *buffer;
 	int unwind_idx;
 	int ret, i;
+	size_t sz;
 
 	channels = indio_dev->channels;
 	if (channels) {
@@ -1493,6 +1612,18 @@  int iio_buffers_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
 			goto error_unwind_sysfs_and_mask;
 		}
 	}
+	unwind_idx = iio_dev_opaque->attached_buffers_cnt - 1;
+
+	sz = sizeof(*(iio_dev_opaque->buffer_ioctl_handler));
+	iio_dev_opaque->buffer_ioctl_handler = kzalloc(sz, GFP_KERNEL);
+	if (!iio_dev_opaque->buffer_ioctl_handler) {
+		ret = -ENOMEM;
+		goto error_unwind_sysfs_and_mask;
+	}
+
+	iio_dev_opaque->buffer_ioctl_handler->ioctl = iio_device_buffer_ioctl;
+	iio_device_ioctl_handler_register(indio_dev,
+					  iio_dev_opaque->buffer_ioctl_handler);
 
 	return 0;
 
@@ -1514,6 +1645,9 @@  void iio_buffers_free_sysfs_and_mask(struct iio_dev *indio_dev)
 	if (!iio_dev_opaque->attached_buffers_cnt)
 		return;
 
+	iio_device_ioctl_handler_unregister(iio_dev_opaque->buffer_ioctl_handler);
+	kfree(iio_dev_opaque->buffer_ioctl_handler);
+
 	iio_buffer_unregister_legacy_sysfs_groups(indio_dev);
 
 	for (i = iio_dev_opaque->attached_buffers_cnt - 1; i >= 0; i--) {
diff --git a/include/linux/iio/buffer_impl.h b/include/linux/iio/buffer_impl.h
index 768b90c64412..245b32918ae1 100644
--- a/include/linux/iio/buffer_impl.h
+++ b/include/linux/iio/buffer_impl.h
@@ -6,6 +6,8 @@ 
 
 #ifdef CONFIG_IIO_BUFFER
 
+#include <uapi/linux/iio/buffer.h>
+
 struct iio_dev;
 struct iio_buffer;
 
@@ -72,6 +74,9 @@  struct iio_buffer {
 	/** @length: Number of datums in buffer. */
 	unsigned int length;
 
+	/** @flags: File ops flags including busy flag. */
+	unsigned long flags;
+
 	/**  @bytes_per_datum: Size of individual datum including timestamp. */
 	size_t bytes_per_datum;
 
diff --git a/include/linux/iio/iio-opaque.h b/include/linux/iio/iio-opaque.h
index b6ebc04af3e7..32addd5e790e 100644
--- a/include/linux/iio/iio-opaque.h
+++ b/include/linux/iio/iio-opaque.h
@@ -9,6 +9,7 @@ 
  * @event_interface:		event chrdevs associated with interrupt lines
  * @attached_buffers:		array of buffers statically attached by the driver
  * @attached_buffers_cnt:	number of buffers in the array of statically attached buffers
+ * @buffer_ioctl_handler:	ioctl() handler for this IIO device's buffer interface
  * @buffer_list:		list of all buffers currently attached
  * @channel_attr_list:		keep track of automatically created channel
  *				attributes
@@ -28,6 +29,7 @@  struct iio_dev_opaque {
 	struct iio_event_interface	*event_interface;
 	struct iio_buffer		**attached_buffers;
 	unsigned int			attached_buffers_cnt;
+	struct iio_ioctl_handler	*buffer_ioctl_handler;
 	struct list_head		buffer_list;
 	struct list_head		channel_attr_list;
 	struct attribute_group		chan_attr_group;
diff --git a/include/uapi/linux/iio/buffer.h b/include/uapi/linux/iio/buffer.h
new file mode 100644
index 000000000000..13939032b3f6
--- /dev/null
+++ b/include/uapi/linux/iio/buffer.h
@@ -0,0 +1,10 @@ 
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+/* industrial I/O buffer definitions needed both in and out of kernel
+ */
+
+#ifndef _UAPI_IIO_BUFFER_H_
+#define _UAPI_IIO_BUFFER_H_
+
+#define IIO_BUFFER_GET_FD_IOCTL			_IOWR('i', 0x91, int)
+
+#endif /* _UAPI_IIO_BUFFER_H_ */