Re: [PATCH 13/22] gpio: uapi: define uAPI V2

[Kent Gibson <warthog618@gmail.com>]

On Wed, Jun 24, 2020 at 05:33:26PM +0300, Andy Shevchenko wrote:
> 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.
> 

Fair enough.  And you don't need to point out every single case - a
catch all is sufficient.

[ snip ]
> 
> > +#include <linux/kernel.h>
> 
> Perhaps keep it in order?
> 

Haha, I didn't notice that one.

> ...
> 
> > + * Must be a multiple of 8 to ensure 32/64bit alignment of structs.
> 
> Dash. And same for all cases like this.
> 

And you are repeating yourself.  Again ;-).

> ...
> 
> > +/* 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.
> 

We are talking about include/uapi/linux/kernel.h, right?
I thought headers in uapi/linux were fair game for other uapi headers.
That is what they are there for right?

Similarly, I thought those macros were there to avoid having the recalc
manually.

> ...
> 
> > +/*
> > + * 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?
> 

Ironically they are there as you wanted the padding sizes, and their
impact on alignment, documented and it made sense to me to group them
like this.

> ...
> 
> > +/**
> > + * 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.
> 

That is doable, but gets complicated when you have embedded structs, as
this one is in gpioline_request and gpioline_info_v2.

To keep decoding simple on the kernel side, we would have to explode all
the struct definitions so new fields are always added to the end.
Or we would end up with rather complex decoding on the kernel side.

We can always use that as a fallback if we run out of padding.

> > +};
> 
> 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.
> 

I agree in general, but not sure where to draw the line (pun not
indended).
Or how to provide a coherent API at a higher layer for that matter.

Do you have a concrete use case you need a solution for?

> ...
> 
> > +/*
> > + *  ABI V1
> 
> V1 -> v1
> 
> And below V2 -> v2.
> 

Fair enough.

Cheers,
Kent.