| Message ID | 20210102022949.92304-1-warthog618@gmail.com |
|---|---|
| Headers | show |
| Series | selftests: gpio: rework and port to GPIO uAPI v2 | expand |
On Sat, Jan 02, 2021 at 03:52:32PM +0200, Andy Shevchenko wrote: > On Saturday, January 2, 2021, Kent Gibson <warthog618@gmail.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. > > > > > > I briefly looked at code in shell below... there are places to improve > (useless use of: cat, test, negation, etc). > My shell is clearly pretty poor, so I would really appreciate a pointer to an example of each, and I'll then hunt down the rest. Thanks, Kent.
On Sat, Jan 2, 2021 at 4:32 AM Kent Gibson <warthog618@gmail.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. ... > +#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
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@gmail.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.
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@gmail.com> 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@gmail.com> 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.
On Mon, Jan 04, 2021 at 03:52:49PM +0200, Andy Shevchenko wrote: > On Mon, Jan 4, 2021 at 3:51 AM Kent Gibson <warthog618@gmail.com> wrote: > > On Sun, Jan 03, 2021 at 05:10:10PM +0200, Andy Shevchenko wrote: > > ... > > > In this example it is the 508: > > > > # e.g. gpiochip0: GPIOs 508-511, parent: platform/gpio-mockup.0, gpio-mockup-A: > > > > So I'll use that - unless it is unreliable for some reason? > > debugfs is not an ABI and tomorrow this can be changed without notice. > I had a bad feeling that might be the case, and all my current solutions use debugfs one way or another, so back to the drawing board on that one. Thanks, Kent.
On Mon, Jan 04, 2021 at 11:00:31PM +0800, Kent Gibson wrote: > On Mon, Jan 04, 2021 at 03:52:49PM +0200, Andy Shevchenko wrote: > > On Mon, Jan 4, 2021 at 3:51 AM Kent Gibson <warthog618@gmail.com> wrote: > > > On Sun, Jan 03, 2021 at 05:10:10PM +0200, Andy Shevchenko wrote: > > > > ... > > > > > In this example it is the 508: > > > > > > # e.g. gpiochip0: GPIOs 508-511, parent: platform/gpio-mockup.0, gpio-mockup-A: > > > > > > So I'll use that - unless it is unreliable for some reason? > > > > debugfs is not an ABI and tomorrow this can be changed without notice. > > > > I had a bad feeling that might be the case, and all my current solutions > use debugfs one way or another, so back to the drawing board on that one. > Hang on - the find approach that I was looking at previously only uses /sys/devices/platform, so I'll revert to that one - and add handling for multi-match. Cheers, Kent.
On Sat, Jan 2, 2021 at 3:30 AM Kent Gibson <warthog618@gmail.com> wrote: > Initially I just wanted to port the selftests to the latest GPIO uAPI, > but on finding that, due to dependency issues, the selftests are not built > for the buildroot environments that I do most of my GPIO testing in, I > decided to take a closer look. All patches look good to me, I see Andy is helping you to hash out some shell script, anyway: Acked-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij