| Message ID | 20200922023151.387447-8-warthog618@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: cdev: add uAPI v2 | expand |
On Tue, Sep 22, 2020 at 5:35 AM Kent Gibson <warthog618@gmail.com> wrote: > > 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. ... > + /* > + * 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. > + * the hardware actually supports enabling both at the same time the > + * electrical result would be disastrous. > + */ ... > + /* 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. ... > + /* 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) > + 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; ... > +static void gpio_v2_line_config_flags_to_desc_flags(u64 flags, > + unsigned long *flagsp) > +{ > + 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? > + 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); > +} ... > +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; > + /* 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... > + 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() { ... } } > + 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; > + > + 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... > + if (copy_to_user(ip, &lv, sizeof(lv))) > + return -EFAULT; > + > + return 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...
On Wed, Sep 23, 2020 at 02:11:54PM +0300, Andy Shevchenko wrote: > On Tue, Sep 22, 2020 at 5:35 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > 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. > > ... > Ooops, nearly missed this one... > > + /* > > + * 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. > Understand these are pulled directly from the v1 implementation, so I think that is actually one of Bart's. But, yeah, it should be 'and'. > > + * the hardware actually supports enabling both at the same time the > > + * electrical result would be disastrous. > > + */ > > ... > > > + /* 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. > ... > Again, this is drawn directly from the v1 implementation, and I didn't go changing anything from v1 without good reason. > > + /* 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) > > > + 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; > > ... > > > +static void gpio_v2_line_config_flags_to_desc_flags(u64 flags, > > + unsigned long *flagsp) > > +{ > > > + 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? > > + 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); > > +} > > ... > > > +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; > > > + /* 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... > > > + 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() { > ... > } > } > As per elsewhere - will give it a shot. > > + 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; > > + > > > + 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... > > > + if (copy_to_user(ip, &lv, sizeof(lv))) > > + return -EFAULT; > > + > > + return 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... > 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. Cheers, Kent.
On Wed, Sep 23, 2020 at 1:12 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > [snip!] > > > + /* 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. > ... > > > + /* 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) > This is an abstraction layer. Only because some controllers allow this, doesn't mean we should reflect this in our abstraction layer - especially if we know well that this has no purpose. Bartosz
On Thu, Sep 24, 2020 at 11:09 AM Kent Gibson <warthog618@gmail.com> wrote: > On Wed, Sep 23, 2020 at 02:11:54PM +0300, Andy Shevchenko wrote: > > On Tue, Sep 22, 2020 at 5:35 AM Kent Gibson <warthog618@gmail.com> wrote: ... > > > + 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? I'm not sure. I think if you are using non-atomic in one place, it means that all automatically drop the atomicity guarantee. So, it's all or none for atomicity, for non-atomicity it's rather none or at least one. That said, code should be carefully checked before doing such. > > > + 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); ... > > > + /* 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... > > > > 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. 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: > On Thu, Sep 24, 2020 at 11:09 AM Kent Gibson <warthog618@gmail.com> wrote: > > On Wed, Sep 23, 2020 at 02:11:54PM +0300, Andy Shevchenko wrote: > > > On Tue, Sep 22, 2020 at 5:35 AM Kent Gibson <warthog618@gmail.com> wrote: > ... > > > > + /* 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... > > > > > > > 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. > > 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. > 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?? Cheers, Kent.
On Sat, Sep 26, 2020 at 12:16 PM Kent Gibson <warthog618@gmail.com> wrote: > 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?? I really don't want to delay this series, choose what you think is better and we may amend it later.
On Sun, Sep 27, 2020 at 12:00:04PM +0300, Andy Shevchenko wrote: > On Sat, Sep 26, 2020 at 12:16 PM Kent Gibson <warthog618@gmail.com> wrote: > > 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?? > > I really don't want to delay this series, choose what you think is > better and we may amend it later. > OK, as this code is common with v1 I'll leave it as is - we can always change it for all cases in a later patch. I think that is everything outstanding for v9, so should have a v10 out shortly. Cheers, Kent.
diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c index 86679397d09c..7a3ed2617f74 100644 --- a/drivers/gpio/gpiolib-cdev.c +++ b/drivers/gpio/gpiolib-cdev.c @@ -1,7 +1,9 @@ // SPDX-License-Identifier: GPL-2.0 #include <linux/anon_inodes.h> +#include <linux/atomic.h> #include <linux/bitmap.h> +#include <linux/build_bug.h> #include <linux/cdev.h> #include <linux/compat.h> #include <linux/device.h> @@ -24,6 +26,25 @@ #include "gpiolib.h" #include "gpiolib-cdev.h" +/* + * Array sizes must ensure 64-bit alignment and not create holes in the + * struct packing. + */ +static_assert(IS_ALIGNED(GPIO_V2_LINES_MAX, 2)); +static_assert(IS_ALIGNED(GPIO_MAX_NAME_SIZE, 8)); + +/* + * Check that uAPI structs are 64-bit aligned for 32/64-bit compatibility + */ +static_assert(IS_ALIGNED(sizeof(struct gpio_v2_line_attribute), 8)); +static_assert(IS_ALIGNED(sizeof(struct gpio_v2_line_config_attribute), 8)); +static_assert(IS_ALIGNED(sizeof(struct gpio_v2_line_config), 8)); +static_assert(IS_ALIGNED(sizeof(struct gpio_v2_line_request), 8)); +static_assert(IS_ALIGNED(sizeof(struct gpio_v2_line_info), 8)); +static_assert(IS_ALIGNED(sizeof(struct gpio_v2_line_info_changed), 8)); +static_assert(IS_ALIGNED(sizeof(struct gpio_v2_line_event), 8)); +static_assert(IS_ALIGNED(sizeof(struct gpio_v2_line_values), 8)); + /* Character device interface to GPIO. * * The GPIO character device, /dev/gpiochipN, provides userspace an @@ -34,6 +55,7 @@ * GPIO line handle management */ +#ifdef CONFIG_GPIO_CDEV_V1 /** * struct linehandle_state - contains the state of a userspace handle * @gdev: the GPIO device the handle pertains to @@ -376,6 +398,400 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip) linehandle_free(lh); return ret; } +#endif /* CONFIG_GPIO_CDEV_V1 */ + +/** + * struct line - contains the state of a requested line + * @desc: the GPIO descriptor for this line. + */ +struct line { + struct gpio_desc *desc; +}; + +/** + * struct linereq - contains the state of a userspace line request + * @gdev: the GPIO device the line request pertains to + * @label: consumer label used to tag GPIO descriptors + * @num_lines: the number of lines in the lines array + * @lines: the lines held by this line request, with @num_lines elements. + */ +struct linereq { + struct gpio_device *gdev; + const char *label; + u32 num_lines; + struct line lines[]; +}; + +#define GPIO_V2_LINE_BIAS_FLAGS \ + (GPIO_V2_LINE_FLAG_BIAS_PULL_UP | \ + GPIO_V2_LINE_FLAG_BIAS_PULL_DOWN | \ + GPIO_V2_LINE_FLAG_BIAS_DISABLED) + +#define GPIO_V2_LINE_DIRECTION_FLAGS \ + (GPIO_V2_LINE_FLAG_INPUT | \ + GPIO_V2_LINE_FLAG_OUTPUT) + +#define GPIO_V2_LINE_DRIVE_FLAGS \ + (GPIO_V2_LINE_FLAG_OPEN_DRAIN | \ + GPIO_V2_LINE_FLAG_OPEN_SOURCE) + +#define GPIO_V2_LINE_VALID_FLAGS \ + (GPIO_V2_LINE_FLAG_ACTIVE_LOW | \ + GPIO_V2_LINE_DIRECTION_FLAGS | \ + GPIO_V2_LINE_DRIVE_FLAGS | \ + GPIO_V2_LINE_BIAS_FLAGS) + +static u64 gpio_v2_line_config_flags(struct gpio_v2_line_config *lc, + unsigned int line_idx) +{ + unsigned int i; + u64 mask = BIT_ULL(line_idx); + + for (i = 0; i < lc->num_attrs; i++) { + if ((lc->attrs[i].attr.id == GPIO_V2_LINE_ATTR_ID_FLAGS) && + (lc->attrs[i].mask & mask)) + return lc->attrs[i].attr.flags; + } + return lc->flags; +} + +static int gpio_v2_line_config_output_value(struct gpio_v2_line_config *lc, + unsigned int line_idx) +{ + unsigned int i; + u64 mask = BIT_ULL(line_idx); + + for (i = 0; i < lc->num_attrs; i++) { + if ((lc->attrs[i].attr.id == GPIO_V2_LINE_ATTR_ID_OUTPUT_VALUES) && + (lc->attrs[i].mask & mask)) + return !!(lc->attrs[i].attr.values & mask); + } + return 0; +} + +static int gpio_v2_line_flags_validate(u64 flags) +{ + /* Return an error if an unknown flag is set */ + if (flags & ~GPIO_V2_LINE_VALID_FLAGS) + return -EINVAL; + + /* + * Do not allow both INPUT & OUTPUT flags to be set as they are + * contradictory. + */ + if ((flags & GPIO_V2_LINE_FLAG_INPUT) && + (flags & GPIO_V2_LINE_FLAG_OUTPUT)) + return -EINVAL; + + /* + * Do not allow OPEN_SOURCE & OPEN_DRAIN flags in a single request. If + * the hardware actually supports enabling both at the same time the + * electrical result would be disastrous. + */ + if ((flags & GPIO_V2_LINE_FLAG_OPEN_DRAIN) && + (flags & GPIO_V2_LINE_FLAG_OPEN_SOURCE)) + return -EINVAL; + + /* Drive requires explicit output direction. */ + if ((flags & GPIO_V2_LINE_DRIVE_FLAGS) && + !(flags & GPIO_V2_LINE_FLAG_OUTPUT)) + return -EINVAL; + + /* Bias requires explicit direction. */ + if ((flags & GPIO_V2_LINE_BIAS_FLAGS) && + !(flags & GPIO_V2_LINE_DIRECTION_FLAGS)) + return -EINVAL; + + /* Only one bias flag can be set. */ + 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; + + return 0; +} + +static int gpio_v2_line_config_validate(struct gpio_v2_line_config *lc, + unsigned int num_lines) +{ + unsigned int i; + u64 flags; + int ret; + + if (lc->num_attrs > GPIO_V2_LINE_NUM_ATTRS_MAX) + return -EINVAL; + + if (memchr_inv(lc->padding, 0, sizeof(lc->padding))) + return -EINVAL; + + for (i = 0; i < num_lines; i++) { + flags = gpio_v2_line_config_flags(lc, i); + ret = gpio_v2_line_flags_validate(flags); + if (ret) + return ret; + } + return 0; +} + +static void gpio_v2_line_config_flags_to_desc_flags(u64 flags, + unsigned long *flagsp) +{ + assign_bit(FLAG_ACTIVE_LOW, flagsp, + flags & GPIO_V2_LINE_FLAG_ACTIVE_LOW); + + 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); +} + +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; + + /* 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; + } + } + + if (num_get == 0) + return -EINVAL; + + if (num_get != 1) { + 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; + + 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++; + } + } + + if (copy_to_user(ip, &lv, sizeof(lv))) + return -EFAULT; + + return 0; +} + +static long linereq_ioctl(struct file *file, unsigned int cmd, + unsigned long arg) +{ + struct linereq *lr = file->private_data; + void __user *ip = (void __user *)arg; + + if (cmd == GPIO_V2_LINE_GET_VALUES_IOCTL) + return linereq_get_values(lr, ip); + + return -EINVAL; +} + +#ifdef CONFIG_COMPAT +static long linereq_ioctl_compat(struct file *file, unsigned int cmd, + unsigned long arg) +{ + return linereq_ioctl(file, cmd, (unsigned long)compat_ptr(arg)); +} +#endif + +static void linereq_free(struct linereq *lr) +{ + unsigned int i; + + for (i = 0; i < lr->num_lines; i++) { + if (lr->lines[i].desc) + gpiod_free(lr->lines[i].desc); + } + kfree(lr->label); + put_device(&lr->gdev->dev); + kfree(lr); +} + +static int linereq_release(struct inode *inode, struct file *file) +{ + struct linereq *lr = file->private_data; + + linereq_free(lr); + return 0; +} + +static const struct file_operations line_fileops = { + .release = linereq_release, + .owner = THIS_MODULE, + .llseek = noop_llseek, + .unlocked_ioctl = linereq_ioctl, +#ifdef CONFIG_COMPAT + .compat_ioctl = linereq_ioctl_compat, +#endif +}; + +static int linereq_create(struct gpio_device *gdev, void __user *ip) +{ + struct gpio_v2_line_request ulr; + struct gpio_v2_line_config *lc; + struct linereq *lr; + struct file *file; + u64 flags; + unsigned int i; + int fd, ret; + + if (copy_from_user(&ulr, ip, sizeof(ulr))) + return -EFAULT; + + if ((ulr.num_lines == 0) || (ulr.num_lines > GPIO_V2_LINES_MAX)) + return -EINVAL; + + if (memchr_inv(ulr.padding, 0, sizeof(ulr.padding))) + return -EINVAL; + + lc = &ulr.config; + ret = gpio_v2_line_config_validate(lc, ulr.num_lines); + if (ret) + return ret; + + lr = kzalloc(struct_size(lr, lines, ulr.num_lines), GFP_KERNEL); + if (!lr) + return -ENOMEM; + + lr->gdev = gdev; + get_device(&gdev->dev); + + /* 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; + } + } + + lr->num_lines = ulr.num_lines; + + /* Request each GPIO */ + for (i = 0; i < ulr.num_lines; i++) { + u32 offset = ulr.offsets[i]; + struct gpio_desc *desc = gpiochip_get_desc(gdev->chip, offset); + + if (IS_ERR(desc)) { + ret = PTR_ERR(desc); + goto out_free_linereq; + } + + ret = gpiod_request(desc, lr->label); + if (ret) + goto out_free_linereq; + + lr->lines[i].desc = desc; + flags = gpio_v2_line_config_flags(lc, i); + gpio_v2_line_config_flags_to_desc_flags(flags, &desc->flags); + + ret = gpiod_set_transitory(desc, false); + if (ret < 0) + goto out_free_linereq; + + /* + * Lines have to be requested explicitly for input + * or output, else the line will be treated "as is". + */ + if (flags & GPIO_V2_LINE_FLAG_OUTPUT) { + int val = gpio_v2_line_config_output_value(lc, i); + + ret = gpiod_direction_output(desc, val); + if (ret) + goto out_free_linereq; + } else if (flags & GPIO_V2_LINE_FLAG_INPUT) { + ret = gpiod_direction_input(desc); + if (ret) + goto out_free_linereq; + } + + blocking_notifier_call_chain(&desc->gdev->notifier, + GPIOLINE_CHANGED_REQUESTED, desc); + + dev_dbg(&gdev->dev, "registered chardev handle for line %d\n", + offset); + } + + fd = get_unused_fd_flags(O_RDONLY | O_CLOEXEC); + if (fd < 0) { + ret = fd; + goto out_free_linereq; + } + + file = anon_inode_getfile("gpio-line", &line_fileops, lr, + O_RDONLY | O_CLOEXEC); + if (IS_ERR(file)) { + ret = PTR_ERR(file); + goto out_put_unused_fd; + } + + ulr.fd = fd; + if (copy_to_user(ip, &ulr, sizeof(ulr))) { + /* + * fput() will trigger the release() callback, so do not go onto + * the regular error cleanup path here. + */ + fput(file); + put_unused_fd(fd); + return -EFAULT; + } + + fd_install(fd, file); + + dev_dbg(&gdev->dev, "registered chardev handle for %d lines\n", + lr->num_lines); + + return 0; + +out_put_unused_fd: + put_unused_fd(fd); +out_free_linereq: + linereq_free(lr); + return ret; +} + +#ifdef CONFIG_GPIO_CDEV_V1 /* * GPIO line event management @@ -745,6 +1161,8 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip) return ret; } +#endif /* CONFIG_GPIO_CDEV_V1 */ + static void gpio_desc_to_lineinfo(struct gpio_desc *desc, struct gpioline_info *info) { @@ -843,6 +1261,7 @@ static long gpio_ioctl(struct file *file, unsigned int cmd, unsigned long arg) if (copy_to_user(ip, &chipinfo, sizeof(chipinfo))) return -EFAULT; return 0; +#ifdef CONFIG_GPIO_CDEV_V1 } else if (cmd == GPIO_GET_LINEINFO_IOCTL) { struct gpioline_info lineinfo; @@ -885,6 +1304,9 @@ 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_LINE_IOCTL) { + return linereq_create(gdev, ip); } else if (cmd == GPIO_GET_LINEINFO_UNWATCH_IOCTL) { if (copy_from_user(&offset, ip, sizeof(offset))) return -EFAULT;
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. Signed-off-by: Kent Gibson <warthog618@gmail.com> --- The linereq_ioctl() is a simple wrapper around linereq_get_values() here, but will be extended with other ioctls in subsequent patches. Similarly, the struct line only contains the desc here, but will receive the edge detector and debouncer fields in subsequent patches. Changed for v8: - fix BUILD_BUG_ON conditions and relocate them before the return in gpiolib_cdev_register() Changes for v7: - add check on kmalloc_array return value Changes for v5: - as per cover letter Changes for v4: - fix handling of mask in line_get_values drivers/gpio/gpiolib-cdev.c | 422 ++++++++++++++++++++++++++++++++++++ 1 file changed, 422 insertions(+)