diff mbox series

[RFC] gpio: uapi: v2 proposal

Message ID 20200516064507.19058-1-warthog618@gmail.com
State New
Headers show
Series [RFC] gpio: uapi: v2 proposal | expand

Commit Message

Kent Gibson May 16, 2020, 6:45 a.m. UTC
Add a new version of the uAPI to address existing 32/64bit alignment
issues, add support for debounce, and provide some future proofing by
adding padding reserved for future use.

Signed-off-by: Kent Gibson <warthog618@gmail.com>

---

This patch is a proposal to replace the majority of the uAPI, so some 
background and justification is in order.

The alignment issue relates to the gpioevent_data, which packs to different
sizes on 32bit and 64bit platforms. That creates problems for 32bit apps
running on 64bit kernels.  The patch addresses that particular issue, and
the problem more generally, by adding pad fields that explicitly pad
structs out to 64bit 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 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 reserved padding in all
structs for future features.  Specifically, the line request, 
config and info structs get updated versions and ioctls.

I haven't added any padding to gpiochip_info, as I haven't seen any calls
for new features for the corresponding ioctl, but I'm open to updating that
as well.

As the majority of the structs and ioctls were being replaced, it seemed
opportune to rework some of the other aspects of the uAPI.

Firstly, I've reworked the flags field throughout.  v1 has three different
flags fields, each with their own separate bit definitions.  In v2 that is
collapsed to one.  Further, the bits of the v2 flags field are used
as feature enable flags, with any other necessary configuration fields encoded
separately.  This is simpler and clearer, while also providing a foundation
for adding features in the future.

I've also merged the handle and event requests into a single request, the
line request, as the two requests where 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.

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

And v1 GPIOHANDLES_MAX and gpiohandle_data become GPIOLINES_MAX and 
gpioline_values for v2 - the only change being the renaming for clarity.

The v2 uAPI is mostly just a reorganisation of v1, so userspace code,
particularly libgpiod, should easily port to it.

This patch is obviously only one patch in a much bigger series that 
will actually implement it, but I would appreciate a review and any feedback,
as it is foundational to the rest of that series.

Thanks,
Kent.

 include/uapi/linux/gpio.h | 204 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 197 insertions(+), 7 deletions(-)

Comments

Linus Walleij May 25, 2020, 8:39 a.m. UTC | #1
On Sat, May 16, 2020 at 8:45 AM Kent Gibson <warthog618@gmail.com> wrote:

> Add a new version of the uAPI to address existing 32/64bit alignment
> issues, add support for debounce, and provide some future proofing by
> adding padding reserved for future use.
>
> Signed-off-by: Kent Gibson <warthog618@gmail.com>

I don't see any major problems with it, just some comments:

+ * @padding: reserved for future use and should be zero filled

*MUST* be zerofilled is what it should say.

