Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1632486 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v6 3/5] test: add new driver_data load tester "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-04-28 03:50 +0200
    Re: [PATCH v6 3/5] test: add new driver_data load tester AKASHI Takahiro <takahiro.akashi@linaro.org> - 2017-05-11 12:50 +0200
      Re: [PATCH v6 3/5] test: add new driver_data load tester "Luis R. Rodriguez" <mcgrof@suse.com> - 2017-05-11 19:20 +0200
        Re: [PATCH v6 3/5] test: add new driver_data load tester "Li, Yi" <yi1.li@linux.intel.com> - 2017-05-18 00:50 +0200
          Re: [PATCH v6 3/5] test: add new driver_data load tester "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-05-19 20:40 +0200
      Re: [PATCH v6 3/5] test: add new driver_data load tester "Luis R. Rodriguez" <mcgrof@suse.com> - 2017-05-11 20:20 +0200
      Re: [PATCH v6 3/5] test: add new driver_data load tester "Luis R. Rodriguez" <mcgrof@suse.com> - 2017-05-11 20:30 +0200
        Re: [PATCH v6 3/5] test: add new driver_data load tester "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-05-11 20:50 +0200
          Re: [PATCH v6 3/5] test: add new driver_data load tester AKASHI Takahiro <takahiro.akashi@linaro.org> - 2017-05-12 02:30 +0200
            Re: [PATCH v6 3/5] test: add new driver_data load tester "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-05-12 18:00 +0200
              Re: [PATCH v6 3/5] test: add new driver_data load tester AKASHI Takahiro <takahiro.akashi@linaro.org> - 2017-05-17 11:10 +0200
                Re: [PATCH v6 3/5] test: add new driver_data load tester "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-05-17 17:40 +0200
        Re: [PATCH v6 3/5] test: add new driver_data load tester AKASHI Takahiro <takahiro.akashi@linaro.org> - 2017-05-12 02:20 +0200
          Re: [PATCH v6 3/5] test: add new driver_data load tester "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-05-12 18:00 +0200
            Re: [PATCH v6 3/5] test: add new driver_data load tester "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-05-13 20:50 +0200

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

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@kernel.org>
> > ---
> >  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@kernel.org>
> > + *
> > + * 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@mail.gmail.com

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

[toc] | [next] | [standalone]

#1639301

Luis,

On Fri, Apr 28, 2017 at 03:45:35AM +0200, Luis R. Rodriguez wrote:
> > > +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. 
> """

That looks fine, but I think we'd better change the line:

> > > +        echo anything > /lib/firmware/test-driver_data.bin

since it is just incorrect as far as driver_data.sh goes.

> > > 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@kernel.org>
> > > + *
> > > + * 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@mail.gmail.com

Thank you for this heads-up.
According to Linus' comment, he seems to expect an explicit GPL license
term to be in the beginning of the file, and then if you want, an additional
license to be added, quote "if you want to dual-license it, just put something
like "or, at your option, copyleft-next" in the comment at the top."


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

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.

> 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.
This may also be inconvenient if I add signature verification tests.
(Not sure though.)

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

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

Got it.

Thanks,
-Takahiro AKASHI

>   Luis

[toc] | [prev] | [next] | [standalone]

#1639909

On Thu, May 11, 2017 at 07:46:27PM +0900, AKASHI Takahiro wrote:
> Luis,
> 
> On Fri, Apr 28, 2017 at 03:45:35AM +0200, Luis R. Rodriguez wrote:
> > > > +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. 
> > """
> 
> That looks fine, but I think we'd better change the line:
> 
> > > > +        echo anything > /lib/firmware/test-driver_data.bin
> 
> since it is just incorrect as far as driver_data.sh goes.

But that is accurate, given the default file we search for on test_driver_data.c
is test-driver_data.bin. It also does not create a conflict to overwrite a file
used on driver_data.sh as driver_data.sh uses a custom path. I think the note
above on custom path is sufficient for the developer or user to be aware of
the fact the driver_data.sh does it own thing, and that the example is just a
manual test case.

What do you mean by that its incorrect ?

  Luis

