firmware: development for v4.17

Greg,

Here's a respin of what I have queued up for v4.17 for the firmware API. It combines the cleanup I've been working on and the addition of the new API call request_firmware_cache() for fixing a corner case suspend issue on some type of cards with an optimization in place where the firmware is *not* needed on reboot.

The cleanup work allows us to test the firmware API with one kernel configuration. I've addressed Kees' feedback on this respin and combined the code into drivers/base/firmware_class/.

I've made one new test_firmware change in consideration for one firmware change, the patch "firmware: ensure the firmware cache is not used on incompatible calls" requires us to modify our tests scripts to use the APIs sanely as well.

I've put up these changes on my git tree, refer to the branch "20180307-firmware-dev-for-v4.17" based on linux-next [0] and the same name based on Linus' tree [1].

Questions, feedback, and specially rants are always welcomed.

[0] https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux-next.git/log/?h=20180307-firmware-dev-for-v4.17
[1] https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux.git/log/?h=20180307-firmware-dev-for-v4.17

Luis R. Rodriguez (20):
test_firmware: add simple firmware firmware test library
test_firmware: enable custom fallback testing on limited kernel
configs
test_firmware: replace syfs fallback check with kconfig_has helper
firmware: enable to split firmware_class into separate target files
firmware: simplify CONFIG_FW_LOADER_USER_HELPER_FALLBACK further
firmware: use helpers for setting up a temporary cache timeout
firmware: move loading timeout under struct firmware_fallback_config
firmware: split firmware fallback functionality into its own file
firmware: move firmware loader into its own directory
firmware: enable run time change of forcing fallback loader
firmware: enable to force disable the fallback mechanism at run time
test_firmware: expand on library with shared helpers
test_firmware: test three firmware kernel configs using a proc knob
rename: _request_firmware_load() fw_load_sysfs_fallback()
firmware: fix checking for return values for fw_add_devm_name()
firmware: add helper to check to see if fw cache is setup
test_firmware: modify custom fallback tests to use unique files
firmware: ensure the firmware cache is not used on incompatible calls
firmware: add request_firmware_cache() to help with cache on reboot
mt7601u: use request_firmware_cache() to address cache on reboot

 .../driver-api/firmware/fallback-mechanisms.rst    |   2 +-
 .../driver-api/firmware/request_firmware.rst       |  14 +
 MAINTAINERS                                        |   2 +-
 drivers/base/Makefile                              |   2 +-
 drivers/base/firmware_loader/Makefile              |   7 +
 drivers/base/firmware_loader/fallback.c            | 674 +++++++++++++++++
 drivers/base/firmware_loader/fallback.h            |  67 ++
 drivers/base/firmware_loader/fallback_table.c      |  55 ++
 drivers/base/firmware_loader/firmware.h            | 115 +++
 .../{firmware_class.c => firmware_loader/main.c}   | 833 ++-------------------
 drivers/net/wireless/mediatek/mt7601u/mcu.c        |   2 +-
 include/linux/firmware.h                           |   3 +
 kernel/sysctl.c                                    |  11 +
 tools/testing/selftests/firmware/Makefile          |   2 +-
 tools/testing/selftests/firmware/config            |   4 +
 tools/testing/selftests/firmware/fw_fallback.sh    |  65 +-
 tools/testing/selftests/firmware/fw_filesystem.sh  |  72 +-
 tools/testing/selftests/firmware/fw_lib.sh         | 194 +++++
 tools/testing/selftests/firmware/fw_run_tests.sh   |  70 ++
 19 files changed, 1332 insertions(+), 862 deletions(-)
 create mode 100644 drivers/base/firmware_loader/Makefile
 create mode 100644 drivers/base/firmware_loader/fallback.c
 create mode 100644 drivers/base/firmware_loader/fallback.h
 create mode 100644 drivers/base/firmware_loader/fallback_table.c
 create mode 100644 drivers/base/firmware_loader/firmware.h
 rename drivers/base/{firmware_class.c => firmware_loader/main.c} (60%)
 create mode 100755 tools/testing/selftests/firmware/fw_lib.sh
 create mode 100755 tools/testing/selftests/firmware/fw_run_tests.sh

2.16.2

We'll expland on this later, for now just add basic module checker. While at it, move this all to use /bin/bash as we'll have much more flexibility with it.

---
 tools/testing/selftests/firmware/fw_fallback.sh   |  7 ++--
 tools/testing/selftests/firmware/fw_filesystem.sh | 20 ++---------
 tools/testing/selftests/firmware/fw_lib.sh        | 44 +++++++++++++++++++++++
 3 files changed, 51 insertions(+), 20 deletions(-)
 create mode 100755 tools/testing/selftests/firmware/fw_lib.sh

index 722cad91df74..755147a8c967 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,9 +6,10 @@
 # won't find so that we can do the load ourself manually.
 set -e
 
-modprobe test_firmware
+TEST_DIR=$(dirname $0)
+source $TEST_DIR/fw_lib.sh
 
