[PATCH v2 00/23] firmware: cleanup for v4.16

[PATCH v2 00/23] firmware: cleanup for v4.16

[Luis R. Rodriguez]

Here's a series of cleanups for the firmware loader for consideration for
v4.16, *after* the traditional yearly American turkey massacre. This
depends on the few fixes I just posted for consideration for v4.15 [0].

While discussing the cleanup patch I posted last for the firmware API [1],
Greg suggested making the new data structure we use an opaque pointer so
we can avoid modifying so many call sites needing the new parameter. This
was a good suggestion, but we already have our own private opaque data
structure, it was just used for storing a few variables. We also have
our own fw_priv data structure, its however a complete misnomer given
it actually is only used now for the old sysfs firmware loading facility.

The naming mishap happened given the sysfs loader was the default loader back
in the hay day. This is no longer the case, we always directly load firmware
from the filesystem first, and only rely on the sysfs loader mechanism as
a fallback, if its enabled.

So we rename the old fw_priv to reflect its for sysfs only, and re-use the name
now for our existing opaque private data structure, which we'll extend for the
original proposed cleanup.

IMHO this ends up tidying up the code even more and makes the code *much*
easier to read.

While doing this work I also realized how with a small amount of work we
could end up testing the 3 different functional kernel builds possible for
the firmware loader with *one* kernel build with a few debugfs knobs. This
adds this as well and also paves the way to enable us to later split the
sysfs firmware loader onto its own file.

Worth discussing is how the debugfs knobs also reveal how two kernel build
configs could potentially just later end up as module params, if we're happy
to to live with implicating the fallback mechanism always when the firmware
loader is enabled. This would simplfy the kernel build considerably. I'm not
aware of anyone disabling the syfs fallback mechanism completely, given its
only a fallback mechanism now and we have a way to avoid it completely as
well for sync calls, so this very well may be worth a direction worth
evaluating forward

0-day has been happy with this. I've also hammered testing the API on the 3
different kernel builds possible, no regressions were found, and also addressed
ensuring that the selftests still works on older kernels as expected. On older
kernels we run the typical test of two scripts as usual, using whatever
whatever defaults your kernel provides and doing our best with that.

Comments and questions are, as always, welcome. 

[0] https://lkml.kernel.org/r/20171120174535.27000-1-mcgrof@kernel.org
[1] https://lkml.kernel.org/r/20170914225422.31034-1-mcgrof@kernel.org

Luis R. Rodriguez (23):
  firmware: rename struct firmware_priv to struct fw_sysfs
  firmware: rename struct firmware_buf to struct fw_priv
  firmware: rename struct fw_priv->fw_id to fw_name
  firmware: move core data structures to the top of file
  firmware: remove duplicate fw_state_aborted()
  firmware: remove unused __fw_state_is_done()
  firmware: use static inlines for state machine checking
  firmware: rename sysfs state checks with sysfs prefix
  firmware: use static inline for to_fw_priv()
  firmware: add helper to copy built-in data to pre-alloc buffer
  firmware: provide helper for FW_OPT_USERHELPER
  firmware: replace #ifdef over FW_OPT_FALLBACK with function checks
  test_firmware: wrap sysfs timeout test into helper
  test_firmware: wrap basic sysfs fallback tests into helper
  test_firmware: wrap custom sysfs load tests into helper
  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: add debug facility to emulate forcing sysfs fallback
  firmware: add debug facility to emulate disabling sysfs fallback
  test_firmware: add a library for shared helpers
  test_firmware: test the 3 firmware kernel configs using debugfs
  firmware: cleanup - group and document up private firmware parameters

 drivers/base/Kconfig                               |   6 +
 drivers/base/Makefile                              |   2 +
 drivers/base/firmware_debug.c                      |  39 +
 drivers/base/firmware_debug.h                      |  58 ++
 .../base/{firmware_class.c => firmware_loader.c}   | 981 +++++++++++++--------
 tools/testing/selftests/firmware/Makefile          |   2 +-
 tools/testing/selftests/firmware/config            |   5 +
 tools/testing/selftests/firmware/fw_fallback.sh    | 207 +++--
 tools/testing/selftests/firmware/fw_filesystem.sh  |  61 +-
 tools/testing/selftests/firmware/fw_lib.sh         | 174 ++++
 tools/testing/selftests/firmware/fw_run_tests.sh   |  67 ++
 11 files changed, 1064 insertions(+), 538 deletions(-)
 create mode 100644 drivers/base/firmware_debug.c
 create mode 100644 drivers/base/firmware_debug.h
 rename drivers/base/{firmware_class.c => firmware_loader.c} (64%)
 create mode 100755 tools/testing/selftests/firmware/fw_lib.sh
 create mode 100755 tools/testing/selftests/firmware/fw_run_tests.sh

-- 
2.15.0

[PATCH v2 01/23] firmware: rename struct firmware_priv to struct fw_sysfs

[Luis R. Rodriguez]

