[RFC] libgpiod public API reviews needed

[RFC] libgpiod public API reviews needed

[Bartosz Golaszewski]

Hi,

I would like to release libgpiod v1.0 soon (preferably before FOSDEM
2018). The first version (v0.x series) was written in a matter of two
weeks and the API was not well designed. Since v0.3 release I've been
working on redesigning the public API to make it simplier and more
useful.

I want to commit to a stable interface for the library starting from
v1.0 but it would be great if I could get some reviews first - it's
basically only about reviewing a single public header: include/gpiod.h
in which all public symbols are documented. Any implementation details
can always be changed later.

The most prominent changes since 0.3 include:
- removal of custom error handling in favor of errnos
- merge of two separate interfaces for event requests and regular line requests
- redesign of the simple API
- addition of a bunch of helpers for line requests
- redesign of the iterator API
- making the use of prefixes more consistent
- renaming of symbols all over the place
- and various minor tweaks

As far as usefulness goes - the user space tools that are included all
use this API and it looks pretty clean.

The project is hosted at kernel.org:

    https://git.kernel.org/pub/scm/libs/libgpiod/libgpiod.git/

and the code that needs review is on the master branch.

Thanks in advance to anyone who'll find the time to take a look.

Best regards,
Bartosz Golaszewski

Re: [RFC] libgpiod public API reviews needed

[Clemens Gruber]

Hi Bartosz,

On Fri, Jan 19, 2018 at 02:28:40PM +0100, Bartosz Golaszewski wrote:
> Hi,
> 
> I would like to release libgpiod v1.0 soon (preferably before FOSDEM
> 2018). The first version (v0.x series) was written in a matter of two
> weeks and the API was not well designed. Since v0.3 release I've been
> working on redesigning the public API to make it simplier and more
> useful.
> 
> I want to commit to a stable interface for the library starting from
> v1.0 but it would be great if I could get some reviews first - it's
> basically only about reviewing a single public header: include/gpiod.h
> in which all public symbols are documented. Any implementation details
> can always be changed later.
> 
> The most prominent changes since 0.3 include:
> - removal of custom error handling in favor of errnos
> - merge of two separate interfaces for event requests and regular line requests
> - redesign of the simple API
> - addition of a bunch of helpers for line requests
> - redesign of the iterator API
> - making the use of prefixes more consistent
> - renaming of symbols all over the place
> - and various minor tweaks
> 
> As far as usefulness goes - the user space tools that are included all
> use this API and it looks pretty clean.
> 
> The project is hosted at kernel.org:
> 
>     https://git.kernel.org/pub/scm/libs/libgpiod/libgpiod.git/
> 
> and the code that needs review is on the master branch.
> 
> Thanks in advance to anyone who'll find the time to take a look.

Looks good to me, great work!

Only one question:

- GPIOD_LINE_BULK_MAX_LINES
Would increasing this in the future break the API in your opinion? And
if so, is 64 enough?


I also have a few minor patches in the works but they are only
implementation and build system related, not touching gpiod.h.

Best regards,
Clemens

> 
> Best regards,
> Bartosz Golaszewski

Re: [RFC] libgpiod public API reviews needed

[Linus Walleij]

On Fri, Jan 19, 2018 at 2:28 PM, Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> I want to commit to a stable interface for the library starting from
> v1.0 but it would be great if I could get some reviews first - it's
> basically only about reviewing a single public header: include/gpiod.h

I have no real objections, then, I'm not a great API designer at all.

- In my opinion, all design should consider Rusty Russell's design
  manifesto:
  http://sweng.the-davies.net/Home/rustys-api-design-manifesto
  Just ask yourself the questions in the manifesto to the function
  signature.

- Things named with the infix "simple" *_simple_* *SIMPLE* etc.
  This is a weasel word.
  https://en.wikipedia.org/wiki/Weasel_word
  I have been adviced against ever using that in code, because the
  idea of what is "simple" is extremely subjective. A function with
  six obscure parameters, is that "simple"?

  I would have called it *rudimentary*, *coarse"* or *plain* or something
  similar less subjective.

