Messages in this thread | | | From | Andy Shevchenko <> | Date | Sun, 3 Jan 2021 00:20:26 +0200 | Subject | Re: [PATCH 1/7] selftests: gpio: rework and simplify test implementation |
| |
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
| |