[toc] | [prev] | [next] | [standalone]

#1643684

hi Luis


On 5/11/2017 12:11 PM, Luis R. Rodriguez wrote:
> On Thu, May 11, 2017 at 07:46:27PM +0900, AKASHI Takahiro wrote:
>> Luis,
>>
>> On Fri, Apr 28, 2017 at 03:45:35AM +0200, Luis R. Rodriguez wrote:
>>>>> +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.
>>> """
>> That looks fine, but I think we'd better change the line:
>>
>>>>> +        echo anything > /lib/firmware/test-driver_data.bin
>> since it is just incorrect as far as driver_data.sh goes.
> But that is accurate, given the default file we search for on test_driver_data.c
> is test-driver_data.bin. It also does not create a conflict to overwrite a file
> used on driver_data.sh as driver_data.sh uses a custom path. I think the note
> above on custom path is sufficient for the developer or user to be aware of
> the fact the driver_data.sh does it own thing, and that the example is just a
> manual test case.
> What do you mean by that its incorrect ?

I understand it now, but was on the same boat as Akashi. Renamed my 12MB 
test firmware binary to /lib/firmware/test-driver_data.bin, but 
driver_data.sh only read back 9 byes from the tmp "ABCD0123". :-)
What's the proper way to test a real image in the driver_data.sh script, 
change config_set_name?

Yi

>
>    Luis
>

[toc] | [prev] | [next] | [standalone]

#1645838

On Wed, May 17, 2017 at 05:45:22PM -0500, Li, Yi wrote:
> hi Luis
> 
> 
> On 5/11/2017 12:11 PM, Luis R. Rodriguez wrote:
> > On Thu, May 11, 2017 at 07:46:27PM +0900, AKASHI Takahiro wrote:
> > > Luis,
> > > 
> > > On Fri, Apr 28, 2017 at 03:45:35AM +0200, Luis R. Rodriguez wrote:
> > > > > > +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.
> > > > """
> > > That looks fine, but I think we'd better change the line:
> > > 
> > > > > > +        echo anything > /lib/firmware/test-driver_data.bin
> > > since it is just incorrect as far as driver_data.sh goes.
> > But that is accurate, given the default file we search for on test_driver_data.c
> > is test-driver_data.bin. It also does not create a conflict to overwrite a file
> > used on driver_data.sh as driver_data.sh uses a custom path. I think the note
> > above on custom path is sufficient for the developer or user to be aware of
> > the fact the driver_data.sh does it own thing, and that the example is just a
> > manual test case.
> > What do you mean by that its incorrect ?
> 
> I understand it now, but was on the same boat as Akashi. Renamed my 12MB
> test firmware binary to /lib/firmware/test-driver_data.bin, but
> driver_data.sh only read back 9 byes from the tmp "ABCD0123". :-)

Right, driver_data.sh is not to stress test a target file, it *builds*
tests and uses a temporary directory to avoid interfering with your own
files.

> What's the proper way to test a real image in the driver_data.sh script,
> change config_set_name?

Yes! You can review how it configures a test case to get an idea but the
name change is one, since you are doing it manually you would just avoid
resetting the firmware test path.

We could extend the script with an argument to leave the directory intact
and trust your own file given in an argument, but that would be a new test
case.

  Luis

[toc] | [prev] | [next] | [standalone]

#1639937

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/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@kernel.org>
> > > > + *
> > > > + * 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@mail.gmail.com
> 
> Thank you for this heads-up.
> According to Linus' comment, he seems to expect an explicit GPL license
> term to be in the beginning of the file, and then if you want, an additional
> license to be added, quote "if you want to dual-license it, just put something
> like "or, at your option, copyleft-next" in the comment at the top."

I think the or clause thing deserves some clarification so brought this up on the
old thread and Cc'd you.

  Luis

[toc] | [prev] | [next] | [standalone]

#1639941

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.

> 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 ?

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

> 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

[toc] | [prev] | [next] | [standalone]

#1639958

On Thu, May 11, 2017 at 11:26 AM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
>
> 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 ?

