gpio: cdev: add uAPI v2

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

+/**
+ * struct gpio_v2_line_attribute - a configurable attribute of a line
+ * @id: attribute identifier with value from &enum gpio_v2_line_attr_id
+ * @padding: reserved for future use and must be zero filled
+ * @flags: if id is GPIO_V2_LINE_ATTR_ID_FLAGS, the flags for the GPIO
+ * line, with values from enum gpio_v2_line_flag, such as
+ * GPIO_V2_LINE_FLAG_ACTIVE_LOW, GPIO_V2_LINE_FLAG_OUTPUT etc, OR:ed
+ * together.  This overrides the default flags contained in the &struct
+ * gpio_v2_line_config for the associated line.
+ * @values: if id is GPIO_V2_LINE_ATTR_ID_OUTPUT_VALUES, a bitmap
+ * containing the values to which the lines will be set, with each bit
+ * number corresponding to the index into &struct
+ * gpio_v2_line_request.offsets.
+ * @debounce_period_us: if id is GPIO_V2_LINE_ATTR_ID_DEBOUNCE, the desired
+ * debounce period, in microseconds

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

+struct gpio_v2_line_request {
+       __u32 offsets[GPIO_V2_LINES_MAX];
+       char consumer[GPIO_MAX_NAME_SIZE];
+       struct gpio_v2_line_config config;
+       __u32 num_lines;
+       __u32 event_buffer_size;
+       /* Pad to fill implicit padding and reserve space for future use. */
+       __u32 padding[5];
+       __s32 fd;
+};
 

Quoted 21 lines · Kent Gibson

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

+/**
+ * struct gpio_v2_line_attribute - a configurable attribute of a line
+ * @id: attribute identifier with value from &enum gpio_v2_line_attr_id
+ * @padding: reserved for future use and must be zero filled
+ * @flags: if id is GPIO_V2_LINE_ATTR_ID_FLAGS, the flags for the GPIO
+ * line, with values from enum gpio_v2_line_flag, such as
+ * GPIO_V2_LINE_FLAG_ACTIVE_LOW, GPIO_V2_LINE_FLAG_OUTPUT etc, OR:ed
+ * together.  This overrides the default flags contained in the &struct
+ * gpio_v2_line_config for the associated line.
+ * @values: if id is GPIO_V2_LINE_ATTR_ID_OUTPUT_VALUES, a bitmap
+ * containing the values to which the lines will be set, with each bit
+ * number corresponding to the index into &struct
+ * gpio_v2_line_request.offsets.
+ * @debounce_period_us: if id is GPIO_V2_LINE_ATTR_ID_DEBOUNCE, the desired
+ * debounce period, in microseconds
+ */
+struct gpio_v2_line_attribute {
+       __u32 id;
+       __u32 padding;
+       union {
+               __aligned_u64 flags;

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

Quoted 4 lines

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

+               __aligned_u64 values;
+               __u32 debounce_period_us;
+       };
+};

Having different-sized members in the union makes it hard for something like strace to print the contents. How about just making them all __aligned_u64 even when 32 bits are sufficient?

Quoted 10 lines

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

+struct gpio_v2_line_request {
+       __u32 offsets[GPIO_V2_LINES_MAX];
+       char consumer[GPIO_MAX_NAME_SIZE];
+       struct gpio_v2_line_config config;
+       __u32 num_lines;
+       __u32 event_buffer_size;
+       /* Pad to fill implicit padding and reserve space for future use. */
+       __u32 padding[5];
+       __s32 fd;
+};

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

Quoted 10 lines

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

+struct gpio_v2_line_info {
+       char name[GPIO_MAX_NAME_SIZE];
+       char consumer[GPIO_MAX_NAME_SIZE];
+       __u32 offset;
+       __u32 num_attrs;
+       __aligned_u64 flags;
+       struct gpio_v2_line_attribute attrs[GPIO_V2_LINE_NUM_ATTRS_MAX];
+       /* Space reserved for future use. */
+       __u32 padding[4];
+};

These are both several hundred bytes long, requiring a lot of data to be copied to the stack and take up space there. I see this is not actually much different for the v1 API, but I wonder if there has been any analysis of whether this has a noticeable effect on application runtime.

Quoted 23 lines · Arnd Bergmann

Quoted 21 lines · Kent Gibson

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

+/**
+ * struct gpio_v2_line_attribute - a configurable attribute of a line
+ * @id: attribute identifier with value from &enum gpio_v2_line_attr_id
+ * @padding: reserved for future use and must be zero filled
+ * @flags: if id is GPIO_V2_LINE_ATTR_ID_FLAGS, the flags for the GPIO
+ * line, with values from enum gpio_v2_line_flag, such as
+ * GPIO_V2_LINE_FLAG_ACTIVE_LOW, GPIO_V2_LINE_FLAG_OUTPUT etc, OR:ed
+ * together.  This overrides the default flags contained in the &struct
+ * gpio_v2_line_config for the associated line.
+ * @values: if id is GPIO_V2_LINE_ATTR_ID_OUTPUT_VALUES, a bitmap
+ * containing the values to which the lines will be set, with each bit
+ * number corresponding to the index into &struct
+ * gpio_v2_line_request.offsets.
+ * @debounce_period_us: if id is GPIO_V2_LINE_ATTR_ID_DEBOUNCE, the desired
+ * debounce period, in microseconds
+ */
+struct gpio_v2_line_attribute {
+       __u32 id;
+       __u32 padding;
+       union {
+               __aligned_u64 flags;

Quoted 9 lines

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

Quoted 4 lines

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

+               __aligned_u64 values;
+               __u32 debounce_period_us;
+       };
+};

Having different-sized members in the union makes it hard for something like strace to print the contents. How about just making them all __aligned_u64 even when 32 bits are sufficient?

Ah yes, adding support for GPIO ioctl()'s to strace has been on my TODO list for 3 years now. :(

Quoted 4 lines · Arnd Bergmann

Quoted 1 line · Kent Gibson

+/**

Quoted 4 lines

Having different-sized members in the union makes it hard for something like strace to print the contents. How about just making them all __aligned_u64 even when 32 bits are sufficient?

Ah yes, adding support for GPIO ioctl()'s to strace has been on my TODO list for 3 years now. :(

Quoted 5 lines · Bartosz Golaszewski

Quoted 4 lines · Arnd Bergmann

Quoted 1 line · Kent Gibson

+/**

Quoted 8 lines

Quoted 4 lines

Having different-sized members in the union makes it hard for something like strace to print the contents. How about just making them all __aligned_u64 even when 32 bits are sufficient?

Ah yes, adding support for GPIO ioctl()'s to strace has been on my TODO list for 3 years now. :(

I'm looking at doing this as it would be useful to have.

Just starting with the v1 uAPI, as v2 isn't merged yet.

I think I've got it basically done, but I thought I'd run it past you before sending a patch to the strace list.

Successful calls currently look like this:

ioctl(3, GPIO_GET_CHIPINFO_IOCTL, {name="gpiochip1", label="gpio-mockup-B", lines=8}) = 0

ioctl(3, GPIO_GET_LINEHANDLE_IOCTL, {lines=8, lineoffsets=[3, 2, 1, 0, 4, 5, 6, 7], flags=GPIOHANDLE_REQUEST_INPUT, default_values=[0, 0, 0, 0, 0, 0, 0, 0], consumer_label="", fd=7}) = 0

ioctl(0, GPIO_GET_LINEEVENT_IOCTL, {lineoffset=2, handleflags=GPIOHANDLE_REQUEST_INPUT, eventflags=GPIOEVENT_REQUEST_BOTH_EDGES, consumer_label="high to low", fd=3}) = 0

ioctl(3, GPIO_GET_LINEINFO_IOCTL, {line_offset=1, flags=GPIOLINE_FLAG_KERNEL|GPIOLINE_FLAG_IS_OUT, name="gpio-mockup-A-1", consumer="output atv-lo to as-is atv-hi"}) = 0

ioctl(7, GPIOHANDLE_GET_LINE_VALUES_IOCTL, {values=[1, 1, 0, 1, 1, 1, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, ...]}) = 0

ioctl(7, GPIOHANDLE_SET_LINE_VALUES_IOCTL, {values=[1, 1, 0, 1, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, ...]}) = 0

ioctl(7, GPIOHANDLE_SET_CONFIG_IOCTL, {flags=GPIOHANDLE_REQUEST_ACTIVE_LOW, default_values=[0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, ...]}) = 0

ioctl(3, GPIO_GET_LINEINFO_WATCH_IOCTL, {line_offset=3, flags=0, name="gpio-mockup-A-3", consumer=""}) = 0

ioctl(3, GPIO_GET_LINEINFO_UNWATCH_IOCTL, {offset=3}) = 0

For unsuccessful calls it only prints the input fields, and drops the output fields. e.g.

ioctl(3, GPIO_GET_LINEINFO_WATCH_IOCTL, {line_offset=3}) = -1 EPERM (Operation not permitted)

Does that work for you?

Quoted 41 lines · Kent Gibson

Quoted 5 lines · Bartosz Golaszewski

Quoted 4 lines · Arnd Bergmann

Quoted 1 line · Kent Gibson

+/**

Quoted 8 lines

Quoted 4 lines

Having different-sized members in the union makes it hard for something like strace to print the contents. How about just making them all __aligned_u64 even when 32 bits are sufficient?

Ah yes, adding support for GPIO ioctl()'s to strace has been on my TODO list for 3 years now. :(

I'm looking at doing this as it would be useful to have.

Just starting with the v1 uAPI, as v2 isn't merged yet.

I think I've got it basically done, but I thought I'd run it past you before sending a patch to the strace list.

Successful calls currently look like this:

ioctl(3, GPIO_GET_CHIPINFO_IOCTL, {name="gpiochip1", label="gpio-mockup-B", lines=8}) = 0

ioctl(3, GPIO_GET_LINEHANDLE_IOCTL, {lines=8, lineoffsets=[3, 2, 1, 0, 4, 5, 6, 7], flags=GPIOHANDLE_REQUEST_INPUT, default_values=[0, 0, 0, 0, 0, 0, 0, 0], consumer_label="", fd=7}) = 0

ioctl(0, GPIO_GET_LINEEVENT_IOCTL, {lineoffset=2, handleflags=GPIOHANDLE_REQUEST_INPUT, eventflags=GPIOEVENT_REQUEST_BOTH_EDGES, consumer_label="high to low", fd=3}) = 0

ioctl(3, GPIO_GET_LINEINFO_IOCTL, {line_offset=1, flags=GPIOLINE_FLAG_KERNEL|GPIOLINE_FLAG_IS_OUT, name="gpio-mockup-A-1", consumer="output atv-lo to as-is atv-hi"}) = 0

ioctl(7, GPIOHANDLE_GET_LINE_VALUES_IOCTL, {values=[1, 1, 0, 1, 1, 1, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, ...]}) = 0

ioctl(7, GPIOHANDLE_SET_LINE_VALUES_IOCTL, {values=[1, 1, 0, 1, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, ...]}) = 0

ioctl(7, GPIOHANDLE_SET_CONFIG_IOCTL, {flags=GPIOHANDLE_REQUEST_ACTIVE_LOW, default_values=[0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, ...]}) = 0

ioctl(3, GPIO_GET_LINEINFO_WATCH_IOCTL, {line_offset=3, flags=0, name="gpio-mockup-A-3", consumer=""}) = 0

Quoted 9 lines

ioctl(3, GPIO_GET_LINEINFO_UNWATCH_IOCTL, {offset=3}) = 0

For unsuccessful calls it only prints the input fields, and drops the output fields. e.g.

ioctl(3, GPIO_GET_LINEINFO_WATCH_IOCTL, {line_offset=3}) = -1 EPERM (Operation not permitted)

Does that work for you?

Kent,

This is perfect! Thanks for doing this! For the error cases: I'm not sure how strace handles this for other ioctls() - maybe we could print something like 'flags=N/A'? Unless what you did is the standard way, then leave it.

Quoted 1 line · Kent Gibson

On Tue, Sep 22, 2020 at 11:50:51AM +0200, Bartosz Golaszewski wrote:

Quoted 3 lines

I'm looking at doing this as it would be useful to have.

Does that work for you?

This is perfect! Thanks for doing this! For the error cases: I'm not sure how strace handles this for other ioctls() - maybe we could print something like 'flags=N/A'? Unless what you did is the standard way, then leave it.

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

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

The new structs have been analysed with pahole to ensure that they are sized as expected and contain no implicit padding.

The lack of future proofing in v1 makes it impossible to, for example, add the debounce feature that is included in v2.
The future proofing is addressed by providing configurable attributes in line config and reserved padding in all structs for future features. Specifically, the line request, config, info, info_changed and event structs receive updated versions and new ioctls.

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

v1 has three different flags fields, each with their own separate bit definitions. In v2 that is collapsed to one - gpio_v2_line_flag.

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

As a consequence, there are now only two types of file handle to be concerned with, the chip and the line, and it is clearer which ioctls apply to which type of handle.

There is also some minor renaming of fields for consistency compared to their v1 counterparts, e.g. offset rather than lineoffset or line_offset, and consumer rather than consumer_label.

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

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

(quoted diff) +2 -0

+       struct gpio_v2_line_config_attribute attrs[GPIO_V2_LINE_NUM_ATTRS_MAX];
+};

(quoted diff) +2 -0

+       __s32 fd;
+};

+};

+};

+};

Quoted 1 line · Kent Gibson

Quoted 10 lines

There is also some minor renaming of fields for consistency compared to their v1 counterparts, e.g. offset rather than lineoffset or line_offset, and consumer rather than consumer_label.

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

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

...

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

Quoted 6 lines

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

+struct gpio_v2_line_config {
+       __aligned_u64 flags;
+       __u32 num_attrs;
 
+       /* Pad to fill implicit padding and reserve space for future use. */
+       __u32 padding[5];

Probably I somehow missed the answer, but why do we need 5 here and not 1?

Quoted 2 lines · Andy Shevchenko

Quoted 1 line · Kent Gibson

[snip]

Quoted 13 lines

Quoted 10 lines

There is also some minor renaming of fields for consistency compared to their v1 counterparts, e.g. offset rather than lineoffset or line_offset, and consumer rather than consumer_label.

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

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

...

Quoted 9 lines

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

Quoted 6 lines

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