> +struct gpioline_config {
> +       __u8 default_values[GPIOLINES_MAX];

So 32 bytes

> +       __u32 flags;
> +       __u8 direction;
> +       __u8 drive;
> +       __u8 bias;
> +       __u8 edge_detection;

Some comment in the struc that this adds up to 32 bits?

> +       __u32 debounce;
> +       __u32 padding[7]; /* for future use */

What we need to do inside the kernel with all of these
is to ascertain that they are 0 for now when they are
sent in and refuse them otherwise, I think it was Lars-Peter
Clausen who said that they had to retire a whole slew of
ioctl()s because some userspace just used unallocated
memory for this and since that was random bytes it needed to
be supported with that content forever and the reserved
padding could never be used for the reserved purpose.

This kind of comment goes for all the structs.

But mostly it is a question about the kernel code receiving
or emitting these.

Yours,
Linus Walleij
Kent Gibson May 25, 2020, 2:19 p.m. UTC | #2
On Mon, May 25, 2020 at 10:39:42AM +0200, Linus Walleij wrote:
> On Sat, May 16, 2020 at 8:45 AM Kent Gibson <warthog618@gmail.com> wrote:
> 
> > Add a new version of the uAPI to address existing 32/64bit alignment
> > issues, add support for debounce, and provide some future proofing by
> > adding padding reserved for future use.
> >
> > Signed-off-by: Kent Gibson <warthog618@gmail.com>
> 
> I don't see any major problems with it, just some comments:
> 
> + * @padding: reserved for future use and should be zero filled
> 
> *MUST* be zerofilled is what it should say.
> 
> > +struct gpioline_config {
> > +       __u8 default_values[GPIOLINES_MAX];
> 
> So 32 bytes
> 

Actually that one is 64 bytes, which is the same as v1, i.e. GPIOLINES_MAX
is the same as GPIOHANDLES_MAX - just renamed.

On the subject of values, is there any reason to use a byte for each line
rather value than a bit?

> > +       __u32 flags;
> > +       __u8 direction;
> > +       __u8 drive;
> > +       __u8 bias;
> > +       __u8 edge_detection;
> 
> Some comment in the struc that this adds up to 32 bits?
> 
> > +       __u32 debounce;
> > +       __u32 padding[7]; /* for future use */
> 
> What we need to do inside the kernel with all of these
> is to ascertain that they are 0 for now when they are
> sent in and refuse them otherwise, I think it was Lars-Peter
> Clausen who said that they had to retire a whole slew of
> ioctl()s because some userspace just used unallocated
> memory for this and since that was random bytes it needed to
> be supported with that content forever and the reserved
> padding could never be used for the reserved purpose.
> 
> This kind of comment goes for all the structs.
> 

OK, I wasn't sure how strict the validation should be on the kernel
side, but I'll take a strict stance and check the whole struct.

Having said that, when adding future fields, the idea was to have a bit
in the flags that indicates that the corresponding field is now valid.
If the flag is not set then whatever value is there is irrelevant.
e.g. the debounce field value is irrelevent and ignored unless the
GPIOLINE_FLAG_V2_DEBOUNCE is set in flags.
OTOH, if the bit is set then the field would have to be set correctly.
And the current kernel would reject a future request with an unsupported
bit set in flags.

But definitely better to play it safe - will check the padding is
zeroed as well, as well as any field for which the flag bit is clear.

> But mostly it is a question about the kernel code receiving
> or emitting these.
> 

For sure - all the structs returned will be zeroed before use so as not
to leak kernel stack - unless they originate from userspace of course.

Back on retired ioctls, I notice that 5, 6, and 7 are missing from gpio.
Have those been retired, or just skipped over by accident?

Cheers,
Kent.
Bartosz Golaszewski May 25, 2020, 4:24 p.m. UTC | #3
sob., 16 maj 2020 o 08:45 Kent Gibson <warthog618@gmail.com> napisał(a):
>
> Add a new version of the uAPI to address existing 32/64bit alignment
> issues, add support for debounce, and provide some future proofing by
> adding padding reserved for future use.
>
> Signed-off-by: Kent Gibson <warthog618@gmail.com>
>
> ---
>
> This patch is a proposal to replace the majority of the uAPI, so some
> background and justification is in order.
>
> The alignment issue relates to the gpioevent_data, which packs to different
> sizes on 32bit and 64bit platforms. That creates problems for 32bit apps
> running on 64bit kernels.  The patch addresses that particular issue, and
> the problem more generally, by adding pad fields that explicitly pad
> structs out to 64bit 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 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 reserved padding in all
> structs for future features.  Specifically, the line request,
> config and info structs get updated versions and ioctls.
>
> I haven't added any padding to gpiochip_info, as I haven't seen any calls
> for new features for the corresponding ioctl, but I'm open to updating that
> as well.
>
> As the majority of the structs and ioctls were being replaced, it seemed
> opportune to rework some of the other aspects of the uAPI.
>
> Firstly, I've reworked the flags field throughout.  v1 has three different
> flags fields, each with their own separate bit definitions.  In v2 that is
> collapsed to one.  Further, the bits of the v2 flags field are used
> as feature enable flags, with any other necessary configuration fields encoded
> separately.  This is simpler and clearer, while also providing a foundation
> for adding features in the future.
>
> I've also merged the handle and event requests into a single request, the
> line request, as the two requests where 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.
>
> This means 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.
>
> And v1 GPIOHANDLES_MAX and gpiohandle_data become GPIOLINES_MAX and
> gpioline_values for v2 - the only change being the renaming for clarity.
>
> The v2 uAPI is mostly just a reorganisation of v1, so userspace code,
> particularly libgpiod, should easily port to it.
>
> This patch is obviously only one patch in a much bigger series that
> will actually implement it, but I would appreciate a review and any feedback,
> as it is foundational to the rest of that series.
>
> Thanks,
> Kent.
>

Hi Kent,

Thanks for posting this. I like the general direction a lot. I'll
review this in detail later this week.

Seeing the speed at which you make progress I think I won't be
implementing support for the v1 of the watch ioctl() in libgpiod after
all. Once the v2 is live I will probably bump the API version in
libgpiod to v2.0.0 and make some non-compatible changes anyway.

Bart
Andy Shevchenko May 26, 2020, 8:04 a.m. UTC | #4
+Cc: Ville

Ville, this is a v2 of the GPIO ABI we discussed with some time ago.
If you have time to briefly look at it and perhaps comment if it's
right way to go.

On Sat, May 16, 2020 at 9:50 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> Add a new version of the uAPI to address existing 32/64bit alignment
> issues, add support for debounce, and provide some future proofing by
> adding padding reserved for future use.
>
> Signed-off-by: Kent Gibson <warthog618@gmail.com>
>
> ---
>
> This patch is a proposal to replace the majority of the uAPI, so some
> background and justification is in order.
>
> The alignment issue relates to the gpioevent_data, which packs to different
> sizes on 32bit and 64bit platforms. That creates problems for 32bit apps
> running on 64bit kernels.  The patch addresses that particular issue, and
> the problem more generally, by adding pad fields that explicitly pad
> structs out to 64bit 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 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 reserved padding in all
> structs for future features.  Specifically, the line request,
> config and info structs get updated versions and ioctls.
>
> I haven't added any padding to gpiochip_info, as I haven't seen any calls
> for new features for the corresponding ioctl, but I'm open to updating that
> as well.
>
> As the majority of the structs and ioctls were being replaced, it seemed
> opportune to rework some of the other aspects of the uAPI.
>
> Firstly, I've reworked the flags field throughout.  v1 has three different
> flags fields, each with their own separate bit definitions.  In v2 that is
> collapsed to one.  Further, the bits of the v2 flags field are used
> as feature enable flags, with any other necessary configuration fields encoded
> separately.  This is simpler and clearer, while also providing a foundation
> for adding features in the future.
>
> I've also merged the handle and event requests into a single request, the
> line request, as the two requests where 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.
>
> This means 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.

> And v1 GPIOHANDLES_MAX and gpiohandle_data become GPIOLINES_MAX and
> gpioline_values for v2 - the only change being the renaming for clarity.

Hmm... I think it makes sense if you fully replace uAPI, otherwise in
my opinion it adds to the confusion.
My point is that we may try to be less invasive, perhaps?

> The v2 uAPI is mostly just a reorganisation of v1, so userspace code,
> particularly libgpiod, should easily port to it.
>
> This patch is obviously only one patch in a much bigger series that
> will actually implement it, but I would appreciate a review and any feedback,
> as it is foundational to the rest of that series.
>
> Thanks,
> Kent.
>
>  include/uapi/linux/gpio.h | 204 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 197 insertions(+), 7 deletions(-)
>
> diff --git a/include/uapi/linux/gpio.h b/include/uapi/linux/gpio.h
> index 0206383c0383..3db7e0bc1312 100644
> --- a/include/uapi/linux/gpio.h
> +++ b/include/uapi/linux/gpio.h
> @@ -14,6 +14,9 @@
>  #include <linux/ioctl.h>
>  #include <linux/types.h>
>
> +/* The maximum size of name and label arrays */
> +#define GPIO_MAX_NAME_SIZE 32
> +
>  /**
>   * struct gpiochip_info - Information about a certain GPIO chip
>   * @name: the Linux kernel name of this GPIO chip
> @@ -27,6 +30,184 @@ struct gpiochip_info {
>         __u32 lines;
>  };
>
> +/* Maximum number of requested lines */
> +#define GPIOLINES_MAX 64
> +
> +enum gpioline_direction {
> +       GPIOLINE_DIRECTION_INPUT        = 1,
> +       GPIOLINE_DIRECTION_OUTPUT       = 2,
> +};
> +
> +enum gpioline_drive {
> +       GPIOLINE_DRIVE_PUSH_PULL        = 0,
> +       GPIOLINE_DRIVE_OPEN_DRAIN       = 1,
> +       GPIOLINE_DRIVE_OPEN_SOURCE      = 2,
> +};
> +
> +enum gpioline_bias {
> +       GPIOLINE_BIAS_DISABLE           = 0,
> +       GPIOLINE_BIAS_PULL_UP           = 1,
> +       GPIOLINE_BIAS_PULL_DOWN         = 2,
> +};
> +
> +enum gpioline_edge {
> +       GPIOLINE_EDGE_NONE              = 0,
> +       GPIOLINE_EDGE_RISING            = 1,
> +       GPIOLINE_EDGE_FALLING           = 2,
> +       GPIOLINE_EDGE_BOTH              = 3,
> +};
> +
> +/* Line flags - V2 */
> +#define GPIOLINE_FLAG_V2_KERNEL                (1UL << 0) /* Line used by the kernel */
> +#define GPIOLINE_FLAG_V2_ACTIVE_LOW    (1UL << 1)
> +#define GPIOLINE_FLAG_V2_DIRECTION     (1UL << 2)
> +#define GPIOLINE_FLAG_V2_DRIVE         (1UL << 3)
> +#define GPIOLINE_FLAG_V2_BIAS          (1UL << 4)
> +#define GPIOLINE_FLAG_V2_EDGE_DETECTION        (1UL << 5)
> +#define GPIOLINE_FLAG_V2_DEBOUNCE      (1UL << 6)
> +
> +/**
> + * struct gpioline_config - Configuration for GPIO lines
> + * @default_values: if the direction is GPIOLINE_DIRECTION_OUTPUT, this
> + * specifies the default output value, should be 0 (low) or 1 (high),
> + * anything else than 0 or 1 will be interpreted as 1 (high)
> + * @flags: flags for the GPIO lines, such as GPIOLINE_FLAG_V2_ACTIVE_LOW,
> + * GPIOLINE_FLAG_V2_DIRECTION etc, OR:ed together
> + * @direction: if GPIOLINE_FLAG_V2_DIRECTION is set in flags, the desired
> + * direction for the requested lines, with a value from enum
> + * gpioline_direction
> + * @drive: if GPIOLINE_FLAG_V2_DRIVE is set in flags, the desired drive for
> + * the requested lines, with a value from enum gpioline_drive
> + * @bias: if GPIOLINE_FLAG_V2_BIAS is set in flags, the desired bias for
> + * the requested lines, with a value from enum gpioline_bias
> + * @edge_detection: if GPIOLINE_FLAG_V2_EDGE_DETECTION is set in flags, the
> + * desired edge_detection for the requested lines, with a value from enum
> + * gpioline_edge
> + * @debounce: if GPIOLINE_FLAG_V2_DEBOUNCE is set in flags, the desired
> + * debounce period for the requested lines, in microseconds
> + * @padding: reserved for future use and should be zero filled
> + */
> +struct gpioline_config {
> +       __u8 default_values[GPIOLINES_MAX];

This basically means that whatever in square brackets must be aligned
to 8. So, BUILD_BUG_ON() here and in similar places where it makes
sense.

> +       __u32 flags;
> +       __u8 direction;
> +       __u8 drive;
> +       __u8 bias;
> +       __u8 edge_detection;
> +       __u32 debounce;

> +       __u32 padding[7]; /* for future use */

Why 7 and not 5 or even 1?
Can we use size of struct as a versioning?
Or maybe explicit version field?

Same questions to all big paddings.

> +};
> +
> +/**
> + * struct gpioline_request - Information about a request for GPIO lines
> + * @offsets: an array of desired lines, specified by offset index for the
> + * associated GPIO device
> + * @consumer: a desired consumer label for the selected GPIO lines such
> + * as "my-bitbanged-relay"
> + * @config: Requested configuration for the requested lines. Note that
> + * even if multiple lines are requested, the same configuration must be
> + * applicable to all of them. If you want lines with individual
> + * configuration, request them one by one. It is possible to select a
> + * batch of input or output lines, but they must all have the same
> + * configuration, i.e. all inputs or all outputs, all active low etc
> + * @lines: number of lines requested in this request, i.e. the number of
> + * valid fields in the GPIOLINES_MAX sized arrays, set to 1 to request a
> + * single line
> + * @padding: reserved for future use and should 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 gpioline_request {
> +       __u32 offsets[GPIOLINES_MAX];
> +       char consumer[GPIO_MAX_NAME_SIZE];
> +       struct gpioline_config config;
> +       __u32 lines;
> +       __u32 padding[4]; /* for future use */
> +       __s32 fd;
> +};
> +
> +/**
> + * struct gpioline_values - Values of GPIO lines
> + * @values: when getting the state of lines this contains the current
> + * state of a line, when setting the state of lines these should contain
> + * the desired target state
> + */
> +struct gpioline_values {
> +       __u8 values[GPIOLINES_MAX];
> +};
> +
> +/**
> + * struct gpioline_info_v2 - 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
> + * @offset: the local offset on this GPIO device, fill this in when
> + * requesting the line information from the kernel
> + * @flags: various flags for this line
> + * @direction: if GPIOLINE_FLAG_V2_DIRECTION is set in flags, the direction
> + * of the line, with a value from enum gpioline_direction
> + * @drive: if GPIOLINE_FLAG_V2_DRIVE is set in flags, the drive for the
> + * line, with a value from enum gpioline_drive
> + * @bias: if GPIOLINE_FLAG_V2_BIAS is set in flags, the bias for the line,
> + * with a value from enum gpioline_bias
> + * @edge_detection: if GPIOLINE_FLAG_V2_EDGE_DETECTION is set in flags, the
> + * edge_detection for the line, with a value from enum gpioline_edge
> + * @debounce: if GPIOLINE_FLAG_V2_DEBOUNCE is set in flags, the debounce
> + * period for the line, in microseconds
> + * @padding: reserved for future use
> + */

> +struct gpioline_info_v2 {
> +       char name[GPIO_MAX_NAME_SIZE];
> +       char consumer[GPIO_MAX_NAME_SIZE];
> +       __u32 offset;
> +       __u32 flags;
> +       __u8 direction;
> +       __u8 drive;
> +       __u8 bias;
> +       __u8 edge_detection;
> +       __u32 debounce;
> +       __u32 padding[12]; /* for future use */
> +};
> +
> +/**
> + * struct gpioline_info_changed_v2 - Information about a change in status
> + * of a GPIO line
> + * @info: updated line information
> + * @timestamp: estimate of time of status change occurrence, in nanoseconds
> + * and GPIOLINE_CHANGED_CONFIG
> + * @event_type: one of GPIOLINE_CHANGED_REQUESTED, GPIOLINE_CHANGED_RELEASED
> + * @padding: reserved for future use
> + */
> +struct gpioline_info_changed_v2 {
> +       struct gpioline_info_v2 info;
> +       __u64 timestamp;
> +       __u32 event_type;
> +       __u32 padding[5]; /* for future use */
> +};
> +
> +enum gpioline_event_id {
> +       GPIOLINE_EVENT_RISING_EDGE      = 1,
> +       GPIOLINE_EVENT_FALLING_EDGE     = 2,
> +};
> +
> +/**
> + * struct gpioline_event - The actual event being pushed to userspace
> + * @timestamp: best estimate of time of event occurrence, in nanoseconds
> + * @id: event identifier with value from enum gpioline_event_id
> + * @offset: the offset of the line that triggered the event
> + * @padding: reserved for future use
> + */
> +struct gpioline_event {
> +       __u64 timestamp;
> +       __u32 id;
> +       __u32 offset;
> +       __u32 padding[2]; /* for future use */
> +};
> +
>  /* Informational flags */
>  #define GPIOLINE_FLAG_KERNEL           (1UL << 0) /* Line used by the kernel */
>  #define GPIOLINE_FLAG_IS_OUT           (1UL << 1)
> @@ -144,8 +325,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
> @@ -156,9 +335,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)
> @@ -202,11 +378,25 @@ struct gpioevent_data {
>         __u32 id;
>  };
>
> +/* V1 and V2 */
>  #define GPIO_GET_CHIPINFO_IOCTL _IOR(0xB4, 0x01, struct gpiochip_info)
> +#define GPIO_GET_LINEINFO_UNWATCH_IOCTL _IOWR(0xB4, 0x0C, __u32)
> +
> +/* V1 */
>  #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)
> +
> +/* V2 */
> +#define GPIO_GET_LINEINFO_V2_IOCTL _IOWR(0xB4, 0x0D, struct gpioline_info_v2)
> +#define GPIO_GET_LINEINFO_WATCH_V2_IOCTL _IOWR(0xB4, 0x0E, struct gpioline_info_v2)
> +#define GPIO_GET_LINE_IOCTL _IOWR(0xB4, 0x0F, struct gpioline_request)
> +#define GPIOLINE_SET_CONFIG_IOCTL _IOWR(0xB4, 0x10, struct gpioline_config)
> +#define GPIOLINE_GET_VALUES_IOCTL _IOWR(0xB4, 0x11, struct gpioline_values)
> +#define GPIOLINE_SET_VALUES_IOCTL _IOWR(0xB4, 0x12, struct gpioline_values)

