Re: [PATCH v6 3/5] test: add new driver_data load tester

[Luis R. Rodriguez]

On Tue, Apr 11, 2017 at 05:32:52PM +0900, AKASHI Takahiro wrote:
> On Wed, Mar 29, 2017 at 08:25:12PM -0700, Luis R. Rodriguez wrote:
> > This adds a load tester driver test_driver_data a for the new extensible
> > driver_data loader API, part of firmware_class. This test driver enables
> > you to build your tests in userspace by exposing knobs of the exported
> > API to userspace and enables a trigger action to mimic a one time use
> > of the kernel API. This gives us the flexibility to build test case from
> > userspace with less kernel changes.
> > 
> > Signed-off-by: Luis R. Rodriguez <mcgrof@xxxxxxxxxx>
> > ---
> >  Documentation/driver-api/firmware/driver_data.rst |   32 +
> >  MAINTAINERS                                       |    1 +
> >  lib/Kconfig.debug                                 |   12 +
> >  lib/Makefile                                      |    1 +
> >  lib/test_driver_data.c                            | 1272 +++++++++++++++++++++
> >  tools/testing/selftests/firmware/Makefile         |    2 +-
> >  tools/testing/selftests/firmware/config           |    1 +
> >  tools/testing/selftests/firmware/driver_data.sh   |  996 ++++++++++++++++
> >  8 files changed, 2316 insertions(+), 1 deletion(-)
> >  create mode 100644 lib/test_driver_data.c
> >  create mode 100755 tools/testing/selftests/firmware/driver_data.sh
> > 
> > diff --git a/Documentation/driver-api/firmware/driver_data.rst b/Documentation/driver-api/firmware/driver_data.rst
> > index 08407b7568fe..757c2ffa4ba6 100644
> > --- a/Documentation/driver-api/firmware/driver_data.rst
> > +++ b/Documentation/driver-api/firmware/driver_data.rst
> > @@ -68,6 +68,38 @@ When driver_data_file_request_async() completes you can rest assured all the
> >  work for both triggering, and processing the driver data using any of your
> >  callbacks has completed.
> >  
> > +Testing the driver_data API
> > +===========================
> > +
> > +The driver data API has a selftest driver: lib/test_driver_data.c. The
> > +test_driver_data enables you to build your tests in userspace by exposing knobs
> > +of the exported API in userspace and enabling userspace to configure and
> > +trigger a kernel call. This lets us build most possible test cases of
> > +the kernel APIs from userspace.
> > +
> > +The test_driver_data also enables multiple test triggers to be created
> > +enabling testing to be done in parallel, one test interface per test case.
> > +
> > +To test an async call one could do::
> > +
> > +        echo anything > /lib/firmware/test-driver_data.bin
> 
> Your current shell script doesn't search for the firmware in
> /lib/firmware unless you explicitly specify $FWPATH.

This is true but that is the *test* shell script, and it purposely avoids the
existing firmware path to avoid overriding dummy test files on the production
path. So the above still stands as it is not using the test shell script
driver_data.sh.

I'll add a note:

"""
Note that driver_data.sh uses its own temporary custom path for creating and    
looking for driver data files, it does this to not overwrite any production     
files you might have which may share the same names used by the test shell      
script driver_data.sh. If you are not using the driver_data.sh script your      
default path will be used. 
"""