To be clear, the fw cache feature reads the files from the fs prior to
suspend, and then uses the in-memory cache on resume. So it would make
sense to me only to rely on fw verification on resume then when the fw
cache is used ?

 Luis

[toc] | [prev] | [next] | [standalone]

#1640086

On Thu, May 11, 2017 at 11:32:30AM -0700, Luis R. Rodriguez wrote:
> On Thu, May 11, 2017 at 11:26 AM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
> >
> > 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 ?
> 
> To be clear, the fw cache feature reads the files from the fs prior to
> suspend, and then uses the in-memory cache on resume. So it would make
> sense to me only to rely on fw verification on resume then when the fw
> cache is used ?

Good point. I was thinking of need for verification on resume.
As cache is not protected and visible to the kernel, some malware
might want to rewrite it :)

Thanks,
-Takahiro AKASHI

>  Luis

[toc] | [prev] | [next] | [standalone]

#1640551

On Fri, May 12, 2017 at 09:28:47AM +0900, AKASHI Takahiro wrote:
> On Thu, May 11, 2017 at 11:32:30AM -0700, Luis R. Rodriguez wrote:
> > On Thu, May 11, 2017 at 11:26 AM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
> > >
> > > 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 ?
> > 
> > To be clear, the fw cache feature reads the files from the fs prior to
> > suspend, and then uses the in-memory cache on resume. So it would make
> > sense to me only to rely on fw verification on resume then when the fw
> > cache is used ?
> 
> Good point. I was thinking of need for verification on resume.

From what we have discussed so far it would seem to me only necessary
for a sig_check_ok (if we accept a file can have only one signature
requirement) for a cache entry, and if its not set but a lookup needs
a sig check it can do a full fs lookup. If such a lookup succeeded
then it can fill the sig_check_ok in, provided the file contents
match of course, given the file could have changed under the hood
between the last file cache lookup (if the file did change that puts
us at odd with the first lookup, but since its an update and no sig
check is required, I guess it is fine to use its contents).

> As cache is not protected 

Cache should be protected, it should be const and if its not we should fix that.

> and visible to the kernel,

You mean it is visible to the kernel ?

>  some malware might want to rewrite it :)

Right, we want to be pedantic about that sort of stuff and signature
verification can help here but those benefits should carry their own
weight. We should do what we can without file signature verification to
protect the cache.

The cache is short lived though, it exists only during suspend/resume.

  Luis

[toc] | [prev] | [next] | [standalone]

#1643156

On Fri, May 12, 2017 at 05:59:23PM +0200, Luis R. Rodriguez wrote:
> On Fri, May 12, 2017 at 09:28:47AM +0900, AKASHI Takahiro wrote:
> > On Thu, May 11, 2017 at 11:32:30AM -0700, Luis R. Rodriguez wrote:
> > > On Thu, May 11, 2017 at 11:26 AM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
> > > >
> > > > 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 ?
> > > 
> > > To be clear, the fw cache feature reads the files from the fs prior to
> > > suspend, and then uses the in-memory cache on resume. So it would make
> > > sense to me only to rely on fw verification on resume then when the fw
> > > cache is used ?
> > 
> > Good point. I was thinking of need for verification on resume.
> 
> From what we have discussed so far it would seem to me only necessary
> for a sig_check_ok (if we accept a file can have only one signature
> requirement) for a cache entry, and if its not set but a lookup needs
> a sig check it can do a full fs lookup. If such a lookup succeeded
> then it can fill the sig_check_ok in, provided the file contents
> match of course, given the file could have changed under the hood
> between the last file cache lookup (if the file did change that puts
> us at odd with the first lookup, but since its an update and no sig
> check is required, I guess it is fine to use its contents).
> 
> > As cache is not protected 
> 
> Cache should be protected, it should be const and if its not we should fix that.

Yeah, but

> > and visible to the kernel,
> 
> You mean it is visible to the kernel ?

your current implementation doesn't provide any write protection.

> >  some malware might want to rewrite it :)
> 
> Right, we want to be pedantic about that sort of stuff and signature
> verification can help here but those benefits should carry their own
> weight. We should do what we can without file signature verification to
> protect the cache.
> 
> The cache is short lived though, it exists only during suspend/resume.

