[PATCH v4 2/6] Add GPIO ioctl decoding

Kent Gibson warthog618 at gmail.com
Mon Jan 25 00:34:56 UTC 2021


On Sun, Jan 24, 2021 at 06:06:42PM +0300, Dmitry V. Levin wrote:
> On Sat, Jan 23, 2021 at 09:56:41AM +0800, Kent Gibson wrote:
> > Decode the GPIO character device ioctls first introduced in Linux v4.8,
> > as well as those added in subsequent kernel releases up to Linux v5.7.
> > 
> > Signed-off-by: Kent Gibson <warthog618 at gmail.com>
> 
> I've given this a wider testing and found some portability issues.
> 
> First of all, include/uapi/linux/gpio.h was first introduced a bit
> earlier: in Linux v4.6 it contained just 2 ioctl commands
> (GPIO_GET_CHIPINFO_IOCTL and GPIO_GET_LINEINFO_IOCTL) and just two
> definitions of structures (gpiochip_info and gpioline_info), so this code
> doesn't compile with kernel headers from Linux v4.6 and v4.7.
> 

Sorry - didn't realise those preliminary versions of gpio.h made it into
the released kernel.  They actually define a different ioctl major, 'o'
instead of 0xb4, that conflicted with other ioctls.
So probably best to not use those versions of gpio.h from the kernel.
That rules out using HAVE_LINUX_GPIO_H.

> I see 3 alternative fixes for this portability issue:
> - check for structures available since v4.8 and bump requirements
>   appropriately;
> - add to types/gpio.h definitions of those 4 structures that were
>   introduced in Linux v4.8;
> - add to types/gpio.h definitions of all 6 structures defined since Linux
>   v4.8 and lift the HAVE_LINUX_GPIO_H requirement altogether.
> 
> I'm inclined to choose the last alternative, see the proposed patch below.

Of the three options, I agree that the third is the best - only use the
kernel gpio.h to check that the local type definitions match.

Is there a reason you couldn't just include the latest kernel gpio.h in
the strace build directly, rather than having to redefine everything in
types/gpio.h?

> 
> This also affects the test, I'll post a separate message about it.
> 
> [...]
> > diff --git a/configure.ac b/configure.ac
> > index c199f02d..55597ad1 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -414,6 +414,7 @@ AC_CHECK_HEADERS(m4_normalize([
> >  	linux/dm-ioctl.h
> >  	linux/dqblk_xfs.h
> >  	linux/falloc.h
> > +	linux/gpio.h
> >  	linux/hiddev.h
> >  	linux/ip_vs.h
> >  	linux/ipc.h
> 
> Introduction of types/gpio.h made this hunk redundant because of
> types/check-gpio.m4 file automatically generated by types/gen.sh machinery.
> 
> [...]
> > diff --git a/gpio_ioctl.c b/gpio_ioctl.c
> > new file mode 100644
> > index 00000000..1098699b
> > --- /dev/null
> > +++ b/gpio_ioctl.c
> > @@ -0,0 +1,226 @@
> > +/*
> > + * Copyright (c) 2020 The strace developers.
> > + * All rights reserved.
> > + *
> > + * SPDX-License-Identifier: LGPL-2.1-or-later
> > + */
> > +
> > +#include "defs.h"
> > +#include "print_fields.h"
> > +
> > +#if HAVE_LINUX_GPIO_H
> > +
> > +#include "types/gpio.h"
> 
> I suggest to create an xlat/gpio_ioctl_cmds.in file with all GPIO ioctl
> commands, and include it here with a XLAT_MACROS_ONLY wrapping, see below.
> 
> This seems to be a prevalent way of defining missing constants along with
> a check that they match the kernel constants when the latter are
> available.
> 
> [...]
> > diff --git a/types/gpio.h b/types/gpio.h
> > new file mode 100644
> > index 00000000..47fa00da
> > --- /dev/null
> > +++ b/types/gpio.h
> > @@ -0,0 +1,38 @@
> > +/*
> > + * Copyright (c) 2020 The strace developers.
> > + * All rights reserved.
> > + *
> > + * SPDX-License-Identifier: LGPL-2.1-or-later
> > + */
> > +
> > +#ifndef STRACE_TYPES_GPIO_H
> > +# define STRACE_TYPES_GPIO_H
> > +
> > +# include <inttypes.h>
> 
> I suppose it's <stdint.h> that is actually needed, not <inttypes.h>.
> 

Indeed - that was for PRIx64 which is not used in this header.

> Here is a patch on top of yours I suggest to consider:
> 
> diff --git a/NEWS b/NEWS
> index cd6435866..73de66e3e 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -2,6 +2,7 @@ Noteworthy changes in release ?.? (????-??-??)
>  ==============================================
>  
>  * Improvements
> +  * Implemented decoding of GPIO_* ioctl commands.
>    * Implemented decoding of FS_IOC_FS[GS]ETXATTR, FS_IOC_[GS]ETFLAGS,
>      and FS_IOC32_[GS]ETFLAGS ioctl commands.
>    * Implemented decoding of SIOCADDMULTI, SIOCDELMULTI, SIOCGIFENCAP,
> diff --git a/configure.ac b/configure.ac
> index 55597ad1b..c199f02d3 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -414,7 +414,6 @@ AC_CHECK_HEADERS(m4_normalize([
>  	linux/dm-ioctl.h
>  	linux/dqblk_xfs.h
>  	linux/falloc.h
> -	linux/gpio.h
>  	linux/hiddev.h
>  	linux/ip_vs.h
>  	linux/ipc.h
> diff --git a/gpio_ioctl.c b/gpio_ioctl.c
> index 1098699bf..bf9d02a04 100644
> --- a/gpio_ioctl.c
> +++ b/gpio_ioctl.c
> @@ -8,14 +8,15 @@
>  #include "defs.h"
>  #include "print_fields.h"
>  
> -#if HAVE_LINUX_GPIO_H
> -
>  #include "types/gpio.h"
> +#define XLAT_MACROS_ONLY
> +# include "xlat/gpio_ioctl_cmds.h"
> +#undef XLAT_MACROS_ONLY
>  
>  static int
>  print_gpiochip_info(struct tcb *const tcp, const kernel_ulong_t arg)
>  {
> -	struct gpiochip_info info;
> +	struct_gpiochip_info info;
>  
>  	if (entering(tcp))
>  		return 0;
> @@ -37,7 +38,7 @@ print_gpiochip_info(struct tcb *const tcp, const kernel_ulong_t arg)
>  static int
>  print_gpioline_info(struct tcb *const tcp, const kernel_ulong_t arg)
>  {
> -	struct gpioline_info info;
> +	struct_gpioline_info info;
>  
>  	if (entering(tcp))
>  		tprints(", ");
> @@ -81,7 +82,7 @@ print_gpioline_info_unwatch(struct tcb *const tcp, const kernel_ulong_t arg)
>  static int
>  print_gpiohandle_request(struct tcb *const tcp, const kernel_ulong_t arg)
>  {
> -	struct gpiohandle_request hr;
> +	struct_gpiohandle_request hr;
>  
>  	if (entering(tcp))
>  		tprints(", ");
> @@ -117,7 +118,7 @@ print_gpiohandle_request(struct tcb *const tcp, const kernel_ulong_t arg)
>  static int
>  print_gpioevent_request(struct tcb *const tcp, const kernel_ulong_t arg)
>  {
> -	struct gpioevent_request er;
> +	struct_gpioevent_request er;
>  
>  	if (entering(tcp))
>  		tprints(", ");
> @@ -147,7 +148,7 @@ print_gpioevent_request(struct tcb *const tcp, const kernel_ulong_t arg)
>  }
>  
>  static void
> -print_gpiohandle_data(struct tcb *const tcp, const struct gpiohandle_data *vals)
> +print_gpiohandle_data(struct tcb *const tcp, const struct_gpiohandle_data *vals)
>  {
>  	PRINT_FIELD_ARRAY("{", *vals, values, tcp, print_uint8_array_member);
>  	tprints("}");
> @@ -156,7 +157,7 @@ print_gpiohandle_data(struct tcb *const tcp, const struct gpiohandle_data *vals)
>  static int
>  print_gpiohandle_get_values(struct tcb *const tcp, const kernel_ulong_t arg)
>  {
> -	struct gpiohandle_data vals;
> +	struct_gpiohandle_data vals;
>  
>  	if (entering(tcp))
>  		return 0;
> @@ -172,7 +173,7 @@ print_gpiohandle_get_values(struct tcb *const tcp, const kernel_ulong_t arg)
>  static int
>  print_gpiohandle_set_values(struct tcb *const tcp, const kernel_ulong_t arg)
>  {
> -	struct gpiohandle_data vals;
> +	struct_gpiohandle_data vals;
>  
>  	tprints(", ");
>  	if (!umove_or_printaddr(tcp, arg, &vals))
> @@ -222,5 +223,3 @@ gpio_ioctl(struct tcb *const tcp, const unsigned int code,
>  	}
>  	return RVAL_DECODED;
>  }
> -
> -#endif /* HAVE_LINUX_GPIO_H */
> diff --git a/ioctl.c b/ioctl.c
> index 6b8256a34..f3d31b8ad 100644
> --- a/ioctl.c
> +++ b/ioctl.c
> @@ -361,10 +361,8 @@ ioctl_decode(struct tcb *tcp)
>  	case 0xae:
>  		return kvm_ioctl(tcp, code, arg);
>  #endif
> -#ifdef HAVE_LINUX_GPIO_H
>  	case 0xb4:
>  		return gpio_ioctl(tcp, code, arg);
> -#endif
>  	case 0xb7:
>  		return nsfs_ioctl(tcp, code, arg);
>  #ifdef HAVE_LINUX_DM_IOCTL_H
> diff --git a/types/gpio.h b/types/gpio.h
> index 47fa00da3..747188526 100644
> --- a/types/gpio.h
> +++ b/types/gpio.h
> @@ -8,26 +8,53 @@
>  #ifndef STRACE_TYPES_GPIO_H
>  # define STRACE_TYPES_GPIO_H
>  
> -# include <inttypes.h>
> +# include <stdint.h>
>  # include <linux/ioctl.h>
>  # ifdef HAVE_LINUX_GPIO_H
>  #  include <linux/gpio.h>
>  # endif
>  
> +# ifndef GPIO_MAX_NAME_SIZE
> +#  define GPIO_MAX_NAME_SIZE 32
> +# endif
> +
>  # ifndef GPIOHANDLES_MAX
>  #  define GPIOHANDLES_MAX 64
>  # endif
>  
> -/* added in Linux v5.5 */
> -# ifndef GPIOHANDLE_SET_CONFIG_IOCTL
> -#  define GPIOHANDLE_SET_CONFIG_IOCTL _IOWR(0xB4, 0x0A, struct_gpiohandle_config)
> -# endif
> +typedef struct {
> +	char name[GPIO_MAX_NAME_SIZE];
> +	char label[GPIO_MAX_NAME_SIZE];
> +	uint32_t lines;
> +} struct_gpiochip_info;
>  
> -/* added in Linux v5.7 */
> -# ifndef GPIO_GET_LINEINFO_WATCH_IOCTL
> -#  define GPIO_GET_LINEINFO_WATCH_IOCTL _IOWR(0xB4, 0x0B, struct gpioline_info)
> -#  define GPIO_GET_LINEINFO_UNWATCH_IOCTL _IOWR(0xB4, 0x0C, uint32_t)
> -# endif
> +typedef struct {
> +	uint32_t line_offset;
> +	uint32_t flags;
> +	char name[GPIO_MAX_NAME_SIZE];
> +	char consumer[GPIO_MAX_NAME_SIZE];
> +} struct_gpioline_info;
> +
> +typedef struct {
> +	uint32_t lineoffsets[GPIOHANDLES_MAX];
> +	uint32_t flags;
> +	uint8_t default_values[GPIOHANDLES_MAX];
> +	char consumer_label[GPIO_MAX_NAME_SIZE];
> +	uint32_t lines;
> +	int fd;
> +} struct_gpiohandle_request;
> +
> +typedef struct {
> +	uint32_t lineoffset;
> +	uint32_t handleflags;
> +	uint32_t eventflags;
> +	char consumer_label[GPIO_MAX_NAME_SIZE];
> +	int fd;
> +} struct_gpioevent_request;
> +
> +typedef struct {
> +	uint8_t values[GPIOHANDLES_MAX];
> +} struct_gpiohandle_data;
>  
>  typedef struct {
>  	uint32_t flags;
> diff --git a/xlat/gpio_ioctl_cmds.in b/xlat/gpio_ioctl_cmds.in
> new file mode 100644
> index 000000000..28deaf69a
> --- /dev/null
> +++ b/xlat/gpio_ioctl_cmds.in
> @@ -0,0 +1,9 @@
> +GPIO_GET_CHIPINFO_IOCTL			_IOR(0xB4, 0x01, struct_gpiochip_info)
> +GPIO_GET_LINEINFO_IOCTL			_IOWR(0xB4, 0x02, struct_gpioline_info)
> +GPIO_GET_LINEHANDLE_IOCTL		_IOWR(0xB4, 0x03, struct_gpiohandle_request)
> +GPIO_GET_LINEEVENT_IOCTL		_IOWR(0xB4, 0x04, struct_gpioevent_request)
> +GPIOHANDLE_GET_LINE_VALUES_IOCTL	_IOWR(0xB4, 0x08, struct_gpiohandle_data)
> +GPIOHANDLE_SET_LINE_VALUES_IOCTL	_IOWR(0xB4, 0x09, struct_gpiohandle_data)
> +GPIOHANDLE_SET_CONFIG_IOCTL		_IOWR(0xB4, 0x0A, struct_gpiohandle_config)
> +GPIO_GET_LINEINFO_WATCH_IOCTL		_IOWR(0xB4, 0x0B, struct_gpioline_info)
> +GPIO_GET_LINEINFO_UNWATCH_IOCTL		_IOWR(0xB4, 0x0C, uint32_t)
> 
> 
No problems with your patch, if that is the way you want to go.

Cheers,
Kent.

> -- 
> ldv


More information about the Strace-devel mailing list