[PATCH 0/7] selftests: gpio: rework and port to GPIO uAPI v2

[PATCH 0/7] selftests: gpio: rework and port to GPIO uAPI v2

[Kent Gibson]

Initially I just wanted to port the selftests to the latest GPIO uAPI,
but on finding that, due to dependency issues, the selftests are not built
for the buildroot environments that I do most of my GPIO testing in, I
decided to take a closer look.

The first patch is essentially a rewrite of the exising test suite.
It uses a simplified abstraction of the uAPI interfaces to allow a common
test suite to test the gpio-mockup using either of the uAPI interfaces.
The simplified cdev interface is implemented in gpio-mockup.sh, with the
actual driving of the uAPI implemented in gpio-mockup-cdev.c.
The simplified sysfs interface replaces gpio-mockup-sysfs.sh and is
loaded over the cdev implementation when selected.

The new tests should also be simpler to extend to cover new mockup
interfaces, such as the one Bart has been working on.

I have dropped support for testing modules other than gpio-mockup from
the command line options, as the tests are very gpio-mockup specific so
I didn't see any calling for it.

I have also tried to emphasise in the test output that the tests are
covering the gpio-mockup itself.  They do perform some implicit testing
of gpiolib and the uAPI interfaces, and so can be useful as smoke tests
for those, but their primary focus is the gpio-mockup.

Patches 2 through 5 do some cleaning up that is now possible with the
new implementation, including enabling building in buildroot environments.
Patch 4 doesn't strictly clean up all the old gpio references that it
could - the gpio was the only Level 1 test, so the Level 1 tests could
potentially be removed, but I was unsure if there may be other
implications to removing a whole test level, or that it may be useful
as a placeholder in case other static LDLIBS tests are added in
the future??

Patch 6 finally gets around to porting the tests to the latest GPIO uAPI.

And Patch 7 updates the config to set the CONFIG_GPIO_CDEV option that
was added in v5.10.

Cheers,
Kent.

Kent Gibson (7):
  selftests: gpio: rework and simplify test implementation
  selftests: gpio: remove obsolete gpio-mockup-chardev.c
  selftests: remove obsolete build restriction for gpio
  selftests: remove obsolete gpio references from kselftest_deps.sh
  tools: gpio: remove uAPI v1 code no longer used by selftests
  selftests: gpio: port to GPIO uAPI v2
  selftests: gpio: add CONFIG_GPIO_CDEV to config

 tools/gpio/gpio-utils.c                       |  89 ----
 tools/gpio/gpio-utils.h                       |   6 -
 tools/testing/selftests/Makefile              |   9 -
 tools/testing/selftests/gpio/Makefile         |  26 +-
 tools/testing/selftests/gpio/config           |   1 +
 .../testing/selftests/gpio/gpio-mockup-cdev.c | 198 ++++++++
 .../selftests/gpio/gpio-mockup-chardev.c      | 323 ------------
 .../selftests/gpio/gpio-mockup-sysfs.sh       | 168 ++-----
 tools/testing/selftests/gpio/gpio-mockup.sh   | 469 ++++++++++++------
 tools/testing/selftests/kselftest_deps.sh     |   4 +-
 10 files changed, 573 insertions(+), 720 deletions(-)
 create mode 100644 tools/testing/selftests/gpio/gpio-mockup-cdev.c
 delete mode 100644 tools/testing/selftests/gpio/gpio-mockup-chardev.c

-- 
2.30.0

[PATCH 1/7] selftests: gpio: rework and simplify test implementation

[Kent Gibson]

The GPIO mockup selftests are overly complicated with separate
implementations of the tests for sysfs and cdev uAPI, and with the cdev
implementation being dependent on tools/gpio and libmount.

Rework the test implementation to provide a common test suite with a
simplified pluggable uAPI interface.  The cdev implementation utilises
the GPIO uAPI directly to remove the dependence on tools/gpio.
The simplified uAPI interface removes the need for any file system mount
checks in C, and so removes the dependence on libmount.

The rework also fixes the sysfs test implementation which has been broken
since the device created in the multiple gpiochip case was split into
separate devices.

Fixes: commit 8a39f597bcfd ("gpio: mockup: rework device probing")
Signed-off-by: Kent Gibson <warthog618@gmail.com>
---
 tools/testing/selftests/gpio/Makefile         |  26 +-
 .../testing/selftests/gpio/gpio-mockup-cdev.c | 139 ++++++
 .../selftests/gpio/gpio-mockup-sysfs.sh       | 168 ++-----
 tools/testing/selftests/gpio/gpio-mockup.sh   | 462 ++++++++++++------
 4 files changed, 505 insertions(+), 290 deletions(-)
 create mode 100644 tools/testing/selftests/gpio/gpio-mockup-cdev.c

diff --git a/tools/testing/selftests/gpio/Makefile b/tools/testing/selftests/gpio/Makefile
index 32bdc978a711..e4363c64d40d 100644
--- a/tools/testing/selftests/gpio/Makefile
+++ b/tools/testing/selftests/gpio/Makefile
@@ -1,32 +1,8 @@
 # SPDX-License-Identifier: GPL-2.0
 
-VAR_CFLAGS := $(shell pkg-config --cflags mount 2>/dev/null)
-VAR_LDLIBS := $(shell pkg-config --libs mount 2>/dev/null)
-ifeq ($(VAR_LDLIBS),)
-VAR_LDLIBS := -lmount -I/usr/include/libmount
-endif
-
-CFLAGS += -O2 -g -std=gnu99 -Wall -I../../../../usr/include/ $(VAR_CFLAGS)
-LDLIBS += $(VAR_LDLIBS)
-
 TEST_PROGS := gpio-mockup.sh
 TEST_FILES := gpio-mockup-sysfs.sh
-TEST_PROGS_EXTENDED := gpio-mockup-chardev
-
-GPIODIR := $(realpath ../../../gpio)
-GPIOOBJ := gpio-utils.o
+TEST_GEN_PROGS_EXTENDED := gpio-mockup-cdev
 
-all: $(TEST_PROGS_EXTENDED)
-
-override define CLEAN
-	$(RM) $(TEST_PROGS_EXTENDED)
-	$(MAKE) -C $(GPIODIR) OUTPUT=$(GPIODIR)/ clean
-endef
-
-KSFT_KHDR_INSTALL := 1
 include ../lib.mk
 
-$(TEST_PROGS_EXTENDED): $(GPIODIR)/$(GPIOOBJ)
-
-$(GPIODIR)/$(GPIOOBJ):
-	$(MAKE) OUTPUT=$(GPIODIR)/ -C $(GPIODIR)
diff --git a/tools/testing/selftests/gpio/gpio-mockup-cdev.c b/tools/testing/selftests/gpio/gpio-mockup-cdev.c
new file mode 100644
index 000000000000..3bfd876a8b6a
--- /dev/null
+++ b/tools/testing/selftests/gpio/gpio-mockup-cdev.c
@@ -0,0 +1,139 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * GPIO mockup cdev test helper
+ *
+ * Copyright (C) 2020 Kent Gibson
+ */
+
+#include <errno.h>
+#include <fcntl.h>
+#include <linux/gpio.h>
+#include <signal.h>
+#include <stdint.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/ioctl.h>
+#include <unistd.h>
+
+#define CONSUMER	"gpio-mockup-cdev"
+
+static int request_line_v1(int cfd, unsigned int offset,
+			   uint32_t flags, unsigned int val)
+{
+	struct gpiohandle_request req;
+	int ret;
+
+	memset(&req, 0, sizeof(req));
+	req.lines = 1;
+	req.lineoffsets[0] = offset;
+	req.flags = flags;
+	strcpy(req.consumer_label, CONSUMER);
+	if (flags & GPIOHANDLE_REQUEST_OUTPUT)
+		req.default_values[0] = val;
+
+	ret = ioctl(cfd, GPIO_GET_LINEHANDLE_IOCTL, &req);
+	if (ret == -1)
+		return -errno;
+	return req.fd;
+}
+
+static int get_value_v1(int lfd)
+{
+	struct gpiohandle_data vals;
+	int ret;
+
+	memset(&vals, 0, sizeof(vals));
+	ret = ioctl(lfd, GPIOHANDLE_GET_LINE_VALUES_IOCTL, &vals);
+	if (ret == -1)
+		return -errno;
+	return vals.values[0];
+}
+
+static void usage(char *prog)
+{
+	printf("Usage: %s [-l] [-b <bias>] [-s <value>] [-u <uAPI>] <gpiochip> <offset>\n", prog);
+	printf("        -b: set line bias to one of pull-down, pull-up, disabled\n");
+	printf("               (default is to leave bias unchanged):\n");
+	printf("        -l: set line active low (default is active high)\n");
+	printf("        -s: set line value (default is to get line value)\n");
+	exit(-1);
+}
+
+static int wait_signal(void)
+{
+	int sig;
+	sigset_t wset;
+
+	sigemptyset(&wset);
+	sigaddset(&wset, SIGHUP);
+	sigaddset(&wset, SIGINT);
+	sigaddset(&wset, SIGTERM);
+	sigwait(&wset, &sig);
+
+	return sig;
+}
+
+int main(int argc, char *argv[])
+{
+	char *chip;
+	int opt, ret, cfd, lfd;
+	unsigned int offset, val;
+	uint32_t flags_v1;
+
+	ret = 0;
+	flags_v1 = GPIOHANDLE_REQUEST_INPUT;
+
+	while ((opt = getopt(argc, argv, "lb:s:u:")) != -1) {
+		switch (opt) {
+		case 'l':
+			flags_v1 |= GPIOHANDLE_REQUEST_ACTIVE_LOW;
+			break;
+		case 'b':
+			if (strcmp("pull-up", optarg) == 0)
+				flags_v1 |= GPIOHANDLE_REQUEST_BIAS_PULL_UP;
+			else if (strcmp("pull-down", optarg) == 0)
+				flags_v1 |= GPIOHANDLE_REQUEST_BIAS_PULL_DOWN;
+			else if (strcmp("disabled", optarg) == 0)
+				flags_v1 |= GPIOHANDLE_REQUEST_BIAS_DISABLE;
+			break;
+		case 's':
+			val = atoi(optarg);
+			flags_v1 &= ~GPIOHANDLE_REQUEST_INPUT;
+			flags_v1 |= GPIOHANDLE_REQUEST_OUTPUT;
+			break;
+		default:
+			usage(argv[0]);
+		}
+	}
+
+	if (argc < optind + 2)
+		usage(argv[0]);
+
+	chip = argv[optind];
+	offset = atoi(argv[optind+1]);
+
+	cfd = open(chip, 0);
+	if (cfd == -1) {
+		fprintf(stderr, "Failed to open %s: %s\n", chip, strerror(errno));
+		return -errno;
+	}
+
+	lfd = request_line_v1(cfd, offset, flags_v1, val);
+
+	close(cfd);
+
+	if (lfd < 0) {
+		fprintf(stderr, "Failed to request %s:%d: %s\n", chip, offset, strerror(-lfd));
+		return lfd;
+	}
+
+	if (flags_v1 & GPIOHANDLE_REQUEST_OUTPUT)
+		wait_signal();
+	else
+		ret = get_value_v1(lfd);
+
+	close(lfd);
+
+	return ret;
+}
diff --git a/tools/testing/selftests/gpio/gpio-mockup-sysfs.sh b/tools/testing/selftests/gpio/gpio-mockup-sysfs.sh
index dd269d877562..8ec99b4b12e5 100755
--- a/tools/testing/selftests/gpio/gpio-mockup-sysfs.sh
+++ b/tools/testing/selftests/gpio/gpio-mockup-sysfs.sh
@@ -1,135 +1,71 @@
 
 # SPDX-License-Identifier: GPL-2.0