Can't we use existing flags (field) to distinguish versions at least
between v1 and v2?
The IOCTL namespace is not big and adding such a group of new calls
perhaps too much.

>  #endif /* _UAPI_GPIO_H_ */
Kent Gibson May 26, 2020, 12:42 p.m. UTC | #5
On Tue, May 26, 2020 at 11:04:25AM +0300, Andy Shevchenko wrote:
> +Cc: Ville
> 
> Ville, this is a v2 of the GPIO ABI we discussed with some time ago.
> If you have time to briefly look at it and perhaps comment if it's
> right way to go.
> 
> On Sat, May 16, 2020 at 9:50 AM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > Add a new version of the uAPI to address existing 32/64bit alignment
> > issues, add support for debounce, and provide some future proofing by
> > adding padding reserved for future use.
> >
> > Signed-off-by: Kent Gibson <warthog618@gmail.com>
> >
> > ---
> >
> > This patch is a proposal to replace the majority of the uAPI, so some
> > background and justification is in order.
> >
> > The alignment issue relates to the gpioevent_data, which packs to different
> > sizes on 32bit and 64bit platforms. That creates problems for 32bit apps
> > running on 64bit kernels.  The patch addresses that particular issue, and
> > the problem more generally, by adding pad fields that explicitly pad
> > structs out to 64bit 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 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 reserved padding in all
> > structs for future features.  Specifically, the line request,
> > config and info structs get updated versions and ioctls.
> >
> > I haven't added any padding to gpiochip_info, as I haven't seen any calls
> > for new features for the corresponding ioctl, but I'm open to updating that
> > as well.
> >
> > As the majority of the structs and ioctls were being replaced, it seemed
> > opportune to rework some of the other aspects of the uAPI.
> >
> > Firstly, I've reworked the flags field throughout.  v1 has three different
> > flags fields, each with their own separate bit definitions.  In v2 that is
> > collapsed to one.  Further, the bits of the v2 flags field are used
> > as feature enable flags, with any other necessary configuration fields encoded
> > separately.  This is simpler and clearer, while also providing a foundation
> > for adding features in the future.
> >
> > I've also merged the handle and event requests into a single request, the
> > line request, as the two requests where 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.
> >
> > This means 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.
> 
> > And v1 GPIOHANDLES_MAX and gpiohandle_data become GPIOLINES_MAX and
> > gpioline_values for v2 - the only change being the renaming for clarity.
> 
> Hmm... I think it makes sense if you fully replace uAPI, otherwise in
> my opinion it adds to the confusion.
> My point is that we may try to be less invasive, perhaps?
> 

