Re: [RFC] libgpiod public API reviews needed

[Linus Walleij <linus.walleij@linaro.org>]

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

[Other mail]
> - On anything other than x86-32, you are leaking 32 bits of kernel stack
>  data for each event, which might be a security issue.

OK discussed in following mails, I'll patch this.

> - '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 :/

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.

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

Prior to the above change, all timestamps in IIO were also using
ktime_get_real_ns(), they changed this without adding a new
ABI so I don't see why GPIO should. Userspace is not using
these timestamps much, yet. If there are users who want anything
else than a relative timestamp, Bartosz probably know who they
are.

The reason GPIO was using ktime_get_real_ns() was that the
code was copied from IIO so we got their mistake as legacy,
and I guess it is fair that we deal with the legacy the same way
they did. The users are pretty much the same. (Stuff like
GNURadio.)

Yours,
Linus Walleij