Message-ID: <20170512002023.GG22134@linaro.org>
Date:   Fri, 12 May 2017 09:20:24 +0900
From:   AKASHI Takahiro <takahiro.akashi@...aro.org>
To:     "Luis R. Rodriguez" <mcgrof@...e.com>
Cc:     "Luis R. Rodriguez" <mcgrof@...nel.org>,
        gregkh@...uxfoundation.org, wagi@...om.org, dwmw2@...radead.org,
        rafal@...ecki.pl, arend.vanspriel@...adcom.com, rjw@...ysocki.net,
        yi1.li@...ux.intel.com, atull@...nsource.altera.com,
        moritz.fischer@...us.com, pmladek@...e.com,
        johannes.berg@...el.com, emmanuel.grumbach@...el.com,
        luciano.coelho@...el.com, kvalo@...eaurora.org, luto@...nel.org,
        dhowells@...hat.com, pjones@...hat.com,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v6 3/5] test: add new driver_data load tester

On Thu, May 11, 2017 at 08:26:29PM +0200, Luis R. Rodriguez wrote:
> On Thu, May 11, 2017 at 07:46:27PM +0900, AKASHI Takahiro wrote:
> > On Fri, Apr 28, 2017 at 03:45:35AM +0200, Luis R. Rodriguez wrote:
> > > > > 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
> > 
> > In my opinion, "1," or "2" given the nature of firmware caching, is good
> > enough, but it's up to you.
> 
> No, firmware caching deserves its own test unit on its own, but that will be
> enabled through a separate test once we move DRIVER_DATA_PRIV_REQ_NO_CACHE 
> to DRIVER_DATA_REQ_NO_CACHE (a private feature to a publicly available
> enabled feature). This will be enabled for the upcoming work which enables
> FGPA firmware upload which will first enable request_firmware_into_buf()
> through the driver data API which uses this.

I thought of implementing NO_CACHE option by myself, but came up with
no practical use cases other than my test case :)

> > BTW, firmware caching is a bit annoying in my signature tests
> > because caching will bypass the verification checks when we iterates
> > tests with different conditions.
> 
> It would seems to make sense to me to only need to verify files when read
> for the first time, once its cache I don't see why we would re-verify them ?

Let me explain my test scenario:
case (a): firmware w/o signature for signature-not-required driver
case (b): firmware w/o signature for signature-required driver
   ... and so on

Case (a) and (b) are exercised with the same firmware blob but with
different 'test_config's of test_driver_data driver.

First do case (a) and it succeeds, caching the blob, then do case (b).
It should fail, but actually succeeds due to firmware caching.

Yes, we can change the order, (b) then (a), to make tests run correctly.

But again, when we iterate this set of test cases under "-c" repeatedly,
this will happen again as you can imagine.

Thanks to firmware caching, each of iterations of tests is not executed
under the exactly same condition. That is the problem.

Let's think about more realistic case:
we want to run the system without stopping/rebooting it, but also
want to update some driver. OK, remove the driver module and insert
a new one which, in this case, enables signature verification option.
But, unlike we expect, loading a firmware will succeed anyway even
if we don't provide its signature as it's already in cache.

I hope we can fix it by modifying the logic of caching.

> > > 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.
> > 
> > Right, but looking into the script, even if an user supplies a firmware
> > blob, the script overwrites it unnecessarily.
> 
> FWPATH=$(mktemp -d)
> ...
> FW="$FWPATH/$DEFAULT_DRIVER_DATA"
> ...
> echo "ABCD0123" >"$FW"
> 
> So this is really just touching the custom path stuff, unless of course the
> caller overrides the above variables, in which case its trusted they know what
> they are doing, ie custom test cases / setup / system. That was the goal.

Even if an user gives a full-path name to $FW, do you want to overwrite it?

Thanks,
-Takahiro AKASHI

> > This may also be inconvenient if I add signature verification tests.
> > (Not sure though.)
> 
> You can feel free to modify this as you see fit to account for firmware
> signing. If you run into issues please feel free to make adjustments.
> 
> > > > > +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.
> > 
> > Okay, up to you.
> 
> In the end I hope we get rid of all this anyway and replace it with
> a centralized way for selftests but IMHO this is a secondary step.
> 
>   Luis

Powered by blists - more mailing lists