I had an earlier draft that did just that, but I found that having the
"handle" names still floating around confusing - particularly
considering what the header will eventually look like once v1 is
eventually removed.  I'm also assuming the v1 fields will get
documentation updates should we proceed with v2 and deprecate v1.

> > The v2 uAPI is mostly just a reorganisation of v1, so userspace code,
> > particularly libgpiod, should easily port to it.
> >
> > This patch is obviously only one patch in a much bigger series that
> > will actually implement it, but I would appreciate a review and any feedback,
> > as it is foundational to the rest of that series.
> >
> > Thanks,
> > Kent.
> >
> >  include/uapi/linux/gpio.h | 204 ++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 197 insertions(+), 7 deletions(-)
> >
> > diff --git a/include/uapi/linux/gpio.h b/include/uapi/linux/gpio.h
> > index 0206383c0383..3db7e0bc1312 100644
> > --- a/include/uapi/linux/gpio.h
> > +++ b/include/uapi/linux/gpio.h
> > @@ -14,6 +14,9 @@
> >  #include <linux/ioctl.h>
> >  #include <linux/types.h>
> >
> > +/* The maximum size of name and label arrays */
> > +#define GPIO_MAX_NAME_SIZE 32
> > +
> >  /**
> >   * struct gpiochip_info - Information about a certain GPIO chip
> >   * @name: the Linux kernel name of this GPIO chip
> > @@ -27,6 +30,184 @@ struct gpiochip_info {
> >         __u32 lines;
> >  };
> >
> > +/* Maximum number of requested lines */
> > +#define GPIOLINES_MAX 64
> > +
> > +enum gpioline_direction {
> > +       GPIOLINE_DIRECTION_INPUT        = 1,
> > +       GPIOLINE_DIRECTION_OUTPUT       = 2,
> > +};
> > +
> > +enum gpioline_drive {
> > +       GPIOLINE_DRIVE_PUSH_PULL        = 0,
> > +       GPIOLINE_DRIVE_OPEN_DRAIN       = 1,
> > +       GPIOLINE_DRIVE_OPEN_SOURCE      = 2,
> > +};
> > +
> > +enum gpioline_bias {
> > +       GPIOLINE_BIAS_DISABLE           = 0,
> > +       GPIOLINE_BIAS_PULL_UP           = 1,
> > +       GPIOLINE_BIAS_PULL_DOWN         = 2,
> > +};
> > +
> > +enum gpioline_edge {
> > +       GPIOLINE_EDGE_NONE              = 0,
> > +       GPIOLINE_EDGE_RISING            = 1,
> > +       GPIOLINE_EDGE_FALLING           = 2,
> > +       GPIOLINE_EDGE_BOTH              = 3,
> > +};
> > +
> > +/* Line flags - V2 */
> > +#define GPIOLINE_FLAG_V2_KERNEL                (1UL << 0) /* Line used by the kernel */
> > +#define GPIOLINE_FLAG_V2_ACTIVE_LOW    (1UL << 1)
> > +#define GPIOLINE_FLAG_V2_DIRECTION     (1UL << 2)
> > +#define GPIOLINE_FLAG_V2_DRIVE         (1UL << 3)
> > +#define GPIOLINE_FLAG_V2_BIAS          (1UL << 4)
> > +#define GPIOLINE_FLAG_V2_EDGE_DETECTION        (1UL << 5)
> > +#define GPIOLINE_FLAG_V2_DEBOUNCE      (1UL << 6)
> > +
> > +/**
> > + * struct gpioline_config - Configuration for GPIO lines
> > + * @default_values: if the direction is GPIOLINE_DIRECTION_OUTPUT, this
> > + * specifies the default output value, should be 0 (low) or 1 (high),
> > + * anything else than 0 or 1 will be interpreted as 1 (high)
> > + * @flags: flags for the GPIO lines, such as GPIOLINE_FLAG_V2_ACTIVE_LOW,
> > + * GPIOLINE_FLAG_V2_DIRECTION etc, OR:ed together
> > + * @direction: if GPIOLINE_FLAG_V2_DIRECTION is set in flags, the desired
> > + * direction for the requested lines, with a value from enum
> > + * gpioline_direction
> > + * @drive: if GPIOLINE_FLAG_V2_DRIVE is set in flags, the desired drive for
> > + * the requested lines, with a value from enum gpioline_drive
> > + * @bias: if GPIOLINE_FLAG_V2_BIAS is set in flags, the desired bias for
> > + * the requested lines, with a value from enum gpioline_bias
> > + * @edge_detection: if GPIOLINE_FLAG_V2_EDGE_DETECTION is set in flags, the
> > + * desired edge_detection for the requested lines, with a value from enum
> > + * gpioline_edge
> > + * @debounce: if GPIOLINE_FLAG_V2_DEBOUNCE is set in flags, the desired
> > + * debounce period for the requested lines, in microseconds
> > + * @padding: reserved for future use and should be zero filled
> > + */
> > +struct gpioline_config {
> > +       __u8 default_values[GPIOLINES_MAX];
> 
> This basically means that whatever in square brackets must be aligned
> to 8. So, BUILD_BUG_ON() here and in similar places where it makes
> sense.
> 

That is true, and also applies to GPIO_MAX_NAME_SIZE.

Can't put BUILD_BUG_ON checks in the header, but happy to add checks to
gpiolib-cdev.c, as well as some comments in the header.

> > +       __u32 flags;
> > +       __u8 direction;
> > +       __u8 drive;
> > +       __u8 bias;
> > +       __u8 edge_detection;
> > +       __u32 debounce;
> 
> > +       __u32 padding[7]; /* for future use */
> 
> Why 7 and not 5 or even 1?
> Can we use size of struct as a versioning?
> Or maybe explicit version field?
> 

Basically I tried to pick a number that pads the struct out to 64bits,
provides a reasonable amount of room for future use, but isn't stupidly
large.  How much is reasonable depends on how likely the struct is to be
extended in the future.  _config is on the upper end of that, so gets
more than the rest. _info_v2 may have to contain the contents of
_request, which has _config embedded, so its pad is as large as _config
and _request combined.

I'm open to other sizings - these just seemed reasonable to me. YMMV.

> Same questions to all big paddings.
> 
> > +};
> > +
> > +/**
> > + * struct gpioline_request - Information about a request for GPIO lines
> > + * @offsets: an array of desired lines, specified by offset index for the
> > + * associated GPIO device
> > + * @consumer: a desired consumer label for the selected GPIO lines such
> > + * as "my-bitbanged-relay"
> > + * @config: Requested configuration for the requested lines. Note that
> > + * even if multiple lines are requested, the same configuration must be
> > + * applicable to all of them. If you want lines with individual
> > + * configuration, request them one by one. It is possible to select a
> > + * batch of input or output lines, but they must all have the same
> > + * configuration, i.e. all inputs or all outputs, all active low etc
> > + * @lines: number of lines requested in this request, i.e. the number of
> > + * valid fields in the GPIOLINES_MAX sized arrays, set to 1 to request a
> > + * single line
> > + * @padding: reserved for future use and should 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 gpioline_request {
> > +       __u32 offsets[GPIOLINES_MAX];
> > +       char consumer[GPIO_MAX_NAME_SIZE];
> > +       struct gpioline_config config;
> > +       __u32 lines;
> > +       __u32 padding[4]; /* for future use */
> > +       __s32 fd;
> > +};
> > +
> > +/**
> > + * struct gpioline_values - Values of GPIO lines
> > + * @values: when getting the state of lines this contains the current
> > + * state of a line, when setting the state of lines these should contain
> > + * the desired target state
> > + */
> > +struct gpioline_values {
> > +       __u8 values[GPIOLINES_MAX];
> > +};
> > +
> > +/**
> > + * struct gpioline_info_v2 - 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
> > + * @offset: the local offset on this GPIO device, fill this in when
> > + * requesting the line information from the kernel
> > + * @flags: various flags for this line
> > + * @direction: if GPIOLINE_FLAG_V2_DIRECTION is set in flags, the direction
> > + * of the line, with a value from enum gpioline_direction
> > + * @drive: if GPIOLINE_FLAG_V2_DRIVE is set in flags, the drive for the
> > + * line, with a value from enum gpioline_drive
> > + * @bias: if GPIOLINE_FLAG_V2_BIAS is set in flags, the bias for the line,
> > + * with a value from enum gpioline_bias
> > + * @edge_detection: if GPIOLINE_FLAG_V2_EDGE_DETECTION is set in flags, the
> > + * edge_detection for the line, with a value from enum gpioline_edge
> > + * @debounce: if GPIOLINE_FLAG_V2_DEBOUNCE is set in flags, the debounce
> > + * period for the line, in microseconds
> > + * @padding: reserved for future use
> > + */
> 
> > +struct gpioline_info_v2 {
> > +       char name[GPIO_MAX_NAME_SIZE];
> > +       char consumer[GPIO_MAX_NAME_SIZE];
> > +       __u32 offset;
> > +       __u32 flags;
> > +       __u8 direction;
> > +       __u8 drive;
> > +       __u8 bias;
> > +       __u8 edge_detection;
> > +       __u32 debounce;
> > +       __u32 padding[12]; /* for future use */
> > +};
> > +
> > +/**
> > + * struct gpioline_info_changed_v2 - Information about a change in status
> > + * of a GPIO line
> > + * @info: updated line information
> > + * @timestamp: estimate of time of status change occurrence, in nanoseconds
> > + * and GPIOLINE_CHANGED_CONFIG
> > + * @event_type: one of GPIOLINE_CHANGED_REQUESTED, GPIOLINE_CHANGED_RELEASED
> > + * @padding: reserved for future use
> > + */
> > +struct gpioline_info_changed_v2 {
> > +       struct gpioline_info_v2 info;
> > +       __u64 timestamp;
> > +       __u32 event_type;
> > +       __u32 padding[5]; /* for future use */
> > +};
> > +
> > +enum gpioline_event_id {
> > +       GPIOLINE_EVENT_RISING_EDGE      = 1,
> > +       GPIOLINE_EVENT_FALLING_EDGE     = 2,
> > +};
> > +
> > +/**
> > + * struct gpioline_event - The actual event being pushed to userspace
> > + * @timestamp: best estimate of time of event occurrence, in nanoseconds
> > + * @id: event identifier with value from enum gpioline_event_id
> > + * @offset: the offset of the line that triggered the event
> > + * @padding: reserved for future use
> > + */
> > +struct gpioline_event {
> > +       __u64 timestamp;
> > +       __u32 id;
> > +       __u32 offset;
> > +       __u32 padding[2]; /* for future use */
> > +};
> > +
> >  /* Informational flags */
> >  #define GPIOLINE_FLAG_KERNEL           (1UL << 0) /* Line used by the kernel */
> >  #define GPIOLINE_FLAG_IS_OUT           (1UL << 1)
> > @@ -144,8 +325,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
> > @@ -156,9 +335,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)
> > @@ -202,11 +378,25 @@ struct gpioevent_data {
> >         __u32 id;
> >  };
> >
> > +/* V1 and V2 */
> >  #define GPIO_GET_CHIPINFO_IOCTL _IOR(0xB4, 0x01, struct gpiochip_info)
> > +#define GPIO_GET_LINEINFO_UNWATCH_IOCTL _IOWR(0xB4, 0x0C, __u32)
> > +
> > +/* V1 */
> >  #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)
> > +
> > +/* V2 */
> > +#define GPIO_GET_LINEINFO_V2_IOCTL _IOWR(0xB4, 0x0D, struct gpioline_info_v2)
> > +#define GPIO_GET_LINEINFO_WATCH_V2_IOCTL _IOWR(0xB4, 0x0E, struct gpioline_info_v2)
> > +#define GPIO_GET_LINE_IOCTL _IOWR(0xB4, 0x0F, struct gpioline_request)
> > +#define GPIOLINE_SET_CONFIG_IOCTL _IOWR(0xB4, 0x10, struct gpioline_config)
> > +#define GPIOLINE_GET_VALUES_IOCTL _IOWR(0xB4, 0x11, struct gpioline_values)
> > +#define GPIOLINE_SET_VALUES_IOCTL _IOWR(0xB4, 0x12, struct gpioline_values)
> 
> Can't we use existing flags (field) to distinguish versions at least
> between v1 and v2?

