From: Alexandru Ardelean <ardeleanalex@gmail.com>
To: Lars-Peter Clausen <lars@metafoo.de>
Cc: "Jonathan Cameron" <Jonathan.Cameron@huawei.com>,
"Alexandru Ardelean" <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>,
"Nuno Sá" <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
Date: Sun, 28 Feb 2021 20:09:59 +0200 [thread overview]
Message-ID: <CA+U=DsoqJQP=U8efMUfbPDOTTTiqfpMsEfjK1k6VgFbJ_DBt7w@mail.gmail.com> (raw)
In-Reply-To: <5f9070a5-2c3d-f185-1981-10ec768dbb4a@metafoo.de>
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
>
next prev parent reply other threads:[~2021-02-28 18:11 UTC|newest]
Thread overview: 67+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-02-15 10:40 [PATCH v6 00/24] iio: core,buffer: add support for multiple IIO buffers per IIO device Alexandru Ardelean
2021-02-15 10:40 ` [PATCH v6 01/24] iio: adc: ti_am335x_adc: remove omitted iio_kfifo_free() Alexandru Ardelean
2021-02-15 11:18 ` Jonathan Cameron
2021-02-15 10:40 ` [PATCH v6 02/24] iio: kfifo: add devm_iio_kfifo_buffer_setup() helper Alexandru Ardelean
2021-02-28 8:06 ` Lars-Peter Clausen
2021-02-28 17:45 ` Alexandru Ardelean
2021-02-15 10:40 ` [PATCH v6 03/24] iio: make use of " Alexandru Ardelean
2021-02-15 12:11 ` Jonathan Cameron
2021-02-16 23:46 ` Gwendal Grignou
2021-02-18 8:22 ` Matt Ranostay
2021-02-18 13:25 ` Jonathan Cameron
2021-02-15 10:40 ` [PATCH v6 04/24] iio: accel: sca3000: use " Alexandru Ardelean
2021-02-15 10:40 ` [PATCH v6 05/24] iio: kfifo: un-export devm_iio_kfifo_allocate() function Alexandru Ardelean
2021-02-15 10:40 ` [PATCH v6 06/24] iio: buffer-dma,adi-axi-adc: introduce devm_iio_dmaengine_buffer_setup() Alexandru Ardelean
2021-02-15 10:40 ` [PATCH v6 07/24] docs: ioctl-number.rst: reserve IIO subsystem ioctl() space Alexandru Ardelean
2021-02-15 10:40 ` [PATCH v6 08/24] iio: core: register chardev only if needed Alexandru Ardelean
2021-02-15 10:40 ` [PATCH v6 09/24] iio: core-trigger: make iio_device_register_trigger_consumer() an int return Alexandru Ardelean
2021-02-15 10:40 ` [PATCH v6 10/24] iio: core: rework iio device group creation Alexandru Ardelean
2021-02-15 10:40 ` [PATCH v6 11/24] iio: buffer: group attr count and attr alloc Alexandru Ardelean
2021-02-15 10:40 ` [PATCH v6 12/24] iio: core: merge buffer/ & scan_elements/ attributes Alexandru Ardelean
2021-02-15 10:40 ` [PATCH v6 13/24] iio: add reference to iio buffer on iio_dev_attr Alexandru Ardelean
2021-02-15 10:40 ` [PATCH v6 14/24] iio: buffer: wrap all buffer attributes into iio_dev_attr Alexandru Ardelean
[not found] ` <CGME20210401073947eucas1p2c7f672475bce79dea00e9398cc562073@eucas1p2.samsung.com>
2021-04-01 7:39 ` Marek Szyprowski
2021-04-01 8:26 ` Jonathan Cameron
2021-04-01 11:10 ` Alexandru Ardelean
2022-09-09 8:12 ` Vaittinen, Matti
2022-09-19 8:52 ` [RFT] potential bug with IIO_CONST_ATTR usage with triggered buffers Vaittinen, Matti
2022-09-19 15:32 ` Jonathan Cameron
2022-09-19 17:18 ` Jonathan Cameron
2022-09-19 18:06 ` Vaittinen, Matti
2022-09-24 13:49 ` Jonathan Cameron
2022-09-25 13:28 ` Alexandru Ardelean
2022-10-06 8:33 ` Claudiu.Beznea
2021-02-15 10:40 ` [PATCH v6 15/24] iio: buffer: dmaengine: obtain buffer object from attribute Alexandru Ardelean
2021-02-15 12:44 ` Jonathan Cameron
2021-02-15 10:40 ` [PATCH v6 16/24] iio: core: wrap iio device & buffer into struct for character devices Alexandru Ardelean
2021-02-15 10:40 ` [PATCH v6 17/24] iio: buffer: move __iio_buffer_free_sysfs_and_mask() before alloc Alexandru Ardelean
2021-02-15 10:40 ` [PATCH v6 18/24] iio: dummy: iio_simple_dummy_buffer: use triggered buffer core calls Alexandru Ardelean
2021-02-15 10:40 ` [PATCH v6 19/24] iio: buffer: introduce support for attaching more IIO buffers Alexandru Ardelean
2021-02-28 8:29 ` Lars-Peter Clausen
2021-02-28 17:46 ` Alexandru Ardelean
2021-02-15 10:40 ` [PATCH v6 20/24] iio: buffer: add ioctl() to support opening extra buffers for IIO device Alexandru Ardelean
2021-02-28 7:57 ` Lars-Peter Clausen
2021-02-28 18:04 ` Alexandru Ardelean
2021-02-28 8:51 ` Lars-Peter Clausen
2021-02-28 14:34 ` Jonathan Cameron
2021-02-28 15:51 ` Lars-Peter Clausen
2021-02-28 17:27 ` Jonathan Cameron
2021-03-06 17:00 ` Alexandru Ardelean
2021-03-07 12:13 ` Jonathan Cameron
2021-03-13 18:46 ` Jonathan Cameron
2021-03-15 9:58 ` Sa, Nuno
2021-03-20 17:41 ` Jonathan Cameron
2021-03-21 17:37 ` Jonathan Cameron
2021-03-23 9:51 ` Alexandru Ardelean
2021-03-23 11:34 ` Jonathan Cameron
2021-03-24 9:10 ` Alexandru Ardelean
2021-03-27 12:00 ` Lars-Peter Clausen
2021-02-28 18:09 ` Alexandru Ardelean [this message]
2021-02-15 10:40 ` [PATCH v6 21/24] iio: core: rename 'dev' -> 'indio_dev' in iio_device_alloc() Alexandru Ardelean
2021-02-15 10:40 ` [PATCH v6 22/24] tools: iio: make iioutils_get_type() private in iio_utils Alexandru Ardelean
2021-02-15 10:40 ` [PATCH v6 23/24] tools: iio: privatize globals and functions in iio_generic_buffer.c file Alexandru Ardelean
2021-02-15 10:40 ` [PATCH v6 24/24] tools: iio: convert iio_generic_buffer to use new IIO buffer API Alexandru Ardelean
2021-02-15 13:52 ` Jonathan Cameron
2021-02-15 13:57 ` [PATCH v6 00/24] iio: core,buffer: add support for multiple IIO buffers per IIO device Jonathan Cameron
2021-02-15 14:10 ` Alexandru Ardelean
2021-02-16 11:19 ` Jonathan Cameron
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CA+U=DsoqJQP=U8efMUfbPDOTTTiqfpMsEfjK1k6VgFbJ_DBt7w@mail.gmail.com' \
--to=ardeleanalex@gmail.com \
--cc=Jonathan.Cameron@huawei.com \
--cc=Michael.Hennerich@analog.com \
--cc=alexandru.ardelean@analog.com \
--cc=dragos.bogdan@analog.com \
--cc=jic23@kernel.org \
--cc=lars@metafoo.de \
--cc=linux-iio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=nuno.sa@analog.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).