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

[Kent Gibson]

On Wed, Jan 20, 2021 at 01:43:15PM +0300, Dmitry V. Levin wrote:
> On Wed, Jan 20, 2021 at 03:34:34PM +0800, Kent Gibson wrote:
> > On Tue, Jan 19, 2021 at 11:14:32PM +0300, Dmitry V. Levin wrote:
> > > On Mon, Jan 11, 2021 at 11:09:10PM +0800, Kent Gibson wrote:
> > > > Add decoding of GPIO uAPI v2 ioctls added in Linux v5.10.
> > > > 
> > > > Signed-off-by: Kent Gibson <warthog618 at gmail.com>
> > > > ---
> > [snip]
> > 
> > > > +
> > > > +static void
> > > > +print_gpio_v2_line_attribute(struct tcb *const tcp,
> > > > +			     struct gpio_v2_line_attribute *attr)
> > > > +{
> > > > +	switch (attr->id) {
> > > > +	case GPIO_V2_LINE_ATTR_ID_FLAGS:
> > > > +		PRINT_FIELD_FLAGS("", *attr, flags, gpio_v2_line_flags,
> > > > +				  "GPIO_V2_LINE_FLAG_???");
> > > > +		break;
> > > > +	case GPIO_V2_LINE_ATTR_ID_OUTPUT_VALUES:
> > > > +		PRINT_FIELD_X("", *attr, values);
> > > > +		break;
> > > > +	case GPIO_V2_LINE_ATTR_ID_DEBOUNCE:
> > > > +		PRINT_FIELD_U("", *attr, debounce_period_us);
> > > > +		break;
> 
> [...]
> > > > +	default:
> > > > +		tprintf("(id=%u)=%#" PRIx64, attr->id, (uint64_t)attr->values);
> > > > +		break;
> > > 
> > > We already discussed this before, but I'm still not quite happy with the
> > > output because (id=%u)=%#x is very odd looking thing indeed, it doesn't
> > > look similar to any valid C construct I'm aware of, and we strive to
> > > produce a C-like output.
> > > 
> > > What if we invent a non-exiting field and print it instead?  For example,
> > > 
> > > 	PRINT_FIELD_U("", *attr, id);
> > > #define unknown_data values
> > > 	PRINT_FIELD_X("", *attr, unknown_data);
> > > #undef unknown_data
> > > 
> > 
> > I'm ok with effectively decoding it raw, e.g. 
> > "{id=42, data=0x1234567812345678}", but I'm uncomfortable with the #define
> > and #undef, particularly if I were to call the field "data".
> > So I would lean towards a tprintf for this one:
> > 
> > 		PRINT_FIELD_U("{", *attr, id);
> > 		tprintf(", data=%#" PRIx64 "}", attr->values);
> > 
> > unless there is some other helper function that could do the same
> > without requiring preprocessor gymnastics.
> > 
> > Or are you trying to replace all instances of tprintf with a PRINT_xxx
> > variant?
> 
> Yes, we are trying to replace all tprintf instances, but we don't have to
> replace all of them right now, so let it remain tprintf here for a while.
> 
> You can add a short comment in the code telling why the field is printed
> with a different name.
> 
> [...]
> > > > +static bool
> > > > +print_gpio_v2_line_attr_array_member(struct tcb *tcp, void *elem_buf,
> > > > +				     size_t elem_size, void *data)
> > > > +{
> > > > +	struct gpio_v2_line_attribute *la;
> > > > +
> > > > +	la = (struct gpio_v2_line_attribute *)elem_buf;
> > > > +	if (la->padding) {
> > > > +		PRINT_FIELD_X("{", *la, padding);
> > > > +		tprints(", ");
> > > > +	}
> > > > +	print_gpio_v2_line_attribute(tcp, la);
> > > > +	if (la->padding)
> > > > +		tprints("}");
> > > > +
> > > > +	return true;
> > > > +}
> > > 
> > > If struct gpio_v2_line_attribute is a structure regardless of the value
> > > stored in la->padding, then I suggest to print "{" and "}"
> > > unconditionally.
> > 
> > Logically, the array of line attributes is equivalent to a struct.
> > Each element in the array corresponds to one field in the logical struct.
> > The uAPI is written that way to allow the line configuration to be
> > flexible - only the fields of interest need to be passed, and new fields
> > can be defined in the future without breaking the ABI. 
> > Each struct gpio_v2_line_attribute is decoded as a single field=value,
> > based on the id field.  It is only decoded as a struct if the id is
> > unknown to the decoder - see the comment above.
> > 
> > The padding should be zeroed.  If not then it probably means the
> > protocol has been extended with fields this decoder doesn't understand,
> > and so how about we also fallback to raw decoding in this case, so
> > something like "{id=42, padding=0x12345678, data=0x8765432112345678}"?
> > 
> > The padding being non-zero may also mean the user doesn't understand
> > the protocol - which requires the padding to be zero or the request will
> > be rejected by the kernel - and I'm ok with decoding to a raw struct in
> > that case as well.
> 
> If this is the case, then I suggest turning
> print_gpio_v2_line_attr_array_member into a trivial wrapper around
> print_gpio_v2_line_attribute, similar to its neighbour
> print_gpio_v2_line_config_attr_array_member being a trivial wrapper around
> print_gpio_v2_line_config_attribute.
> 

Agreed - and already done in my working tree.

> 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.


Also working on a types header, but not sure how the types directory works,
or what its purpose is.
If I just drop my header in there gen.sh complains 

./types/gpio.h: no types found

so I'm not sure what it is expecting.
I'll keep digging, but any pointers you could give would be appreciated.

Thanks,
Kent.