[PATCH 0/4] firmware: few more fixes - fix batched requests

[PATCH 0/4] firmware: few more fixes - fix batched requests

[Luis R. Rodriguez]

Greg,

As I hinted as I worked on the last firmware fixes, the batched requests
mechanism of the firmware API still had one remaining issue which has been
broken for a long time. This series fixes the last issue I am aware of.

I've written test cases for it using a smiller test interface which we
can later port the other tests for it. I haven't moved the old tests to
it given a) kselftests are being tested on older kernels with older
test_firmware drivers so moving the tests would nullify the old tests. We
could later address this by checking for both triggers for compatibility
purposes but we would also have to test for everything we tested for the
old tests. The new tests only first prioritize batched requets for now.
With time we could mesh them all together and still remain compatible with
the old test_firmware on older kernels.

I've pushed testsing on all kernel configuration possible and provide
report of the issue on all configurations as well, before and after for
all the APIs.

This series depends on the last series of fixes I had posted before. As usual
I have a tree with this and all other pending fixes on my linux-next tree on the
20170714-fw-fix-batched-req branch [0] based on linux-next tag next-20170714.

If there any questions please let me know.

[0] https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux-next.git/log/?h=20170714-fw-fix-batched-req

Luis R. Rodriguez (4):
  firmware: define pr_fmt
  firwmare: enable a debug print for batched requests
  test_firmware: add batched firmware tests
  firmware: fix batched requests: send wake up on failure on direct
    lookups

 drivers/base/firmware_class.c                     |  42 +-
 lib/test_firmware.c                               | 594 ++++++++++++++++++++++
 tools/testing/selftests/firmware/fw_filesystem.sh | 105 +++-
 3 files changed, 731 insertions(+), 10 deletions(-)

-- 
2.11.0

[PATCH 1/4] firmware: define pr_fmt

[Luis R. Rodriguez]

For some reason we have always forgotten this. Without this
we don't get a nice prefix on our pr_debug() / pr_*() messages.

Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 drivers/base/firmware_class.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index d3d071dbd2a5..6f58a15efa2b 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -7,6 +7,8 @@
  *
  */
 
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
 #include <linux/capability.h>
 #include <linux/device.h>
 #include <linux/module.h>
-- 
2.11.0

[PATCH 2/4] firwmare: enable a debug print for batched requests

[Luis R. Rodriguez]

Otherwise there is no easy way this actually happened.

Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 drivers/base/firmware_class.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 6f58a15efa2b..a85dabeb1e20 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -334,6 +334,7 @@ static struct firmware_buf *__fw_lookup_buf(const char *fw_name)
 	return NULL;
 }
 