I found out why my test cases fail:
trigger_config_sync() in test_driver_data.c always enables REQ_KEEP flag
and so cached data (firmware_buf->data) has not been cleaned up.
I haven't fixed it in my test environment although I pointed it out before.

But the issue on write protection is still there wehn REQ_KEEP is used.

Thanks,
-Takahiro AKASHI

>   Luis

[toc] | [prev] | [next] | [standalone]

#1643474

On Wed, May 17, 2017 at 06:08:36PM +0900, AKASHI Takahiro wrote:
> On Fri, May 12, 2017 at 05:59:23PM +0200, Luis R. Rodriguez wrote:
> > On Fri, May 12, 2017 at 09:28:47AM +0900, AKASHI Takahiro wrote:
> > > On Thu, May 11, 2017 at 11:32:30AM -0700, Luis R. Rodriguez wrote:
> > > > On Thu, May 11, 2017 at 11:26 AM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
> > > > >
> > > > > 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 ?
> > > > 
> > > > To be clear, the fw cache feature reads the files from the fs prior to
> > > > suspend, and then uses the in-memory cache on resume. So it would make
> > > > sense to me only to rely on fw verification on resume then when the fw
> > > > cache is used ?
> > > 
> > > Good point. I was thinking of need for verification on resume.
> > 
> > From what we have discussed so far it would seem to me only necessary
> > for a sig_check_ok (if we accept a file can have only one signature
> > requirement) for a cache entry, and if its not set but a lookup needs
> > a sig check it can do a full fs lookup. If such a lookup succeeded
> > then it can fill the sig_check_ok in, provided the file contents
> > match of course, given the file could have changed under the hood
> > between the last file cache lookup (if the file did change that puts
> > us at odd with the first lookup, but since its an update and no sig
> > check is required, I guess it is fine to use its contents).
> > 
> > > As cache is not protected 
> > 
> > Cache should be protected, it should be const and if its not we should fix that.
> 
> Yeah, but
> 
> > > and visible to the kernel,
> > 
> > You mean it is visible to the kernel ?
> 
> your current implementation doesn't provide any write protection.

The cache was implemented long ago by someone other than myself. Patches
are welcomed.

> > >  some malware might want to rewrite it :)
> > 
> > Right, we want to be pedantic about that sort of stuff and signature
> > verification can help here but those benefits should carry their own
> > weight. We should do what we can without file signature verification to
> > protect the cache.
> > 
> > The cache is short lived though, it exists only during suspend/resume.
> 
> I found out why my test cases fail:
> trigger_config_sync() in test_driver_data.c always enables REQ_KEEP flag
> and so cached data (firmware_buf->data) has not been cleaned up.
> I haven't fixed it in my test environment although I pointed it out before.

Ah, perhaps the later revision of the test driver I posted captured this fix?

> But the issue on write protection is still there wehn REQ_KEEP is used.

Cache of firmware is a feature internal to the firmware_class, so it can only
be fixed there. Patches welcomed.

  Luis

[toc] | [prev] | [next] | [standalone]

#1640085

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

[toc] | [prev] | [next] | [standalone]

#1640557

On Fri, May 12, 2017 at 09:20:24AM +0900, AKASHI Takahiro wrote:
> 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
> > > > ...
> > > > > > +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 :)

FWIW, my understanding is that such work is underway. I think it may be
ready first than your signature work so it may make sense to coordinate
with that developer on their work as you might be able to make use of their
tests, etc.

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

I see. OK we should consider if firmware requirements can ever vary for
a file. If not then a cached firmware file can have a sig_check which
any request can fill in. Upon request firmware, if the cache is present
*but* the sig check requirements differ (in this simple case where
signature requirements cannot vary) then the cache present will not
suffice so we should allow a full new file check through to allow
to fill the sig_check. If signatures requirements can vary then we
could have a linked list of signature requirements and at cache check
time we'd have to check if they match the requirements.

