gpio: cdev: add uAPI V2

Add a new version of the uAPI to address existing 32/64-bit alignment issues, add support for debounce and event sequence numbers, and provide some future proofing by adding padding reserved for future use.

The alignment issue relates to the gpioevent_data, which packs to different sizes on 32-bit and 64-bit platforms. That creates problems for 32-bit apps running on 64-bit kernels. The patch addresses that particular issue, and the problem more generally, by adding pad fields that explicitly pad structs out to 64-bit boundaries, so they will pack to the same size now, and even if some of the reserved padding is used for __u64 fields in the future.

The 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, info, info_changed and event structs receive updated versions, and the first three new ioctls.

Signed-off-by Kent Gibson <[email protected]>

---

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.

I've also merged the handle and event requests into a single request, the line request, as the two requests were mostly the same, other than the edge detection provided by event requests. As a byproduct, the v2 uAPI allows for multiple lines producing edge events on the same line handle. This is a new capability as v1 only supports a single line in an event request.

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.

Additionally, v1 GPIOHANDLES_MAX becomes GPIOLINES_MAX in v2 for clarity, and the gpiohandle_data __u8 array becomes a bitmap gpioline_values.

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

Changes since v1:
- lower case V1 and V2, except in capitalized names
- hyphenate 32/64-bit
- rename bitmap field to bits
- drop PAD_SIZE consts in favour of hard coded numbers
- sort includes
- change config flags to __u64
- increase padding of gpioline_event
- relocate GPIOLINE_CHANGED enum into v2 section (is common with v1)
- rework config to collapse direction, drive, bias and edge enums back into flags and add optional attributes that can be associated with a subset of the requested lines.

Changes since the RFC:
- document the constraints on array sizes to maintain 32/64 alignment
- add sequence numbers to gpioline_event
- use bitmap for values instead of array of __u8
- gpioline_info_v2 contains gpioline_config instead of its composite fields
- provide constants for all array sizes, especially padding
- renamed "GPIOLINE_FLAG_V2_KERNEL" to "GPIOLINE_FLAG_V2_USED"
- renamed "default_values" to "values"
- made gpioline_direction zero based
- document clock used in gpioline_event timestamp
- add event_buffer_size to gpioline_request
- rename debounce to debounce_period
- rename lines to num_lines

 include/uapi/linux/gpio.h | 284 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 270 insertions(+), 14 deletions(-)

include/uapi/linux/gpio.h +3 -0

index 285cc10355b2..3f6db33014f0 100644
@@ -12,10 +12,13 @@
 #define _UAPI_GPIO_H_
 
 #include <linux/ioctl.h>
+#include <linux/kernel.h>
 #include <linux/types.h>
 
 /*
  * The maximum size of name and label arrays.
+ *
+ * Must be a multiple of 8 to ensure 32/64-bit alignment of structs.
  */
 #define GPIO_MAX_NAME_SIZE 32
 

@@ -32,6 +35,251 @@ struct gpiochip_info {
        __u32 lines;
 };
 

include/uapi/linux/gpio.h +18 -0

