[PATCH 0/4] Add GPIO chardev ioctl decoding

[Kent Gibson]

On Wed, Dec 30, 2020 at 04:13:40AM +0300, Dmitry V. Levin wrote:
> On Wed, Dec 30, 2020 at 08:30:20AM +0800, Kent Gibson wrote:
> > On Tue, Dec 29, 2020 at 11:44:42PM +0300, Dmitry V. Levin wrote:
> > > On Wed, Dec 23, 2020 at 06:07:44AM +0800, Kent Gibson wrote:
> > > > This patch set implements decoding of the GPIO character device ioctls
> > > > first added in Linux v4.8, extended in v5.5 and v5.7, and the v2 ioctls
> > > > added in v5.10, and a minor update for v5.11.
> > > > 
> > > > The first patch adds a helper method used to print arrays of uint8,
> > > > as found in the GPIO API.
> > > > The second patch adds the decoding of the GPIO ioctls.
> > > > The third patch adds the decoding of the GPIO v2 ioctls.
> > > > The fourth patch adds decoding of the
> > > > GPIO_V2_LINE_FLAG_EVENT_CLOCK_REALTIME flag which is on track to be added
> > > > in Linux v5.11.
> > > > 
> > > > Kent Gibson (4):
> > > >   Add print_uint8_array_member() helper
> > > >   Add GPIO ioctl decoding
> > > >   Add GPIO uAPI v2 ioctl decoding
> > > >   Add GPIO uAPI realtime clock flag
> > > > 
> > > >  Makefile.am                   |   1 +
> > > >  configure.ac                  |   1 +
> > > >  defs.h                        |   4 +
> > > >  gpio_ioctl.c                  | 536 ++++++++++++++++++++++++++++++++++
> > > >  ioctl.c                       |   4 +
> > > >  util.c                        |  18 ++
> > > >  xlat/gpio_event_flags.in      |   3 +
> > > >  xlat/gpio_handle_flags.in     |   8 +
> > > >  xlat/gpio_line_flags.in       |   8 +
> > > >  xlat/gpio_v2_line_attr_ids.in |   5 +
> > > >  xlat/gpio_v2_line_flags.in    |  14 +
> > > >  11 files changed, 602 insertions(+)
> > > >  create mode 100644 gpio_ioctl.c
> > > >  create mode 100644 xlat/gpio_event_flags.in
> > > >  create mode 100644 xlat/gpio_handle_flags.in
> > > >  create mode 100644 xlat/gpio_line_flags.in
> > > >  create mode 100644 xlat/gpio_v2_line_attr_ids.in
> > > >  create mode 100644 xlat/gpio_v2_line_flags.in
> > > 
> > > Thanks for the patch set.  Unfortunately, I don't see any changes
> > > to the test suite in this series.  How did you test the new code?
> > 
> > Yeah, sorry - not sure how to integrate my tests into your test suite,
> > and I didn't see anything in the commit requirements mentioning tests
> > either.
> 
> There is a point of view that explicit requirements of tests would scare
> some potential contributors off, although we might reconsider this in the
> future.
> 
> We strive to have a decent test coverage of strace decoders, currently we
> are at 87.5% lines coverage.
> 

Totally agree with you there - you can't have too much testing.

> strace can be configured using --enable-code-coverage, this allows to
> capture the coverage using "make code-coverage-capture" after "make
> check".  Please note that strace configured for capturing coverage
> is not intended for regular use.
> 
> > I'm open to suggestions as to what you would like to see from some tests
> > for this.  Testing the whole API is a bear that is probably best left
> > out of strace itself.  Even smoke testing a handful of the ioctls in the
> > most trivial way is non-trivial to setup - you obviously don't want to
> > go messing with real hardware so you need to setup mocks which requires
> > kernel modules that you may not have available etc.
> > 
> > Unless there is some other way to inject ioctls for strace to decode?
> 
> strace test suite is run unprivileged and is not expected to have access
> to anything non-trivial.
> The idea is to test decoders, not the kernel, although we happen to bump
> into some kernel corner cases from time to time.
> Yes, we use syscall tampering features of strace in some tests.
> 
> There is a relatively recent commit v5.6~27 that introduced a comprehensive
> set of HDIO_* ioctl decoding tests, you could use it as an example.
> 

Thanks for the pointers - I'll take a look at it and hopefully get something
out in time for 5.11.

Cheers,
Kent.