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

2021-03-27 Thread Lars-Peter Clausen

On 3/24/21 10:10 AM, Alexandru Ardelean wrote:

On Tue, Mar 23, 2021 at 1:35 PM Jonathan Cameron
 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.





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

2021-03-24 Thread Alexandru Ardelean
On Tue, Mar 23, 2021 at 1:35 PM Jonathan Cameron
 wrote:
>
> On Tue, 23 Mar 2021 11:51:04 +0200
> Alexandru Ardelean  wrote:
>
> > On Sun, Mar 21, 2021 at 7:37 PM Jonathan Cameron
> >  wrote:
> > >
> > > On Sat, 20 Mar 2021 17:41:00 +
> > > Jonathan Cameron  wrote:
> > >
> > > > On Mon, 15 Mar 2021 09:58:08 +
> > > > "Sa, Nuno"  wrote:
> > > >
> > > > > > -Original Message-
> > > > > > From: Alexandru Ardelean 
> > > > > > Sent: Saturday, March 6, 2021 6:01 PM
> > > > > > To: Jonathan Cameron 
> > > > > > Cc: Lars-Peter Clausen ; Ardelean,
> > > > > > zzzzAlexandru ; LKML  > > > > > ker...@vger.kernel.org>; linux-iio ;
> > > > > > Hennerich, Michael ; Jonathan
> > > > > > Cameron ; Sa, Nuno ;
> > > > > > Bogdan, Dragos 
> > > > > > 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
> > > > > >  wrote:
> > > > > > >
> > > > > > > On Sun, 28 Feb 2021 16:51:51 +0100
> > > > > > > Lars-Peter Clausen  wrote:
> > > > > > >
> > > > > > > > On 2/28/21 3:34 PM, Jonathan Cameron wrote:
> > > > > > > > > On Sun, 28 Feb 2021 09:51:38 +0100
> > > > > > > > > Lars-Peter Clausen  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 
> > > > > > > > >>>#include 
> > > > > > > > >>>#include 
> > > > > > > > >>>#include 

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

2021-03-23 Thread Jonathan Cameron
On Tue, 23 Mar 2021 11:51:04 +0200
Alexandru Ardelean  wrote:

> On Sun, Mar 21, 2021 at 7:37 PM Jonathan Cameron
>  wrote:
> >
> > On Sat, 20 Mar 2021 17:41:00 +
> > Jonathan Cameron  wrote:
> >  
> > > On Mon, 15 Mar 2021 09:58:08 +
> > > "Sa, Nuno"  wrote:
> > >  
> > > > > -Original Message-
> > > > > From: Alexandru Ardelean 
> > > > > Sent: Saturday, March 6, 2021 6:01 PM
> > > > > To: Jonathan Cameron 
> > > > > Cc: Lars-Peter Clausen ; Ardelean,
> > > > > Alexandru ; LKML  > > > > ker...@vger.kernel.org>; linux-iio ;  
> > > > > Hennerich, Michael ; Jonathan
> > > > > Cameron ; Sa, Nuno ;
> > > > > Bogdan, Dragos 
> > > > > 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
> > > > >  wrote:  
> > > > > >
> > > > > > On Sun, 28 Feb 2021 16:51:51 +0100
> > > > > > Lars-Peter Clausen  wrote:
> > > > > >  
> > > > > > > On 2/28/21 3:34 PM, Jonathan Cameron wrote:  
> > > > > > > > On Sun, 28 Feb 2021 09:51:38 +0100
> > > > > > > > Lars-Peter Clausen  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 
> > > > > > > >>>#include 
> > > > > > > >>>#include 
> > > > > > > >>>#include 
> > > > > > > >>>#include  > > > > > > >>>#include 
> > > > > > > >>>
> > > > > > > >>>#define IIO_BUFFER_GET_FD_IOCTL  _IOWR('i', 0x91, int)
> > > > > > > >>>
> > > > > > > >>> int main(int argc, char *

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

2021-03-23 Thread Alexandru Ardelean
On Sun, Mar 21, 2021 at 7:37 PM Jonathan Cameron
 wrote:
>
> On Sat, 20 Mar 2021 17:41:00 +
> Jonathan Cameron  wrote:
>
> > On Mon, 15 Mar 2021 09:58:08 +
> > "Sa, Nuno"  wrote:
> >
> > > > -Original Message-
> > > > From: Alexandru Ardelean 
> > > > Sent: Saturday, March 6, 2021 6:01 PM
> > > > To: Jonathan Cameron 
> > > > Cc: Lars-Peter Clausen ; Ardelean,
> > > > Alexandru ; LKML  > > > ker...@vger.kernel.org>; linux-iio ;
> > > > Hennerich, Michael ; Jonathan
> > > > Cameron ; Sa, Nuno ;
> > > > Bogdan, Dragos 
> > > > 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
> > > >  wrote:
> > > > >
> > > > > On Sun, 28 Feb 2021 16:51:51 +0100
> > > > > Lars-Peter Clausen  wrote:
> > > > >
> > > > > > On 2/28/21 3:34 PM, Jonathan Cameron wrote:
> > > > > > > On Sun, 28 Feb 2021 09:51:38 +0100
> > > > > > > Lars-Peter Clausen  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 
> > > > > > >>>#include 
> > > > > > >>>#include 
> > > > > > >>>#include 
> > > > > > >>>#include  > > > > > >>>#include 
> > > > > > >>>
> > > > > > >>>#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;
> > > > > &g

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

2021-03-21 Thread Jonathan Cameron
On Sat, 20 Mar 2021 17:41:00 +
Jonathan Cameron  wrote:

> On Mon, 15 Mar 2021 09:58:08 +
> "Sa, Nuno"  wrote:
> 
> > > -Original Message-
> > > From: Alexandru Ardelean 
> > > Sent: Saturday, March 6, 2021 6:01 PM
> > > To: Jonathan Cameron 
> > > Cc: Lars-Peter Clausen ; Ardelean,
> > > Alexandru ; LKML  > > ker...@vger.kernel.org>; linux-iio ;
> > > Hennerich, Michael ; Jonathan
> > > Cameron ; Sa, Nuno ;
> > > Bogdan, Dragos 
> > > 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
> > >  wrote:
> > > >
> > > > On Sun, 28 Feb 2021 16:51:51 +0100
> > > > Lars-Peter Clausen  wrote:
> > > >
> > > > > On 2/28/21 3:34 PM, Jonathan Cameron wrote:
> > > > > > On Sun, 28 Feb 2021 09:51:38 +0100
> > > > > > Lars-Peter Clausen  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 
> > > > > >>>#include 
> > > > > >>>#include 
> > > > > >>>#include 
> > > > > >>>#include  > > > > >>>#include 
> > > > > >>>
> > > > > >>>#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,

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

2021-03-20 Thread Jonathan Cameron
On Mon, 15 Mar 2021 09:58:08 +
"Sa, Nuno"  wrote:

> > -Original Message-
> > From: Alexandru Ardelean 
> > Sent: Saturday, March 6, 2021 6:01 PM
> > To: Jonathan Cameron 
> > Cc: Lars-Peter Clausen ; Ardelean,
> > Alexandru ; LKML  > ker...@vger.kernel.org>; linux-iio ;  
> > Hennerich, Michael ; Jonathan
> > Cameron ; Sa, Nuno ;
> > Bogdan, Dragos 
> > 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
> >  wrote:  
> > >
> > > On Sun, 28 Feb 2021 16:51:51 +0100
> > > Lars-Peter Clausen  wrote:
> > >  
> > > > On 2/28/21 3:34 PM, Jonathan Cameron wrote:  
> > > > > On Sun, 28 Feb 2021 09:51:38 +0100
> > > > > Lars-Peter Clausen  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 
> > > > >>>#include 
> > > > >>>#include 
> > > > >>>#include 
> > > > >>>#include  > > > >>>#include 
> > > > >>>
> > > > >>>#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, );
> > > > >>>   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);
> > > > >>> 

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

2021-03-15 Thread Sa, Nuno

> -Original Message-
> From: Alexandru Ardelean 
> Sent: Saturday, March 6, 2021 6:01 PM
> To: Jonathan Cameron 
> Cc: Lars-Peter Clausen ; Ardelean,
> Alexandru ; LKML  ker...@vger.kernel.org>; linux-iio ;
> Hennerich, Michael ; Jonathan
> Cameron ; Sa, Nuno ;
> Bogdan, Dragos 
> 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
>  wrote:
> >
> > On Sun, 28 Feb 2021 16:51:51 +0100
> > Lars-Peter Clausen  wrote:
> >
> > > On 2/28/21 3:34 PM, Jonathan Cameron wrote:
> > > > On Sun, 28 Feb 2021 09:51:38 +0100
> > > > Lars-Peter Clausen  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 
> > > >>>#include 
> > > >>>#include 
> > > >>>#include 
> > > >>>#include  > > >>>#include 
> > > >>>
> > > >>>#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, );
> > > >>>   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
> > > &

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

2021-03-13 Thread Jonathan Cameron
On Sun, 7 Mar 2021 12:13:08 +
Jonathan Cameron  wrote:

