* [PATCH 0/2] selftests: firmware: fw_filesystem fix for busybox @ 2018-11-27 3:12 Dan Rue 2018-11-27 3:12 ` [PATCH 1/2] selftests: firmware: remove use of non-standard diff -Z option Dan Rue 2018-11-27 3:12 ` [PATCH 2/2] selftests: firmware: add CONFIG_FW_LOADER_USER_HELPER_FALLBACK to config Dan Rue 0 siblings, 2 replies; 13+ messages in thread From: Dan Rue @ 2018-11-27 3:12 UTC (permalink / raw) To: linux-kernel Cc: Dan Rue, Shuah Khan, Greg Kroah-Hartman, Kees Cook, Luis R. Rodriguez, linux-kselftest Fix fw_filesystem.sh to run in an automated environment under busybox. After this change, fw_run_tests.sh still fails at some point in fw_fallback.sh, with error "usermode helper disabled so ignoring test". This is coming from fw_lib.sh:verify_reqs() because $HAS_FW_LOADER_USER_HELPER is set to no. Dan Rue (2): selftests: firmware: remove use of non-standard diff -Z option selftests: firmware: add CONFIG_FW_LOADER_USER_HELPER_FALLBACK to config tools/testing/selftests/firmware/config | 1 + tools/testing/selftests/firmware/fw_filesystem.sh | 9 +++------ 2 files changed, 4 insertions(+), 6 deletions(-) -- 2.19.1 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/2] selftests: firmware: remove use of non-standard diff -Z option 2018-11-27 3:12 [PATCH 0/2] selftests: firmware: fw_filesystem fix for busybox Dan Rue @ 2018-11-27 3:12 ` Dan Rue 2018-11-27 17:56 ` Kees Cook 2018-11-30 2:37 ` Luis Chamberlain 2018-11-27 3:12 ` [PATCH 2/2] selftests: firmware: add CONFIG_FW_LOADER_USER_HELPER_FALLBACK to config Dan Rue 1 sibling, 2 replies; 13+ messages in thread From: Dan Rue @ 2018-11-27 3:12 UTC (permalink / raw) To: linux-kernel Cc: Dan Rue, Shuah Khan, Greg Kroah-Hartman, Kees Cook, Luis R. Rodriguez, linux-kselftest diff -Z is used to trim the trailing whitespace when comparing the loaded firmware file with the source firmware file. However, per the comment in the source code, -Z should not be necessary. In testing, the input and output files are identical. Additionally, -Z is not a standard option and is not available in environments such as busybox. When -Z is not supported, diff fails with a usage error, which is suppressed, but then causes read_firmwares() to exit with a false failure message. Signed-off-by: Dan Rue <dan.rue@linaro.org> --- tools/testing/selftests/firmware/fw_filesystem.sh | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/tools/testing/selftests/firmware/fw_filesystem.sh b/tools/testing/selftests/firmware/fw_filesystem.sh index a4320c4b44dc..466cf2f91ba0 100755 --- a/tools/testing/selftests/firmware/fw_filesystem.sh +++ b/tools/testing/selftests/firmware/fw_filesystem.sh @@ -155,11 +155,8 @@ read_firmwares() { for i in $(seq 0 3); do config_set_read_fw_idx $i - # Verify the contents are what we expect. - # -Z required for now -- check for yourself, md5sum - # on $FW and DIR/read_firmware will yield the same. Even - # cmp agrees, so something is off. - if ! diff -q -Z "$FW" $DIR/read_firmware 2>/dev/null ; then + # Verify the contents match + if ! diff -q "$FW" $DIR/read_firmware 2>/dev/null ; then echo "request #$i: firmware was not loaded" >&2 exit 1 fi @@ -171,7 +168,7 @@ read_firmwares_expect_nofile() for i in $(seq 0 3); do config_set_read_fw_idx $i # Ensures contents differ - if diff -q -Z "$FW" $DIR/read_firmware 2>/dev/null ; then + if diff -q "$FW" $DIR/read_firmware 2>/dev/null ; then echo "request $i: file was not expected to match" >&2 exit 1 fi -- 2.19.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] selftests: firmware: remove use of non-standard diff -Z option 2018-11-27 3:12 ` [PATCH 1/2] selftests: firmware: remove use of non-standard diff -Z option Dan Rue @ 2018-11-27 17:56 ` Kees Cook 2018-11-30 2:37 ` Luis Chamberlain 1 sibling, 0 replies; 13+ messages in thread From: Kees Cook @ 2018-11-27 17:56 UTC (permalink / raw) To: Dan Rue Cc: LKML, Shuah Khan, Greg Kroah-Hartman, Luis R. Rodriguez, open list:KERNEL SELFTEST FRAMEWORK On Mon, Nov 26, 2018 at 7:12 PM, Dan Rue <dan.rue@linaro.org> wrote: > diff -Z is used to trim the trailing whitespace when comparing the > loaded firmware file with the source firmware file. However, per the > comment in the source code, -Z should not be necessary. In testing, the > input and output files are identical. > > Additionally, -Z is not a standard option and is not available in > environments such as busybox. When -Z is not supported, diff fails with > a usage error, which is suppressed, but then causes read_firmwares() to > exit with a false failure message. > > Signed-off-by: Dan Rue <dan.rue@linaro.org> Acked-by: Kees Cook <keescook@chromium.org> -Kees > --- > tools/testing/selftests/firmware/fw_filesystem.sh | 9 +++------ > 1 file changed, 3 insertions(+), 6 deletions(-) > > diff --git a/tools/testing/selftests/firmware/fw_filesystem.sh b/tools/testing/selftests/firmware/fw_filesystem.sh > index a4320c4b44dc..466cf2f91ba0 100755 > --- a/tools/testing/selftests/firmware/fw_filesystem.sh > +++ b/tools/testing/selftests/firmware/fw_filesystem.sh > @@ -155,11 +155,8 @@ read_firmwares() > { > for i in $(seq 0 3); do > config_set_read_fw_idx $i > - # Verify the contents are what we expect. > - # -Z required for now -- check for yourself, md5sum > - # on $FW and DIR/read_firmware will yield the same. Even > - # cmp agrees, so something is off. > - if ! diff -q -Z "$FW" $DIR/read_firmware 2>/dev/null ; then > + # Verify the contents match > + if ! diff -q "$FW" $DIR/read_firmware 2>/dev/null ; then > echo "request #$i: firmware was not loaded" >&2 > exit 1 > fi > @@ -171,7 +168,7 @@ read_firmwares_expect_nofile() > for i in $(seq 0 3); do > config_set_read_fw_idx $i > # Ensures contents differ > - if diff -q -Z "$FW" $DIR/read_firmware 2>/dev/null ; then > + if diff -q "$FW" $DIR/read_firmware 2>/dev/null ; then > echo "request $i: file was not expected to match" >&2 > exit 1 > fi > -- > 2.19.1 > -- Kees Cook ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] selftests: firmware: remove use of non-standard diff -Z option 2018-11-27 3:12 ` [PATCH 1/2] selftests: firmware: remove use of non-standard diff -Z option Dan Rue 2018-11-27 17:56 ` Kees Cook @ 2018-11-30 2:37 ` Luis Chamberlain 2018-12-05 20:43 ` Dan Rue 1 sibling, 1 reply; 13+ messages in thread From: Luis Chamberlain @ 2018-11-30 2:37 UTC (permalink / raw) To: Dan Rue Cc: linux-kernel, Shuah Khan, Greg Kroah-Hartman, Kees Cook, linux-kselftest On Mon, Nov 26, 2018 at 09:12:15PM -0600, Dan Rue wrote: > diff -Z is used to trim the trailing whitespace when comparing the > loaded firmware file with the source firmware file. However, per the > comment in the source code, -Z should not be necessary. In testing, the > input and output files are identical. > > Additionally, -Z is not a standard option and is not available in > environments such as busybox. When -Z is not supported, diff fails with > a usage error, which is suppressed, but then causes read_firmwares() to > exit with a false failure message. NACK -- this breaks testing on debian: Testing with the file present... Batched request_firmware() try #1: Files /tmp/tmp.8GWkoSo5jZ/test-firmware.bin and /sys/devices/virtual/misc/test_firmware/read_firmware differ request #0: firmware was not loaded Please add a quirks check, enable it by default, and remove it for Busybox. Luis ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] selftests: firmware: remove use of non-standard diff -Z option 2018-11-30 2:37 ` Luis Chamberlain @ 2018-12-05 20:43 ` Dan Rue 2019-02-07 18:20 ` Luis Chamberlain 0 siblings, 1 reply; 13+ messages in thread From: Dan Rue @ 2018-12-05 20:43 UTC (permalink / raw) To: Luis Chamberlain Cc: linux-kernel, Shuah Khan, Greg Kroah-Hartman, Kees Cook, linux-kselftest On Thu, Nov 29, 2018 at 06:37:32PM -0800, Luis Chamberlain wrote: > On Mon, Nov 26, 2018 at 09:12:15PM -0600, Dan Rue wrote: > > diff -Z is used to trim the trailing whitespace when comparing the > > loaded firmware file with the source firmware file. However, per the > > comment in the source code, -Z should not be necessary. In testing, the > > input and output files are identical. > > > > Additionally, -Z is not a standard option and is not available in > > environments such as busybox. When -Z is not supported, diff fails with > > a usage error, which is suppressed, but then causes read_firmwares() to > > exit with a false failure message. > > NACK -- this breaks testing on debian: > > Testing with the file present... > Batched request_firmware() try #1: Files > /tmp/tmp.8GWkoSo5jZ/test-firmware.bin and > /sys/devices/virtual/misc/test_firmware/read_firmware differ > request #0: firmware was not loaded > > Please add a quirks check, enable it by default, and remove it for > Busybox. Thanks for the review. Shuah, can you please drop this one? Dan -- Linaro - Kernel Validation ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] selftests: firmware: remove use of non-standard diff -Z option 2018-12-05 20:43 ` Dan Rue @ 2019-02-07 18:20 ` Luis Chamberlain 2019-02-08 17:53 ` shuah 0 siblings, 1 reply; 13+ messages in thread From: Luis Chamberlain @ 2019-02-07 18:20 UTC (permalink / raw) To: Dan Rue Cc: linux-kernel, Shuah Khan, Greg Kroah-Hartman, Kees Cook, linux-kselftest On Wed, Dec 5, 2018 at 2:43 PM Dan Rue <dan.rue@linaro.org> wrote: > > On Thu, Nov 29, 2018 at 06:37:32PM -0800, Luis Chamberlain wrote: > > On Mon, Nov 26, 2018 at 09:12:15PM -0600, Dan Rue wrote: > > > diff -Z is used to trim the trailing whitespace when comparing the > > > loaded firmware file with the source firmware file. However, per the > > > comment in the source code, -Z should not be necessary. In testing, the > > > input and output files are identical. > > > > > > Additionally, -Z is not a standard option and is not available in > > > environments such as busybox. When -Z is not supported, diff fails with > > > a usage error, which is suppressed, but then causes read_firmwares() to > > > exit with a false failure message. > > > > NACK -- this breaks testing on debian: > > > > Testing with the file present... > > Batched request_firmware() try #1: Files > > /tmp/tmp.8GWkoSo5jZ/test-firmware.bin and > > /sys/devices/virtual/misc/test_firmware/read_firmware differ > > request #0: firmware was not loaded > > > > Please add a quirks check, enable it by default, and remove it for > > Busybox. > > Thanks for the review. Shuah, can you please drop this one? So much for review. This patch was still merged. I'll have to request this to be reverted now too. Luis ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] selftests: firmware: remove use of non-standard diff -Z option 2019-02-07 18:20 ` Luis Chamberlain @ 2019-02-08 17:53 ` shuah 0 siblings, 0 replies; 13+ messages in thread From: shuah @ 2019-02-08 17:53 UTC (permalink / raw) To: Luis Chamberlain, Dan Rue Cc: linux-kernel, Greg Kroah-Hartman, Kees Cook, linux-kselftest, shuah On 2/7/19 11:20 AM, Luis Chamberlain wrote: > On Wed, Dec 5, 2018 at 2:43 PM Dan Rue <dan.rue@linaro.org> wrote: >> >> On Thu, Nov 29, 2018 at 06:37:32PM -0800, Luis Chamberlain wrote: >>> On Mon, Nov 26, 2018 at 09:12:15PM -0600, Dan Rue wrote: >>>> diff -Z is used to trim the trailing whitespace when comparing the >>>> loaded firmware file with the source firmware file. However, per the >>>> comment in the source code, -Z should not be necessary. In testing, the >>>> input and output files are identical. >>>> >>>> Additionally, -Z is not a standard option and is not available in >>>> environments such as busybox. When -Z is not supported, diff fails with >>>> a usage error, which is suppressed, but then causes read_firmwares() to >>>> exit with a false failure message. >>> >>> NACK -- this breaks testing on debian: >>> >>> Testing with the file present... >>> Batched request_firmware() try #1: Files >>> /tmp/tmp.8GWkoSo5jZ/test-firmware.bin and >>> /sys/devices/virtual/misc/test_firmware/read_firmware differ >>> request #0: firmware was not loaded >>> >>> Please add a quirks check, enable it by default, and remove it for >>> Busybox. >> >> Thanks for the review. Shuah, can you please drop this one? > > So much for review. This patch was still merged. I'll have to request > this to be reverted now too. > > Luis > Sorry about this. I will pull the reverts in. thanks, -- Shuah ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/2] selftests: firmware: add CONFIG_FW_LOADER_USER_HELPER_FALLBACK to config 2018-11-27 3:12 [PATCH 0/2] selftests: firmware: fw_filesystem fix for busybox Dan Rue 2018-11-27 3:12 ` [PATCH 1/2] selftests: firmware: remove use of non-standard diff -Z option Dan Rue @ 2018-11-27 3:12 ` Dan Rue 2018-11-27 17:56 ` Kees Cook 2018-11-30 2:31 ` Luis Chamberlain 1 sibling, 2 replies; 13+ messages in thread From: Dan Rue @ 2018-11-27 3:12 UTC (permalink / raw) To: linux-kernel Cc: Dan Rue, Shuah Khan, Luis R. Rodriguez, Greg Kroah-Hartman, Kees Cook, linux-kselftest CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y is required for fw_fallback.sh. Without it, fw_fallback.sh fails with 'usermode helper disabled so ignoring test'. Enable the config in selftest so that it gets built by default. Signed-off-by: Dan Rue <dan.rue@linaro.org> --- tools/testing/selftests/firmware/config | 1 + 1 file changed, 1 insertion(+) diff --git a/tools/testing/selftests/firmware/config b/tools/testing/selftests/firmware/config index bf634dda0720..913a25a4a32b 100644 --- a/tools/testing/selftests/firmware/config +++ b/tools/testing/selftests/firmware/config @@ -1,5 +1,6 @@ CONFIG_TEST_FIRMWARE=y CONFIG_FW_LOADER=y CONFIG_FW_LOADER_USER_HELPER=y +CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y CONFIG_IKCONFIG=y CONFIG_IKCONFIG_PROC=y -- 2.19.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] selftests: firmware: add CONFIG_FW_LOADER_USER_HELPER_FALLBACK to config 2018-11-27 3:12 ` [PATCH 2/2] selftests: firmware: add CONFIG_FW_LOADER_USER_HELPER_FALLBACK to config Dan Rue @ 2018-11-27 17:56 ` Kees Cook 2018-11-30 2:31 ` Luis Chamberlain 1 sibling, 0 replies; 13+ messages in thread From: Kees Cook @ 2018-11-27 17:56 UTC (permalink / raw) To: Dan Rue Cc: LKML, Shuah Khan, Luis R. Rodriguez, Greg Kroah-Hartman, open list:KERNEL SELFTEST FRAMEWORK On Mon, Nov 26, 2018 at 7:12 PM, Dan Rue <dan.rue@linaro.org> wrote: > CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y is required for fw_fallback.sh. > Without it, fw_fallback.sh fails with 'usermode helper disabled so > ignoring test'. Enable the config in selftest so that it gets built by > default. > > Signed-off-by: Dan Rue <dan.rue@linaro.org> Acked-by: Kees Cook <keescook@chromium.org> -Kees > --- > tools/testing/selftests/firmware/config | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/tools/testing/selftests/firmware/config b/tools/testing/selftests/firmware/config > index bf634dda0720..913a25a4a32b 100644 > --- a/tools/testing/selftests/firmware/config > +++ b/tools/testing/selftests/firmware/config > @@ -1,5 +1,6 @@ > CONFIG_TEST_FIRMWARE=y > CONFIG_FW_LOADER=y > CONFIG_FW_LOADER_USER_HELPER=y > +CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y > CONFIG_IKCONFIG=y > CONFIG_IKCONFIG_PROC=y > -- > 2.19.1 > -- Kees Cook ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] selftests: firmware: add CONFIG_FW_LOADER_USER_HELPER_FALLBACK to config 2018-11-27 3:12 ` [PATCH 2/2] selftests: firmware: add CONFIG_FW_LOADER_USER_HELPER_FALLBACK to config Dan Rue 2018-11-27 17:56 ` Kees Cook @ 2018-11-30 2:31 ` Luis Chamberlain 2019-02-04 23:39 ` Luis Chamberlain 1 sibling, 1 reply; 13+ messages in thread From: Luis Chamberlain @ 2018-11-30 2:31 UTC (permalink / raw) To: Dan Rue, Shuah Khan Cc: linux-kernel, Greg Kroah-Hartman, Kees Cook, linux-kselftest, Brendan Higgins On Mon, Nov 26, 2018 at 09:12:16PM -0600, Dan Rue wrote: > CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y is required for fw_fallback.sh. > Without it, fw_fallback.sh fails with 'usermode helper disabled so > ignoring test'. Enable the config in selftest so that it gets built by > default. > > Signed-off-by: Dan Rue <dan.rue@linaro.org> > --- > tools/testing/selftests/firmware/config | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/tools/testing/selftests/firmware/config b/tools/testing/selftests/firmware/config > index bf634dda0720..913a25a4a32b 100644 > --- a/tools/testing/selftests/firmware/config > +++ b/tools/testing/selftests/firmware/config > @@ -1,5 +1,6 @@ > CONFIG_TEST_FIRMWARE=y > CONFIG_FW_LOADER=y > CONFIG_FW_LOADER_USER_HELPER=y > +CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y > CONFIG_IKCONFIG=y > CONFIG_IKCONFIG_PROC=y NACK -- the point of the changes was to *allow* us to mimic such configuration through a proc sysctl knob. You aren forcing CONFIG_FW_LOADER_USER_HELPER_FALLBACK but just having CONFIG_FW_LOADER_USER_HELPER suffices to emulate the_FALLBACK functionality. The issue here seems to be that *all* tests fail once a configuration is found which is not suitable a tests. With the shiny new proc sysctls we can test all 3 kernel configurations in one shot. Since we test 3 different kernel configurations naturally some of these won't have the features needed, so that failure should be treated as non-fatal to allow the chain of other tests to continue. This issue was a regression due to commit a6a9be9270c87 ("selftests: firmware: return Kselftest Skip code for skipped tests") by Shuah for the verify_reqs(). We need to treat this as a non-fatal / don't skip return value. The following would fix this chaining issue: diff --git a/tools/testing/selftests/firmware/fw_lib.sh b/tools/testing/selftests/firmware/fw_lib.sh index 6c5f1b2ffb74..1cbb12e284a6 100755 --- a/tools/testing/selftests/firmware/fw_lib.sh +++ b/tools/testing/selftests/firmware/fw_lib.sh @@ -91,7 +91,7 @@ verify_reqs() if [ "$TEST_REQS_FW_SYSFS_FALLBACK" = "yes" ]; then if [ ! "$HAS_FW_LOADER_USER_HELPER" = "yes" ]; then echo "usermode helper disabled so ignoring test" - exit $ksft_skip + exit 0 fi fi } However its not clear to me if instead we want some new special return value for selftests so that the framework can detect an that an error is non-fatal, and can continue. This is a tricky situation given the script, existing upstream kernel module, are aware of such emulation hacks via sysctl, but knowledge of this is not obvious to selftests. Shuah, how do you suggest we handle this corner case? If you are OK with the above hunk for now I can send a fix for it. In either case this commit was added on v4.18, so the fix would be a stable fix. Luis ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] selftests: firmware: add CONFIG_FW_LOADER_USER_HELPER_FALLBACK to config 2018-11-30 2:31 ` Luis Chamberlain @ 2019-02-04 23:39 ` Luis Chamberlain 2019-02-05 2:41 ` Dan Rue 0 siblings, 1 reply; 13+ messages in thread From: Luis Chamberlain @ 2019-02-04 23:39 UTC (permalink / raw) To: Dan Rue, Shuah Khan, Masahiro Yamada Cc: linux-kernel, Greg Kroah-Hartman, Kees Cook, linux-kselftest, Brendan Higgins On Thu, Nov 29, 2018 at 8:31 PM Luis Chamberlain <mcgrof@kernel.org> wrote: > > On Mon, Nov 26, 2018 at 09:12:16PM -0600, Dan Rue wrote: > > CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y is required for fw_fallback.sh. > > Without it, fw_fallback.sh fails with 'usermode helper disabled so > > ignoring test'. Enable the config in selftest so that it gets built by > > default. > > > > Signed-off-by: Dan Rue <dan.rue@linaro.org> > > --- > > tools/testing/selftests/firmware/config | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/tools/testing/selftests/firmware/config b/tools/testing/selftests/firmware/config > > index bf634dda0720..913a25a4a32b 100644 > > --- a/tools/testing/selftests/firmware/config > > +++ b/tools/testing/selftests/firmware/config > > @@ -1,5 +1,6 @@ > > CONFIG_TEST_FIRMWARE=y > > CONFIG_FW_LOADER=y > > CONFIG_FW_LOADER_USER_HELPER=y > > +CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y > > CONFIG_IKCONFIG=y > > CONFIG_IKCONFIG_PROC=y > > NACK -- the point of the changes was to *allow* us to mimic such > configuration through a proc sysctl knob. > > You aren forcing CONFIG_FW_LOADER_USER_HELPER_FALLBACK but just having > CONFIG_FW_LOADER_USER_HELPER suffices to emulate the_FALLBACK > functionality. Dan, again, you broke the whole point to the amount of work that went into emulating testing. As such anyone testing their changes would yield incorrect results. > The issue here seems to be that *all* tests fail once a configuration is > found which is not suitable a tests. With the shiny new proc sysctls we > can test all 3 kernel configurations in one shot. Since we test 3 > different kernel configurations naturally some of these won't have the > features needed, so that failure should be treated as non-fatal to allow > the chain of other tests to continue. > > This issue was a regression due to commit a6a9be9270c87 ("selftests: > firmware: return Kselftest Skip code for skipped tests") by Shuah for > the verify_reqs(). We need to treat this as a non-fatal / don't skip > return value. > > The following would fix this chaining issue: > > diff --git a/tools/testing/selftests/firmware/fw_lib.sh b/tools/testing/selftests/firmware/fw_lib.sh > index 6c5f1b2ffb74..1cbb12e284a6 100755 > --- a/tools/testing/selftests/firmware/fw_lib.sh > +++ b/tools/testing/selftests/firmware/fw_lib.sh > @@ -91,7 +91,7 @@ verify_reqs() > if [ "$TEST_REQS_FW_SYSFS_FALLBACK" = "yes" ]; then > if [ ! "$HAS_FW_LOADER_USER_HELPER" = "yes" ]; then > echo "usermode helper disabled so ignoring test" > - exit $ksft_skip > + exit 0 > fi > fi > } > > However its not clear to me if instead we want some new special return > value for selftests so that the framework can detect an that an error > is non-fatal, and can continue. This is a tricky situation given the > script, existing upstream kernel module, are aware of such emulation > hacks via sysctl, but knowledge of this is not obvious to selftests. > > Shuah, how do you suggest we handle this corner case? If you are OK > with the above hunk for now I can send a fix for it. In either case > this commit was added on v4.18, so the fix would be a stable fix. In lieu of any suggestion I'm going to request we revert this commit and send the above fix. Luis ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] selftests: firmware: add CONFIG_FW_LOADER_USER_HELPER_FALLBACK to config 2019-02-04 23:39 ` Luis Chamberlain @ 2019-02-05 2:41 ` Dan Rue 2019-02-05 19:14 ` Luis Chamberlain 0 siblings, 1 reply; 13+ messages in thread From: Dan Rue @ 2019-02-05 2:41 UTC (permalink / raw) To: Luis Chamberlain Cc: Shuah Khan, Masahiro Yamada, linux-kernel, Greg Kroah-Hartman, Kees Cook, linux-kselftest, Brendan Higgins On Mon, Feb 04, 2019 at 05:39:57PM -0600, Luis Chamberlain wrote: > On Thu, Nov 29, 2018 at 8:31 PM Luis Chamberlain <mcgrof@kernel.org> wrote: > > > > On Mon, Nov 26, 2018 at 09:12:16PM -0600, Dan Rue wrote: > > > CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y is required for fw_fallback.sh. > > > Without it, fw_fallback.sh fails with 'usermode helper disabled so > > > ignoring test'. Enable the config in selftest so that it gets built by > > > default. > > > > > > Signed-off-by: Dan Rue <dan.rue@linaro.org> > > > --- > > > tools/testing/selftests/firmware/config | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/tools/testing/selftests/firmware/config b/tools/testing/selftests/firmware/config > > > index bf634dda0720..913a25a4a32b 100644 > > > --- a/tools/testing/selftests/firmware/config > > > +++ b/tools/testing/selftests/firmware/config > > > @@ -1,5 +1,6 @@ > > > CONFIG_TEST_FIRMWARE=y > > > CONFIG_FW_LOADER=y > > > CONFIG_FW_LOADER_USER_HELPER=y > > > +CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y > > > CONFIG_IKCONFIG=y > > > CONFIG_IKCONFIG_PROC=y > > > > NACK -- the point of the changes was to *allow* us to mimic such > > configuration through a proc sysctl knob. > > > > You aren forcing CONFIG_FW_LOADER_USER_HELPER_FALLBACK but just having > > CONFIG_FW_LOADER_USER_HELPER suffices to emulate the_FALLBACK > > functionality. > > Dan, again, you broke the whole point to the amount of work that went > into emulating testing. As such anyone testing their changes would > yield incorrect results. > > > The issue here seems to be that *all* tests fail once a configuration is > > found which is not suitable a tests. With the shiny new proc sysctls we > > can test all 3 kernel configurations in one shot. Since we test 3 > > different kernel configurations naturally some of these won't have the > > features needed, so that failure should be treated as non-fatal to allow > > the chain of other tests to continue. > > > > This issue was a regression due to commit a6a9be9270c87 ("selftests: > > firmware: return Kselftest Skip code for skipped tests") by Shuah for > > the verify_reqs(). We need to treat this as a non-fatal / don't skip > > return value. > > > > The following would fix this chaining issue: > > > > diff --git a/tools/testing/selftests/firmware/fw_lib.sh b/tools/testing/selftests/firmware/fw_lib.sh > > index 6c5f1b2ffb74..1cbb12e284a6 100755 > > --- a/tools/testing/selftests/firmware/fw_lib.sh > > +++ b/tools/testing/selftests/firmware/fw_lib.sh > > @@ -91,7 +91,7 @@ verify_reqs() > > if [ "$TEST_REQS_FW_SYSFS_FALLBACK" = "yes" ]; then > > if [ ! "$HAS_FW_LOADER_USER_HELPER" = "yes" ]; then > > echo "usermode helper disabled so ignoring test" > > - exit $ksft_skip > > + exit 0 > > fi > > fi > > } > > > > However its not clear to me if instead we want some new special return > > value for selftests so that the framework can detect an that an error > > is non-fatal, and can continue. This is a tricky situation given the > > script, existing upstream kernel module, are aware of such emulation > > hacks via sysctl, but knowledge of this is not obvious to selftests. > > > > Shuah, how do you suggest we handle this corner case? If you are OK > > with the above hunk for now I can send a fix for it. In either case > > this commit was added on v4.18, so the fix would be a stable fix. > > In lieu of any suggestion I'm going to request we revert this commit > and send the above fix. Sorry, I didn't realize this was waiting on me. I agree with all of your feedback. Please revert 7492902e8d22 ("selftests: firmware: add CONFIG_FW_LOADER_USER_HELPER_FALLBACK to config") and add my Acked-by to the proposed fix above. Shuah, do I need to send a patch for that revert? It would be nice if there were a way (maybe there is?) to let each of the individual tests be exposed and run by run_kselftest.sh so that each test gets its own proper pass/skip/fail. It could be done in this case by making fw_run_tests.sh look more like run_kselftest.sh (running each test in a subshell and capturing its exit code), but that starts to get a bit fragile and ugly, too. Dan > > Luis ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] selftests: firmware: add CONFIG_FW_LOADER_USER_HELPER_FALLBACK to config 2019-02-05 2:41 ` Dan Rue @ 2019-02-05 19:14 ` Luis Chamberlain 0 siblings, 0 replies; 13+ messages in thread From: Luis Chamberlain @ 2019-02-05 19:14 UTC (permalink / raw) To: Dan Rue Cc: Shuah Khan, Masahiro Yamada, linux-kernel, Greg Kroah-Hartman, Kees Cook, linux-kselftest, Brendan Higgins On Mon, Feb 04, 2019 at 08:41:50PM -0600, Dan Rue wrote: > On Mon, Feb 04, 2019 at 05:39:57PM -0600, Luis Chamberlain wrote: > > On Thu, Nov 29, 2018 at 8:31 PM Luis Chamberlain <mcgrof@kernel.org> wrote: > > > > > > On Mon, Nov 26, 2018 at 09:12:16PM -0600, Dan Rue wrote: > > > > CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y is required for fw_fallback.sh. > > > > Without it, fw_fallback.sh fails with 'usermode helper disabled so > > > > ignoring test'. Enable the config in selftest so that it gets built by > > > > default. > > > > > > > > Signed-off-by: Dan Rue <dan.rue@linaro.org> > > > > --- > > > > tools/testing/selftests/firmware/config | 1 + > > > > 1 file changed, 1 insertion(+) > > > > > > > > diff --git a/tools/testing/selftests/firmware/config b/tools/testing/selftests/firmware/config > > > > index bf634dda0720..913a25a4a32b 100644 > > > > --- a/tools/testing/selftests/firmware/config > > > > +++ b/tools/testing/selftests/firmware/config > > > > @@ -1,5 +1,6 @@ > > > > CONFIG_TEST_FIRMWARE=y > > > > CONFIG_FW_LOADER=y > > > > CONFIG_FW_LOADER_USER_HELPER=y > > > > +CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y > > > > CONFIG_IKCONFIG=y > > > > CONFIG_IKCONFIG_PROC=y > > > > > > NACK -- the point of the changes was to *allow* us to mimic such > > > configuration through a proc sysctl knob. > > > > > > You aren forcing CONFIG_FW_LOADER_USER_HELPER_FALLBACK but just having > > > CONFIG_FW_LOADER_USER_HELPER suffices to emulate the_FALLBACK > > > functionality. > > > > Dan, again, you broke the whole point to the amount of work that went > > into emulating testing. As such anyone testing their changes would > > yield incorrect results. > > > > > The issue here seems to be that *all* tests fail once a configuration is > > > found which is not suitable a tests. With the shiny new proc sysctls we > > > can test all 3 kernel configurations in one shot. Since we test 3 > > > different kernel configurations naturally some of these won't have the > > > features needed, so that failure should be treated as non-fatal to allow > > > the chain of other tests to continue. > > > > > > This issue was a regression due to commit a6a9be9270c87 ("selftests: > > > firmware: return Kselftest Skip code for skipped tests") by Shuah for > > > the verify_reqs(). We need to treat this as a non-fatal / don't skip > > > return value. > > > > > > The following would fix this chaining issue: > > > > > > diff --git a/tools/testing/selftests/firmware/fw_lib.sh b/tools/testing/selftests/firmware/fw_lib.sh > > > index 6c5f1b2ffb74..1cbb12e284a6 100755 > > > --- a/tools/testing/selftests/firmware/fw_lib.sh > > > +++ b/tools/testing/selftests/firmware/fw_lib.sh > > > @@ -91,7 +91,7 @@ verify_reqs() > > > if [ "$TEST_REQS_FW_SYSFS_FALLBACK" = "yes" ]; then > > > if [ ! "$HAS_FW_LOADER_USER_HELPER" = "yes" ]; then > > > echo "usermode helper disabled so ignoring test" > > > - exit $ksft_skip > > > + exit 0 > > > fi > > > fi > > > } > > > > > > However its not clear to me if instead we want some new special return > > > value for selftests so that the framework can detect an that an error > > > is non-fatal, and can continue. This is a tricky situation given the > > > script, existing upstream kernel module, are aware of such emulation > > > hacks via sysctl, but knowledge of this is not obvious to selftests. > > > > > > Shuah, how do you suggest we handle this corner case? If you are OK > > > with the above hunk for now I can send a fix for it. In either case > > > this commit was added on v4.18, so the fix would be a stable fix. > > > > In lieu of any suggestion I'm going to request we revert this commit > > and send the above fix. > > Sorry, I didn't realize this was waiting on me. I agree with all of your > feedback. Please revert 7492902e8d22 ("selftests: firmware: add > CONFIG_FW_LOADER_USER_HELPER_FALLBACK to config") and add my Acked-by to > the proposed fix above. > > Shuah, do I need to send a patch for that revert? I can send the revert and the subsequent fix. Luis ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2019-02-08 17:53 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-11-27 3:12 [PATCH 0/2] selftests: firmware: fw_filesystem fix for busybox Dan Rue 2018-11-27 3:12 ` [PATCH 1/2] selftests: firmware: remove use of non-standard diff -Z option Dan Rue 2018-11-27 17:56 ` Kees Cook 2018-11-30 2:37 ` Luis Chamberlain 2018-12-05 20:43 ` Dan Rue 2019-02-07 18:20 ` Luis Chamberlain 2019-02-08 17:53 ` shuah 2018-11-27 3:12 ` [PATCH 2/2] selftests: firmware: add CONFIG_FW_LOADER_USER_HELPER_FALLBACK to config Dan Rue 2018-11-27 17:56 ` Kees Cook 2018-11-30 2:31 ` Luis Chamberlain 2019-02-04 23:39 ` Luis Chamberlain 2019-02-05 2:41 ` Dan Rue 2019-02-05 19:14 ` Luis Chamberlain
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).