| Message ID | 20210215104043.91251-21-alexandru.ardelean@analog.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iio: core,buffer: add support for multiple IIO buffers per IIO device | expand |
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;
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.
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
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
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 >
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; > >
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 >
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 > > >
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 > > > > >
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 > > > > > > > >
> -----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á
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á
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á >
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á > > >
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á > > > > >
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á > > > > > > > >
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 --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_ */
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