Just one way to try to address this I think. But hey this is all firmware
signing related. I suppose a more relevant question to this thread is if
there might be current criteria or criteria being introduced in this patch
series which is similar to the signature check which should also be
extended in these sorts of caching checks.

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

Makes sense.

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

Sure, specially since asynchornous lookups won't be serialized we can't
expect any proper order for which request goes in first. If the extra
caching requirements were however taken into consideration I would expect
this to not matter though.

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

Its a good point. If caching had the requirements checks (signature would be
one) then this would fail at the cache lookup and continue to chug on.

For files which are kept the same name, say db.txt the example you provide
also runs into the situation where one file might be signed but through time
can be expected to be updated, as such the signature will change as will the
contents of the file. In such cases the firmware cache would get in the way
of file updates, but let us be clear that the firmware cache is not a cache
of all firmware, its only used prior to suspend and cleared after resume.
As such it seems valid for that time period. Likewise if the cache is known
to not be a good idea for a file they can in the future use the no-cache
requirement criteria, but then they'd have to implement something to deal
with the suspend/resume race.

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

Sure.

> > > > 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?

User cannot give a full path to a file, its one of the promises of the
firmware_class.c today, it uses its own prefix set of directories, it
could be security issue otherwise. In the future this may change and the
scope of what prefixes can be used might change but we're far from there.

If you meant if user gives a full-path to $FW by testing -- well this is
all part of the driver-data.sh and driver-data.sh uses its own custom
temporary directory for stashing tests, so what it does is specific to
it and compartamentalized there. If a users does not use driver-data.sh
and mucks with the knobs on their own, its expected they know what they
are doing, and by default the paths for lookups will be default so its
up to them to make sure to pick a proper file and not overwrite things.

I think its safe for test_driver.c to use the default path and assume
the test user will not go try to overwrite /lib/firwmare/iwlwifi-7260-17.ucode
for example.

If this is your concern we could devise the temporary directory logic in
the script into the test driver but I really would prefer to avoid that.

  Luis

[toc] | [prev] | [next] | [standalone]

#1640976

On Fri, May 12, 2017 at 05:52:18PM +0200, Luis R. Rodriguez wrote:
> On Fri, May 12, 2017 at 09:20:24AM +0900, AKASHI Takahiro wrote:
> > 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
> > > > > ...
> > > > > > > +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 :)
> 
> FWIW, my understanding is that such work is underway. I think it may be
> ready first than your signature work so it may make sense to coordinate
> with that developer on their work as you might be able to make use of their
> tests, etc.
> 
> > > > 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.
> 
> I see. OK we should consider if firmware requirements can ever vary for
> a file. If not then a cached firmware file can have a sig_check which
> any request can fill in. Upon request firmware, if the cache is present
> *but* the sig check requirements differ (in this simple case where
> signature requirements cannot vary) then the cache present will not
> suffice so we should allow a full new file check through to allow
> to fill the sig_check. If signatures requirements can vary then we
> could have a linked list of signature requirements and at cache check
> time we'd have to check if they match the requirements.
> 
> Just one way to try to address this I think. But hey this is all firmware
> signing related. I suppose a more relevant question to this thread is if
> there might be current criteria or criteria being introduced in this patch
> series which is similar to the signature check which should also be
> extended in these sorts of caching checks.
> 
> > But again, when we iterate this set of test cases under "-c" repeatedly,
> > this will happen again as you can imagine.
> 
> Makes sense.
> 
> > Thanks to firmware caching, each of iterations of tests is not executed
> > under the exactly same condition. That is the problem.
> 
> Sure, specially since asynchornous lookups won't be serialized we can't
> expect any proper order for which request goes in first. If the extra
> caching requirements were however taken into consideration I would expect
> this to not matter though.

Note that a bug was just opened on multiple fw requests and only one completes
fine, it sounds like a real bug, and if so sounds related to this topical case
you are testing against as well.

https://bugzilla.kernel.org/show_bug.cgi?id=195477

It has a proposed patch I have not yet had a chance to properly review / test:

https://bugzilla.kernel.org/attachment.cgi?id=256493&action=diff&collapsed=&headers=1&format=raw

  Luis

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web