I'm not sure how that would work. Can you provide an example?

> The IOCTL namespace is not big and adding such a group of new calls
> perhaps too much.
> 

The NR field is 8bits, and so far we've used only 18 values (although
I'm not sure on 5,6, and 7).  I must be missing something, cos that is
about as close to exhausting the namespace as we were before??

Having said that, your recent gpiolib patch has made me consider merging
GPIO_GET_LINEINFO_V2_IOCTL and GPIO_GET_LINEINFO_WATCH_V2_IOCTL.
That would save one ioctl and may reduce the code a little as well.

Hmmm, and perhaps the GPIOHANDLE_GET_LINE_VALUES_IOCTL and
GPIOHANDLE_SET_LINE_VALUES_IOCTL can just be aliases for the GPIOLINE
equivalents, given the structs are identical other than naming??

Cheers,
Kent.
Kent Gibson May 27, 2020, 12:35 a.m. UTC | #6
On Mon, May 25, 2020 at 06:24:04PM +0200, Bartosz Golaszewski wrote:
> sob., 16 maj 2020 o 08:45 Kent Gibson <warthog618@gmail.com> napisał(a):
> >
> > Add a new version of the uAPI to address existing 32/64bit alignment
> > issues, add support for debounce, and provide some future proofing by
> > adding padding reserved for future use.
> >
> > Signed-off-by: Kent Gibson <warthog618@gmail.com>
> >
> > ---
> >
> > This patch is a proposal to replace the majority of the uAPI, so some
> > background and justification is in order.
> >
> > The alignment issue relates to the gpioevent_data, which packs to different
> > sizes on 32bit and 64bit platforms. That creates problems for 32bit apps
> > running on 64bit kernels.  The patch addresses that particular issue, and
> > the problem more generally, by adding pad fields that explicitly pad
> > structs out to 64bit 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 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 reserved padding in all
> > structs for future features.  Specifically, the line request,
> > config and info structs get updated versions and ioctls.
> >
> > I haven't added any padding to gpiochip_info, as I haven't seen any calls
> > for new features for the corresponding ioctl, but I'm open to updating that
> > as well.
> >
> > As the majority of the structs and ioctls were being replaced, it seemed
> > opportune to rework some of the other aspects of the uAPI.
> >
> > Firstly, I've reworked the flags field throughout.  v1 has three different
> > flags fields, each with their own separate bit definitions.  In v2 that is
> > collapsed to one.  Further, the bits of the v2 flags field are used
> > as feature enable flags, with any other necessary configuration fields encoded
> > separately.  This is simpler and clearer, while also providing a foundation
> > for adding features in the future.
> >
> > I've also merged the handle and event requests into a single request, the
> > line request, as the two requests where 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.
> >
> > This means 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.
> >
> > And v1 GPIOHANDLES_MAX and gpiohandle_data become GPIOLINES_MAX and
> > gpioline_values for v2 - the only change being the renaming for clarity.
> >
> > The v2 uAPI is mostly just a reorganisation of v1, so userspace code,
> > particularly libgpiod, should easily port to it.
> >
> > This patch is obviously only one patch in a much bigger series that
> > will actually implement it, but I would appreciate a review and any feedback,
> > as it is foundational to the rest of that series.
> >
> > Thanks,
> > Kent.
> >
> 
> Hi Kent,
> 
> Thanks for posting this. I like the general direction a lot. I'll
> review this in detail later this week.
> 
> Seeing the speed at which you make progress I think I won't be
> implementing support for the v1 of the watch ioctl() in libgpiod after
> all. Once the v2 is live I will probably bump the API version in
> libgpiod to v2.0.0 and make some non-compatible changes anyway.
> 

