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 <[email protected]>
---
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(-)
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_ */
--
2.26.2
On Sat, May 16, 2020 at 8:45 AM Kent Gibson <[email protected]> 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 <[email protected]>
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
On Mon, May 25, 2020 at 10:39:42AM +0200, Linus Walleij wrote:
> On Sat, May 16, 2020 at 8:45 AM Kent Gibson <[email protected]> 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 <[email protected]>
>
> 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.
sob., 16 maj 2020 o 08:45 Kent Gibson <[email protected]> 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 <[email protected]>
>
> ---
>
> 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
+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 <[email protected]> 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 <[email protected]>
>
> ---
>
> 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_ */
--
With Best Regards,
Andy Shevchenko
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 <[email protected]> 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 <[email protected]>
> >
> > ---
> >
> > 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.
On Mon, May 25, 2020 at 06:24:04PM +0200, Bartosz Golaszewski wrote:
> sob., 16 maj 2020 o 08:45 Kent Gibson <[email protected]> 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 <[email protected]>
> >
> > ---
> >
> > 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.
On Mon, May 25, 2020 at 4:19 PM Kent Gibson <[email protected]> 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
śr., 27 maj 2020 o 07:58 Linus Walleij <[email protected]> napisał(a):
>
> On Mon, May 25, 2020 at 4:19 PM Kent Gibson <[email protected]> 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.
>
I'm in favor of using bits here. I think we can rely on libgpiod to
make things simple for user-space, the kernel interface can be as
brief as possible.
Bart
On Thu, Jun 04, 2020 at 02:06:31PM +0200, Bartosz Golaszewski wrote:
> śr., 27 maj 2020 o 07:58 Linus Walleij <[email protected]> napisał(a):
> >
> > On Mon, May 25, 2020 at 4:19 PM Kent Gibson <[email protected]> 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.
> >
>
> I'm in favor of using bits here. I think we can rely on libgpiod to
> make things simple for user-space, the kernel interface can be as
> brief as possible.
>
OK, I'll take another look at it. If changed to a bitmap it will have
to be sized as a multiple of 64bits to maintain alignment. Other than
that it should be pretty straight forward.
Cheers,
Kent.
sob., 16 maj 2020 o 08:45 Kent Gibson <[email protected]> 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 <[email protected]>
>
I'm a bit late to the party but here's my review.
>
> 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,
> +};
I would skip the names of the enum types if we're not reusing them anywhere.
> +
> +/* Line flags - V2 */
> +#define GPIOLINE_FLAG_V2_KERNEL (1UL << 0) /* Line used by the kernel */
In v1 this flag is also set if the line is used by user-space. Maybe a
simple GPIOLINE_FLAG_V2_USED would be better?
> +#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];
As I said elsewhere - bitfield is fine here for me: for instance a single u64.
> + __u32 flags;
> + __u8 direction;
> + __u8 drive;
> + __u8 bias;
> + __u8 edge_detection;
> + __u32 debounce;
Maybe debounce_time for clarity?
> + __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;
Maybe num_lines would be clearer?
> + __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];
Same here for bitfield. And maybe reuse this structure in
gpioline_config for default values?
> +};
> +
> +/**
> + * 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;
I'd specify in the comment the type of clock used for the 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_ */
> --
> 2.26.2
>
Bartosz
On Thu, Jun 04, 2020 at 02:43:08PM +0200, Bartosz Golaszewski wrote:
> sob., 16 maj 2020 o 08:45 Kent Gibson <[email protected]> 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 <[email protected]>
> >
>
> I'm a bit late to the party but here's my review.
>
> >
> > 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,
> > +};
>
> I would skip the names of the enum types if we're not reusing them anywhere.
>
I thought it was useful to name them even if it was just to be able to
reference them in the documentation for relevant fields, such as that in
struct gpioline_config below, rather than having to either list all
possible values or a GPIOLINE_EDGE_* glob.
And I'm currently using enum gpioline_edge in my edge detector
implementation - is that sufficient?
> > +
> > +/* Line flags - V2 */
> > +#define GPIOLINE_FLAG_V2_KERNEL (1UL << 0) /* Line used by the kernel */
>
> In v1 this flag is also set if the line is used by user-space. Maybe a
> simple GPIOLINE_FLAG_V2_USED would be better?
>
Agreed - the _KERNEL name is confusing.
In my latest draft I've already renamed it GPIOLINE_FLAG_V2_BUSY,
as EBUSY is what the ioctl returns when you try to request such a line.
Does that work for you?
I was also considering _IN_USE, and was using _UNAVAILABLE for a while.
> > +#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];
>
> As I said elsewhere - bitfield is fine here for me: for instance a single u64.
>
> > + __u32 flags;
> > + __u8 direction;
> > + __u8 drive;
> > + __u8 bias;
> > + __u8 edge_detection;
> > + __u32 debounce;
>
> Maybe debounce_time for clarity?
>
> > + __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;
>
> Maybe num_lines would be clearer?
>
> > + __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];
>
> Same here for bitfield. And maybe reuse this structure in
> gpioline_config for default values?
>
Can do. What makes me reticent is the extra level of indirection
and the stuttering that would cause when referencing them.
e.g. config.default_values.values
So not sure the gain is worth the pain.
And I've renamed "default_values" to just "values" in my latest draft
which doesn't help with the stuttering.
> > +};
> > +
> > +/**
> > + * 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;
>
> I'd specify in the comment the type of clock used for the timestamp.
>
Agreed - as this one will be guaranteed to be CLOCK_MONOTONIC.
I'm also kicking around the idea of adding sequence numbers to events,
one per line and one per handle, so userspace can more easily detect
mis-ordering or buffer overflows. Does that make any sense?
And would it be useful for userspace to be able to influence the size of
the event buffer (currently fixed at 16 events per line)?
Cheers,
Kent.
> > + __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_ */
> > --
> > 2.26.2
> >
>
> Bartosz
czw., 4 cze 2020 o 18:00 Kent Gibson <[email protected]> napisał(a):
>
[snip!]
> > > +
> > > +enum gpioline_edge {
> > > + GPIOLINE_EDGE_NONE = 0,
> > > + GPIOLINE_EDGE_RISING = 1,
> > > + GPIOLINE_EDGE_FALLING = 2,
> > > + GPIOLINE_EDGE_BOTH = 3,
> > > +};
> >
> > I would skip the names of the enum types if we're not reusing them anywhere.
> >
>
> I thought it was useful to name them even if it was just to be able to
> reference them in the documentation for relevant fields, such as that in
> struct gpioline_config below, rather than having to either list all
> possible values or a GPIOLINE_EDGE_* glob.
>
> And I'm currently using enum gpioline_edge in my edge detector
> implementation - is that sufficient?
>
The documentation argument is more convincing. :)
> > > +
> > > +/* Line flags - V2 */
> > > +#define GPIOLINE_FLAG_V2_KERNEL (1UL << 0) /* Line used by the kernel */
> >
> > In v1 this flag is also set if the line is used by user-space. Maybe a
> > simple GPIOLINE_FLAG_V2_USED would be better?
> >
>
> Agreed - the _KERNEL name is confusing.
> In my latest draft I've already renamed it GPIOLINE_FLAG_V2_BUSY,
> as EBUSY is what the ioctl returns when you try to request such a line.
> Does that work for you?
> I was also considering _IN_USE, and was using _UNAVAILABLE for a while.
>
BUSY sounds less precise to me than USED or IN_USE of which both are
fine (with a preference for the former).
[snip!]
> > > +
> > > +/**
> > > + * 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];
> >
> > Same here for bitfield. And maybe reuse this structure in
> > gpioline_config for default values?
> >
>
> Can do. What makes me reticent is the extra level of indirection
> and the stuttering that would cause when referencing them.
> e.g. config.default_values.values
> So not sure the gain is worth the pain.
>
I'd say yes - consolidation and reuse of data structures is always
good and normally they are going to be wrapped in some kind of
low-level user-space library anyway.
> And I've renamed "default_values" to just "values" in my latest draft
> which doesn't help with the stuttering.
>
Why though? Aren't these always default values for output?
[snip!]
> > > +
> > > +/**
> > > + * 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;
> >
> > I'd specify in the comment the type of clock used for the timestamp.
> >
>
> Agreed - as this one will be guaranteed to be CLOCK_MONOTONIC.
>
> I'm also kicking around the idea of adding sequence numbers to events,
> one per line and one per handle, so userspace can more easily detect
> mis-ordering or buffer overflows. Does that make any sense?
>
Hmm, now that you mention it - and in the light of the recent post by
Ryan Lovelett about polling precision - I think it makes sense to have
this. Especially since it's very easy to add.
> And would it be useful for userspace to be able to influence the size of
> the event buffer (currently fixed at 16 events per line)?
>
Good question. I would prefer to not overdo it though. The event
request would need to contain the desired kfifo size and we'd only
allow to set it on request, right?
[snip!]
Bartosz
On Fri, Jun 05, 2020 at 11:53:05AM +0200, Bartosz Golaszewski wrote:
> czw., 4 cze 2020 o 18:00 Kent Gibson <[email protected]> napisał(a):
> >
>
> [snip!]
>
> > > > +
> > > > +enum gpioline_edge {
> > > > + GPIOLINE_EDGE_NONE = 0,
> > > > + GPIOLINE_EDGE_RISING = 1,
> > > > + GPIOLINE_EDGE_FALLING = 2,
> > > > + GPIOLINE_EDGE_BOTH = 3,
> > > > +};
> > >
> > > I would skip the names of the enum types if we're not reusing them anywhere.
> > >
> >
> > I thought it was useful to name them even if it was just to be able to
> > reference them in the documentation for relevant fields, such as that in
> > struct gpioline_config below, rather than having to either list all
> > possible values or a GPIOLINE_EDGE_* glob.
> >
> > And I'm currently using enum gpioline_edge in my edge detector
> > implementation - is that sufficient?
> >
>
> The documentation argument is more convincing. :)
>
I know - but your criteria was reuse... ;-).
> > > > +
> > > > +/* Line flags - V2 */
> > > > +#define GPIOLINE_FLAG_V2_KERNEL (1UL << 0) /* Line used by the kernel */
> > >
> > > In v1 this flag is also set if the line is used by user-space. Maybe a
> > > simple GPIOLINE_FLAG_V2_USED would be better?
> > >
> >
> > Agreed - the _KERNEL name is confusing.
> > In my latest draft I've already renamed it GPIOLINE_FLAG_V2_BUSY,
> > as EBUSY is what the ioctl returns when you try to request such a line.
> > Does that work for you?
> > I was also considering _IN_USE, and was using _UNAVAILABLE for a while.
> >
>
> BUSY sounds less precise to me than USED or IN_USE of which both are
> fine (with a preference for the former).
>
OK, USED it shall be.
> [snip!]
>
> > > > +
> > > > +/**
> > > > + * 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];
> > >
> > > Same here for bitfield. And maybe reuse this structure in
> > > gpioline_config for default values?
> > >
> >
> > Can do. What makes me reticent is the extra level of indirection
> > and the stuttering that would cause when referencing them.
> > e.g. config.default_values.values
> > So not sure the gain is worth the pain.
> >
>
> I'd say yes - consolidation and reuse of data structures is always
> good and normally they are going to be wrapped in some kind of
> low-level user-space library anyway.
>
Ok, and I've changed the values field name to bitmap, along with the change
to a bitmap type, so the stuttering is gone.
And, as the change to bitmap substantially reduced the size of
gpioline_config, I now embed that in the gpioline_info instead of
duplicating all the other fields. The values field will be zeroed
when returned within info.
> > And I've renamed "default_values" to just "values" in my latest draft
> > which doesn't help with the stuttering.
> >
>
> Why though? Aren't these always default values for output?
>
To me "default" implies a fallback value, and that de-emphasises the
fact that the lines will be immediately set to those values as they
are switched to outputs.
These are the values the outputs will take - the "default" doesn't add
anything.
> [snip!]
>
> > > > +
> > > > +/**
> > > > + * 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;
> > >
> > > I'd specify in the comment the type of clock used for the timestamp.
> > >
> >
> > Agreed - as this one will be guaranteed to be CLOCK_MONOTONIC.
> >
> > I'm also kicking around the idea of adding sequence numbers to events,
> > one per line and one per handle, so userspace can more easily detect
> > mis-ordering or buffer overflows. Does that make any sense?
> >
>
> Hmm, now that you mention it - and in the light of the recent post by
> Ryan Lovelett about polling precision - I think it makes sense to have
> this. Especially since it's very easy to add.
>
OK. I was only thinking about the edge events, but you might want it
for your line info events on the chip fd as well?
> > And would it be useful for userspace to be able to influence the size of
> > the event buffer (currently fixed at 16 events per line)?
> >
>
> Good question. I would prefer to not overdo it though. The event
> request would need to contain the desired kfifo size and we'd only
> allow to set it on request, right?
>
Yeah, it would only be relevant if edge detection was set and, as per
edge detection itself, would only be settable via the request, not
via set_config. It would only be a suggestion, as the kfifo size gets
rounded up to a power of 2 anyway. It would be capped - I'm open to
suggestions for a suitable max value. And the 0 value would mean use
the default - currently 16 per line.
If you want the equivalent for the info watch then I'm not sure where to
hook it in. It should be at the chip scope, and there isn't any
suitable ioctl to hook it into so it would need a new one - maybe a
set_config for the chip? But the buffer size would only be settable up
until you add a watch.
Cheers,
Kent.
sob., 6 cze 2020 o 03:56 Kent Gibson <[email protected]> napisał(a):
>
[snip!]
> >
> > I'd say yes - consolidation and reuse of data structures is always
> > good and normally they are going to be wrapped in some kind of
> > low-level user-space library anyway.
> >
>
> Ok, and I've changed the values field name to bitmap, along with the change
> to a bitmap type, so the stuttering is gone.
>
> And, as the change to bitmap substantially reduced the size of
> gpioline_config, I now embed that in the gpioline_info instead of
> duplicating all the other fields. The values field will be zeroed
> when returned within info.
>
Could you post an example, I'm not sure I follow.
> > > And I've renamed "default_values" to just "values" in my latest draft
> > > which doesn't help with the stuttering.
> > >
> >
> > Why though? Aren't these always default values for output?
> >
>
> To me "default" implies a fallback value, and that de-emphasises the
> fact that the lines will be immediately set to those values as they
> are switched to outputs.
> These are the values the outputs will take - the "default" doesn't add
> anything.
>
Fair enough, values it is.
[snip!]
> > >
> > > I'm also kicking around the idea of adding sequence numbers to events,
> > > one per line and one per handle, so userspace can more easily detect
> > > mis-ordering or buffer overflows. Does that make any sense?
> > >
> >
> > Hmm, now that you mention it - and in the light of the recent post by
> > Ryan Lovelett about polling precision - I think it makes sense to have
> > this. Especially since it's very easy to add.
> >
>
> OK. I was only thinking about the edge events, but you might want it
> for your line info events on the chip fd as well?
>
I don't see the need for it now, but you never know. Let's leave it
out for now and if we ever need it - we now have the appropriate
padding.
> > > And would it be useful for userspace to be able to influence the size of
> > > the event buffer (currently fixed at 16 events per line)?
> > >
> >
> > Good question. I would prefer to not overdo it though. The event
> > request would need to contain the desired kfifo size and we'd only
> > allow to set it on request, right?
> >
>
> Yeah, it would only be relevant if edge detection was set and, as per
> edge detection itself, would only be settable via the request, not
> via set_config. It would only be a suggestion, as the kfifo size gets
> rounded up to a power of 2 anyway. It would be capped - I'm open to
> suggestions for a suitable max value. And the 0 value would mean use
> the default - currently 16 per line.
>
This sounds good. How about 512 for max value for now and we can
always increase it if needed. I don't think we should explicitly cap
it though - let the user specify any value and just silently limit it
to 512 in the kernel.
> If you want the equivalent for the info watch then I'm not sure where to
> hook it in. It should be at the chip scope, and there isn't any
> suitable ioctl to hook it into so it would need a new one - maybe a
> set_config for the chip? But the buffer size would only be settable up
> until you add a watch.
>
I don't think we need this. Status changes are naturally much less
frequent and the potential for buffer overflow is miniscule here.
Bart
wt., 9 cze 2020 o 11:43 Kent Gibson <[email protected]> napisał(a):
>
> On Tue, Jun 09, 2020 at 10:03:42AM +0200, Bartosz Golaszewski wrote:
> > sob., 6 cze 2020 o 03:56 Kent Gibson <[email protected]> napisał(a):
> > >
> >
> > [snip!]
> >
> > > >
> > > > I'd say yes - consolidation and reuse of data structures is always
> > > > good and normally they are going to be wrapped in some kind of
> > > > low-level user-space library anyway.
> > > >
> > >
> > > Ok, and I've changed the values field name to bitmap, along with the change
> > > to a bitmap type, so the stuttering is gone.
> > >
> > > And, as the change to bitmap substantially reduced the size of
> > > gpioline_config, I now embed that in the gpioline_info instead of
> > > duplicating all the other fields. The values field will be zeroed
> > > when returned within info.
> > >
> >
> > Could you post an example, I'm not sure I follow.
> >
>
> The gpioline_info_v2 now looks like this:
>
> /**
> * 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
> * @config: the configuration of the line. Note that the values field is
> * always zeroed.
> * @offset: the local offset on this GPIO device, fill this in when
> * requesting the line information from the kernel
> * @padding: reserved for future use
> */
> struct gpioline_info_v2 {
> char name[GPIO_MAX_NAME_SIZE];
> char consumer[GPIO_MAX_NAME_SIZE];
> struct gpioline_config config;
> __u32 offset;
> __u32 padding[GPIOLINE_INFO_V2_PAD_SIZE]; /* for future use */
> };
>
> Previously that had all the fields from config - other than the values.
>
> When that is populated the config.values will always be zeroed.
>
We'll probably abstract this away in libgpiod and your Go library but
for someone looking at the ABI it may be confusing because a zeroed
values array is still valid. I don't have a better idea though.
[snip!]
>
> > > > > And would it be useful for userspace to be able to influence the size of
> > > > > the event buffer (currently fixed at 16 events per line)?
> > > > >
> > > >
> > > > Good question. I would prefer to not overdo it though. The event
> > > > request would need to contain the desired kfifo size and we'd only
> > > > allow to set it on request, right?
> > > >
> > >
> > > Yeah, it would only be relevant if edge detection was set and, as per
> > > edge detection itself, would only be settable via the request, not
> > > via set_config. It would only be a suggestion, as the kfifo size gets
> > > rounded up to a power of 2 anyway. It would be capped - I'm open to
> > > suggestions for a suitable max value. And the 0 value would mean use
> > > the default - currently 16 per line.
> > >
> >
> > This sounds good. How about 512 for max value for now and we can
> > always increase it if needed. I don't think we should explicitly cap
> > it though - let the user specify any value and just silently limit it
> > to 512 in the kernel.
> >
>
> It will be an internal cap only - no error if the user requests more.
> I was thinking 1024, which corresponds to the maximum default - 16*64.
>
Yes, this sounds good too.
Bart
On Tue, Jun 09, 2020 at 10:03:42AM +0200, Bartosz Golaszewski wrote:
> sob., 6 cze 2020 o 03:56 Kent Gibson <[email protected]> napisał(a):
> >
>
> [snip!]
>
> > >
> > > I'd say yes - consolidation and reuse of data structures is always
> > > good and normally they are going to be wrapped in some kind of
> > > low-level user-space library anyway.
> > >
> >
> > Ok, and I've changed the values field name to bitmap, along with the change
> > to a bitmap type, so the stuttering is gone.
> >
> > And, as the change to bitmap substantially reduced the size of
> > gpioline_config, I now embed that in the gpioline_info instead of
> > duplicating all the other fields. The values field will be zeroed
> > when returned within info.
> >
>
> Could you post an example, I'm not sure I follow.
>
The gpioline_info_v2 now looks like this:
/**
* 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
* @config: the configuration of the line. Note that the values field is
* always zeroed.
* @offset: the local offset on this GPIO device, fill this in when
* requesting the line information from the kernel
* @padding: reserved for future use
*/
struct gpioline_info_v2 {
char name[GPIO_MAX_NAME_SIZE];
char consumer[GPIO_MAX_NAME_SIZE];
struct gpioline_config config;
__u32 offset;
__u32 padding[GPIOLINE_INFO_V2_PAD_SIZE]; /* for future use */
};
Previously that had all the fields from config - other than the values.
When that is populated the config.values will always be zeroed.
[snip!]
>
> > > >
> > > > I'm also kicking around the idea of adding sequence numbers to events,
> > > > one per line and one per handle, so userspace can more easily detect
> > > > mis-ordering or buffer overflows. Does that make any sense?
> > > >
> > >
> > > Hmm, now that you mention it - and in the light of the recent post by
> > > Ryan Lovelett about polling precision - I think it makes sense to have
> > > this. Especially since it's very easy to add.
> > >
> >
> > OK. I was only thinking about the edge events, but you might want it
> > for your line info events on the chip fd as well?
> >
>
> I don't see the need for it now, but you never know. Let's leave it
> out for now and if we ever need it - we now have the appropriate
> padding.
>
OK. It is a trivial change - I've already got the patch for it.
> > > > And would it be useful for userspace to be able to influence the size of
> > > > the event buffer (currently fixed at 16 events per line)?
> > > >
> > >
> > > Good question. I would prefer to not overdo it though. The event
> > > request would need to contain the desired kfifo size and we'd only
> > > allow to set it on request, right?
> > >
> >
> > Yeah, it would only be relevant if edge detection was set and, as per
> > edge detection itself, would only be settable via the request, not
> > via set_config. It would only be a suggestion, as the kfifo size gets
> > rounded up to a power of 2 anyway. It would be capped - I'm open to
> > suggestions for a suitable max value. And the 0 value would mean use
> > the default - currently 16 per line.
> >
>
> This sounds good. How about 512 for max value for now and we can
> always increase it if needed. I don't think we should explicitly cap
> it though - let the user specify any value and just silently limit it
> to 512 in the kernel.
>
It will be an internal cap only - no error if the user requests more.
I was thinking 1024, which corresponds to the maximum default - 16*64.
> > If you want the equivalent for the info watch then I'm not sure where to
> > hook it in. It should be at the chip scope, and there isn't any
> > suitable ioctl to hook it into so it would need a new one - maybe a
> > set_config for the chip? But the buffer size would only be settable up
> > until you add a watch.
> >
>
> I don't think we need this. Status changes are naturally much less
> frequent and the potential for buffer overflow is miniscule here.
>
Agreed.
Cheers,
Kent.