On Sat, Jan 2, 2021 at 4:32 AM Kent Gibson [off-list ref] 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