[PATCH v2 4/6] Add GPIO v2 ioctl decoding

[Kent Gibson]

On Wed, Jan 20, 2021 at 07:15:26PM +0300, Dmitry V. Levin wrote:
> On Wed, Jan 20, 2021 at 07:32:49PM +0800, Kent Gibson wrote:
> > On Wed, Jan 20, 2021 at 01:43:15PM +0300, Dmitry V. Levin wrote:
> [...]
> > > I suppose it's still worth printing the structure in
> > > print_gpio_v2_line_attribute even if the padding is non-zero, this would
> > > be consistent with print_gpio_v2_line_config, print_gpio_v2_line_info,
> > > and print_gpio_v2_line_request.
> > 
> > Not sure what you mean.  The line info differs from the other two as the
> > struct gpio_v2_line_config_attribute includes the mask field.  There is
> > structure wrapping around that, but not the attribute itself.  For the
> > line info there is just the list of attributes associated with the line
> > listed in the attrs array.
> 
> OK, let's have a look at examples.
> 
> This is a line from tests/ioctl_gpio-success.dir/log:
> 
> ioctl(-1, GPIO_V2_GET_LINEINFO_IOCTL, {offset=56} => {name="line name", consumer="line consumer", flags=GPIO_V2_LINE_FLAG_ACTIVE_LOW|GPIO_V2_LINE_FLAG_BIAS_PULL_UP, num_attrs=1, attrs=[{padding=0xfeedface, debounce_period_us=3735928559}]}) = 42 (INJECTED)
> 
> Everything looks as expected.

That is the non-zero padding case, so it gets the raw struct decoding
treatment.  That is an exceptional case, not the normal.

Btw, my current decoding for that line is:

ioctl(-1, GPIO_V2_GET_LINEINFO_IOCTL, {offset=56} => {name="line name", consumer="line consumer", flags=GPIO_V2_LINE_FLAG_ACTIVE_LOW|GPIO_V2_LINE_FLAG_BIAS_PULL_UP, num_attrs=1, attrs=[{id=2, padding=0xfeedface, data=0xdeadbeefba11c0da}]}) = 42 (INJECTED)

as it no longer assumes it knows how to decode the struct if the padding
is non-zero.

> This is another line from the same file:
> 
> ioctl(-1, GPIO_V2_GET_LINEINFO_IOCTL, {offset=57} => {name="line name", consumer="line consumer", flags=GPIO_V2_LINE_FLAG_ACTIVE_LOW|GPIO_V2_LINE_FLAG_BIAS_PULL_UP, num_attrs=11, attrs=[flags=0xafeddeadd0d00000 /* GPIO_V2_LINE_FLAG_??? */, values=0xafeddeadd0d00001, debounce_period_us=3503292418, (id=4)=0xafeddeadd0d00003, (id=5)=0xafeddeadd0d00004, (id=6)=0xafeddeadd0d00005, (id=7)=0xafeddeadd0d00006, (id=8)=0xafeddeadd0d00007, (id=9)=0xafeddeadd0d00008, (id=10)=0xafeddeadd0d00009]}) = 42 (INJECTED)
> 
> Here you can see this "attrs=[flags=...", but since "attrs" is an array,
> it cannot have named fields as a structure or a union.
> 

Yeah, I was going for the logical struct encoded in an array feel here.
If you want that encoded in a more strict C-style then the elements will
require structure wrapping.

My current decoding is:

ioctl(-1, GPIO_V2_GET_LINEINFO_IOCTL, {offset=57} => {name="line name", consumer="line consumer", flags=GPIO_V2_LINE_FLAG_ACTIVE_LOW|GPIO_V2_LINE_FLAG_BIAS_PULL_UP, num_attrs=11, attrs=[flags=0xafeddeadd0d00000 /* GPIO_V2_LINE_FLAG_??? */, values=0xafeddeadd0d00001, debounce_period_us=3503292418, {id=4, data=0xafeddeadd0d00003}, {id=5, data=0xafeddeadd0d00004}, {id=6, data=0xafeddeadd0d00005}, {id=7, data=0xafeddeadd0d00006}, {id=8, data=0xafeddeadd0d00007}, {id=9, data=0xafeddeadd0d00008}, {id=10, data=0xafeddeadd0d00009}]}) = 42 (INJECTED)

but I'll add the structure wrapping for the flag, values and
debounce_period_us as well.

> Also, print_gpio_v2_line_config_attribute prints
> struct gpio_v2_line_config_attribute.attr field without printing its name
> (there is no attr= printed).
> 

So this one:

ioctl(-1, GPIO_V2_LINE_SET_CONFIG_IOCTL, {flags=GPIO_V2_LINE_FLAG_ACTIVE_LOW|GPIO_V2_LINE_FLAG_BIAS_PULL_UP, num_attrs=11, attrs=[{flags=GPIO_V2_LINE_FLAG_ACTIVE_LOW|GPIO_V2_LINE_FLAG_BIAS_PULL_UP, mask=0x1}, {flags=GPIO_V2_LINE_FLAG_ACTIVE_LOW|GPIO_V2_LINE_FLAG_BIAS_PULL_UP, mask=0x3}, {flags=GPIO_V2_LINE_FLAG_ACTIVE_LOW|GPIO_V2_LINE_FLAG_BIAS_PULL_UP, mask=0x5}, {flags=GPIO_V2_LINE_FLAG_ACTIVE_LOW|GPIO_V2_LINE_FLAG_BIAS_PULL_UP, mask=0x7}, {flags=GPIO_V2_LINE_FLAG_ACTIVE_LOW|GPIO_V2_LINE_FLAG_BIAS_PULL_UP, mask=0x9}, {flags=GPIO_V2_LINE_FLAG_ACTIVE_LOW|GPIO_V2_LINE_FLAG_BIAS_PULL_UP, mask=0xb}, {flags=GPIO_V2_LINE_FLAG_ACTIVE_LOW|GPIO_V2_LINE_FLAG_BIAS_PULL_UP, mask=0xd}, {flags=GPIO_V2_LINE_FLAG_ACTIVE_LOW|GPIO_V2_LINE_FLAG_BIAS_PULL_UP, mask=0xf}, {flags=GPIO_V2_LINE_FLAG_ACTIVE_LOW|GPIO_V2_LINE_FLAG_BIAS_PULL_UP, mask=0x11}, {flags=GPIO_V2_LINE_FLAG_ACTIVE_LOW|GPIO_V2_LINE_FLAG_BIAS_PULL_UP, mask=0x13}]}) = 42 (INJECTED)

That is intentional.  As mentioned earlier, the line attribute usually
resolves to a single field struct (in the example here they are all flags),
so you can skip over a level for brevity.  Again, I'm trying to capture
the information that the API was trying to convey, not the precise
implementation details.

Decoding each element as
"{attr={flags=GPIO_V2_LINE_FLAG_ACTIVE_LOW|GPIO_V2_LINE_FLAG_BIAS_PULL_UP},
mask=0xb}"

seems pointlessly noisy to me.
Though it is probably appropriate where the attr is decoded raw, e.g.
the non-zero padding case. i.e.
"{attr={id=2, padding=0xfeedface, data=0xdeadbeefba11c0da}, mask=0xb}"

Does that make sense?

Cheers,
Kent.