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

Andy Shevchenko
2021-01-02T22:21:40Z

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