Yeah, libgpiod has similar problems to the v1 uAPI - there is no
easy way to extend it without causing breakage.

I've got a patch that ports libgpiod to the v2 uAPI, though it only goes
as far as v1 parity.  That is passing all existing tests.  The patch
doesn't address debounce as that will require libgpiod API changes.  Nor
does it make use of the bulk event capability, as that would change
behaviour and break some of the tests - the wait_multiple test was the
sticking point there.  Those are probably best combined with the v2.0.0
changes you are planning.

I've also got a couple of patches for minor things that I noticed while I
was in there.  I'll post them shortly.

Cheers,
Kent.
Linus Walleij May 27, 2020, 5:58 a.m. UTC | #7
On Mon, May 25, 2020 at 4:19 PM Kent Gibson <warthog618@gmail.com> wrote:

> > > +struct gpioline_config {
> > > +       __u8 default_values[GPIOLINES_MAX];
> >
> > So 32 bytes
> >
>
> Actually that one is 64 bytes, which is the same as v1, i.e. GPIOLINES_MAX
> is the same as GPIOHANDLES_MAX - just renamed.
>
> On the subject of values, is there any reason to use a byte for each line
> rather value than a bit?

Not really, other than making things simple for userspace.

> when adding future fields, the idea was to have a bit
> in the flags that indicates that the corresponding field is now valid.
> If the flag is not set then whatever value is there is irrelevant.

You would need to document that idea, say in the kerneldoc,
else when someone else comes along to do this they will
get it wrong.

> But definitely better to play it safe - will check the padding is
> zeroed as well, as well as any field for which the flag bit is clear.

Yeah better like that. You can write a comment in the code too,
such like "when adding new parameters, update this validation code
to accept it".

> Back on retired ioctls, I notice that 5, 6, and 7 are missing from gpio.
> Have those been retired, or just skipped over by accident?

Just thought it was nice to use jump to 8 for line info.
They should be used when adding generic chip information ioctls().

Yours,
Linus Walleij
diff mbox series

Patch

diff --git a/include/uapi/linux/gpio.h b/include/uapi/linux/gpio.h
index 0206383c0383..3db7e0bc1312 100644
--- a/include/uapi/linux/gpio.h
+++ b/include/uapi/linux/gpio.h
@@ -14,6 +14,9 @@ 
 #include <linux/ioctl.h>
 #include <linux/types.h>
 
+/* The maximum size of name and label arrays */
+#define GPIO_MAX_NAME_SIZE 32
+
 /**
  * struct gpiochip_info - Information about a certain GPIO chip
  * @name: the Linux kernel name of this GPIO chip
@@ -27,6 +30,184 @@  struct gpiochip_info {
 	__u32 lines;
 };
 
+/* Maximum number of requested lines */
+#define GPIOLINES_MAX 64
+
+enum gpioline_direction {
+	GPIOLINE_DIRECTION_INPUT	= 1,
+	GPIOLINE_DIRECTION_OUTPUT	= 2,
+};
+
+enum gpioline_drive {
+	GPIOLINE_DRIVE_PUSH_PULL	= 0,
+	GPIOLINE_DRIVE_OPEN_DRAIN	= 1,
+	GPIOLINE_DRIVE_OPEN_SOURCE	= 2,
+};
+
+enum gpioline_bias {
+	GPIOLINE_BIAS_DISABLE		= 0,
+	GPIOLINE_BIAS_PULL_UP		= 1,
+	GPIOLINE_BIAS_PULL_DOWN		= 2,
+};
+
+enum gpioline_edge {
+	GPIOLINE_EDGE_NONE		= 0,
+	GPIOLINE_EDGE_RISING		= 1,
+	GPIOLINE_EDGE_FALLING		= 2,
+	GPIOLINE_EDGE_BOTH		= 3,
+};
+
+/* Line flags - V2 */
+#define GPIOLINE_FLAG_V2_KERNEL		(1UL << 0) /* Line used by the kernel */
+#define GPIOLINE_FLAG_V2_ACTIVE_LOW	(1UL << 1)
+#define GPIOLINE_FLAG_V2_DIRECTION	(1UL << 2)
+#define GPIOLINE_FLAG_V2_DRIVE		(1UL << 3)
+#define GPIOLINE_FLAG_V2_BIAS		(1UL << 4)
+#define GPIOLINE_FLAG_V2_EDGE_DETECTION	(1UL << 5)
+#define GPIOLINE_FLAG_V2_DEBOUNCE	(1UL << 6)
+
+/**
+ * struct gpioline_config - Configuration for GPIO lines
+ * @default_values: if the direction is GPIOLINE_DIRECTION_OUTPUT, this
+ * specifies the default output value, should be 0 (low) or 1 (high),
+ * anything else than 0 or 1 will be interpreted as 1 (high)
+ * @flags: flags for the GPIO lines, such as GPIOLINE_FLAG_V2_ACTIVE_LOW,
+ * GPIOLINE_FLAG_V2_DIRECTION etc, OR:ed together
+ * @direction: if GPIOLINE_FLAG_V2_DIRECTION is set in flags, the desired
+ * direction for the requested lines, with a value from enum
+ * gpioline_direction
+ * @drive: if GPIOLINE_FLAG_V2_DRIVE is set in flags, the desired drive for
+ * the requested lines, with a value from enum gpioline_drive
+ * @bias: if GPIOLINE_FLAG_V2_BIAS is set in flags, the desired bias for
+ * the requested lines, with a value from enum gpioline_bias
+ * @edge_detection: if GPIOLINE_FLAG_V2_EDGE_DETECTION is set in flags, the
+ * desired edge_detection for the requested lines, with a value from enum
+ * gpioline_edge
+ * @debounce: if GPIOLINE_FLAG_V2_DEBOUNCE is set in flags, the desired
+ * debounce period for the requested lines, in microseconds
+ * @padding: reserved for future use and should be zero filled
+ */
+struct gpioline_config {
+	__u8 default_values[GPIOLINES_MAX];
+	__u32 flags;
+	__u8 direction;
+	__u8 drive;
+	__u8 bias;
+	__u8 edge_detection;
+	__u32 debounce;
+	__u32 padding[7]; /* for future use */
+};
+
+/**
+ * struct gpioline_request - Information about a request for GPIO lines
+ * @offsets: an array of desired lines, specified by offset index for the
+ * associated GPIO device
+ * @consumer: a desired consumer label for the selected GPIO lines such
+ * as "my-bitbanged-relay"
+ * @config: Requested configuration for the requested lines. Note that
+ * even if multiple lines are requested, the same configuration must be
+ * applicable to all of them. If you want lines with individual
+ * configuration, request them one by one. It is possible to select a
+ * batch of input or output lines, but they must all have the same
+ * configuration, i.e. all inputs or all outputs, all active low etc
+ * @lines: number of lines requested in this request, i.e. the number of
+ * valid fields in the GPIOLINES_MAX sized arrays, set to 1 to request a
+ * single line
+ * @padding: reserved for future use and should 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 gpioline_request {
+	__u32 offsets[GPIOLINES_MAX];
+	char consumer[GPIO_MAX_NAME_SIZE];
+	struct gpioline_config config;
+	__u32 lines;
+	__u32 padding[4]; /* for future use */
+	__s32 fd;
+};
+
+/**
+ * struct gpioline_values - Values of GPIO lines
+ * @values: when getting the state of lines this contains the current
+ * state of a line, when setting the state of lines these should contain
+ * the desired target state
+ */
+struct gpioline_values {
+	__u8 values[GPIOLINES_MAX];
+};
+
+/**
+ * struct gpioline_info_v2 - 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
+ * @offset: the local offset on this GPIO device, fill this in when
+ * requesting the line information from the kernel
+ * @flags: various flags for this line
+ * @direction: if GPIOLINE_FLAG_V2_DIRECTION is set in flags, the direction
+ * of the line, with a value from enum gpioline_direction
+ * @drive: if GPIOLINE_FLAG_V2_DRIVE is set in flags, the drive for the
+ * line, with a value from enum gpioline_drive
+ * @bias: if GPIOLINE_FLAG_V2_BIAS is set in flags, the bias for the line,
+ * with a value from enum gpioline_bias
+ * @edge_detection: if GPIOLINE_FLAG_V2_EDGE_DETECTION is set in flags, the
+ * edge_detection for the line, with a value from enum gpioline_edge
+ * @debounce: if GPIOLINE_FLAG_V2_DEBOUNCE is set in flags, the debounce
+ * period for the line, in microseconds
+ * @padding: reserved for future use
+ */
+struct gpioline_info_v2 {
+	char name[GPIO_MAX_NAME_SIZE];
+	char consumer[GPIO_MAX_NAME_SIZE];
+	__u32 offset;
+	__u32 flags;
+	__u8 direction;
+	__u8 drive;
+	__u8 bias;
+	__u8 edge_detection;
+	__u32 debounce;
+	__u32 padding[12]; /* for future use */
+};
+
+/**
+ * struct gpioline_info_changed_v2 - Information about a change in status
+ * of a GPIO line
+ * @info: updated line information
+ * @timestamp: estimate of time of status change occurrence, in nanoseconds
+ * and GPIOLINE_CHANGED_CONFIG
+ * @event_type: one of GPIOLINE_CHANGED_REQUESTED, GPIOLINE_CHANGED_RELEASED
+ * @padding: reserved for future use
+ */
+struct gpioline_info_changed_v2 {
+	struct gpioline_info_v2 info;
+	__u64 timestamp;
+	__u32 event_type;
+	__u32 padding[5]; /* for future use */
+};
+
+enum gpioline_event_id {
+	GPIOLINE_EVENT_RISING_EDGE	= 1,
+	GPIOLINE_EVENT_FALLING_EDGE	= 2,
+};
+
+/**
+ * struct gpioline_event - The actual event being pushed to userspace
+ * @timestamp: best estimate of time of event occurrence, in nanoseconds
+ * @id: event identifier with value from enum gpioline_event_id
+ * @offset: the offset of the line that triggered the event
+ * @padding: reserved for future use
+ */
+struct gpioline_event {
+	__u64 timestamp;
+	__u32 id;
+	__u32 offset;
+	__u32 padding[2]; /* for future use */
+};
+
 /* Informational flags */
 #define GPIOLINE_FLAG_KERNEL		(1UL << 0) /* Line used by the kernel */
 #define GPIOLINE_FLAG_IS_OUT		(1UL << 1)
