| Message ID | 20200922023151.387447-5-warthog618@gmail.com |
|---|---|
| State | Accepted |
| Commit | b53911aa872db462be2e5f1dd611b25c4c2e663b |
| Headers | show |
| Series | gpio: cdev: add uAPI v2 | expand |
On Tue, Sep 22, 2020 at 4:34 AM Kent Gibson <warthog618@gmail.com> wrote: > +/** > + * struct gpio_v2_line_attribute - a configurable attribute of a line > + * @id: attribute identifier with value from &enum gpio_v2_line_attr_id > + * @padding: reserved for future use and must be zero filled > + * @flags: if id is GPIO_V2_LINE_ATTR_ID_FLAGS, the flags for the GPIO > + * line, with values from enum gpio_v2_line_flag, such as > + * GPIO_V2_LINE_FLAG_ACTIVE_LOW, GPIO_V2_LINE_FLAG_OUTPUT etc, OR:ed > + * together. This overrides the default flags contained in the &struct > + * gpio_v2_line_config for the associated line. > + * @values: if id is GPIO_V2_LINE_ATTR_ID_OUTPUT_VALUES, a bitmap > + * containing the values to which the lines will be set, with each bit > + * number corresponding to the index into &struct > + * gpio_v2_line_request.offsets. > + * @debounce_period_us: if id is GPIO_V2_LINE_ATTR_ID_DEBOUNCE, the desired > + * debounce period, in microseconds > + */ > +struct gpio_v2_line_attribute { > + __u32 id; > + __u32 padding; > + union { > + __aligned_u64 flags; > + __aligned_u64 values; > + __u32 debounce_period_us; > + }; > +}; Having different-sized members in the union makes it hard for something like strace to print the contents. How about just making them all __aligned_u64 even when 32 bits are sufficient? > +struct gpio_v2_line_request { > + __u32 offsets[GPIO_V2_LINES_MAX]; > + char consumer[GPIO_MAX_NAME_SIZE]; > + struct gpio_v2_line_config config; > + __u32 num_lines; > + __u32 event_buffer_size; > + /* Pad to fill implicit padding and reserve space for future use. */ > + __u32 padding[5]; > + __s32 fd; > +}; > +struct gpio_v2_line_info { > + char name[GPIO_MAX_NAME_SIZE]; > + char consumer[GPIO_MAX_NAME_SIZE]; > + __u32 offset; > + __u32 num_attrs; > + __aligned_u64 flags; > + struct gpio_v2_line_attribute attrs[GPIO_V2_LINE_NUM_ATTRS_MAX]; > + /* Space reserved for future use. */ > + __u32 padding[4]; > +}; These are both several hundred bytes long, requiring a lot of data to be copied to the stack and take up space there. I see this is not actually much different for the v1 API, but I wonder if there has been any analysis of whether this has a noticeable effect on application runtime. Arnd
On Tue, Sep 22, 2020 at 9:41 AM Arnd Bergmann <arnd@arndb.de> wrote: > > On Tue, Sep 22, 2020 at 4:34 AM Kent Gibson <warthog618@gmail.com> wrote: > > +/** > > + * struct gpio_v2_line_attribute - a configurable attribute of a line > > + * @id: attribute identifier with value from &enum gpio_v2_line_attr_id > > + * @padding: reserved for future use and must be zero filled > > + * @flags: if id is GPIO_V2_LINE_ATTR_ID_FLAGS, the flags for the GPIO > > + * line, with values from enum gpio_v2_line_flag, such as > > + * GPIO_V2_LINE_FLAG_ACTIVE_LOW, GPIO_V2_LINE_FLAG_OUTPUT etc, OR:ed > > + * together. This overrides the default flags contained in the &struct > > + * gpio_v2_line_config for the associated line. > > + * @values: if id is GPIO_V2_LINE_ATTR_ID_OUTPUT_VALUES, a bitmap > > + * containing the values to which the lines will be set, with each bit > > + * number corresponding to the index into &struct > > + * gpio_v2_line_request.offsets. > > + * @debounce_period_us: if id is GPIO_V2_LINE_ATTR_ID_DEBOUNCE, the desired > > + * debounce period, in microseconds > > + */ > > +struct gpio_v2_line_attribute { > > + __u32 id; > > + __u32 padding; > > + union { > > + __aligned_u64 flags; > > + __aligned_u64 values; > > + __u32 debounce_period_us; > > + }; > > +}; > > Having different-sized members in the union makes it hard for > something like strace to print the contents. How about just making > them all __aligned_u64 even when 32 bits are sufficient? > Ah yes, adding support for GPIO ioctl()'s to strace has been on my TODO list for 3 years now. :( > > +struct gpio_v2_line_request { > > + __u32 offsets[GPIO_V2_LINES_MAX]; > > + char consumer[GPIO_MAX_NAME_SIZE]; > > + struct gpio_v2_line_config config; > > + __u32 num_lines; > > + __u32 event_buffer_size; > > + /* Pad to fill implicit padding and reserve space for future use. */ > > + __u32 padding[5]; > > + __s32 fd; > > +}; > > > +struct gpio_v2_line_info { > > + char name[GPIO_MAX_NAME_SIZE]; > > + char consumer[GPIO_MAX_NAME_SIZE]; > > + __u32 offset; > > + __u32 num_attrs; > > + __aligned_u64 flags; > > + struct gpio_v2_line_attribute attrs[GPIO_V2_LINE_NUM_ATTRS_MAX]; > > + /* Space reserved for future use. */ > > + __u32 padding[4]; > > +}; > > These are both several hundred bytes long, requiring a lot of data > to be copied to the stack and take up space there. I see this is not > actually much different for the v1 API, but I wonder if there has been > any analysis of whether this has a noticeable effect on application > runtime. > The main difference between this and V1 is that we can now pass arguments for flags as defined in struct gpio_v2_line_attribute. I haven't measured the impact but first: this is not a hot path (retrieving line info is not done a lot like reading line events or setting/getting values), and second: this structure is 280 bytes long which is still less than a page so we should not face more context switches than with a smaller structure, right? Bartosz
On Tue, Sep 22, 2020 at 11:50:51AM +0200, Bartosz Golaszewski wrote: > On Tue, Sep 22, 2020 at 9:41 AM Arnd Bergmann <arnd@arndb.de> wrote: > > > > On Tue, Sep 22, 2020 at 4:34 AM Kent Gibson <warthog618@gmail.com> wrote: > > > +/** > > > + * struct gpio_v2_line_attribute - a configurable attribute of a line > > > + * @id: attribute identifier with value from &enum gpio_v2_line_attr_id > > > + * @padding: reserved for future use and must be zero filled > > > + * @flags: if id is GPIO_V2_LINE_ATTR_ID_FLAGS, the flags for the GPIO > > > + * line, with values from enum gpio_v2_line_flag, such as > > > + * GPIO_V2_LINE_FLAG_ACTIVE_LOW, GPIO_V2_LINE_FLAG_OUTPUT etc, OR:ed > > > + * together. This overrides the default flags contained in the &struct > > > + * gpio_v2_line_config for the associated line. > > > + * @values: if id is GPIO_V2_LINE_ATTR_ID_OUTPUT_VALUES, a bitmap > > > + * containing the values to which the lines will be set, with each bit > > > + * number corresponding to the index into &struct > > > + * gpio_v2_line_request.offsets. > > > + * @debounce_period_us: if id is GPIO_V2_LINE_ATTR_ID_DEBOUNCE, the desired > > > + * debounce period, in microseconds > > > + */ > > > +struct gpio_v2_line_attribute { > > > + __u32 id; > > > + __u32 padding; > > > + union { > > > + __aligned_u64 flags; > > > + __aligned_u64 values; > > > + __u32 debounce_period_us; > > > + }; > > > +}; > > > > Having different-sized members in the union makes it hard for > > something like strace to print the contents. How about just making > > them all __aligned_u64 even when 32 bits are sufficient? > > > > Ah yes, adding support for GPIO ioctl()'s to strace has been on my > TODO list for 3 years now. :( > Great - you beat me to it - I was going to ask if we could fix strace ;-). If it can be taught the id/union semantics I'd rather do that, so we could then also format the 64-bit values appropriately. e.g. flags and values are both bitmaps but the bits are interpreted very differently. Cheers, Kent.
On Tue, Sep 22, 2020 at 5:34 AM Kent Gibson <warthog618@gmail.com> wrote: > > Add a new version of the uAPI to address existing 32/64-bit alignment > issues, add support for debounce and event sequence numbers, allow > requested lines with different configurations, and provide some future > proofing by adding padding reserved for future use. > > The alignment issue relates to the gpioevent_data, which packs to different > sizes on 32-bit and 64-bit platforms. That creates problems for 32-bit apps > running on 64-bit kernels. uAPI v2 addresses that particular issue, and > the problem more generally, by adding pad fields that explicitly pad > structs out to 64-bit boundaries, so they will pack to the same size now, > and even if some of the reserved padding is used for __u64 fields in the > future. > > The new structs have been analysed with pahole to ensure that they > are sized as expected and contain no implicit padding. > > The lack of future proofing in v1 makes it impossible to, for example, > add the debounce feature that is included in v2. > The future proofing is addressed by providing configurable attributes in > line config and reserved padding in all structs for future features. > Specifically, the line request, config, info, info_changed and event > structs receive updated versions and new ioctls. > > As the majority of the structs and ioctls were being replaced, it is > opportune to rework some of the other aspects of the uAPI: > > v1 has three different flags fields, each with their own separate > bit definitions. In v2 that is collapsed to one - gpio_v2_line_flag. > > The handle and event requests are merged into a single request, the line > request, as the two requests were mostly the same other than the edge > detection provided by event requests. As a byproduct, the v2 uAPI allows > for multiple lines producing edge events on the same line handle. > This is a new capability as v1 only supports a single line in an event > request. > > As a consequence, there are now only two types of file handle to be > concerned with, the chip and the line, and it is clearer which ioctls > apply to which type of handle. > > There is also some minor renaming of fields for consistency compared to > their v1 counterparts, e.g. offset rather than lineoffset or line_offset, > and consumer rather than consumer_label. > > Additionally, v1 GPIOHANDLES_MAX becomes GPIO_V2_LINES_MAX in v2 for > clarity, and the gpiohandle_data __u8 array becomes a bitmap in > gpio_v2_line_values. > > The v2 uAPI is mostly a reorganisation and extension of v1, so userspace > code, particularly libgpiod, should readily port to it. ... > +struct gpio_v2_line_config { > + __aligned_u64 flags; > + __u32 num_attrs; > + /* Pad to fill implicit padding and reserve space for future use. */ > + __u32 padding[5]; Probably I somehow missed the answer, but why do we need 5 here and not 1? > + struct gpio_v2_line_config_attribute attrs[GPIO_V2_LINE_NUM_ATTRS_MAX]; > +}; ... > +struct gpio_v2_line_request { > + __u32 offsets[GPIO_V2_LINES_MAX]; > + char consumer[GPIO_MAX_NAME_SIZE]; > + struct gpio_v2_line_config config; > + __u32 num_lines; > + __u32 event_buffer_size; > + /* Pad to fill implicit padding and reserve space for future use. */ > + __u32 padding[5]; Ditto. > + __s32 fd; > +}; ... > +struct gpio_v2_line_info { > + char name[GPIO_MAX_NAME_SIZE]; > + char consumer[GPIO_MAX_NAME_SIZE]; > + __u32 offset; > + __u32 num_attrs; > + __aligned_u64 flags; > + struct gpio_v2_line_attribute attrs[GPIO_V2_LINE_NUM_ATTRS_MAX]; > + /* Space reserved for future use. */ > + __u32 padding[4]; Here two comments as in previous patches, why this went after attribute structures and why 2 is not enough? > +}; ... > +struct gpio_v2_line_info_changed { > + struct gpio_v2_line_info info; > + __aligned_u64 timestamp_ns; > + __u32 event_type; > + /* Pad struct to 64-bit boundary and reserve space for future use. */ > + __u32 padding[5]; Again, why 5 and not 1? > +}; ... > +struct gpio_v2_line_event { > + __aligned_u64 timestamp_ns; > + __u32 id; > + __u32 offset; > + __u32 seqno; > + __u32 line_seqno; > + /* Space reserved for future use. */ > + __u32 padding[6]; Why 6 and not 2? And here actually sizeof() can be a version. So, I still see possible versioning issues with ABI. > +}; -- With Best Regards, Andy Shevchenko
On Wed, Sep 23, 2020 at 01:04:05PM +0300, Andy Shevchenko wrote: > On Tue, Sep 22, 2020 at 5:34 AM Kent Gibson <warthog618@gmail.com> wrote: > > [snip] > > There is also some minor renaming of fields for consistency compared to > > their v1 counterparts, e.g. offset rather than lineoffset or line_offset, > > and consumer rather than consumer_label. > > > > Additionally, v1 GPIOHANDLES_MAX becomes GPIO_V2_LINES_MAX in v2 for > > clarity, and the gpiohandle_data __u8 array becomes a bitmap in > > gpio_v2_line_values. > > > > The v2 uAPI is mostly a reorganisation and extension of v1, so userspace > > code, particularly libgpiod, should readily port to it. > > ... > > > +struct gpio_v2_line_config { > > + __aligned_u64 flags; > > + __u32 num_attrs; > > > + /* Pad to fill implicit padding and reserve space for future use. */ > > + __u32 padding[5]; > > Probably I somehow missed the answer, but why do we need 5 here and not 1? > Sorry, I got tired of repeating myself, and just acked that we disagree on the approach here. Your suggestion to use the size for version would result in an explosion of ioctl signatures - every time we add a field we have to add a new ioctl and handle it separately in gpio_ioctl() or linereq_ioctl(). Instead what we do here is reserve some space for future use - that we can replace with fields without changing the signature. The padding is required to be zeroed now, and any future use will take a 0 to mean "leave alone". The sizes are a guestimate as to what may be needed in the future, and as such are almost certainly wrong - but hopefully on the high side. If that fails we can always fall back to your approach. Cheers, Kent.
On Wed, Sep 23, 2020 at 1:30 PM Kent Gibson <warthog618@gmail.com> wrote: > > On Wed, Sep 23, 2020 at 01:04:05PM +0300, Andy Shevchenko wrote: > > On Tue, Sep 22, 2020 at 5:34 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > > [snip] > > > > There is also some minor renaming of fields for consistency compared to > > > their v1 counterparts, e.g. offset rather than lineoffset or line_offset, > > > and consumer rather than consumer_label. > > > > > > Additionally, v1 GPIOHANDLES_MAX becomes GPIO_V2_LINES_MAX in v2 for > > > clarity, and the gpiohandle_data __u8 array becomes a bitmap in > > > gpio_v2_line_values. > > > > > > The v2 uAPI is mostly a reorganisation and extension of v1, so userspace > > > code, particularly libgpiod, should readily port to it. > > > > ... > > > > > +struct gpio_v2_line_config { > > > + __aligned_u64 flags; > > > + __u32 num_attrs; > > > > > + /* Pad to fill implicit padding and reserve space for future use. */ > > > + __u32 padding[5]; > > > > Probably I somehow missed the answer, but why do we need 5 here and not 1? > > > > Sorry, I got tired of repeating myself, and just acked that we disagree > on the approach here. > > Your suggestion to use the size for version would result in an > explosion of ioctl signatures - every time we add a field we have to add > a new ioctl and handle it separately in gpio_ioctl() or linereq_ioctl(). No, you just add __u32 version; // implies sizeof() check as well. Look for examples of existing ABIs (e.g. perf ABI). > Instead what we do here is reserve some space for future use - that we > can replace with fields without changing the signature. > The padding is required to be zeroed now, and any future use will take > a 0 to mean "leave alone". > > The sizes are a guestimate as to what may be needed in the future, and > as such are almost certainly wrong - but hopefully on the high side. > If that fails we can always fall back to your approach. I see. So, we have no agreement on these pieces. Linus and Bart can decide what to do, but I think either way has pros and cons. So, guys, I am fine with everything else here, except this versioning issue and waste of resources. -- With Best Regards, Andy Shevchenko
On Wed, Sep 23, 2020 at 1:16 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Wed, Sep 23, 2020 at 1:30 PM Kent Gibson <warthog618@gmail.com> wrote: > > On Wed, Sep 23, 2020 at 01:04:05PM +0300, Andy Shevchenko wrote: > > > On Tue, Sep 22, 2020 at 5:34 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > There is also some minor renaming of fields for consistency compared to > > > > their v1 counterparts, e.g. offset rather than lineoffset or line_offset, > > > > and consumer rather than consumer_label. > > > > > > > > Additionally, v1 GPIOHANDLES_MAX becomes GPIO_V2_LINES_MAX in v2 for > > > > clarity, and the gpiohandle_data __u8 array becomes a bitmap in > > > > gpio_v2_line_values. > > > > > > > > The v2 uAPI is mostly a reorganisation and extension of v1, so userspace > > > > code, particularly libgpiod, should readily port to it. > > > > > > ... > > > > > > > +struct gpio_v2_line_config { > > > > + __aligned_u64 flags; > > > > + __u32 num_attrs; > > > > > > > + /* Pad to fill implicit padding and reserve space for future use. */ > > > > + __u32 padding[5]; > > > > > > Probably I somehow missed the answer, but why do we need 5 here and not 1? > > > > > > > Sorry, I got tired of repeating myself, and just acked that we disagree > > on the approach here. > > > > Your suggestion to use the size for version would result in an > > explosion of ioctl signatures - every time we add a field we have to add > > a new ioctl and handle it separately in gpio_ioctl() or linereq_ioctl(). > > No, you just add > __u32 version; // implies sizeof() check as well. > > Look for examples of existing ABIs (e.g. perf ABI). Please don't ever add a version field to an ioctl structure, this has been shown to cause more problems than it solves many times in the past... Having some reserved fields can be helpful, as long as the kernel returns an error in case any of the unknown fields are nonzero. I'd also prefer fewer than five reserved fields, but that is not as important, just use as many as are likely to be used in the future, but not more: It's easy to add one more ioctl command in case the expectations are wrong, but I agree you wouldn't want a new command every time another field gets added if the past has shown that this happens a lot. Arnd
On Wed, Sep 23, 2020 at 3:19 PM Arnd Bergmann <arnd@arndb.de> wrote: > On Wed, Sep 23, 2020 at 1:16 PM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > On Wed, Sep 23, 2020 at 1:30 PM Kent Gibson <warthog618@gmail.com> wrote: > > > On Wed, Sep 23, 2020 at 01:04:05PM +0300, Andy Shevchenko wrote: > > > > On Tue, Sep 22, 2020 at 5:34 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > > There is also some minor renaming of fields for consistency compared to > > > > > their v1 counterparts, e.g. offset rather than lineoffset or line_offset, > > > > > and consumer rather than consumer_label. > > > > > > > > > > Additionally, v1 GPIOHANDLES_MAX becomes GPIO_V2_LINES_MAX in v2 for > > > > > clarity, and the gpiohandle_data __u8 array becomes a bitmap in > > > > > gpio_v2_line_values. > > > > > > > > > > The v2 uAPI is mostly a reorganisation and extension of v1, so userspace > > > > > code, particularly libgpiod, should readily port to it. > > > > > > > > ... > > > > > > > > > +struct gpio_v2_line_config { > > > > > + __aligned_u64 flags; > > > > > + __u32 num_attrs; > > > > > > > > > + /* Pad to fill implicit padding and reserve space for future use. */ > > > > > + __u32 padding[5]; > > > > > > > > Probably I somehow missed the answer, but why do we need 5 here and not 1? > > > > > > > > > > Sorry, I got tired of repeating myself, and just acked that we disagree > > > on the approach here. > > > > > > Your suggestion to use the size for version would result in an > > > explosion of ioctl signatures - every time we add a field we have to add > > > a new ioctl and handle it separately in gpio_ioctl() or linereq_ioctl(). > > > > No, you just add > > __u32 version; // implies sizeof() check as well. > > > > Look for examples of existing ABIs (e.g. perf ABI). > > Please don't ever add a version field to an ioctl structure, this > has been shown to cause more problems than it solves many > times in the past... > Having some reserved fields can be helpful, as long as the kernel > returns an error in case any of the unknown fields are nonzero. > > I'd also prefer fewer than five reserved fields, but that is not as > important, just use as many as are likely to be used in the future, > but not more: It's easy to add one more ioctl command in case > the expectations are wrong, but I agree you wouldn't want a > new command every time another field gets added if the past > has shown that this happens a lot. Thanks for caution and explanation. Let's go then with this. Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> -- With Best Regards, Andy Shevchenko
On Mon, Sep 28, 2020 at 3:42 PM Kent Gibson <warthog618@gmail.com> wrote: > > On Tue, Sep 22, 2020 at 11:50:51AM +0200, Bartosz Golaszewski wrote: > > On Tue, Sep 22, 2020 at 9:41 AM Arnd Bergmann <arnd@arndb.de> wrote: > > > > > > On Tue, Sep 22, 2020 at 4:34 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > +/** > > > > > > Having different-sized members in the union makes it hard for > > > something like strace to print the contents. How about just making > > > them all __aligned_u64 even when 32 bits are sufficient? > > > > > > > Ah yes, adding support for GPIO ioctl()'s to strace has been on my > > TODO list for 3 years now. :( > > > > I'm looking at doing this as it would be useful to have. > > Just starting with the v1 uAPI, as v2 isn't merged yet. > > I think I've got it basically done, but I thought I'd run it past you > before sending a patch to the strace list. > > Successful calls currently look like this: > > ioctl(3, GPIO_GET_CHIPINFO_IOCTL, {name="gpiochip1", label="gpio-mockup-B", lines=8}) = 0 > > ioctl(3, GPIO_GET_LINEHANDLE_IOCTL, {lines=8, lineoffsets=[3, 2, 1, 0, 4, 5, 6, 7], flags=GPIOHANDLE_REQUEST_INPUT, default_values=[0, 0, 0, 0, 0, 0, 0, 0], consumer_label="", fd=7}) = 0 > > ioctl(0, GPIO_GET_LINEEVENT_IOCTL, {lineoffset=2, handleflags=GPIOHANDLE_REQUEST_INPUT, eventflags=GPIOEVENT_REQUEST_BOTH_EDGES, consumer_label="high to low", fd=3}) = 0 > > ioctl(3, GPIO_GET_LINEINFO_IOCTL, {line_offset=1, flags=GPIOLINE_FLAG_KERNEL|GPIOLINE_FLAG_IS_OUT, name="gpio-mockup-A-1", consumer="output atv-lo to as-is atv-hi"}) = 0 > > ioctl(7, GPIOHANDLE_GET_LINE_VALUES_IOCTL, {values=[1, 1, 0, 1, 1, 1, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, ...]}) = 0 > > ioctl(7, GPIOHANDLE_SET_LINE_VALUES_IOCTL, {values=[1, 1, 0, 1, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, ...]}) = 0 > > ioctl(7, GPIOHANDLE_SET_CONFIG_IOCTL, {flags=GPIOHANDLE_REQUEST_ACTIVE_LOW, default_values=[0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, ...]}) = 0 > > ioctl(3, GPIO_GET_LINEINFO_WATCH_IOCTL, {line_offset=3, flags=0, name="gpio-mockup-A-3", consumer=""}) = 0 > > ioctl(3, GPIO_GET_LINEINFO_UNWATCH_IOCTL, {offset=3}) = 0 > > For unsuccessful calls it only prints the input fields, and drops the > output fields. e.g. > > ioctl(3, GPIO_GET_LINEINFO_WATCH_IOCTL, {line_offset=3}) = -1 EPERM (Operation not permitted) > > Does that work for you? > Kent, This is perfect! Thanks for doing this! For the error cases: I'm not sure how strace handles this for other ioctls() - maybe we could print something like 'flags=N/A'? Unless what you did is the standard way, then leave it. Bartosz
On Tue, Sep 29, 2020 at 03:20:22PM +0200, Bartosz Golaszewski wrote: > On Mon, Sep 28, 2020 at 3:42 PM Kent Gibson <warthog618@gmail.com> wrote: > > > > On Tue, Sep 22, 2020 at 11:50:51AM +0200, Bartosz Golaszewski wrote: > > > On Tue, Sep 22, 2020 at 9:41 AM Arnd Bergmann <arnd@arndb.de> wrote: > > > > > > > > On Tue, Sep 22, 2020 at 4:34 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > > +/** > > > > > > > > Having different-sized members in the union makes it hard for > > > > something like strace to print the contents. How about just making > > > > them all __aligned_u64 even when 32 bits are sufficient? > > > > > > > > > > Ah yes, adding support for GPIO ioctl()'s to strace has been on my > > > TODO list for 3 years now. :( > > > > > > > I'm looking at doing this as it would be useful to have. > > > > Just starting with the v1 uAPI, as v2 isn't merged yet. > > > > I think I've got it basically done, but I thought I'd run it past you > > before sending a patch to the strace list. > > > > Successful calls currently look like this: > > > > ioctl(3, GPIO_GET_CHIPINFO_IOCTL, {name="gpiochip1", label="gpio-mockup-B", lines=8}) = 0 > > > > ioctl(3, GPIO_GET_LINEHANDLE_IOCTL, {lines=8, lineoffsets=[3, 2, 1, 0, 4, 5, 6, 7], flags=GPIOHANDLE_REQUEST_INPUT, default_values=[0, 0, 0, 0, 0, 0, 0, 0], consumer_label="", fd=7}) = 0 > > > > ioctl(0, GPIO_GET_LINEEVENT_IOCTL, {lineoffset=2, handleflags=GPIOHANDLE_REQUEST_INPUT, eventflags=GPIOEVENT_REQUEST_BOTH_EDGES, consumer_label="high to low", fd=3}) = 0 > > > > ioctl(3, GPIO_GET_LINEINFO_IOCTL, {line_offset=1, flags=GPIOLINE_FLAG_KERNEL|GPIOLINE_FLAG_IS_OUT, name="gpio-mockup-A-1", consumer="output atv-lo to as-is atv-hi"}) = 0 > > > > ioctl(7, GPIOHANDLE_GET_LINE_VALUES_IOCTL, {values=[1, 1, 0, 1, 1, 1, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, ...]}) = 0 > > > > ioctl(7, GPIOHANDLE_SET_LINE_VALUES_IOCTL, {values=[1, 1, 0, 1, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, ...]}) = 0 > > > > ioctl(7, GPIOHANDLE_SET_CONFIG_IOCTL, {flags=GPIOHANDLE_REQUEST_ACTIVE_LOW, default_values=[0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, ...]}) = 0 > > > > ioctl(3, GPIO_GET_LINEINFO_WATCH_IOCTL, {line_offset=3, flags=0, name="gpio-mockup-A-3", consumer=""}) = 0 > > > > ioctl(3, GPIO_GET_LINEINFO_UNWATCH_IOCTL, {offset=3}) = 0 > > > > For unsuccessful calls it only prints the input fields, and drops the > > output fields. e.g. > > > > ioctl(3, GPIO_GET_LINEINFO_WATCH_IOCTL, {line_offset=3}) = -1 EPERM (Operation not permitted) > > > > Does that work for you? > > > > Kent, > > This is perfect! Thanks for doing this! For the error cases: I'm not > sure how strace handles this for other ioctls() - maybe we could print > something like 'flags=N/A'? Unless what you did is the standard way, > then leave it. > I think that is pretty standard, but I will double check. Just debugging the uAPI v2 decoding now, though I wont submit that until the uAPI is in an -rc. It may be useful for debugging in the meantime, so I'll push a copy to my github fork[1] shortly - the v1 decoding is there already. I've noticed a few things I want to tidy up in the v1 decoding before I submit that though. Should have it ready to submit in the next few days. Cheers, Kent. [1] https://github.com/warthog618/strace/tree/gpio
On Tue, Sep 29, 2020 at 4:21 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > On Mon, Sep 28, 2020 at 3:42 PM Kent Gibson <warthog618@gmail.com> wrote: > > On Tue, Sep 22, 2020 at 11:50:51AM +0200, Bartosz Golaszewski wrote: ... > > I'm looking at doing this as it would be useful to have. > > Does that work for you? > This is perfect! Thanks for doing this! For the error cases: I'm not > sure how strace handles this for other ioctls() - maybe we could print > something like 'flags=N/A'? Unless what you did is the standard way, > then leave it. +1 here. Thanks for doing this, it's helpful! -- With Best Regards, Andy Shevchenko
diff --git a/include/uapi/linux/gpio.h b/include/uapi/linux/gpio.h index 285cc10355b2..5904f49399de 100644 --- a/include/uapi/linux/gpio.h +++ b/include/uapi/linux/gpio.h @@ -11,11 +11,14 @@ #ifndef _UAPI_GPIO_H_ #define _UAPI_GPIO_H_ +#include <linux/const.h> #include <linux/ioctl.h> #include <linux/types.h> /* * The maximum size of name and label arrays. + * + * Must be a multiple of 8 to ensure 32/64-bit alignment of structs. */ #define GPIO_MAX_NAME_SIZE 32 @@ -32,6 +35,265 @@ struct gpiochip_info { __u32 lines; }; +/* + * Maximum number of requested lines. + * + * Must be no greater than 64, as bitmaps are restricted here to 64-bits + * for simplicity, and a multiple of 2 to ensure 32/64-bit alignment of + * structs. + */ +#define GPIO_V2_LINES_MAX 64 + +/* + * The maximum number of configuration attributes associated with a line + * request. + */ +#define GPIO_V2_LINE_NUM_ATTRS_MAX 10 + +/** + * enum gpio_v2_line_flag - &struct gpio_v2_line_attribute.flags values + * @GPIO_V2_LINE_FLAG_USED: line is not available for request + * @GPIO_V2_LINE_FLAG_ACTIVE_LOW: line active state is physical low + * @GPIO_V2_LINE_FLAG_INPUT: line is an input + * @GPIO_V2_LINE_FLAG_OUTPUT: line is an output + * @GPIO_V2_LINE_FLAG_EDGE_RISING: line detects rising (inactive to active) + * edges + * @GPIO_V2_LINE_FLAG_EDGE_FALLING: line detects falling (active to + * inactive) edges + * @GPIO_V2_LINE_FLAG_OPEN_DRAIN: line is an open drain output + * @GPIO_V2_LINE_FLAG_OPEN_SOURCE: line is an open source output + * @GPIO_V2_LINE_FLAG_BIAS_PULL_UP: line has pull-up bias enabled + * @GPIO_V2_LINE_FLAG_BIAS_PULL_DOWN: line has pull-down bias enabled + * @GPIO_V2_LINE_FLAG_BIAS_DISABLED: line has bias disabled + */ +enum gpio_v2_line_flag { + GPIO_V2_LINE_FLAG_USED = _BITULL(0), + GPIO_V2_LINE_FLAG_ACTIVE_LOW = _BITULL(1), + GPIO_V2_LINE_FLAG_INPUT = _BITULL(2), + GPIO_V2_LINE_FLAG_OUTPUT = _BITULL(3), + GPIO_V2_LINE_FLAG_EDGE_RISING = _BITULL(4), + GPIO_V2_LINE_FLAG_EDGE_FALLING = _BITULL(5), + GPIO_V2_LINE_FLAG_OPEN_DRAIN = _BITULL(6), + GPIO_V2_LINE_FLAG_OPEN_SOURCE = _BITULL(7), + GPIO_V2_LINE_FLAG_BIAS_PULL_UP = _BITULL(8), + GPIO_V2_LINE_FLAG_BIAS_PULL_DOWN = _BITULL(9), + GPIO_V2_LINE_FLAG_BIAS_DISABLED = _BITULL(10), +}; + +/** + * struct gpio_v2_line_values - Values of GPIO lines + * @bits: a bitmap containing the value of the lines, set to 1 for active + * and 0 for inactive. + * @mask: a bitmap identifying the lines to get or set, with each bit + * number corresponding to the index into &struct + * gpio_v2_line_request.offsets. + */ +struct gpio_v2_line_values { + __aligned_u64 bits; + __aligned_u64 mask; +}; + +/** + * enum gpio_v2_line_attr_id - &struct gpio_v2_line_attribute.id values + * identifying which field of the attribute union is in use. + * @GPIO_V2_LINE_ATTR_ID_FLAGS: flags field is in use + * @GPIO_V2_LINE_ATTR_ID_OUTPUT_VALUES: values field is in use + * @GPIO_V2_LINE_ATTR_ID_DEBOUNCE: debounce_period_us is in use + */ +enum gpio_v2_line_attr_id { + GPIO_V2_LINE_ATTR_ID_FLAGS = 1, + GPIO_V2_LINE_ATTR_ID_OUTPUT_VALUES = 2, + GPIO_V2_LINE_ATTR_ID_DEBOUNCE = 3, +}; + +/** + * struct gpio_v2_line_attribute - a configurable attribute of a line + * @id: attribute identifier with value from &enum gpio_v2_line_attr_id + * @padding: reserved for future use and must be zero filled + * @flags: if id is GPIO_V2_LINE_ATTR_ID_FLAGS, the flags for the GPIO + * line, with values from enum gpio_v2_line_flag, such as + * GPIO_V2_LINE_FLAG_ACTIVE_LOW, GPIO_V2_LINE_FLAG_OUTPUT etc, OR:ed + * together. This overrides the default flags contained in the &struct + * gpio_v2_line_config for the associated line. + * @values: if id is GPIO_V2_LINE_ATTR_ID_OUTPUT_VALUES, a bitmap + * containing the values to which the lines will be set, with each bit + * number corresponding to the index into &struct + * gpio_v2_line_request.offsets. + * @debounce_period_us: if id is GPIO_V2_LINE_ATTR_ID_DEBOUNCE, the desired + * debounce period, in microseconds + */ +struct gpio_v2_line_attribute { + __u32 id; + __u32 padding; + union { + __aligned_u64 flags; + __aligned_u64 values; + __u32 debounce_period_us; + }; +}; + +/** + * struct gpio_v2_line_config_attribute - a configuration attribute + * associated with one or more of the requested lines. + * @attr: the configurable attribute + * @mask: a bitmap identifying the lines to which the attribute applies, + * with each bit number corresponding to the index into &struct + * gpio_v2_line_request.offsets. + */ +struct gpio_v2_line_config_attribute { + struct gpio_v2_line_attribute attr; + __aligned_u64 mask; +}; + +/** + * struct gpio_v2_line_config - Configuration for GPIO lines + * @flags: flags for the GPIO lines, with values from enum + * gpio_v2_line_flag, such as GPIO_V2_LINE_FLAG_ACTIVE_LOW, + * GPIO_V2_LINE_FLAG_OUTPUT etc, OR:ed together. This is the default for + * all requested lines but may be overridden for particular lines using + * attrs. + * @num_attrs: the number of attributes in attrs + * @padding: reserved for future use and must be zero filled + * @attrs: the configuration attributes associated with the requested + * lines. Any attribute should only be associated with a particular line + * once. If an attribute is associated with a line multiple times then the + * first occurrence (i.e. lowest index) has precedence. + */ +struct gpio_v2_line_config { + __aligned_u64 flags; + __u32 num_attrs; + /* Pad to fill implicit padding and reserve space for future use. */ + __u32 padding[5]; + struct gpio_v2_line_config_attribute attrs[GPIO_V2_LINE_NUM_ATTRS_MAX]; +}; + +/** + * struct gpio_v2_line_request - Information about a request for GPIO lines + * @offsets: an array of desired lines, specified by offset index for the + * associated GPIO chip + * @consumer: a desired consumer label for the selected GPIO lines such as + * "my-bitbanged-relay" + * @config: requested configuration for the lines. + * @num_lines: number of lines requested in this request, i.e. the number + * of valid fields in the GPIO_V2_LINES_MAX sized arrays, set to 1 to + * request a single line + * @event_buffer_size: a suggested minimum number of line events that the + * kernel should buffer. This is only relevant if edge detection is + * enabled in the configuration. Note that this is only a suggested value + * and the kernel may allocate a larger buffer or cap the size of the + * buffer. If this field is zero then the buffer size defaults to a minimum + * of num_lines*16. + * @padding: reserved for future use and must be zero filled + * @fd: if successful this field will contain a valid anonymous file handle + * after a GPIO_GET_LINE_IOCTL operation, zero or negative value means + * error + */ +struct gpio_v2_line_request { + __u32 offsets[GPIO_V2_LINES_MAX]; + char consumer[GPIO_MAX_NAME_SIZE]; + struct gpio_v2_line_config config; + __u32 num_lines; + __u32 event_buffer_size; + /* Pad to fill implicit padding and reserve space for future use. */ + __u32 padding[5]; + __s32 fd; +}; + +/** + * struct gpio_v2_line_info - Information about a certain GPIO line + * @name: the name of this GPIO line, such as the output pin of the line on + * the chip, a rail or a pin header name on a board, as specified by the + * GPIO chip, may be empty + * @consumer: a functional name for the consumer of this GPIO line as set + * by whatever is using it, will be empty if there is no current user but + * may also be empty if the consumer doesn't set this up + * @flags: flags for the GPIO line, such as GPIO_V2_LINE_FLAG_ACTIVE_LOW, + * GPIO_V2_LINE_FLAG_OUTPUT etc, OR:ed together + * @offset: the local offset on this GPIO chip, fill this in when + * requesting the line information from the kernel + * @num_attrs: the number of attributes in attrs + * @attrs: the configuration attributes associated with the line + * @padding: reserved for future use + */ +struct gpio_v2_line_info { + char name[GPIO_MAX_NAME_SIZE]; + char consumer[GPIO_MAX_NAME_SIZE]; + __u32 offset; + __u32 num_attrs; + __aligned_u64 flags; + struct gpio_v2_line_attribute attrs[GPIO_V2_LINE_NUM_ATTRS_MAX]; + /* Space reserved for future use. */ + __u32 padding[4]; +}; + +/** + * enum gpio_v2_line_changed_type - &struct gpio_v2_line_changed.event_type + * values + * @GPIO_V2_LINE_CHANGED_REQUESTED: line has been requested + * @GPIO_V2_LINE_CHANGED_RELEASED: line has been released + * @GPIO_V2_LINE_CHANGED_CONFIG: line has been reconfigured + */ +enum gpio_v2_line_changed_type { + GPIO_V2_LINE_CHANGED_REQUESTED = 1, + GPIO_V2_LINE_CHANGED_RELEASED = 2, + GPIO_V2_LINE_CHANGED_CONFIG = 3, +}; + +/** + * struct gpio_v2_line_info_changed - Information about a change in status + * of a GPIO line + * @info: updated line information + * @timestamp_ns: estimate of time of status change occurrence, in nanoseconds + * @event_type: the type of change with a value from enum + * gpio_v2_line_changed_type + * @padding: reserved for future use + */ +struct gpio_v2_line_info_changed { + struct gpio_v2_line_info info; + __aligned_u64 timestamp_ns; + __u32 event_type; + /* Pad struct to 64-bit boundary and reserve space for future use. */ + __u32 padding[5]; +}; + +/** + * enum gpio_v2_line_event_id - &struct gpio_v2_line_event.id values + * @GPIO_V2_LINE_EVENT_RISING_EDGE: event triggered by a rising edge + * @GPIO_V2_LINE_EVENT_FALLING_EDGE: event triggered by a falling edge + */ +enum gpio_v2_line_event_id { + GPIO_V2_LINE_EVENT_RISING_EDGE = 1, + GPIO_V2_LINE_EVENT_FALLING_EDGE = 2, +}; + +/** + * struct gpio_v2_line_event - The actual event being pushed to userspace + * @timestamp_ns: best estimate of time of event occurrence, in nanoseconds. + * The timestamp_ns is read from CLOCK_MONOTONIC and is intended to allow the + * accurate measurement of the time between events. It does not provide + * the wall-clock time. + * @id: event identifier with value from enum gpio_v2_line_event_id + * @offset: the offset of the line that triggered the event + * @seqno: the sequence number for this event in the sequence of events for + * all the lines in this line request + * @line_seqno: the sequence number for this event in the sequence of + * events on this particular line + * @padding: reserved for future use + */ +struct gpio_v2_line_event { + __aligned_u64 timestamp_ns; + __u32 id; + __u32 offset; + __u32 seqno; + __u32 line_seqno; + /* Space reserved for future use. */ + __u32 padding[6]; +}; + +/* + * ABI v1 + */ + /* Informational flags */ #define GPIOLINE_FLAG_KERNEL (1UL << 0) /* Line used by the kernel */ #define GPIOLINE_FLAG_IS_OUT (1UL << 1) @@ -149,8 +411,6 @@ struct gpiohandle_config { __u32 padding[4]; /* padding for future use */ }; -#define GPIOHANDLE_SET_CONFIG_IOCTL _IOWR(0xB4, 0x0a, struct gpiohandle_config) - /** * struct gpiohandle_data - Information of values on a GPIO handle * @values: when getting the state of lines this contains the current @@ -161,9 +421,6 @@ struct gpiohandle_data { __u8 values[GPIOHANDLES_MAX]; }; -#define GPIOHANDLE_GET_LINE_VALUES_IOCTL _IOWR(0xB4, 0x08, struct gpiohandle_data) -#define GPIOHANDLE_SET_LINE_VALUES_IOCTL _IOWR(0xB4, 0x09, struct gpiohandle_data) - /* Eventrequest flags */ #define GPIOEVENT_REQUEST_RISING_EDGE (1UL << 0) #define GPIOEVENT_REQUEST_FALLING_EDGE (1UL << 1) @@ -207,11 +464,31 @@ struct gpioevent_data { __u32 id; }; +/* + * v1 and v2 ioctl()s + */ #define GPIO_GET_CHIPINFO_IOCTL _IOR(0xB4, 0x01, struct gpiochip_info) +#define GPIO_GET_LINEINFO_UNWATCH_IOCTL _IOWR(0xB4, 0x0C, __u32) + +/* + * v2 ioctl()s + */ +#define GPIO_V2_GET_LINEINFO_IOCTL _IOWR(0xB4, 0x05, struct gpio_v2_line_info) +#define GPIO_V2_GET_LINEINFO_WATCH_IOCTL _IOWR(0xB4, 0x06, struct gpio_v2_line_info) +#define GPIO_V2_GET_LINE_IOCTL _IOWR(0xB4, 0x07, struct gpio_v2_line_request) +#define GPIO_V2_LINE_SET_CONFIG_IOCTL _IOWR(0xB4, 0x0D, struct gpio_v2_line_config) +#define GPIO_V2_LINE_GET_VALUES_IOCTL _IOWR(0xB4, 0x0E, struct gpio_v2_line_values) +#define GPIO_V2_LINE_SET_VALUES_IOCTL _IOWR(0xB4, 0x0F, struct gpio_v2_line_values) + +/* + * v1 ioctl()s + */ #define GPIO_GET_LINEINFO_IOCTL _IOWR(0xB4, 0x02, struct gpioline_info) -#define GPIO_GET_LINEINFO_WATCH_IOCTL _IOWR(0xB4, 0x0b, struct gpioline_info) -#define GPIO_GET_LINEINFO_UNWATCH_IOCTL _IOWR(0xB4, 0x0c, __u32) #define GPIO_GET_LINEHANDLE_IOCTL _IOWR(0xB4, 0x03, struct gpiohandle_request) #define GPIO_GET_LINEEVENT_IOCTL _IOWR(0xB4, 0x04, struct gpioevent_request) +#define GPIOHANDLE_GET_LINE_VALUES_IOCTL _IOWR(0xB4, 0x08, struct gpiohandle_data) +#define GPIOHANDLE_SET_LINE_VALUES_IOCTL _IOWR(0xB4, 0x09, struct gpiohandle_data) +#define GPIOHANDLE_SET_CONFIG_IOCTL _IOWR(0xB4, 0x0A, struct gpiohandle_config) +#define GPIO_GET_LINEINFO_WATCH_IOCTL _IOWR(0xB4, 0x0B, struct gpioline_info) #endif /* _UAPI_GPIO_H_ */
Add a new version of the uAPI to address existing 32/64-bit alignment issues, add support for debounce and event sequence numbers, allow requested lines with different configurations, and provide some future proofing by adding padding reserved for future use. The alignment issue relates to the gpioevent_data, which packs to different sizes on 32-bit and 64-bit platforms. That creates problems for 32-bit apps running on 64-bit kernels. uAPI v2 addresses that particular issue, and the problem more generally, by adding pad fields that explicitly pad structs out to 64-bit boundaries, so they will pack to the same size now, and even if some of the reserved padding is used for __u64 fields in the future. The new structs have been analysed with pahole to ensure that they are sized as expected and contain no implicit padding. The lack of future proofing in v1 makes it impossible to, for example, add the debounce feature that is included in v2. The future proofing is addressed by providing configurable attributes in line config and reserved padding in all structs for future features. Specifically, the line request, config, info, info_changed and event structs receive updated versions and new ioctls. As the majority of the structs and ioctls were being replaced, it is opportune to rework some of the other aspects of the uAPI: v1 has three different flags fields, each with their own separate bit definitions. In v2 that is collapsed to one - gpio_v2_line_flag. The handle and event requests are merged into a single request, the line request, as the two requests were mostly the same other than the edge detection provided by event requests. As a byproduct, the v2 uAPI allows for multiple lines producing edge events on the same line handle. This is a new capability as v1 only supports a single line in an event request. As a consequence, there are now only two types of file handle to be concerned with, the chip and the line, and it is clearer which ioctls apply to which type of handle. There is also some minor renaming of fields for consistency compared to their v1 counterparts, e.g. offset rather than lineoffset or line_offset, and consumer rather than consumer_label. Additionally, v1 GPIOHANDLES_MAX becomes GPIO_V2_LINES_MAX in v2 for clarity, and the gpiohandle_data __u8 array becomes a bitmap in gpio_v2_line_values. The v2 uAPI is mostly a reorganisation and extension of v1, so userspace code, particularly libgpiod, should readily port to it. Signed-off-by: Kent Gibson <warthog618@gmail.com> --- Changes for v9: - use time scale suffixes on timestamp (_ns) and debounce period (_us) variables - document all enum values - comment wording tweaks - change some field orders to improve readability Changes for v7: - use _BITULL for flag constants Changes for v4: - clarify bitmap width in GPIO_V2_LINES_MAX description Changes for v3: - relocated commentary into commit description - hard limit max requested lines to 64 so bitmaps always fit in a single u64. - prefix all v2 symbols with GPIO_V2 - 64-bit flag values to ULL - use __aligned_u64 to ensure 64-bit fields are 64-bit aligned - support masked get values, as per set values. Changes for v2: - lower case V1 and V2, except in capitalized names - hyphenate 32/64-bit - rename bitmap field to bits - drop PAD_SIZE consts in favour of hard coded numbers - sort includes - change config flags to __u64 - increase padding of gpioline_event - relocate GPIOLINE_CHANGED enum into v2 section (is common with v1) - rework config to collapse direction, drive, bias and edge enums back into flags and add optional attributes that can be associated with a subset of the requested lines. Changes for v1 (since the RFC): - document the constraints on array sizes to maintain 32/64 alignment - add sequence numbers to gpioline_event - use bitmap for values instead of array of __u8 - gpioline_info_v2 contains gpioline_config instead of its composite fields - provide constants for all array sizes, especially padding - renamed "GPIOLINE_FLAG_V2_KERNEL" to "GPIOLINE_FLAG_V2_USED" - renamed "default_values" to "values" - made gpioline_direction zero based - document clock used in gpioline_event timestamp - add event_buffer_size to gpioline_request - rename debounce to debounce_period - rename lines to num_lines include/uapi/linux/gpio.h | 291 +++++++++++++++++++++++++++++++++++++- 1 file changed, 284 insertions(+), 7 deletions(-)