lkml.org 
[lkml]   [2020]   [Sep]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [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

\
 
 \ /
  Last update: 2020-09-23 13:13    [W:0.176 / U:5.356 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site