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

From: Kent Gibson
Date: Sun Sep 20 2020 - 07:12:12 EST


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.