+struct gpio_v2_line_config {
+       __aligned_u64 flags;
+       __u32 num_attrs;
 
+       /* Pad to fill implicit padding and reserve space for future use. */
+       __u32 padding[5];

Probably I somehow missed the answer, but why do we need 5 here and not 1?

Sorry, I got tired of repeating myself, and just acked that we disagree on the approach here.

Your suggestion to use the size for version would result in an explosion of ioctl signatures - every time we add a field we have to add a new ioctl and handle it separately in gpio_ioctl() or linereq_ioctl().

Instead what we do here is reserve some space for future use - that we can replace with fields without changing the signature.
The padding is required to be zeroed now, and any future use will take a 0 to mean "leave alone".

The sizes are a guestimate as to what may be needed in the future, and as such are almost certainly wrong - but hopefully on the high side. If that fails we can always fall back to your approach.

Quoted 28 lines · Kent Gibson

Quoted 14 lines · Andy Shevchenko

Quoted 10 lines · Kent Gibson

There is also some minor renaming of fields for consistency compared to their v1 counterparts, e.g. offset rather than lineoffset or line_offset, and consumer rather than consumer_label.

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

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

...

Quoted 9 lines

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

Quoted 6 lines

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

+struct gpio_v2_line_config {
+       __aligned_u64 flags;
+       __u32 num_attrs;
 
+       /* Pad to fill implicit padding and reserve space for future use. */
+       __u32 padding[5];

Probably I somehow missed the answer, but why do we need 5 here and not 1?

Sorry, I got tired of repeating myself, and just acked that we disagree on the approach here.

Quoted 3 lines

Your suggestion to use the size for version would result in an explosion of ioctl signatures - every time we add a field we have to add a new ioctl and handle it separately in gpio_ioctl() or linereq_ioctl().

No, you just add
__u32 version; // implies sizeof() check as well.

Look for examples of existing ABIs (e.g. perf ABI).

Quoted 29 lines · Andy Shevchenko

Quoted 28 lines · Kent Gibson

Quoted 14 lines · Andy Shevchenko

Quoted 10 lines · Kent Gibson

There is also some minor renaming of fields for consistency compared to their v1 counterparts, e.g. offset rather than lineoffset or line_offset, and consumer rather than consumer_label.

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

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

...

Quoted 9 lines

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

Quoted 6 lines

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

+struct gpio_v2_line_config {
+       __aligned_u64 flags;
+       __u32 num_attrs;
 
+       /* Pad to fill implicit padding and reserve space for future use. */
+       __u32 padding[5];

Probably I somehow missed the answer, but why do we need 5 here and not 1?

Sorry, I got tired of repeating myself, and just acked that we disagree on the approach here.

Quoted 8 lines

Quoted 3 lines

Your suggestion to use the size for version would result in an explosion of ioctl signatures - every time we add a field we have to add a new ioctl and handle it separately in gpio_ioctl() or linereq_ioctl().

No, you just add
__u32 version; // implies sizeof() check as well.

Look for examples of existing ABIs (e.g. perf ABI).

Please don't ever add a version field to an ioctl structure, this has been shown to cause more problems than it solves many times in the past...

Having some reserved fields can be helpful, as long as the kernel returns an error in case any of the unknown fields are nonzero.

I'd also prefer fewer than five reserved fields, but that is not as important, just use as many as are likely to be used in the future, but not more: It's easy to add one more ioctl command in case the expectations are wrong, but I agree you wouldn't want a new command every time another field gets added if the past has shown that this happens a lot.

Add support for requesting lines using the GPIO_V2_GET_LINE_IOCTL, and returning their current values using GPIO_V2_LINE_GET_VALUES_IOCTL.

The struct linereq implementation is based on the v1 struct linehandle implementation.

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

+        * the hardware actually supports enabling both at the same time the
+        * electrical result would be disastrous.
+        */

+ /* Only one bias flag can be set. */

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

+       if (((flags & GPIO_V2_LINE_FLAG_BIAS_DISABLED) &&
+            (flags & (GPIO_V2_LINE_FLAG_BIAS_PULL_DOWN |
+                      GPIO_V2_LINE_FLAG_BIAS_PULL_UP))) ||
+           ((flags & GPIO_V2_LINE_FLAG_BIAS_PULL_DOWN) &&
+            (flags & GPIO_V2_LINE_FLAG_BIAS_PULL_UP)))
+               return -EINVAL;

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

+       if (flags & GPIO_V2_LINE_FLAG_OUTPUT)
+               set_bit(FLAG_IS_OUT, flagsp);
+       else if (flags & GPIO_V2_LINE_FLAG_INPUT)
+               clear_bit(FLAG_IS_OUT, flagsp);
+
+       assign_bit(FLAG_OPEN_DRAIN, flagsp,
+                  flags & GPIO_V2_LINE_FLAG_OPEN_DRAIN);
+       assign_bit(FLAG_OPEN_SOURCE, flagsp,
+                  flags & GPIO_V2_LINE_FLAG_OPEN_SOURCE);
+       assign_bit(FLAG_PULL_UP, flagsp,
+                  flags & GPIO_V2_LINE_FLAG_BIAS_PULL_UP);
+       assign_bit(FLAG_PULL_DOWN, flagsp,
+                  flags & GPIO_V2_LINE_FLAG_BIAS_PULL_DOWN);
+       assign_bit(FLAG_BIAS_DISABLE, flagsp,
+                  flags & GPIO_V2_LINE_FLAG_BIAS_DISABLED);
+}

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

+static long linereq_get_values(struct linereq *lr, void __user *ip)
+{
+       struct gpio_v2_line_values lv;
+       DECLARE_BITMAP(vals, GPIO_V2_LINES_MAX);
+       struct gpio_desc **descs;
+       unsigned int i, didx, num_get;
+       int ret;
 

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

+               descs = kmalloc_array(num_get, sizeof(*descs), GFP_KERNEL);
+               if (!descs)
+                       return -ENOMEM;
+               for (didx = 0, i = 0; i < lr->num_lines; i++) {
+                       if (lv.mask & BIT_ULL(i)) {
+                               descs[didx] = lr->lines[i].desc;
+                               didx++;
+                       }
+               }
+       }
+       ret = gpiod_get_array_value_complex(false, true, num_get,
+                                           descs, NULL, vals);
+
+       if (num_get != 1)
+               kfree(descs);
+       if (ret)
+               return ret;

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

+       if (copy_to_user(ip, &lv, sizeof(lv)))
+               return -EFAULT;
+
+       return 0;
+}

Quoted 6 lines · Kent Gibson

Add support for requesting lines using the GPIO_V2_GET_LINE_IOCTL, and returning their current values using GPIO_V2_LINE_GET_VALUES_IOCTL.

The struct linereq implementation is based on the v1 struct linehandle implementation.

...

Review

Quoted 2 lines

(quoted diff) +2 -0

+       /*
+        * Do not allow OPEN_SOURCE & OPEN_DRAIN flags in a single request. If

You see, in some cases you are using "OR:ed" as understandable for programmers, and here & which should be and in plain English and really confusing from a programmer's perspective. That's why I prefer to see plain English rather than something which is full of encoded meanings.

Quoted 3 lines

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

+        * the hardware actually supports enabling both at the same time the
+        * electrical result would be disastrous.
+        */

...

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

Quoted 4 lines

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

+       /* Bias requires explicit direction. */
+       if ((flags & GPIO_V2_LINE_BIAS_FLAGS) &&
+           !(flags & GPIO_V2_LINE_DIRECTION_FLAGS))
+               return -EINVAL;

Okay, since this is strict we probably may relax it in the future if it will be a use case.
...

Quoted 1 line

+ /* Only one bias flag can be set. */

Ditto. (Some controllers allow to set both simultaneously, though I can't imagine good use case for that)

Quoted 6 lines

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

+       if (((flags & GPIO_V2_LINE_FLAG_BIAS_DISABLED) &&
+            (flags & (GPIO_V2_LINE_FLAG_BIAS_PULL_DOWN |
+                      GPIO_V2_LINE_FLAG_BIAS_PULL_UP))) ||
+           ((flags & GPIO_V2_LINE_FLAG_BIAS_PULL_DOWN) &&
+            (flags & GPIO_V2_LINE_FLAG_BIAS_PULL_UP)))
+               return -EINVAL;

...

Quoted 3 lines

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

+static void gpio_v2_line_config_flags_to_desc_flags(u64 flags,
+                                                   unsigned long *flagsp)
+{

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

Quoted 2 lines

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

+       assign_bit(FLAG_ACTIVE_LOW, flagsp,
+                  flags & GPIO_V2_LINE_FLAG_ACTIVE_LOW);

What I meant is to attach also this to the other assign_bit():s below. And just in case a question: why not __asign_bit() do we really need atomicity?

Quoted 16 lines

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

+       if (flags & GPIO_V2_LINE_FLAG_OUTPUT)
+               set_bit(FLAG_IS_OUT, flagsp);
+       else if (flags & GPIO_V2_LINE_FLAG_INPUT)
+               clear_bit(FLAG_IS_OUT, flagsp);
+
+       assign_bit(FLAG_OPEN_DRAIN, flagsp,
+                  flags & GPIO_V2_LINE_FLAG_OPEN_DRAIN);
+       assign_bit(FLAG_OPEN_SOURCE, flagsp,
+                  flags & GPIO_V2_LINE_FLAG_OPEN_SOURCE);
+       assign_bit(FLAG_PULL_UP, flagsp,
+                  flags & GPIO_V2_LINE_FLAG_BIAS_PULL_UP);
+       assign_bit(FLAG_PULL_DOWN, flagsp,
+                  flags & GPIO_V2_LINE_FLAG_BIAS_PULL_DOWN);
+       assign_bit(FLAG_BIAS_DISABLE, flagsp,
+                  flags & GPIO_V2_LINE_FLAG_BIAS_DISABLED);
+}

...

Quoted 8 lines

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

+static long linereq_get_values(struct linereq *lr, void __user *ip)
+{
+       struct gpio_v2_line_values lv;
+       DECLARE_BITMAP(vals, GPIO_V2_LINES_MAX);
+       struct gpio_desc **descs;
+       unsigned int i, didx, num_get;
+       int ret;
 

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

Quoted 10 lines

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

+       /* NOTE: It's ok to read values of output lines. */
+       if (copy_from_user(&lv, ip, sizeof(lv)))
+               return -EFAULT;
+
+       for (num_get = 0, i = 0; i < lr->num_lines; i++) {
+               if (lv.mask & BIT_ULL(i)) {
+                       num_get++;
+                       descs = &lr->lines[i].desc;
+               }
+       }

So what you can do here is something like

DECLARE_BITMAP(mask, u64);

...

bitmap_from_u64(mask, lv.mask);
num_get = bitmap_weight(mask, lr->num_lines);
if (num_get == 0)
return -EINVAL;

for_each_set_bit(i, mask, lr->num_lines)

      descs = &lr->lines[i].desc;

// I'm not sure I understood a purpose of the above
// ah, looks like malloc() avoidance, but you may move it below...

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

Quoted 5 lines

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

+       if (num_get == 0)
+               return -EINVAL;
+
 
+       if (num_get != 1) {

...something like

if (num_get == 1)
descs = ...[find_first_bit(mask, lr->num_lines)];
else {
...
for_each_set_bit() {
...
}
}

Quoted 17 lines

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

+               descs = kmalloc_array(num_get, sizeof(*descs), GFP_KERNEL);
+               if (!descs)
+                       return -ENOMEM;
+               for (didx = 0, i = 0; i < lr->num_lines; i++) {
+                       if (lv.mask & BIT_ULL(i)) {
+                               descs[didx] = lr->lines[i].desc;
+                               didx++;
+                       }
+               }
+       }
+       ret = gpiod_get_array_value_complex(false, true, num_get,
+                                           descs, NULL, vals);
+
+       if (num_get != 1)
+               kfree(descs);
+       if (ret)
+               return ret;

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

Quoted 10 lines

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

+
 
+       lv.bits = 0;
+       for (didx = 0, i = 0; i < lr->num_lines; i++) {
+               if (lv.mask & BIT_ULL(i)) {
+                       if (test_bit(didx, vals))
+                               lv.bits |= BIT_ULL(i);
+                       didx++;
+               }
+       }

So here...

Quoted 5 lines

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

+       if (copy_to_user(ip, &lv, sizeof(lv)))
+               return -EFAULT;
+
+       return 0;
+}

...

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

Quoted 9 lines

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

+       /* Make sure this is terminated */
+       ulr.consumer[sizeof(ulr.consumer)-1] = '\0';
+       if (strlen(ulr.consumer)) {
+               lr->label = kstrdup(ulr.consumer, GFP_KERNEL);
+               if (!lr->label) {
+                       ret = -ENOMEM;
+                       goto out_free_linereq;
+               }
+       }

Still don't get why we can\t use kstrndup() here...

Quoted 1 line · Andy Shevchenko

On Tue, Sep 22, 2020 at 5:35 AM Kent Gibson <[email protected]> wrote:

Quoted 6 lines

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

Quoted 2 lines

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

+       assign_bit(FLAG_ACTIVE_LOW, flagsp,
+                  flags & GPIO_V2_LINE_FLAG_ACTIVE_LOW);

What I meant is to attach also this to the other assign_bit():s below. And just in case a question: why not __asign_bit() do we really need atomicity?

These are initialized as per their order in the flags so it is easier to tell if any are missing.

The atomicity is not required here, but it is elsewhere so you are oblidged to use it for all accesses, no?

Quoted 15 lines

Quoted 15 lines

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

+       if (flags & GPIO_V2_LINE_FLAG_OUTPUT)
+               set_bit(FLAG_IS_OUT, flagsp);
+       else if (flags & GPIO_V2_LINE_FLAG_INPUT)
+               clear_bit(FLAG_IS_OUT, flagsp);
+
+       assign_bit(FLAG_OPEN_DRAIN, flagsp,
+                  flags & GPIO_V2_LINE_FLAG_OPEN_DRAIN);
+       assign_bit(FLAG_OPEN_SOURCE, flagsp,
+                  flags & GPIO_V2_LINE_FLAG_OPEN_SOURCE);
+       assign_bit(FLAG_PULL_UP, flagsp,
+                  flags & GPIO_V2_LINE_FLAG_BIAS_PULL_UP);
+       assign_bit(FLAG_PULL_DOWN, flagsp,
+                  flags & GPIO_V2_LINE_FLAG_BIAS_PULL_DOWN);
+       assign_bit(FLAG_BIAS_DISABLE, flagsp,
+                  flags & GPIO_V2_LINE_FLAG_BIAS_DISABLED);

Quoted 10 lines

Quoted 9 lines

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

+       /* Make sure this is terminated */
+       ulr.consumer[sizeof(ulr.consumer)-1] = '\0';
+       if (strlen(ulr.consumer)) {
+               lr->label = kstrdup(ulr.consumer, GFP_KERNEL);
+               if (!lr->label) {
+                       ret = -ENOMEM;
+                       goto out_free_linereq;
+               }
+       }

Quoted 2 lines

Still don't get why we can\t use kstrndup() here...

I know ;-).

Another one directly from v1, and the behaviour there is to leave lr->label nulled if consumer is empty.
It just avoids a pointless malloc for the null terminator.

Quoted 2 lines · Kent Gibson

Quoted 1 line · Andy Shevchenko

On Tue, Sep 22, 2020 at 5:35 AM Kent Gibson <[email protected]> wrote:

Quoted 15 lines

Quoted 10 lines

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

Quoted 9 lines

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

+       /* Make sure this is terminated */
+       ulr.consumer[sizeof(ulr.consumer)-1] = '\0';
+       if (strlen(ulr.consumer)) {
+               lr->label = kstrdup(ulr.consumer, GFP_KERNEL);
+               if (!lr->label) {
+                       ret = -ENOMEM;
+                       goto out_free_linereq;
+               }
+       }

Quoted 2 lines

Still don't get why we can\t use kstrndup() here...

I know ;-).