+/* Returns 1 for batching firmware requests with the same name */
 static int fw_lookup_and_allocate_buf(const char *fw_name,
 				      struct firmware_cache *fwc,
 				      struct firmware_buf **buf, void *dbuf,
@@ -347,6 +348,7 @@ static int fw_lookup_and_allocate_buf(const char *fw_name,
 		kref_get(&tmp->ref);
 		spin_unlock(&fwc->lock);
 		*buf = tmp;
+		pr_debug("batched request - sharing the same struct firmware_buf and lookup for multiple requests\n");
 		return 1;
 	}
 	tmp = __allocate_fw_buf(fw_name, fwc, dbuf, size);
-- 
2.11.0

[PATCH 3/4] test_firmware: add batched firmware tests

[Luis R. Rodriguez]

The firmware API has a feature to enable batching requests for the same file
under one worker, so only one lookup is done. This only triggers if we so
happen to schedule two lookups for same file around the same time, or if
release_firmware() has not been called for a successful firmware call. This
can happen for instance if you happen to have multiple devices and one
device driver for certain drivers where the stars line up scheduling
wise.

This adds a new sync and async test trigger. Instead of adding a new trigger
for each new test type we make the tests a bit configurable so that we could
configure the tests in userspace and just kick a test through a few basic
triggers. With this, for instance the two types of sync requests:

  o request_firmware() and
  o request_firmware_direct()

can be modified with a knob. ikewise the two type of async requests:

   o request_firmware_nowait(uevent=true) and
   o request_firmware_nowait(uevent=false)

can be configured with another knob. The call request_firmware_into_buf()
has no users... yet.

The old tests are left in place as-is given they serve a few other purposes
which we are currently not interested in also testing yet. This may change
later and we could just consolidate all tests under a few basic triggers,
and just one general configuration setup.

This testing reveals a few broken things:

============================================================================
CONFIG_FW_LOADER_USER_HELPER_FALLBACK=n
CONFIG_FW_LOADER_USER_HELPER=y

Most common Linux distribution setup.

API-type                               no-firmware-found   firmware-found
----------------------------------------------------------------------
request_firmware()                     FAIL                OK
request_firmware_direct()              FAIL                OK
request_firmware_nowait(uevent=true)   FAIL                OK
request_firmware_nowait(uevent=false)  FAIL                OK
============================================================================
CONFIG_FW_LOADER_USER_HELPER_FALLBACK=n
CONFIG_FW_LOADER_USER_HELPER=n

Only possible if CONFIG_DELL_RBU=n and CONFIG_LEDS_LP55XX_COMMON=n, rare.

API-type                               no-firmware-found   firmware-found
----------------------------------------------------------------------
request_firmware()                     FAIL                OK
request_firmware_direct()              FAIL                OK
request_firmware_nowait(uevent=true)   FAIL                OK
request_firmware_nowait(uevent=false)  FAIL                OK
============================================================================
CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y
CONFIG_FW_LOADER_USER_HELPER=y

Google Android setup.

API-type                               no-firmware-found   firmware-found
----------------------------------------------------------------------
request_firmware()                     OK                  OK
request_firmware_direct()              FAIL                OK
request_firmware_nowait(uevent=true)   OK                  OK
request_firmware_nowait(uevent=false)  OK                  OK
============================================================================

Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 lib/test_firmware.c                               | 594 ++++++++++++++++++++++
 tools/testing/selftests/firmware/fw_filesystem.sh | 105 +++-
 2 files changed, 697 insertions(+), 2 deletions(-)

diff --git a/lib/test_firmware.c b/lib/test_firmware.c
index 09371b0a9baf..ad524b6528f5 100644
--- a/lib/test_firmware.c
+++ b/lib/test_firmware.c
@@ -19,10 +19,75 @@
 #include <linux/miscdevice.h>
 #include <linux/slab.h>
 #include <linux/uaccess.h>
+#include <linux/delay.h>
+#include <linux/kthread.h>
+
+#define TEST_FIRMWARE_NAME	"test-firmware.bin"
+#define TEST_FIRMWARE_NUM_REQS	4
 
 static DEFINE_MUTEX(test_fw_mutex);
 static const struct firmware *test_firmware;
 
+/**
+ * test_config - represents configuration for the test for different triggers
+ *
+ * @name: the name of the firmware file to look for
+ * @test_result: a test may use this to collect the result from the call
+ *	of the request_firmware*() calls used in their tests. In order of
+ *	priority we always keep first any setup error. If no setup errors were
+ *	found then we move on to the first error encountered while running the
+ *	API. Note that for async calls this typically will be a successful
+ *	result (0) unless of course you've used bogus parameters, or the system
+ *	is out of memory.  In the async case the callback is expected to do a
+ *	bit more homework to figure out what happened, unfortunately the only
+ *	information passed today on error is the fact that no firmware was
+ *	found so we can only assume -ENOENT on async calls if the firmware is
+ *	NULL.
+ *
+ *	Errors you can expect:
+ *
+ *	API specific:
+ *
+ *	0:		success for sync, for async it means request was sent
+ *	-EINVAL:	invalid parameters or request
+ *	-ENOENT:	files not found
+ *
+ *	System environment:
+ *
+ *	-ENOMEM:	memory pressure on system
+ *	-ENODEV:	out of number of devices to test
+ *	-EINVAL:	an unexpected error has occured
+ * @sync_direct: when the sync trigger is used if this is true
+ * 	request_firmware_direct() will be used instead.
+ * @req_firmware: if @sync_direct is true this is set to
+ * 	request_firmware_direct(), otherwise request_firmware()
+ * @send_uevent: whether or not to send a uevent for async requests
+ * @num_requests: number of requests to try per test case. This is trigger
+ * 	specific.
+ */
+struct test_config {
+	char *name;
+	int test_result;
+	bool sync_direct;
+	int (*req_firmware)(const struct firmware **fw, const char *name,
+			    struct device *device);
+	bool send_uevent;
+	u8 num_requests;
+};
+
+struct test_config *config;
+
+struct test_batched_req {
+	u8 idx;
+	int rc;
+	bool sent;
+	const struct firmware *fw;
+	const char *name;
+	struct completion completion;
+	struct task_struct *task;
+	struct device *dev;
+};
+
 static ssize_t test_fw_misc_read(struct file *f, char __user *buf,
 				 size_t size, loff_t *offset)
 {
@@ -42,6 +107,280 @@ static const struct file_operations test_fw_fops = {
 	.read           = test_fw_misc_read,
 };
 
+static void __test_firmware_config_free(void)
+{
+	kfree_const(config->name);
+	config->name = NULL;
+}
+
+/*
+ * XXX: move to kstrncpy() once merged.
+ *
+ * Users should use kfree_const() when freeing these.
+ */
+static int __kstrncpy(char **dst, const char *name, size_t count, gfp_t gfp)
+{
+	*dst = kstrndup(name, count, gfp);
+	if (!*dst)
+		return -ENOSPC;
+	return count;
+}
+
+static int __test_firmware_config_init(void)
+{
+	int ret;
+
+	ret = __kstrncpy(&config->name, TEST_FIRMWARE_NAME,
+			 strlen(TEST_FIRMWARE_NAME), GFP_KERNEL);
+	if (ret < 0)
+		goto out;
+
+	config->num_requests = TEST_FIRMWARE_NUM_REQS;
+	config->send_uevent = true;
+	config->sync_direct = false;
+	config->req_firmware = request_firmware;
+	config->test_result = 0;
+
+	return 0;
+
+out:
+	__test_firmware_config_free();
+	return ret;
+}
+
+static ssize_t reset_store(struct device *dev,
+			   struct device_attribute *attr,
+			   const char *buf, size_t count)
+{
+	int ret;
+
+	mutex_lock(&test_fw_mutex);
+
+	__test_firmware_config_free();
+
+	ret = __test_firmware_config_init();
+	if (ret < 0) {
+		ret = -ENOMEM;
+		pr_err("could not alloc settings for config trigger: %d\n",
+		       ret);
+		goto out;
+	}
+
+	pr_info("reset\n");
+	ret = count;
+
+out:
+	mutex_unlock(&test_fw_mutex);
+
+	return ret;
+}
+static DEVICE_ATTR_WO(reset);
+
+static ssize_t config_show(struct device *dev,
+			   struct device_attribute *attr,
+			   char *buf)
+{
+	int len = 0;
+
+	mutex_lock(&test_fw_mutex);
+
+	len += snprintf(buf, PAGE_SIZE,
+			"Custom trigger configuration for: %s\n",
+			dev_name(dev));
+
+	if (config->name)
+		len += snprintf(buf+len, PAGE_SIZE,
+				"name:\t%s\n",
+				config->name);
+	else
+		len += snprintf(buf+len, PAGE_SIZE,
+				"name:\tEMTPY\n");
+
+	len += snprintf(buf+len, PAGE_SIZE,
+			"num_requests:\t%u\n", config->num_requests);
+
+	len += snprintf(buf+len, PAGE_SIZE,
+			"send_uevent:\t\t%s\n",
+			config->send_uevent ? "FW_ACTION_HOTPLUG" : "FW_ACTION_NOHOTPLUG");
+	len += snprintf(buf+len, PAGE_SIZE,
+			"sync_direct:\t\t%s\n",
+			config->sync_direct ? "true" : "false");
+
+	mutex_unlock(&test_fw_mutex);
+
+	return len;
+}
+static DEVICE_ATTR_RO(config);
+
+static ssize_t config_name_store(struct device *dev,
+				 struct device_attribute *attr,
+				 const char *buf, size_t count)
+{
+	int ret;
+
+	mutex_lock(&test_fw_mutex);
+	kfree_const(config->name);
+	ret = __kstrncpy(&config->name, buf, count, GFP_KERNEL);
+	mutex_unlock(&test_fw_mutex);
+
+	return ret;
+}
+
+/*
+ * As per sysfs_kf_seq_show() the buf is max PAGE_SIZE.
+ */
+static ssize_t config_test_show_str(char *dst,
+				    char *src)
+{
+	int len;
+
+	mutex_lock(&test_fw_mutex);
+	len = snprintf(dst, PAGE_SIZE, "%s\n", src);
+	mutex_unlock(&test_fw_mutex);
+
+	return len;
+}
+
+static int test_dev_config_update_bool(const char *buf, size_t size,
+				       bool *cfg)
+{
+	int ret;
+
+	mutex_lock(&test_fw_mutex);
+	if (strtobool(buf, cfg) < 0)
+		ret = -EINVAL;
+	else
+		ret = size;
+	mutex_unlock(&test_fw_mutex);
+
+	return ret;
+}
+
+static ssize_t
+test_dev_config_show_bool(char *buf,
+			  bool config)
+{
+	bool val;
+
+	mutex_lock(&test_fw_mutex);
+	val = config;
+	mutex_unlock(&test_fw_mutex);
+
+	return snprintf(buf, PAGE_SIZE, "%d\n", val);
+}
+
+static ssize_t test_dev_config_show_int(char *buf, int cfg)
+{
+	int val;
+
+	mutex_lock(&test_fw_mutex);
+	val = cfg;
+	mutex_unlock(&test_fw_mutex);
+
+	return snprintf(buf, PAGE_SIZE, "%d\n", val);
+}
+
+static int test_dev_config_update_u8(const char *buf, size_t size, u8 *cfg)
+{
+	int ret;
+	long new;
+
+	ret = kstrtol(buf, 10, &new);
+	if (ret)
+		return ret;
+
+	if (new > U8_MAX)
+		return -EINVAL;
+
+	mutex_lock(&test_fw_mutex);
+	*(u8 *)cfg = new;
+	mutex_unlock(&test_fw_mutex);
+
+	/* Always return full write size even if we didn't consume all */
+	return size;
+}
+
+static ssize_t test_dev_config_show_u8(char *buf, u8 cfg)
+{
+	u8 val;
+
+	mutex_lock(&test_fw_mutex);
+	val = cfg;
+	mutex_unlock(&test_fw_mutex);
+
+	return snprintf(buf, PAGE_SIZE, "%u\n", val);
+}
+
+static ssize_t config_name_show(struct device *dev,
+				struct device_attribute *attr,
+				char *buf)
+{
+	return config_test_show_str(buf, config->name);
+}
+static DEVICE_ATTR(config_name, 0644, config_name_show, config_name_store);
+
+static ssize_t config_num_requests_store(struct device *dev,
+					 struct device_attribute *attr,
+					 const char *buf, size_t count)
+{
+	return test_dev_config_update_u8(buf, count, &config->num_requests);
+}
+
+static ssize_t config_num_requests_show(struct device *dev,
+					struct device_attribute *attr,
+					char *buf)
+{
+	return test_dev_config_show_u8(buf, config->num_requests);
+}
+static DEVICE_ATTR(config_num_requests, 0644, config_num_requests_show,
+		   config_num_requests_store);
+
+static ssize_t config_sync_direct_store(struct device *dev,
+					struct device_attribute *attr,
+					const char *buf, size_t count)
+{
+	int rc = test_dev_config_update_bool(buf, count, &config->sync_direct);
+
+	if (rc == count)
+		config->req_firmware = config->sync_direct ?
+				       request_firmware_direct :
+				       request_firmware;
+	return rc;
+}
+
+static ssize_t config_sync_direct_show(struct device *dev,
+				       struct device_attribute *attr,
+				       char *buf)
+{
+	return test_dev_config_show_bool(buf, config->sync_direct);
+}
+static DEVICE_ATTR(config_sync_direct, 0644, config_sync_direct_show,
+		   config_sync_direct_store);
+
+static ssize_t config_send_uevent_store(struct device *dev,
+					struct device_attribute *attr,
+					const char *buf, size_t count)
+{
+	return test_dev_config_update_bool(buf, count, &config->send_uevent);
+}
+
+static ssize_t config_send_uevent_show(struct device *dev,
+				       struct device_attribute *attr,
+				       char *buf)
+{
+	return test_dev_config_show_bool(buf, config->send_uevent);
+}
+static DEVICE_ATTR(config_send_uevent, 0644, config_send_uevent_show,
+		   config_send_uevent_store);
+
+static ssize_t test_result_show(struct device *dev,
+				struct device_attribute *attr,
+				char *buf)
+{
+	return test_dev_config_show_int(buf, config->test_result);
+}
+static DEVICE_ATTR_RO(test_result);
+
 static ssize_t trigger_request_store(struct device *dev,
 				     struct device_attribute *attr,
 				     const char *buf, size_t count)
@@ -170,12 +509,253 @@ static ssize_t trigger_custom_fallback_store(struct device *dev,
 }
 static DEVICE_ATTR_WO(trigger_custom_fallback);
 
+static int test_fw_run_batch_request(void *data)
+{
+	struct test_batched_req *req = data;
+
+	if (!req) {
+		config->test_result = -EINVAL;
+		return -EINVAL;
+	}
+
+	req->rc = config->req_firmware(&req->fw, req->name, req->dev);
+	if (req->rc) {
+		pr_info("#%u: batched sync load failed: %d\n",
+			req->idx, req->rc);
+		if (!config->test_result)
+			config->test_result = req->rc;
+	} else if (req->fw) {
+		req->sent = true;
+		pr_info("#%u: batched sync loaded %zu\n",
+			req->idx, req->fw->size);
+	}
+	complete(&req->completion);
+
+	req->task = NULL;
+
+	return 0;
+}
+
+/*
+ * We use a kthread as otherwise the kernel serializes all our sync requests
+ * and we would not be able to mimic batched requests on a sync call. Batched
+ * requests on a sync call can for instance happen on a device driver when
+ * multiple cards are used and firmware loading happens outside of probe.
+ */
+static ssize_t trigger_batched_requests_store(struct device *dev,
+					      struct device_attribute *attr,
+					      const char *buf, size_t count)
+{
+	struct test_batched_req *reqs;
+	struct test_batched_req *req;
+	int rc;
+	u8 i;
+
+	reqs = vzalloc(sizeof(struct test_batched_req) * config->num_requests * 2);
+	if (!reqs)
+		return -ENOMEM;
+
+	mutex_lock(&test_fw_mutex);
+
+	pr_info("batched sync firmware loading '%s' %u times\n",
+		config->name, config->num_requests);
+
+	for (i=0; i < config->num_requests; i++) {
+		req = &reqs[i];
+		if (!req) {
+			BUG_ON(1);
+			continue;
+		}
+		req->fw = NULL;
+		req->idx = i;
+		req->name = config->name;
+		req->dev = dev;
+		init_completion(&req->completion);
+		req->task = kthread_run(test_fw_run_batch_request, req,
+					     "%s-%u", KBUILD_MODNAME, req->idx);
+		if (!req->task || IS_ERR(req->task)) {
+			pr_err("Setting up thread %u failed\n", req->idx);
+			req->task = NULL;
+			rc = -ENOMEM;
+			goto out;
+		}
+	}
+
+	rc = count;
+
+	/*
+	 * We do two passes on our way out to enable more time and delay
+	 * calling release_firmware() to improve our chances of forcing a
+	 * batched request. If we instead called release_firmware() right away
+	 * then we might miss on an opportunity of having a successful firmware
+	 * request pass on the opportunity to be come a batched request.
+	 */
+
+out:
+	for (i=0; i < config->num_requests; i++) {
+		req = &reqs[i];
+		if (req->task || req->sent)
+			wait_for_completion(&req->completion);
+	}
+
+	for (i=0; i < config->num_requests; i++) {
+		req = &reqs[i];
+		if (req->fw)
+			release_firmware(req->fw);
+	}
+
+	vfree(reqs);
+
+	/* Override any worker error if we had a general setup error */
+	if (rc < 0)
+		config->test_result = rc;
+
+	mutex_unlock(&test_fw_mutex);
+
+	return rc;
+}
+static DEVICE_ATTR_WO(trigger_batched_requests);
+
+
+/*
+ * We wait for each callback to return with the lock held, no need to lock here
+ */
+static void trigger_batched_cb(const struct firmware *fw, void *context)
+{
+	struct test_batched_req *req = context;
+
+	if (!req) {
+		config->test_result = -EINVAL;
+		return;
+	}
+
+	/* forces *some* batched requests to queue up */
+	if (!req->idx)
+		ssleep(2);
+
+	req->fw = fw;
+
+	/*
+	 * Unfortunately the firmware API gives us nothing other than a null FW
+	 * if the firmware was not found on async requests.  Best we can do is
+	 * just assume -ENOENT. A better API would pass the actual return
+	 * value to the callback.
+	 */
+	if (!fw && !config->test_result)
+		config->test_result = -ENOENT;
+
+	complete(&req->completion);
+}
+
+static
+ssize_t trigger_batched_requests_async_store(struct device *dev,
+					     struct device_attribute *attr,
+					     const char *buf, size_t count)
+{
+	struct test_batched_req *reqs;
+	struct test_batched_req *req;
+	bool send_uevent;
+	int rc;
+	u8 i;
+
+	reqs = vzalloc(sizeof(struct test_batched_req) *
+			config->num_requests * 2);
+	if (!reqs)
+		return -ENOMEM;
+
+	mutex_lock(&test_fw_mutex);
+
+	pr_info("batched loading '%s' custom fallback mechanism %u times\n",
+		config->name, config->num_requests);
+
+	send_uevent = config->send_uevent ? FW_ACTION_HOTPLUG :
+		FW_ACTION_NOHOTPLUG;
+
+	for (i=0; i < config->num_requests; i++) {
+		req = &reqs[i];
+		if (!req) {
+			BUG_ON(1);
+			continue;
+		}
+		req->name = config->name;
+		req->fw = NULL;
+		req->idx = i;
+		init_completion(&req->completion);
+		rc = request_firmware_nowait(THIS_MODULE, send_uevent,
+					     req->name,
+					     dev, GFP_KERNEL, req,
+					     trigger_batched_cb);
+		if (rc) {
+			pr_info("#%u: batched async load failed setup: %d\n",
+				i, rc);
+			req->rc = rc;
+			goto out;
+		} else
+			req->sent = true;
+	}
+
+	rc = count;
+
+out:
+	/*
+	 * We do two passes on our way out to enable more time and delay
+	 * calling release_firmware() to improve our chances of forcing a
+	 * batched request. If we instead called release_firmware() right away
+	 * then we might miss on an opportunity of having a successful firmware
+	 * request pass on the opportunity to be come a batched request.
+	 */
+
+	for (i=0; i < config->num_requests; i++) {
+		req = &reqs[i];
+		if (req->sent)
+			wait_for_completion(&req->completion);
+	}
+
+	for (i=0; i < config->num_requests; i++) {
+		req = &reqs[i];
+		if (req->fw) {
+			pr_info("#%u: loaded %zu\n", i, req->fw->size);
+			release_firmware(req->fw);
+		} else {
+			pr_err("#%u: failed to async load firmware\n", i);
+			if (rc > 0)
+				rc = -ENODEV;
+		}
+	}
+
+	vfree(reqs);
+
+	/* Override any worker error if we had a general setup error */
+	if (rc < 0)
+		config->test_result = rc;
+
+	mutex_unlock(&test_fw_mutex);
+
+	return rc;
+}
+static DEVICE_ATTR_WO(trigger_batched_requests_async);
+
 #define TEST_FW_DEV_ATTR(name)          &dev_attr_##name.attr
 
 static struct attribute *test_dev_attrs[] = {
+	TEST_FW_DEV_ATTR(reset),
+
+	TEST_FW_DEV_ATTR(config),
+	TEST_FW_DEV_ATTR(config_name),
+	TEST_FW_DEV_ATTR(config_num_requests),
+	TEST_FW_DEV_ATTR(config_sync_direct),
+	TEST_FW_DEV_ATTR(config_send_uevent),
+
+	/* These don't use the config at all - they could be ported! */
 	TEST_FW_DEV_ATTR(trigger_request),
 	TEST_FW_DEV_ATTR(trigger_async_request),
 	TEST_FW_DEV_ATTR(trigger_custom_fallback),
+
+	/* These use the config and can use the test_result */
+	TEST_FW_DEV_ATTR(trigger_batched_requests),
+	TEST_FW_DEV_ATTR(trigger_batched_requests_async),
+
+	TEST_FW_DEV_ATTR(test_result),
 	NULL,
 };
 
@@ -192,8 +772,17 @@ static int __init test_firmware_init(void)
 {
 	int rc;
 
+	config = kzalloc(sizeof(struct test_config), GFP_KERNEL);
+	if (!config)
+		return -ENOMEM;
+
+	rc = __test_firmware_config_init();
+	if (rc)
+		return rc;
+
 	rc = misc_register(&test_fw_misc_device);
 	if (rc) {
+		kfree(config);
 		pr_err("could not register misc device: %d\n", rc);
 		return rc;
 	}
@@ -207,8 +796,13 @@ module_init(test_firmware_init);
 
 static void __exit test_firmware_exit(void)
 {
+	mutex_lock(&test_fw_mutex);
 	release_firmware(test_firmware);
 	misc_deregister(&test_fw_misc_device);
+	__test_firmware_config_free();
+	kfree(config);
+	mutex_unlock(&test_fw_mutex);
+
 	pr_warn("removed interface\n");
 }
 
diff --git a/tools/testing/selftests/firmware/fw_filesystem.sh b/tools/testing/selftests/firmware/fw_filesystem.sh
index e35691239350..335c8baba9fd 100755
--- a/tools/testing/selftests/firmware/fw_filesystem.sh
+++ b/tools/testing/selftests/firmware/fw_filesystem.sh
@@ -25,8 +25,9 @@ if [ ! -d $DIR ]; then
 fi
 
 # 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.
+# 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
@@ -116,4 +117,104 @@ else
 	echo "$0: async filesystem loading works"
 fi
 
+### Batched requests tests
+test_config_present()
+{
+	if [ ! -f $DIR/reset ]; then
+		echo "Configuration triggers not present, ignoring test"
+		exit 0
+	fi
+}
+
+# Defaults:
+# send_uevent is 1
+# sync_direct is 0
+config_reset()
+{
+	echo 1 >  $DIR/reset
+}
+
+config_set_sync_direct()
+{
+	echo 1 >  $DIR/config_sync_direct
+}
+
+config_unset_sync_direct()
+{
+	echo 0 >  $DIR/config_sync_direct
+}
+
+config_set_uevent()
+{
+	echo 1 >  $DIR/config_send_uevent
+}
+
+config_unset_uevent()
+{
+	echo 0 >  $DIR/config_send_uevent
+}
+
+config_trigger_sync()
+{
+	echo -n 1 > $DIR/trigger_batched_requests 2>/dev/null
+}
+
+config_trigger_async()
+{
+	echo -n 1 > $DIR/trigger_batched_requests_async 2> /dev/null
+}
+
+test_batched_request_firmware()
+{
+	echo -n "Batched request_firmware() try #$1: "
+	config_reset
+	config_trigger_sync
+	echo "OK"
+}
+
+test_batched_request_firmware_direct()
+{
+	echo -n "Batched request_firmware_direct() try #$1: "
+	config_reset
+	config_set_sync_direct
+	config_trigger_sync
+	echo "OK"
+}
+
+test_request_firmware_nowait_uevent()
+{
+	echo -n "Batched request_firmware_nowait(uevent=true) try #$1: "
+	config_reset
+	config_trigger_async
+	echo "OK"
+}
+
+test_request_firmware_nowait_custom()
+{
+	echo -n "Batched request_firmware_nowait(uevent=false) try #$1: "
+	config_unset_uevent
+	config_trigger_async
+	echo "OK"
+}
+
+# Only continue if batched request triggers are present on the
+# test-firmware driver
+test_config_present
+
+for i in $(seq 1 5); do
+	test_batched_request_firmware $i
+done
+
+for i in $(seq 1 5); do
+	test_batched_request_firmware_direct $i
+done
+
+for i in $(seq 1 5); do
+	test_request_firmware_nowait_uevent $i
+done
+
+for i in $(seq 1 5); do
+	test_request_firmware_nowait_custom $i
+done
+
 exit 0
-- 
2.11.0

[PATCH 4/4] firmware: fix batched requests: send wake up on failure on direct lookups

[Luis R. Rodriguez]

Fix batched requests from waiting forever on failure.

The firmware API batched requests feature has been broken since the API call
request_firmware_direct() was introduced on commit bba3a87e982ad ("firmware:
Introduce request_firmware_direct()"), added on v3.14 *iff* the firmware
being requested was not present in *certail kernel builds* [0].

When no firmware is found the worker which goes on to finish never informs
waiters queued up of this, so any batched request will stall in what seems
to be forever (MAX_SCHEDULE_TIMEOUT). Sadly, a reboot will also stall, as
the reboot notifier was only designed to kill custom fallback workers. The
issue seems to the user as a type of soft lockup, what *actually* happens
underneath the hood is a wait call which never completes as we failed to
issue a completion on error.

For device drivers with optional firmware schemes (ie, Intel iwlwifi, or
Netronome -- even though it uses request_firmware() and not
request_firmware_direct()), this could mean that when you boot a system with
multiple cards the firmware will seem to never load on the system, or that
the card is just not responsive even the driver initialization. Due to
differences in scheduling possible this should not always trigger --
one would need to to ensure that multiple requests are in place at the
right time for this to work, also release_firmware() must not be called
prior to any other incoming request. The complexity may not be worth
supporting batched requests in the future given the wait mechanism is
only used also for the fallback mechanism. We'll keep it for now and
just fix it.

Its reported that at least with the Intel WiFi cards on one system this
issue was creeping up 50% of the boots [0].

Ater this commit batched testing results:

============================================================================
CONFIG_FW_LOADER_USER_HELPER_FALLBACK=n
CONFIG_FW_LOADER_USER_HELPER=y

Most common Linux distribution setup.

API-type                               no-firmware-found   firmware-found
----------------------------------------------------------------------
request_firmware()                     OK                  OK
request_firmware_direct()              OK                  OK
request_firmware_nowait(uevent=true)   OK                  OK
request_firmware_nowait(uevent=false)  OK                  OK
============================================================================
CONFIG_FW_LOADER_USER_HELPER_FALLBACK=n
CONFIG_FW_LOADER_USER_HELPER=n

Only possible if CONFIG_DELL_RBU=n and CONFIG_LEDS_LP55XX_COMMON=n, rare.

API-type                               no-firmware-found   firmware-found
----------------------------------------------------------------------
request_firmware()                     OK                  OK
request_firmware_direct()              OK                  OK
request_firmware_nowait(uevent=true)   OK                  OK
request_firmware_nowait(uevent=false)  OK                  OK
============================================================================
CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y
CONFIG_FW_LOADER_USER_HELPER=y

Google Android setup.

API-type                               no-firmware-found   firmware-found
----------------------------------------------------------------------
request_firmware()                     OK                  OK
request_firmware_direct()              OK                  OK
request_firmware_nowait(uevent=true)   OK                  OK
request_firmware_nowait(uevent=false)  OK                  OK
============================================================================

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

Cc: stable <stable@vger.kernel.org> # v3.14
Fixes: bba3a87e982ad ("firmware: Introduce request_firmware_direct()"
Reported-by: Nicolas <nbroeking@me.com>
Reported-by: John Ewalt  <jewalt@lgsinnovations.com>
Reported-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 drivers/base/firmware_class.c | 38 ++++++++++++++++++++++++++++++--------
 1 file changed, 30 insertions(+), 8 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index a85dabeb1e20..a5fb884a136d 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -154,28 +154,27 @@ static void __fw_state_set(struct fw_state *fw_st,
 	__fw_state_set(fw_st, FW_STATUS_LOADING)
 #define fw_state_done(fw_st)					\
 	__fw_state_set(fw_st, FW_STATUS_DONE)
+#define fw_state_aborted(fw_st)					\
+	__fw_state_set(fw_st, FW_STATUS_ABORTED)
 #define fw_state_wait(fw_st)					\
 	__fw_state_wait_common(fw_st, MAX_SCHEDULE_TIMEOUT)
 
-#ifndef CONFIG_FW_LOADER_USER_HELPER
-
-#define fw_state_is_aborted(fw_st)	false
-
-#else /* CONFIG_FW_LOADER_USER_HELPER */
-
 static int __fw_state_check(struct fw_state *fw_st, enum fw_status status)
 {
 	return fw_st->status == status;
 }
 
+#define fw_state_is_aborted(fw_st)				\
+	__fw_state_check(fw_st, FW_STATUS_ABORTED)
+
+#ifdef CONFIG_FW_LOADER_USER_HELPER
+
 #define fw_state_aborted(fw_st)					\
 	__fw_state_set(fw_st, FW_STATUS_ABORTED)
 #define fw_state_is_done(fw_st)					\
 	__fw_state_check(fw_st, FW_STATUS_DONE)
 #define fw_state_is_loading(fw_st)				\
 	__fw_state_check(fw_st, FW_STATUS_LOADING)
-#define fw_state_is_aborted(fw_st)				\
-	__fw_state_check(fw_st, FW_STATUS_ABORTED)
 #define fw_state_wait_timeout(fw_st, timeout)			\
 	__fw_state_wait_common(fw_st, timeout)
 
@@ -1204,6 +1203,28 @@ _request_firmware_prepare(struct firmware **firmware_p, const char *name,
 	return 1; /* need to load */
 }
 
+/*
+ * Batched requests need only one wake, we need to do this step last due to the
+ * fallback mechanism. The buf is protected with kref_get(), and it won't be
+ * released until the last user calls release_firmware().
+ *
+ * Failed batched requests are possible as well, in such cases we just share
+ * the struct firmware_buf and won't release it until all requests are woken
+ * and have gone through this same path.
+ */
+static void fw_abort_batch_reqs(struct firmware *fw)
+{
+	struct firmware_buf *buf;
+
+	/* Loaded directly? */
+	if (!fw || !fw->priv)
+		return;
+
+	buf = fw->priv;
+	if (!fw_state_is_aborted(&buf->fw_st))
+		fw_state_aborted(&buf->fw_st);
+}
+
 /* called from request_firmware() and request_firmware_work_func() */
 static int
 _request_firmware(const struct firmware **firmware_p, const char *name,
@@ -1247,6 +1268,7 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
 
  out:
 	if (ret < 0) {
+		fw_abort_batch_reqs(fw);
 		release_firmware(fw);
 		fw = NULL;
 	}
-- 
2.11.0

Re: [PATCH 2/4] firwmare: enable a debug print for batched requests

[Greg KH]

On Sat, Jul 15, 2017 at 02:18:51PM -0700, Luis R. Rodriguez wrote:
> Otherwise there is no easy way this actually happened.
> 
> Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>

You misspelled "firmware" in the subject :(