Re: [RFC] libgpiod public API reviews needed

[Arnd Bergmann <arnd@arndb.de>]

On Mon, Jan 22, 2018 at 1:02 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Mon, Jan 22, 2018 at 10:25 AM, Arnd Bergmann <arnd@arndb.de> wrote:
>> On Mon, Jan 22, 2018 at 9:21 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
>
>>> /**
>>>  * 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.
>
> Sigh yeah that's bad... I guess we will be saved by the 64bit
> word coming first in the struct? (with the security problem
> you mention appearing after the 32 bits in ID.)

The main problem is the size of the data: kfifo_copy_to_user()
can copy multiple records, and while the first one would
remain accessible correctly, the second record contains
garbage if the kernel adds 4 bytes of padding inbetween.

>> - 'real' timestamps are inconvenient because time may jump in
>>   either direction. Time stamps should use 'monotonic' time, i.e.
>>   ktime_get_ns().
>
> So what we have in IIO is that it is configurable what timestamp
> you get.
>
> I've been meaning to fix this by breaking their timestamping
> into lib/ and use the same configurability per-gpiochip for GPIO.
> I guess this is the time to actually do it and stop talking on
> my part :/

Ok, makes sense.

> They also use the same default timestamp though.
>
> commit bc2b7dab629a ("iio:core: timestamping clock selection support")
>
> The code is in IIO in drivers/iio/industrialio-core.c:
>
> /**
>  * iio_get_time_ns() - utility function to get a time stamp for events etc
>  * @indio_dev: device
>  */
> s64 iio_get_time_ns(const struct iio_dev *indio_dev)
> {
>         struct timespec tp;
>
>         switch (iio_device_get_clock(indio_dev)) {
>         case CLOCK_REALTIME:
>                 ktime_get_real_ts(&tp);
>                 break;
>         case CLOCK_MONOTONIC:
>                 ktime_get_ts(&tp);
>                 break;
>         case CLOCK_MONOTONIC_RAW:
>                 getrawmonotonic(&tp);
>                 break;
>         case CLOCK_REALTIME_COARSE:
>                 tp = current_kernel_time();
>                 break;
>         case CLOCK_MONOTONIC_COARSE:
>                 tp = get_monotonic_coarse();
>                 break;
>         case CLOCK_BOOTTIME:
>                 get_monotonic_boottime(&tp);
>                 break;
>         case CLOCK_TAI:
>                 timekeeping_clocktai(&tp);
>                 break;
>         default:
>                 BUG();
>         }
>
>         return timespec_to_ns(&tp);
> }
> EXPORT_SYMBOL(iio_get_time_ns);
>
> Which clock is used is configured per-device in sysfs.
>
> It's pretty neat I think.

To be honest, I think this is overcomplicating things. While I
have a patch to change the interface names and make them
all ktime_get_*_ns(), most of these clocks make no sense
at all for a random user space interface, mainly because
I wouldn't trust user space programmers to make an
informed decision which of those seven to use.

        Arnd