Quoted 3 lines

Another one directly from v1, and the behaviour there is to leave lr->label nulled if consumer is empty.
It just avoids a pointless malloc for the null terminator.

Again, similar as for bitmap API usage, if it makes code cleaner and increases readability, I will go for it.
Also don't forget the army of janitors that won't understand the case and simply convert everything that can be converted.

On Fri, Sep 25, 2020 at 01:06:02PM +0300, Andy Shevchenko wrote:

Hmmm, there is more to it than I thought - gpiod_request_commit(), which this code eventually calls, interprets a null label (not an empty string) as indicating that the user has not set the label, in which case it will set the desc label to "?". So userspace cannot force the desc label to be empty.

We need to keep that label as null in that case to maintain that behaviour.

I will add a comment there though.

Hmmm, having said that, does this form work for you:

        if (ulr.consumer[0] != '\0') {
                /* label is only initialized if consumer is set */
                lr->label = kstrndup(ulr.consumer, sizeof(ulr.consumer) - 1, GFP_KERNEL);
        ...

It actually compiles slightly larger, I guess due to the extra parameter in the kstrndup() call, but may be slightly more readable??

Quoted 1 line · Kent Gibson

On Fri, Sep 25, 2020 at 01:06:02PM +0300, Andy Shevchenko wrote:

...

Quoted 18 lines

Hmmm, there is more to it than I thought - gpiod_request_commit(), which this code eventually calls, interprets a null label (not an empty string) as indicating that the user has not set the label, in which case it will set the desc label to "?". So userspace cannot force the desc label to be empty.

We need to keep that label as null in that case to maintain that behaviour.

I will add a comment there though.

Hmmm, having said that, does this form work for you:

        if (ulr.consumer[0] != '\0') {
                /* label is only initialized if consumer is set */
                lr->label = kstrndup(ulr.consumer, sizeof(ulr.consumer) - 1, GFP_KERNEL);
        ...

Quoted 2 lines

It actually compiles slightly larger, I guess due to the extra parameter in the kstrndup() call, but may be slightly more readable??

I really don't want to delay this series, choose what you think is better and we may amend it later.

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

Quoted 5 lines

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

+       /* Bias requires explicit direction. */
+       if ((flags & GPIO_V2_LINE_BIAS_FLAGS) &&
+           !(flags & GPIO_V2_LINE_DIRECTION_FLAGS))
+               return -EINVAL;

Okay, since this is strict we probably may relax it in the future if it will be a use case.
...

Quoted 1 line

+ /* Only one bias flag can be set. */

Ditto. (Some controllers allow to set both simultaneously, though I can't imagine good use case for that)

Add support for GPIO_V2_GET_LINEINFO_IOCTL and
GPIO_V2_GET_LINEINFO_WATCH_IOCTL.

The core of this change is the event kfifo switching to contain struct gpioline_info_changed_v2, instead of v1 as v2 is richer.

The two uAPI versions are mostly independent - other than where they both provide line info changes via reads on the chip fd. As the info change structs differ between v1 and v2, the infowatch implementation tracks which version of the infowatch ioctl, either GPIO_GET_LINEINFO_WATCH_IOCTL or GPIO_V2_GET_LINEINFO_WATCH_IOCTL, initiates the initial watch and returns the corresponding info change struct to the read. The version supported on that fd locks to that version on the first watch request, so subsequent watches from that process must use the same uAPI version.

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

---

Changes for v5:
- as per cover letter

Changes for v4:
- replace strncpy with memcpy in gpio_v2_line_info_to_v1

 drivers/gpio/gpiolib-cdev.c | 197 +++++++++++++++++++++++++++++++-----
 1 file changed, 169 insertions(+), 28 deletions(-)

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

index 7a3ed2617f74..d3857113f58c 100644
@@ -181,7 +181,8 @@ static long linehandle_set_config(struct linehandle_state *lh,
                }
 
                blocking_notifier_call_chain(&desc->gdev->notifier,
-                                            GPIOLINE_CHANGED_CONFIG, desc);
+                                            GPIO_V2_LINE_CHANGED_CONFIG,
+                                            desc);
        }
        return 0;
 }

