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

[Kent Gibson <warthog618@xxxxxxxxx>]

On Sun, Jan 03, 2021 at 05:10:10PM +0200, Andy Shevchenko wrote:
> On Sun, Jan 3, 2021 at 4:17 AM Kent Gibson <warthog618@xxxxxxxxx> wrote:
> > 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@xxxxxxxxx> wrote:
> 
> ...
> 
> > > > +#include <linux/gpio.h>
> > >
> > > Perhaps include it after system headers?
> >
> > hehe, I blindly sorted them.
> > Should it matter?
> 
> I would include more particular headers later.
> Btw system headers can not always be in order because of dependencies.
> 
> ...
> 
> > > > +       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 have `awk`, you can easily switch the entire pipeline to a little
> awk scriptlet.
> 

Ah ok - I was actually going the other way to do away with the awk, so
had replaced it with a pair of cuts, though I'm still looking for better
alternatives for the whole gpiochipN:offset -> sysfs_nr mapping problem
- see below.

> > > you simply not using
> > > /sys/bus/gpio/devices/$chip ?
> >
> > Cos that shows all the gpiochips, not just the ones created by gpio-mockup.
> 
> I didn't get this. What is the content of $chip in your case?
> 

$chip is the gpiochipN name, so gpiochip0, gpiochip1 etc.

What we are trying to find here is the base of the GPIO numbering for
the chip so we can export/unexport them to sysfs (after adding the
offset for the particular line).

> > 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?
> 
> I probably have to understand what is the input and what is the
> expected output. It's possible I missed something here.
> 
> > > > +       # 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?
> 
> When you have glob() in use it may return any number of results
> (starting from 0) and your script should be prepared for that.
> 

Yeah, we really don't want to be using globs at all.

> > > 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.
> 
> I'm talking about getting $syschip. I think there is a way to get it
> without all those shell substitutions from somewhere else.
> 

$syschip is just an intermediate that I'm not really interested in - it
just helps find the base, and so the nr.

I've been playing with alternatives and my current one is:

	# e.g. /sys/devices/platform/gpio-mockup.1/gpiochip1
	local platform=$(find $SYSFS/devices/platform/ -name $chip -type d -maxdepth 2)
	[ "$platform" ] || fail "can't find platform of $chip"
	# e.g. /sys/devices/platform/gpio-mockup.1/gpio/gpiochip508/base
	local base=$(find $(dirname $platform)/gpio/ -name base -type f -maxdepth 2)
	[ "$base" ] || fail "can't find base of $chip"
	sysfs_nr=$(< $base)
	sysfs_nr=$(($sysfs_nr + $offset))

which works, though still doesn't handle the possibility of multiple
matches returned by the finds.

> > > > +       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.
> 
> When GPIO is a PCI device the above won't give a proper path.
> If we wish to give an example to somebody, it would be better to have
> it good enough.
> 

How would it appear for PCI bus?

> > > > +       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.
> 
> Yes and to me logically correct to distinguish acquire_line() with set_line().
> Then wherever you need to set_line(), you may call acquire_line()
> which should be idempotent (the same way as release_line() call).
> 

Oh, ok - it would only be called from set_line - I thought you meant
expose it as part of the uAPI test interface (currently
get_line/set_line/release_line).

> > > > +release_line()
> > > >  {
> > > > +       [ -z "$sysfs_nr" ] && return
> > > > +       echo $sysfs_nr > $GPIO_SYSFS/unexport
> > > > +       sysfs_nr=
> > > > +       sysfs_ldir=
> > > >  }
> 
> ...
> 
> > > > +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.
> 
> Exactly! And "" helps with that.
> 
> If I put parameters as `a b c "d e"`, your case will take them wrongly.
> 
> > > > +       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?
> 
> OK. Just replace it with a dropped useless test call.
> which ... || skip ...
> 

Yup - I've since replaced it with a test call to modprobe -h, so no
`which` required.

> ...
> 
> > > P.S. Also you may use `#!/bin/sh -efu` as shebang and fix other problems.
> >
> > A shebang or a `set -efu`?
> 
> Shebang. The difference is that with shebang you don't need to edit
> the script each time you want to change that.
> sh -x /path/to/the/script will give different results.
> 

OK, didn't consider that.
Have got the scripts running with the -efu flags set - that was entertaining.

> > I don't see shebang options used anywhere in the selftest scripts, but I
> > agree with a set.
> 
> Because shell scripts in the kernel are really badly written (so does
> Python ones).
> Again, even senior developers can't get shell right (including me).
> 
> > 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.
> 
> I'm using busybox on an everyday basis and mentioned shebang works
> there if I'm not mistaken.
> Because all flags are listed in the standard.
> https://pubs.opengroup.org/onlinepubs/007904875/utilities/sh.html
> 

I meant the actual /bin/bash, not the flags.
Though I now build bash in my buildroots, so I don't get that warning
anymore.

Cheers,
Kent.