- gpiod_simple_event_handle_cb uses struct timespec.
  This can create problems.
  On 32 vs 64 bit platforms, because struct timespec is of different
  size on 32 vs 64bit platforms, I think. (Looping in Arnd to verify.)
  You might think "well it is either used on 32bit or on 64bit,
  and everything else is compiled for that so what."
  But in reality you have things like Python bindings, and those get
  a *real* problem when things are in struct timespec, because they
  map byte-by-byte and precompile code doing this map and
  redistribute. Disaster.
  With libmtp I often wish I has just used ISO 8601
  https://en.wikipedia.org/wiki/ISO_8601
  even if it means creating a string on one side and parsing it on
  the other, because it is unambigous.
  This might be an especially bad idea of mine but think it over.

Yours,
Linus Walleij

Re: [RFC] libgpiod public API reviews needed

[Bartosz Golaszewski]

2018-01-20 17:02 GMT+01:00 Clemens Gruber <clemens.gruber@pqgruber.com>:
> Hi Bartosz,
>
> On Fri, Jan 19, 2018 at 02:28:40PM +0100, Bartosz Golaszewski wrote:
>> Hi,
>>
>> I would like to release libgpiod v1.0 soon (preferably before FOSDEM
>> 2018). The first version (v0.x series) was written in a matter of two
>> weeks and the API was not well designed. Since v0.3 release I've been
>> working on redesigning the public API to make it simplier and more
>> useful.
>>
>> I want to commit to a stable interface for the library starting from
>> v1.0 but it would be great if I could get some reviews first - it's
>> basically only about reviewing a single public header: include/gpiod.h
>> in which all public symbols are documented. Any implementation details
>> can always be changed later.
>>
>> The most prominent changes since 0.3 include:
>> - removal of custom error handling in favor of errnos
>> - merge of two separate interfaces for event requests and regular line requests
>> - redesign of the simple API
>> - addition of a bunch of helpers for line requests
>> - redesign of the iterator API
>> - making the use of prefixes more consistent
>> - renaming of symbols all over the place
>> - and various minor tweaks
>>
>> As far as usefulness goes - the user space tools that are included all
>> use this API and it looks pretty clean.
>>
>> The project is hosted at kernel.org:
>>
>>     https://git.kernel.org/pub/scm/libs/libgpiod/libgpiod.git/
>>
>> and the code that needs review is on the master branch.
>>
>> Thanks in advance to anyone who'll find the time to take a look.
>
> Looks good to me, great work!
>
> Only one question:
>
> - GPIOD_LINE_BULK_MAX_LINES
> Would increasing this in the future break the API in your opinion? And
> if so, is 64 enough?
>

This value corresponds to GPIOHANDLES_MAX in linux' UAPI. It's not
very likely that'll be ever increased.

Thanks,
Bartosz

Re: [RFC] libgpiod public API reviews needed

[Bartosz Golaszewski]

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 want to commit to a stable interface for the library starting from
>> v1.0 but it would be great if I could get some reviews first - it's
>> basically only about reviewing a single public header: include/gpiod.h
>
> I have no real objections, then, I'm not a great API designer at all.
>
> - In my opinion, all design should consider Rusty Russell's design
>   manifesto:
>   http://sweng.the-davies.net/Home/rustys-api-design-manifesto
>   Just ask yourself the questions in the manifesto to the function
>   signature.
>
> - Things named with the infix "simple" *_simple_* *SIMPLE* etc.
>   This is a weasel word.
>   https://en.wikipedia.org/wiki/Weasel_word
>   I have been adviced against ever using that in code, because the
>   idea of what is "simple" is extremely subjective. A function with
>   six obscure parameters, is that "simple"?
>
>   I would have called it *rudimentary*, *coarse"* or *plain* or something
>   similar less subjective.
>

I was thinking about something like _contextless_ or _ctxless_ but it
would make the already long function names even longer...

> - gpiod_simple_event_handle_cb uses struct timespec.
>   This can create problems.
>   On 32 vs 64 bit platforms, because struct timespec is of different
>   size on 32 vs 64bit platforms, I think. (Looping in Arnd to verify.)
>   You might think "well it is either used on 32bit or on 64bit,
>   and everything else is compiled for that so what."
>   But in reality you have things like Python bindings, and those get
>   a *real* problem when things are in struct timespec, because they
>   map byte-by-byte and precompile code doing this map and
>   redistribute. Disaster.

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?

Thanks,
Bartosz Golaszewski

>   With libmtp I often wish I has just used ISO 8601
>   https://en.wikipedia.org/wiki/ISO_8601
>   even if it means creating a string on one side and parsing it on
>   the other, because it is unambigous.
>   This might be an especially bad idea of mine but think it over.

Re: [RFC] libgpiod public API reviews needed

[Arnd Bergmann]

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?

In a lot of cases, a simple 64-bit nanosecond counter using CLOCK_MONOTONIC
timestamps is the most robust and simple solution.

         Arnd

Re: [RFC] libgpiod public API reviews needed

[Linus Walleij]

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();

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

Yours,
Linus Walleij

Re: [RFC] libgpiod public API reviews needed

[Arnd Bergmann]

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.
- 'real' timestamps are inconvenient because time may jump in
  either direction. Time stamps should use 'monotonic' time, i.e.
  ktime_get_ns().

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

      Arnd

Re: [RFC] libgpiod public API reviews needed

[Arnd Bergmann]

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:
>> 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.
> - 'real' timestamps are inconvenient because time may jump in
>   either direction. Time stamps should use 'monotonic' time, i.e.
>   ktime_get_ns().

and third:

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

         Arnd

Re: [RFC] libgpiod public API reviews needed

[Bartosz Golaszewski]

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

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

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

Best regards,
Bartosz Golaszewski

Re: [RFC] libgpiod public API reviews needed

[Arnd Bergmann]

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

Re: [RFC] libgpiod public API reviews needed

[Linus Walleij]

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

Re: [RFC] libgpiod public API reviews needed

[Arnd Bergmann]

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

Re: [RFC] libgpiod public API reviews needed

[Ludovic Desroches]

Hi,

On Fri, Jan 19, 2018 at 02:28:40PM +0100, Bartosz Golaszewski wrote:
> Hi,
> 
> I would like to release libgpiod v1.0 soon (preferably before FOSDEM
> 2018). The first version (v0.x series) was written in a matter of two
> weeks and the API was not well designed. Since v0.3 release I've been
> working on redesigning the public API to make it simplier and more
> useful.
> 
> I want to commit to a stable interface for the library starting from
> v1.0 but it would be great if I could get some reviews first - it's
> basically only about reviewing a single public header: include/gpiod.h
> in which all public symbols are documented. Any implementation details
> can always be changed later.
> 
> The most prominent changes since 0.3 include:
> - removal of custom error handling in favor of errnos
> - merge of two separate interfaces for event requests and regular line requests
> - redesign of the simple API
> - addition of a bunch of helpers for line requests
> - redesign of the iterator API
> - making the use of prefixes more consistent
> - renaming of symbols all over the place
> - and various minor tweaks
> 
> As far as usefulness goes - the user space tools that are included all
> use this API and it looks pretty clean.
> 
> The project is hosted at kernel.org:
> 
>     https://git.kernel.org/pub/scm/libs/libgpiod/libgpiod.git/
> 
> and the code that needs review is on the master branch.
> 
> Thanks in advance to anyone who'll find the time to take a look.
> 

Thanks for your work, I am using your library and I am trying to promote it.

