Thread (67 messages) 67 messages, 12 authors, 2022-10-06

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

From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
Date: 2021-02-28 14:36:39
Also in: lkml

On Sun, 28 Feb 2021 09:51:38 +0100
Lars-Peter Clausen [off-list ref] wrote:
On 2/15/21 11:40 AM, Alexandru Ardelean wrote:
quoted
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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help