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

[Kent Gibson]

On Tue, Sep 15, 2020 at 01:31:27PM +0300, Andy Shevchenko wrote:
> On Sat, Sep 5, 2020 at 4:49 PM 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.
> >
> > Signed-off-by: Kent Gibson <warthog618@xxxxxxxxx>
> > ---
> >

[snip]
> 
> > +       /* Bias requires explicit direction. */
> > +       if ((flags & GPIO_V2_LINE_BIAS_FLAGS) &&
> > +           !(flags & GPIO_V2_LINE_DIRECTION_FLAGS))
> > +               return -EINVAL;
> 
> Why is this? If I request a line as is and after set a bias, should I
> really care about direction?
> 

Yeah, you probably should be aware of the direction if they are setting
the bias, so this makes sure they are.

The practical reason is that gpiod_direction_output() or
gpiod_direction_input() have to be called to set the bias, and they are
only called if the corresponding flag is set.

> 
> > +       for (num_get = 0, i = 0; i < lr->num_lines; i++) {
> > +               if (lv.mask & BIT_ULL(i)) {
> 
> for_each_set_bit() ?
> 

No - lv.mask is u64, not unsigned long.
You could do a cast, but it would break on BE-32.
Sound familar? - you caught me doing just that in your review of an earlier
version.

> > +       ulr.consumer[sizeof(ulr.consumer)-1] = '\0';
> > +       if (strlen(ulr.consumer)) {
> > +               lr->label = kstrdup(ulr.consumer, GFP_KERNEL);
> 
> Sounds like kstrndup()
> 

Been here before too - they differ slightly in that here lr->label is
left null if consumer is empty,  whereas kstrndup() would alloc one byte
just for the null terminator.

> > +               }
> > +
> > +               blocking_notifier_call_chain(&desc->gdev->notifier,
> > +                                            GPIOLINE_CHANGED_REQUESTED, desc);
> > +
> 
> > +               dev_dbg(&gdev->dev, "registered chardev handle for line %d\n",
> > +                       offset);
> 
> Hmm... I would rather see trace events / points than new dev_dbg() /
> pr_debug() calls.
> 

Agreed - it is on the TODO list.
I have looked at it and doing it properly would mean adding tracepoints
to gpiolib.c, and modifying the v1 code as well, so it is best done in a
separate patch later...

> > @@ -1104,6 +1505,25 @@ int gpiolib_cdev_register(struct gpio_device *gdev, dev_t devt)
> >                  MAJOR(devt), gdev->id);
> >
> >         return 0;
> > +       /*
> > +        * array sizes must ensure 64-bit alignment and not create holes in
> > +        * the struct packing.
> > +        */
> > +       BUILD_BUG_ON(IS_ALIGNED(GPIO_V2_LINES_MAX, 2));
> > +       BUILD_BUG_ON(IS_ALIGNED(GPIO_MAX_NAME_SIZE, 8));
> > +
> > +       /*
> > +        * check that uAPI structs are 64-bit aligned for 32/64-bit
> > +        * compatibility
> > +        */
> > +       BUILD_BUG_ON(IS_ALIGNED(sizeof(struct gpio_v2_line_attribute), 8));
> > +       BUILD_BUG_ON(IS_ALIGNED(sizeof(struct gpio_v2_line_config_attribute), 8));
> > +       BUILD_BUG_ON(IS_ALIGNED(sizeof(struct gpio_v2_line_config), 8));
> > +       BUILD_BUG_ON(IS_ALIGNED(sizeof(struct gpio_v2_line_request), 8));
> > +       BUILD_BUG_ON(IS_ALIGNED(sizeof(struct gpio_v2_line_info), 8));
> > +       BUILD_BUG_ON(IS_ALIGNED(sizeof(struct gpio_v2_line_info_changed), 8));
> > +       BUILD_BUG_ON(IS_ALIGNED(sizeof(struct gpio_v2_line_event), 8));
> > +       BUILD_BUG_ON(IS_ALIGNED(sizeof(struct gpio_v2_line_values), 8));
> 
> Can we use static_assert() at the top of the file? Presumably after
> inclusion block.
> 

Good idea - will do.

Cheers,
Kent.