Some people were confused concerning the naming when requesting line(s)
as output. 'init_val(s)' may be better than 'default_val(s)'.

The callback for gpiod_simple_set_value(_multiple) is optional, you may mention
it. Concerning the multiple variant, I would enforce that the callback
is called when ALL the values are set.
I don't see the added value of this callback. What's the difference with
executing it depending on gpiod_simple_set_value(_multiple) return value?

Regards

Ludovic

Re: [RFC] libgpiod public API reviews needed

[Bartosz Golaszewski]

2018-01-23 15:15 GMT+01:00 Ludovic Desroches <ludovic.desroches@microchip.com>:
> Hi,
>
> On Fri, Jan 19, 2018 at 02:28:40PM +0100, Bartosz Golaszewski wrote:
>> Hi,
>>
>> I would like to release libgpiod v1.0 soon (preferably before FOSDEM
>> 2018). The first version (v0.x series) was written in a matter of two
>> weeks and the API was not well designed. Since v0.3 release I've been
>> working on redesigning the public API to make it simplier and more
>> useful.
>>
>> I want to commit to a stable interface for the library starting from
>> v1.0 but it would be great if I could get some reviews first - it's
>> basically only about reviewing a single public header: include/gpiod.h
>> in which all public symbols are documented. Any implementation details
>> can always be changed later.
>>
>> The most prominent changes since 0.3 include:
>> - removal of custom error handling in favor of errnos
>> - merge of two separate interfaces for event requests and regular line requests
>> - redesign of the simple API
>> - addition of a bunch of helpers for line requests
>> - redesign of the iterator API
>> - making the use of prefixes more consistent
>> - renaming of symbols all over the place
>> - and various minor tweaks
>>
>> As far as usefulness goes - the user space tools that are included all
>> use this API and it looks pretty clean.
>>
>> The project is hosted at kernel.org:
>>
>>     https://git.kernel.org/pub/scm/libs/libgpiod/libgpiod.git/
>>
>> and the code that needs review is on the master branch.
>>
>> Thanks in advance to anyone who'll find the time to take a look.
>>
>
> Thanks for your work, I am using your library and I am trying to promote it.
>
> Some people were confused concerning the naming when requesting line(s)
> as output. 'init_val(s)' may be better than 'default_val(s)'.
>
> The callback for gpiod_simple_set_value(_multiple) is optional, you may mention
> it. Concerning the multiple variant, I would enforce that the callback
> is called when ALL the values are set.
> I don't see the added value of this callback. What's the difference with
> executing it depending on gpiod_simple_set_value(_multiple) return value?
>

It exists to allow to execute some code or pause the execution while
the device file of the gpiochip is open and the lines are requested.
You can take a look at src/tools/gpioset.c to see how it's used in an
actual program.

For other hints: they make sense, I'll add them soon.

Thanks,
Bartosz

Re: [RFC] libgpiod public API reviews needed

[Bartosz Golaszewski]

2018-01-22 12:12 GMT+01:00 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 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.
>

Besides using struct timespec, I have other structs in gpiod.h which
would pose problems for ctypes-based python bindings - for example
struct gpiod_line_request_config which contains a pointer (C string).
If instead of using ctypes, I'd just use python's C extension modules,
is there any other trap caused by different data layout between 32 and
64 bit architectures that I should be aware of?

Also: if python bindings are the only victim of that - I'm ok with
timespec. It's up to bindings to provide a correct interface after
all, isn't it?

Best regards,
Bartosz Golaszewski

Re: [RFC] libgpiod public API reviews needed

[Bartosz Golaszewski]

