Re: [RFC] libgpiod public API reviews needed

[Arnd Bergmann <arnd@arndb.de>]

On Mon, Jan 22, 2018 at 12:02 PM, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> 2018-01-22 10:25 GMT+01:00 Arnd Bergmann <arnd@arndb.de>:
>> On Mon, Jan 22, 2018 at 9:21 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
>>> On Sun, Jan 21, 2018 at 11:18 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>>>> On Sun, Jan 21, 2018 at 10:30 PM, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>>>>> 2018-01-21 16:49 GMT+01:00 Linus Walleij <linus.walleij@linaro.org>:
>>>>>> On Fri, Jan 19, 2018 at 2:28 PM, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>>>>
>>>>> I was not aware of this, but it seems you're right! Nice catch, thanks.
>>>>>
>>>>> How about defining a local struct gpiod_timespec with both seconds and
>>>>> nanoseconds explicitly defined to uint64_t?
>>>>
>>>> Where is that timestamp generated? Is this purely a user space interface
>>>> with the time read from gettimeofday(), or are we talking about  a new
>>>> kernel-to-user interface?
>>>
>>> This is in include/uapi/linux/gpio.h:
>>>
>>> /**
>>>  * struct gpioevent_data - The actual event being pushed to userspace
>>>  * @timestamp: best estimate of time of event occurrence, in nanoseconds
>>>  * @id: event identifier
>>>  */
>>> struct gpioevent_data {
>>>         __u64 timestamp;
>>>         __u32 id;
>>> };
>>>
>>> It is the same as is used for IIO. Inside the kernel this ultimately
>>> comes from ktime_get_real_ns();
>>
>> Ah, too bad, that already contains two mistakes:
>>
>> - on x86, the structures are incompatible between 32-bit and 64-bit
>>   user space, as the former has no padding.
>
> Is this really an issue? Do distros really ship the same bytecode for
> 32 and 64 bit architecures? I have never run into such problems
> despite having used different python bindings for C libraries (I'm not
> sure however how many of them dealt with any visible C structs).

It's a huge issue, yes. You should be able to run an 32-bit distro
or just a standalone 32-bit binary with a 64-bit kernel. This driver
is otherwise written carefully to allow that, and it will work on all
other architectures AFAICT, just not on x86.

>> - 'real' timestamps are inconvenient because time may jump in
>>   either direction. Time stamps should use 'monotonic' time, i.e.
>>   ktime_get_ns().
>>
>
> @Linus: this doesn't really break the ABI - how do you feel about
> switching to using it in gpiolib.c?

It is an incompatible ABI change, the question here is whether anyone
actually cares. If nothing relies on the timestamps being in
CLOCK_REALTIME domain, then it can be changed, the question
is just how you want to prove that this is the case.

>>>> In a lot of cases, a simple 64-bit nanosecond counter using CLOCK_MONOTONIC
>>>> timestamps is the most robust and simple solution.
>>>
>>> Bartosz also seems to think it is the best so would vote to go
>>> for that and we have one problem less.
>>
>> Could we introduce a new ioctl to replace the gpioevent_data() and
>> use a better interface then?
>>
>
> For the security concern - I guess it would be enough to just zero
> gpioevent_data in lineevent_irq_thread() before putting it into the
> FIFO?

Yes, that part is easy to fix.

      Arnd