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

[Kent Gibson]

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@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.
> 
> ...
> 

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.