-DIR=/sys/devices/virtual/misc/test_firmware
+check_mods
 
 # 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

index f9508e1a4058..748f2f5737e9 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,24 +6,10 @@
 # know so we can be sure we're not accidentally testing the user helper.
 set -e
 
-DIR=/sys/devices/virtual/misc/test_firmware
 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
+check_mods
 
 # 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

2.16.2

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.

We stuff the new kconfig_has() helper into our shared library as we'll later expando on its use elsewhere.

---
 tools/testing/selftests/firmware/config         |  4 ++++
 tools/testing/selftests/firmware/fw_fallback.sh |  6 +++++-
 tools/testing/selftests/firmware/fw_lib.sh      | 24 ++++++++++++++++++++++++
 3 files changed, 33 insertions(+), 1 deletion(-)

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

index 755147a8c967..bf850050e5e9 100755
@@ -15,6 +15,7 @@ check_mods
 # 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)

@@ -287,7 +288,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

index c14bbca7ecf9..467567c758b9 100755
@@ -42,3 +42,27 @@ check_mods()
 		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
+}

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.

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

index bf850050e5e9..323c921a2469 100755
@@ -11,10 +11,7 @@ source $TEST_DIR/fw_lib.sh
 
 check_mods
 
-# 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 loader code has grown quite a bit over the years. The practice of stuffing everything we need into one file makes the code hard to follow.

In order to split the firmware loader code into different components we must pick a module name and a first object target file. We must keep the firmware_class name to remain compatible with scripts which have been relying on the sysfs loader path for years, so the old module name stays. We can however rename the C file without affecting the module name.

The firmware_class used to represent the idea that the code was a simple sysfs firmware loader, provided by the struct class firmware_class. The sysfs firmware loader used to be the default, today its only the fallback mechanism.

This only renames the target code then to make emphasis of what the code does these days. With this change new features can also use a new object files.

---
 drivers/base/Makefile                                | 1 +
 drivers/base/{firmware_class.c => firmware_loader.c} | 0
 2 files changed, 1 insertion(+)
 rename drivers/base/{firmware_class.c => firmware_loader.c} (100%)

index e32a52490051..f261143fafbf 100644
@@ -13,6 +13,7 @@ obj-$(CONFIG_HAS_DMA)	+= dma-mapping.o
 obj-$(CONFIG_HAVE_GENERIC_DMA_COHERENT) += dma-coherent.o
 obj-$(CONFIG_ISA_BUS_API)	+= isa.o
 obj-$(CONFIG_FW_LOADER)	+= firmware_class.o
+firmware_class-objs := firmware_loader.o
 obj-$(CONFIG_NUMA)	+= node.o
 obj-$(CONFIG_MEMORY_HOTPLUG_SPARSE) += memory.o
 ifeq ($(CONFIG_SYSFS),y)

similarity index 100%
rename from drivers/base/firmware_class.c
rename to drivers/base/firmware_loader.c

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.

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

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.

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

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.

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

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

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.

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

index f261143fafbf..b946a408256d 100644
@@ -5,7 +5,8 @@ obj-y			:= component.o core.o bus.o dd.o syscore.o \
 			   driver.o class.o platform.o \
 			   cpu.o firmware.o init.o map.o devres.o \
 			   attribute_container.o transport_class.o \
-			   topology.o container.o property.o cacheinfo.o
+			   topology.o container.o property.o cacheinfo.o \
+			   firmware_fallback_table.o
 obj-$(CONFIG_DEVTMPFS)	+= devtmpfs.o
 obj-$(CONFIG_DMA_CMA) += dma-contiguous.o
 obj-y			+= power/

@@ -14,6 +15,7 @@ obj-$(CONFIG_HAVE_GENERIC_DMA_COHERENT) += dma-coherent.o
 obj-$(CONFIG_ISA_BUS_API)	+= isa.o
 obj-$(CONFIG_FW_LOADER)	+= firmware_class.o
 firmware_class-objs := firmware_loader.o
+firmware_class-$(CONFIG_FW_LOADER_USER_HELPER) += firmware_fallback.o
 obj-$(CONFIG_NUMA)	+= node.o
 obj-$(CONFIG_MEMORY_HOTPLUG_SPARSE) += memory.o
 ifeq ($(CONFIG_SYSFS),y)

new file mode 100644
index 000000000000..53cc4e492520
@@ -0,0 +1,29 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/types.h>
+#include <linux/kconfig.h>
+#include <linux/list.h>
+#include <linux/slab.h>
+#include <linux/security.h>
+#include <linux/highmem.h>
+#include <linux/umh.h>
+#include <linux/sysctl.h>
+
+#include "firmware_fallback.h"
+#include "firmware_loader.h"
+
+/*
+ * firmware fallback configuration table
+ */
+
+/* Module or buit-in */
+#ifdef CONFIG_FW_LOADER_USER_HELPER
+
+struct firmware_fallback_config fw_fallback_config = {
+	.force_sysfs_fallback = IS_ENABLED(CONFIG_FW_LOADER_USER_HELPER_FALLBACK),
+	.loading_timeout = 60,
+	.old_timeout = 60,
+};
+EXPORT_SYMBOL_GPL(fw_fallback_config);
+
+#endif