The struct firmware_priv is only used for the sysfs fallback
mechanism, rename it to make emphasis of this. This will also
enable us to use the name later for something much more
meaninful.

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

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 7ab54f2b032f..82bc70f66996 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -254,7 +254,7 @@ struct fw_name_devm {
 
 static int fw_cache_piggyback_on_request(const char *name);
 
-/* fw_lock could be moved to 'struct firmware_priv' but since it is just
+/* fw_lock could be moved to 'struct fw_sysfs' but since it is just
  * guarding for corner cases a global lock should be OK */
 static DEFINE_MUTEX(fw_lock);
 
@@ -566,16 +566,16 @@ static int assign_firmware_buf(struct firmware *fw, struct device *device,
  * user-mode helper code
  */
 #ifdef CONFIG_FW_LOADER_USER_HELPER
-struct firmware_priv {
+struct fw_sysfs {
 	bool nowait;
 	struct device dev;
 	struct firmware_buf *buf;
 	struct firmware *fw;
 };
 
-static struct firmware_priv *to_firmware_priv(struct device *dev)
+static struct fw_sysfs *to_fw_sysfs(struct device *dev)
 {
-	return container_of(dev, struct firmware_priv, dev);
+	return container_of(dev, struct fw_sysfs, dev);
 }
 
 static void __fw_load_abort(struct firmware_buf *buf)
@@ -591,9 +591,9 @@ static void __fw_load_abort(struct firmware_buf *buf)
 	fw_state_aborted(&buf->fw_st);
 }
 
-static void fw_load_abort(struct firmware_priv *fw_priv)
+static void fw_load_abort(struct fw_sysfs *fw_sysfs)
 {
-	struct firmware_buf *buf = fw_priv->buf;
+	struct firmware_buf *buf = fw_sysfs->buf;
 
 	__fw_load_abort(buf);
 }
@@ -651,18 +651,18 @@ ATTRIBUTE_GROUPS(firmware_class);
 
 static void fw_dev_release(struct device *dev)
 {
-	struct firmware_priv *fw_priv = to_firmware_priv(dev);
+	struct fw_sysfs *fw_sysfs = to_fw_sysfs(dev);
 
-	kfree(fw_priv);
+	kfree(fw_sysfs);
 }
 
-static int do_firmware_uevent(struct firmware_priv *fw_priv, struct kobj_uevent_env *env)
+static int do_firmware_uevent(struct fw_sysfs *fw_sysfs, struct kobj_uevent_env *env)
 {
-	if (add_uevent_var(env, "FIRMWARE=%s", fw_priv->buf->fw_id))
+	if (add_uevent_var(env, "FIRMWARE=%s", fw_sysfs->buf->fw_id))
 		return -ENOMEM;
 	if (add_uevent_var(env, "TIMEOUT=%i", loading_timeout))
 		return -ENOMEM;
-	if (add_uevent_var(env, "ASYNC=%d", fw_priv->nowait))
+	if (add_uevent_var(env, "ASYNC=%d", fw_sysfs->nowait))
 		return -ENOMEM;
 
 	return 0;
@@ -670,12 +670,12 @@ static int do_firmware_uevent(struct firmware_priv *fw_priv, struct kobj_uevent_
 
 static int firmware_uevent(struct device *dev, struct kobj_uevent_env *env)
 {
-	struct firmware_priv *fw_priv = to_firmware_priv(dev);
+	struct fw_sysfs *fw_sysfs = to_fw_sysfs(dev);
 	int err = 0;
 
 	mutex_lock(&fw_lock);
-	if (fw_priv->buf)
-		err = do_firmware_uevent(fw_priv, env);
+	if (fw_sysfs->buf)
+		err = do_firmware_uevent(fw_sysfs, env);
 	mutex_unlock(&fw_lock);
 	return err;
 }
@@ -700,12 +700,12 @@ static inline void unregister_sysfs_loader(void)
 static ssize_t firmware_loading_show(struct device *dev,
 				     struct device_attribute *attr, char *buf)
 {
-	struct firmware_priv *fw_priv = to_firmware_priv(dev);
+	struct fw_sysfs *fw_sysfs = to_fw_sysfs(dev);
 	int loading = 0;
 
 	mutex_lock(&fw_lock);
-	if (fw_priv->buf)
-		loading = fw_state_is_loading(&fw_priv->buf->fw_st);
+	if (fw_sysfs->buf)
+		loading = fw_state_is_loading(&fw_sysfs->buf->fw_st);
 	mutex_unlock(&fw_lock);
 
 	return sprintf(buf, "%d\n", loading);
@@ -746,14 +746,14 @@ static ssize_t firmware_loading_store(struct device *dev,
 				      struct device_attribute *attr,
 				      const char *buf, size_t count)
 {
-	struct firmware_priv *fw_priv = to_firmware_priv(dev);
+	struct fw_sysfs *fw_sysfs = to_fw_sysfs(dev);
 	struct firmware_buf *fw_buf;
 	ssize_t written = count;
 	int loading = simple_strtol(buf, NULL, 10);
 	int i;
 
 	mutex_lock(&fw_lock);
-	fw_buf = fw_priv->buf;
+	fw_buf = fw_sysfs->buf;
 	if (fw_state_is_aborted(&fw_buf->fw_st))
 		goto out;
 
@@ -807,7 +807,7 @@ static ssize_t firmware_loading_store(struct device *dev,
 		dev_err(dev, "%s: unexpected value (%d)\n", __func__, loading);
 		/* fallthrough */
 	case -1:
-		fw_load_abort(fw_priv);
+		fw_load_abort(fw_sysfs);
 		break;
 	}
 out:
@@ -854,12 +854,12 @@ static ssize_t firmware_data_read(struct file *filp, struct kobject *kobj,
 				  char *buffer, loff_t offset, size_t count)
 {
 	struct device *dev = kobj_to_dev(kobj);
-	struct firmware_priv *fw_priv = to_firmware_priv(dev);
+	struct fw_sysfs *fw_sysfs = to_fw_sysfs(dev);
 	struct firmware_buf *buf;
 	ssize_t ret_count;
 
 	mutex_lock(&fw_lock);
-	buf = fw_priv->buf;
+	buf = fw_sysfs->buf;
 	if (!buf || fw_state_is_done(&buf->fw_st)) {
 		ret_count = -ENODEV;
 		goto out;
@@ -883,9 +883,9 @@ static ssize_t firmware_data_read(struct file *filp, struct kobject *kobj,
 	return ret_count;
 }
 
-static int fw_realloc_buffer(struct firmware_priv *fw_priv, int min_size)
+static int fw_realloc_buffer(struct fw_sysfs *fw_sysfs, int min_size)
 {
-	struct firmware_buf *buf = fw_priv->buf;
+	struct firmware_buf *buf = fw_sysfs->buf;
 	int pages_needed = PAGE_ALIGN(min_size) >> PAGE_SHIFT;
 
 	/* If the array of pages is too small, grow it... */
@@ -896,7 +896,7 @@ static int fw_realloc_buffer(struct firmware_priv *fw_priv, int min_size)
 
 		new_pages = vmalloc(new_array_size * sizeof(void *));
 		if (!new_pages) {
-			fw_load_abort(fw_priv);
+			fw_load_abort(fw_sysfs);
 			return -ENOMEM;
 		}
 		memcpy(new_pages, buf->pages,
@@ -913,7 +913,7 @@ static int fw_realloc_buffer(struct firmware_priv *fw_priv, int min_size)
 			alloc_page(GFP_KERNEL | __GFP_HIGHMEM);
 
 		if (!buf->pages[buf->nr_pages]) {
-			fw_load_abort(fw_priv);
+			fw_load_abort(fw_sysfs);
 			return -ENOMEM;
 		}
 		buf->nr_pages++;
@@ -938,7 +938,7 @@ static ssize_t firmware_data_write(struct file *filp, struct kobject *kobj,
 				   char *buffer, loff_t offset, size_t count)
 {
 	struct device *dev = kobj_to_dev(kobj);
-	struct firmware_priv *fw_priv = to_firmware_priv(dev);
+	struct fw_sysfs *fw_sysfs = to_fw_sysfs(dev);
 	struct firmware_buf *buf;
 	ssize_t retval;
 
@@ -946,7 +946,7 @@ static ssize_t firmware_data_write(struct file *filp, struct kobject *kobj,
 		return -EPERM;
 
 	mutex_lock(&fw_lock);
-	buf = fw_priv->buf;
+	buf = fw_sysfs->buf;
 	if (!buf || fw_state_is_done(&buf->fw_st)) {
 		retval = -ENODEV;
 		goto out;
@@ -960,7 +960,7 @@ static ssize_t firmware_data_write(struct file *filp, struct kobject *kobj,
 		firmware_rw_buf(buf, buffer, offset, count, false);
 		retval = count;
 	} else {
-		retval = fw_realloc_buffer(fw_priv, offset + count);
+		retval = fw_realloc_buffer(fw_sysfs, offset + count);
 		if (retval)
 			goto out;
 
@@ -1001,22 +1001,22 @@ static const struct attribute_group *fw_dev_attr_groups[] = {
 	NULL
 };
 
-static struct firmware_priv *
+static struct fw_sysfs *
 fw_create_instance(struct firmware *firmware, const char *fw_name,
 		   struct device *device, unsigned int opt_flags)
 {
-	struct firmware_priv *fw_priv;
+	struct fw_sysfs *fw_sysfs;
 	struct device *f_dev;
 
-	fw_priv = kzalloc(sizeof(*fw_priv), GFP_KERNEL);
-	if (!fw_priv) {
-		fw_priv = ERR_PTR(-ENOMEM);
+	fw_sysfs = kzalloc(sizeof(*fw_sysfs), GFP_KERNEL);
+	if (!fw_sysfs) {
+		fw_sysfs = ERR_PTR(-ENOMEM);
 		goto exit;
 	}
 
-	fw_priv->nowait = !!(opt_flags & FW_OPT_NOWAIT);
-	fw_priv->fw = firmware;
-	f_dev = &fw_priv->dev;
+	fw_sysfs->nowait = !!(opt_flags & FW_OPT_NOWAIT);
+	fw_sysfs->fw = firmware;
+	f_dev = &fw_sysfs->dev;
 
 	device_initialize(f_dev);
 	dev_set_name(f_dev, "%s", fw_name);
@@ -1024,16 +1024,16 @@ fw_create_instance(struct firmware *firmware, const char *fw_name,
 	f_dev->class = &firmware_class;
 	f_dev->groups = fw_dev_attr_groups;
 exit:
-	return fw_priv;
+	return fw_sysfs;
 }
 
 /* load a firmware via user helper */
-static int _request_firmware_load(struct firmware_priv *fw_priv,
+static int _request_firmware_load(struct fw_sysfs *fw_sysfs,
 				  unsigned int opt_flags, long timeout)
 {
 	int retval = 0;
-	struct device *f_dev = &fw_priv->dev;
-	struct firmware_buf *buf = fw_priv->buf;
+	struct device *f_dev = &fw_sysfs->dev;
+	struct firmware_buf *buf = fw_sysfs->buf;
 
 	/* fall back on userspace loading */
 	if (!buf->data)
@@ -1055,7 +1055,7 @@ static int _request_firmware_load(struct firmware_priv *fw_priv,
 		buf->need_uevent = true;
 		dev_set_uevent_suppress(f_dev, false);
 		dev_dbg(f_dev, "firmware: requesting %s\n", buf->fw_id);
-		kobject_uevent(&fw_priv->dev.kobj, KOBJ_ADD);
+		kobject_uevent(&fw_sysfs->dev.kobj, KOBJ_ADD);
 	} else {
 		timeout = MAX_JIFFY_OFFSET;
 	}
@@ -1063,7 +1063,7 @@ static int _request_firmware_load(struct firmware_priv *fw_priv,
 	retval = fw_state_wait_timeout(&buf->fw_st, timeout);
 	if (retval < 0) {
 		mutex_lock(&fw_lock);
-		fw_load_abort(fw_priv);
+		fw_load_abort(fw_sysfs);
 		mutex_unlock(&fw_lock);
 	}
 
@@ -1085,7 +1085,7 @@ static int fw_load_from_user_helper(struct firmware *firmware,
 				    const char *name, struct device *device,
 				    unsigned int opt_flags)
 {
-	struct firmware_priv *fw_priv;
+	struct fw_sysfs *fw_sysfs;
 	long timeout;
 	int ret;
 
@@ -1106,14 +1106,14 @@ static int fw_load_from_user_helper(struct firmware *firmware,
 		}
 	}
 
-	fw_priv = fw_create_instance(firmware, name, device, opt_flags);
-	if (IS_ERR(fw_priv)) {
-		ret = PTR_ERR(fw_priv);
+	fw_sysfs = fw_create_instance(firmware, name, device, opt_flags);
+	if (IS_ERR(fw_sysfs)) {
+		ret = PTR_ERR(fw_sysfs);
 		goto out_unlock;
 	}
 
-	fw_priv->buf = firmware->priv;
-	ret = _request_firmware_load(fw_priv, opt_flags, timeout);
+	fw_sysfs->buf = firmware->priv;
+	ret = _request_firmware_load(fw_sysfs, opt_flags, timeout);
 
 	if (!ret)
 		ret = assign_firmware_buf(firmware, device, opt_flags);
-- 
2.15.0

[PATCH v2 02/23] firmware: rename struct firmware_buf to struct fw_priv

[Luis R. Rodriguez]

This reflects much better what this is used for. It also puts emphasis
on the fact we can and should be able to extend this data structure as
we see fit internally as its the opaque private pointer on struct
firmware.

As we rename the data structure, also rename a few functions that use it
to reflect better what they are for.

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

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 82bc70f66996..3f95a9e55b4b 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -218,7 +218,7 @@ struct firmware_cache {
 #endif
 };
 
-struct firmware_buf {
+struct fw_priv {
 	struct kref ref;
 	struct list_head list;
 	struct firmware_cache *fwc;
@@ -247,7 +247,7 @@ struct fw_name_devm {
 	const char *name;
 };
 
-#define to_fwbuf(d) container_of(d, struct firmware_buf, ref)
+#define to_fw_priv(d) container_of(d, struct fw_priv, ref)
 
 #define	FW_LOADER_NO_CACHE	0
 #define	FW_LOADER_START_CACHE	1
@@ -260,39 +260,39 @@ static DEFINE_MUTEX(fw_lock);
 
 static struct firmware_cache fw_cache;
 
-static struct firmware_buf *__allocate_fw_buf(const char *fw_name,
-					      struct firmware_cache *fwc,
-					      void *dbuf, size_t size)
+static struct fw_priv *__allocate_fw_priv(const char *fw_name,
+					  struct firmware_cache *fwc,
+					  void *dbuf, size_t size)
 {
-	struct firmware_buf *buf;
+	struct fw_priv *fw_priv;
 
-	buf = kzalloc(sizeof(*buf), GFP_ATOMIC);
-	if (!buf)
+	fw_priv = kzalloc(sizeof(*fw_priv), GFP_ATOMIC);
+	if (!fw_priv)
 		return NULL;
 
-	buf->fw_id = kstrdup_const(fw_name, GFP_ATOMIC);
-	if (!buf->fw_id) {
-		kfree(buf);
+	fw_priv->fw_id = kstrdup_const(fw_name, GFP_ATOMIC);
+	if (!fw_priv->fw_id) {
+		kfree(fw_priv);
 		return NULL;
 	}
 
-	kref_init(&buf->ref);
-	buf->fwc = fwc;
-	buf->data = dbuf;
-	buf->allocated_size = size;
-	fw_state_init(&buf->fw_st);
+	kref_init(&fw_priv->ref);
+	fw_priv->fwc = fwc;
+	fw_priv->data = dbuf;
+	fw_priv->allocated_size = size;
+	fw_state_init(&fw_priv->fw_st);
 #ifdef CONFIG_FW_LOADER_USER_HELPER
-	INIT_LIST_HEAD(&buf->pending_list);
+	INIT_LIST_HEAD(&fw_priv->pending_list);
 #endif
 
-	pr_debug("%s: fw-%s buf=%p\n", __func__, fw_name, buf);
+	pr_debug("%s: fw-%s fw_priv=%p\n", __func__, fw_name, fw_priv);
 
-	return buf;
+	return fw_priv;
 }
 
-static struct firmware_buf *__fw_lookup_buf(const char *fw_name)
+static struct fw_priv *__lookup_fw_priv(const char *fw_name)
 {
-	struct firmware_buf *tmp;
+	struct fw_priv *tmp;
 	struct firmware_cache *fwc = &fw_cache;
 
 	list_for_each_entry(tmp, &fwc->head, list)
@@ -302,65 +302,65 @@ static struct firmware_buf *__fw_lookup_buf(const char *fw_name)
 }
 
 /* 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,
-				      size_t size)
+static int alloc_lookup_fw_priv(const char *fw_name,
+				struct firmware_cache *fwc,
+				struct fw_priv **fw_priv, void *dbuf,
+				size_t size)
 {
-	struct firmware_buf *tmp;
+	struct fw_priv *tmp;
 
 	spin_lock(&fwc->lock);
-	tmp = __fw_lookup_buf(fw_name);
+	tmp = __lookup_fw_priv(fw_name);
 	if (tmp) {
 		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");
+		*fw_priv = tmp;
+		pr_debug("batched request - sharing the same struct fw_priv and lookup for multiple requests\n");
 		return 1;
 	}
-	tmp = __allocate_fw_buf(fw_name, fwc, dbuf, size);
+	tmp = __allocate_fw_priv(fw_name, fwc, dbuf, size);
 	if (tmp)
 		list_add(&tmp->list, &fwc->head);
 	spin_unlock(&fwc->lock);
 
-	*buf = tmp;
+	*fw_priv = tmp;
 
 	return tmp ? 0 : -ENOMEM;
 }
 
-static void __fw_free_buf(struct kref *ref)
+static void __free_fw_priv(struct kref *ref)
 	__releases(&fwc->lock)
 {
-	struct firmware_buf *buf = to_fwbuf(ref);
-	struct firmware_cache *fwc = buf->fwc;
+	struct fw_priv *fw_priv = to_fw_priv(ref);
+	struct firmware_cache *fwc = fw_priv->fwc;
 
-	pr_debug("%s: fw-%s buf=%p data=%p size=%u\n",
-		 __func__, buf->fw_id, buf, buf->data,
-		 (unsigned int)buf->size);
+	pr_debug("%s: fw-%s fw_priv=%p data=%p size=%u\n",
+		 __func__, fw_priv->fw_id, fw_priv, fw_priv->data,
+		 (unsigned int)fw_priv->size);
 
-	list_del(&buf->list);
+	list_del(&fw_priv->list);
 	spin_unlock(&fwc->lock);
 
 #ifdef CONFIG_FW_LOADER_USER_HELPER
-	if (buf->is_paged_buf) {
+	if (fw_priv->is_paged_buf) {
 		int i;
-		vunmap(buf->data);
-		for (i = 0; i < buf->nr_pages; i++)
-			__free_page(buf->pages[i]);
-		vfree(buf->pages);
+		vunmap(fw_priv->data);
+		for (i = 0; i < fw_priv->nr_pages; i++)
+			__free_page(fw_priv->pages[i]);
+		vfree(fw_priv->pages);
 	} else
 #endif
-	if (!buf->allocated_size)
-		vfree(buf->data);
-	kfree_const(buf->fw_id);
-	kfree(buf);
+	if (!fw_priv->allocated_size)
+		vfree(fw_priv->data);
+	kfree_const(fw_priv->fw_id);
+	kfree(fw_priv);
 }
 
-static void fw_free_buf(struct firmware_buf *buf)
+static void free_fw_priv(struct fw_priv *fw_priv)
 {
-	struct firmware_cache *fwc = buf->fwc;
+	struct firmware_cache *fwc = fw_priv->fwc;
 	spin_lock(&fwc->lock);
-	if (!kref_put(&buf->ref, __fw_free_buf))
+	if (!kref_put(&fw_priv->ref, __free_fw_priv))
 		spin_unlock(&fwc->lock);
 }
 
@@ -383,7 +383,7 @@ module_param_string(path, fw_path_para, sizeof(fw_path_para), 0644);
 MODULE_PARM_DESC(path, "customized firmware image search path with a higher priority than default path");
 
 static int
-fw_get_filesystem_firmware(struct device *device, struct firmware_buf *buf)
+fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv)
 {
 	loff_t size;
 	int i, len;
@@ -393,9 +393,9 @@ fw_get_filesystem_firmware(struct device *device, struct firmware_buf *buf)
 	size_t msize = INT_MAX;
 
 	/* Already populated data member means we're loading into a buffer */
-	if (buf->data) {
+	if (fw_priv->data) {
 		id = READING_FIRMWARE_PREALLOC_BUFFER;
-		msize = buf->allocated_size;
+		msize = fw_priv->allocated_size;
 	}
 
 	path = __getname();
@@ -408,15 +408,15 @@ fw_get_filesystem_firmware(struct device *device, struct firmware_buf *buf)
 			continue;
 
 		len = snprintf(path, PATH_MAX, "%s/%s",
-			       fw_path[i], buf->fw_id);
+			       fw_path[i], fw_priv->fw_id);
 		if (len >= PATH_MAX) {
 			rc = -ENAMETOOLONG;
 			break;
 		}
 
-		buf->size = 0;
-		rc = kernel_read_file_from_path(path, &buf->data, &size, msize,
-						id);
+		fw_priv->size = 0;
+		rc = kernel_read_file_from_path(path, &fw_priv->data, &size,
+						msize, id);
 		if (rc) {
 			if (rc == -ENOENT)
 				dev_dbg(device, "loading %s failed with error %d\n",
@@ -426,9 +426,9 @@ fw_get_filesystem_firmware(struct device *device, struct firmware_buf *buf)
 					 path, rc);
 			continue;
 		}
-		dev_dbg(device, "direct-loading %s\n", buf->fw_id);
-		buf->size = size;
-		fw_state_done(&buf->fw_st);
+		dev_dbg(device, "direct-loading %s\n", fw_priv->fw_id);
+		fw_priv->size = size;
+		fw_state_done(&fw_priv->fw_st);
 		break;
 	}
 	__putname(path);
@@ -444,22 +444,22 @@ static void firmware_free_data(const struct firmware *fw)
 		vfree(fw->data);
 		return;
 	}
-	fw_free_buf(fw->priv);
+	free_fw_priv(fw->priv);
 }
 
 /* store the pages buffer info firmware from buf */
-static void fw_set_page_data(struct firmware_buf *buf, struct firmware *fw)
+static void fw_set_page_data(struct fw_priv *fw_priv, struct firmware *fw)
 {
-	fw->priv = buf;
+	fw->priv = fw_priv;
 #ifdef CONFIG_FW_LOADER_USER_HELPER
-	fw->pages = buf->pages;
+	fw->pages = fw_priv->pages;
 #endif
-	fw->size = buf->size;
-	fw->data = buf->data;
+	fw->size = fw_priv->size;
+	fw->data = fw_priv->data;
 
-	pr_debug("%s: fw-%s buf=%p data=%p size=%u\n",
-		 __func__, buf->fw_id, buf, buf->data,
-		 (unsigned int)buf->size);
+	pr_debug("%s: fw-%s fw_priv=%p data=%p size=%u\n",
+		 __func__, fw_priv->fw_id, fw_priv, fw_priv->data,
+		 (unsigned int)fw_priv->size);
 }
 
 #ifdef CONFIG_PM_SLEEP
@@ -523,13 +523,13 @@ static int fw_add_devm_name(struct device *dev, const char *name)
 }
 #endif
 
-static int assign_firmware_buf(struct firmware *fw, struct device *device,
-			       unsigned int opt_flags)
+static int assign_fw(struct firmware *fw, struct device *device,
+		     unsigned int opt_flags)
 {
-	struct firmware_buf *buf = fw->priv;
+	struct fw_priv *fw_priv = fw->priv;
 
 	mutex_lock(&fw_lock);
-	if (!buf->size || fw_state_is_aborted(&buf->fw_st)) {
+	if (!fw_priv->size || fw_state_is_aborted(&fw_priv->fw_st)) {
 		mutex_unlock(&fw_lock);
 		return -ENOENT;
 	}
@@ -544,20 +544,20 @@ static int assign_firmware_buf(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, buf->fw_id);
+		fw_add_devm_name(device, fw_priv->fw_id);
 
 	/*
 	 * After caching firmware image is started, let it piggyback
 	 * on request firmware.
 	 */
 	if (!(opt_flags & FW_OPT_NOCACHE) &&
-	    buf->fwc->state == FW_LOADER_START_CACHE) {
-		if (fw_cache_piggyback_on_request(buf->fw_id))
-			kref_get(&buf->ref);
+	    fw_priv->fwc->state == FW_LOADER_START_CACHE) {
+		if (fw_cache_piggyback_on_request(fw_priv->fw_id))
+			kref_get(&fw_priv->ref);
 	}
 
 	/* pass the pages buffer to driver at the last minute */
-	fw_set_page_data(buf, fw);
+	fw_set_page_data(fw_priv, fw);
 	mutex_unlock(&fw_lock);
 	return 0;
 }
@@ -569,7 +569,7 @@ static int assign_firmware_buf(struct firmware *fw, struct device *device,
 struct fw_sysfs {
 	bool nowait;
 	struct device dev;
-	struct firmware_buf *buf;
+	struct fw_priv *fw_priv;
 	struct firmware *fw;
 };
 
@@ -578,37 +578,38 @@ static struct fw_sysfs *to_fw_sysfs(struct device *dev)
 	return container_of(dev, struct fw_sysfs, dev);
 }
 
-static void __fw_load_abort(struct firmware_buf *buf)
+static void __fw_load_abort(struct fw_priv *fw_priv)
 {
 	/*
 	 * There is a small window in which user can write to 'loading'
 	 * between loading done and disappearance of 'loading'
 	 */
-	if (fw_state_is_done(&buf->fw_st))
+	if (fw_state_is_done(&fw_priv->fw_st))
 		return;
 
-	list_del_init(&buf->pending_list);
-	fw_state_aborted(&buf->fw_st);
+	list_del_init(&fw_priv->pending_list);
+	fw_state_aborted(&fw_priv->fw_st);
 }
 
 static void fw_load_abort(struct fw_sysfs *fw_sysfs)
 {
-	struct firmware_buf *buf = fw_sysfs->buf;
+	struct fw_priv *fw_priv = fw_sysfs->fw_priv;
 
-	__fw_load_abort(buf);
+	__fw_load_abort(fw_priv);
 }
 
 static LIST_HEAD(pending_fw_head);
 
 static void kill_pending_fw_fallback_reqs(bool only_kill_custom)
 {
-	struct firmware_buf *buf;
-	struct firmware_buf *next;
+	struct fw_priv *fw_priv;
+	struct fw_priv *next;
 
 	mutex_lock(&fw_lock);
-	list_for_each_entry_safe(buf, next, &pending_fw_head, pending_list) {
-		if (!buf->need_uevent || !only_kill_custom)
-			 __fw_load_abort(buf);
+	list_for_each_entry_safe(fw_priv, next, &pending_fw_head,
+				 pending_list) {
+		if (!fw_priv->need_uevent || !only_kill_custom)
+			 __fw_load_abort(fw_priv);
 	}
 	mutex_unlock(&fw_lock);
 }
@@ -658,7 +659,7 @@ static void fw_dev_release(struct device *dev)
 
 static int do_firmware_uevent(struct fw_sysfs *fw_sysfs, struct kobj_uevent_env *env)
 {
-	if (add_uevent_var(env, "FIRMWARE=%s", fw_sysfs->buf->fw_id))
+	if (add_uevent_var(env, "FIRMWARE=%s", fw_sysfs->fw_priv->fw_id))
 		return -ENOMEM;
 	if (add_uevent_var(env, "TIMEOUT=%i", loading_timeout))
 		return -ENOMEM;
@@ -674,7 +675,7 @@ static int firmware_uevent(struct device *dev, struct kobj_uevent_env *env)
 	int err = 0;
 
 	mutex_lock(&fw_lock);
-	if (fw_sysfs->buf)
+	if (fw_sysfs->fw_priv)
 		err = do_firmware_uevent(fw_sysfs, env);
 	mutex_unlock(&fw_lock);
 	return err;
@@ -704,8 +705,8 @@ static ssize_t firmware_loading_show(struct device *dev,
 	int loading = 0;
 
 	mutex_lock(&fw_lock);
-	if (fw_sysfs->buf)
-		loading = fw_state_is_loading(&fw_sysfs->buf->fw_st);
+	if (fw_sysfs->fw_priv)
+		loading = fw_state_is_loading(&fw_sysfs->fw_priv->fw_st);
 	mutex_unlock(&fw_lock);
 
 	return sprintf(buf, "%d\n", loading);
@@ -717,14 +718,15 @@ static ssize_t firmware_loading_show(struct device *dev,
 #endif
 
 /* one pages buffer should be mapped/unmapped only once */
-static int fw_map_pages_buf(struct firmware_buf *buf)
+static int map_fw_priv_pages(struct fw_priv *fw_priv)
 {
-	if (!buf->is_paged_buf)
+	if (!fw_priv->is_paged_buf)
 		return 0;
 
-	vunmap(buf->data);
-	buf->data = vmap(buf->pages, buf->nr_pages, 0, PAGE_KERNEL_RO);
-	if (!buf->data)
+	vunmap(fw_priv->data);
+	fw_priv->data = vmap(fw_priv->pages, fw_priv->nr_pages, 0,
+			     PAGE_KERNEL_RO);
+	if (!fw_priv->data)
 		return -ENOMEM;
 	return 0;
 }
@@ -747,31 +749,31 @@ static ssize_t firmware_loading_store(struct device *dev,
 				      const char *buf, size_t count)
 {
 	struct fw_sysfs *fw_sysfs = to_fw_sysfs(dev);
-	struct firmware_buf *fw_buf;
+	struct fw_priv *fw_priv;
 	ssize_t written = count;
 	int loading = simple_strtol(buf, NULL, 10);
 	int i;
 
 	mutex_lock(&fw_lock);
-	fw_buf = fw_sysfs->buf;
-	if (fw_state_is_aborted(&fw_buf->fw_st))
+	fw_priv = fw_sysfs->fw_priv;
+	if (fw_state_is_aborted(&fw_priv->fw_st))
 		goto out;
 
 	switch (loading) {
 	case 1:
 		/* discarding any previous partial load */
-		if (!fw_state_is_done(&fw_buf->fw_st)) {
-			for (i = 0; i < fw_buf->nr_pages; i++)
-				__free_page(fw_buf->pages[i]);
-			vfree(fw_buf->pages);
-			fw_buf->pages = NULL;
-			fw_buf->page_array_size = 0;
-			fw_buf->nr_pages = 0;
-			fw_state_start(&fw_buf->fw_st);
+		if (!fw_state_is_done(&fw_priv->fw_st)) {
+			for (i = 0; i < fw_priv->nr_pages; i++)
+				__free_page(fw_priv->pages[i]);
+			vfree(fw_priv->pages);
+			fw_priv->pages = NULL;
+			fw_priv->page_array_size = 0;
+			fw_priv->nr_pages = 0;
+			fw_state_start(&fw_priv->fw_st);
 		}
 		break;
 	case 0:
-		if (fw_state_is_loading(&fw_buf->fw_st)) {
+		if (fw_state_is_loading(&fw_priv->fw_st)) {
 			int rc;
 
 			/*
@@ -780,25 +782,25 @@ static ssize_t firmware_loading_store(struct device *dev,
 			 * see the mapped 'buf->data' once the loading
 			 * is completed.
 			 * */
-			rc = fw_map_pages_buf(fw_buf);
+			rc = map_fw_priv_pages(fw_priv);
 			if (rc)
 				dev_err(dev, "%s: map pages failed\n",
 					__func__);
 			else
 				rc = security_kernel_post_read_file(NULL,
-						fw_buf->data, fw_buf->size,
+						fw_priv->data, fw_priv->size,
 						READING_FIRMWARE);
 
 			/*
 			 * Same logic as fw_load_abort, only the DONE bit
 			 * is ignored and we set ABORT only on failure.
 			 */
-			list_del_init(&fw_buf->pending_list);
+			list_del_init(&fw_priv->pending_list);
 			if (rc) {
-				fw_state_aborted(&fw_buf->fw_st);
+				fw_state_aborted(&fw_priv->fw_st);
 				written = rc;
 			} else {
-				fw_state_done(&fw_buf->fw_st);
+				fw_state_done(&fw_priv->fw_st);
 			}
 			break;
 		}
@@ -817,16 +819,16 @@ static ssize_t firmware_loading_store(struct device *dev,
 
 static DEVICE_ATTR(loading, 0644, firmware_loading_show, firmware_loading_store);
 
-static void firmware_rw_buf(struct firmware_buf *buf, char *buffer,
+static void firmware_rw_data(struct fw_priv *fw_priv, char *buffer,
 			   loff_t offset, size_t count, bool read)
 {
 	if (read)
-		memcpy(buffer, buf->data + offset, count);
+		memcpy(buffer, fw_priv->data + offset, count);
 	else
-		memcpy(buf->data + offset, buffer, count);
+		memcpy(fw_priv->data + offset, buffer, count);
 }
 
-static void firmware_rw(struct firmware_buf *buf, char *buffer,
+static void firmware_rw(struct fw_priv *fw_priv, char *buffer,
 			loff_t offset, size_t count, bool read)
 {
 	while (count) {
@@ -835,14 +837,14 @@ static void firmware_rw(struct firmware_buf *buf, char *buffer,
 		int page_ofs = offset & (PAGE_SIZE-1);
 		int page_cnt = min_t(size_t, PAGE_SIZE - page_ofs, count);
 
-		page_data = kmap(buf->pages[page_nr]);
+		page_data = kmap(fw_priv->pages[page_nr]);
 
 		if (read)
 			memcpy(buffer, page_data + page_ofs, page_cnt);
 		else
 			memcpy(page_data + page_ofs, buffer, page_cnt);
 
-		kunmap(buf->pages[page_nr]);
+		kunmap(fw_priv->pages[page_nr]);
 		buffer += page_cnt;
 		offset += page_cnt;
 		count -= page_cnt;
@@ -855,43 +857,43 @@ static ssize_t firmware_data_read(struct file *filp, struct kobject *kobj,
 {
 	struct device *dev = kobj_to_dev(kobj);
 	struct fw_sysfs *fw_sysfs = to_fw_sysfs(dev);
-	struct firmware_buf *buf;
+	struct fw_priv *fw_priv;
 	ssize_t ret_count;
 
 	mutex_lock(&fw_lock);
-	buf = fw_sysfs->buf;
-	if (!buf || fw_state_is_done(&buf->fw_st)) {
+	fw_priv = fw_sysfs->fw_priv;
+	if (!fw_priv || fw_state_is_done(&fw_priv->fw_st)) {
 		ret_count = -ENODEV;
 		goto out;
 	}
-	if (offset > buf->size) {
+	if (offset > fw_priv->size) {
 		ret_count = 0;
 		goto out;
 	}
-	if (count > buf->size - offset)
-		count = buf->size - offset;
+	if (count > fw_priv->size - offset)
+		count = fw_priv->size - offset;
 
 	ret_count = count;
 
-	if (buf->data)
-		firmware_rw_buf(buf, buffer, offset, count, true);
+	if (fw_priv->data)
+		firmware_rw_data(fw_priv, buffer, offset, count, true);
 	else
-		firmware_rw(buf, buffer, offset, count, true);
+		firmware_rw(fw_priv, buffer, offset, count, true);
 
 out:
 	mutex_unlock(&fw_lock);
 	return ret_count;
 }
 
-static int fw_realloc_buffer(struct fw_sysfs *fw_sysfs, int min_size)
+static int fw_realloc_pages(struct fw_sysfs *fw_sysfs, int min_size)
 {
-	struct firmware_buf *buf = fw_sysfs->buf;
+	struct fw_priv *fw_priv= fw_sysfs->fw_priv;
 	int pages_needed = PAGE_ALIGN(min_size) >> PAGE_SHIFT;
 
 	/* If the array of pages is too small, grow it... */
-	if (buf->page_array_size < pages_needed) {
+	if (fw_priv->page_array_size < pages_needed) {
 		int new_array_size = max(pages_needed,
-					 buf->page_array_size * 2);
+					 fw_priv->page_array_size * 2);
 		struct page **new_pages;
 
 		new_pages = vmalloc(new_array_size * sizeof(void *));
@@ -899,24 +901,24 @@ static int fw_realloc_buffer(struct fw_sysfs *fw_sysfs, int min_size)
 			fw_load_abort(fw_sysfs);
 			return -ENOMEM;
 		}
-		memcpy(new_pages, buf->pages,
-		       buf->page_array_size * sizeof(void *));
-		memset(&new_pages[buf->page_array_size], 0, sizeof(void *) *
-		       (new_array_size - buf->page_array_size));
-		vfree(buf->pages);
-		buf->pages = new_pages;
-		buf->page_array_size = new_array_size;
+		memcpy(new_pages, fw_priv->pages,
+		       fw_priv->page_array_size * sizeof(void *));
+		memset(&new_pages[fw_priv->page_array_size], 0, sizeof(void *) *
+		       (new_array_size - fw_priv->page_array_size));
+		vfree(fw_priv->pages);
+		fw_priv->pages = new_pages;
+		fw_priv->page_array_size = new_array_size;
 	}
 
-	while (buf->nr_pages < pages_needed) {
-		buf->pages[buf->nr_pages] =
+	while (fw_priv->nr_pages < pages_needed) {
+		fw_priv->pages[fw_priv->nr_pages] =
 			alloc_page(GFP_KERNEL | __GFP_HIGHMEM);
 
-		if (!buf->pages[buf->nr_pages]) {
+		if (!fw_priv->pages[fw_priv->nr_pages]) {
 			fw_load_abort(fw_sysfs);
 			return -ENOMEM;
 		}
-		buf->nr_pages++;
+		fw_priv->nr_pages++;
 	}
 	return 0;
 }
@@ -939,36 +941,36 @@ static ssize_t firmware_data_write(struct file *filp, struct kobject *kobj,
 {
 	struct device *dev = kobj_to_dev(kobj);
 	struct fw_sysfs *fw_sysfs = to_fw_sysfs(dev);
-	struct firmware_buf *buf;
+	struct fw_priv *fw_priv;
 	ssize_t retval;
 
 	if (!capable(CAP_SYS_RAWIO))
 		return -EPERM;
 
 	mutex_lock(&fw_lock);
-	buf = fw_sysfs->buf;
-	if (!buf || fw_state_is_done(&buf->fw_st)) {
+	fw_priv = fw_sysfs->fw_priv;
+	if (!fw_priv || fw_state_is_done(&fw_priv->fw_st)) {
 		retval = -ENODEV;
 		goto out;
 	}
 
-	if (buf->data) {
-		if (offset + count > buf->allocated_size) {
+	if (fw_priv->data) {
+		if (offset + count > fw_priv->allocated_size) {
 			retval = -ENOMEM;
 			goto out;
 		}
-		firmware_rw_buf(buf, buffer, offset, count, false);
+		firmware_rw_data(fw_priv, buffer, offset, count, false);
 		retval = count;
 	} else {
-		retval = fw_realloc_buffer(fw_sysfs, offset + count);
+		retval = fw_realloc_pages(fw_sysfs, offset + count);
 		if (retval)
 			goto out;
 
 		retval = count;
-		firmware_rw(buf, buffer, offset, count, false);
+		firmware_rw(fw_priv, buffer, offset, count, false);
 	}
 
-	buf->size = max_t(size_t, offset + count, buf->size);
+	fw_priv->size = max_t(size_t, offset + count, fw_priv->size);
 out:
 	mutex_unlock(&fw_lock);
 	return retval;
@@ -1033,11 +1035,11 @@ static int _request_firmware_load(struct fw_sysfs *fw_sysfs,
 {
 	int retval = 0;
 	struct device *f_dev = &fw_sysfs->dev;
-	struct firmware_buf *buf = fw_sysfs->buf;
+	struct fw_priv *fw_priv = fw_sysfs->fw_priv;
 
 	/* fall back on userspace loading */
-	if (!buf->data)
-		buf->is_paged_buf = true;
+	if (!fw_priv->data)
+		fw_priv->is_paged_buf = true;
 
 	dev_set_uevent_suppress(f_dev, true);
 
@@ -1048,31 +1050,31 @@ static int _request_firmware_load(struct fw_sysfs *fw_sysfs,
 	}
 
 	mutex_lock(&fw_lock);
-	list_add(&buf->pending_list, &pending_fw_head);
+	list_add(&fw_priv->pending_list, &pending_fw_head);
 	mutex_unlock(&fw_lock);
 
 	if (opt_flags & FW_OPT_UEVENT) {
-		buf->need_uevent = true;
+		fw_priv->need_uevent = true;
 		dev_set_uevent_suppress(f_dev, false);
-		dev_dbg(f_dev, "firmware: requesting %s\n", buf->fw_id);
+		dev_dbg(f_dev, "firmware: requesting %s\n", fw_priv->fw_id);
 		kobject_uevent(&fw_sysfs->dev.kobj, KOBJ_ADD);
 	} else {
 		timeout = MAX_JIFFY_OFFSET;
 	}
 
-	retval = fw_state_wait_timeout(&buf->fw_st, timeout);
+	retval = fw_state_wait_timeout(&fw_priv->fw_st, timeout);
 	if (retval < 0) {
 		mutex_lock(&fw_lock);
 		fw_load_abort(fw_sysfs);
 		mutex_unlock(&fw_lock);
 	}
 
-	if (fw_state_is_aborted(&buf->fw_st)) {
+	if (fw_state_is_aborted(&fw_priv->fw_st)) {
 		if (retval == -ERESTARTSYS)
 			retval = -EINTR;
 		else
 			retval = -EAGAIN;
-	} else if (buf->is_paged_buf && !buf->data)
+	} else if (fw_priv->is_paged_buf && !fw_priv->data)
 		retval = -ENOMEM;
 
 	device_del(f_dev);
@@ -1112,11 +1114,11 @@ static int fw_load_from_user_helper(struct firmware *firmware,
 		goto out_unlock;
 	}
 
-	fw_sysfs->buf = firmware->priv;
+	fw_sysfs->fw_priv = firmware->priv;
 	ret = _request_firmware_load(fw_sysfs, opt_flags, timeout);
 
 	if (!ret)
-		ret = assign_firmware_buf(firmware, device, opt_flags);
+		ret = assign_fw(firmware, device, opt_flags);
 
 out_unlock:
 	usermodehelper_read_unlock();
@@ -1154,7 +1156,7 @@ _request_firmware_prepare(struct firmware **firmware_p, const char *name,
 			  struct device *device, void *dbuf, size_t size)
 {
 	struct firmware *firmware;
-	struct firmware_buf *buf;
+	struct fw_priv *fw_priv;
 	int ret;
 
 	*firmware_p = firmware = kzalloc(sizeof(*firmware), GFP_KERNEL);
@@ -1169,18 +1171,18 @@ _request_firmware_prepare(struct firmware **firmware_p, const char *name,
 		return 0; /* assigned */
 	}
 
-	ret = fw_lookup_and_allocate_buf(name, &fw_cache, &buf, dbuf, size);
+	ret = alloc_lookup_fw_priv(name, &fw_cache, &fw_priv, dbuf, size);
 
 	/*
-	 * bind with 'buf' now to avoid warning in failure path
+	 * bind with 'priv' now to avoid warning in failure path
 	 * of requesting firmware.
 	 */
-	firmware->priv = buf;
+	firmware->priv = fw_priv;
 
 	if (ret > 0) {
-		ret = fw_state_wait(&buf->fw_st);
+		ret = fw_state_wait(&fw_priv->fw_st);
 		if (!ret) {
-			fw_set_page_data(buf, firmware);
+			fw_set_page_data(fw_priv, firmware);
 			return 0; /* assigned */
 		}
 	}
@@ -1196,20 +1198,20 @@ _request_firmware_prepare(struct firmware **firmware_p, const char *name,
  * 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
+ * the struct fw_priv 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;
+	struct fw_priv *fw_priv;
 
 	/* Loaded directly? */
 	if (!fw || !fw->priv)
 		return;
 
-	buf = fw->priv;
-	if (!fw_state_is_aborted(&buf->fw_st))
-		fw_state_aborted(&buf->fw_st);
+	fw_priv = fw->priv;
+	if (!fw_state_is_aborted(&fw_priv->fw_st))
+		fw_state_aborted(&fw_priv->fw_st);
 }
 
 /* called from request_firmware() and request_firmware_work_func() */
@@ -1245,7 +1247,7 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
 						       opt_flags);
 		}
 	} else
-		ret = assign_firmware_buf(fw, device, opt_flags);
+		ret = assign_fw(fw, device, opt_flags);
 
  out:
 	if (ret < 0) {
@@ -1482,13 +1484,13 @@ static int cache_firmware(const char *fw_name)
 	return ret;
 }
 
-static struct firmware_buf *fw_lookup_buf(const char *fw_name)
+static struct fw_priv *lookup_fw_priv(const char *fw_name)
 {
-	struct firmware_buf *tmp;
+	struct fw_priv *tmp;
 	struct firmware_cache *fwc = &fw_cache;
 
 	spin_lock(&fwc->lock);
-	tmp = __fw_lookup_buf(fw_name);
+	tmp = __lookup_fw_priv(fw_name);
 	spin_unlock(&fwc->lock);
 
 	return tmp;
@@ -1507,7 +1509,7 @@ static struct firmware_buf *fw_lookup_buf(const char *fw_name)
  */
 static int uncache_firmware(const char *fw_name)
 {
-	struct firmware_buf *buf;
+	struct fw_priv *fw_priv;
 	struct firmware fw;
 
 	pr_debug("%s: %s\n", __func__, fw_name);
@@ -1515,9 +1517,9 @@ static int uncache_firmware(const char *fw_name)
 	if (fw_get_builtin_firmware(&fw, fw_name, NULL, 0))
 		return 0;
 
-	buf = fw_lookup_buf(fw_name);
-	if (buf) {
-		fw_free_buf(buf);
+	fw_priv = lookup_fw_priv(fw_name);
+	if (fw_priv) {
+		free_fw_priv(fw_priv);
 		return 0;
 	}
 
-- 
2.15.0

[PATCH v2 03/23] firmware: rename struct fw_priv->fw_id to fw_name

[Luis R. Rodriguez]

This makes it clear exactly what the field is for. With fw_id it
was not clear to a reader if this was some sort of private concoction
of some sort.

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

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 3f95a9e55b4b..3a79231c2cf5 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -234,7 +234,7 @@ struct fw_priv {
 	int page_array_size;
 	struct list_head pending_list;
 #endif
-	const char *fw_id;
+	const char *fw_name;
 };
 
 struct fw_cache_entry {
@@ -270,8 +270,8 @@ static struct fw_priv *__allocate_fw_priv(const char *fw_name,
 	if (!fw_priv)
 		return NULL;
 
-	fw_priv->fw_id = kstrdup_const(fw_name, GFP_ATOMIC);
-	if (!fw_priv->fw_id) {
+	fw_priv->fw_name = kstrdup_const(fw_name, GFP_ATOMIC);
+	if (!fw_priv->fw_name) {
 		kfree(fw_priv);
 		return NULL;
 	}
@@ -296,7 +296,7 @@ static struct fw_priv *__lookup_fw_priv(const char *fw_name)
 	struct firmware_cache *fwc = &fw_cache;
 
 	list_for_each_entry(tmp, &fwc->head, list)
-		if (!strcmp(tmp->fw_id, fw_name))
+		if (!strcmp(tmp->fw_name, fw_name))
 			return tmp;
 	return NULL;
 }
@@ -335,7 +335,7 @@ static void __free_fw_priv(struct kref *ref)
 	struct firmware_cache *fwc = fw_priv->fwc;
 
 	pr_debug("%s: fw-%s fw_priv=%p data=%p size=%u\n",
-		 __func__, fw_priv->fw_id, fw_priv, fw_priv->data,
+		 __func__, fw_priv->fw_name, fw_priv, fw_priv->data,
 		 (unsigned int)fw_priv->size);
 
 	list_del(&fw_priv->list);
@@ -352,7 +352,7 @@ static void __free_fw_priv(struct kref *ref)
 #endif
 	if (!fw_priv->allocated_size)
 		vfree(fw_priv->data);
-	kfree_const(fw_priv->fw_id);
+	kfree_const(fw_priv->fw_name);
 	kfree(fw_priv);
 }
 
@@ -408,7 +408,7 @@ fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv)
 			continue;
 
 		len = snprintf(path, PATH_MAX, "%s/%s",
-			       fw_path[i], fw_priv->fw_id);
+			       fw_path[i], fw_priv->fw_name);
 		if (len >= PATH_MAX) {
 			rc = -ENAMETOOLONG;
 			break;
@@ -426,7 +426,7 @@ fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv)
 					 path, rc);
 			continue;
 		}
-		dev_dbg(device, "direct-loading %s\n", fw_priv->fw_id);
+		dev_dbg(device, "direct-loading %s\n", fw_priv->fw_name);
 		fw_priv->size = size;
 		fw_state_done(&fw_priv->fw_st);
 		break;
@@ -458,7 +458,7 @@ static void fw_set_page_data(struct fw_priv *fw_priv, struct firmware *fw)
 	fw->data = fw_priv->data;
 
 	pr_debug("%s: fw-%s fw_priv=%p data=%p size=%u\n",
-		 __func__, fw_priv->fw_id, fw_priv, fw_priv->data,
+		 __func__, fw_priv->fw_name, fw_priv, fw_priv->data,
 		 (unsigned int)fw_priv->size);
 }
 
@@ -544,7 +544,7 @@ static 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_id);
+		fw_add_devm_name(device, fw_priv->fw_name);
 
 	/*
 	 * After caching firmware image is started, let it piggyback
@@ -552,7 +552,7 @@ static int assign_fw(struct firmware *fw, struct device *device,
 	 */
 	if (!(opt_flags & FW_OPT_NOCACHE) &&
 	    fw_priv->fwc->state == FW_LOADER_START_CACHE) {
-		if (fw_cache_piggyback_on_request(fw_priv->fw_id))
+		if (fw_cache_piggyback_on_request(fw_priv->fw_name))
 			kref_get(&fw_priv->ref);
 	}
 
@@ -659,7 +659,7 @@ static void fw_dev_release(struct device *dev)
 
 static int do_firmware_uevent(struct fw_sysfs *fw_sysfs, struct kobj_uevent_env *env)
 {
-	if (add_uevent_var(env, "FIRMWARE=%s", fw_sysfs->fw_priv->fw_id))
+	if (add_uevent_var(env, "FIRMWARE=%s", fw_sysfs->fw_priv->fw_name))
 		return -ENOMEM;
 	if (add_uevent_var(env, "TIMEOUT=%i", loading_timeout))
 		return -ENOMEM;
@@ -1056,7 +1056,7 @@ static int _request_firmware_load(struct fw_sysfs *fw_sysfs,
 	if (opt_flags & FW_OPT_UEVENT) {
 		fw_priv->need_uevent = true;
 		dev_set_uevent_suppress(f_dev, false);
-		dev_dbg(f_dev, "firmware: requesting %s\n", fw_priv->fw_id);
+		dev_dbg(f_dev, "firmware: requesting %s\n", fw_priv->fw_name);
 		kobject_uevent(&fw_sysfs->dev.kobj, KOBJ_ADD);
 	} else {
 		timeout = MAX_JIFFY_OFFSET;
-- 
2.15.0

[PATCH v2 04/23] firmware: move core data structures to the top of file

[Luis R. Rodriguez]

Move main core data structures used internally for firmware to the top
of the file. This will allow us to use them earlier later in helpers as
we extend their use.

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

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 3a79231c2cf5..4041548d1a81 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -41,6 +41,101 @@ MODULE_AUTHOR("Manuel Estrada Sainz");
 MODULE_DESCRIPTION("Multi purpose firmware loading support");
 MODULE_LICENSE("GPL");
 
+enum fw_status {
+	FW_STATUS_UNKNOWN,
+	FW_STATUS_LOADING,
+	FW_STATUS_DONE,
+	FW_STATUS_ABORTED,
+};
+
+/*
+ * Concurrent request_firmware() for the same firmware need to be
+ * serialized.  struct fw_state is simple state machine which hold the
+ * state of the firmware loading.
+ */
+struct fw_state {
+	struct completion completion;
+	enum fw_status status;
+};
+
+/* firmware behavior options */
+#define FW_OPT_UEVENT	(1U << 0)
+#define FW_OPT_NOWAIT	(1U << 1)
+#ifdef CONFIG_FW_LOADER_USER_HELPER
+#define FW_OPT_USERHELPER	(1U << 2)
+#else
+#define FW_OPT_USERHELPER	0
+#endif
+#ifdef CONFIG_FW_LOADER_USER_HELPER_FALLBACK
+#define FW_OPT_FALLBACK		FW_OPT_USERHELPER
+#else
+#define FW_OPT_FALLBACK		0
+#endif
+#define FW_OPT_NO_WARN	(1U << 3)
+#define FW_OPT_NOCACHE	(1U << 4)
+
+struct firmware_cache {
+	/* firmware_buf instance will be added into the below list */
+	spinlock_t lock;
+	struct list_head head;
+	int state;
+
+#ifdef CONFIG_PM_SLEEP
+	/*
+	 * Names of firmware images which have been cached successfully
+	 * will be added into the below list so that device uncache
+	 * helper can trace which firmware images have been cached
+	 * before.
+	 */
+	spinlock_t name_lock;
+	struct list_head fw_names;
+
+	struct delayed_work work;
+
+	struct notifier_block   pm_notify;
+#endif
+};
+
+struct fw_priv {
+	struct kref ref;
+	struct list_head list;
+	struct firmware_cache *fwc;
+	struct fw_state fw_st;
+	void *data;
+	size_t size;
+	size_t allocated_size;
+#ifdef CONFIG_FW_LOADER_USER_HELPER
+	bool is_paged_buf;
+	bool need_uevent;
+	struct page **pages;
+	int nr_pages;
+	int page_array_size;
+	struct list_head pending_list;
+#endif
+	const char *fw_name;
+};
+
+struct fw_cache_entry {
+	struct list_head list;
+	const char *name;
+};
+
+struct fw_name_devm {
+	unsigned long magic;
+	const char *name;
+};
+
+#define to_fw_priv(d) container_of(d, struct fw_priv, ref)
+
+#define	FW_LOADER_NO_CACHE	0
+#define	FW_LOADER_START_CACHE	1
+
+/* fw_lock could be moved to 'struct fw_sysfs' but since it is just
+ * guarding for corner cases a global lock should be OK */
+static DEFINE_MUTEX(fw_lock);
+
+static struct firmware_cache fw_cache;
+
 /* Builtin firmware support */
 
 #ifdef CONFIG_FW_LOADER
@@ -93,13 +188,6 @@ static inline bool fw_is_builtin_firmware(const struct firmware *fw)
 }
 #endif
 
-enum fw_status {
-	FW_STATUS_UNKNOWN,
-	FW_STATUS_LOADING,
-	FW_STATUS_DONE,
-	FW_STATUS_ABORTED,
-};
-
 static int loading_timeout = 60;	/* In seconds */
 
 static inline long firmware_loading_timeout(void)
@@ -107,16 +195,6 @@ static inline long firmware_loading_timeout(void)
 	return loading_timeout > 0 ? loading_timeout * HZ : MAX_JIFFY_OFFSET;
 }
 
-/*
- * Concurrent request_firmware() for the same firmware need to be
- * serialized.  struct fw_state is simple state machine which hold the
- * state of the firmware loading.
- */
-struct fw_state {
-	struct completion completion;
-	enum fw_status status;
-};
-
 static void fw_state_init(struct fw_state *fw_st)
 {
 	init_completion(&fw_st->completion);
@@ -180,86 +258,8 @@ static int __fw_state_check(struct fw_state *fw_st, enum fw_status status)
 
 #endif /* CONFIG_FW_LOADER_USER_HELPER */
 
-/* firmware behavior options */
-#define FW_OPT_UEVENT	(1U << 0)
-#define FW_OPT_NOWAIT	(1U << 1)
-#ifdef CONFIG_FW_LOADER_USER_HELPER
-#define FW_OPT_USERHELPER	(1U << 2)
-#else
-#define FW_OPT_USERHELPER	0
-#endif
-#ifdef CONFIG_FW_LOADER_USER_HELPER_FALLBACK
-#define FW_OPT_FALLBACK		FW_OPT_USERHELPER
-#else
-#define FW_OPT_FALLBACK		0
-#endif
-#define FW_OPT_NO_WARN	(1U << 3)
-#define FW_OPT_NOCACHE	(1U << 4)
-
-struct firmware_cache {
-	/* firmware_buf instance will be added into the below list */
-	spinlock_t lock;
-	struct list_head head;
-	int state;
-
-#ifdef CONFIG_PM_SLEEP
-	/*
-	 * Names of firmware images which have been cached successfully
-	 * will be added into the below list so that device uncache
-	 * helper can trace which firmware images have been cached
-	 * before.
-	 */
-	spinlock_t name_lock;
-	struct list_head fw_names;
-
-	struct delayed_work work;
-
-	struct notifier_block   pm_notify;
-#endif
-};
-
-struct fw_priv {
-	struct kref ref;
-	struct list_head list;
-	struct firmware_cache *fwc;
-	struct fw_state fw_st;
-	void *data;
-	size_t size;
-	size_t allocated_size;
-#ifdef CONFIG_FW_LOADER_USER_HELPER
-	bool is_paged_buf;
-	bool need_uevent;
-	struct page **pages;
-	int nr_pages;
-	int page_array_size;
-	struct list_head pending_list;
-#endif
-	const char *fw_name;
-};
-
-struct fw_cache_entry {
-	struct list_head list;
-	const char *name;
-};
-
-struct fw_name_devm {
-	unsigned long magic;
-	const char *name;
-};
-
-#define to_fw_priv(d) container_of(d, struct fw_priv, ref)
-
-#define	FW_LOADER_NO_CACHE	0
-#define	FW_LOADER_START_CACHE	1
-
 static int fw_cache_piggyback_on_request(const char *name);
 
-/* fw_lock could be moved to 'struct fw_sysfs' but since it is just
- * guarding for corner cases a global lock should be OK */
-static DEFINE_MUTEX(fw_lock);
-
-static struct firmware_cache fw_cache;
-
 static struct fw_priv *__allocate_fw_priv(const char *fw_name,
 					  struct firmware_cache *fwc,
 					  void *dbuf, size_t size)
-- 
2.15.0

[PATCH v2 05/23] firmware: remove duplicate fw_state_aborted()

[Luis R. Rodriguez]

The macro is defined twice without need.

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

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 4041548d1a81..485bb18cee1d 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -247,8 +247,6 @@ static int __fw_state_check(struct fw_state *fw_st, enum fw_status status)
 
 #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)				\
-- 
2.15.0

[PATCH v2 06/23] firmware: remove unused __fw_state_is_done()

[Luis R. Rodriguez]

After commit e44565f62a ("firmware: fix batched requests - wake all waiters")
where we moved away from swait to old wait with a completion we also
stopped using __fw_state_is_done(). Since this is longer used kill it.

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

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 485bb18cee1d..0d35ed72c6c6 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -201,11 +201,6 @@ static void fw_state_init(struct fw_state *fw_st)
 	fw_st->status = FW_STATUS_UNKNOWN;
 }
 
-static inline bool __fw_state_is_done(enum fw_status status)
-{
-	return status == FW_STATUS_DONE || status == FW_STATUS_ABORTED;
-}
-
 static int __fw_state_wait_common(struct fw_state *fw_st, long timeout)
 {
 	long ret;
-- 
2.15.0

[PATCH v2 07/23] firmware: use static inlines for state machine checking

[Luis R. Rodriguez]

This will allow us to do proper typechecking on users both of
values passed and return types expected.

While at it, change the parameter passed to be the struct fw_priv,
so we can move around the state machine variable as we see fit with
these helpers.

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

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 0d35ed72c6c6..3b506d375897 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -195,14 +195,17 @@ static inline long firmware_loading_timeout(void)
 	return loading_timeout > 0 ? loading_timeout * HZ : MAX_JIFFY_OFFSET;
 }
 
-static void fw_state_init(struct fw_state *fw_st)
+static void fw_state_init(struct fw_priv *fw_priv)
 {
+	struct fw_state *fw_st = &fw_priv->fw_st;
+
 	init_completion(&fw_st->completion);
 	fw_st->status = FW_STATUS_UNKNOWN;
 }
 
-static int __fw_state_wait_common(struct fw_state *fw_st, long timeout)
+static int __fw_state_wait_common(struct fw_priv *fw_priv, long timeout)
 {
+	struct fw_state *fw_st = &fw_priv->fw_st;
 	long ret;
 
 	ret = wait_for_completion_killable_timeout(&fw_st->completion, timeout);
@@ -214,40 +217,66 @@ static int __fw_state_wait_common(struct fw_state *fw_st, long timeout)
 	return ret < 0 ? ret : 0;
 }
 
-static void __fw_state_set(struct fw_state *fw_st,
+static void __fw_state_set(struct fw_priv *fw_priv,
 			   enum fw_status status)
 {
+	struct fw_state *fw_st = &fw_priv->fw_st;
+
 	WRITE_ONCE(fw_st->status, status);
 
 	if (status == FW_STATUS_DONE || status == FW_STATUS_ABORTED)
 		complete_all(&fw_st->completion);
 }
 
-#define fw_state_start(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)
+static inline void fw_state_start(struct fw_priv *fw_priv)
+{
+	__fw_state_set(fw_priv, FW_STATUS_LOADING);
+}
+
+static inline void fw_state_done(struct fw_priv *fw_priv)
+{
+	__fw_state_set(fw_priv, FW_STATUS_DONE);
+}
+
+static inline void fw_state_aborted(struct fw_priv *fw_priv)
+{
+	__fw_state_set(fw_priv, FW_STATUS_ABORTED);
+}
+
+static inline int fw_state_wait(struct fw_priv *fw_priv)
+{
+	return __fw_state_wait_common(fw_priv, MAX_SCHEDULE_TIMEOUT);
+}
 
-static int __fw_state_check(struct fw_state *fw_st, enum fw_status status)
+static bool __fw_state_check(struct fw_priv *fw_priv,
+			     enum fw_status status)
 {
+	struct fw_state *fw_st = &fw_priv->fw_st;
+
 	return fw_st->status == status;
 }
 
-#define fw_state_is_aborted(fw_st)				\
-	__fw_state_check(fw_st, FW_STATUS_ABORTED)
+static inline bool fw_state_is_aborted(struct fw_priv *fw_priv)
+{
+	return __fw_state_check(fw_priv, FW_STATUS_ABORTED);
+}
 
 #ifdef CONFIG_FW_LOADER_USER_HELPER
 
-#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_wait_timeout(fw_st, timeout)			\
-	__fw_state_wait_common(fw_st, timeout)
+static inline bool fw_state_is_done(struct fw_priv *fw_priv)
+{
+	return __fw_state_check(fw_priv, FW_STATUS_DONE);
+}
+
+static inline bool fw_state_is_loading(struct fw_priv *fw_priv)
+{
+	return __fw_state_check(fw_priv, FW_STATUS_LOADING);
+}
+
+static inline int fw_state_wait_timeout(struct fw_priv *fw_priv,  long timeout)
+{
+	return __fw_state_wait_common(fw_priv, timeout);
+}
 
 #endif /* CONFIG_FW_LOADER_USER_HELPER */
 
@@ -273,7 +302,7 @@ static struct fw_priv *__allocate_fw_priv(const char *fw_name,
 	fw_priv->fwc = fwc;
 	fw_priv->data = dbuf;
 	fw_priv->allocated_size = size;
-	fw_state_init(&fw_priv->fw_st);
+	fw_state_init(fw_priv);
 #ifdef CONFIG_FW_LOADER_USER_HELPER
 	INIT_LIST_HEAD(&fw_priv->pending_list);
 #endif
@@ -421,7 +450,7 @@ fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv)
 		}
 		dev_dbg(device, "direct-loading %s\n", fw_priv->fw_name);
 		fw_priv->size = size;
-		fw_state_done(&fw_priv->fw_st);
+		fw_state_done(fw_priv);
 		break;
 	}
 	__putname(path);
@@ -522,7 +551,7 @@ static int assign_fw(struct firmware *fw, struct device *device,
 	struct fw_priv *fw_priv = fw->priv;
 
 	mutex_lock(&fw_lock);
-	if (!fw_priv->size || fw_state_is_aborted(&fw_priv->fw_st)) {
+	if (!fw_priv->size || fw_state_is_aborted(fw_priv)) {
 		mutex_unlock(&fw_lock);
 		return -ENOENT;
 	}
@@ -577,11 +606,11 @@ static void __fw_load_abort(struct fw_priv *fw_priv)
 	 * There is a small window in which user can write to 'loading'
 	 * between loading done and disappearance of 'loading'
 	 */
-	if (fw_state_is_done(&fw_priv->fw_st))
+	if (fw_state_is_done(fw_priv))
 		return;
 
 	list_del_init(&fw_priv->pending_list);
-	fw_state_aborted(&fw_priv->fw_st);
+	fw_state_aborted(fw_priv);
 }
 
 static void fw_load_abort(struct fw_sysfs *fw_sysfs)
@@ -699,7 +728,7 @@ static ssize_t firmware_loading_show(struct device *dev,
 
 	mutex_lock(&fw_lock);
 	if (fw_sysfs->fw_priv)
-		loading = fw_state_is_loading(&fw_sysfs->fw_priv->fw_st);
+		loading = fw_state_is_loading(fw_sysfs->fw_priv);
 	mutex_unlock(&fw_lock);
 
 	return sprintf(buf, "%d\n", loading);
@@ -749,24 +778,24 @@ static ssize_t firmware_loading_store(struct device *dev,
 
 	mutex_lock(&fw_lock);
 	fw_priv = fw_sysfs->fw_priv;
-	if (fw_state_is_aborted(&fw_priv->fw_st))
+	if (fw_state_is_aborted(fw_priv))
 		goto out;
 
 	switch (loading) {
 	case 1:
 		/* discarding any previous partial load */
-		if (!fw_state_is_done(&fw_priv->fw_st)) {
+		if (!fw_state_is_done(fw_priv)) {
 			for (i = 0; i < fw_priv->nr_pages; i++)
 				__free_page(fw_priv->pages[i]);
 			vfree(fw_priv->pages);
 			fw_priv->pages = NULL;
 			fw_priv->page_array_size = 0;
 			fw_priv->nr_pages = 0;
-			fw_state_start(&fw_priv->fw_st);
+			fw_state_start(fw_priv);
 		}
 		break;
 	case 0:
-		if (fw_state_is_loading(&fw_priv->fw_st)) {
+		if (fw_state_is_loading(fw_priv)) {
 			int rc;
 
 			/*
@@ -790,10 +819,10 @@ static ssize_t firmware_loading_store(struct device *dev,
 			 */
 			list_del_init(&fw_priv->pending_list);
 			if (rc) {
-				fw_state_aborted(&fw_priv->fw_st);
+				fw_state_aborted(fw_priv);
 				written = rc;
 			} else {
-				fw_state_done(&fw_priv->fw_st);
+				fw_state_done(fw_priv);
 			}
 			break;
 		}
@@ -855,7 +884,7 @@ static ssize_t firmware_data_read(struct file *filp, struct kobject *kobj,
 
 	mutex_lock(&fw_lock);
 	fw_priv = fw_sysfs->fw_priv;
-	if (!fw_priv || fw_state_is_done(&fw_priv->fw_st)) {
+	if (!fw_priv || fw_state_is_done(fw_priv)) {
 		ret_count = -ENODEV;
 		goto out;
 	}
@@ -942,7 +971,7 @@ static ssize_t firmware_data_write(struct file *filp, struct kobject *kobj,
 
 	mutex_lock(&fw_lock);
 	fw_priv = fw_sysfs->fw_priv;
-	if (!fw_priv || fw_state_is_done(&fw_priv->fw_st)) {
+	if (!fw_priv || fw_state_is_done(fw_priv)) {
 		retval = -ENODEV;
 		goto out;
 	}
@@ -1055,14 +1084,14 @@ static int _request_firmware_load(struct fw_sysfs *fw_sysfs,
 		timeout = MAX_JIFFY_OFFSET;
 	}
 
-	retval = fw_state_wait_timeout(&fw_priv->fw_st, timeout);
+	retval = fw_state_wait_timeout(fw_priv, timeout);
 	if (retval < 0) {
 		mutex_lock(&fw_lock);
 		fw_load_abort(fw_sysfs);
 		mutex_unlock(&fw_lock);
 	}
 
-	if (fw_state_is_aborted(&fw_priv->fw_st)) {
+	if (fw_state_is_aborted(fw_priv)) {
 		if (retval == -ERESTARTSYS)
 			retval = -EINTR;
 		else
@@ -1173,7 +1202,7 @@ _request_firmware_prepare(struct firmware **firmware_p, const char *name,
 	firmware->priv = fw_priv;
 
 	if (ret > 0) {
-		ret = fw_state_wait(&fw_priv->fw_st);
+		ret = fw_state_wait(fw_priv);
 		if (!ret) {
 			fw_set_page_data(fw_priv, firmware);
 			return 0; /* assigned */
@@ -1203,8 +1232,8 @@ static void fw_abort_batch_reqs(struct firmware *fw)
 		return;
 
 	fw_priv = fw->priv;
-	if (!fw_state_is_aborted(&fw_priv->fw_st))
-		fw_state_aborted(&fw_priv->fw_st);
+	if (!fw_state_is_aborted(fw_priv))
+		fw_state_aborted(fw_priv);
 }
 
 /* called from request_firmware() and request_firmware_work_func() */
-- 
2.15.0

[PATCH v2 08/23] firmware: rename sysfs state checks with sysfs prefix

[Luis R. Rodriguez]

Doing this makes it clearer the states are only to be used
in the context of the sysfs fallback loading interface.

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

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 3b506d375897..dcc1281789e4 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -263,17 +263,17 @@ static inline bool fw_state_is_aborted(struct fw_priv *fw_priv)
 
 #ifdef CONFIG_FW_LOADER_USER_HELPER
 
-static inline bool fw_state_is_done(struct fw_priv *fw_priv)
+static inline bool fw_sysfs_done(struct fw_priv *fw_priv)
 {
 	return __fw_state_check(fw_priv, FW_STATUS_DONE);
 }
 
-static inline bool fw_state_is_loading(struct fw_priv *fw_priv)
+static inline bool fw_sysfs_loading(struct fw_priv *fw_priv)
 {
 	return __fw_state_check(fw_priv, FW_STATUS_LOADING);
 }
 
-static inline int fw_state_wait_timeout(struct fw_priv *fw_priv,  long timeout)
+static inline int fw_sysfs_wait_timeout(struct fw_priv *fw_priv,  long timeout)
 {
 	return __fw_state_wait_common(fw_priv, timeout);
 }
@@ -606,7 +606,7 @@ static void __fw_load_abort(struct fw_priv *fw_priv)
 	 * There is a small window in which user can write to 'loading'
 	 * between loading done and disappearance of 'loading'
 	 */
-	if (fw_state_is_done(fw_priv))
+	if (fw_sysfs_done(fw_priv))
 		return;
 
 	list_del_init(&fw_priv->pending_list);
@@ -728,7 +728,7 @@ static ssize_t firmware_loading_show(struct device *dev,
 
 	mutex_lock(&fw_lock);
 	if (fw_sysfs->fw_priv)
-		loading = fw_state_is_loading(fw_sysfs->fw_priv);
+		loading = fw_sysfs_loading(fw_sysfs->fw_priv);
 	mutex_unlock(&fw_lock);
 
 	return sprintf(buf, "%d\n", loading);
@@ -784,7 +784,7 @@ static ssize_t firmware_loading_store(struct device *dev,
 	switch (loading) {
 	case 1:
 		/* discarding any previous partial load */
-		if (!fw_state_is_done(fw_priv)) {
+		if (!fw_sysfs_done(fw_priv)) {
 			for (i = 0; i < fw_priv->nr_pages; i++)
 				__free_page(fw_priv->pages[i]);
 			vfree(fw_priv->pages);
@@ -795,7 +795,7 @@ static ssize_t firmware_loading_store(struct device *dev,
 		}
 		break;
 	case 0:
-		if (fw_state_is_loading(fw_priv)) {
+		if (fw_sysfs_loading(fw_priv)) {
 			int rc;
 
 			/*
@@ -884,7 +884,7 @@ static ssize_t firmware_data_read(struct file *filp, struct kobject *kobj,
 
 	mutex_lock(&fw_lock);
 	fw_priv = fw_sysfs->fw_priv;
-	if (!fw_priv || fw_state_is_done(fw_priv)) {
+	if (!fw_priv || fw_sysfs_done(fw_priv)) {
 		ret_count = -ENODEV;
 		goto out;
 	}
@@ -971,7 +971,7 @@ static ssize_t firmware_data_write(struct file *filp, struct kobject *kobj,
 
 	mutex_lock(&fw_lock);
 	fw_priv = fw_sysfs->fw_priv;
-	if (!fw_priv || fw_state_is_done(fw_priv)) {
+	if (!fw_priv || fw_sysfs_done(fw_priv)) {
 		retval = -ENODEV;
 		goto out;
 	}
@@ -1084,7 +1084,7 @@ static int _request_firmware_load(struct fw_sysfs *fw_sysfs,
 		timeout = MAX_JIFFY_OFFSET;
 	}
 
-	retval = fw_state_wait_timeout(fw_priv, timeout);
+	retval = fw_sysfs_wait_timeout(fw_priv, timeout);
 	if (retval < 0) {
 		mutex_lock(&fw_lock);
 		fw_load_abort(fw_sysfs);
-- 
2.15.0

[PATCH v2 09/23] firmware: use static inline for to_fw_priv()

[Luis R. Rodriguez]

This lets us type check the callers.

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

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index dcc1281789e4..4f64410fe7e6 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -125,7 +125,10 @@ struct fw_name_devm {
 	const char *name;
 };
 
-#define to_fw_priv(d) container_of(d, struct fw_priv, ref)
+static inline struct fw_priv *to_fw_priv(struct kref *ref)
+{
+	return container_of(ref, struct fw_priv, ref);
+}
 
 #define	FW_LOADER_NO_CACHE	0
 #define	FW_LOADER_START_CACHE	1
-- 
2.15.0

[PATCH v2 10/23] firmware: add helper to copy built-in data to pre-alloc buffer

[Luis R. Rodriguez]

This makes it clearer that the parameters passed are only used for
the preallocated buffer option, ie, when a caller uses:

	request_firmware_into_buf()

Otherwise this code won't run. We flip the logic just so the actual
prellocated buf code is not indented.

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

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 4f64410fe7e6..aba3f2cbe2f4 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -146,6 +146,14 @@ static struct firmware_cache fw_cache;
 extern struct builtin_fw __start_builtin_fw[];
 extern struct builtin_fw __end_builtin_fw[];
 
+static void fw_copy_to_prealloc_buf(struct firmware *fw,
+				    void *buf, size_t size)
+{
+	if (!buf || size < fw->size)
+		return;
+	memcpy(buf, fw->data, fw->size);
+}
+
 static bool fw_get_builtin_firmware(struct firmware *fw, const char *name,
 				    void *buf, size_t size)
 {
@@ -155,9 +163,8 @@ static bool fw_get_builtin_firmware(struct firmware *fw, const char *name,
 		if (strcmp(name, b_fw->name) == 0) {
 			fw->size = b_fw->size;
 			fw->data = b_fw->data;
+			fw_copy_to_prealloc_buf(fw, buf, size);
 
-			if (buf && fw->size <= size)
-				memcpy(buf, fw->data, fw->size);
 			return true;
 		}
 	}
-- 
2.15.0

[PATCH v2 11/23] firmware: provide helper for FW_OPT_USERHELPER

[Luis R. Rodriguez]

The macro FW_OPT_USERHELPER is only currently defined when
CONFIG_FW_LOADER_USER_HELPER is set. This is handled via an
ifdef around CONFIG_FW_LOADER_USER_HELPER. This makes reading
and understanding use FW_OPT_USERHELPER a bit convoluted.

Instead wrap the functionality implemented behind
CONFIG_FW_LOADER_USER_HELPER as we typically do in the
kernel.

Now when CONFIG_FW_LOADER_USER_HELPER is *not set*, then
simply the helper fw_sysfs_fallback() will not do anything.

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

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index aba3f2cbe2f4..316991429032 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -61,11 +61,7 @@ struct fw_state {
 /* firmware behavior options */
 #define FW_OPT_UEVENT	(1U << 0)
 #define FW_OPT_NOWAIT	(1U << 1)
-#ifdef CONFIG_FW_LOADER_USER_HELPER
 #define FW_OPT_USERHELPER	(1U << 2)
-#else
-#define FW_OPT_USERHELPER	0
-#endif
 #ifdef CONFIG_FW_LOADER_USER_HELPER_FALLBACK
 #define FW_OPT_FALLBACK		FW_OPT_USERHELPER
 #else
@@ -1158,12 +1154,25 @@ static int fw_load_from_user_helper(struct firmware *firmware,
 	return ret;
 }
 
+static int fw_sysfs_fallback(struct firmware *fw, const char *name,
+			    struct device *device,
+			    unsigned int opt_flags,
+			    int ret)
+{
+	if (!(opt_flags & FW_OPT_USERHELPER))
+		return ret;
+
+	dev_warn(device, "Falling back to user helper\n");
+	return fw_load_from_user_helper(fw, name, device, opt_flags);
+}
 #else /* CONFIG_FW_LOADER_USER_HELPER */
-static inline int
-fw_load_from_user_helper(struct firmware *firmware, const char *name,
-			 struct device *device, unsigned int opt_flags)
+static int fw_sysfs_fallback(struct firmware *fw, const char *name,
+			     struct device *device,
+			     unsigned int opt_flags,
+			     int ret)
 {
-	return -ENOENT;
+	/* Keep carrying over the same error */
+	return ret;
 }
 
 static inline void kill_pending_fw_fallback_reqs(bool only_kill_custom) { }
@@ -1273,11 +1282,7 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
 			dev_warn(device,
 				 "Direct firmware load for %s failed with error %d\n",
 				 name, ret);
-		if (opt_flags & FW_OPT_USERHELPER) {
-			dev_warn(device, "Falling back to user helper\n");
-			ret = fw_load_from_user_helper(fw, name, device,
-						       opt_flags);
-		}
+		ret = fw_sysfs_fallback(fw, name, device, opt_flags, ret);
 	} else
 		ret = assign_fw(fw, device, opt_flags);
 
-- 
2.15.0

[PATCH v2 12/23] firmware: replace #ifdef over FW_OPT_FALLBACK with function checks

[Luis R. Rodriguez]

Its not easy to follow the logic behind making FW_OPT_FALLBACK map
to an existing flag only if a kernel configuration option was set.
Its much easier to retpresent what was intended with function helpers
which make it clear that if CONFIG_FW_LOADER_USER_HELPER_FALLBACK is
set we force running the fallback mechanism unless a caller specifically
never wants to run it, such as request_firmware_direct().

Prior and after this change we upkeep the tradition:

CONFIG_FW_LOADER_USER_HELPER_FALLBACK
	request_firmware() force fallback
	request_firmware_into_buf() force fallback
	request_firmware_nowait() force fallback
	request_firmware_direct() always ignore fallback

!CONFIG_FW_LOADER_USER_HELPER_FALLBACK
	request_firmware() ignore fallback
	request_firmware_into_buf() ignore fallback
	request_firmware_nowait() depends on uevent flag
	request_firmware_direct() always ignore fallback

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

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 316991429032..43b97a8137f7 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -62,13 +62,9 @@ struct fw_state {
 #define FW_OPT_UEVENT	(1U << 0)
 #define FW_OPT_NOWAIT	(1U << 1)
 #define FW_OPT_USERHELPER	(1U << 2)
-#ifdef CONFIG_FW_LOADER_USER_HELPER_FALLBACK
-#define FW_OPT_FALLBACK		FW_OPT_USERHELPER
-#else
-#define FW_OPT_FALLBACK		0
-#endif
 #define FW_OPT_NO_WARN	(1U << 3)
 #define FW_OPT_NOCACHE	(1U << 4)
+#define FW_OPT_NOFALLBACK (1U << 5)
 
 struct firmware_cache {
 	/* firmware_buf instance will be added into the below list */
@@ -1154,12 +1150,34 @@ 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 (!(opt_flags & FW_OPT_USERHELPER))
+		return false;
+	return true;
+}
+#endif
+
+static bool fw_run_sysfs_fallback(unsigned int opt_flags)
+{
+	if ((opt_flags & FW_OPT_NOFALLBACK))
+		return false;
+
+	return fw_force_sysfs_fallback(opt_flags);
+}
+
 static int fw_sysfs_fallback(struct firmware *fw, const char *name,
 			    struct device *device,
 			    unsigned int opt_flags,
 			    int ret)
 {
-	if (!(opt_flags & FW_OPT_USERHELPER))
+	if (!fw_run_sysfs_fallback(opt_flags))
 		return ret;
 
 	dev_warn(device, "Falling back to user helper\n");
@@ -1326,7 +1344,7 @@ request_firmware(const struct firmware **firmware_p, const char *name,
 	/* Need to pin this module until return */
 	__module_get(THIS_MODULE);
 	ret = _request_firmware(firmware_p, name, device, NULL, 0,
-				FW_OPT_UEVENT | FW_OPT_FALLBACK);
+				FW_OPT_UEVENT);
 	module_put(THIS_MODULE);
 	return ret;
 }
@@ -1350,7 +1368,8 @@ int request_firmware_direct(const struct firmware **firmware_p,
 
 	__module_get(THIS_MODULE);
 	ret = _request_firmware(firmware_p, name, device, NULL, 0,
-				FW_OPT_UEVENT | FW_OPT_NO_WARN);
+				FW_OPT_UEVENT | FW_OPT_NO_WARN |
+				FW_OPT_NOFALLBACK);
 	module_put(THIS_MODULE);
 	return ret;
 }
@@ -1379,8 +1398,7 @@ request_firmware_into_buf(const struct firmware **firmware_p, const char *name,
 
 	__module_get(THIS_MODULE);
 	ret = _request_firmware(firmware_p, name, device, buf, size,
-				FW_OPT_UEVENT | FW_OPT_FALLBACK |
-				FW_OPT_NOCACHE);
+				FW_OPT_UEVENT | FW_OPT_NOCACHE);
 	module_put(THIS_MODULE);
 	return ret;
 }
@@ -1472,7 +1490,7 @@ request_firmware_nowait(
 	fw_work->device = device;
 	fw_work->context = context;
 	fw_work->cont = cont;
-	fw_work->opt_flags = FW_OPT_NOWAIT | FW_OPT_FALLBACK |
+	fw_work->opt_flags = FW_OPT_NOWAIT |
 		(uevent ? FW_OPT_UEVENT : FW_OPT_USERHELPER);
 
 	if (!try_module_get(module)) {
-- 
2.15.0

[PATCH v2 13/23] test_firmware: wrap sysfs timeout test into helper

[Luis R. Rodriguez]

This cannot run on all kernel builds. This will help us later
skip this test on kernel configs where non-applicable.

Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 tools/testing/selftests/firmware/fw_fallback.sh | 69 +++++++++++++------------
 1 file changed, 37 insertions(+), 32 deletions(-)

diff --git a/tools/testing/selftests/firmware/fw_fallback.sh b/tools/testing/selftests/firmware/fw_fallback.sh
index 34a42c68ebfb..e364631837d6 100755
--- a/tools/testing/selftests/firmware/fw_fallback.sh
+++ b/tools/testing/selftests/firmware/fw_fallback.sh
@@ -175,39 +175,44 @@ trap "test_finish" EXIT
 echo "ABCD0123" >"$FW"
 NAME=$(basename "$FW")
 
-DEVPATH="$DIR"/"nope-$NAME"/loading
-
-# Test failure when doing nothing (timeout works).
-echo -n 2 >/sys/class/firmware/timeout
-echo -n "nope-$NAME" >"$DIR"/trigger_request 2>/dev/null &
-
-# Give the kernel some time to load the loading file, must be less
-# than the timeout above.
-sleep 1
-if [ ! -f $DEVPATH ]; then
-	echo "$0: fallback mechanism immediately cancelled"
-	echo ""
-	echo "The file never appeared: $DEVPATH"
-	echo ""
-	echo "This might be a distribution udev rule setup by your distribution"
-	echo "to immediately cancel all fallback requests, this must be"
-	echo "removed before running these tests. To confirm look for"
-	echo "a firmware rule like /lib/udev/rules.d/50-firmware.rules"
-	echo "and see if you have something like this:"
-	echo ""
-	echo "SUBSYSTEM==\"firmware\", ACTION==\"add\", ATTR{loading}=\"-1\""
-	echo ""
-	echo "If you do remove this file or comment out this line before"
-	echo "proceeding with these tests."
-	exit 1
-fi
+test_syfs_timeout()
+{
+	DEVPATH="$DIR"/"nope-$NAME"/loading
+
+	# Test failure when doing nothing (timeout works).
+	echo -n 2 >/sys/class/firmware/timeout
+	echo -n "nope-$NAME" >"$DIR"/trigger_request 2>/dev/null &
+
+	# Give the kernel some time to load the loading file, must be less
+	# than the timeout above.
+	sleep 1
+	if [ ! -f $DEVPATH ]; then
+		echo "$0: fallback mechanism immediately cancelled"
+		echo ""
+		echo "The file never appeared: $DEVPATH"
+		echo ""
+		echo "This might be a distribution udev rule setup by your distribution"
+		echo "to immediately cancel all fallback requests, this must be"
+		echo "removed before running these tests. To confirm look for"
+		echo "a firmware rule like /lib/udev/rules.d/50-firmware.rules"
+		echo "and see if you have something like this:"
+		echo ""
+		echo "SUBSYSTEM==\"firmware\", ACTION==\"add\", ATTR{loading}=\"-1\""
+		echo ""
+		echo "If you do remove this file or comment out this line before"
+		echo "proceeding with these tests."
+		exit 1
+	fi
 
-if diff -q "$FW" /dev/test_firmware >/dev/null ; then
-	echo "$0: firmware was not expected to match" >&2
-	exit 1
-else
-	echo "$0: timeout works"
-fi
+	if diff -q "$FW" /dev/test_firmware >/dev/null ; then
+		echo "$0: firmware was not expected to match" >&2
+		exit 1
+	else
+		echo "$0: timeout works"
+	fi
+}
+
+test_syfs_timeout
 
 # Put timeout high enough for us to do work but not so long that failures
 # slow down this test too much.
-- 
2.15.0

[PATCH v2 14/23] test_firmware: wrap basic sysfs fallback tests into helper

[Luis R. Rodriguez]

These cannot run on all kernel builds. This will help us later
skip this test on kernel configs where non-applicable.

Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 tools/testing/selftests/firmware/fw_fallback.sh | 78 +++++++++++++------------
 1 file changed, 41 insertions(+), 37 deletions(-)

diff --git a/tools/testing/selftests/firmware/fw_fallback.sh b/tools/testing/selftests/firmware/fw_fallback.sh
index e364631837d6..0d4527c5e8a4 100755
--- a/tools/testing/selftests/firmware/fw_fallback.sh
+++ b/tools/testing/selftests/firmware/fw_fallback.sh
@@ -212,37 +212,51 @@ test_syfs_timeout()
 	fi
 }
 
-test_syfs_timeout
+run_sysfs_main_tests()
+{
+	test_syfs_timeout
+	# Put timeout high enough for us to do work but not so long that failures
+	# slow down this test too much.
+	echo 4 >/sys/class/firmware/timeout
 
-# Put timeout high enough for us to do work but not so long that failures
-# slow down this test too much.
-echo 4 >/sys/class/firmware/timeout
+	# Load this script instead of the desired firmware.
+	load_fw "$NAME" "$0"
+	if diff -q "$FW" /dev/test_firmware >/dev/null ; then
+		echo "$0: firmware was not expected to match" >&2
+		exit 1
+	else
+		echo "$0: firmware comparison works"
+	fi
 
-# Load this script instead of the desired firmware.
-load_fw "$NAME" "$0"
-if diff -q "$FW" /dev/test_firmware >/dev/null ; then
-	echo "$0: firmware was not expected to match" >&2
-	exit 1
-else
-	echo "$0: firmware comparison works"
-fi
+	# Do a proper load, which should work correctly.
+	load_fw "$NAME" "$FW"
+	if ! diff -q "$FW" /dev/test_firmware >/dev/null ; then
+		echo "$0: firmware was not loaded" >&2
+		exit 1
+	else
+		echo "$0: fallback mechanism works"
+	fi
 
-# Do a proper load, which should work correctly.
-load_fw "$NAME" "$FW"
-if ! diff -q "$FW" /dev/test_firmware >/dev/null ; then
-	echo "$0: firmware was not loaded" >&2
-	exit 1
-else
-	echo "$0: fallback mechanism works"
-fi
+	load_fw_cancel "nope-$NAME" "$FW"
+	if diff -q "$FW" /dev/test_firmware >/dev/null ; then
+		echo "$0: firmware was expected to be cancelled" >&2
+		exit 1
+	else
+		echo "$0: cancelling fallback mechanism works"
+	fi
 
-load_fw_cancel "nope-$NAME" "$FW"
-if diff -q "$FW" /dev/test_firmware >/dev/null ; then
-	echo "$0: firmware was expected to be cancelled" >&2
-	exit 1
-else
-	echo "$0: cancelling fallback mechanism works"
-fi
+	set +e
+	load_fw_fallback_with_child "nope-signal-$NAME" "$FW"
+	if [ "$?" -eq 0 ]; then
+		echo "$0: SIGCHLD on sync ignored as expected" >&2
+	else
+		echo "$0: error - sync firmware request cancelled due to SIGCHLD" >&2
+		exit 1
+	fi
+	set -e
+}
+
+run_sysfs_main_tests
 
 if load_fw_custom "$NAME" "$FW" ; then
 	if ! diff -q "$FW" /dev/test_firmware >/dev/null ; then
@@ -262,14 +276,4 @@ if load_fw_custom_cancel "nope-$NAME" "$FW" ; then
 	fi
 fi
 
-set +e
-load_fw_fallback_with_child "nope-signal-$NAME" "$FW"
-if [ "$?" -eq 0 ]; then
-	echo "$0: SIGCHLD on sync ignored as expected" >&2
-else
-	echo "$0: error - sync firmware request cancelled due to SIGCHLD" >&2
-	exit 1
-fi
-set -e
-
 exit 0
-- 
2.15.0

[PATCH v2 15/23] test_firmware: wrap custom sysfs load tests into helper

[Luis R. Rodriguez]

These can run on certain kernel configs. This will allow
us later to enable these tests under the right kernel
configurations.

Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 tools/testing/selftests/firmware/fw_fallback.sh | 43 ++++++++++++++++---------
 1 file changed, 28 insertions(+), 15 deletions(-)

diff --git a/tools/testing/selftests/firmware/fw_fallback.sh b/tools/testing/selftests/firmware/fw_fallback.sh
index 0d4527c5e8a4..722cad91df74 100755
--- a/tools/testing/selftests/firmware/fw_fallback.sh
+++ b/tools/testing/selftests/firmware/fw_fallback.sh
@@ -256,24 +256,37 @@ run_sysfs_main_tests()
 	set -e
 }
 
-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
+			echo "$0: firmware was not loaded" >&2
+			exit 1
+		else
+			echo "$0: custom fallback loading mechanism works"
+		fi
+	fi
 
-if load_fw_custom "$NAME" "$FW" ; then
-	if ! diff -q "$FW" /dev/test_firmware >/dev/null ; then
-		echo "$0: firmware was not loaded" >&2
-		exit 1
-	else
-		echo "$0: custom fallback loading mechanism works"
+	if load_fw_custom "$NAME" "$FW" ; then
+		if ! diff -q "$FW" /dev/test_firmware >/dev/null ; then
+			echo "$0: firmware was not loaded" >&2
+			exit 1
+		else
+			echo "$0: custom fallback loading mechanism works"
+		fi
 	fi
-fi
 
-if load_fw_custom_cancel "nope-$NAME" "$FW" ; then
-	if diff -q "$FW" /dev/test_firmware >/dev/null ; then
-		echo "$0: firmware was expected to be cancelled" >&2
-		exit 1
-	else
-		echo "$0: cancelling custom fallback mechanism works"
+	if load_fw_custom_cancel "nope-$NAME" "$FW" ; then
+		if diff -q "$FW" /dev/test_firmware >/dev/null ; then
+			echo "$0: firmware was expected to be cancelled" >&2
+			exit 1
+		else
+			echo "$0: cancelling custom fallback mechanism works"
+		fi
 	fi
-fi
+}
+
+run_sysfs_main_tests
+run_sysfs_custom_load_tests
 
 exit 0
-- 
2.15.0

[PATCH v2 16/23] test_firmware: enable custom fallback testing on limited kernel configs

[Luis R. Rodriguez]

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 <mcgrof@kernel.org>
---
 tools/testing/selftests/firmware/config         |  4 +++
 tools/testing/selftests/firmware/fw_fallback.sh | 45 ++++++++++++++++++++++++-
 2 files changed, 48 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/firmware/config b/tools/testing/selftests/firmware/config
index c8137f70e291..bf634dda0720 100644
--- a/tools/testing/selftests/firmware/config
+++ b/tools/testing/selftests/firmware/config
@@ -1 +1,5 @@
 CONFIG_TEST_FIRMWARE=y
+CONFIG_FW_LOADER=y
+CONFIG_FW_LOADER_USER_HELPER=y
+CONFIG_IKCONFIG=y
+CONFIG_IKCONFIG_PROC=y
diff --git a/tools/testing/selftests/firmware/fw_fallback.sh b/tools/testing/selftests/firmware/fw_fallback.sh
index 722cad91df74..a42e437363d9 100755
--- a/tools/testing/selftests/firmware/fw_fallback.sh
+++ b/tools/testing/selftests/firmware/fw_fallback.sh
@@ -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
+			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
 
@@ -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.15.0

[PATCH v2 17/23] test_firmware: replace syfs fallback check with kconfig_has helper

[Luis R. Rodriguez]

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 <mcgrof@kernel.org>
---
 tools/testing/selftests/firmware/fw_fallback.sh | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/tools/testing/selftests/firmware/fw_fallback.sh b/tools/testing/selftests/firmware/fw_fallback.sh
index a42e437363d9..40b6c1d3e832 100755
--- a/tools/testing/selftests/firmware/fw_fallback.sh
+++ b/tools/testing/selftests/firmware/fw_fallback.sh
@@ -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.15.0

[PATCH v2 18/23] firmware: enable to split firmware_class into separate target files

[Luis R. Rodriguez]

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.

Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 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%)

diff --git a/drivers/base/Makefile b/drivers/base/Makefile
index e32a52490051..f261143fafbf 100644
--- a/drivers/base/Makefile
+++ b/drivers/base/Makefile
@@ -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)
diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_loader.c
similarity index 100%
rename from drivers/base/firmware_class.c
rename to drivers/base/firmware_loader.c
-- 
2.15.0

[PATCH v2 19/23] firmware: add debug facility to emulate forcing sysfs fallback

[Luis R. Rodriguez]

The CONFIG_FW_LOADER_USER_HELPER_FALLBACK kernel config is not
enabled my major distros, however its known to be enabled on
some Android kernels. Testing the firmware API properly then
means to test at least two different kernel configurations.

Since CONFIG_FW_LOADER_USER_HELPER_FALLBACK only does a simple
enablement under certain situations we can just emulate this
behaviour through a debugfs knob. This enables testing two of
the tree possible kernel configs with the firmware loader with
one kernel build.

Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 drivers/base/Kconfig           |  6 ++++++
 drivers/base/Makefile          |  1 +
 drivers/base/firmware_debug.c  | 34 ++++++++++++++++++++++++++++++++
 drivers/base/firmware_debug.h  | 44 ++++++++++++++++++++++++++++++++++++++++++
 drivers/base/firmware_loader.c | 13 ++++++++++++-
 5 files changed, 97 insertions(+), 1 deletion(-)
 create mode 100644 drivers/base/firmware_debug.c
 create mode 100644 drivers/base/firmware_debug.h

diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
index 2f6614c9a229..ab6889ac6787 100644
--- a/drivers/base/Kconfig
+++ b/drivers/base/Kconfig
@@ -149,6 +149,12 @@ config EXTRA_FIRMWARE_DIR
 config FW_LOADER_USER_HELPER
 	bool
 
+config FW_LOADER_DEBUG
+	bool "Firmware loader debugging interface"
+	help
+	  If you are hacking on the firmware API of the kernel, firmware_class,
+	  you can enable this to help debug the kernel.
+
 config FW_LOADER_USER_HELPER_FALLBACK
 	bool "Fallback user-helper invocation for firmware loading"
 	depends on FW_LOADER
diff --git a/drivers/base/Makefile b/drivers/base/Makefile
index f261143fafbf..64eaa4f9cb02 100644
--- a/drivers/base/Makefile
+++ b/drivers/base/Makefile
@@ -14,6 +14,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_DEBUG) += firmware_debug.o
 obj-$(CONFIG_NUMA)	+= node.o
 obj-$(CONFIG_MEMORY_HOTPLUG_SPARSE) += memory.o
 ifeq ($(CONFIG_SYSFS),y)
diff --git a/drivers/base/firmware_debug.c b/drivers/base/firmware_debug.c
new file mode 100644
index 000000000000..f2817eb6f480
--- /dev/null
+++ b/drivers/base/firmware_debug.c
@@ -0,0 +1,34 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Firmware dubugging interface */
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/debugfs.h>
+#include "firmware_debug.h"
+
+struct firmware_debug fw_debug;
+
+static struct dentry *debugfs_firmware;
+
+int __init register_fw_debugfs(void)
+{
+	debugfs_firmware = debugfs_create_dir("firmware", NULL);
+	if (!debugfs_firmware)
+		return -ENOMEM;
+
+	if (!debugfs_create_bool("force_sysfs_fallback", S_IRUSR | S_IWUSR,
+				 debugfs_firmware,
+				 &fw_debug.force_sysfs_fallback))
+		goto err_out;
+
+	return 0;
+err_out:
+	debugfs_remove_recursive(debugfs_firmware);
+	debugfs_firmware = NULL;
+	return -ENOMEM;
+}
+
+void unregister_fw_debugfs(void)
+{
+	debugfs_remove_recursive(debugfs_firmware);
+	debugfs_firmware = NULL;
+}
diff --git a/drivers/base/firmware_debug.h b/drivers/base/firmware_debug.h
new file mode 100644
index 000000000000..bc41bbca9a54
--- /dev/null
+++ b/drivers/base/firmware_debug.h
@@ -0,0 +1,44 @@
+// SPDX-License-Identifier: GPL-2.0
+#ifndef __FIRMWARE_DEBUG_H
+#define __FIRMWARE_DEBUG_H
+
+#ifdef CONFIG_FW_LOADER_DEBUG
+/**
+ * struct firmware_debug - firmware debugging configuration
+ *
+ * Provided to help debug the firmware API.
+ *
+ * @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.
+ */
+struct firmware_debug {
+	bool force_sysfs_fallback;
+};
+
+extern struct firmware_debug fw_debug;
+
+int register_fw_debugfs(void);
+void unregister_fw_debugfs(void);
+
+static inline bool fw_debug_force_sysfs_fallback(void)
+{
+	return fw_debug.force_sysfs_fallback;
+}
+#else
+static inline int register_fw_debugfs(void)
+{
+	return 0;
+}
+static inline void unregister_fw_debugfs(void)
+{
+}
+
+static inline bool fw_debug_force_sysfs_fallback(void)
+{
+	return false;
+}
+#endif /* CONFIG_FW_LOADER_DEBUG */
+
+#endif /* __FIRMWARE_DEBUG_H */
diff --git a/drivers/base/firmware_loader.c b/drivers/base/firmware_loader.c
index 43b97a8137f7..b2b52ba9f245 100644
--- a/drivers/base/firmware_loader.c
+++ b/drivers/base/firmware_loader.c
@@ -36,6 +36,7 @@
 #include <generated/utsrelease.h>
 
 #include "base.h"
+#include "firmware_debug.h"
 
 MODULE_AUTHOR("Manuel Estrada Sainz");
 MODULE_DESCRIPTION("Multi purpose firmware loading support");
@@ -1158,6 +1159,9 @@ static bool fw_force_sysfs_fallback(unsigned int opt_flags)
 #else
 static bool fw_force_sysfs_fallback(unsigned int opt_flags)
 {
+	if (fw_debug_force_sysfs_fallback())
+		return true;
+
 	if (!(opt_flags & FW_OPT_USERHELPER))
 		return false;
 	return true;
@@ -1913,10 +1917,14 @@ static int __init firmware_class_init(void)
 	/* No need to unfold these on exit */
 	fw_cache_init();
 
-	ret = register_fw_pm_ops();
+	ret = register_fw_debugfs();
 	if (ret)
 		return ret;
 
+	ret = register_fw_pm_ops();
+	if (ret)
+		goto out_debugfs;
+
 	ret = register_reboot_notifier(&fw_shutdown_nb);
 	if (ret)
 		goto out;
@@ -1925,6 +1933,8 @@ static int __init firmware_class_init(void)
 
 out:
 	unregister_fw_pm_ops();
+out_debugfs:
+	unregister_fw_debugfs();
 	return ret;
 }
 
@@ -1933,6 +1943,7 @@ static void __exit firmware_class_exit(void)
 	unregister_fw_pm_ops();
 	unregister_reboot_notifier(&fw_shutdown_nb);
 	unregister_sysfs_loader();
+	unregister_fw_debugfs();
 }
 
 fs_initcall(firmware_class_init);
-- 
2.15.0

[PATCH v2 20/23] firmware: add debug facility to emulate disabling sysfs fallback

[Luis R. Rodriguez]

One can build a kernel without CONFIG_FW_LOADER_USER_HELPER being
enabled, but testing it requires a unique kernel configuration most
folks do not enable. This makes it hard to test.

Provide a debugfs facility to enable us to emulate such kernels.

Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 drivers/base/firmware_debug.c  |  5 +++++
 drivers/base/firmware_debug.h  | 14 ++++++++++++++
 drivers/base/firmware_loader.c |  5 +++++
 3 files changed, 24 insertions(+)

diff --git a/drivers/base/firmware_debug.c b/drivers/base/firmware_debug.c
index f2817eb6f480..9332f0acfa8f 100644
--- a/drivers/base/firmware_debug.c
+++ b/drivers/base/firmware_debug.c
@@ -20,6 +20,11 @@ int __init register_fw_debugfs(void)
 				 &fw_debug.force_sysfs_fallback))
 		goto err_out;
 
+	if (!debugfs_create_bool("ignore_sysfs_fallback", S_IRUSR | S_IWUSR,
+				 debugfs_firmware,
+				 &fw_debug.ignore_sysfs_fallback))
+		goto err_out;
+
 	return 0;
 err_out:
 	debugfs_remove_recursive(debugfs_firmware);
diff --git a/drivers/base/firmware_debug.h b/drivers/base/firmware_debug.h
index bc41bbca9a54..5f551d8b9003 100644
--- a/drivers/base/firmware_debug.h
+++ b/drivers/base/firmware_debug.h
@@ -12,9 +12,13 @@
  *	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.
  */
 struct firmware_debug {
 	bool force_sysfs_fallback;
+	bool ignore_sysfs_fallback;
 };
 
 extern struct firmware_debug fw_debug;
@@ -26,6 +30,11 @@ static inline bool fw_debug_force_sysfs_fallback(void)
 {
 	return fw_debug.force_sysfs_fallback;
 }
+
+static inline bool fw_debug_ignore_sysfs_fallback(void)
+{
+	return fw_debug.ignore_sysfs_fallback;
+}
 #else
 static inline int register_fw_debugfs(void)
 {
@@ -39,6 +48,11 @@ static inline bool fw_debug_force_sysfs_fallback(void)
 {
 	return false;
 }
+
+static inline bool fw_debug_ignore_sysfs_fallback(void)
+{
+	return false;
+}
 #endif /* CONFIG_FW_LOADER_DEBUG */
 
 #endif /* __FIRMWARE_DEBUG_H */
diff --git a/drivers/base/firmware_loader.c b/drivers/base/firmware_loader.c
index b2b52ba9f245..6c44ede98be0 100644
--- a/drivers/base/firmware_loader.c
+++ b/drivers/base/firmware_loader.c
@@ -1170,6 +1170,11 @@ static bool fw_force_sysfs_fallback(unsigned int opt_flags)
 
 static bool fw_run_sysfs_fallback(unsigned int opt_flags)
 {
+	if (fw_debug_ignore_sysfs_fallback()) {
+		pr_info("Ignoring firmware sysfs fallback due to debugfs knob\n");
+		return false;
+	}
+
 	if ((opt_flags & FW_OPT_NOFALLBACK))
 		return false;
 
-- 
2.15.0

[PATCH v2 21/23] test_firmware: add a library for shared helpers

[Luis R. Rodriguez]

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 <mcgrof@kernel.org>
---
 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

diff --git a/tools/testing/selftests/firmware/fw_fallback.sh b/tools/testing/selftests/firmware/fw_fallback.sh
index 40b6c1d3e832..0db1c09a6bcc 100755
--- a/tools/testing/selftests/firmware/fw_fallback.sh
+++ b/tools/testing/selftests/firmware/fw_fallback.sh
@@ -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()
 {
diff --git a/tools/testing/selftests/firmware/fw_filesystem.sh b/tools/testing/selftests/firmware/fw_filesystem.sh
index f9508e1a4058..7f47877fa7fa 100755
--- a/tools/testing/selftests/firmware/fw_filesystem.sh
+++ b/tools/testing/selftests/firmware/fw_filesystem.sh
@@ -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
diff --git a/tools/testing/selftests/firmware/fw_lib.sh b/tools/testing/selftests/firmware/fw_lib.sh
new file mode 100755
index 000000000000..0702dbf0f06b
--- /dev/null
+++ b/tools/testing/selftests/firmware/fw_lib.sh
@@ -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.15.0

[PATCH v2 22/23] test_firmware: test the 3 firmware kernel configs using debugfs

[Luis R. Rodriguez]

This enables support to test the three different kernel
configurations by using the new firmware debugfs facility.

We can now test all known kernel configs with one kernel build
and one run.

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_DEBUG
we cannot run these dynamic tests, so just run both scripts just
as we used to before.

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

diff --git a/tools/testing/selftests/firmware/Makefile b/tools/testing/selftests/firmware/Makefile
index 1894d625af2d..826f38d5dd19 100644
--- a/tools/testing/selftests/firmware/Makefile
+++ b/tools/testing/selftests/firmware/Makefile
@@ -3,7 +3,7 @@
 # No binaries, but make sure arg-less "make" doesn't trigger "run_tests"
 all:
 
-TEST_PROGS := fw_filesystem.sh fw_fallback.sh
+TEST_PROGS := fw_run_tests.sh
 
 include ../lib.mk
 
diff --git a/tools/testing/selftests/firmware/config b/tools/testing/selftests/firmware/config
index bf634dda0720..eef53c9ed631 100644
--- a/tools/testing/selftests/firmware/config
+++ b/tools/testing/selftests/firmware/config
@@ -1,5 +1,6 @@
 CONFIG_TEST_FIRMWARE=y
 CONFIG_FW_LOADER=y
 CONFIG_FW_LOADER_USER_HELPER=y
+CONFIG_FW_LOADER_DEBUG=y
 CONFIG_IKCONFIG=y
 CONFIG_IKCONFIG_PROC=y
diff --git a/tools/testing/selftests/firmware/fw_lib.sh b/tools/testing/selftests/firmware/fw_lib.sh
index 0702dbf0f06b..ada60c06a453 100755
--- a/tools/testing/selftests/firmware/fw_lib.sh
+++ b/tools/testing/selftests/firmware/fw_lib.sh
@@ -47,6 +47,35 @@ 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)
+	DEBUGFS_FW_IGNORE_SYSFS_FALLBACK="N"
+	DEBUGFS_FW_FORCE_SYSFS_FALLBACK="N"
+
+	if [ -z $DEBUGFS_DIR ]; then
+		DEBUGFS_DIR="/sys/kernel/debug/"
+	fi
+
+	FW_DEBUGFS="/sys/kernel/debug/firmware/"
+	FW_FORCE_SYSFS_FALLBACK="$FW_DEBUGFS/force_sysfs_fallback"
+	FW_IGNORE_SYSFS_FALLBACK="$FW_DEBUGFS/ignore_sysfs_fallback"
+	HAS_DEBUGFS="no"
+
+	if [ -f $FW_FORCE_SYSFS_FALLBACK ]; then
+		DEBUGFS_FW_FORCE_SYSFS_FALLBACK=$(cat $FW_FORCE_SYSFS_FALLBACK)
+	fi
+
+	if [ -f $FW_IGNORE_SYSFS_FALLBACK ]; then
+		DEBUGFS_FW_IGNORE_SYSFS_FALLBACK=$(cat $FW_IGNORE_SYSFS_FALLBACK)
+	fi
+
+	if [ "$DEBUGFS_FW_IGNORE_SYSFS_FALLBACK" = "Y" ]; then
+		HAS_FW_LOADER_USER_HELPER_FALLBACK="no"
+		HAS_FW_LOADER_USER_HELPER="no"
+	fi
+
+	if [ "$DEBUGFS_FW_FORCE_SYSFS_FALLBACK" = "Y" ]; 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 +105,30 @@ setup_tmp_file()
 	fi
 }
 
+debugfs_set_force_sysfs_fallback()
+{
+	if [ -f $FW_FORCE_SYSFS_FALLBACK ]; then
+		echo -n $1 > $FW_FORCE_SYSFS_FALLBACK
+		DEBUGFS_FW_FORCE_SYSFS_FALLBACK=$(cat $FW_FORCE_SYSFS_FALLBACK)
+		check_setup
+	fi
+}
+
+debugfs_set_ignore_sysfs_fallback()
+{
+	if [ -f $FW_IGNORE_SYSFS_FALLBACK ]; then
+		echo -n $1 > $FW_IGNORE_SYSFS_FALLBACK
+		DEBUGFS_FW_IGNORE_SYSFS_FALLBACK=$(cat $FW_IGNORE_SYSFS_FALLBACK)
+		check_setup
+	fi
+}
+
+debugfs_restore_defaults()
+{
+	debugfs_set_force_sysfs_fallback N
+	debugfs_set_ignore_sysfs_fallback N
+}
+
 test_finish()
 {
 	if [ "$HAS_FW_LOADER_USER_HELPER" = "yes" ]; then
@@ -93,6 +146,7 @@ test_finish()
 	if [ -d $FWPATH ]; then
 		rm -rf "$FWPATH"
 	fi
+	debugfs_restore_defaults
 }
 
 kconfig_has()
diff --git a/tools/testing/selftests/firmware/fw_run_tests.sh b/tools/testing/selftests/firmware/fw_run_tests.sh
new file mode 100755
index 000000000000..845facf6392d
--- /dev/null
+++ b/tools/testing/selftests/firmware/fw_run_tests.sh
@@ -0,0 +1,67 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+
+# This runs all known tests across all known possible configurations we could
+# emulate in one run.
+
+set -e
+
+TEST_DIR=$(dirname $0)
+source $TEST_DIR/fw_lib.sh
+
+run_tests()
+{
+	$TEST_DIR/fw_filesystem.sh
+	$TEST_DIR/fw_fallback.sh
+}
+
+run_test_config_0001()
+{
+	echo "-----------------------------------------------------"
+	echo "Running kernel configuration test 1 -- rare"
+	echo "Emulates:"
+	echo "CONFIG_FW_LOADER=y"
+	echo "CONFIG_FW_LOADER_USER_HELPER=n"
+	echo "CONFIG_FW_LOADER_USER_HELPER_FALLBACK=n"
+	debugfs_set_force_sysfs_fallback N
+	debugfs_set_ignore_sysfs_fallback Y
+	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"
+	debugfs_set_force_sysfs_fallback N
+	debugfs_set_ignore_sysfs_fallback N
+	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"
+	debugfs_set_force_sysfs_fallback Y
+	debugfs_set_ignore_sysfs_fallback N
+	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_tests
+fi
-- 
2.15.0

[PATCH v2 23/23] firmware: cleanup - group and document up private firmware parameters

[Luis R. Rodriguez]

The firmware API has a slew of private options available, which can
sometimes be hard to understand. When new functionality is introduced
we also tend to have modify a slew of internal helpers.

Just stuff all common private requirements into its own data structure
and move features into properly defined flags which can then be carefully
documented. This:

  o reduces the amount of changes we have make as we advance functionality
  o helps remove the #ifdef mess we had created for private features

The above benefits makes the code much easier to understand and maintain.

This cleanup introduces no functional changes. It has been tested using
the firmware selftests [0] [1] against the three main kernel configurations
which we care about:

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

[0] tools/testing/selftests/firmware/fw_fallback.sh
[1] tools/testing/selftests/firmware/fw_filesystem.sh

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

diff --git a/drivers/base/firmware_loader.c b/drivers/base/firmware_loader.c
index 6c44ede98be0..8a1d028f24f4 100644
--- a/drivers/base/firmware_loader.c
+++ b/drivers/base/firmware_loader.c
@@ -59,14 +59,6 @@ struct fw_state {
 	enum fw_status status;
 };
 
-/* firmware behavior options */
-#define FW_OPT_UEVENT	(1U << 0)
-#define FW_OPT_NOWAIT	(1U << 1)
-#define FW_OPT_USERHELPER	(1U << 2)
-#define FW_OPT_NO_WARN	(1U << 3)
-#define FW_OPT_NOCACHE	(1U << 4)
-#define FW_OPT_NOFALLBACK (1U << 5)
-
 struct firmware_cache {
 	/* firmware_buf instance will be added into the below list */
 	spinlock_t lock;
@@ -89,14 +81,80 @@ struct firmware_cache {
 #endif
 };
 
+/**
+ * enum fw_api_mode - firmware API mode of operation
+ * @FW_API_SYNC: used to determine if we should look for the firmware file
+ *	immediatley.
+ * @FW_API_ASYNC: used to determine if we should schedule the search for
+ *	your firmware file to be run at a later time.
+ */
+enum fw_api_mode {
+	FW_API_SYNC = 0,
+	FW_API_ASYNC,
+};
+
+/**
+ * enum fw_priv_flags - private features only used internally
+ *
+ * @FW_PRIV_FALLBACK: specifies that the firmware request
+ *	will use a fallback mechanism if the kernel's direct filesystem
+ *	lookup failed to find the requested firmware. If the flag
+ *	%FW_PRIV_FALLBACK is set but the flag
+ *	%FW_PRIV_FALLBACK_UEVENT is not set, it means the caller
+ *	is relying on a custom fallback mechanism for firmwarwe lookup as a
+ *	fallback mechanism. The custom fallback mechanism is expected to find
+ *	any found firmware using the exposed sysfs interface of the
+ *	firmware_class.  Since the custom fallback mechanism is not compatible
+ *	with the internal caching mechanism for firmware lookups at resume,
+ *	caching will be disabled when the custom fallback mechanism is used.
+ * @FW_PRIV_FALLBACK_UEVENT: indicates that the fallback mechanism
+ *	this firmware request will rely on will be that of having the kernel
+ *	issue a uevent to userspace. Userspace in turn is expected to be
+ *	monitoring for uevents for the firmware_class and will use the
+ *	exposted sysfs interface to upload the firmware for the caller.
+ * @FW_PRIV_NO_CACHE: indicates that the firmware request
+ *	should not set up and use the internal caching mechanism to assist
+ *	drivers from fetching firmware at resume time after suspend.
+ * @FW_PRIV_OPTIONAL: if set it is not a hard requirement by the
+ *	caller that the file requested be present. An error will not be recorded
+ *	if the file is not found.
+ * @FW_PRIV_NO_FALLBACK: if set ensures that no matter what we never use the
+ * 	firmware fallback mechanisms devised.
+ */
+enum fw_priv_flags {
+	FW_PRIV_FALLBACK			= BIT(0),
+	FW_PRIV_FALLBACK_UEVENT			= BIT(1),
+	FW_PRIV_NO_CACHE			= BIT(2),
+	FW_PRIV_OPTIONAL			= BIT(3),
+	FW_PRIV_NO_FALLBACK			= BIT(4),
+};
+
+/**
+ * struct fw_priv - private firmware parameters
+ * @mode: mode of operation
+ * @priv_flags: private set of &enum fw_priv_flags, private requirements for
+ *	the firmware request
+ * @pre_alloc_buf: buffer area preallocated by the caller so we can place the
+ *	respective firmware data onto.
+ * @pre_alloc_buf_size: size of the @pre_alloc_buf, this will be 0 if the
+ * 	caller did not preallocate a buffer and expects us to allocate a buffer
+ * 	on their behalf.
+ */
+struct fw_priv_params {
+	enum fw_api_mode mode;
+	u64 priv_flags;
+	void *pre_alloc_buf;
+	size_t pre_alloc_buf_size;
+};
+
 struct fw_priv {
 	struct kref ref;
+	struct fw_priv_params priv_params;
 	struct list_head list;
 	struct firmware_cache *fwc;
 	struct fw_state fw_st;
 	void *data;
 	size_t size;
-	size_t allocated_size;
 #ifdef CONFIG_FW_LOADER_USER_HELPER
 	bool is_paged_buf;
 	bool need_uevent;
@@ -132,6 +190,48 @@ static DEFINE_MUTEX(fw_lock);
 
 static struct firmware_cache fw_cache;
 
+static inline bool fw_priv_async(struct fw_priv *fw_priv)
+{
+	struct fw_priv_params *priv_params = &fw_priv->priv_params;
+
+	return priv_params->mode == FW_API_ASYNC;
+}
+
+static inline bool fw_priv_use_fallback(struct fw_priv *fw_priv)
+{
+	struct fw_priv_params *priv_params = &fw_priv->priv_params;
+
+	return  !!(priv_params->priv_flags & FW_PRIV_FALLBACK);
+}
+
+static inline bool fw_priv_no_fallback(struct fw_priv *fw_priv)
+{
+	struct fw_priv_params *priv_params = &fw_priv->priv_params;
+
+	return  !!(priv_params->priv_flags & FW_PRIV_NO_FALLBACK);
+}
+
+static inline bool fw_priv_uevent(struct fw_priv *fw_priv)
+{
+	struct fw_priv_params *priv_params = &fw_priv->priv_params;
+
+	return  !!(priv_params->priv_flags & FW_PRIV_FALLBACK_UEVENT);
+}
+
+static inline bool fw_priv_nocache(struct fw_priv *fw_priv)
+{
+	struct fw_priv_params *priv_params = &fw_priv->priv_params;
+
+	return !!(priv_params->priv_flags & FW_PRIV_NO_CACHE);
+}
+
+static inline bool fw_priv_optional(struct fw_priv *fw_priv)
+{
+	struct fw_priv_params *priv_params = &fw_priv->priv_params;
+
+	return !!(priv_params->priv_flags & FW_PRIV_OPTIONAL);
+}
+
 /* Builtin firmware support */
 
 #ifdef CONFIG_FW_LOADER
@@ -140,15 +240,25 @@ extern struct builtin_fw __start_builtin_fw[];
 extern struct builtin_fw __end_builtin_fw[];
 
 static void fw_copy_to_prealloc_buf(struct firmware *fw,
-				    void *buf, size_t size)
+				    struct fw_priv_params *priv_params)
 {
-	if (!buf || size < fw->size)
+	if (!priv_params ||
+	    !priv_params->pre_alloc_buf ||
+	    priv_params->pre_alloc_buf_size < fw->size)
 		return;
-	memcpy(buf, fw->data, fw->size);
+	memcpy(priv_params->pre_alloc_buf, fw->data, fw->size);
 }
 
-static bool fw_get_builtin_firmware(struct firmware *fw, const char *name,
-				    void *buf, size_t size)
+/*
+ * Note: if the firmware is found to be built-in the fw->priv will be NULL,
+ * this also means we never use the firmware cache on built-in firmware since
+ * we can never fail on these requests, they are easy and fast to handle,
+ * since we never look on the filesystem or require a fallback we don't need
+ * to keep any data for them.
+ */
+static bool fw_get_builtin_firmware(struct firmware *fw,
+				    struct fw_priv_params *priv_params,
+				    const char *name)
 {
 	struct builtin_fw *b_fw;
 
@@ -156,8 +266,7 @@ static bool fw_get_builtin_firmware(struct firmware *fw, const char *name,
 		if (strcmp(name, b_fw->name) == 0) {
 			fw->size = b_fw->size;
 			fw->data = b_fw->data;
-			fw_copy_to_prealloc_buf(fw, buf, size);
-
+			fw_copy_to_prealloc_buf(fw, priv_params);
 			return true;
 		}
 	}
@@ -179,8 +288,8 @@ static bool fw_is_builtin_firmware(const struct firmware *fw)
 #else /* Module case - no builtin firmware support */
 
 static inline bool fw_get_builtin_firmware(struct firmware *fw,
-					   const char *name, void *buf,
-					   size_t size)
+					   struct fw_priv_params *priv_params,
+					   const char *name)
 {
 	return false;
 }
@@ -287,7 +396,7 @@ static int fw_cache_piggyback_on_request(const char *name);
 
 static struct fw_priv *__allocate_fw_priv(const char *fw_name,
 					  struct firmware_cache *fwc,
-					  void *dbuf, size_t size)
+					  struct fw_priv_params *priv_params)
 {
 	struct fw_priv *fw_priv;
 
@@ -303,8 +412,7 @@ static struct fw_priv *__allocate_fw_priv(const char *fw_name,
 
 	kref_init(&fw_priv->ref);
 	fw_priv->fwc = fwc;
-	fw_priv->data = dbuf;
-	fw_priv->allocated_size = size;
+	fw_priv->data = priv_params->pre_alloc_buf;
 	fw_state_init(fw_priv);
 #ifdef CONFIG_FW_LOADER_USER_HELPER
 	INIT_LIST_HEAD(&fw_priv->pending_list);
@@ -329,8 +437,8 @@ static struct fw_priv *__lookup_fw_priv(const char *fw_name)
 /* Returns 1 for batching firmware requests with the same name */
 static int alloc_lookup_fw_priv(const char *fw_name,
 				struct firmware_cache *fwc,
-				struct fw_priv **fw_priv, void *dbuf,
-				size_t size)
+				struct fw_priv **fw_priv,
+				struct fw_priv_params *priv_params)
 {
 	struct fw_priv *tmp;
 
@@ -343,7 +451,7 @@ static int alloc_lookup_fw_priv(const char *fw_name,
 		pr_debug("batched request - sharing the same struct fw_priv and lookup for multiple requests\n");
 		return 1;
 	}
-	tmp = __allocate_fw_priv(fw_name, fwc, dbuf, size);
+	tmp = __allocate_fw_priv(fw_name, fwc, priv_params);
 	if (tmp)
 		list_add(&tmp->list, &fwc->head);
 	spin_unlock(&fwc->lock);
@@ -357,6 +465,7 @@ static void __free_fw_priv(struct kref *ref)
 	__releases(&fwc->lock)
 {
 	struct fw_priv *fw_priv = to_fw_priv(ref);
+	struct fw_priv_params *priv_params = &fw_priv->priv_params;
 	struct firmware_cache *fwc = fw_priv->fwc;
 
 	pr_debug("%s: fw-%s fw_priv=%p data=%p size=%u\n",
@@ -375,7 +484,7 @@ static void __free_fw_priv(struct kref *ref)
 		vfree(fw_priv->pages);
 	} else
 #endif
-	if (!fw_priv->allocated_size)
+	if (!priv_params->pre_alloc_buf_size)
 		vfree(fw_priv->data);
 	kfree_const(fw_priv->fw_name);
 	kfree(fw_priv);
@@ -408,8 +517,10 @@ module_param_string(path, fw_path_para, sizeof(fw_path_para), 0644);
 MODULE_PARM_DESC(path, "customized firmware image search path with a higher priority than default path");
 
 static int
-fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv)
+fw_get_filesystem_firmware(struct device *device, struct firmware *fw)
 {
+	struct fw_priv *fw_priv = fw->priv;
+	struct fw_priv_params *priv_params = &fw_priv->priv_params;
 	loff_t size;
 	int i, len;
 	int rc = -ENOENT;
@@ -417,10 +528,9 @@ fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv)
 	enum kernel_read_file_id id = READING_FIRMWARE;
 	size_t msize = INT_MAX;
 
-	/* Already populated data member means we're loading into a buffer */
-	if (fw_priv->data) {
+	if (priv_params->pre_alloc_buf) {
 		id = READING_FIRMWARE_PREALLOC_BUFFER;
-		msize = fw_priv->allocated_size;
+		msize = priv_params->pre_alloc_buf_size;
 	}
 
 	path = __getname();
@@ -464,7 +574,7 @@ fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv)
 /* firmware holds the ownership of pages */
 static void firmware_free_data(const struct firmware *fw)
 {
-	/* Loaded directly? */
+	/* Is built-in ? */
 	if (!fw->priv) {
 		vfree(fw->data);
 		return;
@@ -548,8 +658,7 @@ static int fw_add_devm_name(struct device *dev, const char *name)
 }
 #endif
 
-static int assign_fw(struct firmware *fw, struct device *device,
-		     unsigned int opt_flags)
+static int assign_fw(struct firmware *fw, struct device *device)
 {
 	struct fw_priv *fw_priv = fw->priv;
 
@@ -567,15 +676,16 @@ static int assign_fw(struct firmware *fw, struct device *device,
 	 * should be fixed in devres or driver core.
 	 */
 	/* don't cache firmware handled without uevent */
-	if (device && (opt_flags & FW_OPT_UEVENT) &&
-	    !(opt_flags & FW_OPT_NOCACHE))
+	if (device &&
+	    fw_priv_uevent(fw_priv) &&
+	    !fw_priv_nocache(fw_priv))
 		fw_add_devm_name(device, fw_priv->fw_name);
 
 	/*
 	 * After caching firmware image is started, let it piggyback
 	 * on request firmware.
 	 */
-	if (!(opt_flags & FW_OPT_NOCACHE) &&
+	if (!fw_priv_nocache(fw_priv) &&
 	    fw_priv->fwc->state == FW_LOADER_START_CACHE) {
 		if (fw_cache_piggyback_on_request(fw_priv->fw_name))
 			kref_get(&fw_priv->ref);
@@ -592,7 +702,6 @@ static int assign_fw(struct firmware *fw, struct device *device,
  */
 #ifdef CONFIG_FW_LOADER_USER_HELPER
 struct fw_sysfs {
-	bool nowait;
 	struct device dev;
 	struct fw_priv *fw_priv;
 	struct firmware *fw;
@@ -684,11 +793,13 @@ static void fw_dev_release(struct device *dev)
 
 static int do_firmware_uevent(struct fw_sysfs *fw_sysfs, struct kobj_uevent_env *env)
 {
+	struct fw_priv *fw_priv = fw_sysfs->fw_priv;
+
 	if (add_uevent_var(env, "FIRMWARE=%s", fw_sysfs->fw_priv->fw_name))
 		return -ENOMEM;
 	if (add_uevent_var(env, "TIMEOUT=%i", loading_timeout))
 		return -ENOMEM;
-	if (add_uevent_var(env, "ASYNC=%d", fw_sysfs->nowait))
+	if (add_uevent_var(env, "ASYNC=%d", fw_priv_async(fw_priv)))
 		return -ENOMEM;
 
 	return 0;
@@ -967,6 +1078,7 @@ static ssize_t firmware_data_write(struct file *filp, struct kobject *kobj,
 	struct device *dev = kobj_to_dev(kobj);
 	struct fw_sysfs *fw_sysfs = to_fw_sysfs(dev);
 	struct fw_priv *fw_priv;
+	struct fw_priv_params *priv_params;
 	ssize_t retval;
 
 	if (!capable(CAP_SYS_RAWIO))
@@ -974,13 +1086,14 @@ static ssize_t firmware_data_write(struct file *filp, struct kobject *kobj,
 
 	mutex_lock(&fw_lock);
 	fw_priv = fw_sysfs->fw_priv;
+	priv_params = &fw_priv->priv_params;
 	if (!fw_priv || fw_sysfs_done(fw_priv)) {
 		retval = -ENODEV;
 		goto out;
 	}
 
 	if (fw_priv->data) {
-		if (offset + count > fw_priv->allocated_size) {
+		if (offset + count > priv_params->pre_alloc_buf_size) {
 			retval = -ENOMEM;
 			goto out;
 		}
@@ -1030,8 +1143,9 @@ static const struct attribute_group *fw_dev_attr_groups[] = {
 
 static struct fw_sysfs *
 fw_create_instance(struct firmware *firmware, const char *fw_name,
-		   struct device *device, unsigned int opt_flags)
+		   struct device *device)
 {
+	struct fw_priv *fw_priv = firmware->priv;
 	struct fw_sysfs *fw_sysfs;
 	struct device *f_dev;
 
@@ -1041,7 +1155,7 @@ fw_create_instance(struct firmware *firmware, const char *fw_name,
 		goto exit;
 	}
 
-	fw_sysfs->nowait = !!(opt_flags & FW_OPT_NOWAIT);
+	fw_sysfs->fw_priv = fw_priv;
 	fw_sysfs->fw = firmware;
 	f_dev = &fw_sysfs->dev;
 
@@ -1055,8 +1169,7 @@ fw_create_instance(struct firmware *firmware, const char *fw_name,
 }
 
 /* load a firmware via user helper */
-static int _request_firmware_load(struct fw_sysfs *fw_sysfs,
-				  unsigned int opt_flags, long timeout)
+static int _request_firmware_load(struct fw_sysfs *fw_sysfs, long timeout)
 {
 	int retval = 0;
 	struct device *f_dev = &fw_sysfs->dev;
@@ -1078,7 +1191,7 @@ static int _request_firmware_load(struct fw_sysfs *fw_sysfs,
 	list_add(&fw_priv->pending_list, &pending_fw_head);
 	mutex_unlock(&fw_lock);
 
-	if (opt_flags & FW_OPT_UEVENT) {
+	if (fw_priv_uevent(fw_priv)) {
 		fw_priv->need_uevent = true;
 		dev_set_uevent_suppress(f_dev, false);
 		dev_dbg(f_dev, "firmware: requesting %s\n", fw_priv->fw_name);
@@ -1109,15 +1222,15 @@ static int _request_firmware_load(struct fw_sysfs *fw_sysfs,
 }
 
 static int fw_load_from_user_helper(struct firmware *firmware,
-				    const char *name, struct device *device,
-				    unsigned int opt_flags)
+				    const char *name, struct device *device)
 {
 	struct fw_sysfs *fw_sysfs;
+	struct fw_priv *fw_priv = firmware->priv;
 	long timeout;
 	int ret;
 
 	timeout = firmware_loading_timeout();
-	if (opt_flags & FW_OPT_NOWAIT) {
+	if (fw_priv_async(fw_priv)) {
 		timeout = usermodehelper_read_lock_wait(timeout);
 		if (!timeout) {
 			dev_dbg(device, "firmware: %s loading timed out\n",
@@ -1133,17 +1246,17 @@ static int fw_load_from_user_helper(struct firmware *firmware,
 		}
 	}
 
-	fw_sysfs = fw_create_instance(firmware, name, device, opt_flags);
+	fw_sysfs = fw_create_instance(firmware, name, device);
 	if (IS_ERR(fw_sysfs)) {
 		ret = PTR_ERR(fw_sysfs);
 		goto out_unlock;
 	}
 
 	fw_sysfs->fw_priv = firmware->priv;
-	ret = _request_firmware_load(fw_sysfs, opt_flags, timeout);
+	ret = _request_firmware_load(fw_sysfs, timeout);
 
 	if (!ret)
-		ret = assign_fw(firmware, device, opt_flags);
+		ret = assign_fw(firmware, device);
 
 out_unlock:
 	usermodehelper_read_unlock();
@@ -1152,50 +1265,50 @@ static int fw_load_from_user_helper(struct firmware *firmware,
 }
 
 #ifdef CONFIG_FW_LOADER_USER_HELPER_FALLBACK
-static bool fw_force_sysfs_fallback(unsigned int opt_flags)
+static bool fw_force_sysfs_fallback(struct fw_priv *fw_priv)
 {
 	return true;
 }
 #else
-static bool fw_force_sysfs_fallback(unsigned int opt_flags)
+static bool fw_force_sysfs_fallback(struct fw_priv *fw_priv)
 {
 	if (fw_debug_force_sysfs_fallback())
 		return true;
 
-	if (!(opt_flags & FW_OPT_USERHELPER))
+	if (!fw_priv_use_fallback(fw_priv))
 		return false;
 	return true;
 }
 #endif
 
-static bool fw_run_sysfs_fallback(unsigned int opt_flags)
+static bool fw_run_sysfs_fallback(struct fw_priv *fw_priv)
 {
 	if (fw_debug_ignore_sysfs_fallback()) {
 		pr_info("Ignoring firmware sysfs fallback due to debugfs knob\n");
 		return false;
 	}
 
-	if ((opt_flags & FW_OPT_NOFALLBACK))
+	if (fw_priv_no_fallback(fw_priv))
 		return false;
 
-	return fw_force_sysfs_fallback(opt_flags);
+	return fw_force_sysfs_fallback(fw_priv);
 }
 
 static int fw_sysfs_fallback(struct firmware *fw, const char *name,
 			    struct device *device,
-			    unsigned int opt_flags,
 			    int ret)
 {
-	if (!fw_run_sysfs_fallback(opt_flags))
+	struct fw_priv *fw_priv = fw->priv;
+
+	if (!fw_run_sysfs_fallback(fw_priv))
 		return ret;
 
 	dev_warn(device, "Falling back to user helper\n");
-	return fw_load_from_user_helper(fw, name, device, opt_flags);
+	return fw_load_from_user_helper(fw, name, device);
 }
 #else /* CONFIG_FW_LOADER_USER_HELPER */
 static int fw_sysfs_fallback(struct firmware *fw, const char *name,
 			     struct device *device,
-			     unsigned int opt_flags,
 			     int ret)
 {
 	/* Keep carrying over the same error */
@@ -1221,7 +1334,8 @@ static inline void unregister_sysfs_loader(void)
  */
 static int
 _request_firmware_prepare(struct firmware **firmware_p, const char *name,
-			  struct device *device, void *dbuf, size_t size)
+			  struct device *device,
+			  struct fw_priv_params *priv_params)
 {
 	struct firmware *firmware;
 	struct fw_priv *fw_priv;
@@ -1234,12 +1348,12 @@ _request_firmware_prepare(struct firmware **firmware_p, const char *name,
 		return -ENOMEM;
 	}
 
-	if (fw_get_builtin_firmware(firmware, name, dbuf, size)) {
+	if (fw_get_builtin_firmware(firmware, priv_params, name)) {
 		dev_dbg(device, "using built-in %s\n", name);
 		return 0; /* assigned */
 	}
 
-	ret = alloc_lookup_fw_priv(name, &fw_cache, &fw_priv, dbuf, size);
+	ret = alloc_lookup_fw_priv(name, &fw_cache, &fw_priv, priv_params);
 
 	/*
 	 * bind with 'priv' now to avoid warning in failure path
@@ -1285,10 +1399,11 @@ static void fw_abort_batch_reqs(struct firmware *fw)
 /* called from request_firmware() and request_firmware_work_func() */
 static int
 _request_firmware(const struct firmware **firmware_p, const char *name,
-		  struct device *device, void *buf, size_t size,
-		  unsigned int opt_flags)
+		  struct fw_priv_params *priv_params,
+		  struct device *device)
 {
 	struct firmware *fw = NULL;
+	struct fw_priv *fw_priv = NULL;
 	int ret;
 
 	if (!firmware_p)
@@ -1299,19 +1414,23 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
 		goto out;
 	}
 
-	ret = _request_firmware_prepare(&fw, name, device, buf, size);
+	ret = _request_firmware_prepare(&fw, name, device, priv_params);
 	if (ret <= 0) /* error or already assigned */
 		goto out;
 
-	ret = fw_get_filesystem_firmware(device, fw->priv);
+	fw_priv = fw->priv;
+	memcpy(&fw_priv->priv_params, priv_params,
+	       sizeof(struct fw_priv_params));
+
+	ret = fw_get_filesystem_firmware(device, fw);
 	if (ret) {
-		if (!(opt_flags & FW_OPT_NO_WARN))
+		if (!fw_priv_optional(fw_priv))
 			dev_warn(device,
 				 "Direct firmware load for %s failed with error %d\n",
 				 name, ret);
-		ret = fw_sysfs_fallback(fw, name, device, opt_flags, ret);
+		ret = fw_sysfs_fallback(fw, name, device, ret);
 	} else
-		ret = assign_fw(fw, device, opt_flags);
+		ret = assign_fw(fw, device);
 
  out:
 	if (ret < 0) {
@@ -1349,11 +1468,14 @@ request_firmware(const struct firmware **firmware_p, const char *name,
 		 struct device *device)
 {
 	int ret;
+	struct fw_priv_params priv_params = {
+		.priv_flags = FW_PRIV_FALLBACK |
+			      FW_PRIV_FALLBACK_UEVENT
+	};
 
 	/* Need to pin this module until return */
 	__module_get(THIS_MODULE);
-	ret = _request_firmware(firmware_p, name, device, NULL, 0,
-				FW_OPT_UEVENT);
+	ret = _request_firmware(firmware_p, name, &priv_params, device);
 	module_put(THIS_MODULE);
 	return ret;
 }
@@ -1374,11 +1496,13 @@ int request_firmware_direct(const struct firmware **firmware_p,
 			    const char *name, struct device *device)
 {
 	int ret;
+	struct fw_priv_params priv_params = {
+		.priv_flags = FW_PRIV_FALLBACK_UEVENT |
+			      FW_PRIV_NO_FALLBACK
+	};
 
 	__module_get(THIS_MODULE);
-	ret = _request_firmware(firmware_p, name, device, NULL, 0,
-				FW_OPT_UEVENT | FW_OPT_NO_WARN |
-				FW_OPT_NOFALLBACK);
+	ret = _request_firmware(firmware_p, name, &priv_params, device);
 	module_put(THIS_MODULE);
 	return ret;
 }
@@ -1404,10 +1528,16 @@ request_firmware_into_buf(const struct firmware **firmware_p, const char *name,
 			  struct device *device, void *buf, size_t size)
 {
 	int ret;
+	struct fw_priv_params priv_params = {
+		.priv_flags = FW_PRIV_FALLBACK |
+			      FW_PRIV_FALLBACK_UEVENT |
+			      FW_PRIV_NO_CACHE,
+		.pre_alloc_buf = buf,
+		.pre_alloc_buf_size = size
+	};
 
 	__module_get(THIS_MODULE);
-	ret = _request_firmware(firmware_p, name, device, buf, size,
-				FW_OPT_UEVENT | FW_OPT_NOCACHE);
+	ret = _request_firmware(firmware_p, name, &priv_params, device);
 	module_put(THIS_MODULE);
 	return ret;
 }
@@ -1432,21 +1562,22 @@ struct firmware_work {
 	struct work_struct work;
 	struct module *module;
 	const char *name;
+	struct fw_priv_params priv_params;
 	struct device *device;
 	void *context;
 	void (*cont)(const struct firmware *fw, void *context);
-	unsigned int opt_flags;
 };
 
 static void request_firmware_work_func(struct work_struct *work)
 {
 	struct firmware_work *fw_work;
 	const struct firmware *fw;
+	struct fw_priv_params *priv_params;
 
 	fw_work = container_of(work, struct firmware_work, work);
+	priv_params = &fw_work->priv_params;
 
-	_request_firmware(&fw, fw_work->name, fw_work->device, NULL, 0,
-			  fw_work->opt_flags);
+	_request_firmware(&fw, fw_work->name, priv_params, fw_work->device);
 	fw_work->cont(fw, fw_work->context);
 	put_device(fw_work->device); /* taken in request_firmware_nowait() */
 
@@ -1485,6 +1616,11 @@ request_firmware_nowait(
 	void (*cont)(const struct firmware *fw, void *context))
 {
 	struct firmware_work *fw_work;
+	struct fw_priv_params priv_params = {
+		.mode = FW_API_ASYNC,
+		.priv_flags = FW_PRIV_FALLBACK |
+			     (uevent ? FW_PRIV_FALLBACK_UEVENT : 0)
+	};
 
 	fw_work = kzalloc(sizeof(struct firmware_work), gfp);
 	if (!fw_work)
@@ -1499,8 +1635,8 @@ request_firmware_nowait(
 	fw_work->device = device;
 	fw_work->context = context;
 	fw_work->cont = cont;
-	fw_work->opt_flags = FW_OPT_NOWAIT |
-		(uevent ? FW_OPT_UEVENT : FW_OPT_USERHELPER);
+	memcpy(&fw_work->priv_params, &priv_params,
+	       sizeof(struct fw_priv_params));
 
 	if (!try_module_get(module)) {
 		kfree_const(fw_work->name);
@@ -1578,7 +1714,7 @@ static int uncache_firmware(const char *fw_name)
 
 	pr_debug("%s: %s\n", __func__, fw_name);
 
-	if (fw_get_builtin_firmware(&fw, fw_name, NULL, 0))
+	if (fw_get_builtin_firmware(&fw, NULL, fw_name))
 		return 0;
 
 	fw_priv = lookup_fw_priv(fw_name);
-- 
2.15.0

Re: [PATCH v2 09/23] firmware: use static inline for to_fw_priv()

[Greg KH]

On Mon, Nov 20, 2017 at 10:23:55AM -0800, Luis R. Rodriguez wrote:
> This lets us type check the callers.
> 
> Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
> ---
>  drivers/base/firmware_class.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index dcc1281789e4..4f64410fe7e6 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -125,7 +125,10 @@ struct fw_name_devm {
>  	const char *name;
>  };
>  
> -#define to_fw_priv(d) container_of(d, struct fw_priv, ref)
> +static inline struct fw_priv *to_fw_priv(struct kref *ref)
> +{
> +	return container_of(ref, struct fw_priv, ref);
> +}

It's a kref, no need to typecheck anything, if it isn't the right
structure it will usually just break the build anyway.  So this isn't
really needed, especially as most of the to_* calls in the kernel are
macros like this.

I'll take it, but really, this isn't needed, you are only trying to
protect yourself from your own coding :)

greg k-h

Re: [PATCH v2 10/23] firmware: add helper to copy built-in data to pre-alloc buffer

[Greg KH]

On Mon, Nov 20, 2017 at 10:23:56AM -0800, Luis R. Rodriguez wrote:
> This makes it clearer that the parameters passed are only used for
> the preallocated buffer option, ie, when a caller uses:
> 
> 	request_firmware_into_buf()
> 
> Otherwise this code won't run. We flip the logic just so the actual
> prellocated buf code is not indented.
> 
> Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
> ---
>  drivers/base/firmware_class.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index 4f64410fe7e6..aba3f2cbe2f4 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -146,6 +146,14 @@ static struct firmware_cache fw_cache;
>  extern struct builtin_fw __start_builtin_fw[];
>  extern struct builtin_fw __end_builtin_fw[];
>  
> +static void fw_copy_to_prealloc_buf(struct firmware *fw,
> +				    void *buf, size_t size)
> +{
> +	if (!buf || size < fw->size)
> +		return;

Shouldn't this return an error?

> +	memcpy(buf, fw->data, fw->size);

I'll take this, but it feels really odd...

Re: [PATCH v2 16/23] test_firmware: enable custom fallback testing on limited kernel configs

[Greg KH]

On Mon, Nov 20, 2017 at 10:24:02AM -0800, Luis R. Rodriguez wrote:
> 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 <mcgrof@kernel.org>
> ---
>  tools/testing/selftests/firmware/config         |  4 +++
>  tools/testing/selftests/firmware/fw_fallback.sh | 45 ++++++++++++++++++++++++-
>  2 files changed, 48 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/firmware/config b/tools/testing/selftests/firmware/config
> index c8137f70e291..bf634dda0720 100644
> --- a/tools/testing/selftests/firmware/config
> +++ b/tools/testing/selftests/firmware/config
> @@ -1 +1,5 @@
>  CONFIG_TEST_FIRMWARE=y
> +CONFIG_FW_LOADER=y
> +CONFIG_FW_LOADER_USER_HELPER=y
> +CONFIG_IKCONFIG=y
> +CONFIG_IKCONFIG_PROC=y
> diff --git a/tools/testing/selftests/firmware/fw_fallback.sh b/tools/testing/selftests/firmware/fw_fallback.sh
> index 722cad91df74..a42e437363d9 100755
> --- a/tools/testing/selftests/firmware/fw_fallback.sh
> +++ b/tools/testing/selftests/firmware/fw_fallback.sh
> @@ -6,7 +6,46 @@
>  # won't find so that we can do the load ourself manually.
>  set -e
>  
> +PROC_CONFIG="/proc/config.gz"

How does this work when you are running the test on a target that does
not have this kernel build option?

thanks,

greg k-h

Re: [PATCH v2 16/23] test_firmware: enable custom fallback testing on limited kernel configs

[Greg KH]

On Mon, Nov 20, 2017 at 10:24:02AM -0800, Luis R. Rodriguez wrote:
> 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 <mcgrof@kernel.org>
> ---
>  tools/testing/selftests/firmware/config         |  4 +++
>  tools/testing/selftests/firmware/fw_fallback.sh | 45 ++++++++++++++++++++++++-
>  2 files changed, 48 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/firmware/config b/tools/testing/selftests/firmware/config
> index c8137f70e291..bf634dda0720 100644
> --- a/tools/testing/selftests/firmware/config
> +++ b/tools/testing/selftests/firmware/config
> @@ -1 +1,5 @@
>  CONFIG_TEST_FIRMWARE=y
> +CONFIG_FW_LOADER=y
> +CONFIG_FW_LOADER_USER_HELPER=y
> +CONFIG_IKCONFIG=y
> +CONFIG_IKCONFIG_PROC=y
> diff --git a/tools/testing/selftests/firmware/fw_fallback.sh b/tools/testing/selftests/firmware/fw_fallback.sh
> index 722cad91df74..a42e437363d9 100755
> --- a/tools/testing/selftests/firmware/fw_fallback.sh
> +++ b/tools/testing/selftests/firmware/fw_fallback.sh
> @@ -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
> +			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
> +}

Shouldn't these functions be part of the kselftest core so that all
tests can take advantage of them instead of having to hand-roll them for
every individual test?

And is there no way at runtime to tell what the options are and just
not run that type of test?

thanks,

greg k-h

Re: [PATCH v2 19/23] firmware: add debug facility to emulate forcing sysfs fallback

[Greg KH]

On Mon, Nov 20, 2017 at 10:24:05AM -0800, Luis R. Rodriguez wrote:
> The CONFIG_FW_LOADER_USER_HELPER_FALLBACK kernel config is not
> enabled my major distros, however its known to be enabled on
> some Android kernels. Testing the firmware API properly then
> means to test at least two different kernel configurations.
> 
> Since CONFIG_FW_LOADER_USER_HELPER_FALLBACK only does a simple
> enablement under certain situations we can just emulate this
> behaviour through a debugfs knob. This enables testing two of
> the tree possible kernel configs with the firmware loader with
> one kernel build.
> 
> Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
> ---
>  drivers/base/Kconfig           |  6 ++++++
>  drivers/base/Makefile          |  1 +
>  drivers/base/firmware_debug.c  | 34 ++++++++++++++++++++++++++++++++
>  drivers/base/firmware_debug.h  | 44 ++++++++++++++++++++++++++++++++++++++++++
>  drivers/base/firmware_loader.c | 13 ++++++++++++-
>  5 files changed, 97 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/base/firmware_debug.c
>  create mode 100644 drivers/base/firmware_debug.h
> 
> diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
> index 2f6614c9a229..ab6889ac6787 100644
> --- a/drivers/base/Kconfig
> +++ b/drivers/base/Kconfig
> @@ -149,6 +149,12 @@ config EXTRA_FIRMWARE_DIR
>  config FW_LOADER_USER_HELPER
>  	bool
>  
> +config FW_LOADER_DEBUG
> +	bool "Firmware loader debugging interface"
> +	help
> +	  If you are hacking on the firmware API of the kernel, firmware_class,
> +	  you can enable this to help debug the kernel.
> +
>  config FW_LOADER_USER_HELPER_FALLBACK
>  	bool "Fallback user-helper invocation for firmware loading"
>  	depends on FW_LOADER
> diff --git a/drivers/base/Makefile b/drivers/base/Makefile
> index f261143fafbf..64eaa4f9cb02 100644
> --- a/drivers/base/Makefile
> +++ b/drivers/base/Makefile
> @@ -14,6 +14,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_DEBUG) += firmware_debug.o
>  obj-$(CONFIG_NUMA)	+= node.o
>  obj-$(CONFIG_MEMORY_HOTPLUG_SPARSE) += memory.o
>  ifeq ($(CONFIG_SYSFS),y)
> diff --git a/drivers/base/firmware_debug.c b/drivers/base/firmware_debug.c
> new file mode 100644
> index 000000000000..f2817eb6f480
> --- /dev/null
> +++ b/drivers/base/firmware_debug.c
> @@ -0,0 +1,34 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Firmware dubugging interface */
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/debugfs.h>
> +#include "firmware_debug.h"
> +
> +struct firmware_debug fw_debug;
> +
> +static struct dentry *debugfs_firmware;
> +
> +int __init register_fw_debugfs(void)
> +{
> +	debugfs_firmware = debugfs_create_dir("firmware", NULL);
> +	if (!debugfs_firmware)
> +		return -ENOMEM;

You never need to check the return value of a debugfs call, you should
not care about it, nor do anything different in your code.  The value
returned can always be passed back into any other debugfs call when
needed.

> +
> +	if (!debugfs_create_bool("force_sysfs_fallback", S_IRUSR | S_IWUSR,
> +				 debugfs_firmware,
> +				 &fw_debug.force_sysfs_fallback))

Same here, you don't care.

> +		goto err_out;
> +
> +	return 0;
> +err_out:
> +	debugfs_remove_recursive(debugfs_firmware);

You didn't create any files, why recursive?
Anyway, not needed.

> +	debugfs_firmware = NULL;
> +	return -ENOMEM;
> +}
> +
> +void unregister_fw_debugfs(void)
> +{
> +	debugfs_remove_recursive(debugfs_firmware);
> +	debugfs_firmware = NULL;

Why set this to NULL?

> +}
> diff --git a/drivers/base/firmware_debug.h b/drivers/base/firmware_debug.h
> new file mode 100644
> index 000000000000..bc41bbca9a54
> --- /dev/null
> +++ b/drivers/base/firmware_debug.h
> @@ -0,0 +1,44 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#ifndef __FIRMWARE_DEBUG_H
> +#define __FIRMWARE_DEBUG_H
> +
> +#ifdef CONFIG_FW_LOADER_DEBUG
> +/**
> + * struct firmware_debug - firmware debugging configuration
> + *
> + * Provided to help debug the firmware API.
> + *
> + * @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.
> + */
> +struct firmware_debug {
> +	bool force_sysfs_fallback;
> +};
> +
> +extern struct firmware_debug fw_debug;
> +
> +int register_fw_debugfs(void);
> +void unregister_fw_debugfs(void);
> +
> +static inline bool fw_debug_force_sysfs_fallback(void)
> +{
> +	return fw_debug.force_sysfs_fallback;
> +}
> +#else
> +static inline int register_fw_debugfs(void)
> +{
> +	return 0;
> +}
> +static inline void unregister_fw_debugfs(void)
> +{
> +}
> +
> +static inline bool fw_debug_force_sysfs_fallback(void)
> +{
> +	return false;
> +}
> +#endif /* CONFIG_FW_LOADER_DEBUG */
> +
> +#endif /* __FIRMWARE_DEBUG_H */
> diff --git a/drivers/base/firmware_loader.c b/drivers/base/firmware_loader.c
> index 43b97a8137f7..b2b52ba9f245 100644
> --- a/drivers/base/firmware_loader.c
> +++ b/drivers/base/firmware_loader.c
> @@ -36,6 +36,7 @@
>  #include <generated/utsrelease.h>
>  
>  #include "base.h"
> +#include "firmware_debug.h"
>  
>  MODULE_AUTHOR("Manuel Estrada Sainz");
>  MODULE_DESCRIPTION("Multi purpose firmware loading support");
> @@ -1158,6 +1159,9 @@ static bool fw_force_sysfs_fallback(unsigned int opt_flags)
>  #else
>  static bool fw_force_sysfs_fallback(unsigned int opt_flags)
>  {
> +	if (fw_debug_force_sysfs_fallback())
> +		return true;
> +
>  	if (!(opt_flags & FW_OPT_USERHELPER))
>  		return false;
>  	return true;
> @@ -1913,10 +1917,14 @@ static int __init firmware_class_init(void)
>  	/* No need to unfold these on exit */
>  	fw_cache_init();
>  
> -	ret = register_fw_pm_ops();
> +	ret = register_fw_debugfs();
>  	if (ret)
>  		return ret;

Again you don't care about the state of debugfs.  Did you test this on a
system without CONFIG_DEBUGFS enabled?


thanks,

greg k-h

Re: [PATCH v2 16/23] test_firmware: enable custom fallback testing on limited kernel configs

[Greg KH]

On Mon, Nov 20, 2017 at 10:24:02AM -0800, Luis R. Rodriguez wrote:
> 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 <mcgrof@kernel.org>

I've applied the patches up to here in this series.

thanks,

greg k-h

Re: [PATCH v2 10/23] firmware: add helper to copy built-in data to pre-alloc buffer

[Luis R. Rodriguez]

On Wed, Nov 29, 2017 at 11:17:15AM +0100, Greg KH wrote:
> On Mon, Nov 20, 2017 at 10:23:56AM -0800, Luis R. Rodriguez wrote:
> > This makes it clearer that the parameters passed are only used for
> > the preallocated buffer option, ie, when a caller uses:
> > 
> > 	request_firmware_into_buf()
> > 
> > Otherwise this code won't run. We flip the logic just so the actual
> > prellocated buf code is not indented.
> > 
> > Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
> > ---
> >  drivers/base/firmware_class.c | 11 +++++++++--
> >  1 file changed, 9 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> > index 4f64410fe7e6..aba3f2cbe2f4 100644
> > --- a/drivers/base/firmware_class.c
> > +++ b/drivers/base/firmware_class.c
> > @@ -146,6 +146,14 @@ static struct firmware_cache fw_cache;
> >  extern struct builtin_fw __start_builtin_fw[];
> >  extern struct builtin_fw __end_builtin_fw[];
> >  
> > +static void fw_copy_to_prealloc_buf(struct firmware *fw,
> > +				    void *buf, size_t size)
> > +{
> > +	if (!buf || size < fw->size)
> > +		return;
> 
> Shouldn't this return an error?

No, its a no-op when these are not set, and its actually *why* I created
the function. The parameters are *only* useful for the prealloc buf fw
calls. Keeping track of all this while reading the code is actually not
easy and this should make it clearer.

> 
> > +	memcpy(buf, fw->data, fw->size);
> 
> I'll take this, but it feels really odd...

Thanks.

  Luis

Re: [PATCH v2 16/23] test_firmware: enable custom fallback testing on limited kernel configs

[Luis R. Rodriguez]

On Wed, Nov 29, 2017 at 11:22:35AM +0100, Greg KH wrote:
> On Mon, Nov 20, 2017 at 10:24:02AM -0800, Luis R. Rodriguez wrote:
> > 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 <mcgrof@kernel.org>
> > ---
> >  tools/testing/selftests/firmware/config         |  4 +++
> >  tools/testing/selftests/firmware/fw_fallback.sh | 45 ++++++++++++++++++++++++-
> >  2 files changed, 48 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tools/testing/selftests/firmware/config b/tools/testing/selftests/firmware/config
> > index c8137f70e291..bf634dda0720 100644
> > --- a/tools/testing/selftests/firmware/config
> > +++ b/tools/testing/selftests/firmware/config
> > @@ -1 +1,5 @@
> >  CONFIG_TEST_FIRMWARE=y
> > +CONFIG_FW_LOADER=y
> > +CONFIG_FW_LOADER_USER_HELPER=y
> > +CONFIG_IKCONFIG=y
> > +CONFIG_IKCONFIG_PROC=y
> > diff --git a/tools/testing/selftests/firmware/fw_fallback.sh b/tools/testing/selftests/firmware/fw_fallback.sh
> > index 722cad91df74..a42e437363d9 100755
> > --- a/tools/testing/selftests/firmware/fw_fallback.sh
> > +++ b/tools/testing/selftests/firmware/fw_fallback.sh
> > @@ -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
> > +			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
> > +}
> 
> Shouldn't these functions be part of the kselftest core so that all
> tests can take advantage of them instead of having to hand-roll them for
> every individual test?

At a first glance it would seem to be nice.

Note that in our case we'd still need a way to work without CONFIG_IKCONFIG_PROC,
hence that big else condition, to ensure it works for kernels without
CONFIG_IKCONFIG_PROC set, so we'd still end up with our own fw_kconfig_has(),
and if we had a generic helper it'd look very similar... something like:

fw_kconfig_has()
{
	if [ has_proc_config ]; then
		echo $(kconfig_has("$1"))
	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
}

Not sure if there is a big gain then for now.

Shuah, what do you think? Is it worth it? Do we have a generic bash library we
can use?  If not, if I add one, so that other scripts can source it, where
should I add it?

> And is there no way at runtime to tell what the options are and just
> not run that type of test?

For CONFIG_FW_LOADER_USER_HELPER=y yes, its handled in that else condition.

For CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y though no, there is no way. The
else condition has a big comment indicating there is no current *easy*
run time heuristic possible. This remains true specially since we have to
support these scripts to run on older kernels as well.

  Luis

Re: [PATCH v2 19/23] firmware: add debug facility to emulate forcing sysfs fallback

[Luis R. Rodriguez]

On Wed, Nov 29, 2017 at 11:28:04AM +0100, Greg KH wrote:
> On Mon, Nov 20, 2017 at 10:24:05AM -0800, Luis R. Rodriguez wrote:
> > diff --git a/drivers/base/firmware_debug.c b/drivers/base/firmware_debug.c
> > new file mode 100644
> > index 000000000000..f2817eb6f480
> > --- /dev/null
> > +++ b/drivers/base/firmware_debug.c
> > @@ -0,0 +1,34 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/* Firmware dubugging interface */
> > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > +
> > +#include <linux/debugfs.h>
> > +#include "firmware_debug.h"
> > +
> > +struct firmware_debug fw_debug;
> > +
> > +static struct dentry *debugfs_firmware;
> > +
> > +int __init register_fw_debugfs(void)
> > +{
> > +	debugfs_firmware = debugfs_create_dir("firmware", NULL);
> > +	if (!debugfs_firmware)
> > +		return -ENOMEM;
> 
> You never need to check the return value of a debugfs call, you should
> not care about it, nor do anything different in your code.  The value
> returned can always be passed back into any other debugfs call when
> needed.

Neat, so all uses as in the above are wrong eh?

> > +
> > +	if (!debugfs_create_bool("force_sysfs_fallback", S_IRUSR | S_IWUSR,
> > +				 debugfs_firmware,
> > +				 &fw_debug.force_sysfs_fallback))
> 
> Same here, you don't care.

OK!

> 
> > +		goto err_out;
> > +
> > +	return 0;
> > +err_out:
> > +	debugfs_remove_recursive(debugfs_firmware);
> 
> You didn't create any files, why recursive?
> Anyway, not needed.

Wouldn't this take care of removing the two bool files I add in one shot later?

> > +	debugfs_firmware = NULL;
> > +	return -ENOMEM;
> > +}
> > +
> > +void unregister_fw_debugfs(void)
> > +{
> > +	debugfs_remove_recursive(debugfs_firmware);
> > +	debugfs_firmware = NULL;
> 
> Why set this to NULL?

OK, will avoid.

> > diff --git a/drivers/base/firmware_loader.c b/drivers/base/firmware_loader.c
> > index 43b97a8137f7..b2b52ba9f245 100644
> > --- a/drivers/base/firmware_loader.c
> > +++ b/drivers/base/firmware_loader.c
> > @@ -36,6 +36,7 @@
> >  #include <generated/utsrelease.h>
> >  
> >  #include "base.h"
> > +#include "firmware_debug.h"
> >  
> >  MODULE_AUTHOR("Manuel Estrada Sainz");
> >  MODULE_DESCRIPTION("Multi purpose firmware loading support");
> > @@ -1158,6 +1159,9 @@ static bool fw_force_sysfs_fallback(unsigned int opt_flags)
> >  #else
> >  static bool fw_force_sysfs_fallback(unsigned int opt_flags)
> >  {
> > +	if (fw_debug_force_sysfs_fallback())
> > +		return true;
> > +
> >  	if (!(opt_flags & FW_OPT_USERHELPER))
> >  		return false;
> >  	return true;
> > @@ -1913,10 +1917,14 @@ static int __init firmware_class_init(void)
> >  	/* No need to unfold these on exit */
> >  	fw_cache_init();
> >  
> > -	ret = register_fw_pm_ops();
> > +	ret = register_fw_debugfs();
> >  	if (ret)
> >  		return ret;
> 
> Again you don't care about the state of debugfs.  Did you test this on a
> system without CONFIG_DEBUGFS enabled?

Hrm, nope. Will fix it up. Thanks for the review.

  Luis

Re: [PATCH v2 16/23] test_firmware: enable custom fallback testing on limited kernel configs

[Shuah Khan]

On 11/30/2017 01:31 PM, Luis R. Rodriguez wrote:
> On Wed, Nov 29, 2017 at 11:22:35AM +0100, Greg KH wrote:
>> On Mon, Nov 20, 2017 at 10:24:02AM -0800, Luis R. Rodriguez wrote:
>>> 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 <mcgrof@kernel.org>
>>> ---
>>>  tools/testing/selftests/firmware/config         |  4 +++
>>>  tools/testing/selftests/firmware/fw_fallback.sh | 45 ++++++++++++++++++++++++-
>>>  2 files changed, 48 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/tools/testing/selftests/firmware/config b/tools/testing/selftests/firmware/config
>>> index c8137f70e291..bf634dda0720 100644
>>> --- a/tools/testing/selftests/firmware/config
>>> +++ b/tools/testing/selftests/firmware/config
>>> @@ -1 +1,5 @@
>>>  CONFIG_TEST_FIRMWARE=y
>>> +CONFIG_FW_LOADER=y
>>> +CONFIG_FW_LOADER_USER_HELPER=y
>>> +CONFIG_IKCONFIG=y
>>> +CONFIG_IKCONFIG_PROC=y
>>> diff --git a/tools/testing/selftests/firmware/fw_fallback.sh b/tools/testing/selftests/firmware/fw_fallback.sh
>>> index 722cad91df74..a42e437363d9 100755
>>> --- a/tools/testing/selftests/firmware/fw_fallback.sh
>>> +++ b/tools/testing/selftests/firmware/fw_fallback.sh
>>> @@ -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
>>> +			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
>>> +}
>>
>> Shouldn't these functions be part of the kselftest core so that all
>> tests can take advantage of them instead of having to hand-roll them for
>> every individual test?
> 
> At a first glance it would seem to be nice.
> 
> Note that in our case we'd still need a way to work without CONFIG_IKCONFIG_PROC,
> hence that big else condition, to ensure it works for kernels without
> CONFIG_IKCONFIG_PROC set, so we'd still end up with our own fw_kconfig_has(),
> and if we had a generic helper it'd look very similar... something like:
> 
> fw_kconfig_has()
> {
> 	if [ has_proc_config ]; then
> 		echo $(kconfig_has("$1"))
> 	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
> }
> 
> Not sure if there is a big gain then for now.
> 
> Shuah, what do you think? Is it worth it? Do we have a generic bash library we
> can use?  If not, if I add one, so that other scripts can source it, where
> should I add it?

There is no generic script right now. It depends on whether or not a
generic script will help. Can it made generic or test would still need
their customizations anyway.

Furthermore, this generic script will become a dependency for tests and
will need to be included in the install targets and also when O=dir
cases. hence, the question on how generic can the script be and whether
are not tests can avoid customization.

I am finding in the case of selftests the generic framework sometimes
add more complexity. In some cases it is worth it like lib.mk, however
it takes work to override it as well.

I have some reservations about introducing a generic script for this.

thanks,
-- Shuah

Re: [PATCH v2 19/23] firmware: add debug facility to emulate forcing sysfs fallback

[Luis R. Rodriguez]

On Thu, Nov 30, 2017 at 09:35:16PM +0100, Luis R. Rodriguez wrote:
> On Wed, Nov 29, 2017 at 11:28:04AM +0100, Greg KH wrote:
> > On Mon, Nov 20, 2017 at 10:24:05AM -0800, Luis R. Rodriguez wrote:
> > > diff --git a/drivers/base/firmware_debug.c b/drivers/base/firmware_debug.c
> > > new file mode 100644
> > > index 000000000000..f2817eb6f480
> > > --- /dev/null
> > > +++ b/drivers/base/firmware_debug.c
> > > @@ -0,0 +1,34 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/* Firmware dubugging interface */
> > > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > > +
> > > +#include <linux/debugfs.h>
> > > +#include "firmware_debug.h"
> > > +
> > > +struct firmware_debug fw_debug;
> > > +
> > > +static struct dentry *debugfs_firmware;
> > > +
> > > +int __init register_fw_debugfs(void)
> > > +{
> > > +	debugfs_firmware = debugfs_create_dir("firmware", NULL);
> > > +	if (!debugfs_firmware)
> > > +		return -ENOMEM;
> > 
> > You never need to check the return value of a debugfs call, you should
> > not care about it, nor do anything different in your code.  The value
> > returned can always be passed back into any other debugfs call when
> > needed.
> 
> Neat, so all uses as in the above are wrong eh?

You know, I'm wondering if it just makes sense to go straight into making
CONFIG_FW_LOADER_USER_HELPER_FALLBACK nothing but a setting a bool on a
config to true.

diff --git a/drivers/base/firmware_loader.c b/drivers/base/firmware_loader.c
index 43b97a8137f7..d3f2aabfc41d 100644
--- a/drivers/base/firmware_loader.c
+++ b/drivers/base/firmware_loader.c
@@ -117,6 +117,12 @@ struct fw_name_devm {
 	const char *name;
 };
 
+struct firmware_config {
+	bool force_sysfs_fallback;
+};
+
+static struct firmware_config fw_config;
+
 static inline struct fw_priv *to_fw_priv(struct kref *ref)
 {
 	return container_of(ref, struct fw_priv, ref);
@@ -1151,18 +1157,25 @@ static int fw_load_from_user_helper(struct firmware *firmware,
 }
 
 #ifdef CONFIG_FW_LOADER_USER_HELPER_FALLBACK
-static bool fw_force_sysfs_fallback(unsigned int opt_flags)
+static void __init fw_config_init(void)
 {
-	return true;
+	fw_config.force_sysfs_fallback = true;
 }
+
 #else
+static void __init fw_config_init(void)
+{
+}
+#endif
+
 static bool fw_force_sysfs_fallback(unsigned int opt_flags)
 {
+	if (fw_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)
 {
@@ -1911,6 +1924,7 @@ static int __init firmware_class_init(void)
 	int ret;
 
 	/* No need to unfold these on exit */
+	fw_config_init();
 	fw_cache_init();
 
 	ret = register_fw_pm_ops();

After which we can add two generic syfs firmware knobs to help do the same as I did
for debugfs, only we actually support it as proper API. Thoughts?

For instance for changing to force the usermode helper:

diff --git a/drivers/base/firmware_loader.c b/drivers/base/firmware_loader.c
index d3f2aabfc41d..659db28f5c02 100644
--- a/drivers/base/firmware_loader.c
+++ b/drivers/base/firmware_loader.c
@@ -41,6 +41,9 @@ MODULE_AUTHOR("Manuel Estrada Sainz");
 MODULE_DESCRIPTION("Multi purpose firmware loading support");
 MODULE_LICENSE("GPL");
 
+static unsigned int zero;
+static unsigned int one = 1;
+
 enum fw_status {
 	FW_STATUS_UNKNOWN,
 	FW_STATUS_LOADING,
@@ -1919,6 +1922,19 @@ static struct notifier_block fw_shutdown_nb = {
 	.notifier_call = fw_shutdown_notify,
 };
 
+struct ctl_table firmware_config_table[] = {
+	{
+		.procname	= "force_sysfs_fallback",
+		.data		= &fw_config.force_sysfs_fallback,
+		.maxlen         = sizeof(unsigned int),
+		.mode           = 0644,
+		.proc_handler   = proc_douintvec_minmax,
+		.extra1		= &zero,
+		.extra2		= &one,
+	},
+	{ }
+};
+
 static int __init firmware_class_init(void)
 {
 	int ret;
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 557d46728577..202442f3c58c 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -251,6 +251,10 @@ extern struct ctl_table random_table[];
 extern struct ctl_table epoll_table[];
 #endif
 
+#ifdef CONFIG_FW_LOADER
+extern struct ctl_table firmware_config_table[];
+#endif
+
 #ifdef HAVE_ARCH_PICK_MMAP_LAYOUT
 int sysctl_legacy_va_layout;
 #endif
@@ -746,6 +750,13 @@ static struct ctl_table kern_table[] = {
 		.mode		= 0555,
 		.child		= usermodehelper_table,
 	},
+#ifdef CONFIG_FW_LOADER
+	{
+		.procname	= "firmware_config",
+		.mode		= 0555,
+		.child		= firmware_config_table,
+	},
+#endif
 	{
 		.procname	= "overflowuid",
 		.data		= &overflowuid,

Thoughts, preferences?

  Luis