@@ -353,7 +354,7 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
                }
 
                blocking_notifier_call_chain(&desc->gdev->notifier,
-                                            GPIOLINE_CHANGED_REQUESTED, desc);
+                                            GPIO_V2_LINE_CHANGED_REQUESTED, desc);
 
                dev_dbg(&gdev->dev, "registered chardev handle for line %d\n",
                        offset);

@@ -747,7 +748,7 @@ static int linereq_create(struct gpio_device *gdev, void __user *ip)
                }
 
                blocking_notifier_call_chain(&desc->gdev->notifier,
-                                            GPIOLINE_CHANGED_REQUESTED, desc);
+                                            GPIO_V2_LINE_CHANGED_REQUESTED, desc);
 
                dev_dbg(&gdev->dev, "registered chardev handle for line %d\n",
                        offset);

@@ -1094,7 +1095,7 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
                goto out_free_le;
 
        blocking_notifier_call_chain(&desc->gdev->notifier,
-                                    GPIOLINE_CHANGED_REQUESTED, desc);
+                                    GPIO_V2_LINE_CHANGED_REQUESTED, desc);
 
        irq = gpiod_to_irq(desc);
        if (irq <= 0) {

@@ -1161,17 +1162,59 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
        return ret;
 }

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

+       info_v1->line_offset = info_v2->offset;
+       info_v1->flags = 0;
+
+       if (flagsv2 & GPIO_V2_LINE_FLAG_USED)
+               info_v1->flags |= GPIOLINE_FLAG_KERNEL;
+
+       if (flagsv2 & GPIO_V2_LINE_FLAG_OUTPUT)
+               info_v1->flags |= GPIOLINE_FLAG_IS_OUT;
+
+       if (flagsv2 & GPIO_V2_LINE_FLAG_ACTIVE_LOW)
+               info_v1->flags |= GPIOLINE_FLAG_ACTIVE_LOW;
+
+       if (flagsv2 & GPIO_V2_LINE_FLAG_OPEN_DRAIN)
+               info_v1->flags |= GPIOLINE_FLAG_OPEN_DRAIN;
+       if (flagsv2 & GPIO_V2_LINE_FLAG_OPEN_SOURCE)
+               info_v1->flags |= GPIOLINE_FLAG_OPEN_SOURCE;
+
+       if (flagsv2 & GPIO_V2_LINE_FLAG_BIAS_PULL_UP)
+               info_v1->flags |= GPIOLINE_FLAG_BIAS_PULL_UP;
+       if (flagsv2 & GPIO_V2_LINE_FLAG_BIAS_PULL_DOWN)
+               info_v1->flags |= GPIOLINE_FLAG_BIAS_PULL_DOWN;
+       if (flagsv2 & GPIO_V2_LINE_FLAG_BIAS_DISABLED)
+               info_v1->flags |= GPIOLINE_FLAG_BIAS_DISABLE;
+}
+
+static void gpio_v2_line_info_changed_to_v1(
+               struct gpio_v2_line_info_changed *lic_v2,
+               struct gpioline_info_changed *lic_v1)
+{
+       gpio_v2_line_info_to_v1(&lic_v2->info, &lic_v1->info);
+       lic_v1->timestamp = lic_v2->timestamp_ns;
+       lic_v1->event_type = lic_v2->event_type;
+}
+
 #endif /* CONFIG_GPIO_CDEV_V1 */
 
 static void gpio_desc_to_lineinfo(struct gpio_desc *desc,
-                                 struct gpioline_info *info)
+                                 struct gpio_v2_line_info *info)
 {
        struct gpio_chip *gc = desc->gdev->chip;
        bool ok_for_pinctrl;
        unsigned long flags;
 
        memset(info, 0, sizeof(*info));
-       info->line_offset = gpio_chip_hwgpio(desc);
+       info->offset = gpio_chip_hwgpio(desc);
 
        /*

* This function takes a mutex so we must check this before taking

drivers/gpio/gpiolib-cdev.c +20 -12

@@ -1181,7 +1224,7 @@ static void gpio_desc_to_lineinfo(struct gpio_desc *desc,
         * lock common to both frameworks?
         */
        ok_for_pinctrl =
-               pinctrl_gpio_can_use_line(gc->base + info->line_offset);
+               pinctrl_gpio_can_use_line(gc->base + info->offset);
 
        spin_lock_irqsave(&gpio_lock, flags);
 

@@ -1202,23 +1245,27 @@ static void gpio_desc_to_lineinfo(struct gpio_desc *desc,
            test_bit(FLAG_EXPORT, &desc->flags) ||
            test_bit(FLAG_SYSFS, &desc->flags) ||
            !ok_for_pinctrl)
-               info->flags |= GPIOLINE_FLAG_KERNEL;
+               info->flags |= GPIO_V2_LINE_FLAG_USED;
+
        if (test_bit(FLAG_IS_OUT, &desc->flags))
-               info->flags |= GPIOLINE_FLAG_IS_OUT;
+               info->flags |= GPIO_V2_LINE_FLAG_OUTPUT;
+       else
+               info->flags |= GPIO_V2_LINE_FLAG_INPUT;
+
        if (test_bit(FLAG_ACTIVE_LOW, &desc->flags))
-               info->flags |= GPIOLINE_FLAG_ACTIVE_LOW;
+               info->flags |= GPIO_V2_LINE_FLAG_ACTIVE_LOW;
+
        if (test_bit(FLAG_OPEN_DRAIN, &desc->flags))
-               info->flags |= (GPIOLINE_FLAG_OPEN_DRAIN |
-                               GPIOLINE_FLAG_IS_OUT);
+               info->flags |= GPIO_V2_LINE_FLAG_OPEN_DRAIN;
        if (test_bit(FLAG_OPEN_SOURCE, &desc->flags))
-               info->flags |= (GPIOLINE_FLAG_OPEN_SOURCE |
-                               GPIOLINE_FLAG_IS_OUT);
+               info->flags |= GPIO_V2_LINE_FLAG_OPEN_SOURCE;
+
        if (test_bit(FLAG_BIAS_DISABLE, &desc->flags))
-               info->flags |= GPIOLINE_FLAG_BIAS_DISABLE;
+               info->flags |= GPIO_V2_LINE_FLAG_BIAS_DISABLED;
        if (test_bit(FLAG_PULL_DOWN, &desc->flags))
-               info->flags |= GPIOLINE_FLAG_BIAS_PULL_DOWN;
+               info->flags |= GPIO_V2_LINE_FLAG_BIAS_PULL_DOWN;
        if (test_bit(FLAG_PULL_UP, &desc->flags))
-               info->flags |= GPIOLINE_FLAG_BIAS_PULL_UP;
+               info->flags |= GPIO_V2_LINE_FLAG_BIAS_PULL_UP;
 
        spin_unlock_irqrestore(&gpio_lock, flags);
 }