2.16.2

This will make it much easier to manage as we manage to keep trimming componnents down into their own files to more easily manage and maintain this codebase.

---
 MAINTAINERS                                                       | 2 +-
 drivers/base/Makefile                                             | 7 ++-----
 drivers/base/firmware_loader/Makefile                             | 7 +++++++
 drivers/base/{firmware_fallback.c => firmware_loader/fallback.c}  | 4 ++--
 drivers/base/{firmware_fallback.h => firmware_loader/fallback.h}  | 0
 .../fallback_table.c}                                             | 4 ++--
 drivers/base/{firmware_loader.h => firmware_loader/firmware.h}    | 0
 drivers/base/{firmware_loader.c => firmware_loader/main.c}        | 8 ++++----
 8 files changed, 18 insertions(+), 14 deletions(-)
 create mode 100644 drivers/base/firmware_loader/Makefile
 rename drivers/base/{firmware_fallback.c => firmware_loader/fallback.c} (99%)
 rename drivers/base/{firmware_fallback.h => firmware_loader/fallback.h} (100%)
 rename drivers/base/{firmware_fallback_table.c => firmware_loader/fallback_table.c} (90%)
 rename drivers/base/{firmware_loader.h => firmware_loader/firmware.h} (100%)
 rename drivers/base/{firmware_loader.c => firmware_loader/main.c} (99%)

index e03a130902cd..6ddd6f4aaffa 100644
@@ -5589,7 +5589,7 @@ M:	Luis R. Rodriguez <[email protected]>
 L:	[email protected]
 S:	Maintained
 F:	Documentation/firmware_class/
-F:	drivers/base/firmware*.c
+F:	drivers/base/firmware_loader/
 F:	include/linux/firmware.h
 
 FLASH ADAPTER DRIVER (IBM Flash Adapter 900GB Full Height PCI Flash Card)

index b946a408256d..b9539abec675 100644
@@ -5,17 +5,14 @@ obj-y			:= component.o core.o bus.o dd.o syscore.o \
 			   driver.o class.o platform.o \
 			   cpu.o firmware.o init.o map.o devres.o \
 			   attribute_container.o transport_class.o \
-			   topology.o container.o property.o cacheinfo.o \
-			   firmware_fallback_table.o
+			   topology.o container.o property.o cacheinfo.o
 obj-$(CONFIG_DEVTMPFS)	+= devtmpfs.o
 obj-$(CONFIG_DMA_CMA) += dma-contiguous.o
 obj-y			+= power/
 obj-$(CONFIG_HAS_DMA)	+= dma-mapping.o
 obj-$(CONFIG_HAVE_GENERIC_DMA_COHERENT) += dma-coherent.o
 obj-$(CONFIG_ISA_BUS_API)	+= isa.o
-obj-$(CONFIG_FW_LOADER)	+= firmware_class.o
-firmware_class-objs := firmware_loader.o
-firmware_class-$(CONFIG_FW_LOADER_USER_HELPER) += firmware_fallback.o
+obj-y				+= firmware_loader/
 obj-$(CONFIG_NUMA)	+= node.o
 obj-$(CONFIG_MEMORY_HOTPLUG_SPARSE) += memory.o
 ifeq ($(CONFIG_SYSFS),y)

new file mode 100644
index 000000000000..a97eeb0be1d8
@@ -0,0 +1,7 @@
+# SPDX-License-Identifier: GPL-2.0
+# Makefile for the Linux firmware loader
+
+obj-y			:= fallback_table.o
+obj-$(CONFIG_FW_LOADER)	+= firmware_class.o
+firmware_class-objs := main.o
+firmware_class-$(CONFIG_FW_LOADER_USER_HELPER) += fallback.o

similarity index 99%
rename from drivers/base/firmware_fallback.c
rename to drivers/base/firmware_loader/fallback.c
index 47690207e0ee..9b65837256d6 100644
@@ -8,8 +8,8 @@
 #include <linux/highmem.h>
 #include <linux/umh.h>
 
