| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAHp75VdMs1mP7pK46qKqJbjfyrcKhSGvtyzQpTRsehMz6o=Jpg@mail.gmail.com>
Date: Sun, 3 Jan 2021 00:20:26 +0200
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Kent Gibson <warthog618@...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 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.
...
> +#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
Powered by blists - more mailing lists