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

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Sat, Jan 2, 2021 at 4:32 AM Kent Gibson <warthog618@xxxxxxxxx> 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