@@ -144,8 +325,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
@@ -156,9 +335,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)
@@ -202,11 +378,25 @@  struct gpioevent_data {
 	__u32 id;
 };
 
+/* V1 and V2 */
 #define GPIO_GET_CHIPINFO_IOCTL _IOR(0xB4, 0x01, struct gpiochip_info)
+#define GPIO_GET_LINEINFO_UNWATCH_IOCTL _IOWR(0xB4, 0x0C, __u32)
+
+/* V1 */
 #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)
+
+/* V2 */
+#define GPIO_GET_LINEINFO_V2_IOCTL _IOWR(0xB4, 0x0D, struct gpioline_info_v2)
+#define GPIO_GET_LINEINFO_WATCH_V2_IOCTL _IOWR(0xB4, 0x0E, struct gpioline_info_v2)
+#define GPIO_GET_LINE_IOCTL _IOWR(0xB4, 0x0F, struct gpioline_request)
+#define GPIOLINE_SET_CONFIG_IOCTL _IOWR(0xB4, 0x10, struct gpioline_config)
+#define GPIOLINE_GET_VALUES_IOCTL _IOWR(0xB4, 0x11, struct gpioline_values)
+#define GPIOLINE_SET_VALUES_IOCTL _IOWR(0xB4, 0x12, struct gpioline_values)
 
 #endif /* _UAPI_GPIO_H_ */