-is_consistent()
-{
-	val=
-
-	active_low_sysfs=`cat $GPIO_SYSFS/gpio$nr/active_low`
-	val_sysfs=`cat $GPIO_SYSFS/gpio$nr/value`
-	dir_sysfs=`cat $GPIO_SYSFS/gpio$nr/direction`
-
-	gpio_this_debugfs=`cat $GPIO_DEBUGFS |grep "gpio-$nr" | sed "s/(.*)//g"`
-	dir_debugfs=`echo $gpio_this_debugfs | awk '{print $2}'`
-	val_debugfs=`echo $gpio_this_debugfs | awk '{print $3}'`
-	if [ $val_debugfs = "lo" ]; then
-		val=0
-	elif [ $val_debugfs = "hi" ]; then
-		val=1
-	fi
-
-	if [ $active_low_sysfs = "1" ]; then
-		if [ $val = "0" ]; then
-			val="1"
-		else
-			val="0"
-		fi
-	fi
-
-	if [ $val_sysfs = $val ] && [ $dir_sysfs = $dir_debugfs ]; then
-		echo -n "."
-	else
-		echo "test fail, exit"
-		die
-	fi
-}
-
-test_pin_logic()
-{
-	nr=$1
-	direction=$2
-	active_low=$3
-	value=$4
-
-	echo $direction > $GPIO_SYSFS/gpio$nr/direction
-	echo $active_low > $GPIO_SYSFS/gpio$nr/active_low
-	if [ $direction = "out" ]; then
-		echo $value > $GPIO_SYSFS/gpio$nr/value
-	fi
-	is_consistent $nr
-}
-
-test_one_pin()
-{
-	nr=$1
-
-	echo -n "test pin<$nr>"
-
-	echo $nr > $GPIO_SYSFS/export 2>/dev/null
-
-	if [ X$? != X0 ]; then
-		echo "test GPIO pin $nr failed"
-		die
-	fi
 
-	#"Checking if the sysfs is consistent with debugfs: "
-	is_consistent $nr
+# Overrides functions in gpio-mockup.sh to test using the GPIO SYSFS uAPI
 
-	#"Checking the logic of active_low: "
-	test_pin_logic $nr out 1 1
-	test_pin_logic $nr out 1 0
-	test_pin_logic $nr out 0 1
-	test_pin_logic $nr out 0 0
+SYSFS=`mount -t sysfs | head -1 | awk '{ print $3 }'`
+[ ! -d "$SYSFS" ] && skip "sysfs is not mounted"
 
-	#"Checking the logic of direction: "
-	test_pin_logic $nr in 1 1
-	test_pin_logic $nr out 1 0
-	test_pin_logic $nr low 0 1
-	test_pin_logic $nr high 0 0
+GPIO_SYSFS="${SYSFS}/class/gpio"
+[ ! -d "$GPIO_SYSFS" ] && skip "CONFIG_GPIO_SYSFS is not selected"
 
-	echo $nr > $GPIO_SYSFS/unexport
+sysfs_nr=
+sysfs_ldir=
 
-	echo "successful"
-}
-
-test_one_pin_fail()
+# determines the sysfs GPIO number given the $chip and $offset
+find_sysfs_nr()
 {
-	nr=$1
-
-	echo $nr > $GPIO_SYSFS/export 2>/dev/null
-
-	if [ X$? != X0 ]; then
-		echo "test invalid pin $nr successful"
-	else
-		echo "test invalid pin $nr failed"
-		echo $nr > $GPIO_SYSFS/unexport 2>/dev/null
-		die
-	fi
+	# e.g. gpiochip0: GPIOs 508-511, parent: platform/gpio-mockup.0, gpio-mockup-A:
+	local platform=`cat $SYSFS/kernel/debug/gpio | grep "$chip:" | tr -d ',' | awk '{print $5}'`
+	# e.g. /sys/class/gpio/gpiochip508/device/gpiochip0/dev
+	local syschip=`ls -d $GPIO_SYSFS/gpiochip*/device/$chip/dev`
+	syschip=${syschip#$GPIO_SYSFS}
+	syschip=${syschip%/device/$chip/dev}
+	sysfs_nr=`cat $SYSFS/devices/$platform/gpio/$syschip/base`
+	sysfs_nr=$(($sysfs_nr + $offset))
+	sysfs_ldir=$GPIO_SYSFS/gpio$sysfs_nr
 }
 
-list_chip()
+# The helpers being overridden...
+get_line()
 {
-	echo `ls -d $GPIO_DRV_SYSFS/gpiochip* 2>/dev/null`
+	[ -z "$sysfs_nr" ] && fail "sysfs line $chip:$offset not exported"
+	cat $sysfs_ldir/value
 }
 
-test_chip()
+set_line()
 {
-	chip=$1
-	name=`basename $chip`
-	base=`cat $chip/base`
-	ngpio=`cat $chip/ngpio`
-	printf "%-10s %-5s %-5s\n" $name $base $ngpio
-	if [ $ngpio = "0" ]; then
-		echo "number of gpio is zero is not allowed".
+	if [ -z "$sysfs_nr" ]; then
+		find_sysfs_nr
+		echo $sysfs_nr > $GPIO_SYSFS/export
 	fi
-	test_one_pin $base
-	test_one_pin $(($base + $ngpio - 1))
-	test_one_pin $((( RANDOM % $ngpio )  + $base ))
+	for option in $*; do
+		case $option in
+		active-high)
+			echo 0 > $sysfs_ldir/active_low
+			;;
+		active-low)
+			echo 1 > $sysfs_ldir/active_low
+			;;
+		input)
+			echo "in" > $sysfs_ldir/direction
+			;;
+		0)
+			echo "out" > $sysfs_ldir/direction
+			echo 0 > $sysfs_ldir/value
+			;;
+		1)
+			echo "out" > $sysfs_ldir/direction
+			echo 1 > $sysfs_ldir/value
+			;;
+		esac
+	done
 }
 
-test_chips_sysfs()
+release_line()
 {
-       gpiochip=`list_chip $module`
-       if [ X"$gpiochip" = X ]; then
-               if [ X"$valid" = Xfalse ]; then
-                       echo "successful"
-               else
-                       echo "fail"
-                       die
-               fi
-       else
-               for chip in $gpiochip; do
-                       test_chip $chip
-               done
-       fi
+	[ -z "$sysfs_nr" ] && return
+	echo $sysfs_nr > $GPIO_SYSFS/unexport
+	sysfs_nr=
+	sysfs_ldir=
 }
-
diff --git a/tools/testing/selftests/gpio/gpio-mockup.sh b/tools/testing/selftests/gpio/gpio-mockup.sh
index 7f35b9880485..66eed9b60963 100755
--- a/tools/testing/selftests/gpio/gpio-mockup.sh
+++ b/tools/testing/selftests/gpio/gpio-mockup.sh
@@ -2,71 +2,55 @@
 # SPDX-License-Identifier: GPL-2.0
 
 #exit status
-#1: Internal error
-#2: sysfs/debugfs not mount
-#3: insert module fail when gpio-mockup is a module.
-#4: Skip test including run as non-root user.
-#5: other reason.
-
-SYSFS=
-GPIO_SYSFS=
-GPIO_DRV_SYSFS=
+#0: success
+#1: fail
+#4: skip test - including run as non-root user
+
+BASE=`dirname $0`
 DEBUGFS=
 GPIO_DEBUGFS=
-dev_type=
-module=
+dev_type="cdev"
+module="gpio-mockup"
+verbose=
+random=
+active_opt=
+bias_opt=
+line_set_pid=
 
-# Kselftest framework requirement - SKIP code is 4.
+# Kselftest return codes
+ksft_fail=1
 ksft_skip=4
 
 usage()
 {
 	echo "Usage:"
-	echo "$0 [-f] [-m name] [-t type]"
-	echo "-f:  full test. It maybe conflict with existence gpio device."
-	echo "-m:  module name, default name is gpio-mockup. It could also test"
-	echo "     other gpio device."
-	echo "-t:  interface type: chardev(char device) and sysfs(being"
-	echo "     deprecated). The first one is default"
-	echo ""
-	echo "$0 -h"
-	echo "This usage"
+	echo "$0 [-frv] [-t type]"
+	echo "-f:  full test (minimal set run by default)"
+	echo "-r:  test random lines as well as fence posts"
+	echo "-t:  interface type:"
+	echo "      cdev (character device ABI) - default"
+	echo "      sysfs (deprecated SYSFS ABI)"
+	echo "-v:  verbose progress reporting"
+	exit $ksft_fail
 }
 
-prerequisite()
+skip()
 {
-	msg="skip all tests:"
-	if [ $UID != 0 ]; then
-		echo $msg must be run as root >&2
-		exit $ksft_skip
-	fi
-	SYSFS=`mount -t sysfs | head -1 | awk '{ print $3 }'`
-	if [ ! -d "$SYSFS" ]; then
-		echo $msg sysfs is not mounted >&2
-		exit 2
-	fi
-	GPIO_SYSFS=`echo $SYSFS/class/gpio`
-	GPIO_DRV_SYSFS=`echo $SYSFS/devices/platform/$module/gpio`
-	DEBUGFS=`mount -t debugfs | head -1 | awk '{ print $3 }'`
-	if [ ! -d "$DEBUGFS" ]; then
-		echo $msg debugfs is not mounted >&2
-		exit 2
-	fi
-	GPIO_DEBUGFS=`echo $DEBUGFS/gpio`
-	source gpio-mockup-sysfs.sh
+	echo $* >&2
+	echo GPIO $module test SKIP
+	exit $ksft_skip
 }
 
-try_insert_module()
+prerequisite()
 {
-	if [ -d "$GPIO_DRV_SYSFS" ]; then
-		echo "$GPIO_DRV_SYSFS exist. Skip insert module"
-	else
-		modprobe -q $module $1
-		if [ X$? != X0 ]; then
-			echo $msg insmod $module failed >&2
-			exit 3
-		fi
-	fi
+	[ $(id -u) -ne 0 ] && skip "must be run as root"
+
+        [ ! which modprobe > /dev/null 2>&1 ] && skip "need modprobe installed"
+
+	DEBUGFS=`mount -t debugfs | head -1 | awk '{ print $3 }'`
+	[ ! -d "$DEBUGFS" ] && skip "debugfs is not mounted"
+
+	GPIO_DEBUGFS=$DEBUGFS/$module
 }
 
 remove_module()
@@ -74,133 +58,313 @@ remove_module()
 	modprobe -r -q $module
 }
 
-die()
+cleanup()
 {
+	release_line
 	remove_module
-	exit 5
 }
 
-test_chips()
+fail()
 {
-	if [ X$dev_type = Xsysfs ]; then
-		echo "WARNING: sysfs ABI of gpio is going to deprecated."
-		test_chips_sysfs $*
-	else
-		$BASE/gpio-mockup-chardev $*
-	fi
+	echo "test failed: $*" >&2
+	echo GPIO $module test FAIL
+	exit $ksft_fail
+}
+
+try_insert_module()
+{
+	modprobe -q $module $1
+	err=$?
+	[ $err -ne 0 ] && fail "insert $module failed with error $err"
+}
+
+log()
+{
+	[ "$verbose" ] && echo $*
+}
+
+# The following line helpers, release_Line, get_line and set_line, all
+# make use of the global $chip and $offset variables.
+#
+# This implementation drives the GPIO character device (cdev) uAPI.
+# Other implementations may override these to test different uAPIs.
+
+# Release any resources related to the line
+release_line()
+{
+	[ -z "$line_set_pid" ] && return
+	kill $line_set_pid
+	line_set_pid=
 }
 
-gpio_test()
+# Read the current value of the line
+get_line()
 {
-	param=$1
-	valid=$2
+	release_line
+
+	$BASE/gpio-mockup-cdev $active_opt /dev/$chip $offset > /dev/null 2>&1
+	echo $?
+}
 
-	if [ X"$param" = X ]; then
-		die
+# Set the state of the line
+#
+# Changes to line configuration are provided as parameters.
+# The line is assumed to be an output if the line value 0 or 1 is
+# specified, else an input.
+set_line()
+{
+	local val=
+
+	release_line
+
+	# parse config options...
+	for option in $*; do
+		case $option in
+		active-low)
+			active_opt="-l "
+			;;
+		active-high)
+			active_opt=
+			;;
+		bias-none)
+			bias_opt=
+			;;
+		pull-down)
+			bias_opt="-bpull-down "
+			;;
+		pull-up)
+			bias_opt="-bpull-up "
+			;;
+		0)
+			val=0
+			;;
+		1)
+			val=1
+			;;
+		esac
+	done
+
+	local cdev_opts=${active_opt}
+	if [ "$val" ]; then
+		$BASE/gpio-mockup-cdev $cdev_opts -s$val /dev/$chip $offset 2>&1 >/dev/null &
+		# failure to set is detected by reading mock and toggling values
+		line_set_pid=$!
+		# wait for line to be set...
+		sleep 0.05
+	elif [ "$bias_opt" ]; then
+		cdev_opts=${cdev_opts}${bias_opt}
+		$BASE/gpio-mockup-cdev $cdev_opts /dev/$chip $offset 2>&1 >/dev/null
 	fi