2018-01-23 15:15 GMT+01:00 Ludovic Desroches <ludovic.desroches@microchip.com>:
> Hi,
>
> On Fri, Jan 19, 2018 at 02:28:40PM +0100, Bartosz Golaszewski wrote:
>> Hi,
>>
>> I would like to release libgpiod v1.0 soon (preferably before FOSDEM
>> 2018). The first version (v0.x series) was written in a matter of two
>> weeks and the API was not well designed. Since v0.3 release I've been
>> working on redesigning the public API to make it simplier and more
>> useful.
>>
>> I want to commit to a stable interface for the library starting from
>> v1.0 but it would be great if I could get some reviews first - it's
>> basically only about reviewing a single public header: include/gpiod.h
>> in which all public symbols are documented. Any implementation details
>> can always be changed later.
>>
>> The most prominent changes since 0.3 include:
>> - removal of custom error handling in favor of errnos
>> - merge of two separate interfaces for event requests and regular line requests
>> - redesign of the simple API
>> - addition of a bunch of helpers for line requests
>> - redesign of the iterator API
>> - making the use of prefixes more consistent
>> - renaming of symbols all over the place
>> - and various minor tweaks
>>
>> As far as usefulness goes - the user space tools that are included all
>> use this API and it looks pretty clean.
>>
>> The project is hosted at kernel.org:
>>
>>     https://git.kernel.org/pub/scm/libs/libgpiod/libgpiod.git/
>>
>> and the code that needs review is on the master branch.
>>
>> Thanks in advance to anyone who'll find the time to take a look.
>>
>
> Thanks for your work, I am using your library and I am trying to promote it.
>
> Some people were confused concerning the naming when requesting line(s)
> as output. 'init_val(s)' may be better than 'default_val(s)'.
>

Now I'm seeing that default_val(s) is the name used both within the
kernel and in the linux user API. I'll leave it like this then to not
confuse users.

Thanks,
Bartosz

Re: [RFC] libgpiod public API reviews needed

[Ludovic Desroches]

On Thu, Jan 25, 2018 at 05:29:02PM +0100, Bartosz Golaszewski wrote:
> 2018-01-23 15:15 GMT+01:00 Ludovic Desroches <ludovic.desroches@microchip.com>:
> > Hi,
> >
> > On Fri, Jan 19, 2018 at 02:28:40PM +0100, Bartosz Golaszewski wrote:
> >> Hi,
> >>
> >> I would like to release libgpiod v1.0 soon (preferably before FOSDEM
> >> 2018). The first version (v0.x series) was written in a matter of two
> >> weeks and the API was not well designed. Since v0.3 release I've been
> >> working on redesigning the public API to make it simplier and more
> >> useful.
> >>
> >> I want to commit to a stable interface for the library starting from
> >> v1.0 but it would be great if I could get some reviews first - it's
> >> basically only about reviewing a single public header: include/gpiod.h
> >> in which all public symbols are documented. Any implementation details
> >> can always be changed later.
> >>
> >> The most prominent changes since 0.3 include:
> >> - removal of custom error handling in favor of errnos
> >> - merge of two separate interfaces for event requests and regular line requests
> >> - redesign of the simple API
> >> - addition of a bunch of helpers for line requests
> >> - redesign of the iterator API
> >> - making the use of prefixes more consistent
> >> - renaming of symbols all over the place
> >> - and various minor tweaks
> >>
> >> As far as usefulness goes - the user space tools that are included all
> >> use this API and it looks pretty clean.
> >>
> >> The project is hosted at kernel.org:
> >>
> >>     https://git.kernel.org/pub/scm/libs/libgpiod/libgpiod.git/
> >>
> >> and the code that needs review is on the master branch.
> >>
> >> Thanks in advance to anyone who'll find the time to take a look.
> >>
> >
> > Thanks for your work, I am using your library and I am trying to promote it.
> >
> > Some people were confused concerning the naming when requesting line(s)
> > as output. 'init_val(s)' may be better than 'default_val(s)'.
> >
> 
> Now I'm seeing that default_val(s) is the name used both within the
> kernel and in the linux user API. I'll leave it like this then to not
> confuse users.

I understand, you can mention it in the param description, it should be
enough.

Regards

Ludovic