[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