Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200623040107.22270-1-warthog618@gmail.com> <20200623040107.22270-14-warthog618@gmail.com>
In-Reply-To: <20200623040107.22270-14-warthog618@gmail.com>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Wed, 24 Jun 2020 17:33:26 +0300
Message-ID: <CAHp75VcEDnrQk5FeWTZdV3fMnTsLpnmy+hAnL4V3a0Ge0NEe2A@mail.gmail.com>
Subject: Re: [PATCH 13/22] gpio: uapi: define uAPI V2
To:     Kent Gibson <warthog618@gmail.com>
Cc:     Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
        Bartosz Golaszewski <bgolaszewski@baylibre.com>,
        Linus Walleij <linus.walleij@linaro.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Jun 23, 2020 at 7:04 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> Add a new version of the uAPI to address existing 32/64bit alignment

I think using - would be nice, like 32/64-bit (or at least space like
32/64 bit) as a common practice.

> issues, add support for debounce and event sequence numbers, and provide
> some future proofing by adding padding reserved for future use.
>
> The alignment issue relates to the gpioevent_data, which packs to different
> sizes on 32bit and 64bit platforms. That creates problems for 32bit apps
> running on 64bit kernels.

Dashes, please.

$ git grep -n -w '32 bit' | wc
  1904
$ git grep -n -w '64 bit' | wc
  1155

~3k

$ git grep -n -w '32bit' | wc
  1405
$ git grep -n -w '64bit' | wc
   870

~2.2k

$ git grep -n -w '64-bit' | wc
  3196
$ git grep -n -w '32-bit' | wc
  4369

~7.5k

> The patch addresses that particular issue, and
> the problem more generally, by adding pad fields that explicitly pad
> structs out to 64bit boundaries, so they will pack to the same size now,
> and even if some of the reserved padding is used for __u64 fields in the
> future.
>
> The lack of future proofing in V1 makes it impossible to, for example,
> add the debounce feature that is included in v2.
> The future proofing is addressed by providing reserved padding in all
> structs for future features.  Specifically, the line request,
> config, info, info_changed and event structs receive updated versions,
> and the first three new ioctls.

...

> Firstly, I've reworked the flags field throughout.  V1 has three different
> flags fields, each with their own separate bit definitions.  In V2 that is
> collapsed to one.  Further, the bits of the V2 flags field are used
> as feature enable flags, with any other necessary configuration fields encoded
> separately.  This is simpler and clearer, while also providing a foundation
> for adding features in the future.
>
> I've also merged the handle and event requests into a single request, the
> line request, as the two requests where mostly the same, other than the

where-> were ? (yes I noticed it's not a part of commit message)

> edge detection provided by event requests.  As a byproduct, the V2 uAPI
> allows for multiple lines producing edge events on the same line handle.
> This is a new capability as V1 only supports a single line in an event
> request.
>
> This means there are now only two types of file handle to be concerned with,
> the chip and the line, and it is clearer which ioctls apply to which type
> of handle.
>
> There is also some minor renaming of fields for consistency compared to their
> V1 counterparts, e.g. offset rather than lineoffset or line_offset, and
> consumer rather than consumer_label.
>
> Additionally, V1 GPIOHANDLES_MAX becomes GPIOLINES_MAX in V2 for clarity,
> and the gpiohandle_data __u8 array becomes a bitmap gpioline_values.
>
> The V2 uAPI is mostly just a reorganisation of V1, so userspace code,
> particularly libgpiod, should easily port to it.
>
> The padding sizes have been carried over from the RFC version, although the
> seqnos added to the gpioline_event alone would've used the all of the padding
> for that struct, had they not been added here.  So I'm moderatly concerned
> that those values are too small due to a lack of imagination on may part and
> should be increased to decrease the probability of running out of space in
> the padding and requiring creative solutions or even a V3.

...

> +#include <linux/kernel.h>

Perhaps keep it in order?

...

> + * Must be a multiple of 8 to ensure 32/64bit alignment of structs.

Dash. And same for all cases like this.

...

> +/* the number of __u64 required for a bitmap for GPIOLINES_MAX lines */

the -> The ?

> +#define GPIOLINES_BITMAP_SIZE  __KERNEL_DIV_ROUND_UP(GPIOLINES_MAX, 64)

Not sure we need this definition. The problem is that definitions in
the uAPI header are also part of uAPI. Here is just a calculus which
can be done manually since if anybody changes MAX, they will anyway
have to check if everything else is consistent. And yes, I'm not in
favour of including kernel.h here and there.

...

> +/*
> + * Struct padding sizes.
> + *
> + * These are sized to pad structs to 64bit boundaries.
> + * To maintain 32/64bit alignment, any arbitrary change must be even, as
> + * the pad elements are __u32.
> + */
> +#define GPIOLINE_CONFIG_PAD_SIZE               7
> +#define GPIOLINE_REQUEST_PAD_SIZE              5
> +#define GPIOLINE_INFO_V2_PAD_SIZE              5
> +#define GPIOLINE_INFO_CHANGED_V2_PAD_SIZE      5
> +#define GPIOLINE_EVENT_PAD_SIZE                        2

I'm not sure they (definitions) should be part of UAPI. Can't you
simple hard code numbers per case?

...

> +/**
> + * struct gpioline_config - Configuration for GPIO lines
> + */
> +struct gpioline_config {
> +       struct gpioline_values values;
> +       __u32 flags;
> +       /* Note that the following four fields are equivalent to a single u32. */
> +       __u8 direction;
> +       __u8 drive;
> +       __u8 bias;
> +       __u8 edge_detection;
> +       __u32 debounce_period;

> +       __u32 padding[GPIOLINE_CONFIG_PAD_SIZE]; /* for future use */

I would just put minimum here. If you need to extend you have to use
sizeof(struct) as a version of ABI.

> +};

I'm wondering how many lines (in average) the user usually changes at
once? One? Two?

Perhaps we need to be better with this, something like single line /
multiple lines?

So, having a struct for single line change being embedded multiple
times would allow to configure atomically several lines with different
requirements.
For example you can turn directions of the two lines for some kind of
half-duplex bit banging protocol.

I'm not sure about the rest, but to me it seems reasonable to have
single vs. multiple separation in some of the structures.

...

> +/*
> + *  ABI V1

V1 -> v1

And below V2 -> v2.

> + */

-- 
With Best Regards,
Andy Shevchenko