-#include "firmware_fallback.h"
-#include "firmware_loader.h"
+#include "fallback.h"
+#include "firmware.h"
 
 /*
  * firmware fallback mechanism

similarity index 100%
rename from drivers/base/firmware_fallback.h
rename to drivers/base/firmware_loader/fallback.h

similarity index 90%
rename from drivers/base/firmware_fallback_table.c
rename to drivers/base/firmware_loader/fallback_table.c
index 53cc4e492520..981419044c7e 100644
@@ -9,8 +9,8 @@
 #include <linux/umh.h>
 #include <linux/sysctl.h>
 
-#include "firmware_fallback.h"
-#include "firmware_loader.h"
+#include "fallback.h"
+#include "firmware.h"
 
 /*
  * firmware fallback configuration table

similarity index 100%
rename from drivers/base/firmware_loader.h
rename to drivers/base/firmware_loader/firmware.h

similarity index 99%
rename from drivers/base/firmware_loader.c
rename to drivers/base/firmware_loader/main.c
index 21dd31ef08ae..c8966c84bd44 100644
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 /*
- * firmware_class.c - Multi purpose firmware loading support
+ * main.c - Multi purpose firmware loading support
  *
  * Copyright (c) 2003 Manuel Estrada Sainz
  *

@@ -36,9 +36,9 @@
 
 #include <generated/utsrelease.h>
 
-#include "base.h"
-#include "firmware_loader.h"
-#include "firmware_fallback.h"
+#include "../base.h"
+#include "firmware.h"
+#include "fallback.h"
 
 MODULE_AUTHOR("Manuel Estrada Sainz");
 MODULE_DESCRIPTION("Multi purpose firmware loading support");

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.

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

index 9b65837256d6..45cc40933a47 100644
@@ -7,6 +7,7 @@
 #include <linux/security.h>
 #include <linux/highmem.h>
 #include <linux/umh.h>
+#include <linux/sysctl.h>
 
 #include "fallback.h"
 #include "firmware.h"

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

index 981419044c7e..92365e053e30 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

index 6c68e771e11d..7a8aa6866764 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.

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

index 45cc40933a47..d6838e7ec00c 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;
 

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

index 92365e053e30..7428659d8df9 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

This expands our library with as many things we could find which both scripts we use share.

---
 tools/testing/selftests/firmware/fw_fallback.sh   | 31 +++-----------
 tools/testing/selftests/firmware/fw_filesystem.sh | 41 +++---------------
 tools/testing/selftests/firmware/fw_lib.sh        | 52 +++++++++++++++++++++++
 3 files changed, 63 insertions(+), 61 deletions(-)

index 323c921a2469..9337a0328627 100755
@@ -6,30 +6,17 @@
 # won't find so that we can do the load ourself manually.
 set -e
 
+TEST_REQS_FW_SYSFS_FALLBACK="yes"
+TEST_REQS_FW_SET_CUSTOM_PATH="no"
 TEST_DIR=$(dirname $0)
 source $TEST_DIR/fw_lib.sh
 
 check_mods
+check_setup
+verify_reqs
+setup_tmp_file
 
-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"
-
-test_finish()
-{
-	echo "$OLD_TIMEOUT" >/sys/class/firmware/timeout
-	rm -f "$FW"
-	rmdir "$FWPATH"
-}
+trap "test_finish" EXIT
 
 load_fw()
 {

@@ -168,12 +155,6 @@ load_fw_fallback_with_child()
 	return $RET
 }
 
-trap "test_finish" EXIT
-
-# This is an unlikely real-world firmware content. :)
-echo "ABCD0123" >"$FW"
-NAME=$(basename "$FW")
-
 test_syfs_timeout()
 {
 	DEVPATH="$DIR"/"nope-$NAME"/loading

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.

---
 tools/testing/selftests/firmware/Makefile        |  2 +-
 tools/testing/selftests/firmware/fw_lib.sh       | 51 +++++++++++++++++
 tools/testing/selftests/firmware/fw_run_tests.sh | 70 ++++++++++++++++++++++++
 3 files changed, 122 insertions(+), 1 deletion(-)
 create mode 100755 tools/testing/selftests/firmware/fw_run_tests.sh

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
 

2.16.2

This reflects much clearer what is being done.
While at it, kdoc'ify it.

---
 Documentation/driver-api/firmware/fallback-mechanisms.rst |  2 +-
 drivers/base/firmware_loader/fallback.c                   | 13 ++++++++++---
 2 files changed, 11 insertions(+), 4 deletions(-)

index 4055ac76b288..f353783ae0be 100644
@@ -112,7 +112,7 @@ Since a device is created for the sysfs interface to help load firmware as a
 fallback mechanism userspace can be informed of the addition of the device by
 relying on kobject uevents. The addition of the device into the device
 hierarchy means the fallback mechanism for firmware loading has been initiated.
-For details of implementation refer to _request_firmware_load(), in particular
+For details of implementation refer to fw_load_sysfs_fallback(), in particular
 on the use of dev_set_uevent_suppress() and kobject_uevent().
 
 The kernel's kobject uevent mechanism is implemented in lib/kobject_uevent.c,

index d6838e7ec00c..0a8ec7fec585 100644
@@ -535,8 +535,15 @@ fw_create_instance(struct firmware *firmware, const char *fw_name,
 	return fw_sysfs;
 }
 
-/* load a firmware via user helper */
-static int _request_firmware_load(struct fw_sysfs *fw_sysfs,
+/**
+ * fw_load_sysfs_fallback - load a firmware via the syfs fallback mechanism
+ * @fw_sysfs: firmware syfs information for the firmware to load
+ * @opt_flags: flags of options, FW_OPT_*
+ * @timeout: timeout to wait for the load
+ *
+ * In charge of constructing a sysfs fallback interface for firmware loading.
+ **/
+static int fw_load_sysfs_fallback(struct fw_sysfs *fw_sysfs,
 				  unsigned int opt_flags, long timeout)
 {
 	int retval = 0;

@@ -621,7 +628,7 @@ static int fw_load_from_user_helper(struct firmware *firmware,
 	}
 
 	fw_sysfs->fw_priv = firmware->priv;
-	ret = _request_firmware_load(fw_sysfs, opt_flags, timeout);
+	ret = fw_load_sysfs_fallback(fw_sysfs, opt_flags, timeout);
 
 	if (!ret)
 		ret = assign_fw(firmware, device, opt_flags);

2.16.2

Currently fw_add_devm_name() returns 1 if the firmware cache was already set. This makes it complicated for us to check for correctness. It is actually non-fatal if the firmware cache is already setup, so just return 0, and simplify the checkers.

fw_add_devm_name() adds device's name onto the devres for the device so that prior to suspend we cache the firmware onto memory, so that on resume the firmware is reliably available. We never were checking for success for this call though, meaning in some really rare cases we my have never setup the firmware cache for a device, which could in turn make resume fail.

This is all theoretical, no known issues have been reported. This small issue has been present way since the addition of the devres firmware cache names on v3.7.

---
 drivers/base/firmware_loader/main.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

index c8966c84bd44..f5046887e362 100644
@@ -403,7 +403,7 @@ static int fw_add_devm_name(struct device *dev, const char *name)
 
 	fwn = fw_find_devm_name(dev, name);
 	if (fwn)
-		return 1;
+		return 0;
 
 	fwn = devres_alloc(fw_name_devm_release, sizeof(struct fw_name_devm),
 			   GFP_KERNEL);

@@ -431,6 +431,7 @@ int assign_fw(struct firmware *fw, struct device *device,
 	      unsigned int opt_flags)
 {
 	struct fw_priv *fw_priv = fw->priv;
+	int ret;
 
 	mutex_lock(&fw_lock);
 	if (!fw_priv->size || fw_state_is_aborted(fw_priv)) {

@@ -447,8 +448,13 @@ int assign_fw(struct firmware *fw, struct device *device,
 	 */
 	/* don't cache firmware handled without uevent */
 	if (device && (opt_flags & FW_OPT_UEVENT) &&
-	    !(opt_flags & FW_OPT_NOCACHE))
-		fw_add_devm_name(device, fw_priv->fw_name);
+	    !(opt_flags & FW_OPT_NOCACHE)) {
+		ret = fw_add_devm_name(device, fw_priv->fw_name);
+		if (ret) {
+			mutex_unlock(&fw_lock);
+			return ret;
+		}
+	}
 
 	/*
 	 * After caching firmware image is started, let it piggyback

2.16.2

Add a helper to check if the firmware cache is already setup for a device. This will be used later.

---
 drivers/base/firmware_loader/main.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

index f5046887e362..b569d8a09392 100644
@@ -396,13 +396,23 @@ static struct fw_name_devm *fw_find_devm_name(struct device *dev,
 	return fwn;
 }
 
-/* add firmware name into devres list */
-static int fw_add_devm_name(struct device *dev, const char *name)
+static bool fw_cache_is_setup(struct device *dev, const char *name)
 {
 	struct fw_name_devm *fwn;
 
 	fwn = fw_find_devm_name(dev, name);
 	if (fwn)
+		return true;
+
+	return false;
+}
+
+/* add firmware name into devres list */
+static int fw_add_devm_name(struct device *dev, const char *name)
+{
+	struct fw_name_devm *fwn;
+
+	if (fw_cache_is_setup(dev, name))
 		return 0;
 
 	fwn = devres_alloc(fw_name_devm_release, sizeof(struct fw_name_devm),

2.16.2

Users of the custom firmware fallback interface is are not supposed to use the firmware cache interface, this can happen if for instance the one of the APIs which use the firmware cache is used first with one firmware file and then the request_firmware_nowait(uevent=false) API is used with the same file.

We'll soon become strict about this on the firmware interface to reject such calls later, so correct the test scripts to avoid such uses as well. We address this on the tests scripts by simply using unique names when testing the custom fallback interface.

---
 tools/testing/selftests/firmware/fw_fallback.sh   | 20 ++++++++++++++------
 tools/testing/selftests/firmware/fw_filesystem.sh | 11 +++++++++--
 tools/testing/selftests/firmware/fw_lib.sh        | 23 +++++++++++++++++++++++
 3 files changed, 46 insertions(+), 8 deletions(-)

index 9337a0328627..8e2e34a2ca69 100755
@@ -238,8 +238,10 @@ run_sysfs_main_tests()
 
 run_sysfs_custom_load_tests()
 {
-	if load_fw_custom "$NAME" "$FW" ; then
-		if ! diff -q "$FW" /dev/test_firmware >/dev/null ; then
+	RANDOM_FILE_PATH=$(setup_random_file)
+	RANDOM_FILE="$(basename $RANDOM_FILE_PATH)"
+	if load_fw_custom "$RANDOM_FILE" "$RANDOM_FILE_PATH" ; then
+		if ! diff -q "$RANDOM_FILE_PATH" /dev/test_firmware >/dev/null ; then
 			echo "$0: firmware was not loaded" >&2
 			exit 1
 		else

@@ -247,8 +249,10 @@ run_sysfs_custom_load_tests()
 		fi
 	fi
 
-	if load_fw_custom "$NAME" "$FW" ; then
-		if ! diff -q "$FW" /dev/test_firmware >/dev/null ; then
+	RANDOM_FILE_PATH=$(setup_random_file)
+	RANDOM_FILE="$(basename $RANDOM_FILE_PATH)"
+	if load_fw_custom "$RANDOM_FILE" "$RANDOM_FILE_PATH" ; then
+		if ! diff -q "$RANDOM_FILE_PATH" /dev/test_firmware >/dev/null ; then
 			echo "$0: firmware was not loaded" >&2
 			exit 1
 		else

@@ -256,8 +260,12 @@ run_sysfs_custom_load_tests()
 		fi
 	fi
 
-	if load_fw_custom_cancel "nope-$NAME" "$FW" ; then
-		if diff -q "$FW" /dev/test_firmware >/dev/null ; then
+	RANDOM_FILE_REAL="$RANDOM_FILE_PATH"
+	FAKE_RANDOM_FILE_PATH=$(setup_random_file_fake)
+	FAKE_RANDOM_FILE="$(basename $FAKE_RANDOM_FILE_PATH)"
+
+	if load_fw_custom_cancel "$FAKE_RANDOM_FILE" "$RANDOM_FILE_REAL" ; then
+		if diff -q "$RANDOM_FILE_PATH" /dev/test_firmware >/dev/null ; then
 			echo "$0: firmware was expected to be cancelled" >&2
 			exit 1
 		else

index 7f47877fa7fa..6452d2129cd9 100755
@@ -230,10 +230,13 @@ test_wait_and_cancel_custom_load()
 test_request_firmware_nowait_custom_nofile()
 {
 	echo -n "Batched request_firmware_nowait(uevent=false) nofile try #$1: "
+	config_reset
 	config_unset_uevent
-	config_set_name nope-test-firmware.bin
+	RANDOM_FILE_PATH=$(setup_random_file_fake)
+	RANDOM_FILE="$(basename $RANDOM_FILE_PATH)"
+	config_set_name $RANDOM_FILE
 	config_trigger_async &
-	test_wait_and_cancel_custom_load nope-test-firmware.bin
+	test_wait_and_cancel_custom_load $RANDOM_FILE
 	wait
 	release_all_firmware
 	echo "OK"

@@ -271,7 +274,11 @@ test_request_firmware_nowait_uevent()
 test_request_firmware_nowait_custom()
 {
 	echo -n "Batched request_firmware_nowait(uevent=false) try #$1: "
+	config_reset
 	config_unset_uevent
+	RANDOM_FILE_PATH=$(setup_random_file)
+	RANDOM_FILE="$(basename $RANDOM_FILE_PATH)"
+	config_set_name $RANDOM_FILE
 	config_trigger_async
 	release_all_firmware
 	echo "OK"

index 98dceb847ba0..9ea31b57d71a 100755
@@ -104,6 +104,29 @@ setup_tmp_file()
 	fi
 }
 
+__setup_random_file()
+{
+	RANDOM_FILE_PATH="$(mktemp -p $FWPATH)"
+	# mktemp says dry-run -n is unsafe, so...
+	if [[ "$1" = "fake" ]]; then
+		rm -rf $RANDOM_FILE_PATH
+		sync
+	else
+		echo "ABCD0123" >"$RANDOM_FILE_PATH"
+	fi
+	echo $RANDOM_FILE_PATH
+}
+
+setup_random_file()
+{
+	echo $(__setup_random_file)
+}
+
+setup_random_file_fake()
+{
+	echo $(__setup_random_file fake)
+}
+
 proc_set_force_sysfs_fallback()
 {
 	if [ -f $FW_FORCE_SYSFS_FALLBACK ]; then

2.16.2

request_firmware_into_buf() explicitly disables the firmware cache, meanwhile the firmware cache cannot be used when request_firmware_nowait() is used without the uevent. Enforce a sanity check for this to avoid future issues undocumented behaviours should misuses of the firmware cache happen later.

One of the reasons we want to enforce this is the firmware cache is used for helping with suspend/resume, and if incompatible calls use it they can stall suspend.

---
 drivers/base/firmware_loader/main.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

index b569d8a09392..2913bb0e5e7b 100644
@@ -431,6 +431,11 @@ static int fw_add_devm_name(struct device *dev, const char *name)
 	return 0;
 }
 #else
+static bool fw_cache_is_setup(struct device *dev, const char *name)
+{
+	return false;
+}
+
 static int fw_add_devm_name(struct device *dev, const char *name)
 {
 	return 0;

@@ -672,6 +677,9 @@ request_firmware_into_buf(const struct firmware **firmware_p, const char *name,
 {
 	int ret;
 
+	if (fw_cache_is_setup(device, name))
+		return -EOPNOTSUPP;
+
 	__module_get(THIS_MODULE);
 	ret = _request_firmware(firmware_p, name, device, buf, size,
 				FW_OPT_UEVENT | FW_OPT_NOCACHE);

@@ -769,6 +777,12 @@ request_firmware_nowait(
 	fw_work->opt_flags = FW_OPT_NOWAIT |
 		(uevent ? FW_OPT_UEVENT : FW_OPT_USERHELPER);
 
+	if (!uevent && fw_cache_is_setup(device, name)) {
+		kfree_const(fw_work->name);
+		kfree(fw_work);
+		return -EOPNOTSUPP;
+	}
+
 	if (!try_module_get(module)) {
 		kfree_const(fw_work->name);
 		kfree(fw_work);

2.16.2

Some devices have an optimization in place to enable the firmware to be retaineed during a system reboot, so after reboot the device can skip requesting and loading the firmware. This can save up to 1s in load time. The mt7601u 802.11 device happens to be such a device.

When these devices retain the firmware on a reboot and then suspend they can miss looking for the firmware on resume. To help with this we need a way to cache the firmware when such an optimization has taken place.

---
 .../driver-api/firmware/request_firmware.rst       | 14 +++++++++++++
 drivers/base/firmware_loader/main.c                | 24 ++++++++++++++++++++++
 include/linux/firmware.h                           |  3 +++
 3 files changed, 41 insertions(+)

index cc0aea880824..b554f4338859 100644
@@ -44,6 +44,20 @@ request_firmware_nowait
 .. kernel-doc:: drivers/base/firmware_class.c
    :functions: request_firmware_nowait
 
+Special optimizations on reboot
+===============================
+
+Some devices have an optimization in place to enable the firmware to be
+retained during system reboot. When such optimizations are used the driver
+author must ensure the firmware is still available on resume from suspend,
+this can be done with request_firmware_cache() insted of requesting for the
+firmare to be loaded.
+
+request_firmware_cache()
+-----------------------
+.. kernel-doc:: drivers/base/firmware_class.c
+   :functions: request_firmware_load
+
 request firmware API expected driver use
 ========================================
 

index 2913bb0e5e7b..04bf853f60e9 100644
@@ -656,6 +656,30 @@ int request_firmware_direct(const struct firmware **firmware_p,
 }
 EXPORT_SYMBOL_GPL(request_firmware_direct);
 
+/**
+ * request_firmware_cache: - cache firmware for suspend so resume can use it
+ * @name: name of firmware file
+ * @device: device for which firmware should be cached for
+ *
+ * There are some devices with an optimization that enables the device to not
+ * require loading firmware on system reboot. This optimization may still
+ * require the firmware present on resume from suspend. This routine can be
+ * used to ensure the firmware is present on resume from suspend in these
+ * situations. This helper is not compatible with drivers which use
+ * request_firmware_into_buf() or request_firmware_nowait() with no uevent set.
+ **/
+int request_firmware_cache(struct device *device, const char *name)
+{
+	int ret;
+
+	mutex_lock(&fw_lock);
+	ret = fw_add_devm_name(device, name);
+	mutex_unlock(&fw_lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(request_firmware_cache);
+
 /**
  * request_firmware_into_buf - load firmware into a previously allocated buffer
  * @firmware_p: pointer to firmware image

index d4508080348d..166867910ebc 100644
@@ -85,4 +85,7 @@ static inline int request_firmware_into_buf(const struct firmware **firmware_p,
 }
 
 #endif
+
+int request_firmware_cache(struct device *device, const char *name);
+
 #endif

2.16.2

request_firmware_cache() will ensure the firmware is available on resume from suspend if on reboot the device retains the firmware.

This optimization is in place given otherwise on reboot we have to reload the firmware, the opmization saves us about max 1s, minimum 10ms.

Cantabile has reported back this fixes his woes with both suspend and hibernation.

---
 drivers/net/wireless/mediatek/mt7601u/mcu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 65a8004418ea..b90456a4b4d7 100644
@@ -421,7 +421,7 @@ static int mt7601u_load_firmware(struct mt7601u_dev *dev)
 					 MT_USB_DMA_CFG_TX_BULK_EN));
 
 	if (firmware_running(dev))
-		return 0;
+		return request_firmware_cache(dev->dev, MT7601U_FIRMWARE);
 
 	ret = request_firmware(&fw, MT7601U_FIRMWARE, dev->dev);
 	if (ret)

2.16.2

This all looks good to me! Thanks for respinning. :)

-Kees

Kees Cook
Pixel Security

FWIW:

Thanks Luis!!

Greg,

I'd like to get these baked a bit through linux-next. Let me know if its easier for you if I go through Andrew for the firmware API changes.

Luis

I'm just about to queue up all of the firmware patches right now, so no need to bother Andrew :)

thanks,

greg k-h

Why? What does it matter?

This feels really odd to me. Why would you want to use it as an int, just keep it the same "size" everywhere and it should be simpler and easier to keep working correctly over time.

thanks,

greg k-h

Because we want to remove direct access to things which the direct firmware loader should not care about, and instead have proper wrappers so that the fallback code implements it when needed. Part of the motivation for this then was to move all fallback code into its own file therefore compartmentalizing it.

One is the input/output we provide for it uses ints all over so its much easier to handle this as an int consistently in most places and only deal with the long where needed. See above uses of simple_strtol(), add_uevent_var(). Otherwise the inverse has to be done. This was easier to deal with and vet for.

Luis

For this case yes, but I later expand on this structure for all fallback configuration items. So we have to start it somewhere.

Please read the final results and let me know what you think.

I don't use them for sport, I use them given the fallback stuff *is* not something the direct core firmware code should use openly, it will really depend on if the fallback interface was enabled. As such we provide wrapper for access into them.

Luis

Indeed. Will respin with the other space fix. As for the other changes let me know if you have further feedback or if I should just keep them as-is.

Luis

Sure.

Sure, so you want to also rename the old exported symbols, and add a macro or static inline to enable use of the older names?

Sure thing.

Luis

Renaming is best, let's not keep them around for no good reason :)

thanks,

greg k-h

Sure thing, I'll rename the old firmware calls.

Since that would be a cross-tree collateral evolution, as we have discussed before over these at kernel summits, I'd prefer to leave that large set of renames toward the end of the series, as the last patch. This way the Coccinelle SmPL / sed script could be run at the very end, and in case of merge conflicts re-run.

Which BTW... *long term*, I believe a possible way to address these cross-tree collateral evolutions, due to new merge updates and conflicts, could be for us to embed the SmPL / sed scripts into git notes, and in turn have an *optional* git configuration which could attempt a merge conflict resolution through alternative means, one of which -- if enabled -- could be to try to run the SmPL or sed script in the attached git note (if you downloaded them). One of the other benefits of this is the commit log is no longer visually impaired by such long semantic patches or scripts, and gives us a nice way to pull these up through a generic tooling mechanism. This could all just be optional, no one would have to push or fetch these -- only if they wanted the possible added benefits of having them.

*Iff* this seems sensible, this would only mean kernel.org would have to start accepting git notes long term, for those who optionally want to push them or fetch them.

FWIW I've confirmed with github that they now accepts git notes, the UI for github just needs time to develop how to interact with them, for now they just hide them. One of the difficulties with a UI interface for them is that by default git notes *do not* use a namespace reflective for a branch or anything. We could stick to a actual namespace for branch / commit:

"iso" for isomorphism

git note --ref branch/iso-coccinelle/ add commit-id vi refs/notes/branch/iso-coccinelle/commit-id git note --ref branch/iso-coccinelle/ edit commit-id

This is rather crude though so simple interface such as the following could be added as well:

git smpl add sp.cocci commit-id
git smpl edit commit-id
git smpl list v4.13..v4.14

Similar thing could be done for crash dumps, if we wanted...

Since this still would need to be discussed, and if agreed then implemented, in the meantime I can just do it the old school way of embedding the SmPL and/or sed script into the commit log, but left as the last patch in the series.

Luis

We don't disallow them. You just need to make sure you're fetching and pushing them, because they aren't by default. You can set this in your kernel.org origin section of .git/config:

[remote origin]
...
fetch = +refs/notes/*:refs/notes/*

And then push them separately as "git push origin refs/notes/*".

Since frontends clone with --mirror, the notes will be available on all git.kernel.org nodes (e.g. see
https://git.kernel.org/mricon/hook-test/c/a8d310d4c13)

If you do start using notes, I strongly suggest you pick a dedicated refspace for it instead of putting things into the default refs/notes/commits, e.g.:

git notes --ref crosstree [...]

This will create refs/notes/crosstree that has less of a chance to be clobbered by someone else's use of notes.

Best,

Konstantin Ryabitsev
Director, IT Infrastructure Security
The Linux Foundation

Superb, thanks!

Good to know thanks, and it looks good!

Indeed, the default is crap. I was suggesting perhaps having it also be per branch, so:

git note --ref branch/iso-coccinelle/ add commit-id

If it was required sed I guess

git note --ref branch/sed/ add commit-id

And if we wanted a resolution handler which could manage the above for you:

git note --ref branch/merger-script add commit-id

Which could use the above two to run what is needed to mimic the commit.

Luis

Ah, in looking at this closer, that might take a bit of time, as there are a _lot_ of callers of request_firmware(). So maybe a macro/wrapper is good for that one so we can propagate the changes through the different subsystems and do this over a few kernel releases.

But for anything new, let's get it right the first time :)

thanks,

greg k-h