Messages in this thread | | | From | Andy Shevchenko <> | Date | Wed, 23 Sep 2020 14:11:54 +0300 | Subject | Re: [PATCH v9 07/20] gpiolib: cdev: support GPIO_V2_GET_LINE_IOCTL and GPIO_V2_LINE_GET_VALUES_IOCTL |
| |
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...
-- With Best Regards, Andy Shevchenko
| |