[v2,11/11] test_firmware: test three firmware kernel configs using a proc knob
diff mbox

Message ID 20180224024613.24078-12-mcgrof@kernel.org
State New
Headers show

Commit Message

Luis Chamberlain Feb. 24, 2018, 2:46 a.m. UTC
Since we now have knobs to twiddle what used to be set on kernel
configurations we can build one base kernel configuration and modify
behaviour to mimic such kernel configurations to test them.

Provided you build a kernel with:

CONFIG_TEST_FIRMWARE=y
CONFIG_FW_LOADER=y
CONFIG_FW_LOADER_USER_HELPER=y
CONFIG_IKCONFIG=y
CONFIG_IKCONFIG_PROC=y

We should now be able test all possible kernel configurations
when FW_LOADER=y. Note that when FW_LOADER=m we just don't provide
the built-in functionality of the built-in firmware.

If you're on an old kernel and either don't have /proc/config.gz
(CONFIG_IKCONFIG_PROC) or haven't enabled CONFIG_FW_LOADER_USER_HELPER
we cannot run these dynamic tests, so just run both scripts just
as we used to before making blunt assumptions about your setup
and requirements exactly as we did before.

Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 tools/testing/selftests/firmware/Makefile        |  2 +-
 tools/testing/selftests/firmware/fw_lib.sh       | 53 +++++++++++++++++++
 tools/testing/selftests/firmware/fw_run_tests.sh | 67 ++++++++++++++++++++++++
 3 files changed, 121 insertions(+), 1 deletion(-)
 create mode 100755 tools/testing/selftests/firmware/fw_run_tests.sh

Comments

Kees Cook Feb. 27, 2018, 11:18 p.m. UTC | #1
On Fri, Feb 23, 2018 at 6:46 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> Since we now have knobs to twiddle what used to be set on kernel
> configurations we can build one base kernel configuration and modify
> behaviour to mimic such kernel configurations to test them.
>
> Provided you build a kernel with:
>
> CONFIG_TEST_FIRMWARE=y
> CONFIG_FW_LOADER=y
> CONFIG_FW_LOADER_USER_HELPER=y
> CONFIG_IKCONFIG=y
> CONFIG_IKCONFIG_PROC=y
>
> We should now be able test all possible kernel configurations
> when FW_LOADER=y. Note that when FW_LOADER=m we just don't provide
> the built-in functionality of the built-in firmware.
>
> If you're on an old kernel and either don't have /proc/config.gz
> (CONFIG_IKCONFIG_PROC) or haven't enabled CONFIG_FW_LOADER_USER_HELPER
> we cannot run these dynamic tests, so just run both scripts just
> as we used to before making blunt assumptions about your setup
> and requirements exactly as we did before.
>
> Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>

Cool. Nice to have it all in one test build now. :)

Acked-by: Kees Cook <keescook@chromium.org>

-Kees

> ---
>  tools/testing/selftests/firmware/Makefile        |  2 +-
>  tools/testing/selftests/firmware/fw_lib.sh       | 53 +++++++++++++++++++
>  tools/testing/selftests/firmware/fw_run_tests.sh | 67 ++++++++++++++++++++++++
>  3 files changed, 121 insertions(+), 1 deletion(-)
>  create mode 100755 tools/testing/selftests/firmware/fw_run_tests.sh
>
> diff --git a/tools/testing/selftests/firmware/Makefile b/tools/testing/selftests/firmware/Makefile
> index 1894d625af2d..826f38d5dd19 100644
> --- a/tools/testing/selftests/firmware/Makefile
> +++ b/tools/testing/selftests/firmware/Makefile
> @@ -3,7 +3,7 @@
>  # No binaries, but make sure arg-less "make" doesn't trigger "run_tests"
>  all:
>
> -TEST_PROGS := fw_filesystem.sh fw_fallback.sh
> +TEST_PROGS := fw_run_tests.sh
>
>  include ../lib.mk
>
> diff --git a/tools/testing/selftests/firmware/fw_lib.sh b/tools/testing/selftests/firmware/fw_lib.sh
> index 0702dbf0f06b..3362a2aac40e 100755
> --- a/tools/testing/selftests/firmware/fw_lib.sh
> +++ b/tools/testing/selftests/firmware/fw_lib.sh
> @@ -47,6 +47,34 @@ check_setup()
>  {
>         HAS_FW_LOADER_USER_HELPER=$(kconfig_has CONFIG_FW_LOADER_USER_HELPER=y)
>         HAS_FW_LOADER_USER_HELPER_FALLBACK=$(kconfig_has CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y)
> +       PROC_FW_IGNORE_SYSFS_FALLBACK="N"
> +       PROC_FW_FORCE_SYSFS_FALLBACK="N"
> +
> +       if [ -z $PROC_SYS_DIR ]; then
> +               PROC_SYS_DIR="/proc/sys/kernel"
> +       fi
> +
> +       FW_PROC="${PROC_SYS_DIR}/firmware_config"
> +       FW_FORCE_SYSFS_FALLBACK="$FW_PROC/force_sysfs_fallback"
> +       FW_IGNORE_SYSFS_FALLBACK="$FW_PROC/ignore_sysfs_fallback"
> +
> +       if [ -f $FW_FORCE_SYSFS_FALLBACK ]; then
> +               PROC_FW_FORCE_SYSFS_FALLBACK=$(cat $FW_FORCE_SYSFS_FALLBACK)
> +       fi
> +
> +       if [ -f $FW_IGNORE_SYSFS_FALLBACK ]; then
> +               PROC_FW_IGNORE_SYSFS_FALLBACK=$(cat $FW_IGNORE_SYSFS_FALLBACK)
> +       fi
> +
> +       if [ "$PROC_FW_IGNORE_SYSFS_FALLBACK" = "1" ]; then
> +               HAS_FW_LOADER_USER_HELPER_FALLBACK="no"
> +               HAS_FW_LOADER_USER_HELPER="no"
> +       fi
> +
> +       if [ "$PROC_FW_FORCE_SYSFS_FALLBACK" = "1" ]; then
> +               HAS_FW_LOADER_USER_HELPER="yes"
> +               HAS_FW_LOADER_USER_HELPER_FALLBACK="yes"
> +       fi
>
>         if [ "$HAS_FW_LOADER_USER_HELPER" = "yes" ]; then
>                OLD_TIMEOUT=$(cat /sys/class/firmware/timeout)
> @@ -76,6 +104,30 @@ setup_tmp_file()
>         fi
>  }
>
> +proc_set_force_sysfs_fallback()
> +{
> +       if [ -f $FW_FORCE_SYSFS_FALLBACK ]; then
> +               echo -n $1 > $FW_FORCE_SYSFS_FALLBACK
> +               PROC_FW_FORCE_SYSFS_FALLBACK=$(cat $FW_FORCE_SYSFS_FALLBACK)
> +               check_setup
> +       fi
> +}
> +
> +proc_set_ignore_sysfs_fallback()
> +{
> +       if [ -f $FW_IGNORE_SYSFS_FALLBACK ]; then
> +               echo -n $1 > $FW_IGNORE_SYSFS_FALLBACK
> +               PROC_FW_IGNORE_SYSFS_FALLBACK=$(cat $FW_IGNORE_SYSFS_FALLBACK)
> +               check_setup
> +       fi
> +}
> +
> +proc_restore_defaults()
> +{
> +       proc_set_force_sysfs_fallback 0
> +       proc_set_ignore_sysfs_fallback 0
> +}
> +
>  test_finish()
>  {
>         if [ "$HAS_FW_LOADER_USER_HELPER" = "yes" ]; then
> @@ -93,6 +145,7 @@ test_finish()
>         if [ -d $FWPATH ]; then
>                 rm -rf "$FWPATH"
>         fi
> +       proc_restore_defaults
>  }
>
>  kconfig_has()
> diff --git a/tools/testing/selftests/firmware/fw_run_tests.sh b/tools/testing/selftests/firmware/fw_run_tests.sh
> new file mode 100755
> index 000000000000..a12b5809ad8b
> --- /dev/null
> +++ b/tools/testing/selftests/firmware/fw_run_tests.sh
> @@ -0,0 +1,67 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +
> +# This runs all known tests across all known possible configurations we could
> +# emulate in one run.
> +
> +set -e
> +
> +TEST_DIR=$(dirname $0)
> +source $TEST_DIR/fw_lib.sh
> +
> +run_tests()
> +{
> +       $TEST_DIR/fw_filesystem.sh
> +       $TEST_DIR/fw_fallback.sh
> +}
> +
> +run_test_config_0001()
> +{
> +       echo "-----------------------------------------------------"
> +       echo "Running kernel configuration test 1 -- rare"
> +       echo "Emulates:"
> +       echo "CONFIG_FW_LOADER=y"
> +       echo "CONFIG_FW_LOADER_USER_HELPER=n"
> +       echo "CONFIG_FW_LOADER_USER_HELPER_FALLBACK=n"
> +       proc_set_force_sysfs_fallback 0
> +       proc_set_ignore_sysfs_fallback 1
> +       run_tests
> +}
> +
> +run_test_config_0002()
> +{
> +       echo "-----------------------------------------------------"
> +       echo "Running kernel configuration test 2 -- distro"
> +       echo "Emulates:"
> +       echo "CONFIG_FW_LOADER=y"
> +       echo "CONFIG_FW_LOADER_USER_HELPER=y"
> +       echo "CONFIG_FW_LOADER_USER_HELPER_FALLBACK=n"
> +       proc_set_force_sysfs_fallback 0
> +       proc_set_ignore_sysfs_fallback 0
> +       run_tests
> +}
> +
> +run_test_config_0003()
> +{
> +       echo "-----------------------------------------------------"
> +       echo "Running kernel configuration test 3 -- android"
> +       echo "Emulates:"
> +       echo "CONFIG_FW_LOADER=y"
> +       echo "CONFIG_FW_LOADER_USER_HELPER=y"
> +       echo "CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y"
> +       proc_set_force_sysfs_fallback 1
> +       proc_set_ignore_sysfs_fallback 0
> +       run_tests
> +}
> +
> +check_mods
> +check_setup
> +
> +if [ -f $FW_FORCE_SYSFS_FALLBACK ]; then
> +       run_test_config_0001
> +       run_test_config_0002
> +       run_test_config_0003
> +else
> +       echo "Running basic kernel configuration, working with your config"
> +       run_test
> +fi
> --
> 2.16.2
>
Luis Chamberlain Feb. 28, 2018, 1:32 a.m. UTC | #2
On Tue, Feb 27, 2018 at 03:18:15PM -0800, Kees Cook wrote:
> On Fri, Feb 23, 2018 at 6:46 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> > Since we now have knobs to twiddle what used to be set on kernel
> > configurations we can build one base kernel configuration and modify
> > behaviour to mimic such kernel configurations to test them.
> >
> > Provided you build a kernel with:
> >
> > CONFIG_TEST_FIRMWARE=y
> > CONFIG_FW_LOADER=y
> > CONFIG_FW_LOADER_USER_HELPER=y
> > CONFIG_IKCONFIG=y
> > CONFIG_IKCONFIG_PROC=y
> >
> > We should now be able test all possible kernel configurations
> > when FW_LOADER=y. Note that when FW_LOADER=m we just don't provide
> > the built-in functionality of the built-in firmware.
> >
> > If you're on an old kernel and either don't have /proc/config.gz
> > (CONFIG_IKCONFIG_PROC) or haven't enabled CONFIG_FW_LOADER_USER_HELPER
> > we cannot run these dynamic tests, so just run both scripts just
> > as we used to before making blunt assumptions about your setup
> > and requirements exactly as we did before.
> >
> > Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
> 
> Cool. Nice to have it all in one test build now. :)

Now what about we start discussing one kernel config only for the future?  The
impact would be the size of the fallback mechanism. That should be a bit clear
in terms of size impact after this series.

Wonder what Josh thinks as he help with tinyconfig. We could target v4.18 if
its sensible.

> Acked-by: Kees Cook <keescook@chromium.org>

  Luis
Josh Triplett Feb. 28, 2018, 9:07 a.m. UTC | #3
On Wed, Feb 28, 2018 at 01:32:37AM +0000, Luis R. Rodriguez wrote:
> On Tue, Feb 27, 2018 at 03:18:15PM -0800, Kees Cook wrote:
> > On Fri, Feb 23, 2018 at 6:46 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> > > Since we now have knobs to twiddle what used to be set on kernel
> > > configurations we can build one base kernel configuration and modify
> > > behaviour to mimic such kernel configurations to test them.
> > >
> > > Provided you build a kernel with:
> > >
> > > CONFIG_TEST_FIRMWARE=y
> > > CONFIG_FW_LOADER=y
> > > CONFIG_FW_LOADER_USER_HELPER=y
> > > CONFIG_IKCONFIG=y
> > > CONFIG_IKCONFIG_PROC=y
> > >
> > > We should now be able test all possible kernel configurations
> > > when FW_LOADER=y. Note that when FW_LOADER=m we just don't provide
> > > the built-in functionality of the built-in firmware.
> > >
> > > If you're on an old kernel and either don't have /proc/config.gz
> > > (CONFIG_IKCONFIG_PROC) or haven't enabled CONFIG_FW_LOADER_USER_HELPER
> > > we cannot run these dynamic tests, so just run both scripts just
> > > as we used to before making blunt assumptions about your setup
> > > and requirements exactly as we did before.
> > >
> > > Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
> > 
> > Cool. Nice to have it all in one test build now. :)
> 
> Now what about we start discussing one kernel config only for the future?  The
> impact would be the size of the fallback mechanism. That should be a bit clear
> in terms of size impact after this series.
> 
> Wonder what Josh thinks as he help with tinyconfig. We could target v4.18 if
> its sensible.

Having any of these unconditionally compiled in seems likely to be a
significant impact, both directly and because of what else it would
implicitly prevent compiling out or removing. And the firmware loader,
for instance, is something that many kernels or hardware will not need
at all.
Luis Chamberlain Feb. 28, 2018, 6:26 p.m. UTC | #4
On Wed, Feb 28, 2018 at 01:07:23AM -0800, Josh Triplett wrote:
> On Wed, Feb 28, 2018 at 01:32:37AM +0000, Luis R. Rodriguez wrote:
> > On Tue, Feb 27, 2018 at 03:18:15PM -0800, Kees Cook wrote:
> > > On Fri, Feb 23, 2018 at 6:46 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> > > > Since we now have knobs to twiddle what used to be set on kernel
> > > > configurations we can build one base kernel configuration and modify
> > > > behaviour to mimic such kernel configurations to test them.
> > > >
> > > > Provided you build a kernel with:
> > > >
> > > > CONFIG_TEST_FIRMWARE=y
> > > > CONFIG_FW_LOADER=y
> > > > CONFIG_FW_LOADER_USER_HELPER=y
> > > > CONFIG_IKCONFIG=y
> > > > CONFIG_IKCONFIG_PROC=y
> > > >
> > > > We should now be able test all possible kernel configurations
> > > > when FW_LOADER=y. Note that when FW_LOADER=m we just don't provide
> > > > the built-in functionality of the built-in firmware.
> > > >
> > > > If you're on an old kernel and either don't have /proc/config.gz
> > > > (CONFIG_IKCONFIG_PROC) or haven't enabled CONFIG_FW_LOADER_USER_HELPER
> > > > we cannot run these dynamic tests, so just run both scripts just
> > > > as we used to before making blunt assumptions about your setup
> > > > and requirements exactly as we did before.
> > > >
> > > > Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
> > > 
> > > Cool. Nice to have it all in one test build now. :)
> > 
> > Now what about we start discussing one kernel config only for the future?  The
> > impact would be the size of the fallback mechanism. That should be a bit clear
> > in terms of size impact after this series.
> > 
> > Wonder what Josh thinks as he help with tinyconfig. We could target v4.18 if
> > its sensible.
> 
> Having any of these unconditionally compiled in seems likely to be a
> significant impact, both directly and because of what else it would
> implicitly prevent compiling out or removing. And the firmware loader,
> for instance, is something that many kernels or hardware will not need
> at all.

Oh sorry, I did not mean always enabling the firmware loader, that would add
an extra 828 bytes, and 14264 bytes if the fallback mechanism is enabled as
well.

I meant having only CONFIG_FW_LOADER=y, and removing
CONFIG_FW_LOADER_USER_HELPER so that we just always compile it in if we have
CONFIG_FW_LOADER=y, so a penalty of 13436 bytes for those who enabled the
firmware loader but hadn't before enabled the fallback mechanism.

I'll note CONFIG_FW_LOADER_USER_HELPER is actually known to be enabled by most
distributions these days. We have an extra CONFIG_FW_LOADER_USER_HELPER_FALLBACK
but this is now just a toggle of a boolean, and actually Android is known to
enable it mostly, not other Linux distributions. Since Android enables
CONFIG_FW_LOADER_USER_HELPER_FALLBACK we know they also enable the fallback
mechanism with CONFIG_FW_LOADER_USER_HELPER_FALLBACK.

So for folks who enable CONFIG_FW_LOADER=y, they'd now be forced to gain an
extra 13436 bytes broken down as follows:

-------------------------------------------------------------------------------------------
allnoconfig with no firmware loader (with procfs enabled):                      
$ size vmlinux                                                                  
   text    data     bss     dec     hex filename                                
1135188  272012 1219736 2626936  281578 vmlinux                                 
                                                                                
$ du -b vmlinux                                                                 
1745556 vmlinux                                                                 
-------------------------------------------------------------------------------------------
CONFIG_FW_LOADER=y                                                              
$ size vmlinux                                                                  
   text    data     bss     dec     hex filename                                
1137244  267984 1219716 2624944  280db0 vmlinux                                 
                                                                                
$ du -b vmlinux                                                                 
1746384 vmlinux                                                                 
-------------------------------------------------------------------------------------------
CONFIG_FW_LOADER=y                                                              
CONFIG_FW_LOADER_USER_HELPER=y                                                  
$ size vmlinux                                                                  
   text    data     bss     dec     hex filename                                
1140554  272464 1219716 2632734  282c1e vmlinux                                 
$ du -b vmlinux
1759820 vmlinux

  Luis
Josh Triplett March 1, 2018, midnight UTC | #5
On Wed, Feb 28, 2018 at 06:26:03PM +0000, Luis R. Rodriguez wrote:
> On Wed, Feb 28, 2018 at 01:07:23AM -0800, Josh Triplett wrote:
> > On Wed, Feb 28, 2018 at 01:32:37AM +0000, Luis R. Rodriguez wrote:
> > > On Tue, Feb 27, 2018 at 03:18:15PM -0800, Kees Cook wrote:
> > > > On Fri, Feb 23, 2018 at 6:46 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> > > > > Since we now have knobs to twiddle what used to be set on kernel
> > > > > configurations we can build one base kernel configuration and modify
> > > > > behaviour to mimic such kernel configurations to test them.
> > > > >
> > > > > Provided you build a kernel with:
> > > > >
> > > > > CONFIG_TEST_FIRMWARE=y
> > > > > CONFIG_FW_LOADER=y
> > > > > CONFIG_FW_LOADER_USER_HELPER=y
> > > > > CONFIG_IKCONFIG=y
> > > > > CONFIG_IKCONFIG_PROC=y
> > > > >
> > > > > We should now be able test all possible kernel configurations
> > > > > when FW_LOADER=y. Note that when FW_LOADER=m we just don't provide
> > > > > the built-in functionality of the built-in firmware.
> > > > >
> > > > > If you're on an old kernel and either don't have /proc/config.gz
> > > > > (CONFIG_IKCONFIG_PROC) or haven't enabled CONFIG_FW_LOADER_USER_HELPER
> > > > > we cannot run these dynamic tests, so just run both scripts just
> > > > > as we used to before making blunt assumptions about your setup
> > > > > and requirements exactly as we did before.
> > > > >
> > > > > Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
> > > > 
> > > > Cool. Nice to have it all in one test build now. :)
> > > 
> > > Now what about we start discussing one kernel config only for the future?  The
> > > impact would be the size of the fallback mechanism. That should be a bit clear
> > > in terms of size impact after this series.
> > > 
> > > Wonder what Josh thinks as he help with tinyconfig. We could target v4.18 if
> > > its sensible.
> > 
> > Having any of these unconditionally compiled in seems likely to be a
> > significant impact, both directly and because of what else it would
> > implicitly prevent compiling out or removing. And the firmware loader,
> > for instance, is something that many kernels or hardware will not need
> > at all.
> 
> Oh sorry, I did not mean always enabling the firmware loader, that would add
> an extra 828 bytes, and 14264 bytes if the fallback mechanism is enabled as
> well.
> 
> I meant having only CONFIG_FW_LOADER=y, and removing
> CONFIG_FW_LOADER_USER_HELPER so that we just always compile it in if we have
> CONFIG_FW_LOADER=y, so a penalty of 13436 bytes for those who enabled the
> firmware loader but hadn't before enabled the fallback mechanism.
> 
> I'll note CONFIG_FW_LOADER_USER_HELPER is actually known to be enabled by most
> distributions these days. We have an extra CONFIG_FW_LOADER_USER_HELPER_FALLBACK
> but this is now just a toggle of a boolean, and actually Android is known to
> enable it mostly, not other Linux distributions. Since Android enables
> CONFIG_FW_LOADER_USER_HELPER_FALLBACK we know they also enable the fallback
> mechanism with CONFIG_FW_LOADER_USER_HELPER_FALLBACK.
> 
> So for folks who enable CONFIG_FW_LOADER=y, they'd now be forced to gain an
> extra 13436 bytes broken down as follows:

Ah, I see.

If you have CONFIG_FW_LOADER and not CONFIG_FW_LOADER_USER_HELPER, then
you only have the in-kernel firmware loading mechanism? Given the
*substantial* size difference between the two, it seems useful to have
that option. What would it gain to combine the two?
Luis Chamberlain March 1, 2018, 12:38 a.m. UTC | #6
On Wed, Feb 28, 2018 at 04:00:58PM -0800, Josh Triplett wrote:
> On Wed, Feb 28, 2018 at 06:26:03PM +0000, Luis R. Rodriguez wrote:
> > So for folks who enable CONFIG_FW_LOADER=y, they'd now be forced to gain an
> > extra 13436 bytes broken down as follows:
> 
> Ah, I see.
> 
> If you have CONFIG_FW_LOADER and not CONFIG_FW_LOADER_USER_HELPER, then
> you only have the in-kernel firmware loading mechanism?

Right, we don't have the old fallback mechanism (which BTW used to be
the default way back in the hayday).

> Given the
> *substantial* size difference between the two, it seems useful to have
> that option.

That's what I wanted to get to, is 13436 bytes is*substantial* enough to
merit a kernel configuration option? It seems like that is the case.

> What would it gain to combine the two?

Well Android enables CONFIG_FW_LOADER_USER_HELPER, and if they do, I was trying
to think if there really was any point in having CONFIG_FW_LOADER_USER_HELPER
as an option. Who would enable CONFIG_FW_LOADER but not
CONFIG_FW_LOADER_USER_HELPER?

You see other than
CONFIG_FW_LOADER_USER_HELPER
we also have
CONFIG_FW_LOADER_USER_HELPER_FALLBACK
and Android defaults that to y too.

It used to be that CONFIG_FW_LOADER_USER_HELPER_FALLBACK was a mess to
understand in code, and this series has reduced it to simple bool now.

I started wondering if trimming kconfig options may be worth it. Sadly
I don't think we can remove CONFIG_FW_LOADER_USER_HELPER_FALLBACK, and
we'll have to just deal with it mapping to switching a sysctl option.

But I figured it was a good time to also reconsider also if we even had
any need for CONFIG_FW_LOADER_USER_HELPER.

The less hairball of mess of kconfig options the better to test. Even
though this series has reduced being able to consolidating being
able to make a kernel now which lets us test all configurations in
one build.

Who would save some 13436 bytes in the real world?

  Luis
Josh Triplett March 1, 2018, 2:25 a.m. UTC | #7
On Thu, Mar 01, 2018 at 12:38:16AM +0000, Luis R. Rodriguez wrote:
> On Wed, Feb 28, 2018 at 04:00:58PM -0800, Josh Triplett wrote:
> > On Wed, Feb 28, 2018 at 06:26:03PM +0000, Luis R. Rodriguez wrote:
> > > So for folks who enable CONFIG_FW_LOADER=y, they'd now be forced to gain an
> > > extra 13436 bytes broken down as follows:
> > 
> > Ah, I see.
> > 
> > If you have CONFIG_FW_LOADER and not CONFIG_FW_LOADER_USER_HELPER, then
> > you only have the in-kernel firmware loading mechanism?
> 
> Right, we don't have the old fallback mechanism (which BTW used to be
> the default way back in the hayday).
> 
> > Given the
> > *substantial* size difference between the two, it seems useful to have
> > that option.
> 
> That's what I wanted to get to, is 13436 bytes is*substantial* enough to
> merit a kernel configuration option? It seems like that is the case.

By at least an order of magnitude, yes.

> > What would it gain to combine the two?
> 
> Well Android enables CONFIG_FW_LOADER_USER_HELPER, and if they do, I was trying
> to think if there really was any point in having CONFIG_FW_LOADER_USER_HELPER
> as an option. Who would enable CONFIG_FW_LOADER but not
> CONFIG_FW_LOADER_USER_HELPER?

An embedded system with a fixed set of hardware that needs exclusively a
fixed set of firmware files known at system build time.

> The less hairball of mess of kconfig options the better to test. Even
> though this series has reduced being able to consolidating being
> able to make a kernel now which lets us test all configurations in
> one build.
> 
> Who would save some 13436 bytes in the real world?

*raises hand*
Luis Chamberlain March 1, 2018, 5:33 p.m. UTC | #8
On Wed, Feb 28, 2018 at 06:25:16PM -0800, Josh Triplett wrote:
> On Thu, Mar 01, 2018 at 12:38:16AM +0000, Luis R. Rodriguez wrote:
> > On Wed, Feb 28, 2018 at 04:00:58PM -0800, Josh Triplett wrote:
> > > On Wed, Feb 28, 2018 at 06:26:03PM +0000, Luis R. Rodriguez wrote:
> > > > So for folks who enable CONFIG_FW_LOADER=y, they'd now be forced to gain an
> > > > extra 13436 bytes broken down as follows:
> > > 
> > > Ah, I see.
> > > 
> > > If you have CONFIG_FW_LOADER and not CONFIG_FW_LOADER_USER_HELPER, then
> > > you only have the in-kernel firmware loading mechanism?
> > 
> > Right, we don't have the old fallback mechanism (which BTW used to be
> > the default way back in the hayday).
> > 
> > > Given the
> > > *substantial* size difference between the two, it seems useful to have
> > > that option.
> > 
> > That's what I wanted to get to, is 13436 bytes *substantial* enough to
> > merit a kernel configuration option? It seems like that is the case.
> 
> By at least an order of magnitude, yes.

OK, then now we have a worthy reasonable description to amend into the kconfig
option too. And since its now revisited, I guess we can live with it for a good
while.

> > > What would it gain to combine the two?
> > 
> > Well Android enables CONFIG_FW_LOADER_USER_HELPER, and if they do, I was trying
> > to think if there really was any point in having CONFIG_FW_LOADER_USER_HELPER
> > as an option. Who would enable CONFIG_FW_LOADER but not
> > CONFIG_FW_LOADER_USER_HELPER?
> 
> An embedded system with a fixed set of hardware that needs exclusively a
> fixed set of firmware files known at system build time.

Fair enough, this should help also in the description.

> > The less hairball of mess of kconfig options the better to test. Even
> > though this series has reduced being able to consolidating being
> > able to make a kernel now which lets us test all configurations in
> > one build.
> > 
> > Who would save some 13436 bytes in the real world?
> 
> *raises hand*

Thanks for the feedback.

  Luis

Patch
diff mbox

diff --git a/tools/testing/selftests/firmware/Makefile b/tools/testing/selftests/firmware/Makefile
index 1894d625af2d..826f38d5dd19 100644
--- a/tools/testing/selftests/firmware/Makefile
+++ b/tools/testing/selftests/firmware/Makefile
@@ -3,7 +3,7 @@ 
 # No binaries, but make sure arg-less "make" doesn't trigger "run_tests"
 all:
 
-TEST_PROGS := fw_filesystem.sh fw_fallback.sh
+TEST_PROGS := fw_run_tests.sh
 
 include ../lib.mk
 
diff --git a/tools/testing/selftests/firmware/fw_lib.sh b/tools/testing/selftests/firmware/fw_lib.sh
index 0702dbf0f06b..3362a2aac40e 100755
--- a/tools/testing/selftests/firmware/fw_lib.sh
+++ b/tools/testing/selftests/firmware/fw_lib.sh
@@ -47,6 +47,34 @@  check_setup()
 {
 	HAS_FW_LOADER_USER_HELPER=$(kconfig_has CONFIG_FW_LOADER_USER_HELPER=y)
 	HAS_FW_LOADER_USER_HELPER_FALLBACK=$(kconfig_has CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y)
+	PROC_FW_IGNORE_SYSFS_FALLBACK="N"
+	PROC_FW_FORCE_SYSFS_FALLBACK="N"
+
+	if [ -z $PROC_SYS_DIR ]; then
+		PROC_SYS_DIR="/proc/sys/kernel"
+	fi
+
+	FW_PROC="${PROC_SYS_DIR}/firmware_config"
+	FW_FORCE_SYSFS_FALLBACK="$FW_PROC/force_sysfs_fallback"
+	FW_IGNORE_SYSFS_FALLBACK="$FW_PROC/ignore_sysfs_fallback"
+
+	if [ -f $FW_FORCE_SYSFS_FALLBACK ]; then
+		PROC_FW_FORCE_SYSFS_FALLBACK=$(cat $FW_FORCE_SYSFS_FALLBACK)
+	fi
+
+	if [ -f $FW_IGNORE_SYSFS_FALLBACK ]; then
+		PROC_FW_IGNORE_SYSFS_FALLBACK=$(cat $FW_IGNORE_SYSFS_FALLBACK)
+	fi
+
+	if [ "$PROC_FW_IGNORE_SYSFS_FALLBACK" = "1" ]; then
+		HAS_FW_LOADER_USER_HELPER_FALLBACK="no"
+		HAS_FW_LOADER_USER_HELPER="no"
+	fi
+
+	if [ "$PROC_FW_FORCE_SYSFS_FALLBACK" = "1" ]; then
+		HAS_FW_LOADER_USER_HELPER="yes"
+		HAS_FW_LOADER_USER_HELPER_FALLBACK="yes"
+	fi
 
 	if [ "$HAS_FW_LOADER_USER_HELPER" = "yes" ]; then
 	       OLD_TIMEOUT=$(cat /sys/class/firmware/timeout)
@@ -76,6 +104,30 @@  setup_tmp_file()
 	fi
 }
 
+proc_set_force_sysfs_fallback()
+{
+	if [ -f $FW_FORCE_SYSFS_FALLBACK ]; then
+		echo -n $1 > $FW_FORCE_SYSFS_FALLBACK
+		PROC_FW_FORCE_SYSFS_FALLBACK=$(cat $FW_FORCE_SYSFS_FALLBACK)
+		check_setup
+	fi
+}
+
+proc_set_ignore_sysfs_fallback()
+{
+	if [ -f $FW_IGNORE_SYSFS_FALLBACK ]; then
+		echo -n $1 > $FW_IGNORE_SYSFS_FALLBACK
+		PROC_FW_IGNORE_SYSFS_FALLBACK=$(cat $FW_IGNORE_SYSFS_FALLBACK)
+		check_setup
+	fi
+}
+
+proc_restore_defaults()
+{
+	proc_set_force_sysfs_fallback 0
+	proc_set_ignore_sysfs_fallback 0
+}
+
 test_finish()
 {
 	if [ "$HAS_FW_LOADER_USER_HELPER" = "yes" ]; then
@@ -93,6 +145,7 @@  test_finish()
 	if [ -d $FWPATH ]; then
 		rm -rf "$FWPATH"
 	fi
+	proc_restore_defaults
 }
 
 kconfig_has()
diff --git a/tools/testing/selftests/firmware/fw_run_tests.sh b/tools/testing/selftests/firmware/fw_run_tests.sh
new file mode 100755
index 000000000000..a12b5809ad8b
--- /dev/null
+++ b/tools/testing/selftests/firmware/fw_run_tests.sh
@@ -0,0 +1,67 @@ 
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+
+# This runs all known tests across all known possible configurations we could
+# emulate in one run.
+
+set -e
+
+TEST_DIR=$(dirname $0)
+source $TEST_DIR/fw_lib.sh
+
+run_tests()
+{
+	$TEST_DIR/fw_filesystem.sh
+	$TEST_DIR/fw_fallback.sh
+}
+
+run_test_config_0001()
+{
+	echo "-----------------------------------------------------"
+	echo "Running kernel configuration test 1 -- rare"
+	echo "Emulates:"
+	echo "CONFIG_FW_LOADER=y"
+	echo "CONFIG_FW_LOADER_USER_HELPER=n"
+	echo "CONFIG_FW_LOADER_USER_HELPER_FALLBACK=n"
+	proc_set_force_sysfs_fallback 0
+	proc_set_ignore_sysfs_fallback 1
+	run_tests
+}
+
+run_test_config_0002()
+{
+	echo "-----------------------------------------------------"
+	echo "Running kernel configuration test 2 -- distro"
+	echo "Emulates:"
+	echo "CONFIG_FW_LOADER=y"
+	echo "CONFIG_FW_LOADER_USER_HELPER=y"
+	echo "CONFIG_FW_LOADER_USER_HELPER_FALLBACK=n"
+	proc_set_force_sysfs_fallback 0
+	proc_set_ignore_sysfs_fallback 0
+	run_tests
+}
+
+run_test_config_0003()
+{
+	echo "-----------------------------------------------------"
+	echo "Running kernel configuration test 3 -- android"
+	echo "Emulates:"
+	echo "CONFIG_FW_LOADER=y"
+	echo "CONFIG_FW_LOADER_USER_HELPER=y"
+	echo "CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y"
+	proc_set_force_sysfs_fallback 1
+	proc_set_ignore_sysfs_fallback 0
+	run_tests
+}
+
+check_mods
+check_setup
+
+if [ -f $FW_FORCE_SYSFS_FALLBACK ]; then
+	run_test_config_0001
+	run_test_config_0002
+	run_test_config_0003
+else
+	echo "Running basic kernel configuration, working with your config"
+	run_test
+fi