Message-ID: <20210103021725.GA6622@sol>
Date:   Sun, 3 Jan 2021 10:17:25 +0800
From:   Kent Gibson <warthog618@...il.com>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@...r.kernel.org>,
        Bartosz Golaszewski <bgolaszewski@...libre.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        Shuah Khan <shuah@...nel.org>,
        Bamvor Jian Zhang <bamv2005@...il.com>
Subject: Re: [PATCH 1/7] selftests: gpio: rework and simplify test
 implementation

On Sun, Jan 03, 2021 at 12:20:26AM +0200, Andy Shevchenko wrote:
> On Sat, Jan 2, 2021 at 4:32 AM Kent Gibson <warthog618@...il.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.
> 

You are telling me - there are about six million ways to do even the
most trivial tasks.  Makes you appreciate more constrained languages.

> ...
> 
> > +#include <linux/gpio.h>
> 
> Perhaps include it after system headers?
> 

hehe, I blindly sorted them.
Should it matter?

> > +#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' '
> 

That looks good - the other is a carry over from the old gpio-mockup.sh.

> ...
> 
> > +[ ! -d "$SYSFS" ] && skip "sysfs is not mounted"
> 
> [ -d ... ] || skip "..."
> 

Yeah, those were if [ .. ]; then fi originally. I did the first step
of simplification and missed the second :-(.

> ...
> 
> > +[ ! -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

What do you suggest for the tr/awk simplification?

> you simply not using
> /sys/bus/gpio/devices/$chip ?
> 

Cos that shows all the gpiochips, not just the ones created by gpio-mockup.
And I certainly don't want to go messing with real hardware.
The default tests should still run on real hardware - but only
accessing the mockup devices.

Got a better way to filter out real hardware?

> > +       # 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 ...`
> 

OK 

> > +       syschip=${syschip#$GPIO_SYSFS}
> > +       syschip=${syschip%/device/$chip/dev}
> 
> How does this handle more than one gpiochip listed?

It is filtered by $chip so there can only be one.
Or is that a false assumption?

> Also, can you consider optimizing these to get whatever you want easily?
> 

Sadly that IS my optimized way - I don't know of an easier way to find
the sysfs GPIO number given the gpiochip and offset :-(.
Happy to learn of any alternative.

> > +       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)
> 

Not sure what you mean here.

> > +       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).
> 

The cdev implementation has to release and re-acquire in the background
as there is no simple way to perform a set_config on a requested line
from shell - just holding the requested line for a set is painful enough,
and the goal here was to keep the tests simple.

I didn't want to make line acquisition/release explicit in the gpio-mockup
tests, as that would make them needlessly complicated, so the acquire is
bundled into the set_line - and anywhere else the uAPI implementation
needs it.  There is an implicit assumption that a set_line will always
be called before a get_line, but that is always true - there is no
"as-is" being tested here.

Of course you still need the release_line at the end of the test, so
that is still there.

> > +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.
> 

Yup

> ...
> 
> > +skip()
> >  {
> 
> > +       echo $* >&2
> 
> In all cases better to use "$*" (note surrounding double quotes).
> 

Agreed - except where

	for option in $*; do

is used to parse parameters.

> > +       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.
> 

That is how other selftests check for availability of modprobe.
e.g. selftests/kmod/kmod.sh and selftests/vm/test_hmm.sh, so I assumed
it was acceptable.

Is there an alternative?

> ...
> 
> > +       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 "... $?"
> 

Yup

> > +}
> 
> ...
> 
> > +       [ ! -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.
> 

Agreed - the gc certainly shouldn't be splitting the two.

> ...
> 
> > +       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.
> 

Ahh, replacing the && with -a. Good to know.

> > +                       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" ]
> 

I'm going with empty for false, so you can drop the = "true" here.

> P.S. Also you may use `#!/bin/sh -efu` as shebang and fix other problems.
> 

A shebang or a `set -efu`?
I don't see shebang options used anywhere in the selftest scripts, but I
agree with a set.

Either way I am unsure what the shebang should be.
The majority of the selftest scripts use bash as the shebang, with the
remainder using plain sh.
These scripts do use some bash extensions, and it was originally bash, so
I left it as that.
My test setups mainly use busybox, and don't have bash, so they complain
about the bash shebang - though the ash(??) busybox is using still runs
the script fine.

Thanks again for the review - always a learning experience.

Cheers,
Kent.

Powered by blists - more mailing lists