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: <20200922023151.387447-1-warthog618@gmail.com> <20200922023151.387447-5-warthog618@gmail.com>
 <CAHp75VewJYDQ1Moi4jw=wbBMLNpaUGPgz+AsPjNdZqtHCgkjwA@mail.gmail.com>
 <20200923103031.GA579645@sol> <CAHp75VdQ4qWe-870QyH-E42=518vkbLSueQWQOs44M-qKudRsw@mail.gmail.com>
In-Reply-To: <CAHp75VdQ4qWe-870QyH-E42=518vkbLSueQWQOs44M-qKudRsw@mail.gmail.com>
From:   Arnd Bergmann <arnd@arndb.de>
Date:   Wed, 23 Sep 2020 14:18:53 +0200
Message-ID: <CAK8P3a3AzqLfnT_SLXQMbzXAnGXtDnYFFxFry-_vRiYQyXjMXQ@mail.gmail.com>
Subject: Re: [PATCH v9 04/20] gpio: uapi: define uAPI v2
To:     Andy Shevchenko <andy.shevchenko@gmail.com>
Cc:     Kent Gibson <warthog618@gmail.com>,
        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"
Precedence: bulk

On Wed, Sep 23, 2020 at 1:16 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Wed, Sep 23, 2020 at 1:30 PM Kent Gibson <warthog618@gmail.com> wrote:
> > On Wed, Sep 23, 2020 at 01:04:05PM +0300, Andy Shevchenko wrote:
> > > On Tue, Sep 22, 2020 at 5:34 AM Kent Gibson <warthog618@gmail.com> wrote:
> > > > 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 GPIO_V2_LINES_MAX in v2 for
> > > > clarity, and the gpiohandle_data __u8 array becomes a bitmap in
> > > > gpio_v2_line_values.
> > > >
> > > > The v2 uAPI is mostly a reorganisation and extension of v1, so userspace
> > > > code, particularly libgpiod, should readily port to it.
> > >
> > > ...
> > >
> > > > +struct gpio_v2_line_config {
> > > > +       __aligned_u64 flags;
> > > > +       __u32 num_attrs;
> > >
> > > > +       /* Pad to fill implicit padding and reserve space for future use. */
> > > > +       __u32 padding[5];
> > >
> > > Probably I somehow missed the answer, but why do we need 5 here and not 1?
> > >
> >
> > Sorry, I got tired of repeating myself, and just acked that we disagree
> > on the approach here.
> >
> > Your suggestion to use the size for version would result in an
> > explosion of ioctl signatures - every time we add a field we have to add
> > a new ioctl and handle it separately in gpio_ioctl() or linereq_ioctl().
>
> No, you just add
> __u32 version; // implies sizeof() check as well.
>
> Look for examples of existing ABIs (e.g. perf ABI).

Please don't ever add a version field to an ioctl structure, this
has been shown to cause more problems than it solves many
times in the past...

Having some reserved fields can be helpful, as long as the kernel
returns an error in case any of the unknown fields are nonzero.

I'd also prefer fewer than five reserved fields, but that is not as
important, just use as many as are likely to be used in the future,
but not more: It's easy to add one more ioctl command in case
the expectations are wrong, but I agree you wouldn't want a
new command every time another field gets added if the past
has shown that this happens a lot.

       Arnd