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

From: Lars-Peter Clausen <lars@metafoo.de>
To: Alexandru Ardelean <ardeleanalex@gmail.com>,
	Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Jonathan Cameron <jic23@jic23.retrosnub.co.uk>,
	"Sa, Nuno" <Nuno.Sa@analog.com>,
	"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>,
	"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: Sat, 27 Mar 2021 13:00:18 +0100	[thread overview]
Message-ID: <77db0332-4fd1-fb5f-8c22-10653139f3e7@metafoo.de> (raw)
In-Reply-To: <CA+U=DspJkK=sqy=va7mds0cOfJrS3nEd3-pymjztuYYNaG8vPA@mail.gmail.com>

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.
> 
>>