+/**
+ * enum gpioline_flag_v2 - &struct gpioline_attribute.flags values
+ */
+enum gpioline_flag_v2 {
+       GPIOLINE_FLAG_V2_USED                   = 1UL << 0, /* line is not available for request */
+       GPIOLINE_FLAG_V2_ACTIVE_LOW             = 1UL << 1,
+       GPIOLINE_FLAG_V2_INPUT                  = 1UL << 2,
+       GPIOLINE_FLAG_V2_OUTPUT                 = 1UL << 3,
+       GPIOLINE_FLAG_V2_EDGE_RISING            = 1UL << 4,
+       GPIOLINE_FLAG_V2_EDGE_FALLING           = 1UL << 5,
+       GPIOLINE_FLAG_V2_OPEN_DRAIN             = 1UL << 6,
+       GPIOLINE_FLAG_V2_OPEN_SOURCE            = 1UL << 7,
+       GPIOLINE_FLAG_V2_BIAS_PULL_UP           = 1UL << 8,
+       GPIOLINE_FLAG_V2_BIAS_PULL_DOWN         = 1UL << 9,
+       GPIOLINE_FLAG_V2_BIAS_DISABLED          = 1UL << 10,
+};
+
+/**

include/uapi/linux/gpio.h +35 -0

+/**
+ * struct gpioline_set_values - Values to set a group of GPIO lines
+ * @mask: a bitmap identifying the lines to set.
+ * @bits: a bitmap containing the value of the lines, set to 1 for active
+ * and 0 for inactive.  Note that this is the logical value, which will be
+ * the opposite of the physical value if the line is configured as active
+ * low.
+ */
+struct gpioline_set_values {
+       __u64 mask[GPIOLINES_BITMAP_SIZE];
+       __u64 bits[GPIOLINES_BITMAP_SIZE];
+};
+
+/**
+ * enum gpioline_attr_id - &struct gpioline_attribute.id values
+ */
+enum gpioline_attr_id {
+       GPIOLINE_ATTR_ID_FLAGS                  = 1,
+       GPIOLINE_ATTR_ID_OUTPUT_VALUES          = 2,
+       GPIOLINE_ATTR_ID_DEBOUNCE               = 3,
+};
+
+/**
+ * struct gpioline_attribute - a configurable attribute of a line
+ * @id: attribute identifier with value from &enum gpioline_attr_id
+ * @padding: reserved for future use and must be zero filled
+ * @flags: if id is GPIOLINE_ATTR_ID_FLAGS, the flags for the GPIO line,
+ * with values from enum gpioline_flag_v2, such as
+ * GPIOLINE_FLAG_V2_ACTIVE_LOW, GPIOLINE_FLAG_V2_OUTPUT etc, OR:ed
+ * together.  This overrides the default flags contained in the &struct
+ * gpioline_config for the associated line.
+ * @values: if id is GPIOLINE_ATTR_ID_OUTPUT_VALUES, the values to which
+ * the lines will be set
+ * @debounce_period: if id is GPIOLINE_ATTR_ID_DEBOUNCE, the desired
+ * debounce period, in microseconds

include/uapi/linux/gpio.h +9 -0

+
+/**
+ * struct gpioline_config_attribute - a configuration attribute associated
+ * with one or more of the requested lines.
+ * @mask: a bitmap identifying the lines to which the attribute applies
+ * @attr: the configurable attribute
+ */
+struct gpioline_config_attribute {
+       __u64 mask[GPIOLINES_BITMAP_SIZE];

include/uapi/linux/gpio.h +159 -15

+ * @num_attrs: the number of attributes in attrs
+ * @padding: reserved for future use and must be zero filled
+ * @attrs: the configuration attributes associated with the requested
+ * lines.
+ */
+struct gpioline_config {
+       __u64 flags;
+       __u32 num_attrs;
+       /*
+        * Pad to fill implicit padding and provide space for future use.
+        */
+       __u32 padding[5];
+       struct gpioline_config_attribute attrs[GPIOLINE_NUM_ATTRS_MAX];
+};
+
+/**
+ * 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 lines.
+ * @num_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
+ * @event_buffer_size: a suggested minimum number of line events that the
+ * kernel should buffer.  This is only relevant if edge detection is
+ * enabled in the configuration. Note that this is only a suggested value
+ * and the kernel may allocate a larger buffer or cap the size of the
+ * buffer. If this field is zero then the buffer size defaults to a minimum
+ * of num_lines*16.
+ * @padding: reserved for future use and must be zero filled
+ * @fd: if successful this field will contain a valid anonymous file handle
+ * after a GPIO_GET_LINE_IOCTL operation, zero or negative value means
+ * error
+ */
+struct gpioline_request {
+       __u32 offsets[GPIOLINES_MAX];
+       char consumer[GPIO_MAX_NAME_SIZE];
+       struct gpioline_config config;
+       __u32 num_lines;
+       __u32 event_buffer_size;
+       /*
+        * Pad struct to 64-bit boundary and provide space for future use.
+        */
+       __u32 padding[5];
+       __s32 fd;
+};
+
+/**
+ * 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
+ * @flags: flags for the GPIO line, such as GPIOLINE_FLAG_V2_ACTIVE_LOW,
+ * GPIOLINE_FLAG_V2_OUTPUT etc, OR:ed together
+ * @offset: the local offset on this GPIO device, fill this in when
+ * requesting the line information from the kernel
+ * @num_attrs: the number of attributes in attrs
+ * @attrs: the configuration attributes associated with the line.
+ * @padding: reserved for future use
+ */
+struct gpioline_info_v2 {
+       char name[GPIO_MAX_NAME_SIZE];
+       char consumer[GPIO_MAX_NAME_SIZE];
+       __u32 offset;
+       __u32 num_attrs;
+       __u64 flags;
+       struct gpioline_attribute attrs[GPIOLINE_NUM_ATTRS_MAX];
+       /*
+        * Pad struct to 64-bit boundary and provide space for future use.
+        */
+       __u32 padding[4];
+};
+
+enum gpioline_changed_type {
+       GPIOLINE_CHANGED_REQUESTED      = 1,
+       GPIOLINE_CHANGED_RELEASED       = 2,
+       GPIOLINE_CHANGED_CONFIG         = 3,
+};
+
+/**
+ * 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
+ * @event_type: the type of change with a value from enum gpioline_changed_type
+ * @padding: reserved for future use
+ */
+struct gpioline_info_changed_v2 {
+       struct gpioline_info_v2 info;
+       __u64 timestamp;
+       __u32 event_type;
+       /*
+        * Pad struct to 64-bit boundary and provide space for future use.
+        */
+       __u32 padding[5];
+};
+
+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.
+ * The timestamp is read from CLOCK_MONOTONIC and is intended to allow the
+ * accurate measurement of the time between events.  It does not provide
+ * the wall-clock time.
+ * @id: event identifier with value from enum gpioline_event_id
+ * @offset: the offset of the line that triggered the event
+ * @seqno: the sequence number for this event in the sequence of events for
+ * all the lines in this line request
+ * @line_seqno: the sequence number for this event in the sequence of
+ * events on this particular line
+ * @padding: reserved for future use
+ */
+struct gpioline_event {
+       __u64 timestamp;
+       __u32 id;
+       __u32 offset;
+       __u32 seqno;
+       __u32 line_seqno;
+       /*
+        * Pad struct to 64-bit boundary and provide space for future use.
+        */
+       __u32 padding[6];
+};
+
+/*
+ *  ABI v1
+ */
+
 /* Informational flags */
 #define GPIOLINE_FLAG_KERNEL           (1UL << 0) /* Line used by the kernel */
 #define GPIOLINE_FLAG_IS_OUT           (1UL << 1)

@@ -64,13 +312,6 @@ struct gpioline_info {
 /* Maximum number of requested handles */
 #define GPIOHANDLES_MAX 64
 
-/* Possible line status change events */
-enum {
-       GPIOLINE_CHANGED_REQUESTED = 1,
-       GPIOLINE_CHANGED_RELEASED,
-       GPIOLINE_CHANGED_CONFIG,
-};
-
 /**
  * struct gpioline_info_changed - Information about a change in status
  * of a GPIO line

@@ -149,8 +390,6 @@ struct gpiohandle_config {
        __u32 padding[4]; /* padding for future use */
 };
 
-#define GPIOHANDLE_SET_CONFIG_IOCTL _IOWR(0xB4, 0x0a, struct gpiohandle_config)
-
 /**
  * struct gpiohandle_data - Information of values on a GPIO handle
  * @values: when getting the state of lines this contains the current

@@ -161,9 +400,6 @@ struct gpiohandle_data {
        __u8 values[GPIOHANDLES_MAX];
 };
 
-#define GPIOHANDLE_GET_LINE_VALUES_IOCTL _IOWR(0xB4, 0x08, struct gpiohandle_data)
-#define GPIOHANDLE_SET_LINE_VALUES_IOCTL _IOWR(0xB4, 0x09, struct gpiohandle_data)
-
 /* Eventrequest flags */
 #define GPIOEVENT_REQUEST_RISING_EDGE  (1UL << 0)
 #define GPIOEVENT_REQUEST_FALLING_EDGE (1UL << 1)

@@ -207,11 +443,31 @@ struct gpioevent_data {
        __u32 id;
 };
 
+/*
+ * v1 and v2 ioctl()s
+ */
 #define GPIO_GET_CHIPINFO_IOCTL _IOR(0xB4, 0x01, struct gpiochip_info)
+#define GPIO_GET_LINEINFO_UNWATCH_IOCTL _IOWR(0xB4, 0x0C, __u32)
+
+/*
+ * v2 ioctl()s
+ */
+#define GPIO_GET_LINEINFO_V2_IOCTL _IOWR(0xB4, 0x05, struct gpioline_info_v2)
+#define GPIO_GET_LINEINFO_WATCH_V2_IOCTL _IOWR(0xB4, 0x06, struct gpioline_info_v2)
+#define GPIO_GET_LINE_IOCTL _IOWR(0xB4, 0x07, struct gpioline_request)
+#define GPIOLINE_SET_CONFIG_IOCTL _IOWR(0xB4, 0x0D, struct gpioline_config)
+#define GPIOLINE_GET_VALUES_IOCTL _IOWR(0xB4, 0x0E, struct gpioline_values)
+#define GPIOLINE_SET_VALUES_IOCTL _IOWR(0xB4, 0x0F, struct gpioline_set_values)
+
+/*
+ * v1 ioctl()s
+ */
 #define GPIO_GET_LINEINFO_IOCTL _IOWR(0xB4, 0x02, struct gpioline_info)
-#define GPIO_GET_LINEINFO_WATCH_IOCTL _IOWR(0xB4, 0x0b, struct gpioline_info)
-#define GPIO_GET_LINEINFO_UNWATCH_IOCTL _IOWR(0xB4, 0x0c, __u32)
 #define GPIO_GET_LINEHANDLE_IOCTL _IOWR(0xB4, 0x03, struct gpiohandle_request)
 #define GPIO_GET_LINEEVENT_IOCTL _IOWR(0xB4, 0x04, struct gpioevent_request)
+#define GPIOHANDLE_GET_LINE_VALUES_IOCTL _IOWR(0xB4, 0x08, struct gpiohandle_data)
+#define GPIOHANDLE_SET_LINE_VALUES_IOCTL _IOWR(0xB4, 0x09, struct gpiohandle_data)
+#define GPIOHANDLE_SET_CONFIG_IOCTL _IOWR(0xB4, 0x0A, struct gpiohandle_config)
+#define GPIO_GET_LINEINFO_WATCH_IOCTL _IOWR(0xB4, 0x0B, struct gpioline_info)
 
 #endif /* _UAPI_GPIO_H_ */
--

2.27.0

Quoted 20 lines · Kent Gibson

Add a new version of the uAPI to address existing 32/64-bit alignment issues, add support for debounce and event sequence numbers, and provide some future proofing by adding padding reserved for future use.

The alignment issue relates to the gpioevent_data, which packs to different sizes on 32-bit and 64-bit platforms. That creates problems for 32-bit apps running on 64-bit kernels. The patch addresses that particular issue, and the problem more generally, by adding pad fields that explicitly pad structs out to 64-bit boundaries, so they will pack to the same size now, and even if some of the reserved padding is used for __u64 fields in the future.

The 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, info, info_changed and event structs receive updated versions, and the first three new ioctls.

Quoted 2 lines

Signed-off-by Kent Gibson <[email protected]>

---

Hi Kent,

Thanks a lot for your work on this. Please see comments below.

One thing I'd change globally for better readability is to have all new symbols marked as v2 - even if they have no counterparts in v1. I know libgpiod will wrap it all anyway but I think it's still a good way to make our work in user-space easier.

Quoted 30 lines

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.

I've also merged the handle and event requests into a single request, the line request, as the two requests were mostly the same, other than the edge detection provided by event requests. As a byproduct, the v2 uAPI allows for multiple lines producing edge events on the same line handle. This is a new capability as v1 only supports a single line in an event request.

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.

Additionally, v1 GPIOHANDLES_MAX becomes GPIOLINES_MAX in v2 for clarity, and the gpiohandle_data __u8 array becomes a bitmap gpioline_values.

Quoted 3 lines

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

I think the info above is worth putting into the commit message. Especially the part about merging the two event types.

Quoted 52 lines

Changes since v1:
- lower case V1 and V2, except in capitalized names
- hyphenate 32/64-bit
- rename bitmap field to bits
- drop PAD_SIZE consts in favour of hard coded numbers
- sort includes
- change config flags to __u64
- increase padding of gpioline_event
- relocate GPIOLINE_CHANGED enum into v2 section (is common with v1)
- rework config to collapse direction, drive, bias and edge enums back into flags and add optional attributes that can be associated with a subset of the requested lines.

Changes since the RFC:
- document the constraints on array sizes to maintain 32/64 alignment
- add sequence numbers to gpioline_event
- use bitmap for values instead of array of __u8
- gpioline_info_v2 contains gpioline_config instead of its composite fields
- provide constants for all array sizes, especially padding
- renamed "GPIOLINE_FLAG_V2_KERNEL" to "GPIOLINE_FLAG_V2_USED"
- renamed "default_values" to "values"
- made gpioline_direction zero based
- document clock used in gpioline_event timestamp
- add event_buffer_size to gpioline_request
- rename debounce to debounce_period
- rename lines to num_lines

 include/uapi/linux/gpio.h | 284 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 270 insertions(+), 14 deletions(-)

include/uapi/linux/gpio.h +3 -0

index 285cc10355b2..3f6db33014f0 100644
@@ -12,10 +12,13 @@
 #define _UAPI_GPIO_H_
 
 #include <linux/ioctl.h>
+#include <linux/kernel.h>
 #include <linux/types.h>
 
 /*
  * The maximum size of name and label arrays.
+ *
+ * Must be a multiple of 8 to ensure 32/64-bit alignment of structs.
  */
 #define GPIO_MAX_NAME_SIZE 32
 

@@ -32,6 +35,251 @@ struct gpiochip_info {
        __u32 lines;
 };
 

include/uapi/linux/gpio.h struct gpiochip_info { · line 47

Quoted 10 lines

include/uapi/linux/gpio.h +10 -0

+/*
+ * Maximum number of requested lines.
+ *
+ * Must be a multiple of 8 to ensure 32/64-bit alignment of structs.
+ */
+#define GPIOLINES_MAX 64
+
+/* The number of __u64 required for a bitmap for GPIOLINES_MAX lines */
+#define GPIOLINES_BITMAP_SIZE  __KERNEL_DIV_ROUND_UP(GPIOLINES_MAX, 64)
+

In what circumstances can this be different than 1? It's worth documenting here I suppose.

include/uapi/linux/gpio.h struct gpiochip_info { · line 53

Quoted 6 lines

include/uapi/linux/gpio.h +6 -0

+/*
+ * The maximum number of configuration attributes associated with a line
+ * request.
+ */
+#define GPIOLINE_NUM_ATTRS_MAX 10
+

How did you choose this number? I mean: it's reasonable - just asking for clarification.

Quoted 18 lines

include/uapi/linux/gpio.h +18 -0

+/**
+ * enum gpioline_flag_v2 - &struct gpioline_attribute.flags values
+ */
+enum gpioline_flag_v2 {
+       GPIOLINE_FLAG_V2_USED                   = 1UL << 0, /* line is not available for request */
+       GPIOLINE_FLAG_V2_ACTIVE_LOW             = 1UL << 1,
+       GPIOLINE_FLAG_V2_INPUT                  = 1UL << 2,
+       GPIOLINE_FLAG_V2_OUTPUT                 = 1UL << 3,
+       GPIOLINE_FLAG_V2_EDGE_RISING            = 1UL << 4,
+       GPIOLINE_FLAG_V2_EDGE_FALLING           = 1UL << 5,
+       GPIOLINE_FLAG_V2_OPEN_DRAIN             = 1UL << 6,
+       GPIOLINE_FLAG_V2_OPEN_SOURCE            = 1UL << 7,
+       GPIOLINE_FLAG_V2_BIAS_PULL_UP           = 1UL << 8,
+       GPIOLINE_FLAG_V2_BIAS_PULL_DOWN         = 1UL << 9,
+       GPIOLINE_FLAG_V2_BIAS_DISABLED          = 1UL << 10,
+};
+
+/**

include/uapi/linux/gpio.h struct gpiochip_info { · line 81

Quoted 10 lines

include/uapi/linux/gpio.h +10 -0

+ * struct gpioline_values - Values of GPIO lines
+ * @bits: a bitmap containing the value of the lines, set to 1 for active
+ * and 0 for inactive.  Note that this is the logical value, which will be
+ * the opposite of the physical value if the line is configured as active
+ * low.
+ */
+struct gpioline_values {
+       __u64 bits[GPIOLINES_BITMAP_SIZE];
+};
+

We can set values only for a subset of requested lines but AFAICT we can't read values of only a subset of lines. Would it be difficult to remove this limitation? While reading values always succeeds - even if the line is in input mode and has edge detected - I think that someone may want to request the max number of lines without reading all their values each time. Maybe consider merging this with struct gpioline_set_values?

Quoted 42 lines

include/uapi/linux/gpio.h +42 -0

+/**
+ * struct gpioline_set_values - Values to set a group of GPIO lines
+ * @mask: a bitmap identifying the lines to set.
+ * @bits: a bitmap containing the value of the lines, set to 1 for active
+ * and 0 for inactive.  Note that this is the logical value, which will be
+ * the opposite of the physical value if the line is configured as active
+ * low.
+ */
+struct gpioline_set_values {
+       __u64 mask[GPIOLINES_BITMAP_SIZE];
+       __u64 bits[GPIOLINES_BITMAP_SIZE];
+};
+
+/**
+ * enum gpioline_attr_id - &struct gpioline_attribute.id values
+ */
+enum gpioline_attr_id {
+       GPIOLINE_ATTR_ID_FLAGS                  = 1,
+       GPIOLINE_ATTR_ID_OUTPUT_VALUES          = 2,
+       GPIOLINE_ATTR_ID_DEBOUNCE               = 3,
+};
+
+/**
+ * struct gpioline_attribute - a configurable attribute of a line
+ * @id: attribute identifier with value from &enum gpioline_attr_id
+ * @padding: reserved for future use and must be zero filled
+ * @flags: if id is GPIOLINE_ATTR_ID_FLAGS, the flags for the GPIO line,
+ * with values from enum gpioline_flag_v2, such as
+ * GPIOLINE_FLAG_V2_ACTIVE_LOW, GPIOLINE_FLAG_V2_OUTPUT etc, OR:ed
+ * together.  This overrides the default flags contained in the &struct
+ * gpioline_config for the associated line.
+ * @values: if id is GPIOLINE_ATTR_ID_OUTPUT_VALUES, the values to which
+ * the lines will be set
+ * @debounce_period: if id is GPIOLINE_ATTR_ID_DEBOUNCE, the desired
+ * debounce period, in microseconds
+ */
+struct gpioline_attribute {
+       __u32 id;
+       __u32 padding;
+       union {
+               __u64 flags;
+               struct gpioline_values values;

include/uapi/linux/gpio.h struct gpiochip_info { · line 126

Quoted 3 lines

include/uapi/linux/gpio.h +3 -0

+               __u32 debounce_period;
+       };
+};

I'm afraid that if we don't have enough padding here (at the end), we'll end up wanting to add a new attribute at some point whose argument won't fit. Maybe have a specific field in the union that's even larger than __u64?

Quoted 9 lines

include/uapi/linux/gpio.h +9 -0

+
+/**
+ * struct gpioline_config_attribute - a configuration attribute associated
+ * with one or more of the requested lines.
+ * @mask: a bitmap identifying the lines to which the attribute applies
+ * @attr: the configurable attribute
+ */
+struct gpioline_config_attribute {
+       __u64 mask[GPIOLINES_BITMAP_SIZE];

include/uapi/linux/gpio.h struct gpiochip_info { · line 145

Quoted 10 lines

include/uapi/linux/gpio.h +10 -0

+       struct gpioline_attribute attr;
+};
+
+/**
+ * struct gpioline_config - Configuration for GPIO lines
+ * @flags: flags for the GPIO lines, with values from enum
+ * gpioline_flag_v2, such as GPIOLINE_FLAG_V2_ACTIVE_LOW,
+ * GPIOLINE_FLAG_V2_OUTPUT etc, OR:ed together.  This is the default for
+ * all requested lines but may be overridden for particular lines using
+ * attrs.

So I'm having a hard time with this. I understand that the thinking behind it was: use the flags field to set all lines to INPUT by

default and only set certain lines to OUTPUT with attrs. This would make life easier for user-space but it complicates the kernel code and I also believe that any such simplification should be handled by user-space libraries, not be exposed by kernel uAPI. My personal preference would be to drop the flags field and only handle attributes (maybe even define a special macro to set all bits in mask - GPIOLINE_CONFIG_ALL_LINES or something) on a first-in-wins basis. I'm open to other suggestions though.

Quoted 15 lines

include/uapi/linux/gpio.h struct gpiochip_info { · line 47

Quoted 11 lines

include/uapi/linux/gpio.h +10 -0

+/*
+ * Maximum number of requested lines.
+ *
+ * Must be a multiple of 8 to ensure 32/64-bit alignment of structs.
+ */
+#define GPIOLINES_MAX 64
+
+/* The number of __u64 required for a bitmap for GPIOLINES_MAX lines */
+#define GPIOLINES_BITMAP_SIZE  __KERNEL_DIV_ROUND_UP(GPIOLINES_MAX, 64)
+

In what circumstances can this be different than 1? It's worth documenting here I suppose.

In terms of the API definition, GPIOLINES_MAX can be anything you want and the definitions are still valid. In practice in the mainline kernel it would always be 1 for ABI compatibility.

Chiselling GPIOLINES_MAX <= 64 into stone could simplify things a bit, as all the bitmaps reduce to a single __u64. Would you prefer that?

Quoted 10 lines

include/uapi/linux/gpio.h struct gpiochip_info { · line 53

Quoted 6 lines

include/uapi/linux/gpio.h +6 -0

+/*
+ * The maximum number of configuration attributes associated with a line
+ * request.
+ */
+#define GPIOLINE_NUM_ATTRS_MAX 10
+

How did you choose this number? I mean: it's reasonable - just asking for clarification.

I didn't want to constrain the possible configurations by making it too small, particularly allowing for future attributes, but wanted to keep the request size down so it can still comfortably fit on the stack. The gpioline_request stands at 592 bytes, which is already substantially larger than the 364 bytes of the v1 request, and each additional config attribute slot adds another 24 bytes.

So 10 seemed like a happy medium.

Quoted 29 lines

Quoted 18 lines

include/uapi/linux/gpio.h +18 -0

+/**
+ * enum gpioline_flag_v2 - &struct gpioline_attribute.flags values
+ */
+enum gpioline_flag_v2 {
+       GPIOLINE_FLAG_V2_USED                   = 1UL << 0, /* line is not available for request */
+       GPIOLINE_FLAG_V2_ACTIVE_LOW             = 1UL << 1,
+       GPIOLINE_FLAG_V2_INPUT                  = 1UL << 2,
+       GPIOLINE_FLAG_V2_OUTPUT                 = 1UL << 3,
+       GPIOLINE_FLAG_V2_EDGE_RISING            = 1UL << 4,
+       GPIOLINE_FLAG_V2_EDGE_FALLING           = 1UL << 5,
+       GPIOLINE_FLAG_V2_OPEN_DRAIN             = 1UL << 6,
+       GPIOLINE_FLAG_V2_OPEN_SOURCE            = 1UL << 7,
+       GPIOLINE_FLAG_V2_BIAS_PULL_UP           = 1UL << 8,
+       GPIOLINE_FLAG_V2_BIAS_PULL_DOWN         = 1UL << 9,
+       GPIOLINE_FLAG_V2_BIAS_DISABLED          = 1UL << 10,
+};
+
+/**

include/uapi/linux/gpio.h struct gpiochip_info { · line 81

Quoted 10 lines

include/uapi/linux/gpio.h +10 -0

+ * struct gpioline_values - Values of GPIO lines
+ * @bits: a bitmap containing the value of the lines, set to 1 for active
+ * and 0 for inactive.  Note that this is the logical value, which will be
+ * the opposite of the physical value if the line is configured as active
+ * low.
+ */
+struct gpioline_values {
+       __u64 bits[GPIOLINES_BITMAP_SIZE];
+};
+

Quoted 8 lines

We can set values only for a subset of requested lines but AFAICT we can't read values of only a subset of lines. Would it be difficult to remove this limitation? While reading values always succeeds - even if the line is in input mode and has edge detected - I think that someone may want to request the max number of lines without reading all their values each time. Maybe consider merging this with struct gpioline_set_values?

That is correct.

I considered that corner case to be unlikely, as a major point of requesting lines together is to be able to perform collective operations on them as atomically as possible. If you only want subsets then request them as separate subsets.

Do you have a case in mind where you would have overlapping subsets?

Not difficult to remove the limitation - I just didn't see sufficient benefit.

Quoted 42 lines

Quoted 42 lines

include/uapi/linux/gpio.h +42 -0

+/**
+ * struct gpioline_set_values - Values to set a group of GPIO lines
+ * @mask: a bitmap identifying the lines to set.
+ * @bits: a bitmap containing the value of the lines, set to 1 for active
+ * and 0 for inactive.  Note that this is the logical value, which will be
+ * the opposite of the physical value if the line is configured as active
+ * low.
+ */
+struct gpioline_set_values {
+       __u64 mask[GPIOLINES_BITMAP_SIZE];
+       __u64 bits[GPIOLINES_BITMAP_SIZE];
+};
+
+/**
+ * enum gpioline_attr_id - &struct gpioline_attribute.id values
+ */
+enum gpioline_attr_id {
+       GPIOLINE_ATTR_ID_FLAGS                  = 1,
+       GPIOLINE_ATTR_ID_OUTPUT_VALUES          = 2,
+       GPIOLINE_ATTR_ID_DEBOUNCE               = 3,
+};
+
+/**
+ * struct gpioline_attribute - a configurable attribute of a line
+ * @id: attribute identifier with value from &enum gpioline_attr_id
+ * @padding: reserved for future use and must be zero filled
+ * @flags: if id is GPIOLINE_ATTR_ID_FLAGS, the flags for the GPIO line,
+ * with values from enum gpioline_flag_v2, such as
+ * GPIOLINE_FLAG_V2_ACTIVE_LOW, GPIOLINE_FLAG_V2_OUTPUT etc, OR:ed
+ * together.  This overrides the default flags contained in the &struct
+ * gpioline_config for the associated line.
+ * @values: if id is GPIOLINE_ATTR_ID_OUTPUT_VALUES, the values to which
+ * the lines will be set
+ * @debounce_period: if id is GPIOLINE_ATTR_ID_DEBOUNCE, the desired
+ * debounce period, in microseconds
+ */
+struct gpioline_attribute {
+       __u32 id;
+       __u32 padding;
+       union {
+               __u64 flags;
+               struct gpioline_values values;

Quoted 9 lines

include/uapi/linux/gpio.h struct gpiochip_info { · line 126

Quoted 3 lines

include/uapi/linux/gpio.h +3 -0

+               __u32 debounce_period;
+       };
+};

I'm afraid that if we don't have enough padding here (at the end), we'll end up wanting to add a new attribute at some point whose argument won't fit. Maybe have a specific field in the union that's even larger than __u64?

I'm satisfied with the 64-bit value restriction.

I don't want to go adding another 8 bytes of pad per attribute on the off chance that we ever find such an attribute, and that we couldn't find some other solution like using the __u32 padding, or user the gpioline_config padding, or split it over two attributes....

Quoted 22 lines

Quoted 9 lines

include/uapi/linux/gpio.h +9 -0

+
+/**
+ * struct gpioline_config_attribute - a configuration attribute associated
+ * with one or more of the requested lines.
+ * @mask: a bitmap identifying the lines to which the attribute applies
+ * @attr: the configurable attribute
+ */
+struct gpioline_config_attribute {
+       __u64 mask[GPIOLINES_BITMAP_SIZE];

include/uapi/linux/gpio.h struct gpiochip_info { · line 145

Quoted 10 lines

include/uapi/linux/gpio.h +10 -0

+       struct gpioline_attribute attr;
+};
+
+/**
+ * struct gpioline_config - Configuration for GPIO lines
+ * @flags: flags for the GPIO lines, with values from enum
+ * gpioline_flag_v2, such as GPIOLINE_FLAG_V2_ACTIVE_LOW,
+ * GPIOLINE_FLAG_V2_OUTPUT etc, OR:ed together.  This is the default for
+ * all requested lines but may be overridden for particular lines using
+ * attrs.

So I'm having a hard time with this. I understand that the thinking behind it was: use the flags field to set all lines to INPUT by

Quoted 9 lines

default and only set certain lines to OUTPUT with attrs. This would make life easier for user-space but it complicates the kernel code and I also believe that any such simplification should be handled by user-space libraries, not be exposed by kernel uAPI. My personal preference would be to drop the flags field and only handle attributes (maybe even define a special macro to set all bits in mask - GPIOLINE_CONFIG_ALL_LINES or something) on a first-in-wins basis. I'm open to other suggestions though.

I think I've addressed this elsewhere, and still think it is worthwhile and very low cost. I thought it was an easy win when I added it, and still do.

Happy to change the attrs to first-in-wins though - the validation of the attrs is still my biggest bugbear with this version.

Quoted 1 line · Kent Gibson

[snip]

Quoted 16 lines

Quoted 15 lines

include/uapi/linux/gpio.h struct gpiochip_info { · line 47

Quoted 11 lines

include/uapi/linux/gpio.h +10 -0

+/*
+ * Maximum number of requested lines.
+ *
+ * Must be a multiple of 8 to ensure 32/64-bit alignment of structs.
+ */
+#define GPIOLINES_MAX 64
+
+/* The number of __u64 required for a bitmap for GPIOLINES_MAX lines */
+#define GPIOLINES_BITMAP_SIZE  __KERNEL_DIV_ROUND_UP(GPIOLINES_MAX, 64)
+

In what circumstances can this be different than 1? It's worth documenting here I suppose.

Quoted 7 lines

In terms of the API definition, GPIOLINES_MAX can be anything you want and the definitions are still valid. In practice in the mainline kernel it would always be 1 for ABI compatibility.

Chiselling GPIOLINES_MAX <= 64 into stone could simplify things a bit, as all the bitmaps reduce to a single __u64. Would you prefer that?

I'm not sure I follow. We need to chisel some max value in stone. Up to that point it's been 64. We can make it more and the bitmap API would handle it alright but if we don't, then this __KERNEL_DIV_ROUND_UP() is unnecessary. Limiting it to 64 makes things very simple thanks to fitting into a __u64 though. I've personally never needed to request even half that so I guess this value's fine?

Quoted 15 lines

Quoted 4 lines

include/uapi/linux/gpio.h struct gpiochip_info { · line 81

Quoted 3 lines

include/uapi/linux/gpio.h +3 -0

+       __u64 bits[GPIOLINES_BITMAP_SIZE];
+};
+

Quoted 8 lines

We can set values only for a subset of requested lines but AFAICT we can't read values of only a subset of lines. Would it be difficult to remove this limitation? While reading values always succeeds - even if the line is in input mode and has edge detected - I think that someone may want to request the max number of lines without reading all their values each time. Maybe consider merging this with struct gpioline_set_values?

That is correct.

Quoted 5 lines

I considered that corner case to be unlikely, as a major point of requesting lines together is to be able to perform collective operations on them as atomically as possible. If you only want subsets then request them as separate subsets.

And yet this version implements heterogeneous config and setting edge detection and values of subsets of requested lines. :)

Quoted 2 lines

Do you have a case in mind where you would have overlapping subsets?

No, not really but then I also don't have a use-case for setting only a certain subset of lines.

Quoted 3 lines

Not difficult to remove the limitation - I just didn't see sufficient benefit.

Using the same structure for setting and getting values is a benefit IMO. If it's not a difficult task, then I think it's worth adding it.

Quoted 9 lines

Quoted 4 lines

(maybe even define a special macro to set all bits in mask - GPIOLINE_CONFIG_ALL_LINES or something) on a first-in-wins basis. I'm open to other suggestions though.

I think I've addressed this elsewhere, and still think it is worthwhile and very low cost. I thought it was an easy win when I added it, and still do.

Quoted 2 lines

Happy to change the attrs to first-in-wins though - the validation of the attrs is still my biggest bugbear with this version.

Yes, I read your other reply. Ok, makes sense to have default flags with an attribute for overrides. This just needs very explicit documentation.

Quoted 1 line · Bartosz Golaszewski

On Sat, Jul 25, 2020 at 6:20 AM Kent Gibson <[email protected]> wrote:

Quoted 10 lines

Quoted 5 lines

config, info, info_changed and event structs receive updated versions, and the first three new ioctls.

Signed-off-by Kent Gibson <[email protected]>

---

Hi Kent,

Thanks a lot for your work on this. Please see comments below.

Quoted 5 lines

One thing I'd change globally for better readability is to have all new symbols marked as v2 - even if they have no counterparts in v1. I know libgpiod will wrap it all anyway but I think it's still a good way to make our work in user-space easier.

Fair enough. Oh joy.

Quoted 2 lines · Kent Gibson

Quoted 1 line · Bartosz Golaszewski

On Sat, Jul 25, 2020 at 6:20 AM Kent Gibson <[email protected]> wrote:

[snip]

Quoted 10 lines

Quoted 10 lines

Quoted 5 lines

config, info, info_changed and event structs receive updated versions, and the first three new ioctls.

Signed-off-by Kent Gibson <[email protected]>

---

Hi Kent,

Thanks a lot for your work on this. Please see comments below.

Quoted 8 lines

Quoted 5 lines

One thing I'd change globally for better readability is to have all new symbols marked as v2 - even if they have no counterparts in v1. I know libgpiod will wrap it all anyway but I think it's still a good way to make our work in user-space easier.

Fair enough. Oh joy.

Given that the intent is to highlight that the symbols are related to the v2 of the GPIO uAPI, and not the second version of a particular type, it makes more sense to me that the v2 is placed adjacent to the GPIO in the name. e.g. gpioline_flag_v2 would become gpiov2line_flag.

Does that work for you?

Make the gpiolib-cdev module a build option. This allows the CDEV interface to be removed from the kernel to reduce kernel size in applications where is it not required, and provides the parent for other other CDEV interface specific build options to follow.

Suggested-by Bartosz Golaszewski <[email protected]>

Signed-off-by Kent Gibson <[email protected]>

---
 drivers/gpio/Kconfig        | 16 ++++++++++++++--
 drivers/gpio/Makefile       |  2 +-
 drivers/gpio/gpiolib-cdev.h | 15 +++++++++++++++
 3 files changed, 30 insertions(+), 3 deletions(-)

drivers/gpio/Kconfig +1 -2

index 8030fd91a3cc..b5bb9efc1092 100644
@@ -66,8 +66,20 @@ config GPIO_SYSFS
 
          This ABI is deprecated. If you want to use GPIO from userspace,
          use the character device /dev/gpiochipN with the appropriate
-         ioctl() operations instead. The character device is always
-         available.
+         ioctl() operations instead.

drivers/gpio/Kconfig +3 -0

+         required.
+
+         If unsure, say Y.
 
 config GPIO_GENERIC
        depends on HAS_IOMEM # Only for IOMEM drivers

drivers/gpio/Makefile +1 -1

index 4f9abff4f2dc..7c24c8d77068 100644
@@ -7,8 +7,8 @@ obj-$(CONFIG_GPIOLIB)           += gpiolib.o
 obj-$(CONFIG_GPIOLIB)          += gpiolib-devres.o
 obj-$(CONFIG_GPIOLIB)          += gpiolib-legacy.o
 obj-$(CONFIG_GPIOLIB)          += gpiolib-devprop.o
-obj-$(CONFIG_GPIOLIB)          += gpiolib-cdev.o
 obj-$(CONFIG_OF_GPIO)          += gpiolib-of.o
+obj-$(CONFIG_GPIO_CDEV)                += gpiolib-cdev.o
 obj-$(CONFIG_GPIO_SYSFS)       += gpiolib-sysfs.o
 obj-$(CONFIG_GPIO_ACPI)                += gpiolib-acpi.o
 

drivers/gpio/gpiolib-cdev.h +15 -1

index 973578e7ad10..19a4e3d57120 100644
@@ -5,7 +5,22 @@
 
 #include <linux/device.h>
 
+#ifdef CONFIG_GPIO_CDEV
+
 int gpiolib_cdev_register(struct gpio_device *gdev, dev_t devt);
 void gpiolib_cdev_unregister(struct gpio_device *gdev);
 
+#else
+
+static inline int gpiolib_cdev_register(struct gpio_device *gdev, dev_t devt)
+{
+       return 0;
+}
+
+static inline void gpiolib_cdev_unregister(struct gpio_device *gdev)
+{
+}
+
+#endif /* CONFIG_GPIO_CDEV */
+
 #endif /* GPIOLIB_CDEV_H */
--

2.27.0

drivers/gpio/Kconfig config GPIO_SYSFS · line 73

Quoted 3 lines · Kent Gibson

drivers/gpio/Kconfig +3 -0

+config GPIO_CDEV
+       bool "/dev/gpiochipN (character device interface)"
+       default y

I don't want to make it too easy to do this, as I see it as a standard kernel feature.

Can we add:

depends on EXPERT

as with other standard kernel features?

Quoted 6 lines · Linus Walleij

drivers/gpio/Kconfig config GPIO_SYSFS · line 73

Quoted 3 lines · Kent Gibson

drivers/gpio/Kconfig +3 -0

+config GPIO_CDEV
+       bool "/dev/gpiochipN (character device interface)"
+       default y

Quoted 9 lines

I don't want to make it too easy to do this, as I see it as a standard kernel feature.

Can we add:

depends on EXPERT

as with other standard kernel features?

Fair enough.

But what of the GPIO_CDEV_V1 option to disable uAPI V1 added in patch 04, and that depends on GPIO_CDEV?
That is equivalent to GPIO_SYSFS, which is not dependent on EXPERT, so I'll need to restructure the dependencies so it doesn't inherit the EXPERT dependency.
Unless you also want it to be dependent on EXPERT.

Hmmm, and maybe patch 04 should be later in the series - after V2 is fully implemented and V1 is deprecated - around patch 11.

I've gone with this:

drivers/gpio/Kconfig +4 -0

+config GPIO_CDEV
+       bool
+       prompt "Character device (/dev/gpiochipN) support" if EXPERT
+       default y

so the entry is always present in menuconfig, and GPIO_CDEV_V1 can still depend on it, but GPIO_CDEV can only be disabled if EXPERT is set.

Add support for requesting lines using the GPIO_GET_LINE_IOCTL, and returning their current values using GPIOLINE_GET_VALUES_IOCTL.

drivers/gpio/gpiolib-cdev.c +2 -0

+       struct gpio_desc *descs[];
+};

+}

+}

drivers/gpio/gpiolib-cdev.c +6 -0

+                       gpiod_free(line->descs[i]);
+       }
+       kfree(line->label);
+       put_device(&line->gdev->dev);
+       kfree(line);
+}

drivers/gpio/gpiolib-cdev.c +5 -0

+               if (!line->label) {
+                       ret = -ENOMEM;
+                       goto out_free_line;
+               }
+       }

+ struct gpio_desc *desc = gpiochip_get_desc(gdev->chip, offset);

drivers/gpio/gpiolib-cdev.c +4 -0

+               if (IS_ERR(desc)) {
+                       ret = PTR_ERR(desc);
+                       goto out_free_line;
+               }

Quoted 3 lines · Kent Gibson

Add support for requesting lines using the GPIO_GET_LINE_IOCTL, and returning their current values using GPIOLINE_GET_VALUES_IOCTL.

...

drivers/gpio/gpiolib-cdev.c static int linehandle_create(struct gpio_device *gdev, void __user *ip) · line 396

Quoted 6 lines

drivers/gpio/gpiolib-cdev.c +5 -0

+struct line {
+       struct gpio_device *gdev;
+       const char *label;
+       u32 num_descs;
 
+       /* descs must be last so it can be dynamically sized */

I guess [] implies above comment and thus comment can be dropped.

Quoted 2 lines

drivers/gpio/gpiolib-cdev.c +2 -0

+       struct gpio_desc *descs[];
+};

...

drivers/gpio/gpiolib-cdev.c static int linehandle_create(struct gpio_device *gdev, void __user *ip) · line 408

Quoted 9 lines

drivers/gpio/gpiolib-cdev.c +9 -0

+static bool padding_not_zeroed(__u32 *padding, int pad_size)
+{
+       int i, sum = 0;
+
+       for (i = 0; i < pad_size; i++)
+               sum |= padding[i];
+
+       return sum;
+}

Reimplementation of memchr_inv() ?

...

drivers/gpio/gpiolib-cdev.c static int linehandle_create(struct gpio_device *gdev, void __user *ip) · line 433

Quoted 5 lines

drivers/gpio/gpiolib-cdev.c +5 -0

+static u64 gpioline_config_flags(struct gpioline_config *lc, int line_idx)
+{
+       int i;
+
+       for (i = lc->num_attrs - 1; i >= 0; i--) {

Much better to read is

unsigned int i = lc->num_attrs;

while (i--) {
...
}

drivers/gpio/gpiolib-cdev.c static int linehandle_create(struct gpio_device *gdev, void __user *ip) · line 435

Quoted 3 lines

drivers/gpio/gpiolib-cdev.c +2 -0

+               if ((lc->attrs[i].attr.id == GPIOLINE_ATTR_ID_FLAGS) &&
 
+                   test_bit(line_idx, (unsigned long *)lc->attrs[i].mask))

This casting is not good. What about BE 32-bit architecture?

drivers/gpio/gpiolib-cdev.c static int linehandle_create(struct gpio_device *gdev, void __user *ip) · line 443

Quoted 8 lines

drivers/gpio/gpiolib-cdev.c +8 -0

+                       return lc->attrs[i].attr.flags;
+       }
+       return lc->flags;
+}
+
+static int gpioline_config_output_value(struct gpioline_config *lc,
+                                       int line_idx)
+{

Same comments as per above.

Quoted 1 line

+}

...

drivers/gpio/gpiolib-cdev.c static int linehandle_create(struct gpio_device *gdev, void __user *ip) · line 543

Quoted 5 lines

drivers/gpio/gpiolib-cdev.c +4 -0

+static long line_get_values(struct line *line, void __user *ip)
+{
+       struct gpioline_values lv;
 
+       unsigned long *vals = (unsigned long *)lv.bits;

Casting u64 to unsigned long is not good.

Quoted 1 line

+}

...

Quoted 5 lines

drivers/gpio/gpiolib-cdev.c +5 -0

+static void line_free(struct line *line)
+{
+       int i;
+
+       for (i = 0; i < line->num_descs; i++) {

Quoted 1 line

+ if (line->descs[i])

Redundant?

drivers/gpio/gpiolib-cdev.c static int linehandle_create(struct gpio_device *gdev, void __user *ip) · line 594

Quoted 6 lines

drivers/gpio/gpiolib-cdev.c +6 -0

+                       gpiod_free(line->descs[i]);
+       }
+       kfree(line->label);
+       put_device(&line->gdev->dev);
+       kfree(line);
+}

...

drivers/gpio/gpiolib-cdev.c static int linehandle_create(struct gpio_device *gdev, void __user *ip) · line 647

Quoted 4 lines

drivers/gpio/gpiolib-cdev.c +4 -0

+       /* Make sure this is terminated */
+       linereq.consumer[sizeof(linereq.consumer)-1] = '\0';
+       if (strlen(linereq.consumer)) {
+               line->label = kstrdup(linereq.consumer, GFP_KERNEL);

kstrndup() ?

drivers/gpio/gpiolib-cdev.c static int linehandle_create(struct gpio_device *gdev, void __user *ip) · line 652

Quoted 5 lines

drivers/gpio/gpiolib-cdev.c +5 -0

+               if (!line->label) {
+                       ret = -ENOMEM;
+                       goto out_free_line;
+               }
+       }

...

Quoted 1 line

+ struct gpio_desc *desc = gpiochip_get_desc(gdev->chip, offset);

I prefer to see this split, but it's minor.

drivers/gpio/gpiolib-cdev.c static int linehandle_create(struct gpio_device *gdev, void __user *ip) · line 664

Quoted 4 lines

drivers/gpio/gpiolib-cdev.c +4 -0

+               if (IS_ERR(desc)) {
+                       ret = PTR_ERR(desc);
+                       goto out_free_line;
+               }

...

drivers/gpio/gpiolib-cdev.c static int linehandle_create(struct gpio_device *gdev, void __user *ip) · line 698

Quoted 2 lines

drivers/gpio/gpiolib-cdev.c +2 -0

+               dev_dbg(&gdev->dev, "registered chardev handle for line %d\n",
+                       offset);

Perhaps tracepoint / event?

Quoted 2 lines · Andy Shevchenko

Quoted 1 line · Kent Gibson

Quoted 13 lines

drivers/gpio/gpiolib-cdev.c static int linehandle_create(struct gpio_device *gdev, void __user *ip) · line 408

Quoted 10 lines

drivers/gpio/gpiolib-cdev.c +9 -0

+static bool padding_not_zeroed(__u32 *padding, int pad_size)
+{
+       int i, sum = 0;
+
+       for (i = 0; i < pad_size; i++)
+               sum |= padding[i];
+
+       return sum;
+}

Reimplementation of memchr_inv() ?

I was hoping to find an existing function, surely checking a region is zeroed is a common thing, right?, so this was a place holder as much as anything. Not sure memchr_inv fits the bill, but I'll give it a try...

Quoted 2 lines · Andy Shevchenko

Quoted 1 line · Kent Gibson

Quoted 4 lines

Quoted 1 line

+ test_bit(line_idx, (unsigned long *)lc->attrs[i].mask))

