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