firmware: cleanup for v4.17

When a kernel is not built with:

CONFIG_HAS_FW_LOADER_USER_HELPER_FALLBACK=y

We don't currently enable testing fw_fallback.sh. For kernels that still enable the fallback mechanism, its possible to use the async request firmware API call request_firmware_nowait() using the custom interface to use the fallback mechanism, so we should be able to test this but we currently cannot.

We can enable testing without CONFIG_HAS_FW_LOADER_USER_HELPER_FALLBACK=y by relying on /proc/config.gz (CONFIG_IKCONFIG_PROC), if present. If you don't have this we'll have no option but to rely on old heuristics for now.

Signed-off-by Luis R. Rodriguez <[email protected]>

---
 tools/testing/selftests/firmware/config         |  4 +++
 tools/testing/selftests/firmware/fw_fallback.sh | 45 ++++++++++++++++++++++++-
 2 files changed, 48 insertions(+), 1 deletion(-)

tools/testing/selftests/firmware/config +4 -0

index c8137f70e291..bf634dda0720 100644
@@ -1 +1,5 @@
 CONFIG_TEST_FIRMWARE=y
+CONFIG_FW_LOADER=y
+CONFIG_FW_LOADER_USER_HELPER=y
+CONFIG_IKCONFIG=y
+CONFIG_IKCONFIG_PROC=y

tools/testing/selftests/firmware/fw_fallback.sh +29 -0

index 722cad91df74..a42e437363d9 100755
@@ -6,7 +6,46 @@
 # won't find so that we can do the load ourself manually.
 set -e
 
+PROC_CONFIG="/proc/config.gz"
+TEST_DIR=$(dirname $0)
+
 modprobe test_firmware
+if [ ! -f $PROC_CONFIG ]; then
+       if modprobe configs 2>/dev/null; then
+               echo "Loaded configs module"
+               if [ ! -f $PROC_CONFIG ]; then
+                       echo "You must have the following enabled in your kernel:" >&2
+                       cat $TEST_DIR/config >&2
+                       echo "Resorting to old heuristics" >&2
+               fi
+       else
+               echo "Failed to load configs module, using old heuristics" >&2
+       fi
+fi
+
+kconfig_has()
+{
+       if [ -f $PROC_CONFIG ]; then
+               if zgrep -q $1 $PROC_CONFIG 2>/dev/null; then
+                       echo "yes"
+               else
+                       echo "no"
+               fi
+       else
+               # We currently don't have easy heuristics to infer this
+               # so best we can do is just try to use the kernel assuming
+               # you had enabled it. This matches the old behaviour.
+               if [ "$1" = "CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y" ]; then

tools/testing/selftests/firmware/fw_fallback.sh +5 -2

 DIR=/sys/devices/virtual/misc/test_firmware
 

@@ -14,6 +53,7 @@ DIR=/sys/devices/virtual/misc/test_firmware
 # These days no one enables CONFIG_FW_LOADER_USER_HELPER so check for that
 # as an indicator for CONFIG_FW_LOADER_USER_HELPER.
 HAS_FW_LOADER_USER_HELPER=$(if [ -d /sys/class/firmware/ ]; then echo yes; else echo no; fi)
+HAS_FW_LOADER_USER_HELPER_FALLBACK=$(kconfig_has CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y)
 
 if [ "$HAS_FW_LOADER_USER_HELPER" = "yes" ]; then
        OLD_TIMEOUT=$(cat /sys/class/firmware/timeout)

@@ -286,7 +326,10 @@ run_sysfs_custom_load_tests()
        fi
 }
 
-run_sysfs_main_tests
+if [ "$HAS_FW_LOADER_USER_HELPER_FALLBACK" = "yes" ]; then
+       run_sysfs_main_tests
+fi
+
 run_sysfs_custom_load_tests
 
 exit 0
--

2.16.2

Now that we have a kconfig checker just use that instead of relying on testing a sysfs directory being present, since our requirements are spelled out.

Signed-off-by Luis R. Rodriguez <[email protected]>

---
 tools/testing/selftests/firmware/fw_fallback.sh | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

tools/testing/selftests/firmware/fw_fallback.sh +1 -5

index a42e437363d9..40b6c1d3e832 100755
@@ -49,10 +49,7 @@ kconfig_has()
 
 DIR=/sys/devices/virtual/misc/test_firmware
 
-# CONFIG_FW_LOADER_USER_HELPER has a sysfs class under /sys/class/firmware/
-# These days no one enables CONFIG_FW_LOADER_USER_HELPER so check for that
-# as an indicator for CONFIG_FW_LOADER_USER_HELPER.
-HAS_FW_LOADER_USER_HELPER=$(if [ -d /sys/class/firmware/ ]; then echo yes; else echo no; fi)
+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)
 
 if [ "$HAS_FW_LOADER_USER_HELPER" = "yes" ]; then
--

2.16.2

The firmware fallback code is optional. Split that code out to help distinguish the fallback functionlity from othere core firmware loader features. This should make it easier to maintain and review code changes.

The reason for keeping the configuration onto a table which is built-in if you enable firmware loading is so that we can later enable the kernel after subsequent patches to tweak this configuration, even if the firmware loader is modular.

This introduces no functional changes.

Signed-off-by Luis R. Rodriguez <[email protected]>

---
 drivers/base/Makefile                  |   4 +-
 drivers/base/firmware_fallback.c       | 661 +++++++++++++++++++++++++++
 drivers/base/firmware_fallback.h       |  61 +++
 drivers/base/firmware_fallback_table.c |  29 ++
 drivers/base/firmware_loader.c         | 803 +--------------------------------
 drivers/base/firmware_loader.h         | 115 +++++
 6 files changed, 874 insertions(+), 799 deletions(-)
 create mode 100644 drivers/base/firmware_fallback.c
 create mode 100644 drivers/base/firmware_fallback.h
 create mode 100644 drivers/base/firmware_fallback_table.c
 create mode 100644 drivers/base/firmware_loader.h

Both fw_fallback.sh and fw_filesystem.sh share a common set of boiler plate setup and tests. We can share these in a common place. While at it, move both test to use /bin/bash.

Signed-off-by Luis R. Rodriguez <[email protected]>

---
 tools/testing/selftests/firmware/fw_fallback.sh   |  69 ++-----------
 tools/testing/selftests/firmware/fw_filesystem.sh |  61 ++---------
 tools/testing/selftests/firmware/fw_lib.sh        | 120 ++++++++++++++++++++++

 3 files changed, 137 insertions(+), 113 deletions(-)
 create mode 100755 tools/testing/selftests/firmware/fw_lib.sh

tools/testing/selftests/firmware/fw_fallback.sh +9 -60

index 40b6c1d3e832..0db1c09a6bcc 100755
@@ -1,4 +1,4 @@
-#!/bin/sh
+#!/bin/bash
 # SPDX-License-Identifier: GPL-2.0
 # This validates that the kernel will fall back to using the fallback mechanism
 # to load firmware it can't find on disk itself. We must request a firmware

@@ -6,68 +6,17 @@
 # won't find so that we can do the load ourself manually.
 set -e
 
-PROC_CONFIG="/proc/config.gz"
+TEST_REQS_FW_SYSFS_FALLBACK="yes"
+TEST_REQS_FW_SET_CUSTOM_PATH="no"
 TEST_DIR=$(dirname $0)
+source $TEST_DIR/fw_lib.sh
 
-modprobe test_firmware
-if [ ! -f $PROC_CONFIG ]; then
-       if modprobe configs 2>/dev/null; then
-               echo "Loaded configs module"
-               if [ ! -f $PROC_CONFIG ]; then
-                       echo "You must have the following enabled in your kernel:" >&2
-                       cat $TEST_DIR/config >&2
-                       echo "Resorting to old heuristics" >&2
-               fi
-       else
-               echo "Failed to load configs module, using old heuristics" >&2
-       fi
-fi
-
-kconfig_has()
-{
-       if [ -f $PROC_CONFIG ]; then
-               if zgrep -q $1 $PROC_CONFIG 2>/dev/null; then
-                       echo "yes"
-               else
-                       echo "no"
-               fi
-       else
-               # We currently don't have easy heuristics to infer this
-               # so best we can do is just try to use the kernel assuming
-               # you had enabled it. This matches the old behaviour.
-               if [ "$1" = "CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y" ]; then
-                       echo "yes"
-               elif [ "$1" = "CONFIG_FW_LOADER_USER_HELPER=y" ]; then
-                       if [ -d /sys/class/firmware/ ]; then
-                               echo yes
-                       else
-                               echo no
-                       fi
-               fi
-       fi
-}
-
-DIR=/sys/devices/virtual/misc/test_firmware
-
-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)
-
-if [ "$HAS_FW_LOADER_USER_HELPER" = "yes" ]; then
-       OLD_TIMEOUT=$(cat /sys/class/firmware/timeout)
-else
-       echo "usermode helper disabled so ignoring test"
-       exit 0
-fi
-
-FWPATH=$(mktemp -d)
-FW="$FWPATH/test-firmware.bin"
+check_mods
+check_setup
+verify_reqs
+setup_tmp_file
 
-test_finish()
-{
-       echo "$OLD_TIMEOUT" >/sys/class/firmware/timeout
-       rm -f "$FW"
-       rmdir "$FWPATH"
-}
+trap "test_finish" EXIT
 
 load_fw()
 {

tools/testing/selftests/firmware/fw_filesystem.sh +8 -53

index f9508e1a4058..7f47877fa7fa 100755
@@ -1,4 +1,4 @@
-#!/bin/sh
+#!/bin/bash
 # SPDX-License-Identifier: GPL-2.0
 # This validates that the kernel will load firmware out of its list of
 # firmware locations on disk. Since the user helper does similar work,

@@ -6,52 +6,15 @@
 # know so we can be sure we're not accidentally testing the user helper.
 set -e
 
-DIR=/sys/devices/virtual/misc/test_firmware
+TEST_REQS_FW_SYSFS_FALLBACK="no"
+TEST_REQS_FW_SET_CUSTOM_PATH="yes"
 TEST_DIR=$(dirname $0)
+source $TEST_DIR/fw_lib.sh
 
-test_modprobe()
-{
-       if [ ! -d $DIR ]; then
-               echo "$0: $DIR not present"
-               echo "You must have the following enabled in your kernel:"
-               cat $TEST_DIR/config
-               exit 1
-       fi
-}
-
-trap "test_modprobe" EXIT
-
-if [ ! -d $DIR ]; then
-       modprobe test_firmware
-fi
-
-# CONFIG_FW_LOADER_USER_HELPER has a sysfs class under /sys/class/firmware/
-# These days most distros enable CONFIG_FW_LOADER_USER_HELPER but disable
-# CONFIG_FW_LOADER_USER_HELPER_FALLBACK. We use /sys/class/firmware/ as an
-# indicator for CONFIG_FW_LOADER_USER_HELPER.
-HAS_FW_LOADER_USER_HELPER=$(if [ -d /sys/class/firmware/ ]; then echo yes; else echo no; fi)
-
-if [ "$HAS_FW_LOADER_USER_HELPER" = "yes" ]; then
-       OLD_TIMEOUT=$(cat /sys/class/firmware/timeout)
-fi
-
-OLD_FWPATH=$(cat /sys/module/firmware_class/parameters/path)
-
-FWPATH=$(mktemp -d)
-FW="$FWPATH/test-firmware.bin"
-
-test_finish()
-{
-       if [ "$HAS_FW_LOADER_USER_HELPER" = "yes" ]; then
-               echo "$OLD_TIMEOUT" >/sys/class/firmware/timeout
-       fi
-       if [ "$OLD_FWPATH" = "" ]; then
-               OLD_FWPATH=" "
-       fi
-       echo -n "$OLD_FWPATH" >/sys/module/firmware_class/parameters/path
-       rm -f "$FW"
-       rmdir "$FWPATH"
-}
+check_mods
+check_setup
+verify_reqs
+setup_tmp_file
 
 trap "test_finish" EXIT
 

@@ -60,14 +23,6 @@ if [ "$HAS_FW_LOADER_USER_HELPER" = "yes" ]; then
        echo 1 >/sys/class/firmware/timeout
 fi
 
-# Set the kernel search path.
-echo -n "$FWPATH" >/sys/module/firmware_class/parameters/path
-
-# This is an unlikely real-world firmware content. :)
-echo "ABCD0123" >"$FW"
-
-NAME=$(basename "$FW")
-
 if printf '\000' >"$DIR"/trigger_request 2> /dev/null; then
        echo "$0: empty filename should not succeed" >&2
        exit 1

tools/testing/selftests/firmware/fw_lib.sh new file +120 -1

new file mode 100755
index 000000000000..0702dbf0f06b
@@ -0,0 +1,120 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0
+
+# Library of helpers for test scripts.
+set -e
+
+DIR=/sys/devices/virtual/misc/test_firmware
+
+PROC_CONFIG="/proc/config.gz"
+TEST_DIR=$(dirname $0)
+
+print_reqs_exit()
+{
+       echo "You must have the following enabled in your kernel:" >&2
+       cat $TEST_DIR/config >&2
+       exit 1
+}
+
+test_modprobe()
+{
+       if [ ! -d $DIR ]; then
+               print_reqs_exit
+       fi
+}
+
+check_mods()
+{
+       trap "test_modprobe" EXIT
+       if [ ! -d $DIR ]; then
+               modprobe test_firmware
+       fi
+       if [ ! -f $PROC_CONFIG ]; then
+               if modprobe configs 2>/dev/null; then
+                       echo "Loaded configs module"
+                       if [ ! -f $PROC_CONFIG ]; then
+                               echo "You must have the following enabled in your kernel:" >&2
+                               cat $TEST_DIR/config >&2
+                               echo "Resorting to old heuristics" >&2
+                       fi
+               else
+                       echo "Failed to load configs module, using old heuristics" >&2
+               fi
+       fi
+}
+
+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)
+
+       if [ "$HAS_FW_LOADER_USER_HELPER" = "yes" ]; then
+              OLD_TIMEOUT=$(cat /sys/class/firmware/timeout)
+       fi
+
+       OLD_FWPATH=$(cat /sys/module/firmware_class/parameters/path)
+}
+
+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 1
+               fi
+       fi
+}
+
+setup_tmp_file()
+{
+       FWPATH=$(mktemp -d)
+       FW="$FWPATH/test-firmware.bin"
+       echo "ABCD0123" >"$FW"
+       NAME=$(basename "$FW")
+       if [ "$TEST_REQS_FW_SET_CUSTOM_PATH" = "yes" ]; then
+               echo -n "$FWPATH" >/sys/module/firmware_class/parameters/path
+       fi
+}
+
+test_finish()
+{
+       if [ "$HAS_FW_LOADER_USER_HELPER" = "yes" ]; then
+               echo "$OLD_TIMEOUT" >/sys/class/firmware/timeout
+       fi
+       if [ "$OLD_FWPATH" = "" ]; then
+               OLD_FWPATH=" "
+       fi
+       if [ "$TEST_REQS_FW_SET_CUSTOM_PATH" = "yes" ]; then
+               echo -n "$OLD_FWPATH" >/sys/module/firmware_class/parameters/path
+       fi
+       if [ -f $FW ]; then
+               rm -f "$FW"
+       fi
+       if [ -d $FWPATH ]; then
+               rm -rf "$FWPATH"
+       fi
+}
+
+kconfig_has()
+{
+       if [ -f $PROC_CONFIG ]; then
+               if zgrep -q $1 $PROC_CONFIG 2>/dev/null; then
+                       echo "yes"
+               else
+                       echo "no"
+               fi
+       else
+               # We currently don't have easy heuristics to infer this
+               # so best we can do is just try to use the kernel assuming
+               # you had enabled it. This matches the old behaviour.
+               if [ "$1" = "CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y" ]; then
+                       echo "yes"
+               elif [ "$1" = "CONFIG_FW_LOADER_USER_HELPER=y" ]; then
+                       if [ -d /sys/class/firmware/ ]; then
+                               echo yes
+                       else
+                               echo no
+                       fi
+               fi
+       fi
+}
--

2.16.2

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 <[email protected]>

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

tools/testing/selftests/firmware/Makefile +1 -1

index 1894d625af2d..826f38d5dd19 100644
@@ -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
 

tools/testing/selftests/firmware/fw_lib.sh +53 -0

index 0702dbf0f06b..3362a2aac40e 100755
@@ -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()

tools/testing/selftests/firmware/fw_run_tests.sh new file +67 -1

new file mode 100755
index 000000000000..a12b5809ad8b
@@ -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

All CONFIG_FW_LOADER_USER_HELPER_FALLBACK really is, is just a bool, initailized at build time. Define it as such. This simplifies the logic even further, removing now all explicit #ifdefs around the code.

Signed-off-by Luis R. Rodriguez <[email protected]>

---
 drivers/base/firmware_loader.c | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

drivers/base/firmware_loader.c +18 -8

index 7dd36ace6152..59dba794ce1a 100644
@@ -266,6 +266,22 @@ static inline bool fw_state_is_aborted(struct fw_priv *fw_priv)
 
 #ifdef CONFIG_FW_LOADER_USER_HELPER
 
+/**
+ * struct firmware_fallback_config - firmware fallback configuratioon settings
+ *
+ * Helps describe and fine tune the fallback mechanism.
+ *
+ * @force_sysfs_fallback: force the sysfs fallback mechanism to be used
+ *     as if one had enabled CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y.
+ */
+struct firmware_fallback_config {
+       bool force_sysfs_fallback;
+};
+
+static const struct firmware_fallback_config fw_fallback_config = {
+       .force_sysfs_fallback = IS_ENABLED(CONFIG_FW_LOADER_USER_HELPER_FALLBACK),
+};
+
 static inline bool fw_sysfs_done(struct fw_priv *fw_priv)
 {
        return __fw_state_check(fw_priv, FW_STATUS_DONE);

@@ -1151,19 +1167,14 @@ static int fw_load_from_user_helper(struct firmware *firmware,
        return ret;
 }
 
-#ifdef CONFIG_FW_LOADER_USER_HELPER_FALLBACK
-static bool fw_force_sysfs_fallback(unsigned int opt_flags)
-{
-       return true;
-}
-#else
 static bool fw_force_sysfs_fallback(unsigned int opt_flags)
 {
+       if (fw_fallback_config.force_sysfs_fallback)
+               return true;
        if (!(opt_flags & FW_OPT_USERHELPER))
                return false;
        return true;
 }
-#endif
 
 static bool fw_run_sysfs_fallback(unsigned int opt_flags)
 {
--

2.16.2

We only use the timeout for the firmware fallback mechanism except for trying to set the timeout during the cache setup for resume/suspend. For those cases, setting the timeout should be a no-op, so just reflect this in code by adding helpers for it.

This change introduces no functional changes.

Signed-off-by Luis R. Rodriguez <[email protected]>

---
 drivers/base/firmware_loader.c | 49 ++++++++++++++++++++++++++----------------
 1 file changed, 30 insertions(+), 19 deletions(-)

drivers/base/firmware_loader.c +30 -20

index 59dba794ce1a..2d819875348d 100644
@@ -191,13 +191,6 @@ static inline bool fw_is_builtin_firmware(const struct firmware *fw)
 }
 #endif
 
-static int loading_timeout = 60;       /* In seconds */
-
-static inline long firmware_loading_timeout(void)
-{
-       return loading_timeout > 0 ? loading_timeout * HZ : MAX_JIFFY_OFFSET;
-}
-
 static void fw_state_init(struct fw_priv *fw_priv)
 {
        struct fw_state *fw_st = &fw_priv->fw_st;

@@ -282,6 +275,32 @@ static const struct firmware_fallback_config fw_fallback_config = {
        .force_sysfs_fallback = IS_ENABLED(CONFIG_FW_LOADER_USER_HELPER_FALLBACK),
 };
 
+static int old_timeout;
+static int loading_timeout = 60;       /* In seconds */
+
+static inline long firmware_loading_timeout(void)
+{
+       return loading_timeout > 0 ? loading_timeout * HZ : MAX_JIFFY_OFFSET;
+}
+
+/*
+ * use small loading timeout for caching devices' firmware because all these
+ * firmware images have been loaded successfully at lease once, also system is
+ * ready for completing firmware loading now. The maximum size of firmware in
+ * current distributions is about 2M bytes, so 10 secs should be enough.
+ */
+static void fw_fallback_set_cache_timeout(void)
+{
+       old_timeout = loading_timeout;
+       loading_timeout = 10;
+}
+
+/* Restores the timeout to the value last configured during normal operation */
+static void fw_fallback_set_default_timeout(void)
+{
+       loading_timeout =  old_timeout;
+}
+
 static inline bool fw_sysfs_done(struct fw_priv *fw_priv)
 {
        return __fw_state_check(fw_priv, FW_STATUS_DONE);

@@ -1206,6 +1225,8 @@ static int fw_sysfs_fallback(struct firmware *fw, const char *name,
 }
 
 static inline void kill_pending_fw_fallback_reqs(bool only_kill_custom) { }
+static inline void fw_fallback_set_cache_timeout(void) { }
+static inline void fw_fallback_set_default_timeout(void) { }
 
 static inline int register_sysfs_loader(void)
 {

@@ -1752,7 +1773,6 @@ static void __device_uncache_fw_images(void)
 static void device_cache_fw_images(void)
 {
        struct firmware_cache *fwc = &fw_cache;
-       int old_timeout;
        DEFINE_WAIT(wait);
 
        pr_debug("%s\n", __func__);

@@ -1760,16 +1780,7 @@ static void device_cache_fw_images(void)
        /* cancel uncache work */
        cancel_delayed_work_sync(&fwc->work);
 
-       /*
-        * use small loading timeout for caching devices' firmware
-        * because all these firmware images have been loaded
-        * successfully at lease once, also system is ready for
-        * completing firmware loading now. The maximum size of
-        * firmware in current distributions is about 2M bytes,
-        * so 10 secs should be enough.
-        */
-       old_timeout = loading_timeout;
-       loading_timeout = 10;
+       fw_fallback_set_cache_timeout();
 
        mutex_lock(&fw_lock);
        fwc->state = FW_LOADER_START_CACHE;

@@ -1779,7 +1790,7 @@ static void device_cache_fw_images(void)
        /* wait for completion of caching firmware for all devices */
        async_synchronize_full_domain(&fw_cache_domain);
 
-       loading_timeout = old_timeout;
+       fw_fallback_set_default_timeout();
 }
 
 /**
--

2.16.2

The timeout is a fallback construct, so we can just stuff the timeout configuration under struct firmware_fallback_config.

While at it, add a few helpers which vets the use of getting or setting the timeout as an int. The main use of the timeout is to set a timeout for completion, and that is used as an unsigned long. There a few cases however where it makes sense to get or set the timeout as an int, the helpers annotate these use cases have been properly vetted for.

Signed-off-by Luis R. Rodriguez <[email protected]>

---
 drivers/base/firmware_loader.c | 46 ++++++++++++++++++++++++++++++------------
 1 file changed, 33 insertions(+), 13 deletions(-)

drivers/base/firmware_loader.c +33 -14

index 2d819875348d..9757f9fff01e 100644
@@ -266,21 +266,38 @@ static inline bool fw_state_is_aborted(struct fw_priv *fw_priv)
  *
  * @force_sysfs_fallback: force the sysfs fallback mechanism to be used
  *     as if one had enabled CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y.
+ * @old_timeout: for internal use
+ * @loading_timeout: the timeout to wait for the fallback mechanism before
+ *     giving up, in seconds.
  */
 struct firmware_fallback_config {
-       bool force_sysfs_fallback;
+       const bool force_sysfs_fallback;
+       int old_timeout;
+       int loading_timeout;
 };
 
-static const struct firmware_fallback_config fw_fallback_config = {
+static struct firmware_fallback_config fw_fallback_config = {
        .force_sysfs_fallback = IS_ENABLED(CONFIG_FW_LOADER_USER_HELPER_FALLBACK),
+       .loading_timeout = 60,
+       .old_timeout = 60,
 };
 
-static int old_timeout;
-static int loading_timeout = 60;       /* In seconds */
+/* These getters are vetted to use int properly */
+static inline int __firmware_loading_timeout(void)
+{
+       return fw_fallback_config.loading_timeout;
+}
+
+/* These setters are vetted to use int properly */
+static void __fw_fallback_set_timeout(int timeout)
+{
+       fw_fallback_config.loading_timeout = timeout;
+}
 
 static inline long firmware_loading_timeout(void)
 {
-       return loading_timeout > 0 ? loading_timeout * HZ : MAX_JIFFY_OFFSET;
+       return __firmware_loading_timeout() > 0 ?
+               __firmware_loading_timeout() * HZ : MAX_JIFFY_OFFSET;
 }
 
 /*

@@ -291,14 +308,14 @@ static inline long firmware_loading_timeout(void)
  */
 static void fw_fallback_set_cache_timeout(void)
 {
-       old_timeout = loading_timeout;
-       loading_timeout = 10;
+       fw_fallback_config.old_timeout = __firmware_loading_timeout();
+       __fw_fallback_set_timeout(10);
 }
 
 /* Restores the timeout to the value last configured during normal operation */
 static void fw_fallback_set_default_timeout(void)
 {
-       loading_timeout =  old_timeout;
+       __fw_fallback_set_timeout(fw_fallback_config.old_timeout);
 }
 
 static inline bool fw_sysfs_done(struct fw_priv *fw_priv)

@@ -677,7 +694,7 @@ static void kill_pending_fw_fallback_reqs(bool only_kill_custom)
 static ssize_t timeout_show(struct class *class, struct class_attribute *attr,
                            char *buf)
 {
-       return sprintf(buf, "%d\n", loading_timeout);
+       return sprintf(buf, "%d\n", __firmware_loading_timeout());
 }
 
 /**

@@ -696,9 +713,12 @@ static ssize_t timeout_show(struct class *class, struct class_attribute *attr,
 static ssize_t timeout_store(struct class *class, struct class_attribute *attr,
                             const char *buf, size_t count)
 {
-       loading_timeout = simple_strtol(buf, NULL, 10);
-       if (loading_timeout < 0)
-               loading_timeout = 0;
+       int tmp_loading_timeout = simple_strtol(buf, NULL, 10);
+
+       if (tmp_loading_timeout < 0)
+               tmp_loading_timeout = 0;
+
+       __fw_fallback_set_timeout(tmp_loading_timeout);
 
        return count;
 }

@@ -721,7 +741,7 @@ static int do_firmware_uevent(struct fw_sysfs *fw_sysfs, struct kobj_uevent_env
 {
        if (add_uevent_var(env, "FIRMWARE=%s", fw_sysfs->fw_priv->fw_name))
                return -ENOMEM;
-       if (add_uevent_var(env, "TIMEOUT=%i", loading_timeout))
+       if (add_uevent_var(env, "TIMEOUT=%i", __firmware_loading_timeout()))
                return -ENOMEM;
        if (add_uevent_var(env, "ASYNC=%d", fw_sysfs->nowait))
                return -ENOMEM;
--

2.16.2

Currently one requires to test four kernel configurations to test the firmware API completely:

0)
CONFIG_FW_LOADER=y

1)
o CONFIG_FW_LOADER=y
o CONFIG_FW_LOADER_USER_HELPER=y

2)
o CONFIG_FW_LOADER=y
o CONFIG_FW_LOADER_USER_HELPER=y
o CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y

3) When CONFIG_FW_LOADER=m the built-in stuff is disabled, we have no current tests for this.

We can reduce the requirements to three kernel configurations by making fw_config.force_sysfs_fallback a proc knob we flip on off. For kernels that disable CONFIG_IKCONFIG_PROC this can also enable one to inspect if CONFIG_FW_LOADER_USER_HELPER_FALLBACK was enabled at build time by checking the proc value at boot time.

Signed-off-by Luis R. Rodriguez <[email protected]>

---
 drivers/base/firmware_fallback.c       |  1 +
 drivers/base/firmware_fallback.h       |  4 +++-
 drivers/base/firmware_fallback_table.c | 17 +++++++++++++++++
 kernel/sysctl.c                        | 11 +++++++++++
 4 files changed, 32 insertions(+), 1 deletion(-)

drivers/base/firmware_fallback.c +1 -0

index 47690207e0ee..cbce9a950cd8 100644
@@ -7,6 +7,7 @@
 #include <linux/security.h>
 #include <linux/highmem.h>
 #include <linux/umh.h>
+#include <linux/sysctl.h>
 
 #include "firmware_fallback.h"
 #include "firmware_loader.h"

drivers/base/firmware_fallback.h +3 -1

index 550498c7fa4c..ca7e69a8417b 100644
@@ -12,12 +12,14 @@
  *
  * @force_sysfs_fallback: force the sysfs fallback mechanism to be used
  *     as if one had enabled CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y.
+ *     Useful to help debug a CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y
+ *     functionality on a kernel where that config entry has been disabled.
  * @old_timeout: for internal use
  * @loading_timeout: the timeout to wait for the fallback mechanism before
  *     giving up, in seconds.
  */
 struct firmware_fallback_config {
-       const bool force_sysfs_fallback;
+       unsigned int force_sysfs_fallback;
        int old_timeout;
        int loading_timeout;
 };

drivers/base/firmware_fallback_table.c +17 -0

index 53cc4e492520..77300d5e9c52 100644
@@ -19,6 +19,9 @@
 /* Module or buit-in */
 #ifdef CONFIG_FW_LOADER_USER_HELPER
 
+static unsigned int zero;
+static unsigned int one = 1;
+
 struct firmware_fallback_config fw_fallback_config = {
        .force_sysfs_fallback = IS_ENABLED(CONFIG_FW_LOADER_USER_HELPER_FALLBACK),
        .loading_timeout = 60,

@@ -26,4 +29,18 @@ struct firmware_fallback_config fw_fallback_config = {
 };
 EXPORT_SYMBOL_GPL(fw_fallback_config);
 
+struct ctl_table firmware_config_table[] = {
+       {
+               .procname       = "force_sysfs_fallback",
+               .data           = &fw_fallback_config.force_sysfs_fallback,
+               .maxlen         = sizeof(unsigned int),
+               .mode           = 0644,
+               .proc_handler   = proc_douintvec_minmax,
+               .extra1         = &zero,
+               .extra2         = &one,
+       },
+       { }
+};
+EXPORT_SYMBOL_GPL(firmware_config_table);
+
 #endif

kernel/sysctl.c +11 -1

index acdf4e85c0a1..aa8355953fcf 100644
@@ -253,6 +253,10 @@ extern struct ctl_table random_table[];
 extern struct ctl_table epoll_table[];
 #endif
 
+#ifdef CONFIG_FW_LOADER_USER_HELPER
+extern struct ctl_table firmware_config_table[];
+#endif
+
 #ifdef HAVE_ARCH_PICK_MMAP_LAYOUT
 int sysctl_legacy_va_layout;
 #endif

@@ -748,6 +752,13 @@ static struct ctl_table kern_table[] = {
                .mode           = 0555,
                .child          = usermodehelper_table,
        },
+#ifdef CONFIG_FW_LOADER_USER_HELPER
+       {
+               .procname       = "firmware_config",
+               .mode           = 0555,
+               .child          = firmware_config_table,
+       },
+#endif
        {
                .procname       = "overflowuid",
                .data           = &overflowuid,
--

2.16.2

You currently need four different kernel builds to test the firmware API fully. By adding a proc knob to force disable the fallback mechanism completely we are able to reduce the amount of kernels you need built to test the firmware API down to two.

Signed-off-by Luis R. Rodriguez <[email protected]>

---
 drivers/base/firmware_fallback.c       | 5 +++++
 drivers/base/firmware_fallback.h       | 4 ++++
 drivers/base/firmware_fallback_table.c | 9 +++++++++
 3 files changed, 18 insertions(+)

drivers/base/firmware_fallback.c +5 -0

index cbce9a950cd8..13fa5ff2b46c 100644
@@ -643,6 +643,11 @@ static bool fw_force_sysfs_fallback(unsigned int opt_flags)
 
 static bool fw_run_sysfs_fallback(unsigned int opt_flags)
 {
+       if (fw_fallback_config.ignore_sysfs_fallback) {
+               pr_info_once("Ignoring firmware sysfs fallback due to debugfs knob\n");
+               return false;
+       }
+
        if ((opt_flags & FW_OPT_NOFALLBACK))
                return false;
 

drivers/base/firmware_fallback.h +4 -0

index ca7e69a8417b..dfebc644ed35 100644
@@ -14,12 +14,16 @@
  *     as if one had enabled CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y.
  *     Useful to help debug a CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y
  *     functionality on a kernel where that config entry has been disabled.
+ * @ignore_sysfs_fallback: force to disable the sysfs fallback mechanism.
+ *     This emulates the behaviour as if we had set the kernel
+ *     config CONFIG_FW_LOADER_USER_HELPER=n.
  * @old_timeout: for internal use
  * @loading_timeout: the timeout to wait for the fallback mechanism before
  *     giving up, in seconds.
  */
 struct firmware_fallback_config {
        unsigned int force_sysfs_fallback;
+       unsigned int ignore_sysfs_fallback;
        int old_timeout;
        int loading_timeout;
 };

drivers/base/firmware_fallback_table.c +9 -1

index 77300d5e9c52..5e990b0330c7 100644
@@ -39,6 +39,15 @@ struct ctl_table firmware_config_table[] = {
                .extra1         = &zero,
                .extra2         = &one,
        },
+       {
+               .procname       = "ignore_sysfs_fallback",
+               .data           = &fw_fallback_config.ignore_sysfs_fallback,
+               .maxlen         = sizeof(unsigned int),
+               .mode           = 0644,
+               .proc_handler   = proc_douintvec_minmax,
+               .extra1         = &zero,
+               .extra2         = &one,
+       },
        { }
 };
 EXPORT_SYMBOL_GPL(firmware_config_table);
--

2.16.2

Quoted 13 lines · Luis R. Rodriguez

The firmware fallback code is optional. Split that code out to help distinguish the fallback functionlity from othere core firmware loader features. This should make it easier to maintain and review code changes.

The reason for keeping the configuration onto a table which is built-in if you enable firmware loading is so that we can later enable the kernel after subsequent patches to tweak this configuration, even if the firmware loader is modular.

This introduces no functional changes.

Signed-off-by Luis R. Rodriguez <[email protected]>

Quoted 12 lines

---
 drivers/base/Makefile                  |   4 +-
 drivers/base/firmware_fallback.c       | 661 +++++++++++++++++++++++++++
 drivers/base/firmware_fallback.h       |  61 +++
 drivers/base/firmware_fallback_table.c |  29 ++
 drivers/base/firmware_loader.c         | 803 +--------------------------------
 drivers/base/firmware_loader.h         | 115 +++++
 6 files changed, 874 insertions(+), 799 deletions(-)
 create mode 100644 drivers/base/firmware_fallback.c
 create mode 100644 drivers/base/firmware_fallback.h
 create mode 100644 drivers/base/firmware_fallback_table.c
 create mode 100644 drivers/base/firmware_loader.h

Does it make sense to have a separate subdirectory for firmware instead? I did this _ stuff with lkdtm and have regretted it. (I'm likely going to make a subdirectory for it this cycle...)

Quoted 22 lines · Luis R. Rodriguez

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.

Quoted 1 line

Signed-off-by Luis R. Rodriguez <[email protected]>

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

Acked-by Kees Cook <[email protected]>

Quoted 27 lines · Kees Cook

Quoted 25 lines · Luis R. Rodriguez

The firmware fallback code is optional. Split that code out to help distinguish the fallback functionlity from othere core firmware loader features. This should make it easier to maintain and review code changes.

The reason for keeping the configuration onto a table which is built-in if you enable firmware loading is so that we can later enable the kernel after subsequent patches to tweak this configuration, even if the firmware loader is modular.

This introduces no functional changes.

Signed-off-by Luis R. Rodriguez <[email protected]>

---
 drivers/base/Makefile                  |   4 +-
 drivers/base/firmware_fallback.c       | 661 +++++++++++++++++++++++++++
 drivers/base/firmware_fallback.h       |  61 +++
 drivers/base/firmware_fallback_table.c |  29 ++
 drivers/base/firmware_loader.c         | 803 +--------------------------------
 drivers/base/firmware_loader.h         | 115 +++++
 6 files changed, 874 insertions(+), 799 deletions(-)
 create mode 100644 drivers/base/firmware_fallback.c
 create mode 100644 drivers/base/firmware_fallback.h
 create mode 100644 drivers/base/firmware_fallback_table.c
 create mode 100644 drivers/base/firmware_loader.h

Quoted 3 lines

Does it make sense to have a separate subdirectory for firmware instead? I did this _ stuff with lkdtm and have regretted it. (I'm likely going to make a subdirectory for it this cycle...)

Sure, the only eyesore is that drivers/base/firmware.c what is that for?

drivers/base/firmware_loader/ ok?

Quoted 32 lines · Luis R. Rodriguez

Quoted 30 lines · Kees Cook

Quoted 13 lines · Luis R. Rodriguez

The firmware fallback code is optional. Split that code out to help distinguish the fallback functionlity from othere core firmware loader features. This should make it easier to maintain and review code changes.

The reason for keeping the configuration onto a table which is built-in if you enable firmware loading is so that we can later enable the kernel after subsequent patches to tweak this configuration, even if the firmware loader is modular.

This introduces no functional changes.

Signed-off-by Luis R. Rodriguez <[email protected]>

Quoted 12 lines

---
 drivers/base/Makefile                  |   4 +-
 drivers/base/firmware_fallback.c       | 661 +++++++++++++++++++++++++++
 drivers/base/firmware_fallback.h       |  61 +++
 drivers/base/firmware_fallback_table.c |  29 ++
 drivers/base/firmware_loader.c         | 803 +--------------------------------
 drivers/base/firmware_loader.h         | 115 +++++
 6 files changed, 874 insertions(+), 799 deletions(-)
 create mode 100644 drivers/base/firmware_fallback.c
 create mode 100644 drivers/base/firmware_fallback.h
 create mode 100644 drivers/base/firmware_fallback_table.c
 create mode 100644 drivers/base/firmware_loader.h

Does it make sense to have a separate subdirectory for firmware instead? I did this _ stuff with lkdtm and have regretted it. (I'm likely going to make a subdirectory for it this cycle...)

Quoted 3 lines

Sure, the only eyesore is that drivers/base/firmware.c what is that for?

drivers/base/firmware_loader/ ok?

Yeah? Seems fine to me. Greg, do you have thoughts on this?

Quoted 25 lines · Kees Cook

Quoted 23 lines · Luis R. Rodriguez

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 <[email protected]>

Quoted 1 line

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.

Quoted 32 lines · Luis R. Rodriguez

Quoted 23 lines · Kees Cook

Quoted 22 lines · Luis R. Rodriguez

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.

Quoted 3 lines

Quoted 1 line

Signed-off-by Luis R. Rodriguez <[email protected]>

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.

Quoted 2 lines

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.

Quoted 33 lines · Josh Triplett

Quoted 32 lines · Luis R. Rodriguez

Quoted 23 lines · Kees Cook

Quoted 22 lines · Luis R. Rodriguez

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.

Quoted 3 lines

Quoted 1 line

Signed-off-by Luis R. Rodriguez <[email protected]>

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.

Quoted 8 lines

Quoted 2 lines

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:

Quoted 2 lines · Luis R. Rodriguez

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?

Quoted 8 lines · Josh Triplett

Quoted 2 lines · Luis R. Rodriguez

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

Quoted 3 lines

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.

Quoted 1 line

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?

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?

Quoted 13 lines · Luis R. Rodriguez

Quoted 8 lines · Josh Triplett

Quoted 2 lines · Luis R. Rodriguez

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

Quoted 6 lines

Quoted 3 lines

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.

Quoted 6 lines

Quoted 1 line

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.

Quoted 6 lines

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*

Quoted 33 lines · Kees Cook

Quoted 32 lines · Luis R. Rodriguez

Quoted 30 lines · Kees Cook

Quoted 13 lines · Luis R. Rodriguez

The firmware fallback code is optional. Split that code out to help distinguish the fallback functionlity from othere core firmware loader features. This should make it easier to maintain and review code changes.

The reason for keeping the configuration onto a table which is built-in if you enable firmware loading is so that we can later enable the kernel after subsequent patches to tweak this configuration, even if the firmware loader is modular.

This introduces no functional changes.

Signed-off-by Luis R. Rodriguez <[email protected]>

Quoted 12 lines

---
 drivers/base/Makefile                  |   4 +-
 drivers/base/firmware_fallback.c       | 661 +++++++++++++++++++++++++++
 drivers/base/firmware_fallback.h       |  61 +++
 drivers/base/firmware_fallback_table.c |  29 ++
 drivers/base/firmware_loader.c         | 803 +--------------------------------
 drivers/base/firmware_loader.h         | 115 +++++
 6 files changed, 874 insertions(+), 799 deletions(-)
 create mode 100644 drivers/base/firmware_fallback.c
 create mode 100644 drivers/base/firmware_fallback.h
 create mode 100644 drivers/base/firmware_fallback_table.c
 create mode 100644 drivers/base/firmware_loader.h

Does it make sense to have a separate subdirectory for firmware instead? I did this _ stuff with lkdtm and have regretted it. (I'm likely going to make a subdirectory for it this cycle...)

Quoted 5 lines

Quoted 3 lines

Sure, the only eyesore is that drivers/base/firmware.c what is that for?

drivers/base/firmware_loader/ ok?

Yeah? Seems fine to me. Greg, do you have thoughts on this?

I don't care :)