-	try_insert_module "gpio_mockup_ranges=$param"
-	echo -n "GPIO $module test with ranges: <"
-	echo "$param>: "
-	printf "%-10s %s\n" $param
-	test_chips $module $valid
-	remove_module
 }
 
-BASE=`dirname $0`
+assert_line()
+{
+	local val=`get_line`
+	[ "$val" -ne $1 ] && fail "line value is $val when $1 was expected"
+}
+
+# The following mock helpers all make use of the $mock_line
+set_mock()
+{
+	echo $1 > $mock_line
+}
 
-dev_type=
-TEMP=`getopt -o fhm:t: -n '$0' -- "$@"`
+assert_mock()
+{
+	local val=`cat $mock_line`
+	[ "$val" -ne $1 ] && fail "mock $mock_line value $val when $1 expected"
+}
 
-if [ "$?" != "0" ]; then
-        echo "Parameter process failed, Terminating..." >&2
-        exit 1
-fi
+# test the functionality of a line
+#
+# The line is set from the mockup side and is read from the userspace side
+# (input), and is set from the userspace side and is read from the mockup side
+# (output).
+#
+# Setting the mockup pull using the userspace interface bias settings is
+# tested where supported by the userspace interface (cdev).
+test_line()
+{
+	chip=$1
+	offset=$2
+	log "test_line $chip $offset"
+	mock_line=$GPIO_DEBUGFS/$chip/$offset
+	[ ! -e "$mock_line" ] && fail "missing line $chip:$offset"
 
-# Note the quotes around `$TEMP': they are essential!
-eval set -- "$TEMP"
+	# test as input
+	set_mock 1
+	set_line input active-high
+	assert_line 1
+	set_mock 0
+	assert_line 0
 
-while true; do
-	case $1 in
-	-f)
-		full_test=true
+	if [ "$full_test" ]; then
+		# test as input
+		if [ "$dev_type" != "sysfs" ]; then
+			set_mock 0
+			set_line input pull-up
+			assert_line 1
+			set_mock 0
+			assert_line 0
+
+			set_mock 1
+			set_line input pull-down
+			assert_line 0
+			set_mock 1
+			assert_line 1
+
+			set_line bias-none
+		fi
+
+		set_mock 0
+		set_line active-low
+		assert_line 1
+		set_mock 1
+		assert_line 0
+
+		# test as output
+		set_mock 1
+		set_line active-low 1
+		assert_mock 0
+		set_line 0
+		assert_mock 1
+	fi
+
+	# test as output
+	set_mock 1
+	set_line active-high 0
+	assert_mock 0
+	set_line 1
+	assert_mock 1
+
+	release_line
+}
+
+test_no_line()
+{
+	log test_no_line $*
+	[ -e "$GPIO_DEBUGFS/$1/$2" ] && fail "unexpected line $1:$2"
+}
+
+# Load the module and check that the expected number of gpiochips, with the
+# expected number of lines, are created and are functional.
+#
+# $1 is the gpio_mockup_ranges parameter for the module
+# The remaining parameters are the number of lines, n, expected for each of
+# the gpiochips expected to be created.
+#
+# For each gpiochip the fence post lines, 0 and n-1, are tested, and the
+# line on the far side of the fence post, n, is tested to not exist.
+#
+# If the $random flag is set then a random line in the middle of the
+# gpiochip is tested as well.
+insmod_test()
+{
+	local ranges=$1
+	local gc=
+	shift
+
+	[ -z "$ranges" ] && fail "missing ranges"
+	try_insert_module "gpio_mockup_ranges=$ranges"
+	log "GPIO $module test with ranges: <$ranges>: "
+	gpiochip=`ls -d $DEBUGFS/$module/gpiochip* 2>/dev/null`
+	for chip in $gpiochip; do
+		gc=`basename $chip`
+		[ -z "$1" ] && fail "unexpected chip - $gc"
+		test_line $gc 0
+		if [ "$random" ] && [ $1 -gt 2 ]; then
+			test_line $gc $((( RANDOM % ($1 - 2) + 1)))
+		fi
+		test_line $gc $(($1 - 1))
+		test_no_line $gc $1
 		shift
+	done
+	[ "$1" ] && fail "missing expected chip of width $1"
+	remove_module
+}
+
+while getopts ":frvt:" opt; do
+	case $opt in
+	f)
+		full_test=true
 		;;
-	-h)
-		usage
-		exit
+	r)
+		random=true
 		;;
-	-m)
-		module=$2
-		shift 2
+	t)
+		dev_type=$OPTARG
 		;;
-	-t)
-		dev_type=$2
-		shift 2
-		;;
-	--)
-		shift
-		break
+	v)
+		verbose=true
 		;;
 	*)
-		echo "Internal error!"
-		exit 1
+		usage
 		;;
 	esac
 done
+shift $((OPTIND-1))
 
-if [ X"$module" = X ]; then
-	module="gpio-mockup"
-fi
+prerequisite
 
-if [ X$dev_type != Xsysfs ]; then
-	dev_type="chardev"
-fi
+trap exit SIGTERM SIGINT
+trap cleanup EXIT
 
-prerequisite
+case "$dev_type" in
+sysfs)
+	source $BASE/gpio-mockup-sysfs.sh
+	echo "WARNING: gpio sysfs ABI is deprecated."
+	;;
+cdev)
+	;;
+*)
+	fail "unknown interface type: $dev_type"
+	;;
+esac
+
+remove_module
 
-echo "1.  Test dynamic allocation of gpio successful means insert gpiochip and"
-echo "    manipulate gpio pin successful"
-gpio_test "-1,32" true
-gpio_test "-1,32,-1,32" true
-gpio_test "-1,32,-1,32,-1,32" true
-if [ X$full_test = Xtrue ]; then
-	gpio_test "-1,32,32,64" true
-	gpio_test "-1,32,40,64,-1,5" true
-	gpio_test "-1,32,32,64,-1,32" true
-	gpio_test "0,32,32,64,-1,32,-1,32" true
-	gpio_test "-1,32,-1,32,0,32,32,64" true
-	echo "2.  Do basic test: successful means insert gpiochip and"
-	echo "    manipulate gpio pin successful"
-	gpio_test "0,32" true
-	gpio_test "0,32,32,64" true
-	gpio_test "0,32,40,64,64,96" true
+# manual gpio allocation tests fail if a physical chip already exists
+[ "$full_test" ] && [ -e "/dev/gpiochip0" ] && skip "full tests conflict with gpiochip0"
+
+echo "1.  Module load tests"
+echo "1.1.  dynamic allocation of gpio"
+insmod_test "-1,32" 32
+insmod_test "-1,32,-1,32" 32 32
+insmod_test "-1,32,-1,32,-1,32" 32 32 32
+if [ "$full_test" ]; then
+	echo "1.2.  manual allocation of gpio"
+	insmod_test "0,32" 32
+	insmod_test "0,32,32,64" 32 32
+	insmod_test "0,32,40,64,64,96" 32 24 32
+	echo "1.3.  dynamic and manual allocation of gpio"
+	insmod_test "-1,32,32,64" 32 32
+	insmod_test "-1,32,-1,32,0,32,32,64" 32 32 32 32
+	insmod_test "-1,32,32,64,-1,32" 32 32 32
+	insmod_test "-1,32,40,64,-1,5" 32 24 5
+	insmod_test "0,32,32,64,-1,32,-1,32" 32 32 32 32
 fi
-echo "3.  Error test: successful means insert gpiochip failed"
-echo "3.1 Test number of gpio overflow"
-#Currently: The max number of gpio(1024) is defined in arm architecture.
-gpio_test "-1,32,-1,1024" false
-if [ X$full_test = Xtrue ]; then
-	echo "3.2 Test zero line of gpio"
-	gpio_test "0,0" false
-	echo "3.3 Test range overlap"
-	echo "3.3.1 Test corner case"
-	gpio_test "0,32,0,1" false
-	gpio_test "0,32,32,64,32,40" false
-	gpio_test "0,32,35,64,35,45" false
-	gpio_test "0,32,31,32" false
-	gpio_test "0,32,32,64,36,37" false
-	gpio_test "0,32,35,64,34,36" false
-	echo "3.3.2 Test inserting invalid second gpiochip"
-	gpio_test "0,32,30,35" false
-	gpio_test "0,32,1,5" false
-	gpio_test "10,32,9,14" false
-	gpio_test "10,32,30,35" false
-	echo "3.3.3 Test others"
-	gpio_test "0,32,40,56,39,45" false
-	gpio_test "0,32,40,56,30,33" false
-	gpio_test "0,32,40,56,30,41" false
-	gpio_test "0,32,40,56,20,21" false
+echo "2.  Module load error tests"
+echo "2.1 gpio overflow"
+# Currently: The max number of gpio(1024) is defined in arm architecture.
+insmod_test "-1,1024"
+if [ "$full_test" ]; then
+	echo "2.2 no lines defined"
+	insmod_test "0,0"
+	echo "2.3 ignore range overlap"
+	insmod_test "0,32,0,1" 32
+	insmod_test "0,32,1,5" 32
+	insmod_test "0,32,30,35" 32
+	insmod_test "0,32,31,32" 32
+	insmod_test "10,32,30,35" 22
+	insmod_test "10,32,9,14" 22
+	insmod_test "0,32,20,21,40,56" 32 16
+	insmod_test "0,32,32,64,32,40" 32 32
+	insmod_test "0,32,32,64,36,37" 32 32
+	insmod_test "0,32,35,64,34,36" 32 29
+	insmod_test "0,32,35,64,35,45" 32 29
+	insmod_test "0,32,40,56,30,33" 32 16
+	insmod_test "0,32,40,56,30,41" 32 16
+	insmod_test "0,32,40,56,39,45" 32 16
 fi
 
-echo GPIO test PASS
-
+echo GPIO $module test PASS
-- 
2.30.0

Re: [PATCH 1/7] selftests: gpio: rework and simplify test implementation

[Kent Gibson]

On Sat, Jan 02, 2021 at 03:52:32PM +0200, Andy Shevchenko wrote:
> On Saturday, January 2, 2021, Kent Gibson <warthog618@gmail.com> wrote:
> 
> > The GPIO mockup selftests are overly complicated with separate
> > implementations of the tests for sysfs and cdev uAPI, and with the cdev
> > implementation being dependent on tools/gpio and libmount.
> >
> > Rework the test implementation to provide a common test suite with a
> > simplified pluggable uAPI interface.  The cdev implementation utilises
> > the GPIO uAPI directly to remove the dependence on tools/gpio.
> > The simplified uAPI interface removes the need for any file system mount
> > checks in C, and so removes the dependence on libmount.
> >
> > The rework also fixes the sysfs test implementation which has been broken
> > since the device created in the multiple gpiochip case was split into
> > separate devices.
> >
> >
> 
> I briefly looked at code in shell below... there are places to improve
> (useless use of: cat, test, negation, etc).
> 

My shell is clearly pretty poor, so I would really appreciate a pointer
to an example of each, and I'll then hunt down the rest.

Thanks,
Kent.

Re: [PATCH 1/7] selftests: gpio: rework and simplify test implementation

[Andy Shevchenko]

On Sat, Jan 2, 2021 at 4:32 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> The GPIO mockup selftests are overly complicated with separate
> implementations of the tests for sysfs and cdev uAPI, and with the cdev
> implementation being dependent on tools/gpio and libmount.
>
> Rework the test implementation to provide a common test suite with a
> simplified pluggable uAPI interface.  The cdev implementation utilises
> the GPIO uAPI directly to remove the dependence on tools/gpio.
> The simplified uAPI interface removes the need for any file system mount
> checks in C, and so removes the dependence on libmount.
>
> The rework also fixes the sysfs test implementation which has been broken
> since the device created in the multiple gpiochip case was split into
> separate devices.

