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

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

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.

> > 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?

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

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

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

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

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

...

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

> 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

> Thanks again for the review - always a learning experience.

-- 
With Best Regards,
Andy Shevchenko