On Sun, Jan 3, 2021 at 4:17 AM Kent Gibson [off-list ref] wrote:

On Sun, Jan 03, 2021 at 12:20:26AM +0200, Andy Shevchenko wrote:

quoted

On Sat, Jan 2, 2021 at 4:32 AM Kent Gibson [off-list ref] wrote:

...

quoted

quoted

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

...

quoted

quoted

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

quoted

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.

quoted

quoted

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

quoted

quoted

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

quoted

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.

quoted

quoted

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

quoted

quoted

+       sysfs_nr=$(($sysfs_nr + $offset))
+       sysfs_ldir=$GPIO_SYSFS/gpio$sysfs_nr
 }

...

quoted

quoted

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

quoted

quoted

+release_line()
 {
+       [ -z "$sysfs_nr" ] && return
+       echo $sysfs_nr > $GPIO_SYSFS/unexport
+       sysfs_nr=
+       sysfs_ldir=
 }

...

quoted

quoted

+skip()
 {

quoted

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

quoted

quoted

+       echo GPIO $module test SKIP
+       exit $ksft_skip
 }

...

quoted

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

...

quoted

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