@@ -1226,11 +1273,65 @@ static void gpio_desc_to_lineinfo(struct gpio_desc *desc,
 struct gpio_chardev_data {
        struct gpio_device *gdev;
        wait_queue_head_t wait;
-       DECLARE_KFIFO(events, struct gpioline_info_changed, 32);
+       DECLARE_KFIFO(events, struct gpio_v2_line_info_changed, 32);
        struct notifier_block lineinfo_changed_nb;
        unsigned long *watched_lines;
+#ifdef CONFIG_GPIO_CDEV_V1
+       atomic_t watch_abi_version;
+#endif
 };
 
+#ifdef CONFIG_GPIO_CDEV_V1

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

+       return 0;
+}
+#endif
+
+static int lineinfo_get(struct gpio_chardev_data *cdev, void __user *ip,
+                       bool watch)
+{
+       struct gpio_desc *desc;
+       struct gpio_v2_line_info lineinfo;
+
+       if (copy_from_user(&lineinfo, ip, sizeof(lineinfo)))
+               return -EFAULT;
+
+       if (memchr_inv(lineinfo.padding, 0, sizeof(lineinfo.padding)))
+               return -EINVAL;

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

+#endif
+               if (test_and_set_bit(lineinfo.offset, cdev->watched_lines))
+                       return -EBUSY;
+       }
+       gpio_desc_to_lineinfo(desc, &lineinfo);
+
+       if (copy_to_user(ip, &lineinfo, sizeof(lineinfo))) {
+               if (watch)
+                       clear_bit(lineinfo.offset, cdev->watched_lines);
+               return -EFAULT;
+       }
+
+       return 0;
+}
+

/*
* gpio_ioctl() - ioctl handler for the GPIO chardev */

drivers/gpio/gpiolib-cdev.c +50 -11

@@ -1240,7 +1341,6 @@ static long gpio_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
        struct gpio_device *gdev = cdev->gdev;
        struct gpio_chip *gc = gdev->chip;
        void __user *ip = (void __user *)arg;
-       struct gpio_desc *desc;
        __u32 offset;
 
        /* We fail any subsequent ioctl():s when the chip is gone */