> > diff --git a/lib/test_driver_data.c b/lib/test_driver_data.c
> > new file mode 100644
> > index 000000000000..11175a3b9f0a
> > --- /dev/null
> > +++ b/lib/test_driver_data.c
> > @@ -0,0 +1,1272 @@
> > +/*
> > + * Driver data test interface
> > + *
> > + * Copyright (C) 2017 Luis R. Rodriguez <mcgrof@xxxxxxxxxx>
> > + *
> > + * This program is free software; you can redistribute it and/or modify it
> > + * under the terms of copyleft-next (version 0.3.1 or later) as published
> > + * at http://copyleft-next.org/.
> 
> Is this compatible with GPLv2 for kernel modules?

Yes, I went through all possible channels to vet for this, for details refer
to the thread which explains this [0] where the first attempt was to actually add
the license to the list of compatible licenses. So Linus' preference is to use
MODULE_LICENSE("GPL") rather.

[0] https://lkml.kernel.org/r/CA+55aFyhxcvD+q7tp+-yrSFDKfR0mOHgyEAe=f_94aKLsOu0Og@xxxxxxxxxxxxxx

> > diff --git a/tools/testing/selftests/firmware/driver_data.sh b/tools/testing/selftests/firmware/driver_data.sh

...

> > +TEST_NAME="driver_data"
> > +TEST_DRIVER="test_${TEST_NAME}"
> > +TEST_DIR=$(dirname $0)
> > +
> > +# This represents
> > +#
> > +# TEST_ID:TEST_COUNT:ENABLED
> > +#
> > +# TEST_ID: is the test id number
> > +# TEST_COUNT: number of times we should run the test
> > +# ENABLED: 1 if enabled, 0 otherwise
> > +#
> > +# Once these are enabled please leave them as-is. Write your own test,
> > +# we have tons of space.
> > +ALL_TESTS="0001:3:1"
> > +ALL_TESTS="$ALL_TESTS 0002:3:1"
> > +ALL_TESTS="$ALL_TESTS 0003:3:1"
> > +ALL_TESTS="$ALL_TESTS 0004:10:1"
> > +ALL_TESTS="$ALL_TESTS 0005:10:1"
> > +ALL_TESTS="$ALL_TESTS 0006:10:1"
> > +ALL_TESTS="$ALL_TESTS 0007:10:1"
> > +ALL_TESTS="$ALL_TESTS 0008:10:1"
> > +ALL_TESTS="$ALL_TESTS 0009:10:1"
> > +ALL_TESTS="$ALL_TESTS 0010:10:1"
> > +ALL_TESTS="$ALL_TESTS 0011:10:1"
> > +ALL_TESTS="$ALL_TESTS 0012:1:1"
> > +ALL_TESTS="$ALL_TESTS 0013:1:1"
> 
> Do you have good reasons for "the number of times" here?

Just that 1 was not enough and more than 10 seemed too much. As is the tests
are rather simple compared to what we can do given the flexibility in how we
can perform tests due to the test driver structure, in the future this will
become more important. But best to just get in the basics before we hammer and
expand on this a lot. There is also the question of sharing this sort of logic
with the upper testing layers so that they deal with this and not us
(tools/testing/selftests/), in that sense all this is just sufficient for us to do
our own testing for now, but we may and should consider how to get the upper
layers to deal this for us. But we can address this later.

> > +# Not yet sure how to automate suspend test well yet.  For now we expect a
> > +# manual run. If using qemu you can resume a guest using something like the
> > +# following on the monitor pts.
> > +# system_wakeupakeup | socat - /dev/pts/7,raw,echo=0,crnl
> > +#ALL_TESTS="$ALL_TESTS 0014:0:1"
> > +
> > +test_modprobe()
> > +{
> > +       if [ ! -d $DIR ]; then
> > +               echo "$0: $DIR not present" >&2
> > +               echo "You must have the following enabled in your kernel:" >&2
> > +               cat $TEST_DIR/config >&2
> > +               exit 1
> > +       fi
> > +}
> > +
> > +function allow_user_defaults()
> > +{
> > +	if [ -z $DEFAULT_NUM_TESTS ]; then
> > +		DEFAULT_NUM_TESTS=50
> > +	fi
> > +
> > +	if [ -z $FW_SYSFSPATH ]; then
> > +		FW_SYSFSPATH="/sys/module/firmware_class/parameters/path"
> > +	fi
> > +
> > +	if [ -z $OLD_FWPATH ]; then
> > +		OLD_FWPATH=$(cat $FW_SYSFSPATH)
> > +	fi
> > +
> > +	if [ -z $FWPATH]; then
> > +		FWPATH=$(mktemp -d)
> > +	fi
> > +
> > +	if [ -z $DEFAULT_DRIVER_DATA ]; then
> > +		config_reset
> > +		DEFAULT_DRIVER_DATA=$(config_get_name)
> > +	fi
> > +
> > +	if [ -z $FW ]; then
> > +		FW="$FWPATH/$DEFAULT_DRIVER_DATA"
> > +	fi
> > +
> > +	if [ -z $SYS_STATE_PATH ]; then
> > +		SYS_STATE_PATH="/sys/power/state"
> > +	fi
> > +
> > +	# Set the kernel search path.
> > +	echo -n "$FWPATH" > $FW_SYSFSPATH
> > +
> > +	# This is an unlikely real-world firmware content. :)
> > +	echo "ABCD0123" >"$FW"
> 
> Do you always want to overwrite the firmware even if user explicitly
> provides it?

This is a test script so it constructs its own temporary path so it can
have the confidence to overwrite anything it pleases. So in this case yes.
Its just as the old firmware test script.

> > +usage()
> > +{
> > +	NUM_TESTS=$(grep -o ' ' <<<"$ALL_TESTS" | grep -c .)
> > +	let NUM_TESTS=$NUM_TESTS+1
> > +	MAX_TEST=$(printf "%04d\n" $NUM_TESTS)
> > +	echo "Usage: $0 [ -t <4-number-digit> ] | [ -w <4-number-digit> ] |"
> > +	echo "		 [ -s <4-number-digit> ] | [ -c <4-number-digit> <test- count>"
> > +	echo "           [ all ] [ -h | --help ] [ -l ]"
> > +	echo ""
> > +	echo "Valid tests: 0001-$MAX_TEST"
> > +	echo ""
> > +	echo "    all     Runs all tests (default)"
> > +	echo "    -t      Run test ID the number amount of times is recommended"
> > +	echo "    -w      Watch test ID run until it runs into an error"
> > +	echo "    -c      Run test ID once"
> 
>                -> -s
> 
> > +	echo "    -s      Run test ID x test-count number of times"
> 
>                -> -c

Good thing you highlighted these, I had them flipped, -s was for single run
and -c was for test-count number of times.

> If you make the second parameter optional, you don't need
> -t nor -s:
>         driver_data.sh -c 0004     ; recommended times
>         driver_data.sh -c 0004 1   ; only once
>         driver_data.sh -c 0004 100 ; as many times as you want

True but I prefer having short-hand notations as well.

PS. In the future I'd highly appreciate if you can trim your responses
so you leave only in context enough information to just review the
criteria you are commenting on, rather than keeping every single line.

  Luis