Okay, I commented something, not sure if everything is correct, needs
double checking.
Shell is quite a hard programming language. Everyday I found something
new about it.

...

> +#include <linux/gpio.h>

Perhaps include it after system headers?

> +#include <signal.h>
> +#include <stdint.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <sys/ioctl.h>
> +#include <unistd.h>

...

> +SYSFS=`mount -t sysfs | head -1 | awk '{ print $3 }'`

Oh, would below be better?
  grep -w sysfs /proc/mounts | cut -f2 -d' '

...

> +[ ! -d "$SYSFS" ] && skip "sysfs is not mounted"

[ -d ... ] || skip "..."

...

> +[ ! -d "$GPIO_SYSFS" ] && skip "CONFIG_GPIO_SYSFS is not selected"

Ditto.

...

> +       local platform=`cat $SYSFS/kernel/debug/gpio | grep "$chip:" | tr -d ',' | awk '{print $5}'`

Besides useless use of cat (and tr + awk can be simplified) why are
you simply not using
/sys/bus/gpio/devices/$chip ?

> +       # e.g. /sys/class/gpio/gpiochip508/device/gpiochip0/dev
> +       local syschip=`ls -d $GPIO_SYSFS/gpiochip*/device/$chip/dev`

ls -d is fragile, better to use `find ...`

> +       syschip=${syschip#$GPIO_SYSFS}
> +       syschip=${syschip%/device/$chip/dev}

How does this handle more than one gpiochip listed?
Also, can you consider optimizing these to get whatever you want easily?

> +       sysfs_nr=`cat $SYSFS/devices/$platform/gpio/$syschip/base`

(It's probably fine here, but this doesn't work against PCI bus, for
example, see above for the fix)

> +       sysfs_nr=$(($sysfs_nr + $offset))
> +       sysfs_ldir=$GPIO_SYSFS/gpio$sysfs_nr
>  }

...

> +set_line()
>  {
> +       if [ -z "$sysfs_nr" ]; then
> +               find_sysfs_nr
> +               echo $sysfs_nr > $GPIO_SYSFS/export
>         fi

It sounds like a separate function (you have release_line(), perhaps
acquire_line() is good to have).

> +release_line()
>  {
> +       [ -z "$sysfs_nr" ] && return
> +       echo $sysfs_nr > $GPIO_SYSFS/unexport
> +       sysfs_nr=
> +       sysfs_ldir=
>  }

...

> +BASE=`dirname $0`

Can be used via shell substitutions.

...

> +skip()
>  {

> +       echo $* >&2

In all cases better to use "$*" (note surrounding double quotes).

> +       echo GPIO $module test SKIP
> +       exit $ksft_skip
>  }

...

> +        [ ! which modprobe > /dev/null 2>&1 ] && skip "need modprobe installed"

AFAIR `which` can be optional on some systems.

...

> +       DEBUGFS=`mount -t debugfs | head -1 | awk '{ print $3 }'`
> +       [ ! -d "$DEBUGFS" ] && skip "debugfs is not mounted"

Same as per sysfs in another script.

...

> +try_insert_module()
> +{
> +       modprobe -q $module $1
> +       err=$?
> +       [ $err -ne 0 ] && fail "insert $module failed with error $err"

I guess it's as simple as `modprobe ... || fail "... $?"

> +}

...

> +       [ ! -e "$mock_line" ] && fail "missing line $chip:$offset"

[ -e ... ] || ...

...

> +       local ranges=$1
> +       local gc=
> +       shift

I found that combination
       local ranges=$1; shift
is better to read.

...

> +       gpiochip=`ls -d $DEBUGFS/$module/gpiochip* 2>/dev/null`

`find ...` is a better choice.

> +       for chip in $gpiochip; do
> +               gc=`basename $chip`
> +               [ -z "$1" ] && fail "unexpected chip - $gc"
> +               test_line $gc 0

> +               if [ "$random" ] && [ $1 -gt 2 ]; then

You call the test twice, while you may do it in one go.

> +                       test_line $gc $((( RANDOM % ($1 - 2) + 1)))
> +               fi
> +               test_line $gc $(($1 - 1))
> +               test_no_line $gc $1
>                 shift
> +       done
> +       [ "$1" ] && fail "missing expected chip of width $1"

...

> +# manual gpio allocation tests fail if a physical chip already exists
> +[ "$full_test" ] && [ -e "/dev/gpiochip0" ] && skip "full tests conflict with gpiochip0"

I guess it should be rather something like

[ "$full_test" = "true" -a -e "/dev/gpiochip0" ]

P.S. Also you may use `#!/bin/sh -efu` as shebang and fix other problems.

--
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/7] selftests: gpio: rework and simplify test implementation

[Kent Gibson]

On Sun, Jan 03, 2021 at 12:20:26AM +0200, Andy Shevchenko wrote:
> On Sat, Jan 2, 2021 at 4:32 AM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > The GPIO mockup selftests are overly complicated with separate
> > implementations of the tests for sysfs and cdev uAPI, and with the cdev
> > implementation being dependent on tools/gpio and libmount.
> >
> > Rework the test implementation to provide a common test suite with a
> > simplified pluggable uAPI interface.  The cdev implementation utilises
> > the GPIO uAPI directly to remove the dependence on tools/gpio.
> > The simplified uAPI interface removes the need for any file system mount
> > checks in C, and so removes the dependence on libmount.
> >
> > The rework also fixes the sysfs test implementation which has been broken
> > since the device created in the multiple gpiochip case was split into
> > separate devices.
> 
> Okay, I commented something, not sure if everything is correct, needs
> double checking.
> Shell is quite a hard programming language. Everyday I found something
> new about it.
> 

You are telling me - there are about six million ways to do even the
most trivial tasks.  Makes you appreciate more constrained languages.

> ...
> 
> > +#include <linux/gpio.h>
> 
> Perhaps include it after system headers?
> 

hehe, I blindly sorted them.
Should it matter?

> > +#include <signal.h>
> > +#include <stdint.h>
> > +#include <stdio.h>
> > +#include <stdlib.h>
> > +#include <string.h>
> > +#include <sys/ioctl.h>
> > +#include <unistd.h>
> 
> ...
> 
> > +SYSFS=`mount -t sysfs | head -1 | awk '{ print $3 }'`
> 
> Oh, would below be better?
>   grep -w sysfs /proc/mounts | cut -f2 -d' '
> 

That looks good - the other is a carry over from the old gpio-mockup.sh.

> ...
> 
> > +[ ! -d "$SYSFS" ] && skip "sysfs is not mounted"
> 
> [ -d ... ] || skip "..."
> 