@@ -1263,7 +1363,9 @@ static long gpio_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
                return 0;
 #ifdef CONFIG_GPIO_CDEV_V1
        } else if (cmd == GPIO_GET_LINEINFO_IOCTL) {
+               struct gpio_desc *desc;
                struct gpioline_info lineinfo;
+               struct gpio_v2_line_info lineinfo_v2;
 
                if (copy_from_user(&lineinfo, ip, sizeof(lineinfo)))
                        return -EFAULT;

@@ -1273,7 +1375,8 @@ static long gpio_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
                if (IS_ERR(desc))
                        return PTR_ERR(desc);
 
-               gpio_desc_to_lineinfo(desc, &lineinfo);
+               gpio_desc_to_lineinfo(desc, &lineinfo_v2);
+               gpio_v2_line_info_to_v1(&lineinfo_v2, &lineinfo);
 
                if (copy_to_user(ip, &lineinfo, sizeof(lineinfo)))
                        return -EFAULT;

@@ -1283,7 +1386,9 @@ static long gpio_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
        } else if (cmd == GPIO_GET_LINEEVENT_IOCTL) {
                return lineevent_create(gdev, ip);
        } else if (cmd == GPIO_GET_LINEINFO_WATCH_IOCTL) {
+               struct gpio_desc *desc;
                struct gpioline_info lineinfo;
+               struct gpio_v2_line_info lineinfo_v2;
 
                if (copy_from_user(&lineinfo, ip, sizeof(lineinfo)))
                        return -EFAULT;

@@ -1293,10 +1398,14 @@ static long gpio_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
                if (IS_ERR(desc))
                        return PTR_ERR(desc);
 
+               if (lineinfo_ensure_abi_version(cdev, 1))
+                       return -EPERM;
+
                if (test_and_set_bit(lineinfo.line_offset, cdev->watched_lines))
                        return -EBUSY;
 
-               gpio_desc_to_lineinfo(desc, &lineinfo);
+               gpio_desc_to_lineinfo(desc, &lineinfo_v2);
+               gpio_v2_line_info_to_v1(&lineinfo_v2, &lineinfo);
 
                if (copy_to_user(ip, &lineinfo, sizeof(lineinfo))) {
                        clear_bit(lineinfo.line_offset, cdev->watched_lines);

@@ -1305,6 +1414,10 @@ static long gpio_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 
                return 0;
 #endif /* CONFIG_GPIO_CDEV_V1 */
+       } else if (cmd == GPIO_V2_GET_LINEINFO_IOCTL ||
+                  cmd == GPIO_V2_GET_LINEINFO_WATCH_IOCTL) {
+               return lineinfo_get(cdev, ip,
+                                   cmd == GPIO_V2_GET_LINEINFO_WATCH_IOCTL);
        } else if (cmd == GPIO_V2_GET_LINE_IOCTL) {
                return linereq_create(gdev, ip);
        } else if (cmd == GPIO_GET_LINEINFO_UNWATCH_IOCTL) {

@@ -1340,7 +1453,7 @@ static int lineinfo_changed_notify(struct notifier_block *nb,
                                   unsigned long action, void *data)
 {
        struct gpio_chardev_data *cdev = to_gpio_chardev_data(nb);
-       struct gpioline_info_changed chg;
+       struct gpio_v2_line_info_changed chg;
        struct gpio_desc *desc = data;
        int ret;
 

@@ -1349,7 +1462,7 @@ static int lineinfo_changed_notify(struct notifier_block *nb,
 
        memset(&chg, 0, sizeof(chg));
        chg.event_type = action;
-       chg.timestamp = ktime_get_ns();
+       chg.timestamp_ns = ktime_get_ns();
        gpio_desc_to_lineinfo(desc, &chg.info);
 
        ret = kfifo_in_spinlocked(&cdev->events, &chg, 1, &cdev->wait.lock);

@@ -1380,12 +1493,16 @@ static ssize_t lineinfo_watch_read(struct file *file, char __user *buf,
                                   size_t count, loff_t *off)
 {
        struct gpio_chardev_data *cdev = file->private_data;
-       struct gpioline_info_changed event;
+       struct gpio_v2_line_info_changed event;
        ssize_t bytes_read = 0;
        int ret;
+       size_t event_size;
 
-       if (count < sizeof(event))
+#ifndef CONFIG_GPIO_CDEV_V1
+       event_size = sizeof(struct gpio_v2_line_info_changed);
+       if (count < event_size)
                return -EINVAL;
+#endif
 
        do {
                spin_lock(&cdev->wait.lock);

@@ -1407,7 +1524,17 @@ static ssize_t lineinfo_watch_read(struct file *file, char __user *buf,
                                return ret;
                        }
                }
-
+#ifdef CONFIG_GPIO_CDEV_V1
+               /* must be after kfifo check so watch_abi_version is set */
+               if (atomic_read(&cdev->watch_abi_version) == 2)
+                       event_size = sizeof(struct gpio_v2_line_info_changed);
+               else
+                       event_size = sizeof(struct gpioline_info_changed);
+               if (count < event_size) {
+                       spin_unlock(&cdev->wait.lock);
+                       return -EINVAL;
+               }
+#endif
                ret = kfifo_out(&cdev->events, &event, 1);
                spin_unlock(&cdev->wait.lock);
                if (ret != 1) {

@@ -1416,9 +1543,23 @@ static ssize_t lineinfo_watch_read(struct file *file, char __user *buf,
                        /* We should never get here. See lineevent_read(). */
                }
 
-               if (copy_to_user(buf + bytes_read, &event, sizeof(event)))
+#ifdef CONFIG_GPIO_CDEV_V1
+               if (event_size == sizeof(struct gpio_v2_line_info_changed)) {
+                       if (copy_to_user(buf + bytes_read, &event, event_size))
+                               return -EFAULT;
+               } else {
+                       struct gpioline_info_changed event_v1;
+
+                       gpio_v2_line_info_changed_to_v1(&event, &event_v1);
+                       if (copy_to_user(buf + bytes_read, &event_v1,
+                                        event_size))
+                               return -EFAULT;
+               }
+#else
+               if (copy_to_user(buf + bytes_read, &event, event_size))
                        return -EFAULT;
-               bytes_read += sizeof(event);
+#endif
+               bytes_read += event_size;
        } while (count >= bytes_read + sizeof(event));
 
        return bytes_read;
--

2.28.0

Quoted 4 lines · Kent Gibson

Add support for GPIO_V2_GET_LINEINFO_IOCTL and
GPIO_V2_GET_LINEINFO_WATCH_IOCTL.

Quoted 7 lines

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

+static void gpio_v2_line_info_to_v1(struct gpio_v2_line_info *info_v2,
+                                   struct gpioline_info *info_v1)
+{
+       u64 flagsv2 = info_v2->flags;
+
+       memcpy(info_v1->name, info_v2->name, sizeof(info_v1->name));

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

Quoted 2 lines

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

+       memcpy(info_v1->consumer, info_v2->consumer,
+              sizeof(info_v1->consumer));

One line?

Quoted 5 lines

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

+#ifdef CONFIG_GPIO_CDEV_V1
+static int lineinfo_ensure_abi_version(struct gpio_chardev_data *cdata,
+                                      unsigned int version)
+{

drivers/gpio/gpiolib-cdev.c static void gpio_desc_to_lineinfo(struct gpio_desc *desc, · line 1292

Quoted 6 lines

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

+       int abiv = atomic_read(&cdata->watch_abi_version);
+
+       if (abiv == 0) {
 
+               atomic_cmpxchg(&cdata->watch_abi_version, 0, version);
+               abiv = atomic_read(&cdata->watch_abi_version);

atomic_cmpxchng() returns a value.

Also there are no barriers here...

drivers/gpio/gpiolib-cdev.c static void gpio_desc_to_lineinfo(struct gpio_desc *desc, · line 1295

Quoted 3 lines

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

+       }
+       if (abiv != version)
+               return -EPERM;

I'm not sure I understand why this is atomic.

Also this seems to be racy if cdata changed in background.

Shouldn't be rather

if (atomic_cmpxchg() == 0) {
if (atomic_read() != version)

    return ...;
}

But here is still the question: why do you expect the version to be changed on background? And what about barriers?

Quoted 22 lines

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

+       return 0;
+}
+#endif
+
+static int lineinfo_get(struct gpio_chardev_data *cdev, void __user *ip,
+                       bool watch)
+{
+       struct gpio_desc *desc;
+       struct gpio_v2_line_info lineinfo;
+
+       if (copy_from_user(&lineinfo, ip, sizeof(lineinfo)))
+               return -EFAULT;
+
+       if (memchr_inv(lineinfo.padding, 0, sizeof(lineinfo.padding)))
+               return -EINVAL;
+
+       desc = gpiochip_get_desc(cdev->gdev->chip, lineinfo.offset);
+       if (IS_ERR(desc))
+               return PTR_ERR(desc);
+
+       if (watch) {
+#ifdef CONFIG_GPIO_CDEV_V1

drivers/gpio/gpiolib-cdev.c static void gpio_desc_to_lineinfo(struct gpio_desc *desc, · line 1319

Quoted 2 lines

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

+               if (lineinfo_ensure_abi_version(cdev, 2))
+                       return -EPERM;

Can't you propagate error code from the function?

Quoted 1 line · Andy Shevchenko

On Tue, Sep 22, 2020 at 5:35 AM Kent Gibson <[email protected]> wrote:

Quoted 5 lines

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

Quoted 2 lines

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

+       memcpy(info_v1->consumer, info_v2->consumer,
+              sizeof(info_v1->consumer));

One line?

It can be now the line length limit has been raised - it just breaks the old 80 character limit.

Quoted 8 lines

Quoted 7 lines

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

+static int lineinfo_ensure_abi_version(struct gpio_chardev_data *cdata,
+                                      unsigned int version)
+{
 
+       int abiv = atomic_read(&cdata->watch_abi_version);
+
+       if (abiv == 0) {

Quoted 4 lines

drivers/gpio/gpiolib-cdev.c static void gpio_desc_to_lineinfo(struct gpio_desc *desc, · line 1292

Quoted 2 lines

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

+               atomic_cmpxchg(&cdata->watch_abi_version, 0, version);
+               abiv = atomic_read(&cdata->watch_abi_version);

atomic_cmpxchng() returns a value.

Yep, it returns the old value - which we don't care about - see below.

Quoted 2 lines

Also there are no barriers here...

No barriers required - the atomic_cmpxchg() is sufficient.

Quoted 6 lines

drivers/gpio/gpiolib-cdev.c static void gpio_desc_to_lineinfo(struct gpio_desc *desc, · line 1295

Quoted 3 lines

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

+       }
+       if (abiv != version)
+               return -EPERM;

I'm not sure I understand why this is atomic.

The algorithm requires some level of protection and atomic is sufficient.

Quoted 2 lines

Also this seems to be racy if cdata changed in background.

Can you provide a case?

The atomic_cmpxchg() ensures cdata->watch_abi_version is only set once - first in wins. The atomic_read() is so we can check that the set version matches what the caller wants.
Note that multiple callers may request the same version - and all should succeed.

Quoted 8 lines

Shouldn't be rather

if (atomic_cmpxchg() == 0) {
if (atomic_read() != version)

    return ...;
}

My algorithm allows for multiple callers requesting the same version to all succeed. Yours would fail the first conditional for all but the first, and you haven't provided an else for that case...

... but it would probably look the same so the conditional is pointless, or it would reject the request - which would be wrong.

Quoted 3 lines

But here is still the question: why do you expect the version to be changed on background? And what about barriers?

While it is unlikely that userspace will be attempting to use both ABI versions simultaneously on the same chip request, it is a possiblity and so needs to be protected against. And better to have the watch request fail than the read fail or worse - return the wrong struct version.

The atomic_cmpxchg() is sufficient for this algorithm - no barriers required. It could also be written with a spinlock but I was trying to avoid locks unless they were absolutely necessary. A spinlock version may arguably be more readable, but it would certainly be more verbose, larger and slower.

I'm happy to add some documentation to the function if that would help.

Quoted 23 lines

Quoted 22 lines

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

+       return 0;
+}
+#endif
+
+static int lineinfo_get(struct gpio_chardev_data *cdev, void __user *ip,
+                       bool watch)
+{
+       struct gpio_desc *desc;
+       struct gpio_v2_line_info lineinfo;
+
+       if (copy_from_user(&lineinfo, ip, sizeof(lineinfo)))
+               return -EFAULT;
+
+       if (memchr_inv(lineinfo.padding, 0, sizeof(lineinfo.padding)))
+               return -EINVAL;
+
+       desc = gpiochip_get_desc(cdev->gdev->chip, lineinfo.offset);
+       if (IS_ERR(desc))
+               return PTR_ERR(desc);
+
+       if (watch) {
+#ifdef CONFIG_GPIO_CDEV_V1

Quoted 5 lines

drivers/gpio/gpiolib-cdev.c static void gpio_desc_to_lineinfo(struct gpio_desc *desc, · line 1319

Quoted 2 lines

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

+               if (lineinfo_ensure_abi_version(cdev, 2))
+                       return -EPERM;

Can't you propagate error code from the function?

You mean:

(quoted diff) +3 -0

+               ret = lineinfo_ensure_abi_version(cdev, 2)
+               if (ret)
+                       return ret;

That seems more verbose and less clear. And I'd need to conditionally declare a ret - as this test is compiled out if CDEV_V1 is not defined.

I did flip-flop on what lineinfo_ensure_abi_version() should return - either a bool or an error code.

If a bool then the code would include the dreaded negative conditional ;-(:

(quoted diff) +2 -0

+               if (!lineinfo_is_abi_version(cdev, 2))
+                       return -EPERM;

so I eventually settled for the error code. But I'm on the fence on this one and happy to change it if you think the bool form is clearer.

Cheers,
Kent.

Quoted 2 lines · Kent Gibson

Quoted 1 line · Andy Shevchenko

On Tue, Sep 22, 2020 at 5:35 AM Kent Gibson <[email protected]> wrote:

...

Quoted 9 lines

Quoted 9 lines

Quoted 8 lines

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

+static int lineinfo_ensure_abi_version(struct gpio_chardev_data *cdata,
+                                      unsigned int version)
+{
 
+       int abiv = atomic_read(&cdata->watch_abi_version);
+
+       if (abiv == 0) {

Quoted 6 lines

Quoted 4 lines

drivers/gpio/gpiolib-cdev.c static void gpio_desc_to_lineinfo(struct gpio_desc *desc, · line 1292

Quoted 2 lines

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

+               atomic_cmpxchg(&cdata->watch_abi_version, 0, version);
+               abiv = atomic_read(&cdata->watch_abi_version);

atomic_cmpxchng() returns a value.

Yep, it returns the old value - which we don't care about - see below.

Then what's the point to read back?..

Quoted 18 lines

Quoted 2 lines

Also there are no barriers here...

No barriers required - the atomic_cmpxchg() is sufficient.

Quoted 6 lines

drivers/gpio/gpiolib-cdev.c static void gpio_desc_to_lineinfo(struct gpio_desc *desc, · line 1295

Quoted 3 lines

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

+       }
+       if (abiv != version)
+               return -EPERM;

I'm not sure I understand why this is atomic.

The algorithm requires some level of protection and atomic is sufficient.

Quoted 2 lines

Also this seems to be racy if cdata changed in background.