This casting is not good. What about BE 32-bit architecture?

I agree the casting is hideous, but I thought the outcome was correct as it is manipulating addresses, not data.
You think the address of a 64-bit differs based on endian?? Happy to change it - but not sure what to.

On Sun, Jul 26, 2020 at 09:12:44AM +0800, Kent Gibson wrote:

I'll rework that for v3.

Quoted 1 line · Kent Gibson

On Sun, Jul 26, 2020 at 09:12:44AM +0800, Kent Gibson wrote:

...

Quoted 1 line

I'll rework that for v3.

Please give some more time to review v2. Especially the v2 API approach.

Quoted 13 lines

drivers/gpio/gpiolib-cdev.c static int linehandle_create(struct gpio_device *gdev, void __user *ip) · line 408

Quoted 10 lines

drivers/gpio/gpiolib-cdev.c +9 -0

+static bool padding_not_zeroed(__u32 *padding, int pad_size)
+{
+       int i, sum = 0;
+
+       for (i = 0; i < pad_size; i++)
+               sum |= padding[i];
+
+       return sum;
+}

Reimplementation of memchr_inv() ?

I was hoping to find an existing function, surely checking a region is zeroed is a common thing, right?, so this was a place holder as much as anything. Not sure memchr_inv fits the bill, but I'll give it a try...

Quoted 12 lines

...

drivers/gpio/gpiolib-cdev.c static int linehandle_create(struct gpio_device *gdev, void __user *ip) · line 433

Quoted 5 lines

drivers/gpio/gpiolib-cdev.c +5 -0

+static u64 gpioline_config_flags(struct gpioline_config *lc, int line_idx)
+{
+       int i;
+
+       for (i = lc->num_attrs - 1; i >= 0; i--) {

Much better to read is

unsigned int i = lc->num_attrs;

Quoted 4 lines

while (i--) {
...
}

Really? I find that the post-decrement in the while makes determining the bounds of the loop more confusing.

Quoted 16 lines

...

Quoted 1 line

+ struct gpio_desc *desc = gpiochip_get_desc(gdev->chip, offset);

I prefer to see this split, but it's minor.

Quoted 4 lines

drivers/gpio/gpiolib-cdev.c +4 -0

+               if (IS_ERR(desc)) {
+                       ret = PTR_ERR(desc);
+                       goto out_free_line;
+               }

...

Quoted 2 lines

drivers/gpio/gpiolib-cdev.c +2 -0

+               dev_dbg(&gdev->dev, "registered chardev handle for line %d\n",
+                       offset);

Quoted 2 lines

Perhaps tracepoint / event?

Again, a cut-and-paste from V1, and I have no experience with tracepoints or events, so I have no opinion on that.

So, yeah - perhaps?

Quoted 1 line · Kent Gibson

[snip]

Quoted 14 lines

Quoted 13 lines

drivers/gpio/gpiolib-cdev.c static int linehandle_create(struct gpio_device *gdev, void __user *ip) · line 408

Quoted 10 lines

drivers/gpio/gpiolib-cdev.c +9 -0

+static bool padding_not_zeroed(__u32 *padding, int pad_size)
+{
+       int i, sum = 0;
+
+       for (i = 0; i < pad_size; i++)
+               sum |= padding[i];
+
+       return sum;
+}

Reimplementation of memchr_inv() ?

Quoted 5 lines

I was hoping to find an existing function, surely checking a region is zeroed is a common thing, right?, so this was a place holder as much as anything. Not sure memchr_inv fits the bill, but I'll give it a try...

If you don't find an appropriate function: please put your new implementation in lib/ so that others may reuse it.

Quoted 17 lines

Quoted 16 lines

...

drivers/gpio/gpiolib-cdev.c static int linehandle_create(struct gpio_device *gdev, void __user *ip) · line 433

Quoted 5 lines

drivers/gpio/gpiolib-cdev.c +5 -0

+static u64 gpioline_config_flags(struct gpioline_config *lc, int line_idx)
+{
+       int i;
+
+       for (i = lc->num_attrs - 1; i >= 0; i--) {

Much better to read is

unsigned int i = lc->num_attrs;

while (i--) {
...
}

Quoted 3 lines

Really? I find that the post-decrement in the while makes determining the bounds of the loop more confusing.

Agreed, Andy: this is too much nit-picking. :)

[snip]

Quoted 22 lines

Quoted 11 lines

...

Quoted 1 line

+ struct gpio_desc *desc = gpiochip_get_desc(gdev->chip, offset);

I prefer to see this split, but it's minor.

drivers/gpio/gpiolib-cdev.c static int linehandle_create(struct gpio_device *gdev, void __user *ip) · line 664

Quoted 4 lines

drivers/gpio/gpiolib-cdev.c +4 -0

+               if (IS_ERR(desc)) {
+                       ret = PTR_ERR(desc);
+                       goto out_free_line;
+               }

Quoted 7 lines

...

drivers/gpio/gpiolib-cdev.c static int linehandle_create(struct gpio_device *gdev, void __user *ip) · line 698

Quoted 2 lines

drivers/gpio/gpiolib-cdev.c +2 -0

+               dev_dbg(&gdev->dev, "registered chardev handle for line %d\n",
+                       offset);

Perhaps tracepoint / event?

Again, a cut-and-paste from V1, and I have no experience with tracepoints or events, so I have no opinion on that.

Quoted 2 lines

So, yeah - perhaps?

I think it's a good idea to add some proper instrumentation this time other than much less reliable logs. Can you take a look at include/trace/events/gpio.h? Adding new GPIO trace events should be pretty straightforward by copy-pasti... drawing inspiration from existing ones.

Quoted 13 lines · Bartosz Golaszewski

Quoted 12 lines · Kent Gibson

Quoted 8 lines

drivers/gpio/gpiolib-cdev.c static int linehandle_create(struct gpio_device *gdev, void __user *ip) · line 698

Quoted 5 lines

drivers/gpio/gpiolib-cdev.c +2 -0

+               dev_dbg(&gdev->dev, "registered chardev handle for line %d\n",
+                       offset);

Perhaps tracepoint / event?

Again, a cut-and-paste from V1, and I have no experience with tracepoints or events, so I have no opinion on that.

Quoted 9 lines

Quoted 2 lines

So, yeah - perhaps?

I think it's a good idea to add some proper instrumentation this time other than much less reliable logs. Can you take a look at include/trace/events/gpio.h? Adding new GPIO trace events should be pretty straightforward by copy-pasti... drawing inspiration from existing ones.

You only want tracepoints to replace those dev_dbg()s, so when a line is requested? What about the release? Any other points?

Quoted 24 lines · Kent Gibson

Quoted 13 lines · Bartosz Golaszewski

Quoted 12 lines · Kent Gibson

Quoted 8 lines

drivers/gpio/gpiolib-cdev.c static int linehandle_create(struct gpio_device *gdev, void __user *ip) · line 698

Quoted 5 lines

drivers/gpio/gpiolib-cdev.c +2 -0

+               dev_dbg(&gdev->dev, "registered chardev handle for line %d\n",
+                       offset);

Perhaps tracepoint / event?

Again, a cut-and-paste from V1, and I have no experience with tracepoints or events, so I have no opinion on that.

Quoted 9 lines

Quoted 2 lines

So, yeah - perhaps?

I think it's a good idea to add some proper instrumentation this time other than much less reliable logs. Can you take a look at include/trace/events/gpio.h? Adding new GPIO trace events should be pretty straightforward by copy-pasti... drawing inspiration from existing ones.

Quoted 3 lines

You only want tracepoints to replace those dev_dbg()s, so when a line is requested? What about the release? Any other points?

Had a closer look and it seems to me that the correct place to add such tracepoints would be gpiod_request() and gpiod_free(), so they catch all requests, not just the cdev ones. And that moves it outside the scope of this patch.

I personally don't have any use for the dev_dbg()s here and am happy to remove them - they were only there to match the behaviour of linehandle_create as closely as possible.

Quoted 1 line · Bartosz Golaszewski

On Sun, Jul 26, 2020 at 3:12 AM Kent Gibson <[email protected]> wrote:

Quoted 16 lines

Quoted 16 lines

Quoted 15 lines

drivers/gpio/gpiolib-cdev.c static int linehandle_create(struct gpio_device *gdev, void __user *ip) · line 433

Quoted 6 lines

drivers/gpio/gpiolib-cdev.c +5 -0

+static u64 gpioline_config_flags(struct gpioline_config *lc, int line_idx)
+{
+       int i;
+
+       for (i = lc->num_attrs - 1; i >= 0; i--) {

Much better to read is

unsigned int i = lc->num_attrs;

while (i--) {
...
}

Quoted 6 lines

Quoted 3 lines

Really? I find that the post-decrement in the while makes determining the bounds of the loop more confusing.

Agreed, Andy: this is too much nit-picking. :)

I was actually hoping for some feedback on the direction of that loop, as it relates to the handling of multiple instances of the same attribute associated with a given line.

The reverse loop here implements a last in wins policy, but I'm now thinking the kernel should be encouraging userspace to only associate a given attribute with a line once, and that a first in wins would help do that - as additional associations would be ignored.

Alternatively, the kernel should enforce that an attribute can only be associated once, but that would require adding more request validation.

Quoted 3 lines · Kent Gibson

Quoted 1 line · Bartosz Golaszewski

On Sun, Jul 26, 2020 at 3:12 AM Kent Gibson <[email protected]> wrote:

[snip!]

Quoted 32 lines

Quoted 16 lines

Quoted 16 lines

Quoted 15 lines

drivers/gpio/gpiolib-cdev.c static int linehandle_create(struct gpio_device *gdev, void __user *ip) · line 433

Quoted 6 lines

drivers/gpio/gpiolib-cdev.c +5 -0

+static u64 gpioline_config_flags(struct gpioline_config *lc, int line_idx)
+{
+       int i;
+
+       for (i = lc->num_attrs - 1; i >= 0; i--) {

Much better to read is

unsigned int i = lc->num_attrs;

while (i--) {
...
}

Quoted 6 lines

Quoted 3 lines

Really? I find that the post-decrement in the while makes determining the bounds of the loop more confusing.

Agreed, Andy: this is too much nit-picking. :)

I was actually hoping for some feedback on the direction of that loop, as it relates to the handling of multiple instances of the same attribute associated with a given line.

The reverse loop here implements a last in wins policy, but I'm now thinking the kernel should be encouraging userspace to only associate a given attribute with a line once, and that a first in wins would help do that - as additional associations would be ignored.

Quoted 3 lines

Alternatively, the kernel should enforce that an attribute can only be associated once, but that would require adding more request validation.

I guess this would result in a lot of churn to do validation which is largely unnecessary? To me the first in wins sounds more consistent.

Also: I just started going through the patches - nice idea with the GPIO attributes, I really like it. Although I need to give it a longer thought tomorrow - I'm wondering if we can maybe unify them and the flags.

Quoted 6 lines

Also: I just started going through the patches - nice idea with the GPIO attributes, I really like it. Although I need to give it a longer thought tomorrow - I'm wondering if we can maybe unify them and the flags.

I had an earlier draft that did just that - and that is partially why the loop is last in wins - I was using slot 0 as the default flags. But the default flags cover a lot of use cases, including all of v1, and it was simple and cheap to provide a default - and it simplified the initial port of libgpiod to v2...

Quoted 1 line · Kent Gibson

[snip]

Quoted 7 lines

Quoted 6 lines

Also: I just started going through the patches - nice idea with the GPIO attributes, I really like it. Although I need to give it a longer thought tomorrow - I'm wondering if we can maybe unify them and the flags.

Quoted 6 lines

I had an earlier draft that did just that - and that is partially why the loop is last in wins - I was using slot 0 as the default flags. But the default flags cover a lot of use cases, including all of v1, and it was simple and cheap to provide a default - and it simplified the initial port of libgpiod to v2...

If porting libgpiod to v2 is the only concern then I wouldn't stress about it. At the same time I'm wondering - is there any use-case where we wouldn't need the flags attribute for at least some lines? Because if it's always required than maybe having a default isn't that bad.