Yeah, those were if [ .. ]; then fi originally. I did the first step
of simplification and missed the second :-(.

> ...
> 
> > +[ ! -d "$GPIO_SYSFS" ] && skip "CONFIG_GPIO_SYSFS is not selected"
> 
> Ditto.
> 
> ...
> 
> > +       local platform=`cat $SYSFS/kernel/debug/gpio | grep "$chip:" | tr -d ',' | awk '{print $5}'`
> 
> Besides useless use of cat (and tr + awk can be simplified) why are

What do you suggest for the tr/awk simplification?

> you simply not using
> /sys/bus/gpio/devices/$chip ?
> 

Cos that shows all the gpiochips, not just the ones created by gpio-mockup.
And I certainly don't want to go messing with real hardware.
The default tests should still run on real hardware - but only
accessing the mockup devices.

Got a better way to filter out real hardware?

> > +       # e.g. /sys/class/gpio/gpiochip508/device/gpiochip0/dev
> > +       local syschip=`ls -d $GPIO_SYSFS/gpiochip*/device/$chip/dev`
> 
> ls -d is fragile, better to use `find ...`
> 

OK 

> > +       syschip=${syschip#$GPIO_SYSFS}
> > +       syschip=${syschip%/device/$chip/dev}
> 
> How does this handle more than one gpiochip listed?

It is filtered by $chip so there can only be one.
Or is that a false assumption?

> Also, can you consider optimizing these to get whatever you want easily?
> 

Sadly that IS my optimized way - I don't know of an easier way to find
the sysfs GPIO number given the gpiochip and offset :-(.
Happy to learn of any alternative.

> > +       sysfs_nr=`cat $SYSFS/devices/$platform/gpio/$syschip/base`
> 
> (It's probably fine here, but this doesn't work against PCI bus, for
> example, see above for the fix)
> 

Not sure what you mean here.

> > +       sysfs_nr=$(($sysfs_nr + $offset))
> > +       sysfs_ldir=$GPIO_SYSFS/gpio$sysfs_nr
> >  }
> 
> ...
> 
> > +set_line()
> >  {
> > +       if [ -z "$sysfs_nr" ]; then
> > +               find_sysfs_nr
> > +               echo $sysfs_nr > $GPIO_SYSFS/export
> >         fi
> 
> It sounds like a separate function (you have release_line(), perhaps
> acquire_line() is good to have).
> 

The cdev implementation has to release and re-acquire in the background
as there is no simple way to perform a set_config on a requested line
from shell - just holding the requested line for a set is painful enough,
and the goal here was to keep the tests simple.

I didn't want to make line acquisition/release explicit in the gpio-mockup
tests, as that would make them needlessly complicated, so the acquire is
bundled into the set_line - and anywhere else the uAPI implementation
needs it.  There is an implicit assumption that a set_line will always
be called before a get_line, but that is always true - there is no
"as-is" being tested here.

Of course you still need the release_line at the end of the test, so
that is still there.

> > +release_line()
> >  {
> > +       [ -z "$sysfs_nr" ] && return
> > +       echo $sysfs_nr > $GPIO_SYSFS/unexport
> > +       sysfs_nr=
> > +       sysfs_ldir=
> >  }
> 
> ...
> 
> > +BASE=`dirname $0`
> 
> Can be used via shell substitutions.
> 

Yup

> ...
> 
> > +skip()
> >  {
> 
> > +       echo $* >&2
> 
> In all cases better to use "$*" (note surrounding double quotes).
> 

Agreed - except where

	for option in $*; do

is used to parse parameters.

> > +       echo GPIO $module test SKIP
> > +       exit $ksft_skip
> >  }
> 
> ...
> 
> > +        [ ! which modprobe > /dev/null 2>&1 ] && skip "need modprobe installed"
> 
> AFAIR `which` can be optional on some systems.
> 

That is how other selftests check for availability of modprobe.
e.g. selftests/kmod/kmod.sh and selftests/vm/test_hmm.sh, so I assumed
it was acceptable.

Is there an alternative?

> ...
> 
> > +       DEBUGFS=`mount -t debugfs | head -1 | awk '{ print $3 }'`
> > +       [ ! -d "$DEBUGFS" ] && skip "debugfs is not mounted"
> 
> Same as per sysfs in another script.
> 
> ...
> 
> > +try_insert_module()
> > +{
> > +       modprobe -q $module $1
> > +       err=$?
> > +       [ $err -ne 0 ] && fail "insert $module failed with error $err"
> 
> I guess it's as simple as `modprobe ... || fail "... $?"
> 

Yup

> > +}
> 
> ...
> 
> > +       [ ! -e "$mock_line" ] && fail "missing line $chip:$offset"
> 
> [ -e ... ] || ...
> 
> ...
> 
> > +       local ranges=$1
> > +       local gc=
> > +       shift
> 
> I found that combination
>        local ranges=$1; shift
> is better to read.
> 

Agreed - the gc certainly shouldn't be splitting the two.

> ...
> 
> > +       gpiochip=`ls -d $DEBUGFS/$module/gpiochip* 2>/dev/null`
> 
> `find ...` is a better choice.
> 
> > +       for chip in $gpiochip; do
> > +               gc=`basename $chip`
> > +               [ -z "$1" ] && fail "unexpected chip - $gc"
> > +               test_line $gc 0
> 
> > +               if [ "$random" ] && [ $1 -gt 2 ]; then
> 
> You call the test twice, while you may do it in one go.
> 

Ahh, replacing the && with -a. Good to know.

> > +                       test_line $gc $((( RANDOM % ($1 - 2) + 1)))
> > +               fi
> > +               test_line $gc $(($1 - 1))
> > +               test_no_line $gc $1
> >                 shift
> > +       done
> > +       [ "$1" ] && fail "missing expected chip of width $1"
> 
> ...
> 
> > +# manual gpio allocation tests fail if a physical chip already exists
> > +[ "$full_test" ] && [ -e "/dev/gpiochip0" ] && skip "full tests conflict with gpiochip0"
> 
> I guess it should be rather something like
> 
> [ "$full_test" = "true" -a -e "/dev/gpiochip0" ]
> 

I'm going with empty for false, so you can drop the = "true" here.

> P.S. Also you may use `#!/bin/sh -efu` as shebang and fix other problems.
> 

A shebang or a `set -efu`?
I don't see shebang options used anywhere in the selftest scripts, but I
agree with a set.

Either way I am unsure what the shebang should be.
The majority of the selftest scripts use bash as the shebang, with the
remainder using plain sh.
These scripts do use some bash extensions, and it was originally bash, so
I left it as that.
My test setups mainly use busybox, and don't have bash, so they complain
about the bash shebang - though the ash(??) busybox is using still runs
the script fine.

Thanks again for the review - always a learning experience.

Cheers,
Kent.

Re: [PATCH 1/7] selftests: gpio: rework and simplify test implementation

[Andy Shevchenko]

On Sun, Jan 3, 2021 at 4:17 AM Kent Gibson <warthog618@gmail.com> wrote:
> On Sun, Jan 03, 2021 at 12:20:26AM +0200, Andy Shevchenko wrote:
> > On Sat, Jan 2, 2021 at 4:32 AM Kent Gibson <warthog618@gmail.com> wrote:

...

> > > +#include <linux/gpio.h>
> >
> > Perhaps include it after system headers?
>
> hehe, I blindly sorted them.
> Should it matter?

I would include more particular headers later.
Btw system headers can not always be in order because of dependencies.

...

> > > +       local platform=`cat $SYSFS/kernel/debug/gpio | grep "$chip:" | tr -d ',' | awk '{print $5}'`
> >
> > Besides useless use of cat (and tr + awk can be simplified) why are
>
> What do you suggest for the tr/awk simplification?

You have `awk`, you can easily switch the entire pipeline to a little
awk scriptlet.

> > you simply not using
> > /sys/bus/gpio/devices/$chip ?
>
> Cos that shows all the gpiochips, not just the ones created by gpio-mockup.

I didn't get this. What is the content of $chip in your case?

> And I certainly don't want to go messing with real hardware.
> The default tests should still run on real hardware - but only
> accessing the mockup devices.
>
> Got a better way to filter out real hardware?

I probably have to understand what is the input and what is the
expected output. It's possible I missed something here.

> > > +       # e.g. /sys/class/gpio/gpiochip508/device/gpiochip0/dev
> > > +       local syschip=`ls -d $GPIO_SYSFS/gpiochip*/device/$chip/dev`
> >
> > ls -d is fragile, better to use `find ...`
>
> OK
>
> > > +       syschip=${syschip#$GPIO_SYSFS}
> > > +       syschip=${syschip%/device/$chip/dev}
> >
> > How does this handle more than one gpiochip listed?
>
> It is filtered by $chip so there can only be one.
> Or is that a false assumption?

When you have glob() in use it may return any number of results
(starting from 0) and your script should be prepared for that.

> > Also, can you consider optimizing these to get whatever you want easily?
>
> Sadly that IS my optimized way - I don't know of an easier way to find
> the sysfs GPIO number given the gpiochip and offset :-(.
> Happy to learn of any alternative.

I'm talking about getting $syschip. I think there is a way to get it
without all those shell substitutions from somewhere else.

> > > +       sysfs_nr=`cat $SYSFS/devices/$platform/gpio/$syschip/base`
> >
> > (It's probably fine here, but this doesn't work against PCI bus, for
> > example, see above for the fix)
>
> Not sure what you mean here.

When GPIO is a PCI device the above won't give a proper path.
If we wish to give an example to somebody, it would be better to have
it good enough.

> > > +       sysfs_nr=$(($sysfs_nr + $offset))
> > > +       sysfs_ldir=$GPIO_SYSFS/gpio$sysfs_nr
> > >  }

...

> > > +set_line()
> > >  {
> > > +       if [ -z "$sysfs_nr" ]; then
> > > +               find_sysfs_nr
> > > +               echo $sysfs_nr > $GPIO_SYSFS/export
> > >         fi
> >
> > It sounds like a separate function (you have release_line(), perhaps
> > acquire_line() is good to have).
> >
>
> The cdev implementation has to release and re-acquire in the background
> as there is no simple way to perform a set_config on a requested line
> from shell - just holding the requested line for a set is painful enough,
> and the goal here was to keep the tests simple.
>
> I didn't want to make line acquisition/release explicit in the gpio-mockup
> tests, as that would make them needlessly complicated, so the acquire is
> bundled into the set_line - and anywhere else the uAPI implementation
> needs it.  There is an implicit assumption that a set_line will always
> be called before a get_line, but that is always true - there is no
> "as-is" being tested here.
>
> Of course you still need the release_line at the end of the test, so
> that is still there.

Yes and to me logically correct to distinguish acquire_line() with set_line().
Then wherever you need to set_line(), you may call acquire_line()
which should be idempotent (the same way as release_line() call).

> > > +release_line()
> > >  {
> > > +       [ -z "$sysfs_nr" ] && return
> > > +       echo $sysfs_nr > $GPIO_SYSFS/unexport
> > > +       sysfs_nr=
> > > +       sysfs_ldir=
> > >  }

...

> > > +skip()
> > >  {
> >
> > > +       echo $* >&2
> >
> > In all cases better to use "$*" (note surrounding double quotes).
> >
>
> Agreed - except where
>
>         for option in $*; do
>
> is used to parse parameters.

Exactly! And "" helps with that.

If I put parameters as `a b c "d e"`, your case will take them wrongly.

> > > +       echo GPIO $module test SKIP
> > > +       exit $ksft_skip
> > >  }
> >
> > ...
> >
> > > +        [ ! which modprobe > /dev/null 2>&1 ] && skip "need modprobe installed"
> >
> > AFAIR `which` can be optional on some systems.
> >
>
> That is how other selftests check for availability of modprobe.
> e.g. selftests/kmod/kmod.sh and selftests/vm/test_hmm.sh, so I assumed
> it was acceptable.
>
> Is there an alternative?

OK. Just replace it with a dropped useless test call.
which ... || skip ...

...

> > P.S. Also you may use `#!/bin/sh -efu` as shebang and fix other problems.
>
> A shebang or a `set -efu`?

Shebang. The difference is that with shebang you don't need to edit
the script each time you want to change that.
sh -x /path/to/the/script will give different results.

> I don't see shebang options used anywhere in the selftest scripts, but I
> agree with a set.

Because shell scripts in the kernel are really badly written (so does
Python ones).
Again, even senior developers can't get shell right (including me).

> Either way I am unsure what the shebang should be.
> The majority of the selftest scripts use bash as the shebang, with the
> remainder using plain sh.
> These scripts do use some bash extensions, and it was originally bash, so
> I left it as that.
> My test setups mainly use busybox, and don't have bash, so they complain
> about the bash shebang - though the ash(??) busybox is using still runs
> the script fine.

I'm using busybox on an everyday basis and mentioned shebang works
there if I'm not mistaken.
Because all flags are listed in the standard.
https://pubs.opengroup.org/onlinepubs/007904875/utilities/sh.html

> Thanks again for the review - always a learning experience.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/7] selftests: gpio: rework and simplify test implementation

[Kent Gibson]

On Sun, Jan 03, 2021 at 05:10:10PM +0200, Andy Shevchenko wrote:
> On Sun, Jan 3, 2021 at 4:17 AM Kent Gibson <warthog618@gmail.com> wrote:
> > On Sun, Jan 03, 2021 at 12:20:26AM +0200, Andy Shevchenko wrote:
> > > On Sat, Jan 2, 2021 at 4:32 AM Kent Gibson <warthog618@gmail.com> wrote:
> 
> ...
> 
> > > > +#include <linux/gpio.h>
> > >
> > > Perhaps include it after system headers?
> >
> > hehe, I blindly sorted them.
> > Should it matter?
> 
> I would include more particular headers later.
> Btw system headers can not always be in order because of dependencies.
> 
> ...
> 
> > > > +       local platform=`cat $SYSFS/kernel/debug/gpio | grep "$chip:" | tr -d ',' | awk '{print $5}'`
> > >
> > > Besides useless use of cat (and tr + awk can be simplified) why are
> >
> > What do you suggest for the tr/awk simplification?
> 
> You have `awk`, you can easily switch the entire pipeline to a little
> awk scriptlet.
> 

Ah ok - I was actually going the other way to do away with the awk, so
had replaced it with a pair of cuts, though I'm still looking for better
alternatives for the whole gpiochipN:offset -> sysfs_nr mapping problem
- see below.

> > > you simply not using
> > > /sys/bus/gpio/devices/$chip ?
> >
> > Cos that shows all the gpiochips, not just the ones created by gpio-mockup.
> 
> I didn't get this. What is the content of $chip in your case?
> 

$chip is the gpiochipN name, so gpiochip0, gpiochip1 etc.

What we are trying to find here is the base of the GPIO numbering for
the chip so we can export/unexport them to sysfs (after adding the
offset for the particular line).

> > And I certainly don't want to go messing with real hardware.
> > The default tests should still run on real hardware - but only
> > accessing the mockup devices.
> >
> > Got a better way to filter out real hardware?
> 
> I probably have to understand what is the input and what is the
> expected output. It's possible I missed something here.
> 
> > > > +       # e.g. /sys/class/gpio/gpiochip508/device/gpiochip0/dev
> > > > +       local syschip=`ls -d $GPIO_SYSFS/gpiochip*/device/$chip/dev`
> > >
> > > ls -d is fragile, better to use `find ...`
> >
> > OK
> >
> > > > +       syschip=${syschip#$GPIO_SYSFS}
> > > > +       syschip=${syschip%/device/$chip/dev}
> > >
> > > How does this handle more than one gpiochip listed?
> >
> > It is filtered by $chip so there can only be one.
> > Or is that a false assumption?
> 
> When you have glob() in use it may return any number of results
> (starting from 0) and your script should be prepared for that.
> 

Yeah, we really don't want to be using globs at all.

> > > Also, can you consider optimizing these to get whatever you want easily?
> >
> > Sadly that IS my optimized way - I don't know of an easier way to find
> > the sysfs GPIO number given the gpiochip and offset :-(.
> > Happy to learn of any alternative.
> 
> I'm talking about getting $syschip. I think there is a way to get it
> without all those shell substitutions from somewhere else.
> 

$syschip is just an intermediate that I'm not really interested in - it
just helps find the base, and so the nr.

I've been playing with alternatives and my current one is:

	# e.g. /sys/devices/platform/gpio-mockup.1/gpiochip1
	local platform=$(find $SYSFS/devices/platform/ -name $chip -type d -maxdepth 2)
	[ "$platform" ] || fail "can't find platform of $chip"
	# e.g. /sys/devices/platform/gpio-mockup.1/gpio/gpiochip508/base
	local base=$(find $(dirname $platform)/gpio/ -name base -type f -maxdepth 2)
	[ "$base" ] || fail "can't find base of $chip"
	sysfs_nr=$(< $base)
	sysfs_nr=$(($sysfs_nr + $offset))

which works, though still doesn't handle the possibility of multiple
matches returned by the finds.

> > > > +       sysfs_nr=`cat $SYSFS/devices/$platform/gpio/$syschip/base`
> > >
> > > (It's probably fine here, but this doesn't work against PCI bus, for
> > > example, see above for the fix)
> >
> > Not sure what you mean here.
> 
> When GPIO is a PCI device the above won't give a proper path.
> If we wish to give an example to somebody, it would be better to have
> it good enough.
> 

How would it appear for PCI bus?

> > > > +       sysfs_nr=$(($sysfs_nr + $offset))
> > > > +       sysfs_ldir=$GPIO_SYSFS/gpio$sysfs_nr
> > > >  }
> 
> ...
> 
> > > > +set_line()
> > > >  {
> > > > +       if [ -z "$sysfs_nr" ]; then
> > > > +               find_sysfs_nr
> > > > +               echo $sysfs_nr > $GPIO_SYSFS/export
> > > >         fi
> > >
> > > It sounds like a separate function (you have release_line(), perhaps
> > > acquire_line() is good to have).
> > >
> >
> > The cdev implementation has to release and re-acquire in the background
> > as there is no simple way to perform a set_config on a requested line
> > from shell - just holding the requested line for a set is painful enough,
> > and the goal here was to keep the tests simple.
> >
> > I didn't want to make line acquisition/release explicit in the gpio-mockup
> > tests, as that would make them needlessly complicated, so the acquire is
> > bundled into the set_line - and anywhere else the uAPI implementation
> > needs it.  There is an implicit assumption that a set_line will always
> > be called before a get_line, but that is always true - there is no
> > "as-is" being tested here.
> >
> > Of course you still need the release_line at the end of the test, so
> > that is still there.
> 
> Yes and to me logically correct to distinguish acquire_line() with set_line().
> Then wherever you need to set_line(), you may call acquire_line()
> which should be idempotent (the same way as release_line() call).
> 

Oh, ok - it would only be called from set_line - I thought you meant
expose it as part of the uAPI test interface (currently
get_line/set_line/release_line).

> > > > +release_line()
> > > >  {
> > > > +       [ -z "$sysfs_nr" ] && return
> > > > +       echo $sysfs_nr > $GPIO_SYSFS/unexport
> > > > +       sysfs_nr=
> > > > +       sysfs_ldir=
> > > >  }
> 
> ...
> 
> > > > +skip()
> > > >  {
> > >
> > > > +       echo $* >&2
> > >
> > > In all cases better to use "$*" (note surrounding double quotes).
> > >
> >
> > Agreed - except where
> >
> >         for option in $*; do
> >
> > is used to parse parameters.
> 
> Exactly! And "" helps with that.
> 
> If I put parameters as `a b c "d e"`, your case will take them wrongly.
> 
> > > > +       echo GPIO $module test SKIP
> > > > +       exit $ksft_skip
> > > >  }
> > >
> > > ...
> > >
> > > > +        [ ! which modprobe > /dev/null 2>&1 ] && skip "need modprobe installed"
> > >
> > > AFAIR `which` can be optional on some systems.
> > >
> >
> > That is how other selftests check for availability of modprobe.
> > e.g. selftests/kmod/kmod.sh and selftests/vm/test_hmm.sh, so I assumed
> > it was acceptable.
> >
> > Is there an alternative?
> 
> OK. Just replace it with a dropped useless test call.
> which ... || skip ...
> 

Yup - I've since replaced it with a test call to modprobe -h, so no
`which` required.

> ...
> 
> > > P.S. Also you may use `#!/bin/sh -efu` as shebang and fix other problems.
> >
> > A shebang or a `set -efu`?
> 
> Shebang. The difference is that with shebang you don't need to edit
> the script each time you want to change that.
> sh -x /path/to/the/script will give different results.
> 

OK, didn't consider that.
Have got the scripts running with the -efu flags set - that was entertaining.

> > I don't see shebang options used anywhere in the selftest scripts, but I
> > agree with a set.
> 
> Because shell scripts in the kernel are really badly written (so does
> Python ones).
> Again, even senior developers can't get shell right (including me).
> 
> > Either way I am unsure what the shebang should be.
> > The majority of the selftest scripts use bash as the shebang, with the
> > remainder using plain sh.
> > These scripts do use some bash extensions, and it was originally bash, so
> > I left it as that.
> > My test setups mainly use busybox, and don't have bash, so they complain
> > about the bash shebang - though the ash(??) busybox is using still runs
> > the script fine.
> 
> I'm using busybox on an everyday basis and mentioned shebang works
> there if I'm not mistaken.
> Because all flags are listed in the standard.
> https://pubs.opengroup.org/onlinepubs/007904875/utilities/sh.html
> 

I meant the actual /bin/bash, not the flags.
Though I now build bash in my buildroots, so I don't get that warning
anymore.

Cheers,
Kent.

Re: [PATCH 1/7] selftests: gpio: rework and simplify test implementation

[Kent Gibson]

On Sun, Jan 03, 2021 at 05:10:10PM +0200, Andy Shevchenko wrote:
> On Sun, Jan 3, 2021 at 4:17 AM Kent Gibson <warthog618@gmail.com> wrote:
> > On Sun, Jan 03, 2021 at 12:20:26AM +0200, Andy Shevchenko wrote:
> > > On Sat, Jan 2, 2021 at 4:32 AM Kent Gibson <warthog618@gmail.com> wrote:
> 
[snip]
> 
> ...
> 
> > > > +       local platform=`cat $SYSFS/kernel/debug/gpio | grep "$chip:" | tr -d ',' | awk '{print $5}'`
> > >
> > > Besides useless use of cat (and tr + awk can be simplified) why are
> >
> > What do you suggest for the tr/awk simplification?
> 
> You have `awk`, you can easily switch the entire pipeline to a little
> awk scriptlet.
> 

Baah, the number that I'm after is in the $SYSFS/kernel/debug/gpio that I
was pulling the platform from, so I can just pull it directly from there.

No need to go hunting through the file system for the base file - the
range of GPIOs assigned to the chip is right there.

In this example it is the 508:

# e.g. gpiochip0: GPIOs 508-511, parent: platform/gpio-mockup.0, gpio-mockup-A:

So I'll use that - unless it is unreliable for some reason?

Cheers,
Kent.

Re: [PATCH 1/7] selftests: gpio: rework and simplify test implementation

[Andy Shevchenko]

On Mon, Jan 4, 2021 at 3:51 AM Kent Gibson <warthog618@gmail.com> wrote:
> On Sun, Jan 03, 2021 at 05:10:10PM +0200, Andy Shevchenko wrote:

...

> In this example it is the 508:
>
> # e.g. gpiochip0: GPIOs 508-511, parent: platform/gpio-mockup.0, gpio-mockup-A:
>
> So I'll use that - unless it is unreliable for some reason?

debugfs is not an ABI and tomorrow this can be changed without notice.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/7] selftests: gpio: rework and simplify test implementation

[Kent Gibson]

On Mon, Jan 04, 2021 at 03:52:49PM +0200, Andy Shevchenko wrote:
> On Mon, Jan 4, 2021 at 3:51 AM Kent Gibson <warthog618@gmail.com> wrote:
> > On Sun, Jan 03, 2021 at 05:10:10PM +0200, Andy Shevchenko wrote:
> 
> ...
> 
> > In this example it is the 508:
> >
> > # e.g. gpiochip0: GPIOs 508-511, parent: platform/gpio-mockup.0, gpio-mockup-A:
> >
> > So I'll use that - unless it is unreliable for some reason?
> 
> debugfs is not an ABI and tomorrow this can be changed without notice.
> 

I had a bad feeling that might be the case, and all my current solutions
use debugfs one way or another, so back to the drawing board on that one.

Thanks,
Kent.

Re: [PATCH 1/7] selftests: gpio: rework and simplify test implementation

[Kent Gibson]

On Mon, Jan 04, 2021 at 11:00:31PM +0800, Kent Gibson wrote:
> On Mon, Jan 04, 2021 at 03:52:49PM +0200, Andy Shevchenko wrote:
> > On Mon, Jan 4, 2021 at 3:51 AM Kent Gibson <warthog618@gmail.com> wrote:
> > > On Sun, Jan 03, 2021 at 05:10:10PM +0200, Andy Shevchenko wrote:
> > 
> > ...
> > 
> > > In this example it is the 508:
> > >
> > > # e.g. gpiochip0: GPIOs 508-511, parent: platform/gpio-mockup.0, gpio-mockup-A:
> > >
> > > So I'll use that - unless it is unreliable for some reason?
> > 
> > debugfs is not an ABI and tomorrow this can be changed without notice.
> > 
> 
> I had a bad feeling that might be the case, and all my current solutions
> use debugfs one way or another, so back to the drawing board on that one.
> 

Hang on - the find approach that I was looking at previously only uses
/sys/devices/platform, so I'll revert to that one - and add handling for
multi-match.

Cheers,
Kent.

[PATCH 2/7] selftests: gpio: remove obsolete gpio-mockup-chardev.c

[Kent Gibson]

GPIO selftests have changed to new gpio-mockup-cdev helper, so remove
old gpio-mockup-chardev helper.

Signed-off-by: Kent Gibson <warthog618@gmail.com>
---
 .../selftests/gpio/gpio-mockup-chardev.c      | 323 ------------------
 1 file changed, 323 deletions(-)
 delete mode 100644 tools/testing/selftests/gpio/gpio-mockup-chardev.c

diff --git a/tools/testing/selftests/gpio/gpio-mockup-chardev.c b/tools/testing/selftests/gpio/gpio-mockup-chardev.c
deleted file mode 100644
index 73ead8828d3a..000000000000
--- a/tools/testing/selftests/gpio/gpio-mockup-chardev.c
+++ /dev/null
@@ -1,323 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0-only
-/*
- * GPIO chardev test helper
- *
- * Copyright (C) 2016 Bamvor Jian Zhang
- */
-
-#define _GNU_SOURCE
-#include <unistd.h>
-#include <stdio.h>
-#include <stdlib.h>
-#include <errno.h>
-#include <string.h>
-#include <fcntl.h>
-#include <getopt.h>
-#include <sys/ioctl.h>
-#include <libmount.h>
-#include <err.h>
-#include <dirent.h>
-#include <linux/gpio.h>
-#include "../../../gpio/gpio-utils.h"
-
-#define CONSUMER	"gpio-selftest"
-#define	GC_NUM		10
-enum direction {
-	OUT,
-	IN
-};
-
-static int get_debugfs(char **path)
-{
-	struct libmnt_context *cxt;
-	struct libmnt_table *tb;
-	struct libmnt_iter *itr = NULL;
-	struct libmnt_fs *fs;
-	int found = 0, ret;
-
-	cxt = mnt_new_context();
-	if (!cxt)
-		err(EXIT_FAILURE, "libmount context allocation failed");
-
-	itr = mnt_new_iter(MNT_ITER_FORWARD);
-	if (!itr)
-		err(EXIT_FAILURE, "failed to initialize libmount iterator");
-
-	if (mnt_context_get_mtab(cxt, &tb))
-		err(EXIT_FAILURE, "failed to read mtab");
-
-	while (mnt_table_next_fs(tb, itr, &fs) == 0) {
-		const char *type = mnt_fs_get_fstype(fs);
-
-		if (!strcmp(type, "debugfs")) {
-			found = 1;
-			break;
-		}
-	}
-	if (found) {
-		ret = asprintf(path, "%s/gpio", mnt_fs_get_target(fs));
-		if (ret < 0)
-			err(EXIT_FAILURE, "failed to format string");
-	}
-
-	mnt_free_iter(itr);
-	mnt_free_context(cxt);
-
-	if (!found)
-		return -1;
-
-	return 0;
-}
-
-static int gpio_debugfs_get(const char *consumer, int *dir, int *value)
-{
-	char *debugfs;
-	FILE *f;
-	char *line = NULL;
-	size_t len = 0;
-	char *cur;
-	int found = 0;
-
-	if (get_debugfs(&debugfs) != 0)
-		err(EXIT_FAILURE, "debugfs is not mounted");
-
-	f = fopen(debugfs, "r");
-	if (!f)
-		err(EXIT_FAILURE, "read from gpio debugfs failed");
-
-	/*
-	 * gpio-2   (                    |gpio-selftest               ) in  lo
-	 */
-	while (getline(&line, &len, f) != -1) {
-		cur = strstr(line, consumer);
-		if (cur == NULL)
-			continue;
-
-		cur = strchr(line, ')');
-		if (!cur)
-			continue;
-
-		cur += 2;
-		if (!strncmp(cur, "out", 3)) {
-			*dir = OUT;
-			cur += 4;
-		} else if (!strncmp(cur, "in", 2)) {
-			*dir = IN;
-			cur += 4;
-		}
-
-		if (!strncmp(cur, "hi", 2))
-			*value = 1;
-		else if (!strncmp(cur, "lo", 2))
-			*value = 0;
-
-		found = 1;
-		break;
-	}
-	free(debugfs);
-	fclose(f);
-	free(line);
-
-	if (!found)
-		return -1;
-
-	return 0;
-}
-
-static struct gpiochip_info *list_gpiochip(const char *gpiochip_name, int *ret)
-{
-	struct gpiochip_info *cinfo;
-	struct gpiochip_info *current;
-	const struct dirent *ent;
-	DIR *dp;
-	char *chrdev_name;
-	int fd;
-	int i = 0;
-
-	cinfo = calloc(sizeof(struct gpiochip_info) * 4, GC_NUM + 1);
-	if (!cinfo)
-		err(EXIT_FAILURE, "gpiochip_info allocation failed");
-
-	current = cinfo;
-	dp = opendir("/dev");
-	if (!dp) {
-		*ret = -errno;
-		goto error_out;
-	} else {
-		*ret = 0;
-	}
-
-	while (ent = readdir(dp), ent) {
-		if (check_prefix(ent->d_name, "gpiochip")) {
-			*ret = asprintf(&chrdev_name, "/dev/%s", ent->d_name);
-			if (*ret < 0)
-				goto error_out;
-
-			fd = open(chrdev_name, 0);
-			if (fd == -1) {
-				*ret = -errno;
-				fprintf(stderr, "Failed to open %s\n",
-					chrdev_name);
-				goto error_close_dir;
-			}
-			*ret = ioctl(fd, GPIO_GET_CHIPINFO_IOCTL, current);
-			if (*ret == -1) {
-				perror("Failed to issue CHIPINFO IOCTL\n");
-				goto error_close_dir;
-			}
-			close(fd);
-			if (strcmp(current->label, gpiochip_name) == 0
-			    || check_prefix(current->label, gpiochip_name)) {
-				*ret = 0;
-				current++;
-				i++;
-			}
-		}
-	}
-
-	if ((!*ret && i == 0) || *ret < 0) {
-		free(cinfo);
-		cinfo = NULL;
-	}
-	if (!*ret && i > 0) {
-		cinfo = realloc(cinfo, sizeof(struct gpiochip_info) * 4 * i);
-		*ret = i;
-	}
-
-error_close_dir:
-	closedir(dp);
-error_out:
-	if (*ret < 0)
-		err(EXIT_FAILURE, "list gpiochip failed: %s", strerror(*ret));
-
-	return cinfo;
-}
-
-int gpio_pin_test(struct gpiochip_info *cinfo, int line, int flag, int value)
-{
-	struct gpiohandle_data data;
-	unsigned int lines[] = {line};
-	int fd;
-	int debugfs_dir = IN;
-	int debugfs_value = 0;
-	int ret;
-
-	data.values[0] = value;
-	ret = gpiotools_request_linehandle(cinfo->name, lines, 1, flag, &data,
-					   CONSUMER);
-	if (ret < 0)
-		goto fail_out;
-	else
-		fd = ret;
-
-	ret = gpio_debugfs_get(CONSUMER, &debugfs_dir, &debugfs_value);
-	if (ret) {
-		ret = -EINVAL;
-		goto fail_out;
-	}
-	if (flag & GPIOHANDLE_REQUEST_INPUT) {
-		if (debugfs_dir != IN) {
-			errno = -EINVAL;
-			ret = -errno;
-		}
-	} else if (flag & GPIOHANDLE_REQUEST_OUTPUT) {
-		if (flag & GPIOHANDLE_REQUEST_ACTIVE_LOW)
-			debugfs_value = !debugfs_value;
-
-		if (!(debugfs_dir == OUT && value == debugfs_value)) {
-			errno = -EINVAL;
-			ret = -errno;
-		}
-	}
-	gpiotools_release_linehandle(fd);
-
-fail_out:
-	if (ret)
-		err(EXIT_FAILURE, "gpio<%s> line<%d> test flag<0x%x> value<%d>",
-		    cinfo->name, line, flag, value);
-
-	return ret;
-}
-
-void gpio_pin_tests(struct gpiochip_info *cinfo, unsigned int line)
-{
-	printf("line<%d>", line);
-	gpio_pin_test(cinfo, line, GPIOHANDLE_REQUEST_OUTPUT, 0);
-	printf(".");
-	gpio_pin_test(cinfo, line, GPIOHANDLE_REQUEST_OUTPUT, 1);
-	printf(".");
-	gpio_pin_test(cinfo, line,
-		      GPIOHANDLE_REQUEST_OUTPUT | GPIOHANDLE_REQUEST_ACTIVE_LOW,
-		      0);
-	printf(".");
-	gpio_pin_test(cinfo, line,
-		      GPIOHANDLE_REQUEST_OUTPUT | GPIOHANDLE_REQUEST_ACTIVE_LOW,
-		      1);
-	printf(".");
-	gpio_pin_test(cinfo, line, GPIOHANDLE_REQUEST_INPUT, 0);
-	printf(".");
-}
-
-/*
- * ./gpio-mockup-chardev gpio_chip_name_prefix is_valid_gpio_chip
- * Return 0 if successful or exit with EXIT_FAILURE if test failed.
- * gpio_chip_name_prefix: The prefix of gpiochip you want to test. E.g.
- *			  gpio-mockup
- * is_valid_gpio_chip:	  Whether the gpio_chip is valid. 1 means valid,
- *			  0 means invalid which could not be found by
- *			  list_gpiochip.
- */
-int main(int argc, char *argv[])
-{
-	char *prefix;
-	int valid;
-	struct gpiochip_info *cinfo;
-	struct gpiochip_info *current;
-	int i;
-	int ret;
-
-	if (argc < 3) {
-		printf("Usage: %s prefix is_valid", argv[0]);
-		exit(EXIT_FAILURE);
-	}
-
-	prefix = argv[1];
-	valid = strcmp(argv[2], "true") == 0 ? 1 : 0;
-
-	printf("Test gpiochip %s: ", prefix);
-	cinfo = list_gpiochip(prefix, &ret);
-	if (!cinfo) {
-		if (!valid && ret == 0) {
-			printf("Invalid test successful\n");
-			ret = 0;
-			goto out;
-		} else {
-			ret = -EINVAL;
-			goto out;
-		}
-	} else if (cinfo && !valid) {
-		ret = -EINVAL;
-		goto out;
-	}
-	current = cinfo;
-	for (i = 0; i < ret; i++) {
-		gpio_pin_tests(current, 0);
-		gpio_pin_tests(current, current->lines - 1);
-		gpio_pin_tests(current, random() % current->lines);
-		current++;
-	}
-	ret = 0;
-	printf("successful\n");
-
-out:
-	if (ret)
-		fprintf(stderr, "gpio<%s> test failed\n", prefix);
-
-	if (cinfo)
-		free(cinfo);
-
-	if (ret)
-		exit(EXIT_FAILURE);
-
-	return ret;
-}
-- 
2.30.0

[PATCH 3/7] selftests: remove obsolete build restriction for gpio

[Kent Gibson]

Build restrictions related to the gpio-mockup-chardev helper are
no longer relevant so remove them.

Signed-off-by: Kent Gibson <warthog618@gmail.com>
---
 tools/testing/selftests/Makefile | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
index d9c283503159..5411041e63a0 100644
--- a/tools/testing/selftests/Makefile
+++ b/tools/testing/selftests/Makefile
@@ -121,15 +121,6 @@ ARCH           ?= $(SUBARCH)
 export KSFT_KHDR_INSTALL_DONE := 1
 export BUILD
 
-# build and run gpio when output directory is the src dir.
-# gpio has dependency on tools/gpio and builds tools/gpio
-# objects in the src directory in all cases making the src
-# repo dirty even when objects are relocated.
-ifneq (1,$(DEFAULT_INSTALL_HDR_PATH))
-	TMP := $(filter-out gpio, $(TARGETS))
-	TARGETS := $(TMP)
-endif
-
 # set default goal to all, so make without a target runs all, even when
 # all isn't the first target in the file.
 .DEFAULT_GOAL := all
-- 
2.30.0

[PATCH 4/7] selftests: remove obsolete gpio references from kselftest_deps.sh

[Kent Gibson]

GPIO Makefile has been greatly simplified so remove references to lines
which no longer exist.

Signed-off-by: Kent Gibson <warthog618@gmail.com>
---
 tools/testing/selftests/kselftest_deps.sh | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/tools/testing/selftests/kselftest_deps.sh b/tools/testing/selftests/kselftest_deps.sh
index bbc04646346b..00e60d6eb16b 100755
--- a/tools/testing/selftests/kselftest_deps.sh
+++ b/tools/testing/selftests/kselftest_deps.sh
@@ -129,13 +129,11 @@ l2_tests=$(grep -r --include=Makefile ": LDLIBS" | \
 		grep -v "VAR_LDLIBS" | awk -F: '{print $1}')
 
 # Level 3
-# gpio,  memfd and others use pkg-config to find mount and fuse libs
+# memfd and others use pkg-config to find mount and fuse libs
 # respectively and save it in VAR_LDLIBS. If pkg-config doesn't find
 # any, VAR_LDLIBS set to default.
 # Use the default value and filter out pkg-config for dependency check.
 # e.g:
-# gpio/Makefile
-#	VAR_LDLIBS := $(shell pkg-config --libs mount) 2>/dev/null)
 # memfd/Makefile
 #	VAR_LDLIBS := $(shell pkg-config fuse --libs 2>/dev/null)
 
-- 
2.30.0

[PATCH 5/7] tools: gpio: remove uAPI v1 code no longer used by selftests

[Kent Gibson]

gpio-mockup-chardev helper has been obsoleted and removed, so also remove
the tools/gpio code that it, and nothing else, was using.

Signed-off-by: Kent Gibson <warthog618@gmail.com>
---
 tools/gpio/gpio-utils.c | 89 -----------------------------------------
 tools/gpio/gpio-utils.h |  6 ---
 2 files changed, 95 deletions(-)

diff --git a/tools/gpio/gpio-utils.c b/tools/gpio/gpio-utils.c
index 37187e056c8b..1639b4d832cd 100644
--- a/tools/gpio/gpio-utils.c
+++ b/tools/gpio/gpio-utils.c
@@ -32,74 +32,6 @@
  * following api will request gpio lines, do the operation and then
  * release these lines.
  */
-/**
- * gpiotools_request_linehandle() - request gpio lines in a gpiochip
- * @device_name:	The name of gpiochip without prefix "/dev/",
- *			such as "gpiochip0"
- * @lines:		An array desired lines, specified by offset
- *			index for the associated GPIO device.
- * @num_lines:		The number of lines to request.
- * @flag:		The new flag for requsted gpio. Reference
- *			"linux/gpio.h" for the meaning of flag.
- * @data:		Default value will be set to gpio when flag is
- *			GPIOHANDLE_REQUEST_OUTPUT.
- * @consumer_label:	The name of consumer, such as "sysfs",
- *			"powerkey". This is useful for other users to
- *			know who is using.
- *
- * Request gpio lines through the ioctl provided by chardev. User
- * could call gpiotools_set_values() and gpiotools_get_values() to
- * read and write respectively through the returned fd. Call
- * gpiotools_release_linehandle() to release these lines after that.
- *
- * Return:		On success return the fd;
- *			On failure return the errno.
- */
-int gpiotools_request_linehandle(const char *device_name, unsigned int *lines,
-				 unsigned int num_lines, unsigned int flag,
-				 struct gpiohandle_data *data,
-				 const char *consumer_label)
-{
-	struct gpiohandle_request req;
-	char *chrdev_name;
-	int fd;
-	int i;
-	int ret;
-
-	ret = asprintf(&chrdev_name, "/dev/%s", device_name);
-	if (ret < 0)
-		return -ENOMEM;
-
-	fd = open(chrdev_name, 0);
-	if (fd == -1) {
-		ret = -errno;
-		fprintf(stderr, "Failed to open %s, %s\n",
-			chrdev_name, strerror(errno));
-		goto exit_free_name;
-	}
-
-	for (i = 0; i < num_lines; i++)
-		req.lineoffsets[i] = lines[i];
-
-	req.flags = flag;
-	strcpy(req.consumer_label, consumer_label);
-	req.lines = num_lines;
-	if (flag & GPIOHANDLE_REQUEST_OUTPUT)
-		memcpy(req.default_values, data, sizeof(req.default_values));
-
-	ret = ioctl(fd, GPIO_GET_LINEHANDLE_IOCTL, &req);
-	if (ret == -1) {
-		ret = -errno;
-		fprintf(stderr, "Failed to issue %s (%d), %s\n",
-			"GPIO_GET_LINEHANDLE_IOCTL", ret, strerror(errno));
-	}
-
-	if (close(fd) == -1)
-		perror("Failed to close GPIO character device file");
-exit_free_name:
-	free(chrdev_name);
-	return ret < 0 ? ret : req.fd;
-}
 
 /**
  * gpiotools_request_line() - request gpio lines in a gpiochip
@@ -215,27 +147,6 @@ int gpiotools_get_values(const int fd, struct gpio_v2_line_values *values)
 	return ret;
 }
 
-/**
- * gpiotools_release_linehandle(): Release the line(s) of gpiochip
- * @fd:			The fd returned by
- *			gpiotools_request_linehandle().
- *
- * Return:		On success return 0;
- *			On failure return the errno.
- */
-int gpiotools_release_linehandle(const int fd)
-{
-	int ret;
-
-	ret = close(fd);
-	if (ret == -1) {
-		perror("Failed to close GPIO LINEHANDLE device file");
-		ret = -errno;
-	}
-
-	return ret;
-}
-
 /**
  * gpiotools_release_line(): Release the line(s) of gpiochip
  * @fd:			The fd returned by
diff --git a/tools/gpio/gpio-utils.h b/tools/gpio/gpio-utils.h
index 6c69a9f1c253..8af7c8ee19ce 100644
--- a/tools/gpio/gpio-utils.h
+++ b/tools/gpio/gpio-utils.h
@@ -24,12 +24,6 @@ static inline int check_prefix(const char *str, const char *prefix)
 		strncmp(str, prefix, strlen(prefix)) == 0;
 }
 
-int gpiotools_request_linehandle(const char *device_name, unsigned int *lines,
-				 unsigned int num_lines, unsigned int flag,
-				 struct gpiohandle_data *data,
-				 const char *consumer_label);
-int gpiotools_release_linehandle(const int fd);
-
 int gpiotools_request_line(const char *device_name,
 			   unsigned int *lines,
 			   unsigned int num_lines,
-- 
2.30.0

[PATCH 6/7] selftests: gpio: port to GPIO uAPI v2

[Kent Gibson]

Add a port to the GPIO uAPI v2 interface and make it the default.

Signed-off-by: Kent Gibson <warthog618@gmail.com>
---
 .../testing/selftests/gpio/gpio-mockup-cdev.c | 75 +++++++++++++++++--
 tools/testing/selftests/gpio/gpio-mockup.sh   | 11 ++-
 2 files changed, 76 insertions(+), 10 deletions(-)

diff --git a/tools/testing/selftests/gpio/gpio-mockup-cdev.c b/tools/testing/selftests/gpio/gpio-mockup-cdev.c
index 3bfd876a8b6a..e8e3d2ec662c 100644
--- a/tools/testing/selftests/gpio/gpio-mockup-cdev.c
+++ b/tools/testing/selftests/gpio/gpio-mockup-cdev.c
@@ -18,6 +18,44 @@
 
 #define CONSUMER	"gpio-mockup-cdev"
 
+static int request_line_v2(int cfd, unsigned int offset,
+			   uint64_t flags, unsigned int val)
+{
+	struct gpio_v2_line_request req;
+	int ret;
+
+	memset(&req, 0, sizeof(req));
+	req.num_lines = 1;
+	req.offsets[0] = offset;
+	req.config.flags = flags;
+	strcpy(req.consumer, CONSUMER);
+	if (flags & GPIO_V2_LINE_FLAG_OUTPUT) {
+		req.config.num_attrs = 1;
+		req.config.attrs[0].mask = 1;
+		req.config.attrs[0].attr.id = GPIO_V2_LINE_ATTR_ID_OUTPUT_VALUES;
+		if (val)
+			req.config.attrs[0].attr.values = 1;
+	}
+	ret = ioctl(cfd, GPIO_V2_GET_LINE_IOCTL, &req);
+	if (ret == -1)
+		return -errno;
+	return req.fd;
+}
+
+
+static int get_value_v2(int lfd)
+{
+	struct gpio_v2_line_values vals;
+	int ret;
+
+	memset(&vals, 0, sizeof(vals));
+	vals.mask = 1;
+	ret = ioctl(lfd, GPIO_V2_LINE_GET_VALUES_IOCTL, &vals);
+	if (ret == -1)
+		return -errno;
+	return vals.bits & 0x1;
+}
+
 static int request_line_v1(int cfd, unsigned int offset,
 			   uint32_t flags, unsigned int val)
 {
@@ -57,6 +95,7 @@ static void usage(char *prog)
 	printf("               (default is to leave bias unchanged):\n");
 	printf("        -l: set line active low (default is active high)\n");
 	printf("        -s: set line value (default is to get line value)\n");
+	printf("        -u: uAPI version to use (default is 2)\n");
 	exit(-1);
 }
 
@@ -78,29 +117,42 @@ int main(int argc, char *argv[])
 {
 	char *chip;
 	int opt, ret, cfd, lfd;
-	unsigned int offset, val;
+	unsigned int offset, val, abiv;
 	uint32_t flags_v1;
+	uint64_t flags_v2;
 
+	abiv = 2;
 	ret = 0;
 	flags_v1 = GPIOHANDLE_REQUEST_INPUT;
+	flags_v2 = GPIO_V2_LINE_FLAG_INPUT;
 
 	while ((opt = getopt(argc, argv, "lb:s:u:")) != -1) {
 		switch (opt) {
 		case 'l':
 			flags_v1 |= GPIOHANDLE_REQUEST_ACTIVE_LOW;
+			flags_v2 |= GPIO_V2_LINE_FLAG_ACTIVE_LOW;
 			break;
 		case 'b':
-			if (strcmp("pull-up", optarg) == 0)
+			if (strcmp("pull-up", optarg) == 0) {
 				flags_v1 |= GPIOHANDLE_REQUEST_BIAS_PULL_UP;
-			else if (strcmp("pull-down", optarg) == 0)
+				flags_v2 |= GPIO_V2_LINE_FLAG_BIAS_PULL_UP;
+			} else if (strcmp("pull-down", optarg) == 0) {
 				flags_v1 |= GPIOHANDLE_REQUEST_BIAS_PULL_DOWN;
-			else if (strcmp("disabled", optarg) == 0)
+				flags_v2 |= GPIO_V2_LINE_FLAG_BIAS_PULL_DOWN;
+			} else if (strcmp("disabled", optarg) == 0) {
 				flags_v1 |= GPIOHANDLE_REQUEST_BIAS_DISABLE;
+				flags_v2 |= GPIO_V2_LINE_FLAG_BIAS_DISABLED;
+			}
 			break;
 		case 's':
 			val = atoi(optarg);
 			flags_v1 &= ~GPIOHANDLE_REQUEST_INPUT;
 			flags_v1 |= GPIOHANDLE_REQUEST_OUTPUT;
+			flags_v2 &= ~GPIO_V2_LINE_FLAG_INPUT;
+			flags_v2 |= GPIO_V2_LINE_FLAG_OUTPUT;
+			break;
+		case 'u':
+			abiv = atoi(optarg);
 			break;
 		default:
 			usage(argv[0]);
@@ -119,7 +171,10 @@ int main(int argc, char *argv[])
 		return -errno;
 	}
 
-	lfd = request_line_v1(cfd, offset, flags_v1, val);
+	if (abiv == 1)
+		lfd = request_line_v1(cfd, offset, flags_v1, val);
+	else
+		lfd = request_line_v2(cfd, offset, flags_v2, val);
 
 	close(cfd);
 
@@ -128,10 +183,14 @@ int main(int argc, char *argv[])
 		return lfd;
 	}
 
-	if (flags_v1 & GPIOHANDLE_REQUEST_OUTPUT)
+	if (flags_v2 & GPIO_V2_LINE_FLAG_OUTPUT) {
 		wait_signal();
-	else
-		ret = get_value_v1(lfd);
+	} else {
+		if (abiv == 1)
+			ret = get_value_v1(lfd);
+		else
+			ret = get_value_v2(lfd);
+	}
 
 	close(lfd);
 
diff --git a/tools/testing/selftests/gpio/gpio-mockup.sh b/tools/testing/selftests/gpio/gpio-mockup.sh
index 66eed9b60963..843f829b3dd8 100755
--- a/tools/testing/selftests/gpio/gpio-mockup.sh
+++ b/tools/testing/selftests/gpio/gpio-mockup.sh
@@ -13,6 +13,7 @@ dev_type="cdev"
 module="gpio-mockup"
 verbose=
 random=
+uapi_opt=
 active_opt=
 bias_opt=
 line_set_pid=
@@ -29,6 +30,7 @@ usage()
 	echo "-r:  test random lines as well as fence posts"
 	echo "-t:  interface type:"
 	echo "      cdev (character device ABI) - default"
+	echo "      cdev_v1 (deprecated character device ABI)"
 	echo "      sysfs (deprecated SYSFS ABI)"
 	echo "-v:  verbose progress reporting"
 	exit $ksft_fail
@@ -102,7 +104,8 @@ get_line()
 {
 	release_line
 
-	$BASE/gpio-mockup-cdev $active_opt /dev/$chip $offset > /dev/null 2>&1
+	local cdev_opts=${uapi_opt}${active_opt}
+	$BASE/gpio-mockup-cdev $cdev_opts /dev/$chip $offset > /dev/null 2>&1
 	echo $?
 }
 
@@ -144,7 +147,7 @@ set_line()
 		esac
 	done
 
-	local cdev_opts=${active_opt}
+	local cdev_opts=${uapi_opt}${active_opt}
 	if [ "$val" ]; then
 		$BASE/gpio-mockup-cdev $cdev_opts -s$val /dev/$chip $offset 2>&1 >/dev/null &
 		# failure to set is detected by reading mock and toggling values
@@ -314,6 +317,10 @@ sysfs)
 	source $BASE/gpio-mockup-sysfs.sh
 	echo "WARNING: gpio sysfs ABI is deprecated."
 	;;
+cdev_v1)
+	echo "WARNING: gpio cdev ABI v1 is deprecated."
+	uapi_opt="-u1 "
+	;;
 cdev)
 	;;
 *)
-- 
2.30.0

[PATCH 7/7] selftests: gpio: add CONFIG_GPIO_CDEV to config

[Kent Gibson]

GPIO CDEV is now optional and required for the selftests so add it to
the config.

Signed-off-by: Kent Gibson <warthog618@gmail.com>
---
 tools/testing/selftests/gpio/config | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/testing/selftests/gpio/config b/tools/testing/selftests/gpio/config
index abaa6902b7b6..ce100342c20b 100644
--- a/tools/testing/selftests/gpio/config
+++ b/tools/testing/selftests/gpio/config
@@ -1,2 +1,3 @@
 CONFIG_GPIOLIB=y
+CONFIG_GPIO_CDEV=y
 CONFIG_GPIO_MOCKUP=m
-- 
2.30.0

Re: [PATCH 0/7] selftests: gpio: rework and port to GPIO uAPI v2

[Linus Walleij]

On Sat, Jan 2, 2021 at 3:30 AM Kent Gibson <warthog618@gmail.com> wrote:

> Initially I just wanted to port the selftests to the latest GPIO uAPI,
> but on finding that, due to dependency issues, the selftests are not built
> for the buildroot environments that I do most of my GPIO testing in, I
> decided to take a closer look.

All patches look good to me, I see Andy is helping you to hash out
some shell script, anyway:
Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij