Re: [PATCH v9 07/20] gpiolib: cdev: support GPIO_V2_GET_LINE_IOCTL and GPIO_V2_LINE_GET_VALUES_IOCTL

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Tue, Sep 22, 2020 at 5:35 AM Kent Gibson <warthog618@xxxxxxxxx> 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