> On Sat, 6 Mar 2021 19:00:52 +0200
> Alexandru Ardelean  wrote:
> 
> > On Sun, Feb 28, 2021 at 9:00 PM Jonathan Cameron
> >  wrote:  
> > >
> > > On Sun, 28 Feb 2021 16:51:51 +0100
> > > Lars-Peter Clausen  wrote:
> > >
> > > > On 2/28/21 3:34 PM, Jonathan Cameron wrote:
> > > > > On Sun, 28 Feb 2021 09:51:38 +0100
> > > > > Lars-Peter Clausen  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 
> > > > >>>#include 
> > > > >>>#include 
> > > > >>>#include 
> > > > >>>#include  > > > >>>#include 
> > > > >>>
> > > > >>>#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, );
> > > > >>>   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
> 

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

2021-03-07 Thread Jonathan Cameron
On Sat, 6 Mar 2021 19:00:52 +0200
Alexandru Ardelean  wrote:

> On Sun, Feb 28, 2021 at 9:00 PM Jonathan Cameron
>  wrote:
> >
> > On Sun, 28 Feb 2021 16:51:51 +0100
> > Lars-Peter Clausen  wrote:
> >  
> > > On 2/28/21 3:34 PM, Jonathan Cameron wrote:  
> > > > On Sun, 28 Feb 2021 09:51:38 +0100
> > > > Lars-Peter Clausen  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 
> > > >>>#include 
> > > >>>#include 
> > > >>>#include 
> > > >>>#include  > > >>>#include 
> > > >>>
> > > >>>#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, );
> > > >>>   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 

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

2021-03-06 Thread Alexandru Ardelean
On Sun, Feb 28, 2021 at 9:00 PM Jonathan Cameron
 wrote:
>
> On Sun, 28 Feb 2021 16:51:51 +0100
> Lars-Peter Clausen  wrote:
>
> > On 2/28/21 3:34 PM, Jonathan Cameron wrote:
> > > On Sun, 28 Feb 2021 09:51:38 +0100
> > > Lars-Peter Clausen  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 
> > >>>#include 
> > >>>#include 
> > >>>#include 
> > >>>#include  > >>>#include 
> > >>>
> > >>>#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, );
> > >>>   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 

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

2021-02-28 Thread Alexandru Ardelean
On Sun, Feb 28, 2021 at 5:54 PM Lars-Peter Clausen  wrote:
>
> On 2/28/21 3:34 PM, Jonathan Cameron wrote:
> > On Sun, 28 Feb 2021 09:51:38 +0100
> > Lars-Peter Clausen  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 
> >>>#include 
> >>>#include 
> >>>#include 
> >>>#include  >>>#include 
> >>>
> >>>#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, );
> >>>   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 

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

2021-02-28 Thread Alexandru Ardelean
On Sun, Feb 28, 2021 at 9:58 AM Lars-Peter Clausen  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(, 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, >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", _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, , 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, >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 intattached_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_headbuffer_list;
> >   struct list_headchannel_attr_list;
> >   struct attribute_group  chan_attr_group;
>
>


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

2021-02-28 Thread Jonathan Cameron
On Sun, 28 Feb 2021 16:51:51 +0100
Lars-Peter Clausen  wrote:

> On 2/28/21 3:34 PM, Jonathan Cameron wrote:
> > On Sun, 28 Feb 2021 09:51:38 +0100
> > Lars-Peter Clausen  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 
> >>>#include 
> >>>#include 
> >>>#include 
> >>>#include  >>>#include 
> >>>
> >>>#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, );
> >>>   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
> > 

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

2021-02-28 Thread Lars-Peter Clausen

On 2/28/21 3:34 PM, Jonathan Cameron wrote:

On Sun, 28 Feb 2021 09:51:38 +0100
Lars-Peter Clausen  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 
   #include 
   #include 
   #include 
   #include 

   #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, );
  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 

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

2021-02-28 Thread Jonathan Cameron
On Sun, 28 Feb 2021 09:51:38 +0100
Lars-Peter Clausen  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 
> >   #include 
> >   #include 
> >   #include 
> >   #include  >   #include 
> >
> >   #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, );
> >  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 

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

2021-02-28 Thread Lars-Peter Clausen

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 
  #include 
  #include 
  #include 
  #include 

  #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, );
 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.




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

2021-02-28 Thread Lars-Peter Clausen

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(, 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, >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", _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, , 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, >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 intattached_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_headbuffer_list;
struct list_headchannel_attr_list;
struct attribute_group  chan_attr_group;





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

2021-02-15 Thread Alexandru Ardelean
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 
 #include 
 #include 
 #include 
 #include 

 #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, );
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 
---
 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

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 (_buffer_poll)
-#define iio_buffer_read_outer_addr (_buffer_read_outer)
+#define iio_buffer_poll_addr (_buffer_poll_wrapper)
+#define iio_buffer_read_outer_addr (_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 
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -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