[PATCH] firmware: Fix security issue with request_firmware_into_buf()

[Luis R. Rodriguez]

From: Rishabh Bhatnagar 

When calling request_firmware_into_buf() with the FW_OPT_NOCACHE flag
it is expected that firmware is loaded into buffer from memory.
But inside alloc_lookup_fw_priv every new firmware that is loaded is
added to the firmware cache (fwc) list head. So if any driver requests
a firmware that is already loaded the code iterates over the above
mentioned list and it can end up giving a pointer to other device driver's
firmware buffer.
Also the existing copy may either be modified by drivers, remote processors
or even freed. This causes a potential security issue with batched requests
when using request_firmware_into_buf.

Fix alloc_lookup_fw_priv to not add to the fwc head list if FW_OPT_NOCACHE
is set, and also don't do the lookup in the list.

Fixes: 0e742e9275 ("firmware: provide infrastructure to make fw caching 
optional")
[mcgrof: broken since feature introduction on v4.8]

Cc: sta...@vger.kernel.org # v4.8+
Signed-off-by: Vikram Mulukutla 
Signed-off-by: Rishabh Bhatnagar 
Signed-off-by: Luis Chamberlain 
---

This has been tested with the self test firmware scrip and found no
regressions.

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

diff --git a/drivers/base/firmware_loader/main.c 
b/drivers/base/firmware_loader/main.c
index 0943e7065e0e..b3c0498ee433 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -209,21 +209,24 @@ static struct fw_priv *__lookup_fw_priv(const char 
*fw_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)
+   size_t size, enum fw_opt opt_flags)
 {
struct fw_priv *tmp;
 
spin_lock(>lock);
-   tmp = __lookup_fw_priv(fw_name);
-   if (tmp) {
-   kref_get(>ref);
-   spin_unlock(>lock);
-   *fw_priv = tmp;
-   pr_debug("batched request - sharing the same struct fw_priv and 
lookup for multiple requests\n");
-   return 1;
+   if (!(opt_flags & FW_OPT_NOCACHE)) {
+   tmp = __lookup_fw_priv(fw_name);
+   if (tmp) {
+   kref_get(>ref);
+   spin_unlock(>lock);
+   *fw_priv = tmp;
+   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);
-   if (tmp)
+   if (tmp && !(opt_flags & FW_OPT_NOCACHE))
list_add(>list, >head);
spin_unlock(>lock);
 
@@ -493,7 +496,8 @@ int assign_fw(struct firmware *fw, struct device *device,
  */
 static int
 _request_firmware_prepare(struct firmware **firmware_p, const char *name,
- struct device *device, void *dbuf, size_t size)
+ struct device *device, void *dbuf, size_t size,
+ enum fw_opt opt_flags)
 {
struct firmware *firmware;
struct fw_priv *fw_priv;
@@ -511,7 +515,8 @@ _request_firmware_prepare(struct firmware **firmware_p, 
const char *name,
return 0; /* assigned */
}
 
-   ret = alloc_lookup_fw_priv(name, _cache, _priv, dbuf, size);
+   ret = alloc_lookup_fw_priv(name, _cache, _priv, dbuf, size,
+ opt_flags);
 
/*
 * bind with 'priv' now to avoid warning in failure path
@@ -571,7 +576,8 @@ _request_firmware(const struct firmware **firmware_p, const 
char *name,
goto out;
}
 
-   ret = _request_firmware_prepare(, name, device, buf, size);
+   ret = _request_firmware_prepare(, name, device, buf, size,
+   opt_flags);
if (ret <= 0) /* error or already assigned */
goto out;
 
-- 
2.18.0

Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware

[Luis R. Rodriguez]

On Mon, May 14, 2018 at 10:02:31PM -0400, Mimi Zohar wrote:
> On Mon, 2018-05-14 at 19:28 +0000, Luis R. Rodriguez wrote:
> > > - CONFIG_IMA_APPRAISE is not fine enough grained.
> > > 
> > > The CONFIG_IMA_APPRAISE_FIRMWARE will be a Kconfig option.  Similar
> > > Kconfig options will require kernel modules, kexec'ed image, and the
> > > IMA policy to be signed.
> > 
> > Sure, it is still unclear to me if CONFIG_IMA_APPRAISE_FIRMWARE will be
> > doing firmware verification in userspace or in the kernel.
> 
> The kernel is verifying signatures.
> 
> > > There are a number of reasons that the kernel should be verifying
> > > firmware signatures (eg. requiring a specific version of the firmware,
> > > that was locally signed).
> > 
> > Oh I agree, Linux enterprise distributions also have a strong reason to
> > have this, so that for instance we only trust and run vendor-approved
> > signed firmware. Otherwise the driver should reject the firmware. Every
> > now and then enterprise distros may run into cases were certain customers
> > may run oddball firmwares, and its unclear if we expect proper functionality
> > with that firmware. Having some form of firmware signing would help with
> > this pipeline, but this is currently dealt with at the packaging, and
> > noting other than logs ensures the driver is using an intended firmware.
> > But these needs *IMHO* have not been enough to push to generalize a kernel
> > firmware signing facility.
> 
> In order for IMA-appraisal to verify firmware signatures, the
> signatures need to be distributed with the firmware.  Perhaps this
> will be enough of an incentive for distros to start including firmware
> signatures in the packages.

Best to poke the maintainers about that... We have been sending mixed messages
about firmware signing over years now. Josh, heads up the new one is we can
do firmware signing through IMA future CONFIG_IMA_APPRAISE_FIRMWARE. I'll
bounce you a few emails related to this.

> > If CONFIG_IMA_APPRAISE_FIRMWARE is going to provide this functionality 
> > somehow
> > I'm happy to hear it.
> 
> The functionality has been there since commit 5a9196d ("ima: add
> support for measuring and appraising firmware").  The
> security_kernel_fw_from_file() hook was later replaced with the
> generic security_kernel_read_file() hook.

Groovy, its unclear from the code on that commit how this is done, so I
suppose I need to study this a bit more. Josh, do you grok it?

  Luis

Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware

[Luis R. Rodriguez]

On Mon, May 14, 2018 at 08:58:12AM -0400, Mimi Zohar wrote:
> On Fri, 2018-05-11 at 21:52 +0000, Luis R. Rodriguez wrote:
> > diff --git a/drivers/base/firmware_loader/main.c 
> > b/drivers/base/firmware_loader/main.c
> > index eb34089e4299..d7cdf04a8681 100644
> > --- a/drivers/base/firmware_loader/main.c
> > +++ b/drivers/base/firmware_loader/main.c
> > @@ -318,6 +318,11 @@ fw_get_filesystem_firmware(struct device *device, 
> > struct fw_priv *fw_priv)
> > break;
> > }
> > 
> > +#ifdef CONFIG_CFG80211_REQUIRE_SIGNED_REGDB
> > +   if ((strcmp(fw_priv->fw_name, "regulatory.db") == 0) ||
> > +   (strcmp(fw_priv->fw_name, "regulatory.db.p7s") == 0))
> > +   id = READING_FIRMWARE_REGULATORY_DB;
> > +#endif
> > fw_priv->size = 0;
> > rc = kernel_read_file_from_path(path, _priv->data, ,
> > msize, id);
> > 
> > This is eye-soring, and in turn would mean adding yet more #ifdefs for any
> > code on the kernel which open codes other firmware signing efforts with
> > its own kconfig...
> 
> Agreed, adding ifdef's is ugly.  As previously discussed, this code
> will be removed.
> 
> To coordinate the signature verification, at build time either regdb
> or IMA-appraisal firmware will be enabled. 

But not both, right?

> At runtime, in the case
> that regdb is enabled and a custom policy requires IMA-appraisal
> firmware signature verification, then both signature verification
> methods will verify the signatures.  If either fails, then the
> signature verification will fail.

OK so you're saying that if CONFIG_IMA_APPRAISE_FIRMWARE is disabled you can
still end up with CONFIG_CFG80211_REQUIRE_SIGNED_REGDB as enabled *and* a
custom policy which requires IMA-appraisal for the certain firmware signature
verifications?

> > Then as for my concern on extending and adding new READING_* ID tags
> > for other future open-coded firmware calls, since:
> > 
> > a) You seem to be working on  a CONFIG_IMA_APPRAISE_FIRMWARE which would
> > enable similar functionality as firmware signing but in userspace.
> 
> There are a number of different builtin policies.  The "secure_boot"
> builtin policy enables file signature verification for firmware,
> kernel modules, kexec'ed image, and the IMA policy, but builtin
> policies are enabled on the boot command line.
> 
> There are two problems:
> - there's no way of configuring a builtin policy to verify firmware
> signatures.

I'm not too familiar with IMA however it sounds like you can extend the IMA
built-in policy on the boot command line. If so I'll note MODULE_FIRMWARE()
macro is typically used to annotate firmware description -- not all drivers use
this though for a few reasons, for instance once is that some names are
constructed dynamically at run time. Consider how some drivers rev firmware
with versions, and as they do this they want to keep certain features only for
certain firmware versions. Despite this lack of a direct 1-1 mapping for all
firmwares needed, I *believe* one current use case for this macro is to extract
required firmwares needed on early boot so they can be stashed into the
initramfs if you have these modules enabled on the initramfs. Dracut folks
can confirm but -- dracut *seems* broken then given the semantic gap I
mentioned above.

dracut-init.sh:for _fw in $(modinfo -k $kernel -F firmware $1 2>/dev/null); 
do

If I read this correctly this just complains if the firmware file is
missing if the module is installed on initramfs and the firmware file
is not present. If so we have a current semantic gap and modules with
dynamic names are not handled. And its unclear which modules would be
affected. This is a not a big issue for dracut though since not all drivers
strive to be included on initramfs, unless their storage I suppose -- not
sure how common these storage drivers are for initramfs with dynamic firmware
names which do *not* use MODULE_FIRMWARE().

While the idea of MODULE_FIRMWARE() may need to be given some love or adjusted
to incorporate globs / regexps to fix this existing gap, or we come up with
something more reliable, if fixed, it in theory could end up being re-used to
enable you to extract and build policies for firmware signing at build time...

> - CONFIG_IMA_APPRAISE is not fine enough grained.
> 
> The CONFIG_IMA_APPRAISE_FIRMWARE will be a Kconfig option.  Similar
> Kconfig options will require kernel modules, kexec'ed image, and the
> IMA policy to be signed.

Sure, it is still unclear to me if CONFIG_IMA_APPRAISE_FIRMWARE will be
doing firmware verification in userspace or in the

Re: [PATCH 6/9] firmware: print firmware name on fallback path

[Luis R. Rodriguez]

On Sat, May 12, 2018 at 11:03:52AM +0300, Kalle Valo wrote:
> (sorry for the delay, this got buried in my inbox)
> 
> "Luis R. Rodriguez" <mcg...@kernel.org> writes:
> 
> > On Mon, Apr 23, 2018 at 04:12:02PM -0400, Andres Rodriguez wrote:
> >> Previously, one could assume the firmware name from the preceding
> >> message: "Direct firmware load for {name} failed with error %d".
> >> 
> >> However, with the new firmware_request_nowarn() entrypoint, the message
> >> outlined above will not always be printed.
> >
> > I though the whole point was to not print an error message. What if
> > we want later to disable this error message? This would prove a bit
> > pointless.
> >
> > Let's discuss the exact semantics desired here. Why would only the
> > fallback be desirable here?
> >
> > Andres, Kalle?
> 
> So from ath10k point of view we do not want to have any messages printed
> when calling firmware_request_nowarn(). The warnings get users really
> confused when ath10k is checking if an optional firmware file is
> available or not.

I figured, that is the intended functionality now.

  Luis

Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware

[Luis R. Rodriguez]

On Fri, May 11, 2018 at 01:00:26AM -0400, Mimi Zohar wrote:
> On Thu, 2018-05-10 at 23:26 +0000, Luis R. Rodriguez wrote:
> > On Wed, May 09, 2018 at 10:00:58PM -0400, Mimi Zohar wrote:
> > > On Wed, 2018-05-09 at 23:48 +, Luis R. Rodriguez wrote:
> > > > On Wed, May 09, 2018 at 06:06:57PM -0400, Mimi Zohar wrote:
> > > 
> > > > > > > Yes, writing regdb as a micro/mini LSM sounds reasonable.  The LSM
> > > > > > > would differentiate between other firmware and the regulatory.db 
> > > > > > > based
> > > > > > > on the firmware's pathname.
> > > > > > 
> > > > > > If that is the only way then it would be silly to do the mini LSM 
> > > > > > as all
> > > > > > calls would have to have the check. A special LSM hook for just the
> > > > > > regulatory db also doesn't make much sense.
> > > > > 
> > > > > All calls to request_firmware() are already going through this LSM
> > > > > hook.  I should have said, it would be based on both READING_FIRMWARE
> > > > > and the firmware's pathname.
> > > > 
> > > > Yes, but it would still be a strcmp() computation added for all
> > > > READING_FIRMWARE. In that sense, the current arrangement is only open 
> > > > coding the
> > > > signature verification for the regulatory.db file.  One way to avoid 
> > > > this would
> > > > be to add an LSM specific to the regulatory db
> > > 
> > > Casey already commented on this suggestion.
> > 
> > Sorry but I must have missed this, can you send me the email or URL where 
> > he did that?
> > I never got a copy of that email I think.
> 
> My mistake.  I've posted similar patches for kexec_load and for the
> firmware sysfs fallback, both call security_kernel_read_file().
> Casey's comment was in regards to kexec_load[1], not for the sysfs
> fallback mode.  Here's the link to the most recent version of the
> kexec_load patches.[2]
> 
> [1] 
> http://kernsec.org/pipermail/linux-security-module-archive/2018-May/006690.html
> [2] 
> http://kernsec.org/pipermail/linux-security-module-archive/2018-May/006854.html

It seems I share Eric's concern on these threads are over general architecture,
below some notes which I think may help for the long term on that regards.

In the firmware_loader case we have *one* subsystem which as open coded firmware
signing -- the wireless subsystem open codes firmware verification by doing two
request_firmware() calls, one for the regulatory.bin and one for 
regulatory.bin.p7s,
and then it does its own check. In this patch set you suggested adding
a new READING_FIRMWARE_REGULATORY_DB. But your first patch in the series also
adds READING_FIRMWARE_FALLBACK for the fallback case where we enable use of
the old syfs loading facility.

My concerns are two fold for this case:

a) This would mean adding a new READING_* ID tag per any kernel mechanism which 
open
codes its own signature verification scheme.

b) The way it was implemented was to do (just showing
READING_FIRMWARE_REGULATORY_DB here):

diff --git a/drivers/base/firmware_loader/main.c 
b/drivers/base/firmware_loader/main.c
index eb34089e4299..d7cdf04a8681 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -318,6 +318,11 @@ fw_get_filesystem_firmware(struct device *device, struct 
fw_priv *fw_priv)
break;
}

+#ifdef CONFIG_CFG80211_REQUIRE_SIGNED_REGDB
+   if ((strcmp(fw_priv->fw_name, "regulatory.db") == 0) ||
+   (strcmp(fw_priv->fw_name, "regulatory.db.p7s") == 0))
+   id = READING_FIRMWARE_REGULATORY_DB;
+#endif
fw_priv->size = 0;
rc = kernel_read_file_from_path(path, _priv->data, ,
msize, id);

This is eye-soring, and in turn would mean adding yet more #ifdefs for any
code on the kernel which open codes other firmware signing efforts with
its own kconfig...

I gather from reading the threads above that Eric's concerns are the re-use of
an API for security to read files for something which is really not a file, but
a binary blob of some sort and Casey's rebuttal is adding more hooks for small
things is a bad idea.

In light of all this I'll say that the concerns Eric has are unfortunately
too late, that ship has sailed eons ago. The old non-fd API for module loading
init_module() calls   security_kernel_read_file(NULL, READING_MODULE). Your
patch in this series adds security_kernel_read_file(NULL, 
READING_FIRMWARE_FALLBACK)
for the old syfs loading facility.

So in th

Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware

[Luis R. Rodriguez]

On Wed, May 09, 2018 at 10:00:58PM -0400, Mimi Zohar wrote:
> On Wed, 2018-05-09 at 23:48 +0000, Luis R. Rodriguez wrote:
> > On Wed, May 09, 2018 at 06:06:57PM -0400, Mimi Zohar wrote:
> 
> > > > > Yes, writing regdb as a micro/mini LSM sounds reasonable.  The LSM
> > > > > would differentiate between other firmware and the regulatory.db based
> > > > > on the firmware's pathname.
> > > > 
> > > > If that is the only way then it would be silly to do the mini LSM as all
> > > > calls would have to have the check. A special LSM hook for just the
> > > > regulatory db also doesn't make much sense.
> > > 
> > > All calls to request_firmware() are already going through this LSM
> > > hook.  I should have said, it would be based on both READING_FIRMWARE
> > > and the firmware's pathname.
> > 
> > Yes, but it would still be a strcmp() computation added for all
> > READING_FIRMWARE. In that sense, the current arrangement is only open 
> > coding the
> > signature verification for the regulatory.db file.  One way to avoid this 
> > would
> > be to add an LSM specific to the regulatory db
> 
> Casey already commented on this suggestion.

Sorry but I must have missed this, can you send me the email or URL where he 
did that?
I never got a copy of that email I think.

  Luis

[PATCH v7 01/14] firmware: wrap FW_OPT_* into an enum

[Luis R. Rodriguez]

From: Andres Rodriguez <andre...@gmail.com>

This should let us associate enum kdoc to these values.
While at it, kdocify the fw_opt.

Signed-off-by: Andres Rodriguez <andre...@gmail.com>
Reviewed-by: Kees Cook <keesc...@chromium.org>
Acked-by: Luis R. Rodriguez <mcg...@kernel.org>
[mcgrof: coding style fixes, merge kdoc with enum move]
Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 drivers/base/firmware_loader/fallback.c | 12 
 drivers/base/firmware_loader/fallback.h |  6 ++--
 drivers/base/firmware_loader/firmware.h | 37 +++--
 drivers/base/firmware_loader/main.c |  6 ++--
 4 files changed, 42 insertions(+), 19 deletions(-)

diff --git a/drivers/base/firmware_loader/fallback.c 
b/drivers/base/firmware_loader/fallback.c
index 358354148dec..b57a7b3b4122 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -512,7 +512,7 @@ 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, enum fw_opt opt_flags)
 {
struct fw_sysfs *fw_sysfs;
struct device *f_dev;
@@ -545,7 +545,7 @@ fw_create_instance(struct firmware *firmware, const char 
*fw_name,
  * In charge of constructing a sysfs fallback interface for firmware loading.
  **/
 static int fw_load_sysfs_fallback(struct fw_sysfs *fw_sysfs,
- unsigned int opt_flags, long timeout)
+ enum fw_opt opt_flags, long timeout)
 {
int retval = 0;
struct device *f_dev = _sysfs->dev;
@@ -599,7 +599,7 @@ static int fw_load_sysfs_fallback(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)
+   enum fw_opt opt_flags)
 {
struct fw_sysfs *fw_sysfs;
long timeout;
@@ -640,7 +640,7 @@ static int fw_load_from_user_helper(struct firmware 
*firmware,
return ret;
 }
 
-static bool fw_force_sysfs_fallback(unsigned int opt_flags)
+static bool fw_force_sysfs_fallback(enum fw_opt opt_flags)
 {
if (fw_fallback_config.force_sysfs_fallback)
return true;
@@ -649,7 +649,7 @@ static bool fw_force_sysfs_fallback(unsigned int opt_flags)
return true;
 }
 
-static bool fw_run_sysfs_fallback(unsigned int opt_flags)
+static bool fw_run_sysfs_fallback(enum fw_opt opt_flags)
 {
if (fw_fallback_config.ignore_sysfs_fallback) {
pr_info_once("Ignoring firmware sysfs fallback due to sysctl 
knob\n");
@@ -664,7 +664,7 @@ static bool fw_run_sysfs_fallback(unsigned int opt_flags)
 
 int fw_sysfs_fallback(struct firmware *fw, const char *name,
  struct device *device,
- unsigned int opt_flags,
+ enum fw_opt opt_flags,
  int ret)
 {
if (!fw_run_sysfs_fallback(opt_flags))
diff --git a/drivers/base/firmware_loader/fallback.h 
b/drivers/base/firmware_loader/fallback.h
index f8255670a663..a3b73a09db6c 100644
--- a/drivers/base/firmware_loader/fallback.h
+++ b/drivers/base/firmware_loader/fallback.h
@@ -5,6 +5,8 @@
 #include 
 #include 
 
+#include "firmware.h"
+
 /**
  * struct firmware_fallback_config - firmware fallback configuration settings
  *
@@ -31,7 +33,7 @@ struct firmware_fallback_config {
 #ifdef CONFIG_FW_LOADER_USER_HELPER
 int fw_sysfs_fallback(struct firmware *fw, const char *name,
  struct device *device,
- unsigned int opt_flags,
+ enum fw_opt opt_flags,
  int ret);
 void kill_pending_fw_fallback_reqs(bool only_kill_custom);
 
@@ -43,7 +45,7 @@ void unregister_sysfs_loader(void);
 #else /* CONFIG_FW_LOADER_USER_HELPER */
 static inline int fw_sysfs_fallback(struct firmware *fw, const char *name,
struct device *device,
-   unsigned int opt_flags,
+   enum fw_opt opt_flags,
int ret)
 {
/* Keep carrying over the same error */
diff --git a/drivers/base/firmware_loader/firmware.h 
b/drivers/base/firmware_loader/firmware.h
index 64acbb1a392c..4f433b447367 100644
--- a/drivers/base/firmware_loader/firmware.h
+++ b/drivers/base/firmware_loader/firmware.h
@@ -2,6 +2,7 @@
 #ifndef __FIRMWARE_LOADER_H
 #define __FIRMWARE_LOADER_H
 
+#include 
 #include 
 #include 
 #include 
@@ -10,13 +11,33 @@
 
 #include 
 
-/* firmware behavior options */
-#define FW_OPT_UEVENT  (1U << 0)
-#define FW_OPT_NOWAIT  (1U << 1)
-#define FW_OPT_USERHELPER  

[PATCH v7 02/14] firmware: use () to terminate kernel-doc function names

[Luis R. Rodriguez]

From: Andres Rodriguez <andre...@gmail.com>

The kernel-doc spec dictates a function name ends in ().

Signed-off-by: Andres Rodriguez <andre...@gmail.com>
Reviewed-by: Kees Cook <keesc...@chromium.org>
Acked-by: Randy Dunlap <rdun...@infradead.org>
Acked-by: Luis R. Rodriguez <mcg...@kernel.org>
[mcgrof: adjust since the wide API rename is not yet merged]
Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 drivers/base/firmware_loader/fallback.c |  8 
 drivers/base/firmware_loader/main.c | 22 +++---
 2 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/base/firmware_loader/fallback.c 
b/drivers/base/firmware_loader/fallback.c
index b57a7b3b4122..529f7013616f 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -125,7 +125,7 @@ static ssize_t timeout_show(struct class *class, struct 
class_attribute *attr,
 }
 
 /**
- * firmware_timeout_store - set number of seconds to wait for firmware
+ * firmware_timeout_store() - set number of seconds to wait for firmware
  * @class: device class pointer
  * @attr: device attribute pointer
  * @buf: buffer to scan for timeout value
@@ -239,7 +239,7 @@ static int map_fw_priv_pages(struct fw_priv *fw_priv)
 }
 
 /**
- * firmware_loading_store - set value in the 'loading' control file
+ * firmware_loading_store() - set value in the 'loading' control file
  * @dev: device pointer
  * @attr: device attribute pointer
  * @buf: buffer to scan for loading control value
@@ -431,7 +431,7 @@ static int fw_realloc_pages(struct fw_sysfs *fw_sysfs, int 
min_size)
 }
 
 /**
- * firmware_data_write - write method for firmware
+ * firmware_data_write() - write method for firmware
  * @filp: open sysfs file
  * @kobj: kobject for the device
  * @bin_attr: bin_attr structure
@@ -537,7 +537,7 @@ fw_create_instance(struct firmware *firmware, const char 
*fw_name,
 }
 
 /**
- * fw_load_sysfs_fallback - load a firmware via the sysfs fallback mechanism
+ * fw_load_sysfs_fallback() - load a firmware via the sysfs fallback mechanism
  * @fw_sysfs: firmware sysfs information for the firmware to load
  * @opt_flags: flags of options, FW_OPT_*
  * @timeout: timeout to wait for the load
diff --git a/drivers/base/firmware_loader/main.c 
b/drivers/base/firmware_loader/main.c
index 9919f0e6a7cc..4d11efadb3a4 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -597,7 +597,7 @@ _request_firmware(const struct firmware **firmware_p, const 
char *name,
 }
 
 /**
- * request_firmware: - send firmware request and wait for it
+ * request_firmware() - send firmware request and wait for it
  * @firmware_p: pointer to firmware image
  * @name: name of firmware file
  * @device: device for which firmware is being loaded
@@ -632,7 +632,7 @@ request_firmware(const struct firmware **firmware_p, const 
char *name,
 EXPORT_SYMBOL(request_firmware);
 
 /**
- * request_firmware_direct: - load firmware directly without usermode helper
+ * request_firmware_direct() - load firmware directly without usermode helper
  * @firmware_p: pointer to firmware image
  * @name: name of firmware file
  * @device: device for which firmware is being loaded
@@ -657,7 +657,7 @@ int request_firmware_direct(const struct firmware 
**firmware_p,
 EXPORT_SYMBOL_GPL(request_firmware_direct);
 
 /**
- * firmware_request_cache: - cache firmware for suspend so resume can use it
+ * firmware_request_cache() - cache firmware for suspend so resume can use it
  * @name: name of firmware file
  * @device: device for which firmware should be cached for
  *
@@ -681,7 +681,7 @@ int firmware_request_cache(struct device *device, const 
char *name)
 EXPORT_SYMBOL_GPL(firmware_request_cache);
 
 /**
- * request_firmware_into_buf - load firmware into a previously allocated buffer
+ * request_firmware_into_buf() - load firmware into a previously allocated 
buffer
  * @firmware_p: pointer to firmware image
  * @name: name of firmware file
  * @device: device for which firmware is being loaded and DMA region allocated
@@ -713,7 +713,7 @@ request_firmware_into_buf(const struct firmware 
**firmware_p, const char *name,
 EXPORT_SYMBOL(request_firmware_into_buf);
 
 /**
- * release_firmware: - release the resource associated with a firmware image
+ * release_firmware() - release the resource associated with a firmware image
  * @fw: firmware resource to release
  **/
 void release_firmware(const struct firmware *fw)
@@ -755,7 +755,7 @@ static void request_firmware_work_func(struct work_struct 
*work)
 }
 
 /**
- * request_firmware_nowait - asynchronous version of request_firmware
+ * request_firmware_nowait() - asynchronous version of request_firmware
  * @module: module requesting the firmware
  * @uevent: sends uevent to copy the firmware image if this flag
  * is non-zero else the firmware copy must be done manually.
@@ -824,7 +824,7 @@ EXPORT_SYMBOL(request_firmware_now

[PATCH v7 00/14] firmware_loader changes for v4.18

[Luis R. Rodriguez]

Greg,

This is what I have queued up for the firmware_loader for v4.18. Below
is a changelog of the changes, mostly addressing Andre's changes and a few
other set of cleanups and documentation updates on my part and a few
others pointed out by others.

Mimi will still be working on her changes later to add IMA firmware
signature support, that should not touch the firmware loader code at all,
and should help provide a generic alternative to the wireless regulatory
signing work -- for all firmware.

Hans's EFI changes are still being worked on.

These changes are available in git form on my linux-next [0] and linux [1]
trees on kernel.org on the branch name firmware_loader_for-v4.18. Hans,
feel free to work off of that for your next iteration.

Questions, and specially rants are greatly appreciated.

[0] 
https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux-next.git/log/?h=20180510-firmware_loader_for-v4.18
[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux.git/log/?h=20180510-firmware_loader_for-v4.18

v7:

- Annoted Reviewed-by tags.

- Installed codespell and am now using checkpatch.sh --codespell to check
each patch for spell check and blunders. Edited all files for
corrections missed by other reviewers.

- checkpatch complained about using ---help--- when I moved the Kconfig
file for the firmware loader so I replaced that now with what it
suggests.

- fixed the other series of typos pickd up by reviewers

v6:

- Added the firmware_request_nowarn() patch again, now that we've decided
on not to warn even for the fallback case.

v5:

- I took up Andres' patches and cleaned them up a bit further with
minor details which would have otherwise have taken a few more
iterations to do.

- Dropped the firmware_request_nowarn() patch since there was some
inconsistencies over actually not warning on the fallback case

- Added some further firmware loader Kconfig clarifications which have
come up through recent discussions.

- I dropped my firmware API rename changes for v4.18, so I adjusted
Andres' patches accordingly.

v4:

- Andres removed the async nowarn API call firmware_request_nowait2()
  from his series, given we got into a discussion over how to properly
add a new async API to the firmware loader and there was no real
consensus. Given I was tired of the bikeshedding I offered for folks
recommend a solution and for us to just go with it. None solution was
provided by any.  To not leave Andres hanging during patch review, I
made a set of recommendations but suggested that the asycn API be
punted until *after* we get the simple sync nowarn API added. The
recommendation still stands and my hope is that Andres will follow
up with it later:

https://lkml.kernel.org/r/20180422202609.gx14...@wotan.suse.de

- Andres dropped the drm/amdgpu/brcmfmac driver changes based on feedback
and becuase the async nowarn call was dropped in favor of addressing
this later.

v3:

- Andres fixed typos picked up by Randy Dunlap.

v2:

- Andres added request_firmware_nowait2()

- Based on feedback Andres extended his patches to include use of the new API
on the drivers amdgpu, ath10k and brcmfmac.

v1:

Andres only posted one patch to add request_firmware_optional() without
any users

Andres Rodriguez (6):
  firmware: wrap FW_OPT_* into an enum
  firmware: use () to terminate kernel-doc function names
  firmware: rename fw_sysfs_fallback to firmware_fallback_sysfs()
  firmware: add firmware_request_nowarn() - load firmware without
warnings
  ath10k: use firmware_request_nowarn() to load firmware
  ath10k: re-enable the firmware fallback mechanism for testmode

Luis R. Rodriguez (8):
  firmware_loader: document firmware_sysfs_fallback()
  firmware_loader: enhance Kconfig documentation over FW_LOADER
  firmware_loader: replace ---help--- with help
  firmware_loader: move kconfig FW_LOADER entries to its own file
  firmware_loader: make firmware_fallback_sysfs() print more useful
  Documentation: fix few typos and clarifications for the firmware
loader
  Documentation: remove stale firmware API reference
  Documentation: clarify firmware_class provenance and why we can't
rename the module

 Documentation/dell_rbu.txt|   5 +-
 .../firmware/fallback-mechanisms.rst  |  14 +-
 .../driver-api/firmware/firmware_cache.rst|   4 +-
 .../driver-api/firmware/request_firmware.rst  |   5 +
 drivers/base/Kconfig  |  90 ++
 drivers/base/firmware_loader/Kconfig  | 154 ++
 drivers/base/firmware_loader/fallback.c   |  53 --
 drivers/base/firmware_loader/fallback.h   |  18 +-
 drivers/base/firmware_loader/firmware.h   |  37 -
 drivers/base/firmware_loader/main.c   |  57 +--
 drivers/net/wireless/ath/ath10k/core.c|   2 +-
 drivers/net/wireless/ath/ath10k/testmode.c|   2 +-
 include/linux/firmware.h  |  10 ++
 13 files changed, 320 insertions(+), 131 deletions(-)
 create

[PATCH v7 03/14] firmware: rename fw_sysfs_fallback to firmware_fallback_sysfs()

[Luis R. Rodriguez]

From: Andres Rodriguez <andre...@gmail.com>

This is done since this call is now exposed through kernel-doc,
and since this also paves the way for different future types of
fallback mechanims.

Signed-off-by: Andres Rodriguez <andre...@gmail.com>
Reviewed-by: Kees Cook <keesc...@chromium.org>
Acked-by: Luis R. Rodriguez <mcg...@kernel.org>
[mcgrof: small coding style changes]
Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 drivers/base/firmware_loader/fallback.c |  8 
 drivers/base/firmware_loader/fallback.h | 16 
 drivers/base/firmware_loader/firmware.h |  2 +-
 drivers/base/firmware_loader/main.c |  2 +-
 4 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/base/firmware_loader/fallback.c 
b/drivers/base/firmware_loader/fallback.c
index 529f7013616f..3db9e0f225ac 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -662,10 +662,10 @@ static bool fw_run_sysfs_fallback(enum fw_opt opt_flags)
return fw_force_sysfs_fallback(opt_flags);
 }
 
-int fw_sysfs_fallback(struct firmware *fw, const char *name,
- struct device *device,
- enum fw_opt opt_flags,
- int ret)
+int firmware_fallback_sysfs(struct firmware *fw, const char *name,
+   struct device *device,
+   enum fw_opt opt_flags,
+   int ret)
 {
if (!fw_run_sysfs_fallback(opt_flags))
return ret;
diff --git a/drivers/base/firmware_loader/fallback.h 
b/drivers/base/firmware_loader/fallback.h
index a3b73a09db6c..21063503e4ea 100644
--- a/drivers/base/firmware_loader/fallback.h
+++ b/drivers/base/firmware_loader/fallback.h
@@ -31,10 +31,10 @@ struct firmware_fallback_config {
 };
 
 #ifdef CONFIG_FW_LOADER_USER_HELPER
-int fw_sysfs_fallback(struct firmware *fw, const char *name,
- struct device *device,
- enum fw_opt opt_flags,
- int ret);
+int firmware_fallback_sysfs(struct firmware *fw, const char *name,
+   struct device *device,
+   enum fw_opt opt_flags,
+   int ret);
 void kill_pending_fw_fallback_reqs(bool only_kill_custom);
 
 void fw_fallback_set_cache_timeout(void);
@@ -43,10 +43,10 @@ void fw_fallback_set_default_timeout(void);
 int register_sysfs_loader(void);
 void unregister_sysfs_loader(void);
 #else /* CONFIG_FW_LOADER_USER_HELPER */
-static inline int fw_sysfs_fallback(struct firmware *fw, const char *name,
-   struct device *device,
-   enum fw_opt opt_flags,
-   int ret)
+static inline int firmware_fallback_sysfs(struct firmware *fw, const char 
*name,
+ struct device *device,
+ enum fw_opt opt_flags,
+ int ret)
 {
/* Keep carrying over the same error */
return ret;
diff --git a/drivers/base/firmware_loader/firmware.h 
b/drivers/base/firmware_loader/firmware.h
index 4f433b447367..4c1395f8e7ed 100644
--- a/drivers/base/firmware_loader/firmware.h
+++ b/drivers/base/firmware_loader/firmware.h
@@ -20,7 +20,7 @@
  * @FW_OPT_NOWAIT: Used to describe the firmware request is asynchronous.
  * @FW_OPT_USERHELPER: Enable the fallback mechanism, in case the direct
  * filesystem lookup fails at finding the firmware.  For details refer to
- * fw_sysfs_fallback().
+ * firmware_fallback_sysfs().
  * @FW_OPT_NO_WARN: Quiet, avoid printing warning messages.
  * @FW_OPT_NOCACHE: Disables firmware caching. Firmware caching is used to
  * cache the firmware upon suspend, so that upon resume races against the
diff --git a/drivers/base/firmware_loader/main.c 
b/drivers/base/firmware_loader/main.c
index 4d11efadb3a4..abdc4e4d55f1 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -581,7 +581,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);
-   ret = fw_sysfs_fallback(fw, name, device, opt_flags, ret);
+   ret = firmware_fallback_sysfs(fw, name, device, opt_flags, ret);
} else
ret = assign_fw(fw, device, opt_flags);
 
-- 
2.17.0

[PATCH v7 05/14] firmware_loader: enhance Kconfig documentation over FW_LOADER

[Luis R. Rodriguez]

If you try to read FW_LOADER today it speaks of old riddles and
unless you have been following development closely you will lose
track of what is what. Even the documentation for PREVENT_FIRMWARE_BUILD
is a bit fuzzy and how it fits into this big picture.

Give the FW_LOADER kconfig documentation some love with more up to
date developments and recommendations. While at it, wrap the FW_LOADER
code into its own menu to compartmentalize and make it clearer which
components really are part of the FW_LOADER. This should also make
it easier to later move these kconfig entries into the firmware_loader/
directory later.

This also now recommends using firmwared [0] for folks left needing a
uevent handler in userspace for the sysfs firmware fallback mechanis
given udev's uevent firmware mechanism was ripped out a while ago.

[0] https://github.com/teg/firmwared

Reviewed-by: Kees Cook <keesc...@chromium.org>
Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 drivers/base/Kconfig | 165 ++-
 1 file changed, 131 insertions(+), 34 deletions(-)

diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
index 29b0eb452b3a..db2bbe483927 100644
--- a/drivers/base/Kconfig
+++ b/drivers/base/Kconfig
@@ -70,39 +70,64 @@ config STANDALONE
  If unsure, say Y.
 
 config PREVENT_FIRMWARE_BUILD
-   bool "Prevent firmware from being built"
+   bool "Disable drivers features which enable custom firmware building"
default y
help
- Say yes to avoid building firmware. Firmware is usually shipped
- with the driver and only when updating the firmware should a
- rebuild be made.
- If unsure, say Y here.
+ Say yes to disable driver features which enable building a custom
+ driver firmware at kernel build time. These drivers do not use the
+ kernel firmware API to load firmware (CONFIG_FW_LOADER), instead they
+ use their own custom loading mechanism. The required firmware is
+ usually shipped with the driver, building the driver firmware
+ should only be needed if you have an updated firmware source.
+
+ Firmware should not be being built as part of kernel, these days
+ you should always prevent this and say Y here. There are only two
+ old drivers which enable building of its firmware at kernel build
+ time:
+
+   o CONFIG_WANXL through CONFIG_WANXL_BUILD_FIRMWARE
+   o CONFIG_SCSI_AIC79XX through CONFIG_AIC79XX_BUILD_FIRMWARE
+
+menu "Firmware loader"
 
 config FW_LOADER
-   tristate "Userspace firmware loading support" if EXPERT
+   tristate "Firmware loading facility" if EXPERT
default y
---help---
- This option is provided for the case where none of the in-tree modules
- require userspace firmware loading support, but a module built
- out-of-tree does.
+ This enables the firmware loading facility in the kernel. The kernel
+ will first look for built-in firmware, if it has any. Next, it will
+ look for the requested firmware in a series of filesystem paths:
+
+   o firmware_class path module parameter or kernel boot param
+   o /lib/firmware/updates/UTS_RELEASE
+   o /lib/firmware/updates
+   o /lib/firmware/UTS_RELEASE
+   o /lib/firmware
+
+ Enabling this feature only increases your kernel image by about
+ 828 bytes, enable this option unless you are certain you don't
+ need firmware.
+
+ You typically want this built-in (=y) but you can also enable this
+ as a module, in which case the firmware_class module will be built.
+ You also want to be sure to enable this built-in if you are going to
+ enable built-in firmware (CONFIG_EXTRA_FIRMWARE).
+
+if FW_LOADER
 
 config EXTRA_FIRMWARE
-   string "External firmware blobs to build into the kernel binary"
-   depends on FW_LOADER
+   string "Build named firmware blobs into the kernel binary"
help
- Various drivers in the kernel source tree may require firmware,
- which is generally available in your distribution's linux-firmware
- package.
+ Device drivers which require firmware can typically deal with
+ having the kernel load firmware from the various supported
+ /lib/firmware/ paths. This option enables you to build into the
+ kernel firmware files. Built-in firmware searches are preceded
+ over firmware lookups using your filesystem over the supported
+ /lib/firmware paths documented on CONFIG_FW_LOADER.
 
- The linux-firmware package should install firmware into
- /lib/firmware/ on your system, so they can be loaded by userspace
- helpers on request.
-
- This option allows firmware to be bu

[PATCH v7 07/14] firmware_loader: move kconfig FW_LOADER entries to its own file

[Luis R. Rodriguez]

This will make it easier to track and easier to understand
what components and features are part of the FW_LOADER. There
are some components related to firmware which have *nothing* to
do with the FW_LOADER, souch as PREVENT_FIRMWARE_BUILD.

Reviewed-by: Kees Cook <keesc...@chromium.org>
Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 drivers/base/Kconfig | 155 +--
 drivers/base/firmware_loader/Kconfig | 154 ++
 2 files changed, 155 insertions(+), 154 deletions(-)
 create mode 100644 drivers/base/firmware_loader/Kconfig

diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
index 0c38df32c7fe..3e63a900b330 100644
--- a/drivers/base/Kconfig
+++ b/drivers/base/Kconfig
@@ -88,160 +88,7 @@ config PREVENT_FIRMWARE_BUILD
o CONFIG_WANXL through CONFIG_WANXL_BUILD_FIRMWARE
o CONFIG_SCSI_AIC79XX through CONFIG_AIC79XX_BUILD_FIRMWARE
 
-menu "Firmware loader"
-
-config FW_LOADER
-   tristate "Firmware loading facility" if EXPERT
-   default y
-   help
- This enables the firmware loading facility in the kernel. The kernel
- will first look for built-in firmware, if it has any. Next, it will
- look for the requested firmware in a series of filesystem paths:
-
-   o firmware_class path module parameter or kernel boot param
-   o /lib/firmware/updates/UTS_RELEASE
-   o /lib/firmware/updates
-   o /lib/firmware/UTS_RELEASE
-   o /lib/firmware
-
- Enabling this feature only increases your kernel image by about
- 828 bytes, enable this option unless you are certain you don't
- need firmware.
-
- You typically want this built-in (=y) but you can also enable this
- as a module, in which case the firmware_class module will be built.
- You also want to be sure to enable this built-in if you are going to
- enable built-in firmware (CONFIG_EXTRA_FIRMWARE).
-
-if FW_LOADER
-
-config EXTRA_FIRMWARE
-   string "Build named firmware blobs into the kernel binary"
-   help
- Device drivers which require firmware can typically deal with
- having the kernel load firmware from the various supported
- /lib/firmware/ paths. This option enables you to build into the
- kernel firmware files. Built-in firmware searches are preceded
- over firmware lookups using your filesystem over the supported
- /lib/firmware paths documented on CONFIG_FW_LOADER.
-
- This may be useful for testing or if the firmware is required early on
- in boot and cannot rely on the firmware being placed in an initrd or
- initramfs.
-
- This option is a string and takes the (space-separated) names of the
- firmware files -- the same names that appear in MODULE_FIRMWARE()
- and request_firmware() in the source. These files should exist under
- the directory specified by the EXTRA_FIRMWARE_DIR option, which is
- /lib/firmware by default.
-
- For example, you might set CONFIG_EXTRA_FIRMWARE="usb8388.bin", copy
- the usb8388.bin file into /lib/firmware, and build the kernel. Then
- any request_firmware("usb8388.bin") will be satisfied internally
- inside the kernel without ever looking at your filesystem at runtime.
-
- WARNING: If you include additional firmware files into your binary
- kernel image that are not available under the terms of the GPL,
- then it may be a violation of the GPL to distribute the resulting
- image since it combines both GPL and non-GPL work. You should
- consult a lawyer of your own before distributing such an image.
-
-config EXTRA_FIRMWARE_DIR
-   string "Firmware blobs root directory"
-   depends on EXTRA_FIRMWARE != ""
-   default "/lib/firmware"
-   help
- This option controls the directory in which the kernel build system
- looks for the firmware files listed in the EXTRA_FIRMWARE option.
-
-config FW_LOADER_USER_HELPER
-   bool "Enable the firmware sysfs fallback mechanism"
-   help
- This option enables a sysfs loading facility to enable firmware
- loading to the kernel through userspace as a fallback mechanism
- if and only if the kernel's direct filesystem lookup for the
- firmware failed using the different /lib/firmware/ paths, or the
- path specified in the firmware_class path module parameter, or the
- firmware_class path kernel boot parameter if the firmware_class is
- built-in. For details on how to work with the sysfs fallback mechanism
- refer to Documentation/driver-api/firmware/fallback-mechanisms.rst.
-
- The direct filesystem lookup for firmware is always used first now.
-

[PATCH v7 04/14] firmware_loader: document firmware_sysfs_fallback()

[Luis R. Rodriguez]

This also sets the expecations for future fallback interfaces, even
if they are not exported.

Reviewed-by: Kees Cook <keesc...@chromium.org>
Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 drivers/base/firmware_loader/fallback.c | 20 
 1 file changed, 20 insertions(+)

diff --git a/drivers/base/firmware_loader/fallback.c 
b/drivers/base/firmware_loader/fallback.c
index 3db9e0f225ac..9169e7b9800c 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -662,6 +662,26 @@ static bool fw_run_sysfs_fallback(enum fw_opt opt_flags)
return fw_force_sysfs_fallback(opt_flags);
 }
 
+/**
+ * firmware_fallback_sysfs() - use the fallback mechanism to find firmware
+ * @fw: pointer to firmware image
+ * @name: name of firmware file to look for
+ * @device: device for which firmware is being loaded
+ * @opt_flags: options to control firmware loading behaviour
+ * @ret: return value from direct lookup which triggered the fallback mechanism
+ *
+ * This function is called if direct lookup for the firmware failed, it enables
+ * a fallback mechanism through userspace by exposing a sysfs loading
+ * interface. Userspace is in charge of loading the firmware through the syfs
+ * loading interface. This syfs fallback mechanism may be disabled completely
+ * on a system by setting the proc sysctl value ignore_sysfs_fallback to true.
+ * If this false we check if the internal API caller set the @FW_OPT_NOFALLBACK
+ * flag, if so it would also disable the fallback mechanism. A system may want
+ * to enfoce the sysfs fallback mechanism at all times, it can do this by
+ * setting ignore_sysfs_fallback to false and force_sysfs_fallback to true.
+ * Enabling force_sysfs_fallback is functionally equivalent to build a kernel
+ * with CONFIG_FW_LOADER_USER_HELPER_FALLBACK.
+ **/
 int firmware_fallback_sysfs(struct firmware *fw, const char *name,
struct device *device,
enum fw_opt opt_flags,
-- 
2.17.0

[PATCH v7 08/14] firmware_loader: make firmware_fallback_sysfs() print more useful

[Luis R. Rodriguez]

If we resort to using the sysfs fallback mechanism we don't print
the filename. This can be deceiving given we could have a series of
callers intertwined and it'd be unclear exactly for what firmware
this was meant for.

Additionally, although we don't currently use FW_OPT_NO_WARN when
dealing with the fallback mechanism, we will soon, so just respect
its use consistently.

And even if you *don't* want to print always on failure, you may
want to print when debugging so enable dynamic debug print when
FW_OPT_NO_WARN is used.

Reviewed-by: Kees Cook <keesc...@chromium.org>
Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 drivers/base/firmware_loader/fallback.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/base/firmware_loader/fallback.c 
b/drivers/base/firmware_loader/fallback.c
index 9169e7b9800c..b676a99c469c 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -690,6 +690,11 @@ int firmware_fallback_sysfs(struct firmware *fw, const 
char *name,
if (!fw_run_sysfs_fallback(opt_flags))
return ret;
 
-   dev_warn(device, "Falling back to user helper\n");
+   if (!(opt_flags & FW_OPT_NO_WARN))
+   dev_warn(device, "Falling back to syfs fallback for: %s\n",
+name);
+   else
+   dev_dbg(device, "Falling back to sysfs fallback for: %s\n",
+   name);
return fw_load_from_user_helper(fw, name, device, opt_flags);
 }
-- 
2.17.0

[PATCH v7 10/14] ath10k: use firmware_request_nowarn() to load firmware

[Luis R. Rodriguez]

From: Andres Rodriguez <andre...@gmail.com>

This reduces the unnecessary spew when trying to load optional firmware:
"Direct firmware load for ... failed with error -2"

Signed-off-by: Andres Rodriguez <andre...@gmail.com>
Reviewed-by: Kees Cook <keesc...@chromium.org>
Acked-by: Kalle Valo <kv...@codeaurora.org>
Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 drivers/net/wireless/ath/ath10k/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/ath10k/core.c 
b/drivers/net/wireless/ath/ath10k/core.c
index 4cf54a7ef09a..ad4f6e3c0737 100644
--- a/drivers/net/wireless/ath/ath10k/core.c
+++ b/drivers/net/wireless/ath/ath10k/core.c
@@ -694,7 +694,7 @@ static const struct firmware *ath10k_fetch_fw_file(struct 
ath10k *ar,
dir = ".";
 
snprintf(filename, sizeof(filename), "%s/%s", dir, file);
-   ret = request_firmware(, filename, ar->dev);
+   ret = firmware_request_nowarn(, filename, ar->dev);
ath10k_dbg(ar, ATH10K_DBG_BOOT, "boot fw request '%s': %d\n",
   filename, ret);
 
-- 
2.17.0

[PATCH v7 09/14] firmware: add firmware_request_nowarn() - load firmware without warnings

[Luis R. Rodriguez]

From: Andres Rodriguez <andre...@gmail.com>

Currently the firmware loader only exposes one silent path for querying
optional firmware, and that is firmware_request_direct(). This function
also disables the sysfs fallback mechanism, which might not always be the
desired behaviour [0].

This patch introduces a variations of request_firmware() that enable the
caller to disable the undesired warning messages but enables the sysfs
fallback mechanism. This is equivalent to adding FW_OPT_NO_WARN to the
old behaviour.

[0]: https://git.kernel.org/linus/c0cc00f250e1

Signed-off-by: Andres Rodriguez <andre...@gmail.com>
Reviewed-by: Kees Cook <keesc...@chromium.org>
Acked-by: Luis R. Rodriguez <mcg...@kernel.org>
[mcgrof: used the old API calls as the full rename is not done yet, and
 add the caller for when FW_LOADER is disabled, enhance documentation ]
Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 .../driver-api/firmware/request_firmware.rst  |  5 
 drivers/base/firmware_loader/main.c   | 27 +++
 include/linux/firmware.h  | 10 +++
 3 files changed, 42 insertions(+)

diff --git a/Documentation/driver-api/firmware/request_firmware.rst 
b/Documentation/driver-api/firmware/request_firmware.rst
index d5ec95a7195b..f62bdcbfed5b 100644
--- a/Documentation/driver-api/firmware/request_firmware.rst
+++ b/Documentation/driver-api/firmware/request_firmware.rst
@@ -20,6 +20,11 @@ request_firmware
 .. kernel-doc:: drivers/base/firmware_loader/main.c
:functions: request_firmware
 
+firmware_request_nowarn
+---
+.. kernel-doc:: drivers/base/firmware_loader/main.c
+   :functions: firmware_request_nowarn
+
 request_firmware_direct
 ---
 .. kernel-doc:: drivers/base/firmware_loader/main.c
diff --git a/drivers/base/firmware_loader/main.c 
b/drivers/base/firmware_loader/main.c
index abdc4e4d55f1..0943e7065e0e 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -631,6 +631,33 @@ request_firmware(const struct firmware **firmware_p, const 
char *name,
 }
 EXPORT_SYMBOL(request_firmware);
 
+/**
+ * firmware_request_nowarn() - request for an optional fw module
+ * @firmware: pointer to firmware image
+ * @name: name of firmware file
+ * @device: device for which firmware is being loaded
+ *
+ * This function is similar in behaviour to request_firmware(), except
+ * it doesn't produce warning messages when the file is not found.
+ * The sysfs fallback mechanism is enabled if direct filesystem lookup fails,
+ * however, however failures to find the firmware file with it are still
+ * suppressed. It is therefore up to the driver to check for the return value
+ * of this call and to decide when to inform the users of errors.
+ **/
+int firmware_request_nowarn(const struct firmware **firmware, const char *name,
+   struct device *device)
+{
+   int ret;
+
+   /* Need to pin this module until return */
+   __module_get(THIS_MODULE);
+   ret = _request_firmware(firmware, name, device, NULL, 0,
+   FW_OPT_UEVENT | FW_OPT_NO_WARN);
+   module_put(THIS_MODULE);
+   return ret;
+}
+EXPORT_SYMBOL_GPL(firmware_request_nowarn);
+
 /**
  * request_firmware_direct() - load firmware directly without usermode helper
  * @firmware_p: pointer to firmware image
diff --git a/include/linux/firmware.h b/include/linux/firmware.h
index 41050417cafb..2dd566c91d44 100644
--- a/include/linux/firmware.h
+++ b/include/linux/firmware.h
@@ -42,6 +42,8 @@ struct builtin_fw {
 #if defined(CONFIG_FW_LOADER) || (defined(CONFIG_FW_LOADER_MODULE) && 
defined(MODULE))
 int request_firmware(const struct firmware **fw, const char *name,
 struct device *device);
+int firmware_request_nowarn(const struct firmware **fw, const char *name,
+   struct device *device);
 int request_firmware_nowait(
struct module *module, bool uevent,
const char *name, struct device *device, gfp_t gfp, void *context,
@@ -59,6 +61,14 @@ static inline int request_firmware(const struct firmware 
**fw,
 {
return -EINVAL;
 }
+
+static inline int firmware_request_nowarn(const struct firmware **fw,
+ const char *name,
+ struct device *device)
+{
+   return -EINVAL;
+}
+
 static inline int request_firmware_nowait(
struct module *module, bool uevent,
const char *name, struct device *device, gfp_t gfp, void *context,
-- 
2.17.0

[PATCH v7 14/14] Documentation: clarify firmware_class provenance and why we can't rename the module

[Luis R. Rodriguez]

Clarify the provenance of the firmware loader firmware_class module name
and why we cannot rename the module in the future.

Reviewed-by: Mauro Carvalho Chehab <mchehab+sams...@kernel.org>
Reviewed-by: Kees Cook <keesc...@chromium.org>
Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 .../driver-api/firmware/fallback-mechanisms.rst  | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/Documentation/driver-api/firmware/fallback-mechanisms.rst 
b/Documentation/driver-api/firmware/fallback-mechanisms.rst
index a39323ef7d29..d35fed65eae9 100644
--- a/Documentation/driver-api/firmware/fallback-mechanisms.rst
+++ b/Documentation/driver-api/firmware/fallback-mechanisms.rst
@@ -72,9 +72,12 @@ the firmware requested, and establishes it in the device 
hierarchy by
 associating the device used to make the request as the device's parent.
 The sysfs directory's file attributes are defined and controlled through
 the new device's class (firmware_class) and group (fw_dev_attr_groups).
-This is actually where the original firmware_class.c file name comes from,
-as originally the only firmware loading mechanism available was the
-mechanism we now use as a fallback mechanism.
+This is actually where the original firmware_class module name came from,
+given that originally the only firmware loading mechanism available was the
+mechanism we now use as a fallback mechanism, which registers a struct class
+firmware_class. Because the attributes exposed are part of the module name, the
+module name firmware_class cannot be renamed in the future, to ensure backward
+compatibility with old userspace.
 
 To load firmware using the sysfs interface we expose a loading indicator,
 and a file upload firmware into:
-- 
2.17.0

[PATCH v7 11/14] ath10k: re-enable the firmware fallback mechanism for testmode

[Luis R. Rodriguez]

From: Andres Rodriguez <andre...@gmail.com>

The ath10k testmode uses request_firmware_direct() in order to avoid
producing firmware load warnings. Disabling the fallback mechanism was a
side effect of disabling warnings.

We now have a new API that allows us to avoid warnings while keeping the
fallback mechanism enabled. So use that instead.

Signed-off-by: Andres Rodriguez <andre...@gmail.com>
Reviewed-by: Kees Cook <keesc...@chromium.org>
Acked-by: Kalle Valo <kv...@codeaurora.org>
Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 drivers/net/wireless/ath/ath10k/testmode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/ath10k/testmode.c 
b/drivers/net/wireless/ath/ath10k/testmode.c
index 568810b41657..c24ee616833c 100644
--- a/drivers/net/wireless/ath/ath10k/testmode.c
+++ b/drivers/net/wireless/ath/ath10k/testmode.c
@@ -157,7 +157,7 @@ static int ath10k_tm_fetch_utf_firmware_api_1(struct ath10k 
*ar,
 ar->hw_params.fw.dir, ATH10K_FW_UTF_FILE);
 
/* load utf firmware image */
-   ret = request_firmware_direct(_file->firmware, filename, ar->dev);
+   ret = firmware_request_nowarn(_file->firmware, filename, ar->dev);
ath10k_dbg(ar, ATH10K_DBG_TESTMODE, "testmode fw request '%s': %d\n",
   filename, ret);
 
-- 
2.17.0

[PATCH v7 13/14] Documentation: remove stale firmware API reference

[Luis R. Rodriguez]

It refers to a pending patch, but this was merged eons ago.

Reviewed-by: Mauro Carvalho Chehab <mchehab+sams...@kernel.org>
Reviewed-by: Kees Cook <keesc...@chromium.org>
Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 Documentation/dell_rbu.txt | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/Documentation/dell_rbu.txt b/Documentation/dell_rbu.txt
index 0fdb6aa2704c..5d1ce7bcd04d 100644
--- a/Documentation/dell_rbu.txt
+++ b/Documentation/dell_rbu.txt
@@ -121,10 +121,7 @@ read back the image downloaded.
 
 .. note::
 
-   This driver requires a patch for firmware_class.c which has the modified
-   request_firmware_nowait function.
-
-   Also after updating the BIOS image a user mode application needs to execute
+   After updating the BIOS image a user mode application needs to execute
code which sends the BIOS update request to the BIOS. So on the next reboot
the BIOS knows about the new image downloaded and it updates itself.
Also don't unload the rbu driver if the image has to be updated.
-- 
2.17.0

[PATCH v7 06/14] firmware_loader: replace ---help--- with help

[Luis R. Rodriguez]

As per checkpatch using help is preferred over ---help---.

Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 drivers/base/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
index db2bbe483927..0c38df32c7fe 100644
--- a/drivers/base/Kconfig
+++ b/drivers/base/Kconfig
@@ -93,7 +93,7 @@ menu "Firmware loader"
 config FW_LOADER
tristate "Firmware loading facility" if EXPERT
default y
-   ---help---
+   help
  This enables the firmware loading facility in the kernel. The kernel
  will first look for built-in firmware, if it has any. Next, it will
  look for the requested firmware in a series of filesystem paths:
-- 
2.17.0

[PATCH v7 12/14] Documentation: fix few typos and clarifications for the firmware loader

[Luis R. Rodriguez]

Fix a few typos, and clarify a few sentences.

Reviewed-by: Kees Cook <keesc...@chromium.org>
Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 Documentation/driver-api/firmware/fallback-mechanisms.rst | 5 +++--
 Documentation/driver-api/firmware/firmware_cache.rst  | 4 ++--
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/Documentation/driver-api/firmware/fallback-mechanisms.rst 
b/Documentation/driver-api/firmware/fallback-mechanisms.rst
index f353783ae0be..a39323ef7d29 100644
--- a/Documentation/driver-api/firmware/fallback-mechanisms.rst
+++ b/Documentation/driver-api/firmware/fallback-mechanisms.rst
@@ -83,7 +83,7 @@ and a file upload firmware into:
   * /sys/$DEVPATH/data
 
 To upload firmware you will echo 1 onto the loading file to indicate
-you are loading firmware. You then cat the firmware into the data file,
+you are loading firmware. You then write the firmware into the data file,
 and you notify the kernel the firmware is ready by echo'ing 0 onto
 the loading file.
 
@@ -136,7 +136,8 @@ by kobject uevents. This is specially exacerbated due to 
the fact that most
 distributions today disable CONFIG_FW_LOADER_USER_HELPER_FALLBACK.
 
 Refer to do_firmware_uevent() for details of the kobject event variables
-setup. Variables passwdd with a kobject add event:
+setup. The variables currently passed to userspace with a "kobject add"
+event are:
 
 * FIRMWARE=firmware name
 * TIMEOUT=timeout value
diff --git a/Documentation/driver-api/firmware/firmware_cache.rst 
b/Documentation/driver-api/firmware/firmware_cache.rst
index 2210e5bfb332..c2e69d9c6bf1 100644
--- a/Documentation/driver-api/firmware/firmware_cache.rst
+++ b/Documentation/driver-api/firmware/firmware_cache.rst
@@ -29,8 +29,8 @@ Some implementation details about the firmware cache setup:
 * If an asynchronous call is used the firmware cache is only set up for a
   device if if the second argument (uevent) to request_firmware_nowait() is
   true. When uevent is true it requests that a kobject uevent be sent to
-  userspace for the firmware request. For details refer to the Fackback
-  mechanism documented below.
+  userspace for the firmware request through the sysfs fallback mechanism
+  if the firmware file is not found.
 
 * If the firmware cache is determined to be needed as per the above two
   criteria the firmware cache is setup by adding a devres entry for the
-- 
2.17.0

Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware

[Luis R. Rodriguez]

On Wed, May 09, 2018 at 06:06:57PM -0400, Mimi Zohar wrote:
> On Wed, 2018-05-09 at 21:22 +0000, Luis R. Rodriguez wrote:
> > 
> > OK, its still not clear to what it will do. If it does not touch the 
> > firmware
> > loader code, and it just sets and configures IMA to do file signature 
> > checking
> > on its own, then yes I think both mechanisms would be doing the similar 
> > work.
> > 
> > Wouldn't IMA do file signature checks then for all files? Or it would just
> > enable this for whatever files userspace wishes to cover?
> 
> Enabling CONFIG_IMA_APPRAISE_FIRMWARE would enforce firmware
> signatures on all directly loaded firmware and fail any method of
> loading firmware that the signature couldn't be verified.

Ah, so a generic firmware signing mechanism via IMA. Sounds very sensible to me.
Specially in light of the fact that its what we recommend folks to consider
if they need to address firmware signing for devices which do not have the
ability to do hardware firmware signing verification on their own.

> > One of the things with READING_FIRMWARE_REGULATORY_DB is to also use and 
> > trust
> > the wireless-regdgb maintainer's key for this file, could IMA be configured 
> > to
> > do that?
> 
> IMA has its own trusted keyring.  So either the maintainer's key would
> need to be added to the IMA keyring,

I see so we'd need this documented somehow.

> or IMA-appraisal would need to use the regdb keyring.

Can you describe this a bit more, for those not too familiar with IMA, in terms
of what would be involved in the kernel? Or is this all userspace configuration
stuff?

> > Because that would be one difference here. The whole point of adding
> > CONFIG_CFG80211_REQUIRE_SIGNED_REGDB was to replace CRDA which is a 
> > userspace
> > component which checks the signature of regulatory.db before reading it and
> > passing data to the kernel from it.
> > 
> > Now, however silly it may be to have CONFIG_IMA_APPRAISE_FIRMWARE *and*
> > CONFIG_CFG80211_REQUIRE_SIGNED_REGDB, is your intent in this new patch set
> > you are mentioning, to still enable both to co-exist?
> 
> At build, either CONFIG_CFG80211_REQUIRE_SIGNED_REGDB or
> CONFIG_IMA_APPRAISE_FIRMWARE, where IMA is appraising all firwmare,
> would be enabled, not both.

OK.

> > > Yes, writing regdb as a micro/mini LSM sounds reasonable.  The LSM
> > > would differentiate between other firmware and the regulatory.db based
> > > on the firmware's pathname.
> > 
> > If that is the only way then it would be silly to do the mini LSM as all
> > calls would have to have the check. A special LSM hook for just the
> > regulatory db also doesn't make much sense.
> 
> All calls to request_firmware() are already going through this LSM
> hook.  I should have said, it would be based on both READING_FIRMWARE
> and the firmware's pathname.

Yes, but it would still be a strcmp() computation added for all
READING_FIRMWARE. In that sense, the current arrangement is only open coding the
signature verification for the regulatory.db file.  One way to avoid this would
be to add an LSM specific to the regulatory db and have the
security_check_regulatory_db() do what it needs per LSM, but that would mean
setting a precedent for open possibly open coded future firmware verification
call. Its not too crazy to consider if an end goal may be avoid further open
coded firmware signature verification hacks.

> > > Making regdb an LSM would have the same issues as currently - deciding
> > > if regdb, IMA-appraisal, or both verify the regdb's signature.
> > 
> > Its unclear to me why they can't co-exist yet and not have to touch
> > the firmware_loader code at all.
> 
> With the changes discussed above, they will co-exist.  Other than the
> Kconfig changes, I don't think it will touch the firmware_loader code.

Great.

  Luis

Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware

[Luis R. Rodriguez]

On Wed, May 09, 2018 at 03:57:18PM -0400, Mimi Zohar wrote:
> On Wed, 2018-05-09 at 19:15 +0000, Luis R. Rodriguez wrote:
> 
> > > > > If both are enabled, do we require both signatures or is one enough.
> > > > 
> > > > Good question. Considering it as a stacked LSM (although not implemented
> > > > as one), I'd say its up to who enabled the Kconfig entries. If IMA and
> > > > CONFIG_CFG80211_REQUIRE_SIGNED_REGDB are enabled then both. If someone 
> > > > enabled
> > > > IMA though, then surely I agree that enabling
> > > > CONFIG_CFG80211_REQUIRE_SIGNED_REGDB is stupid and redundant, but its 
> > > > up to the
> > > > system integrator to decide.
> > > 
> > > Just because IMA-appraisal is enabled in the kernel doesn't mean that
> > > firmware signatures will be verified.  That is a run time policy
> > > decision.
> > 
> > Sure, I accept this if IMA does not do signature verification. However
> > signature verification seems like a stackable LSM decision, no?
> 
> IMA-appraisal can be configured to enforce file signatures.  Refer to
> discussion below as to how.
> 
> > > > If we however want to make it clear that such things as
> > > > CONFIG_CFG80211_REQUIRE_SIGNED_REGDB are not required when IMA is 
> > > > enabled we
> > > > could just make the kconfig depend on !IMA or something?  Or perhaps a 
> > > > new
> > > > kconfig for IMA which if selected it means that drivers can opt to open 
> > > > code
> > > > *further* kernel signature verification, even though IMA already is 
> > > > sufficient.
> > > > Perhaps CONFIG_ENABLE_IMA_OVERLAPPING, and the driver depends on it?
> > > 
> > > The existing CONFIG_IMA_APPRAISE is not enough.  If there was a build
> > > time IMA config that translated into an IMA policy requiring firmware
> > > signature verification (eg. CONFIG_IMA_APPRAISE_FIRMWARE), this could
> > > be sorted out at build time.
> > 
> > I see makes sense.
> 
> Ok, so instead of introducing READING_FIRMWARE_REGULATORY_DB, I'll
> post patches introducing CONFIG_IMA_APPRAISE_FIRMWARE, as described
> above.

OK, its still not clear to what it will do. If it does not touch the firmware
loader code, and it just sets and configures IMA to do file signature checking
on its own, then yes I think both mechanisms would be doing the similar work.

Wouldn't IMA do file signature checks then for all files? Or it would just
enable this for whatever files userspace wishes to cover?

One of the things with READING_FIRMWARE_REGULATORY_DB is to also use and trust
the wireless-regdgb maintainer's key for this file, could IMA be configured to
do that?

Because that would be one difference here. The whole point of adding
CONFIG_CFG80211_REQUIRE_SIGNED_REGDB was to replace CRDA which is a userspace
component which checks the signature of regulatory.db before reading it and
passing data to the kernel from it.

Now, however silly it may be to have CONFIG_IMA_APPRAISE_FIRMWARE *and*
CONFIG_CFG80211_REQUIRE_SIGNED_REGDB, is your intent in this new patch set
you are mentioning, to still enable both to co-exist?

> > > > > Assigning a different id for regdb signed firmware allows LSMs and IMA
> > > > > to handle regdb files differently.
> > > > 
> > > > That's not the main concern here, its the precedent we are setting here 
> > > > for
> > > > any new kernel interface which open codes firmware signing on its own. 
> > > > What
> > > > you are doing means other kernel users who open codes their own firmware
> > > > signing may need to add yet-another reading ID. That doesn't either look
> > > > well on code, and seems kind of silly from a coding perspective given
> > > > the above, in which I clarify IMA still is doing its own appraisal on 
> > > > it.
> > > 
> > > Suppose,
> > > 
> > > 1. Either CONFIG_CFG80211_REQUIRE_SIGNED_REGDB or
> > > "CONFIG_IMA_APPRAISE_FIRMWARE" would be configured at build.
> > > 
> > > 2. If CONFIG_CFG80211_REQUIRE_SIGNED_REGDB is configured, not
> > > "CONFIG_IMA_APPRAISE_FIRMWARE", a custom IMA-policy rule that
> > > appraises the firmware signature could be defined.  In this case, both
> > > signature verification methods would be enforced.
> > > 
> > > then READING_FIRMWARE_REGULATORY_DB would not be needed.
> > 
> > True, however I'm suggesting that CONFIG_CFG80211_REQUIRE_SIGNED_REGDB
> > could just be a mini subsystem stackable LSM.
> 
> Yes, writing regdb as a micro/mini LSM sounds reasonable.  The LSM
> would differentiate between other firmware and the regulatory.db based
> on the firmware's pathname.

If that is the only way then it would be silly to do the mini LSM as all
calls would have to have the check. A special LSM hook for just the
regulatory db also doesn't make much sense.

> Making regdb an LSM would have the same issues as currently - deciding
> if regdb, IMA-appraisal, or both verify the regdb's signature.

Its unclear to me why they can't co-exist yet and not have to touch
the firmware_loader code at all.

  Luis

Re: [PATCH v6 05/13] firmware_loader: enhance Kconfig documentation over FW_LOADER

[Luis R. Rodriguez]

On Tue, May 08, 2018 at 03:42:33PM -0700, Kees Cook wrote:
> On Tue, May 8, 2018 at 11:12 AM, Luis R. Rodriguez <mcg...@kernel.org> wrote:
> > + This used to be the default firmware loading facility, and udev 
> > used
> > + to listen for uvents to load firmware for the kernel. The firmware
> > + loading facility functionality in udev has been removed, as such 
> > it
> > + can no longer be relied upon as a fallback mechanism. Linux no 
> > longer
> > + relies on or uses a fallback mechanism in userspace. If you need 
> > to
> > + rely on one refer to the permissively licensed firmwared:
> 
> Typo: firmware

Thanks fixed all typos except this one, this one is meant to be firmwared as
that is the name of the project, the url is below.

> 
> > +
> > + https://github.com/teg/firmwared

  Luis

Re: [PATCH v6 12/13] Documentation: remove stale firmware API reference

[Luis R. Rodriguez]

On Wed, May 09, 2018 at 12:12:09PM -0300, Mauro Carvalho Chehab wrote:
> Em Tue,  8 May 2018 11:12:46 -0700
> "Luis R. Rodriguez" <mcg...@kernel.org> escreveu:
> 
> > It refers to a pending patch, but this was merged eons ago.
> 
> Didn't know that such patch was already merged. Great!
> 
> Regards,
> Mauro
> 
> > 
> > Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
> > ---
> >  Documentation/dell_rbu.txt | 3 ---
> >  1 file changed, 3 deletions(-)
> > 
> > diff --git a/Documentation/dell_rbu.txt b/Documentation/dell_rbu.txt
> > index 0fdb6aa2704c..077fdc29a0d0 100644
> > --- a/Documentation/dell_rbu.txt
> > +++ b/Documentation/dell_rbu.txt
> > @@ -121,9 +121,6 @@ read back the image downloaded.
> >  
> >  .. note::
> >  
> > -   This driver requires a patch for firmware_class.c which has the modified
> > -   request_firmware_nowait function.
> > -
> 
> > Also after updating the BIOS image a user mode application needs to 
> > execute
> > code which sends the BIOS update request to the BIOS. So on the next 
> > reboot
> > the BIOS knows about the new image downloaded and it updates itself.
> 
> You should likely remove the "Also" here.

Good catch. Will adjust.

> 
> With that,
> 
> Reviewed-by: Mauro Carvalho Chehab <mchehab+sams...@kernel.org>

Great, thanks.

  Luis

Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware

[Luis R. Rodriguez]

On Wed, May 09, 2018 at 07:30:28AM -0400, Mimi Zohar wrote:
> On Tue, 2018-05-08 at 17:34 +0000, Luis R. Rodriguez wrote:
> > On Thu, May 03, 2018 at 08:24:26PM -0400, Mimi Zohar wrote:
> > > On Fri, 2018-05-04 at 00:07 +, Luis R. Rodriguez wrote:
> > > > On Tue, May 01, 2018 at 09:48:20AM -0400, Mimi Zohar wrote:
> > > > > Allow LSMs and IMA to differentiate between signed regulatory.db and
> > > > > other firmware.
> > > > > 
> > > > > Signed-off-by: Mimi Zohar <zo...@linux.vnet.ibm.com>
> > > > > Cc: Luis R. Rodriguez <mcg...@suse.com>
> > > > > Cc: David Howells <dhowe...@redhat.com>
> > > > > Cc: Kees Cook <keesc...@chromium.org>
> > > > > Cc: Seth Forshee <seth.fors...@canonical.com>
> > > > > Cc: Johannes Berg <johannes.b...@intel.com>
> > > > > ---
> > > > >  drivers/base/firmware_loader/main.c | 5 +
> > > > >  include/linux/fs.h  | 1 +
> > > > >  2 files changed, 6 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/base/firmware_loader/main.c 
> > > > > b/drivers/base/firmware_loader/main.c
> > > > > index eb34089e4299..d7cdf04a8681 100644
> > > > > --- a/drivers/base/firmware_loader/main.c
> > > > > +++ b/drivers/base/firmware_loader/main.c
> > > > > @@ -318,6 +318,11 @@ fw_get_filesystem_firmware(struct device 
> > > > > *device, struct fw_priv *fw_priv)
> > > > >   break;
> > > > >   }
> > > > >  
> > > > > +#ifdef CONFIG_CFG80211_REQUIRE_SIGNED_REGDB
> > > > > + if ((strcmp(fw_priv->fw_name, "regulatory.db") == 0) ||
> > > > > + (strcmp(fw_priv->fw_name, "regulatory.db.p7s") == 
> > > > > 0))
> > > > > + id = READING_FIRMWARE_REGULATORY_DB;
> > > > > +#endif
> > > > 
> > > > Whoa, no way.
> > > 
> > > There are two methods for the kernel to verify firmware signatures.
> > 
> > Yes, but although CONFIG_CFG80211_REQUIRE_SIGNED_REGDB is its own kernel
> > mechanism to verify firmware it uses the request_firmware*() API for
> > regulatory.db and regulatory.db.p7s, and IMA already can appraise these two
> > files since the firmware API is used.
> 
> IMA-appraisal can verify a signature stored as an xattr, but not a
> detached signature.  That support could be added, but isn't there
> today.  Today, a regulatory.db signature would have to be stored as an
> xattr. 

Right, my point was that if someone has IMA installed:

a) they would add those xattr to files in /lib/firmware/ already
b) upon request_firmware*() calls a security hook would trigger
   which would enable IMA to appraise those files. So not only
   would the kernel in turn do double checks on regulatory.db,
   but also a check on regulatory.db.p7s as well.

The difference I suppose is IMA would use a hash function instead of signature
check, correct?

> > As such I see no reason to add a new ID for them at all.
> > K
> > Its not providing an *alternative*, its providing an *extra* kernel measure.
> > If anything CONFIG_CFG80211_REQUIRE_SIGNED_REGDB perhaps should be its own
> > stacked LSM. I'd be open to see patches which set that out. May be a
> > cleaner interface.
> > 
> > > If both are enabled, do we require both signatures or is one enough.
> > 
> > Good question. Considering it as a stacked LSM (although not implemented
> > as one), I'd say its up to who enabled the Kconfig entries. If IMA and
> > CONFIG_CFG80211_REQUIRE_SIGNED_REGDB are enabled then both. If someone 
> > enabled
> > IMA though, then surely I agree that enabling
> > CONFIG_CFG80211_REQUIRE_SIGNED_REGDB is stupid and redundant, but its up to 
> > the
> > system integrator to decide.
> 
> Just because IMA-appraisal is enabled in the kernel doesn't mean that
> firmware signatures will be verified.  That is a run time policy
> decision.

Sure, I accept this if IMA does not do signature verification. However
signature verification seems like a stackable LSM decision, no?

> > If we however want to make it clear that such things as
> > CONFIG_CFG80211_REQUIRE_SIGNED_REGDB are not required when IMA is enabled we
> > could just make the kconfig depend on !IMA or something?  Or perhaps a new
> > kconfig for IMA which if selected it means that drivers can opt to open co

[PATCH v6 01/13] firmware: wrap FW_OPT_* into an enum

[Luis R. Rodriguez]

From: Andres Rodriguez <andre...@gmail.com>

This should let us associate enum kdoc to these values.
While at it, kdocify the fw_opt.

Signed-off-by: Andres Rodriguez <andre...@gmail.com>
Acked-by: Luis R. Rodriguez <mcg...@kernel.org>
[mcgrof: coding style fixes, merge kdoc with enum move]
Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 drivers/base/firmware_loader/fallback.c | 12 
 drivers/base/firmware_loader/fallback.h |  6 ++--
 drivers/base/firmware_loader/firmware.h | 37 +++--
 drivers/base/firmware_loader/main.c |  6 ++--
 4 files changed, 42 insertions(+), 19 deletions(-)

diff --git a/drivers/base/firmware_loader/fallback.c 
b/drivers/base/firmware_loader/fallback.c
index 358354148dec..b57a7b3b4122 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -512,7 +512,7 @@ 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, enum fw_opt opt_flags)
 {
struct fw_sysfs *fw_sysfs;
struct device *f_dev;
@@ -545,7 +545,7 @@ fw_create_instance(struct firmware *firmware, const char 
*fw_name,
  * In charge of constructing a sysfs fallback interface for firmware loading.
  **/
 static int fw_load_sysfs_fallback(struct fw_sysfs *fw_sysfs,
- unsigned int opt_flags, long timeout)
+ enum fw_opt opt_flags, long timeout)
 {
int retval = 0;
struct device *f_dev = _sysfs->dev;
@@ -599,7 +599,7 @@ static int fw_load_sysfs_fallback(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)
+   enum fw_opt opt_flags)
 {
struct fw_sysfs *fw_sysfs;
long timeout;
@@ -640,7 +640,7 @@ static int fw_load_from_user_helper(struct firmware 
*firmware,
return ret;
 }
 
-static bool fw_force_sysfs_fallback(unsigned int opt_flags)
+static bool fw_force_sysfs_fallback(enum fw_opt opt_flags)
 {
if (fw_fallback_config.force_sysfs_fallback)
return true;
@@ -649,7 +649,7 @@ static bool fw_force_sysfs_fallback(unsigned int opt_flags)
return true;
 }
 
-static bool fw_run_sysfs_fallback(unsigned int opt_flags)
+static bool fw_run_sysfs_fallback(enum fw_opt opt_flags)
 {
if (fw_fallback_config.ignore_sysfs_fallback) {
pr_info_once("Ignoring firmware sysfs fallback due to sysctl 
knob\n");
@@ -664,7 +664,7 @@ static bool fw_run_sysfs_fallback(unsigned int opt_flags)
 
 int fw_sysfs_fallback(struct firmware *fw, const char *name,
  struct device *device,
- unsigned int opt_flags,
+ enum fw_opt opt_flags,
  int ret)
 {
if (!fw_run_sysfs_fallback(opt_flags))
diff --git a/drivers/base/firmware_loader/fallback.h 
b/drivers/base/firmware_loader/fallback.h
index f8255670a663..a3b73a09db6c 100644
--- a/drivers/base/firmware_loader/fallback.h
+++ b/drivers/base/firmware_loader/fallback.h
@@ -5,6 +5,8 @@
 #include 
 #include 
 
+#include "firmware.h"
+
 /**
  * struct firmware_fallback_config - firmware fallback configuration settings
  *
@@ -31,7 +33,7 @@ struct firmware_fallback_config {
 #ifdef CONFIG_FW_LOADER_USER_HELPER
 int fw_sysfs_fallback(struct firmware *fw, const char *name,
  struct device *device,
- unsigned int opt_flags,
+ enum fw_opt opt_flags,
  int ret);
 void kill_pending_fw_fallback_reqs(bool only_kill_custom);
 
@@ -43,7 +45,7 @@ void unregister_sysfs_loader(void);
 #else /* CONFIG_FW_LOADER_USER_HELPER */
 static inline int fw_sysfs_fallback(struct firmware *fw, const char *name,
struct device *device,
-   unsigned int opt_flags,
+   enum fw_opt opt_flags,
int ret)
 {
/* Keep carrying over the same error */
diff --git a/drivers/base/firmware_loader/firmware.h 
b/drivers/base/firmware_loader/firmware.h
index 64acbb1a392c..4f433b447367 100644
--- a/drivers/base/firmware_loader/firmware.h
+++ b/drivers/base/firmware_loader/firmware.h
@@ -2,6 +2,7 @@
 #ifndef __FIRMWARE_LOADER_H
 #define __FIRMWARE_LOADER_H
 
+#include 
 #include 
 #include 
 #include 
@@ -10,13 +11,33 @@
 
 #include 
 
-/* firmware behavior options */
-#define FW_OPT_UEVENT  (1U << 0)
-#define FW_OPT_NOWAIT  (1U << 1)
-#define FW_OPT_USERHELPER  (1U << 2)
-#define

[PATCH v6 04/13] firmware_loader: document firmware_sysfs_fallback()

[Luis R. Rodriguez]

This also sets the expecations for future fallback interfaces, even
if they are not exported.

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

diff --git a/drivers/base/firmware_loader/fallback.c 
b/drivers/base/firmware_loader/fallback.c
index 3db9e0f225ac..9169e7b9800c 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -662,6 +662,26 @@ static bool fw_run_sysfs_fallback(enum fw_opt opt_flags)
return fw_force_sysfs_fallback(opt_flags);
 }
 
+/**
+ * firmware_fallback_sysfs() - use the fallback mechanism to find firmware
+ * @fw: pointer to firmware image
+ * @name: name of firmware file to look for
+ * @device: device for which firmware is being loaded
+ * @opt_flags: options to control firmware loading behaviour
+ * @ret: return value from direct lookup which triggered the fallback mechanism
+ *
+ * This function is called if direct lookup for the firmware failed, it enables
+ * a fallback mechanism through userspace by exposing a sysfs loading
+ * interface. Userspace is in charge of loading the firmware through the syfs
+ * loading interface. This syfs fallback mechanism may be disabled completely
+ * on a system by setting the proc sysctl value ignore_sysfs_fallback to true.
+ * If this false we check if the internal API caller set the @FW_OPT_NOFALLBACK
+ * flag, if so it would also disable the fallback mechanism. A system may want
+ * to enfoce the sysfs fallback mechanism at all times, it can do this by
+ * setting ignore_sysfs_fallback to false and force_sysfs_fallback to true.
+ * Enabling force_sysfs_fallback is functionally equivalent to build a kernel
+ * with CONFIG_FW_LOADER_USER_HELPER_FALLBACK.
+ **/
 int firmware_fallback_sysfs(struct firmware *fw, const char *name,
struct device *device,
enum fw_opt opt_flags,
-- 
2.17.0

[PATCH v6 02/13] firmware: use () to terminate kernel-doc function names

[Luis R. Rodriguez]

From: Andres Rodriguez <andre...@gmail.com>

The kernel-doc spec dictates a function name ends in ().

Signed-off-by: Andres Rodriguez <andre...@gmail.com>
Acked-by: Randy Dunlap <rdun...@infradead.org>
Acked-by: Luis R. Rodriguez <mcg...@kernel.org>
[mcgrof: adjust since the wide API rename is not yet merged]
Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 drivers/base/firmware_loader/fallback.c |  8 
 drivers/base/firmware_loader/main.c | 22 +++---
 2 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/base/firmware_loader/fallback.c 
b/drivers/base/firmware_loader/fallback.c
index b57a7b3b4122..529f7013616f 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -125,7 +125,7 @@ static ssize_t timeout_show(struct class *class, struct 
class_attribute *attr,
 }
 
 /**
- * firmware_timeout_store - set number of seconds to wait for firmware
+ * firmware_timeout_store() - set number of seconds to wait for firmware
  * @class: device class pointer
  * @attr: device attribute pointer
  * @buf: buffer to scan for timeout value
@@ -239,7 +239,7 @@ static int map_fw_priv_pages(struct fw_priv *fw_priv)
 }
 
 /**
- * firmware_loading_store - set value in the 'loading' control file
+ * firmware_loading_store() - set value in the 'loading' control file
  * @dev: device pointer
  * @attr: device attribute pointer
  * @buf: buffer to scan for loading control value
@@ -431,7 +431,7 @@ static int fw_realloc_pages(struct fw_sysfs *fw_sysfs, int 
min_size)
 }
 
 /**
- * firmware_data_write - write method for firmware
+ * firmware_data_write() - write method for firmware
  * @filp: open sysfs file
  * @kobj: kobject for the device
  * @bin_attr: bin_attr structure
@@ -537,7 +537,7 @@ fw_create_instance(struct firmware *firmware, const char 
*fw_name,
 }
 
 /**
- * fw_load_sysfs_fallback - load a firmware via the sysfs fallback mechanism
+ * fw_load_sysfs_fallback() - load a firmware via the sysfs fallback mechanism
  * @fw_sysfs: firmware sysfs information for the firmware to load
  * @opt_flags: flags of options, FW_OPT_*
  * @timeout: timeout to wait for the load
diff --git a/drivers/base/firmware_loader/main.c 
b/drivers/base/firmware_loader/main.c
index 9919f0e6a7cc..4d11efadb3a4 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -597,7 +597,7 @@ _request_firmware(const struct firmware **firmware_p, const 
char *name,
 }
 
 /**
- * request_firmware: - send firmware request and wait for it
+ * request_firmware() - send firmware request and wait for it
  * @firmware_p: pointer to firmware image
  * @name: name of firmware file
  * @device: device for which firmware is being loaded
@@ -632,7 +632,7 @@ request_firmware(const struct firmware **firmware_p, const 
char *name,
 EXPORT_SYMBOL(request_firmware);
 
 /**
- * request_firmware_direct: - load firmware directly without usermode helper
+ * request_firmware_direct() - load firmware directly without usermode helper
  * @firmware_p: pointer to firmware image
  * @name: name of firmware file
  * @device: device for which firmware is being loaded
@@ -657,7 +657,7 @@ int request_firmware_direct(const struct firmware 
**firmware_p,
 EXPORT_SYMBOL_GPL(request_firmware_direct);
 
 /**
- * firmware_request_cache: - cache firmware for suspend so resume can use it
+ * firmware_request_cache() - cache firmware for suspend so resume can use it
  * @name: name of firmware file
  * @device: device for which firmware should be cached for
  *
@@ -681,7 +681,7 @@ int firmware_request_cache(struct device *device, const 
char *name)
 EXPORT_SYMBOL_GPL(firmware_request_cache);
 
 /**
- * request_firmware_into_buf - load firmware into a previously allocated buffer
+ * request_firmware_into_buf() - load firmware into a previously allocated 
buffer
  * @firmware_p: pointer to firmware image
  * @name: name of firmware file
  * @device: device for which firmware is being loaded and DMA region allocated
@@ -713,7 +713,7 @@ request_firmware_into_buf(const struct firmware 
**firmware_p, const char *name,
 EXPORT_SYMBOL(request_firmware_into_buf);
 
 /**
- * release_firmware: - release the resource associated with a firmware image
+ * release_firmware() - release the resource associated with a firmware image
  * @fw: firmware resource to release
  **/
 void release_firmware(const struct firmware *fw)
@@ -755,7 +755,7 @@ static void request_firmware_work_func(struct work_struct 
*work)
 }
 
 /**
- * request_firmware_nowait - asynchronous version of request_firmware
+ * request_firmware_nowait() - asynchronous version of request_firmware
  * @module: module requesting the firmware
  * @uevent: sends uevent to copy the firmware image if this flag
  * is non-zero else the firmware copy must be done manually.
@@ -824,7 +824,7 @@ EXPORT_SYMBOL(request_firmware_nowait);
 static ASYNC_DOMAIN_EXCLUSIVE(fw_cache_domain);
 
 /**

[PATCH v6 05/13] firmware_loader: enhance Kconfig documentation over FW_LOADER

[Luis R. Rodriguez]

If you try to read FW_LOADER today it speaks of old riddles and
unless you have been following development closely you will loose
track of what is what. Even the documentation for PREVENT_FIRMWARE_BUILD
is a bit fuzzy and how it fits into this big picture.

Give the FW_LOADER kconfig documentation some love with more up to
date developments and recommendations. While at it, wrap the FW_LOADER
code into its own menu to compartamentalize and make it clearer which
components really are part of the FW_LOADER. This should also make
it easier to later move these kconfig entries into the firmware_loader/
directory later.

This also now recommends using firmwared [0] for folks left needing a uevent
handler in userspace for the sysfs firmware fallback mechanis given udev's
uevent firmware mechanism was ripped out a while ago.

[0] https://github.com/teg/firmwared

Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 drivers/base/Kconfig | 165 ++-
 1 file changed, 131 insertions(+), 34 deletions(-)

diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
index 29b0eb452b3a..a4fe86caecca 100644
--- a/drivers/base/Kconfig
+++ b/drivers/base/Kconfig
@@ -70,39 +70,64 @@ config STANDALONE
  If unsure, say Y.
 
 config PREVENT_FIRMWARE_BUILD
-   bool "Prevent firmware from being built"
+   bool "Disable drivers features which enable custom firmware building"
default y
help
- Say yes to avoid building firmware. Firmware is usually shipped
- with the driver and only when updating the firmware should a
- rebuild be made.
- If unsure, say Y here.
+ Say yes to disable driver features which enable building a custom
+ driver firmwar at kernel build time. These drivers do not use the
+ kernel firmware API to load firmware (CONFIG_FW_LOADER), instead they
+ use their own custom loading mechanism. The required firmware is
+ usually shipped with the driver, building the driver firmware
+ should only be needed if you have an updated firmware source.
+
+ Firmware should not be being built as part of kernel, these days
+ you should always prevent this and say Y here. There are only two
+ old drivers which enable building of its firmware at kernel build
+ time:
+
+   o CONFIG_WANXL through CONFIG_WANXL_BUILD_FIRMWARE
+   o CONFIG_SCSI_AIC79XX through CONFIG_AIC79XX_BUILD_FIRMWARE
+
+menu "Firmware loader"
 
 config FW_LOADER
-   tristate "Userspace firmware loading support" if EXPERT
+   tristate "Firmware loading facility" if EXPERT
default y
---help---
- This option is provided for the case where none of the in-tree modules
- require userspace firmware loading support, but a module built
- out-of-tree does.
+ This enables the firmware loading facility in the kernel. The kernel
+ will first look for built-in firmware, if it has any. Next, it will
+ look for the requested firmware in a series of filesystem paths:
+
+   o firmware_class path module parameter or kernel boot param
+   o /lib/firmware/updates/UTS_RELEASE
+   o /lib/firmware/updates
+   o /lib/firmware/UTS_RELEASE
+   o /lib/firmware
+
+ Enabling this feature only increases your kernel image by about
+ 828 bytes, enable this option unless you are certain you don't
+ need firmware.
+
+ You typically want this built-in (=y) but you can also enable this
+ as a module, in which case the firmware_class module will be built.
+ You also want to be sure to enable this built-in if you are going to
+ enable built-in firmware (CONFIG_EXTRA_FIRMWARE).
+
+if FW_LOADER
 
 config EXTRA_FIRMWARE
-   string "External firmware blobs to build into the kernel binary"
-   depends on FW_LOADER
+   string "Build these firmware blobs into the kernel binary"
help
- Various drivers in the kernel source tree may require firmware,
- which is generally available in your distribution's linux-firmware
- package.
+ Device drivers which require firmware can typically deal with
+ having the kernel load firmware from the various supported
+ /lib/firmware/ paths. This option enables you to build into the
+ kernel firmware files. Built-in firmware searches are preceeded
+ over firmware lookups using your filesystem over the supported
+ /lib/firmware paths documented on CONFIG_FW_LOADER.
 
- The linux-firmware package should install firmware into
- /lib/firmware/ on your system, so they can be loaded by userspace
- helpers on request.
-
- This option allows firmware to be built into the kernel for the case
- where t

[PATCH v6 03/13] firmware: rename fw_sysfs_fallback to firmware_fallback_sysfs()

[Luis R. Rodriguez]

From: Andres Rodriguez <andre...@gmail.com>

This is done since this call is now exposed through kernel-doc,
and since this also paves the way for different future types of
fallback mechanims.

Signed-off-by: Andres Rodriguez <andre...@gmail.com>
Acked-by: Luis R. Rodriguez <mcg...@kernel.org>
[mcgrof: small coding style changes]
Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 drivers/base/firmware_loader/fallback.c |  8 
 drivers/base/firmware_loader/fallback.h | 16 
 drivers/base/firmware_loader/firmware.h |  2 +-
 drivers/base/firmware_loader/main.c |  2 +-
 4 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/base/firmware_loader/fallback.c 
b/drivers/base/firmware_loader/fallback.c
index 529f7013616f..3db9e0f225ac 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -662,10 +662,10 @@ static bool fw_run_sysfs_fallback(enum fw_opt opt_flags)
return fw_force_sysfs_fallback(opt_flags);
 }
 
-int fw_sysfs_fallback(struct firmware *fw, const char *name,
- struct device *device,
- enum fw_opt opt_flags,
- int ret)
+int firmware_fallback_sysfs(struct firmware *fw, const char *name,
+   struct device *device,
+   enum fw_opt opt_flags,
+   int ret)
 {
if (!fw_run_sysfs_fallback(opt_flags))
return ret;
diff --git a/drivers/base/firmware_loader/fallback.h 
b/drivers/base/firmware_loader/fallback.h
index a3b73a09db6c..21063503e4ea 100644
--- a/drivers/base/firmware_loader/fallback.h
+++ b/drivers/base/firmware_loader/fallback.h
@@ -31,10 +31,10 @@ struct firmware_fallback_config {
 };
 
 #ifdef CONFIG_FW_LOADER_USER_HELPER
-int fw_sysfs_fallback(struct firmware *fw, const char *name,
- struct device *device,
- enum fw_opt opt_flags,
- int ret);
+int firmware_fallback_sysfs(struct firmware *fw, const char *name,
+   struct device *device,
+   enum fw_opt opt_flags,
+   int ret);
 void kill_pending_fw_fallback_reqs(bool only_kill_custom);
 
 void fw_fallback_set_cache_timeout(void);
@@ -43,10 +43,10 @@ void fw_fallback_set_default_timeout(void);
 int register_sysfs_loader(void);
 void unregister_sysfs_loader(void);
 #else /* CONFIG_FW_LOADER_USER_HELPER */
-static inline int fw_sysfs_fallback(struct firmware *fw, const char *name,
-   struct device *device,
-   enum fw_opt opt_flags,
-   int ret)
+static inline int firmware_fallback_sysfs(struct firmware *fw, const char 
*name,
+ struct device *device,
+ enum fw_opt opt_flags,
+ int ret)
 {
/* Keep carrying over the same error */
return ret;
diff --git a/drivers/base/firmware_loader/firmware.h 
b/drivers/base/firmware_loader/firmware.h
index 4f433b447367..4c1395f8e7ed 100644
--- a/drivers/base/firmware_loader/firmware.h
+++ b/drivers/base/firmware_loader/firmware.h
@@ -20,7 +20,7 @@
  * @FW_OPT_NOWAIT: Used to describe the firmware request is asynchronous.
  * @FW_OPT_USERHELPER: Enable the fallback mechanism, in case the direct
  * filesystem lookup fails at finding the firmware.  For details refer to
- * fw_sysfs_fallback().
+ * firmware_fallback_sysfs().
  * @FW_OPT_NO_WARN: Quiet, avoid printing warning messages.
  * @FW_OPT_NOCACHE: Disables firmware caching. Firmware caching is used to
  * cache the firmware upon suspend, so that upon resume races against the
diff --git a/drivers/base/firmware_loader/main.c 
b/drivers/base/firmware_loader/main.c
index 4d11efadb3a4..abdc4e4d55f1 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -581,7 +581,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);
-   ret = fw_sysfs_fallback(fw, name, device, opt_flags, ret);
+   ret = firmware_fallback_sysfs(fw, name, device, opt_flags, ret);
} else
ret = assign_fw(fw, device, opt_flags);
 
-- 
2.17.0

[PATCH v6 06/13] firmware_loader: move kconfig FW_LOADER entries to its own file

[Luis R. Rodriguez]

This will make it easier to track and easier to understand
what components and features are part of the FW_LOADER. There
are some components related to firmware which have *nothing* to
do with the FW_LOADER, souch as PREVENT_FIRMWARE_BUILD.

Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 drivers/base/Kconfig | 155 +--
 drivers/base/firmware_loader/Kconfig | 154 ++
 2 files changed, 155 insertions(+), 154 deletions(-)
 create mode 100644 drivers/base/firmware_loader/Kconfig

diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
index a4fe86caecca..06d6e27784fa 100644
--- a/drivers/base/Kconfig
+++ b/drivers/base/Kconfig
@@ -88,160 +88,7 @@ config PREVENT_FIRMWARE_BUILD
o CONFIG_WANXL through CONFIG_WANXL_BUILD_FIRMWARE
o CONFIG_SCSI_AIC79XX through CONFIG_AIC79XX_BUILD_FIRMWARE
 
-menu "Firmware loader"
-
-config FW_LOADER
-   tristate "Firmware loading facility" if EXPERT
-   default y
-   ---help---
- This enables the firmware loading facility in the kernel. The kernel
- will first look for built-in firmware, if it has any. Next, it will
- look for the requested firmware in a series of filesystem paths:
-
-   o firmware_class path module parameter or kernel boot param
-   o /lib/firmware/updates/UTS_RELEASE
-   o /lib/firmware/updates
-   o /lib/firmware/UTS_RELEASE
-   o /lib/firmware
-
- Enabling this feature only increases your kernel image by about
- 828 bytes, enable this option unless you are certain you don't
- need firmware.
-
- You typically want this built-in (=y) but you can also enable this
- as a module, in which case the firmware_class module will be built.
- You also want to be sure to enable this built-in if you are going to
- enable built-in firmware (CONFIG_EXTRA_FIRMWARE).
-
-if FW_LOADER
-
-config EXTRA_FIRMWARE
-   string "Build these firmware blobs into the kernel binary"
-   help
- Device drivers which require firmware can typically deal with
- having the kernel load firmware from the various supported
- /lib/firmware/ paths. This option enables you to build into the
- kernel firmware files. Built-in firmware searches are preceeded
- over firmware lookups using your filesystem over the supported
- /lib/firmware paths documented on CONFIG_FW_LOADER.
-
- This may be useful for testing or if the firmware is required early on
- in boot and cannot rely on the firmware being placed in an initrd or
- initramfs.
-
- This option is a string and takes the (space-separated) names of the
- firmware files -- the same names that appear in MODULE_FIRMWARE()
- and request_firmware() in the source. These files should exist under
- the directory specified by the EXTRA_FIRMWARE_DIR option, which is
- /lib/firmware by default.
-
- For example, you might set CONFIG_EXTRA_FIRMWARE="usb8388.bin", copy
- the usb8388.bin file into /lib/firmware, and build the kernel. Then
- any request_firmware("usb8388.bin") will be satisfied internally
- inside the kernel without ever looking at your filesystem at runtime.
-
- WARNING: If you include additional firmware files into your binary
- kernel image that are not available under the terms of the GPL,
- then it may be a violation of the GPL to distribute the resulting
- image since it combines both GPL and non-GPL work. You should
- consult a lawyer of your own before distributing such an image.
-
-config EXTRA_FIRMWARE_DIR
-   string "Firmware blobs root directory"
-   depends on EXTRA_FIRMWARE != ""
-   default "/lib/firmware"
-   help
- This option controls the directory in which the kernel build system
- looks for the firmware files listed in the EXTRA_FIRMWARE option.
-
-config FW_LOADER_USER_HELPER
-   bool "Enable the firmware sysfs fallback mechanism"
-   help
- This option enables a sysfs loading facility to enable firmware
- loading to the kernel through userspace as a fallback mechanism
- if and only if the kernel's direct filesystem lookup for the
- firmware failed using the different /lib/firmware/ paths, or the
- path specified in the firmware_class path module parameter, or the
- firmware_class path kernel boot parameter if the firmware_class is
- built-in. For details on how to work with the sysfs fallback mechanism
- refer to Documentation/driver-api/firmware/fallback-mechanisms.rst.
-
- The direct filesystem lookup for firwmare is always used first now.
-
- If the kernel's direct filesystem loo

[PATCH v6 09/13] ath10k: use firmware_request_nowarn() to load firmware

[Luis R. Rodriguez]

From: Andres Rodriguez <andre...@gmail.com>

This reduces the unnecessary spew when trying to load optional firmware:
"Direct firmware load for ... failed with error -2"

Signed-off-by: Andres Rodriguez <andre...@gmail.com>
Acked-by: Kalle Valo <kv...@codeaurora.org>
Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 drivers/net/wireless/ath/ath10k/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/ath10k/core.c 
b/drivers/net/wireless/ath/ath10k/core.c
index 4cf54a7ef09a..ad4f6e3c0737 100644
--- a/drivers/net/wireless/ath/ath10k/core.c
+++ b/drivers/net/wireless/ath/ath10k/core.c
@@ -694,7 +694,7 @@ static const struct firmware *ath10k_fetch_fw_file(struct 
ath10k *ar,
dir = ".";
 
snprintf(filename, sizeof(filename), "%s/%s", dir, file);
-   ret = request_firmware(, filename, ar->dev);
+   ret = firmware_request_nowarn(, filename, ar->dev);
ath10k_dbg(ar, ATH10K_DBG_BOOT, "boot fw request '%s': %d\n",
   filename, ret);
 
-- 
2.17.0

[PATCH v6 07/13] firmware_loader: make firmware_fallback_sysfs() print more useful

[Luis R. Rodriguez]

If we resort to using the sysfs fallback mechanism we don't print
the filename. This can be deceiving given we could have a series of
callers intertwined and it'd be unclear exactly for what firmware
this was meant for.

Additionally, although we don't currently use FW_OPT_NO_WARN when
dealing with the fallback mechanism, we will soon, so just respect
its use consistently.

And even if you *don't* want to print always on failure, you may
want to print when debugging so enable dynamic debug print when
FW_OPT_NO_WARN is used.

Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 drivers/base/firmware_loader/fallback.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/base/firmware_loader/fallback.c 
b/drivers/base/firmware_loader/fallback.c
index 9169e7b9800c..b676a99c469c 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -690,6 +690,11 @@ int firmware_fallback_sysfs(struct firmware *fw, const 
char *name,
if (!fw_run_sysfs_fallback(opt_flags))
return ret;
 
-   dev_warn(device, "Falling back to user helper\n");
+   if (!(opt_flags & FW_OPT_NO_WARN))
+   dev_warn(device, "Falling back to syfs fallback for: %s\n",
+name);
+   else
+   dev_dbg(device, "Falling back to sysfs fallback for: %s\n",
+   name);
return fw_load_from_user_helper(fw, name, device, opt_flags);
 }
-- 
2.17.0

[PATCH v6 10/13] ath10k: re-enable the firmware fallback mechanism for testmode

[Luis R. Rodriguez]

From: Andres Rodriguez <andre...@gmail.com>

The ath10k testmode uses request_firmware_direct() in order to avoid
producing firmware load warnings. Disabling the fallback mechanism was a
side effect of disabling warnings.

We now have a new API that allows us to avoid warnings while keeping the
fallback mechanism enabled. So use that instead.

Signed-off-by: Andres Rodriguez <andre...@gmail.com>
Acked-by: Kalle Valo <kv...@codeaurora.org>
Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 drivers/net/wireless/ath/ath10k/testmode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/ath10k/testmode.c 
b/drivers/net/wireless/ath/ath10k/testmode.c
index 568810b41657..c24ee616833c 100644
--- a/drivers/net/wireless/ath/ath10k/testmode.c
+++ b/drivers/net/wireless/ath/ath10k/testmode.c
@@ -157,7 +157,7 @@ static int ath10k_tm_fetch_utf_firmware_api_1(struct ath10k 
*ar,
 ar->hw_params.fw.dir, ATH10K_FW_UTF_FILE);
 
/* load utf firmware image */
-   ret = request_firmware_direct(_file->firmware, filename, ar->dev);
+   ret = firmware_request_nowarn(_file->firmware, filename, ar->dev);
ath10k_dbg(ar, ATH10K_DBG_TESTMODE, "testmode fw request '%s': %d\n",
   filename, ret);
 
-- 
2.17.0

[PATCH v6 08/13] firmware: add firmware_request_nowarn() - load firmware without warnings

[Luis R. Rodriguez]

From: Andres Rodriguez <andre...@gmail.com>

Currently the firmware loader only exposes one silent path for querying
optional firmware, and that is firmware_request_direct(). This function
also disables the sysfs fallback mechanism, which might not always be the
desired behaviour [0].

This patch introduces a variations of request_firmware() that enable the
caller to disable the undesired warning messages but enables the sysfs
fallback mechanism. This is equivalent to adding FW_OPT_NO_WARN to the
old behaviour.

[0]: https://git.kernel.org/linus/c0cc00f250e1

Signed-off-by: Andres Rodriguez <andre...@gmail.com>
Acked-by: Luis R. Rodriguez <mcg...@kernel.org>
[mcgrof: used the old API calls as the full rename is not done yet, and
 add the caller for when FW_LOADER is disabled, enhance documentation ]
Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 .../driver-api/firmware/request_firmware.rst  |  5 
 drivers/base/firmware_loader/main.c   | 27 +++
 include/linux/firmware.h  | 10 +++
 3 files changed, 42 insertions(+)

diff --git a/Documentation/driver-api/firmware/request_firmware.rst 
b/Documentation/driver-api/firmware/request_firmware.rst
index d5ec95a7195b..f62bdcbfed5b 100644
--- a/Documentation/driver-api/firmware/request_firmware.rst
+++ b/Documentation/driver-api/firmware/request_firmware.rst
@@ -20,6 +20,11 @@ request_firmware
 .. kernel-doc:: drivers/base/firmware_loader/main.c
:functions: request_firmware
 
+firmware_request_nowarn
+---
+.. kernel-doc:: drivers/base/firmware_loader/main.c
+   :functions: firmware_request_nowarn
+
 request_firmware_direct
 ---
 .. kernel-doc:: drivers/base/firmware_loader/main.c
diff --git a/drivers/base/firmware_loader/main.c 
b/drivers/base/firmware_loader/main.c
index abdc4e4d55f1..2be79ee773c0 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -631,6 +631,33 @@ request_firmware(const struct firmware **firmware_p, const 
char *name,
 }
 EXPORT_SYMBOL(request_firmware);
 
+/**
+ * firmware_request_nowarn() - request for an optional fw module
+ * @firmware: pointer to firmware image
+ * @name: name of firmware file
+ * @device: device for which firmware is being loaded
+ *
+ * This function is similar in behaviour to request_firmware(), except
+ * it doesn't produce warning messages when the file is not found.
+ * The sysfs fallback mechanism is enabled if direct filesystem lookup fails,
+ * however, however failures to find the firmware file with it are still
+ * supressed. It is therefore up to the driver to check for the return value
+ * of this call and to decide when to inform the users of errors.
+ **/
+int firmware_request_nowarn(const struct firmware **firmware, const char *name,
+   struct device *device)
+{
+   int ret;
+
+   /* Need to pin this module until return */
+   __module_get(THIS_MODULE);
+   ret = _request_firmware(firmware, name, device, NULL, 0,
+   FW_OPT_UEVENT | FW_OPT_NO_WARN);
+   module_put(THIS_MODULE);
+   return ret;
+}
+EXPORT_SYMBOL_GPL(firmware_request_nowarn);
+
 /**
  * request_firmware_direct() - load firmware directly without usermode helper
  * @firmware_p: pointer to firmware image
diff --git a/include/linux/firmware.h b/include/linux/firmware.h
index 41050417cafb..2dd566c91d44 100644
--- a/include/linux/firmware.h
+++ b/include/linux/firmware.h
@@ -42,6 +42,8 @@ struct builtin_fw {
 #if defined(CONFIG_FW_LOADER) || (defined(CONFIG_FW_LOADER_MODULE) && 
defined(MODULE))
 int request_firmware(const struct firmware **fw, const char *name,
 struct device *device);
+int firmware_request_nowarn(const struct firmware **fw, const char *name,
+   struct device *device);
 int request_firmware_nowait(
struct module *module, bool uevent,
const char *name, struct device *device, gfp_t gfp, void *context,
@@ -59,6 +61,14 @@ static inline int request_firmware(const struct firmware 
**fw,
 {
return -EINVAL;
 }
+
+static inline int firmware_request_nowarn(const struct firmware **fw,
+ const char *name,
+ struct device *device)
+{
+   return -EINVAL;
+}
+
 static inline int request_firmware_nowait(
struct module *module, bool uevent,
const char *name, struct device *device, gfp_t gfp, void *context,
-- 
2.17.0

[PATCH v6 11/13] Documentation: fix few typos and clarifications for the firmware loader

[Luis R. Rodriguez]

Fix a few typos, and clarify a few sentences.

Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 Documentation/driver-api/firmware/fallback-mechanisms.rst | 5 +++--
 Documentation/driver-api/firmware/firmware_cache.rst  | 4 ++--
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/Documentation/driver-api/firmware/fallback-mechanisms.rst 
b/Documentation/driver-api/firmware/fallback-mechanisms.rst
index f353783ae0be..a39323ef7d29 100644
--- a/Documentation/driver-api/firmware/fallback-mechanisms.rst
+++ b/Documentation/driver-api/firmware/fallback-mechanisms.rst
@@ -83,7 +83,7 @@ and a file upload firmware into:
   * /sys/$DEVPATH/data
 
 To upload firmware you will echo 1 onto the loading file to indicate
-you are loading firmware. You then cat the firmware into the data file,
+you are loading firmware. You then write the firmware into the data file,
 and you notify the kernel the firmware is ready by echo'ing 0 onto
 the loading file.
 
@@ -136,7 +136,8 @@ by kobject uevents. This is specially exacerbated due to 
the fact that most
 distributions today disable CONFIG_FW_LOADER_USER_HELPER_FALLBACK.
 
 Refer to do_firmware_uevent() for details of the kobject event variables
-setup. Variables passwdd with a kobject add event:
+setup. The variables currently passed to userspace with a "kobject add"
+event are:
 
 * FIRMWARE=firmware name
 * TIMEOUT=timeout value
diff --git a/Documentation/driver-api/firmware/firmware_cache.rst 
b/Documentation/driver-api/firmware/firmware_cache.rst
index 2210e5bfb332..c2e69d9c6bf1 100644
--- a/Documentation/driver-api/firmware/firmware_cache.rst
+++ b/Documentation/driver-api/firmware/firmware_cache.rst
@@ -29,8 +29,8 @@ Some implementation details about the firmware cache setup:
 * If an asynchronous call is used the firmware cache is only set up for a
   device if if the second argument (uevent) to request_firmware_nowait() is
   true. When uevent is true it requests that a kobject uevent be sent to
-  userspace for the firmware request. For details refer to the Fackback
-  mechanism documented below.
+  userspace for the firmware request through the sysfs fallback mechanism
+  if the firmware file is not found.
 
 * If the firmware cache is determined to be needed as per the above two
   criteria the firmware cache is setup by adding a devres entry for the
-- 
2.17.0

[PATCH v6 12/13] Documentation: remove stale firmware API reference

[Luis R. Rodriguez]

It refers to a pending patch, but this was merged eons ago.

Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 Documentation/dell_rbu.txt | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/Documentation/dell_rbu.txt b/Documentation/dell_rbu.txt
index 0fdb6aa2704c..077fdc29a0d0 100644
--- a/Documentation/dell_rbu.txt
+++ b/Documentation/dell_rbu.txt
@@ -121,9 +121,6 @@ read back the image downloaded.
 
 .. note::
 
-   This driver requires a patch for firmware_class.c which has the modified
-   request_firmware_nowait function.
-
Also after updating the BIOS image a user mode application needs to execute
code which sends the BIOS update request to the BIOS. So on the next reboot
the BIOS knows about the new image downloaded and it updates itself.
-- 
2.17.0

[PATCH v6 13/13] Documentation: clarify firmware_class provenance and why we can't rename the module

[Luis R. Rodriguez]

Clarify the provenance of the firmware loader firmware_class module name
and why we cannot rename the module in the future.

Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 .../driver-api/firmware/fallback-mechanisms.rst  | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/Documentation/driver-api/firmware/fallback-mechanisms.rst 
b/Documentation/driver-api/firmware/fallback-mechanisms.rst
index a39323ef7d29..a8047be4a96e 100644
--- a/Documentation/driver-api/firmware/fallback-mechanisms.rst
+++ b/Documentation/driver-api/firmware/fallback-mechanisms.rst
@@ -72,9 +72,12 @@ the firmware requested, and establishes it in the device 
hierarchy by
 associating the device used to make the request as the device's parent.
 The sysfs directory's file attributes are defined and controlled through
 the new device's class (firmware_class) and group (fw_dev_attr_groups).
-This is actually where the original firmware_class.c file name comes from,
-as originally the only firmware loading mechanism available was the
-mechanism we now use as a fallback mechanism.
+This is actually where the original firmware_class module name came from,
+given that originally the only firmware loading mechanism available was the
+mechanism we now use as a fallback mechanism, which which registers a
+struct class firmware_class. Because the attributes exposed are part of the
+module name, the module name firmware_class cannot be renamed in the future, to
+ensure backward compatibilty with old userspace.
 
 To load firmware using the sysfs interface we expose a loading indicator,
 and a file upload firmware into:
-- 
2.17.0

[PATCH v6 00/13] firmware_loader changes for v4.18

[Luis R. Rodriguez]

Greg,

Here is what I have queued up for the firmware_loader for v4.18. It
includes a slew of cleanup work, and the new firmware_request_nowarn()
which is quiet but enables the sysfs fallback mechanism. I've gone ahead
and also queued up a few minor fixes for the firmware loader documentation
which have come up recently. These changes are available on my git tree
both based on linux-next [0] and Linus' latest tree [1]. Folks working
on new developments for the firmware loader can use my linux-next
branch 20180508-firmware_loader_for-v4.18-try2 for now.

0-day sends its blessings.

The patches from Mimi's series still require a bit more discussion and
review. The discussion over the EFI firmware fallback mechanism is still
ongoing.

As for the rename that you wanted, perhaps we can do this late in the
merge window considering we're at rc4 now. I can prep something up for
that later.

Question, and specially rants are warmly welcomed.

[0] 
https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux-next.git/log/?h=20180508-firmware_loader_for-v4.18-try2
[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux.git/log/?h=20180508-firmware_loader_for-v4.18-try2

Andres Rodriguez (6):
  firmware: wrap FW_OPT_* into an enum
  firmware: use () to terminate kernel-doc function names
  firmware: rename fw_sysfs_fallback to firmware_fallback_sysfs()
  firmware: add firmware_request_nowarn() - load firmware without
warnings
  ath10k: use firmware_request_nowarn() to load firmware
  ath10k: re-enable the firmware fallback mechanism for testmode

Luis R. Rodriguez (7):
  firmware_loader: document firmware_sysfs_fallback()
  firmware_loader: enhance Kconfig documentation over FW_LOADER
  firmware_loader: move kconfig FW_LOADER entries to its own file
  firmware_loader: make firmware_fallback_sysfs() print more useful
  Documentation: fix few typos and clarifications for the firmware
loader
  Documentation: remove stale firmware API reference
  Documentation: clarify firmware_class provenance and why we can't
rename the module

 Documentation/dell_rbu.txt|   3 -
 .../firmware/fallback-mechanisms.rst  |  14 +-
 .../driver-api/firmware/firmware_cache.rst|   4 +-
 .../driver-api/firmware/request_firmware.rst  |   5 +
 drivers/base/Kconfig  |  90 ++
 drivers/base/firmware_loader/Kconfig  | 154 ++
 drivers/base/firmware_loader/fallback.c   |  53 --
 drivers/base/firmware_loader/fallback.h   |  18 +-
 drivers/base/firmware_loader/firmware.h   |  37 -
 drivers/base/firmware_loader/main.c   |  57 +--
 drivers/net/wireless/ath/ath10k/core.c|   2 +-
 drivers/net/wireless/ath/ath10k/testmode.c|   2 +-
 include/linux/firmware.h  |  10 ++
 13 files changed, 319 insertions(+), 130 deletions(-)
 create mode 100644 drivers/base/firmware_loader/Kconfig

-- 
2.17.0

Re: [PATCH 00/18] Fix some build warnings/errors with Sphinx

[Luis R. Rodriguez]

On Tue, May 08, 2018 at 10:13:42AM -0600, Jonathan Corbet wrote:
> On Mon,  7 May 2018 06:35:36 -0300
> Mauro Carvalho Chehab  wrote:
> 
> > I decided to give a try with Sphinx last stable version
> > (1.17.4), and noticed several issues. The worse one was
> > with the networking book: a non-standard footnote there
> > with [*] instead of a number causes it to break PDF building.
> > 
> > So, I took some spare time to address some warnings all over
> > the tree, and moved a few text documents to a book. 
> 
> OK, I've applied the ones that seem to make sense for me to take now.
> There's comments on the firmware one, 

I'll fold in the fixes for the firmware API which do apply to my queue.

  Luis

Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware

[Luis R. Rodriguez]

On Thu, May 03, 2018 at 08:24:26PM -0400, Mimi Zohar wrote:
> On Fri, 2018-05-04 at 00:07 +0000, Luis R. Rodriguez wrote:
> > On Tue, May 01, 2018 at 09:48:20AM -0400, Mimi Zohar wrote:
> > > Allow LSMs and IMA to differentiate between signed regulatory.db and
> > > other firmware.
> > > 
> > > Signed-off-by: Mimi Zohar <zo...@linux.vnet.ibm.com>
> > > Cc: Luis R. Rodriguez <mcg...@suse.com>
> > > Cc: David Howells <dhowe...@redhat.com>
> > > Cc: Kees Cook <keesc...@chromium.org>
> > > Cc: Seth Forshee <seth.fors...@canonical.com>
> > > Cc: Johannes Berg <johannes.b...@intel.com>
> > > ---
> > >  drivers/base/firmware_loader/main.c | 5 +
> > >  include/linux/fs.h  | 1 +
> > >  2 files changed, 6 insertions(+)
> > > 
> > > diff --git a/drivers/base/firmware_loader/main.c 
> > > b/drivers/base/firmware_loader/main.c
> > > index eb34089e4299..d7cdf04a8681 100644
> > > --- a/drivers/base/firmware_loader/main.c
> > > +++ b/drivers/base/firmware_loader/main.c
> > > @@ -318,6 +318,11 @@ fw_get_filesystem_firmware(struct device *device, 
> > > struct fw_priv *fw_priv)
> > >   break;
> > >   }
> > >  
> > > +#ifdef CONFIG_CFG80211_REQUIRE_SIGNED_REGDB
> > > + if ((strcmp(fw_priv->fw_name, "regulatory.db") == 0) ||
> > > + (strcmp(fw_priv->fw_name, "regulatory.db.p7s") == 0))
> > > + id = READING_FIRMWARE_REGULATORY_DB;
> > > +#endif
> > 
> > Whoa, no way.
> 
> There are two methods for the kernel to verify firmware signatures.

Yes, but although CONFIG_CFG80211_REQUIRE_SIGNED_REGDB is its own kernel
mechanism to verify firmware it uses the request_firmware*() API for
regulatory.db and regulatory.db.p7s, and IMA already can appraise these two
files since the firmware API is used.

As such I see no reason to add a new ID for them at all.

Its not providing an *alternative*, its providing an *extra* kernel measure.
If anything CONFIG_CFG80211_REQUIRE_SIGNED_REGDB perhaps should be its own
stacked LSM. I'd be open to see patches which set that out. May be a
cleaner interface.

> If both are enabled, do we require both signatures or is one enough.

Good question. Considering it as a stacked LSM (although not implemented
as one), I'd say its up to who enabled the Kconfig entries. If IMA and
CONFIG_CFG80211_REQUIRE_SIGNED_REGDB are enabled then both. If someone enabled
IMA though, then surely I agree that enabling
CONFIG_CFG80211_REQUIRE_SIGNED_REGDB is stupid and redundant, but its up to the
system integrator to decide.

If we however want to make it clear that such things as
CONFIG_CFG80211_REQUIRE_SIGNED_REGDB are not required when IMA is enabled we
could just make the kconfig depend on !IMA or something? Or perhaps a new
kconfig for IMA which if selected it means that drivers can opt to open code
*further* kernel signature verification, even though IMA already is sufficient.
Perhaps CONFIG_ENABLE_IMA_OVERLAPPING, and the driver depends on it?

> Assigning a different id for regdb signed firmware allows LSMs and IMA
> to handle regdb files differently.

That's not the main concern here, its the precedent we are setting here for
any new kernel interface which open codes firmware signing on its own. What
you are doing means other kernel users who open codes their own firmware
signing may need to add yet-another reading ID. That doesn't either look
well on code, and seems kind of silly from a coding perspective given
the above, in which I clarify IMA still is doing its own appraisal on it.

> > >   fw_priv->size = 0;
> > >   rc = kernel_read_file_from_path(path, _priv->data, ,
> > >   msize, id);
> > > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > > index dc16a73c3d38..d1153c2884b9 100644
> > > --- a/include/linux/fs.h
> > > +++ b/include/linux/fs.h
> > > @@ -2811,6 +2811,7 @@ extern int do_pipe_flags(int *, int);
> > >   id(FIRMWARE, firmware)  \
> > >   id(FIRMWARE_PREALLOC_BUFFER, firmware)  \
> > >   id(FIRMWARE_FALLBACK, firmware) \
> > > + id(FIRMWARE_REGULATORY_DB, firmware)\
> > 
> > Why could IMA not appriase these files? They are part of the standard path.
> 
> The subsequent patch attempts to verify the IMA-appraisal signature, but on
> failure it falls back to allowing regdb signatures. 
> For systems that only want to load firmware based on IMA-appraisal, then
>regdb wouldn't be enabled.

I think we can codify this a bit better, without a new ID.

  Luis

Re: [PATCH v5 0/6] firmware_loader: cleanups for v4.18

[Luis R. Rodriguez]

On Fri, May 04, 2018 at 10:43:49AM -0700, Luis R. Rodriguez wrote:
> Greg,
> 
> I've reviewed the pending patches for the firmware_laoder and as for
> v4.18, the following 3 patches from Andres have been iterated enough
> that they're ready after I made some final minor changes, mostly just
> style fixes and re-arrangements in terms of order. The new API he was
> suggesting to add requires just a bit more review.

We're done with review of the new API for the sync no warn call,
so I'll just re-issue another patch series with those patches
in a new series.

Coming right up.

  Luis

Re: [PATCH 6/9] firmware: print firmware name on fallback path

[Luis R. Rodriguez]

On Fri, May 04, 2018 at 10:57:26PM -0400, Andres Rodriguez wrote:
> On 2018-05-03 07:42 PM, Luis R. Rodriguez wrote:
> > On Mon, Apr 23, 2018 at 04:12:02PM -0400, Andres Rodriguez wrote:
> > > Previously, one could assume the firmware name from the preceding
> > > message: "Direct firmware load for {name} failed with error %d".
> > > 
> > > However, with the new firmware_request_nowarn() entrypoint, the message
> > > outlined above will not always be printed.
> > 
> > I though the whole point was to not print an error message. What if
> > we want later to disable this error message? This would prove a bit
> > pointless.
> > 
> > Let's discuss the exact semantics desired here. Why would only the
> > fallback be desirable here?
> > 
> > Andres, Kalle?
> > 
> > After we address this I'll address resubmitting this lat patch
> > along with the last one. For now I'll skip it.
> 
> You are correct. I initially thought it would be useful to know that the
> usermode fallback was being triggered. And for that message to be useful we
> would need a fw name.
> 
> But now that you point it out, this behaviour is inconsistent with the
> _nowarn() definition. We shouldn't have a message in the first place.
> 
> So it might be better to instead have:
> 
> if (!(opt_flags & FW_OPT_NO_WARN) )
> dev_warn(device, "Falling back to user helper\n");
> 
> No need to add the firmware name, cause we either:
> a) FW_OPT_NO_WARN is set and no messages are printed, or
> b) FW_OPT_NO_WARN is not set and we get both messages.
> 
> Yay, nay?

I welcome such a new warning but not for any of the reasons stated.

It make sense if FW_OPT_NO_WARN is not set and only because the fallback
mechanism can fail for a slew of different firmware files, and just getting
informed a failure with a fallback occurred does not tell us for which file it
failed for.

I'll add such a patch to my queue and send it off soon prior to your own
new API nowarn call.

  Luis

Re: [PATCH v5 0/6] firmware_loader: cleanups for v4.18

[Luis R. Rodriguez]

On Fri, May 04, 2018 at 09:17:08PM +0200, Krzysztof Halasa wrote:
> "Luis R. Rodriguez" <mcg...@kernel.org> writes:
> 
> >   * CONFIG_WANXL --> CONFIG_WANXL_BUILD_FIRMWARE
> >   * CONFIG_SCSI_AIC79XX --> CONFIG_AIC79XX_BUILD_FIRMWARE
> >
> > To this day both of these drivers are building driver *firmwares* when
> > the option CONFIG_PREVENT_FIRMWARE_BUILD is disabled, and they don't
> > even make use of the firmware API at all.
> 
> Don't know for sure about Adaptec, but wanXL firmware fits every
> definition of "stable". BTW it's a 1997 or so early PCI card, built
> around the PLX PCI9060 bus mastering PCI bridge and Motorola 68360
> (the original QUICC) processor. Maximum bit rate of 2 Mb/s on each sync
> serial port.

So we can nuke CONFIG_WANXL_BUILD_FIRMWARE now?

> It's more about delivering the .S source for the firmware, I guess.
> Nobody is expected to build it. The fw is about 2.5 KB and is directly
> linked with the driver.

:P Future work I guess would be to just use the firmware API and stuff
it into linux-firmware?

  Luis

[PATCH v5 1/6] firmware: wrap FW_OPT_* into an enum

[Luis R. Rodriguez]

From: Andres Rodriguez <andre...@gmail.com>

This should let us associate enum kdoc to these values.
While at it, kdocify the fw_opt.

Signed-off-by: Andres Rodriguez <andre...@gmail.com>
Acked-by: Luis R. Rodriguez <mcg...@kernel.org>
[mcgrof: coding style fixes, merge kdoc with enum move]
Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 drivers/base/firmware_loader/fallback.c | 12 
 drivers/base/firmware_loader/fallback.h |  6 ++--
 drivers/base/firmware_loader/firmware.h | 37 +++--
 drivers/base/firmware_loader/main.c |  6 ++--
 4 files changed, 42 insertions(+), 19 deletions(-)

diff --git a/drivers/base/firmware_loader/fallback.c 
b/drivers/base/firmware_loader/fallback.c
index 358354148dec..b57a7b3b4122 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -512,7 +512,7 @@ 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, enum fw_opt opt_flags)
 {
struct fw_sysfs *fw_sysfs;
struct device *f_dev;
@@ -545,7 +545,7 @@ fw_create_instance(struct firmware *firmware, const char 
*fw_name,
  * In charge of constructing a sysfs fallback interface for firmware loading.
  **/
 static int fw_load_sysfs_fallback(struct fw_sysfs *fw_sysfs,
- unsigned int opt_flags, long timeout)
+ enum fw_opt opt_flags, long timeout)
 {
int retval = 0;
struct device *f_dev = _sysfs->dev;
@@ -599,7 +599,7 @@ static int fw_load_sysfs_fallback(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)
+   enum fw_opt opt_flags)
 {
struct fw_sysfs *fw_sysfs;
long timeout;
@@ -640,7 +640,7 @@ static int fw_load_from_user_helper(struct firmware 
*firmware,
return ret;
 }
 
-static bool fw_force_sysfs_fallback(unsigned int opt_flags)
+static bool fw_force_sysfs_fallback(enum fw_opt opt_flags)
 {
if (fw_fallback_config.force_sysfs_fallback)
return true;
@@ -649,7 +649,7 @@ static bool fw_force_sysfs_fallback(unsigned int opt_flags)
return true;
 }
 
-static bool fw_run_sysfs_fallback(unsigned int opt_flags)
+static bool fw_run_sysfs_fallback(enum fw_opt opt_flags)
 {
if (fw_fallback_config.ignore_sysfs_fallback) {
pr_info_once("Ignoring firmware sysfs fallback due to sysctl 
knob\n");
@@ -664,7 +664,7 @@ static bool fw_run_sysfs_fallback(unsigned int opt_flags)
 
 int fw_sysfs_fallback(struct firmware *fw, const char *name,
  struct device *device,
- unsigned int opt_flags,
+ enum fw_opt opt_flags,
  int ret)
 {
if (!fw_run_sysfs_fallback(opt_flags))
diff --git a/drivers/base/firmware_loader/fallback.h 
b/drivers/base/firmware_loader/fallback.h
index f8255670a663..a3b73a09db6c 100644
--- a/drivers/base/firmware_loader/fallback.h
+++ b/drivers/base/firmware_loader/fallback.h
@@ -5,6 +5,8 @@
 #include 
 #include 
 
+#include "firmware.h"
+
 /**
  * struct firmware_fallback_config - firmware fallback configuration settings
  *
@@ -31,7 +33,7 @@ struct firmware_fallback_config {
 #ifdef CONFIG_FW_LOADER_USER_HELPER
 int fw_sysfs_fallback(struct firmware *fw, const char *name,
  struct device *device,
- unsigned int opt_flags,
+ enum fw_opt opt_flags,
  int ret);
 void kill_pending_fw_fallback_reqs(bool only_kill_custom);
 
@@ -43,7 +45,7 @@ void unregister_sysfs_loader(void);
 #else /* CONFIG_FW_LOADER_USER_HELPER */
 static inline int fw_sysfs_fallback(struct firmware *fw, const char *name,
struct device *device,
-   unsigned int opt_flags,
+   enum fw_opt opt_flags,
int ret)
 {
/* Keep carrying over the same error */
diff --git a/drivers/base/firmware_loader/firmware.h 
b/drivers/base/firmware_loader/firmware.h
index 64acbb1a392c..4f433b447367 100644
--- a/drivers/base/firmware_loader/firmware.h
+++ b/drivers/base/firmware_loader/firmware.h
@@ -2,6 +2,7 @@
 #ifndef __FIRMWARE_LOADER_H
 #define __FIRMWARE_LOADER_H
 
+#include 
 #include 
 #include 
 #include 
@@ -10,13 +11,33 @@
 
 #include 
 
-/* firmware behavior options */
-#define FW_OPT_UEVENT  (1U << 0)
-#define FW_OPT_NOWAIT  (1U << 1)
-#define FW_OPT_USERHELPER  (1U << 2)
-#define

[PATCH v5 3/6] firmware: rename fw_sysfs_fallback to firmware_fallback_sysfs()

[Luis R. Rodriguez]

From: Andres Rodriguez <andre...@gmail.com>

This is done since this call is now exposed through kernel-doc,
and since this also paves the way for different future types of
fallback mechanims.

Signed-off-by: Andres Rodriguez <andre...@gmail.com>
Acked-by: Luis R. Rodriguez <mcg...@kernel.org>
[mcgrof: small coding style changes]
Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 drivers/base/firmware_loader/fallback.c |  8 
 drivers/base/firmware_loader/fallback.h | 16 
 drivers/base/firmware_loader/firmware.h |  2 +-
 drivers/base/firmware_loader/main.c |  2 +-
 4 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/base/firmware_loader/fallback.c 
b/drivers/base/firmware_loader/fallback.c
index 529f7013616f..3db9e0f225ac 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -662,10 +662,10 @@ static bool fw_run_sysfs_fallback(enum fw_opt opt_flags)
return fw_force_sysfs_fallback(opt_flags);
 }
 
-int fw_sysfs_fallback(struct firmware *fw, const char *name,
- struct device *device,
- enum fw_opt opt_flags,
- int ret)
+int firmware_fallback_sysfs(struct firmware *fw, const char *name,
+   struct device *device,
+   enum fw_opt opt_flags,
+   int ret)
 {
if (!fw_run_sysfs_fallback(opt_flags))
return ret;
diff --git a/drivers/base/firmware_loader/fallback.h 
b/drivers/base/firmware_loader/fallback.h
index a3b73a09db6c..21063503e4ea 100644
--- a/drivers/base/firmware_loader/fallback.h
+++ b/drivers/base/firmware_loader/fallback.h
@@ -31,10 +31,10 @@ struct firmware_fallback_config {
 };
 
 #ifdef CONFIG_FW_LOADER_USER_HELPER
-int fw_sysfs_fallback(struct firmware *fw, const char *name,
- struct device *device,
- enum fw_opt opt_flags,
- int ret);
+int firmware_fallback_sysfs(struct firmware *fw, const char *name,
+   struct device *device,
+   enum fw_opt opt_flags,
+   int ret);
 void kill_pending_fw_fallback_reqs(bool only_kill_custom);
 
 void fw_fallback_set_cache_timeout(void);
@@ -43,10 +43,10 @@ void fw_fallback_set_default_timeout(void);
 int register_sysfs_loader(void);
 void unregister_sysfs_loader(void);
 #else /* CONFIG_FW_LOADER_USER_HELPER */
-static inline int fw_sysfs_fallback(struct firmware *fw, const char *name,
-   struct device *device,
-   enum fw_opt opt_flags,
-   int ret)
+static inline int firmware_fallback_sysfs(struct firmware *fw, const char 
*name,
+ struct device *device,
+ enum fw_opt opt_flags,
+ int ret)
 {
/* Keep carrying over the same error */
return ret;
diff --git a/drivers/base/firmware_loader/firmware.h 
b/drivers/base/firmware_loader/firmware.h
index 4f433b447367..4c1395f8e7ed 100644
--- a/drivers/base/firmware_loader/firmware.h
+++ b/drivers/base/firmware_loader/firmware.h
@@ -20,7 +20,7 @@
  * @FW_OPT_NOWAIT: Used to describe the firmware request is asynchronous.
  * @FW_OPT_USERHELPER: Enable the fallback mechanism, in case the direct
  * filesystem lookup fails at finding the firmware.  For details refer to
- * fw_sysfs_fallback().
+ * firmware_fallback_sysfs().
  * @FW_OPT_NO_WARN: Quiet, avoid printing warning messages.
  * @FW_OPT_NOCACHE: Disables firmware caching. Firmware caching is used to
  * cache the firmware upon suspend, so that upon resume races against the
diff --git a/drivers/base/firmware_loader/main.c 
b/drivers/base/firmware_loader/main.c
index 7f2bc7e8e3c0..d951af29017a 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -581,7 +581,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);
-   ret = fw_sysfs_fallback(fw, name, device, opt_flags, ret);
+   ret = firmware_fallback_sysfs(fw, name, device, opt_flags, ret);
} else
ret = assign_fw(fw, device, opt_flags);
 
-- 
2.17.0

[PATCH v5 0/6] firmware_loader: cleanups for v4.18

[Luis R. Rodriguez]

Greg,

I've reviewed the pending patches for the firmware_laoder and as for
v4.18, the following 3 patches from Andres have been iterated enough
that they're ready after I made some final minor changes, mostly just
style fixes and re-arrangements in terms of order. The new API he was
suggesting to add requires just a bit more review.

The last 3 patches are my own and are new, so I'd like further review
from others on them, but considering the changes Hans de Goede is having
us consider, I think this will prove useful to his work in terms of
splitting up code and documenting things properly. One thing that was
clear -- is our Kconfig entries for FW_LOADER were *extremely* outdated,
as such I've gone ahead and updated them.

The kconfig wording update for FW_LOADER includes prior conclusions made
to help justify keeping the split of the firmware fallback sysfs
interface in terms of size which was discussed with Josh Triplett a
while ago. It also includes modern recommendations, which would otherwise
get lost, on what to do about corner case firmware situations on
provisioning situations which folks have brought to my attention before
and architectural solutions based on firmwared [0] for a few years now.
Finally this work also reveals that a couple of candidate drivers could
likely move to staging considering their age, *or* we could just remove
the respective firmware build options. SCSI folks? Networking folks? To
my surprise *nothing* has been done about PREVENT_FIRMWARE_BUILD for
them since pre-git days!  These sneaky litte buggers are:

  * CONFIG_WANXL --> CONFIG_WANXL_BUILD_FIRMWARE
  * CONFIG_SCSI_AIC79XX --> CONFIG_AIC79XX_BUILD_FIRMWARE

To this day both of these drivers are building driver *firmwares* when
the option CONFIG_PREVENT_FIRMWARE_BUILD is disabled, and they don't
even make use of the firmware API at all. I find it *highly unlikely*
pre-git day drivers are raging in new radical firmware updates these
days. I'll go put a knife into some of that unless I hear back from
SCSI or networking folks that WANXL_BUILD_FIRMWARE and
AIC79XX_BUILD_FIRMWARE are still hip and very much needed.

On my radar as well are Mimi's latest firmware_loader proposed changes,
but I think those need considerable review and attention from more security
folks, Android folks, and the linux-wireless community, our own
scattered random folks of firmware reviewer folks.

These patches are based on top of linux-next next-20180504, they are
also available in a respective git branch, both for linux-next [1] and
linux [2].

Question, and specially rants are greatly appreciated, and of course...
may the 4th be with you.

[0] https://github.com/teg/firmwared
[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux-next.git/log/?h=20180504-firmware_loader
[2] 
https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux.git/log/?h=20180504-firmware_loader

  Luis

Andres Rodriguez (3):
  firmware: wrap FW_OPT_* into an enum
  firmware: use () to terminate kernel-doc function names
  firmware: rename fw_sysfs_fallback to firmware_fallback_sysfs()

Luis R. Rodriguez (3):
  firmware_loader: document firmware_sysfs_fallback()
  firmware_loader: enhance Kconfig documentation over FW_LOADER
  firmware_loader: move kconfig FW_LOADER entries to its own file

 drivers/base/Kconfig|  90 +++---
 drivers/base/firmware_loader/Kconfig| 149 
 drivers/base/firmware_loader/fallback.c |  46 +---
 drivers/base/firmware_loader/fallback.h |  18 +--
 drivers/base/firmware_loader/firmware.h |  37 --
 drivers/base/firmware_loader/main.c |  28 ++---
 6 files changed, 252 insertions(+), 116 deletions(-)
 create mode 100644 drivers/base/firmware_loader/Kconfig

-- 
2.17.0

[PATCH v5 2/6] firmware: use () to terminate kernel-doc function names

[Luis R. Rodriguez]

From: Andres Rodriguez <andre...@gmail.com>

The kernel-doc spec dictates a function name ends in ().

Signed-off-by: Andres Rodriguez <andre...@gmail.com>
Acked-by: Randy Dunlap <rdun...@infradead.org>
Acked-by: Luis R. Rodriguez <mcg...@kernel.org>
Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 drivers/base/firmware_loader/fallback.c |  8 
 drivers/base/firmware_loader/main.c | 20 ++--
 2 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/base/firmware_loader/fallback.c 
b/drivers/base/firmware_loader/fallback.c
index b57a7b3b4122..529f7013616f 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -125,7 +125,7 @@ static ssize_t timeout_show(struct class *class, struct 
class_attribute *attr,
 }
 
 /**
- * firmware_timeout_store - set number of seconds to wait for firmware
+ * firmware_timeout_store() - set number of seconds to wait for firmware
  * @class: device class pointer
  * @attr: device attribute pointer
  * @buf: buffer to scan for timeout value
@@ -239,7 +239,7 @@ static int map_fw_priv_pages(struct fw_priv *fw_priv)
 }
 
 /**
- * firmware_loading_store - set value in the 'loading' control file
+ * firmware_loading_store() - set value in the 'loading' control file
  * @dev: device pointer
  * @attr: device attribute pointer
  * @buf: buffer to scan for loading control value
@@ -431,7 +431,7 @@ static int fw_realloc_pages(struct fw_sysfs *fw_sysfs, int 
min_size)
 }
 
 /**
- * firmware_data_write - write method for firmware
+ * firmware_data_write() - write method for firmware
  * @filp: open sysfs file
  * @kobj: kobject for the device
  * @bin_attr: bin_attr structure
@@ -537,7 +537,7 @@ fw_create_instance(struct firmware *firmware, const char 
*fw_name,
 }
 
 /**
- * fw_load_sysfs_fallback - load a firmware via the sysfs fallback mechanism
+ * fw_load_sysfs_fallback() - load a firmware via the sysfs fallback mechanism
  * @fw_sysfs: firmware sysfs information for the firmware to load
  * @opt_flags: flags of options, FW_OPT_*
  * @timeout: timeout to wait for the load
diff --git a/drivers/base/firmware_loader/main.c 
b/drivers/base/firmware_loader/main.c
index 9919f0e6a7cc..7f2bc7e8e3c0 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -597,7 +597,7 @@ _request_firmware(const struct firmware **firmware_p, const 
char *name,
 }
 
 /**
- * request_firmware: - send firmware request and wait for it
+ * request_firmware() - send firmware request and wait for it
  * @firmware_p: pointer to firmware image
  * @name: name of firmware file
  * @device: device for which firmware is being loaded
@@ -657,7 +657,7 @@ int request_firmware_direct(const struct firmware 
**firmware_p,
 EXPORT_SYMBOL_GPL(request_firmware_direct);
 
 /**
- * firmware_request_cache: - cache firmware for suspend so resume can use it
+ * firmware_request_cache() - cache firmware for suspend so resume can use it
  * @name: name of firmware file
  * @device: device for which firmware should be cached for
  *
@@ -681,7 +681,7 @@ int firmware_request_cache(struct device *device, const 
char *name)
 EXPORT_SYMBOL_GPL(firmware_request_cache);
 
 /**
- * request_firmware_into_buf - load firmware into a previously allocated buffer
+ * request_firmware_into_buf() - load firmware into a previously allocated 
buffer
  * @firmware_p: pointer to firmware image
  * @name: name of firmware file
  * @device: device for which firmware is being loaded and DMA region allocated
@@ -713,7 +713,7 @@ request_firmware_into_buf(const struct firmware 
**firmware_p, const char *name,
 EXPORT_SYMBOL(request_firmware_into_buf);
 
 /**
- * release_firmware: - release the resource associated with a firmware image
+ * release_firmware() - release the resource associated with a firmware image
  * @fw: firmware resource to release
  **/
 void release_firmware(const struct firmware *fw)
@@ -755,7 +755,7 @@ static void request_firmware_work_func(struct work_struct 
*work)
 }
 
 /**
- * request_firmware_nowait - asynchronous version of request_firmware
+ * request_firmware_nowait() - asynchronous version of request_firmware
  * @module: module requesting the firmware
  * @uevent: sends uevent to copy the firmware image if this flag
  * is non-zero else the firmware copy must be done manually.
@@ -824,7 +824,7 @@ EXPORT_SYMBOL(request_firmware_nowait);
 static ASYNC_DOMAIN_EXCLUSIVE(fw_cache_domain);
 
 /**
- * cache_firmware - cache one firmware image in kernel memory space
+ * cache_firmware() - cache one firmware image in kernel memory space
  * @fw_name: the firmware image name
  *
  * Cache firmware in kernel memory so that drivers can use it when
@@ -866,7 +866,7 @@ static struct fw_priv *lookup_fw_priv(const char *fw_name)
 }
 
 /**
- * uncache_firmware - remove one cached firmware image
+ * uncache_firmware() - remove one cached firmware image
  * @fw_name: the firmware 

[PATCH v5 4/6] firmware_loader: document firmware_sysfs_fallback()

[Luis R. Rodriguez]

This also sets the expecations for future fallback interfaces, even
if they are not exported.

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

diff --git a/drivers/base/firmware_loader/fallback.c 
b/drivers/base/firmware_loader/fallback.c
index 3db9e0f225ac..9169e7b9800c 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -662,6 +662,26 @@ static bool fw_run_sysfs_fallback(enum fw_opt opt_flags)
return fw_force_sysfs_fallback(opt_flags);
 }
 
+/**
+ * firmware_fallback_sysfs() - use the fallback mechanism to find firmware
+ * @fw: pointer to firmware image
+ * @name: name of firmware file to look for
+ * @device: device for which firmware is being loaded
+ * @opt_flags: options to control firmware loading behaviour
+ * @ret: return value from direct lookup which triggered the fallback mechanism
+ *
+ * This function is called if direct lookup for the firmware failed, it enables
+ * a fallback mechanism through userspace by exposing a sysfs loading
+ * interface. Userspace is in charge of loading the firmware through the syfs
+ * loading interface. This syfs fallback mechanism may be disabled completely
+ * on a system by setting the proc sysctl value ignore_sysfs_fallback to true.
+ * If this false we check if the internal API caller set the @FW_OPT_NOFALLBACK
+ * flag, if so it would also disable the fallback mechanism. A system may want
+ * to enfoce the sysfs fallback mechanism at all times, it can do this by
+ * setting ignore_sysfs_fallback to false and force_sysfs_fallback to true.
+ * Enabling force_sysfs_fallback is functionally equivalent to build a kernel
+ * with CONFIG_FW_LOADER_USER_HELPER_FALLBACK.
+ **/
 int firmware_fallback_sysfs(struct firmware *fw, const char *name,
struct device *device,
enum fw_opt opt_flags,
-- 
2.17.0

[PATCH v5 6/6] firmware_loader: move kconfig FW_LOADER entries to its own file

[Luis R. Rodriguez]

This will make it easier to track and easier to understand
what components and features are part of the FW_LOADER. There
are some components related to firmware which have *nothing* to
do with the FW_LOADER, souch as PREVENT_FIRMWARE_BUILD.

Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 drivers/base/Kconfig | 150 +--
 drivers/base/firmware_loader/Kconfig | 149 ++
 2 files changed, 150 insertions(+), 149 deletions(-)
 create mode 100644 drivers/base/firmware_loader/Kconfig

diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
index bf2d464b0e2c..06d6e27784fa 100644
--- a/drivers/base/Kconfig
+++ b/drivers/base/Kconfig
@@ -88,155 +88,7 @@ config PREVENT_FIRMWARE_BUILD
o CONFIG_WANXL through CONFIG_WANXL_BUILD_FIRMWARE
o CONFIG_SCSI_AIC79XX through CONFIG_AIC79XX_BUILD_FIRMWARE
 
-menu "Firmware loader"
-
-config FW_LOADER
-   tristate "Firmware loading facility" if EXPERT
-   default y
-   ---help---
- This enables the firmware loading facility in the kernel. The kernel
- will first look for built-in firmware, if it has any. Next, it will
- look for the requested firmware in a series of filesystem paths:
-
-   o firmware_class path module parameter or kernel boot param
-   o /lib/firmware/updates/UTS_RELEASE
-   o /lib/firmware/updates
-   o /lib/firmware/UTS_RELEASE
-   o /lib/firmware
-
- Enabling this feature only increases your kernel image by about
- 828 bytes, enable this option unless you are certain you don't
- need firmware.
-
- You typically want this built-in (=y) but you can also enable this
- as a module, in which case the firmware_class module will be built.
- You also want to be sure to enable this built-in if you are going to
- enable built-in firmware (CONFIG_EXTRA_FIRMWARE).
-
-if FW_LOADER
-
-config EXTRA_FIRMWARE
-   string "Build these firmware blobs into the kernel binary"
-   help
- Device drivers which require firmware can typically deal with
- having the kernel load firmware from the various supported
- /lib/firmware/ paths. This option enables you to build into the
- kernel firmware files. Built-in firmware searches are preceeded
- over firmware lookups using your filesystem over the supported
- /lib/firmware paths documented on CONFIG_FW_LOADER.
-
- This may be useful for testing or if the firmware is required early on
- in boot and cannot rely on the firmware being placed in an initrd or
- initramfs.
-
- This option is a string and takes the (space-separated) names of the
- firmware files -- the same names that appear in MODULE_FIRMWARE()
- and request_firmware() in the source. These files should exist under
- the directory specified by the EXTRA_FIRMWARE_DIR option, which is
- /lib/firmware by default.
-
- For example, you might set CONFIG_EXTRA_FIRMWARE="usb8388.bin", copy
- the usb8388.bin file into /lib/firmware, and build the kernel. Then
- any request_firmware("usb8388.bin") will be satisfied internally
- inside the kernel without ever looking at your filesystem at runtime.
-
- WARNING: If you include additional firmware files into your binary
- kernel image that are not available under the terms of the GPL,
- then it may be a violation of the GPL to distribute the resulting
- image since it combines both GPL and non-GPL work. You should
- consult a lawyer of your own before distributing such an image.
-
-config EXTRA_FIRMWARE_DIR
-   string "Firmware blobs root directory"
-   depends on EXTRA_FIRMWARE != ""
-   default "/lib/firmware"
-   help
- This option controls the directory in which the kernel build system
- looks for the firmware files listed in the EXTRA_FIRMWARE option.
-
-config FW_LOADER_USER_HELPER
-   bool "Enable the firmware sysfs fallback mechanism"
-   help
- This option enables a sysfs loading facility to enable firmware
- loading to the kernel through userspace as a fallback mechanism
- if and only if the kernel's direct filesystem lookup for the
- firmware failed using the different /lib/firmware/ paths, or the
- path specified in the firmware_class path module parameter, or the
- firmware_class path kernel boot parameter if the firmware_class is
- built-in. For details on how to work with the sysfs fallback mechanism
- refer to Documentation/driver-api/firmware/fallback-mechanisms.rst.
-
- The direct filesystem lookup for firwmare is always used first now.
-
- If the kernel's direct filesystem loo

[PATCH v5 5/6] firmware_loader: enhance Kconfig documentation over FW_LOADER

[Luis R. Rodriguez]

If you try to read FW_LOADER today it speaks of old riddles and
unless you have been following development closely you will loose
track of what is what. Even the documentation for PREVENT_FIRMWARE_BUILD
is a bit fuzzy and how it fits into this big picture.

Give the FW_LOADER kconfig documentation some love with more up to
date developments and recommendations. While at it, wrap the FW_LOADER
code into its own menu to compartamentalize and make it clearer which
components really are part of the FW_LOADER. This should also make
it easier to later move these kconfig entries into the firmware_loader/
directory later.

Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 drivers/base/Kconfig | 160 ++-
 1 file changed, 126 insertions(+), 34 deletions(-)

diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
index 29b0eb452b3a..bf2d464b0e2c 100644
--- a/drivers/base/Kconfig
+++ b/drivers/base/Kconfig
@@ -70,39 +70,64 @@ config STANDALONE
  If unsure, say Y.
 
 config PREVENT_FIRMWARE_BUILD
-   bool "Prevent firmware from being built"
+   bool "Disable drivers features which enable custom firmware building"
default y
help
- Say yes to avoid building firmware. Firmware is usually shipped
- with the driver and only when updating the firmware should a
- rebuild be made.
- If unsure, say Y here.
+ Say yes to disable driver features which enable building a custom
+ driver firmwar at kernel build time. These drivers do not use the
+ kernel firmware API to load firmware (CONFIG_FW_LOADER), instead they
+ use their own custom loading mechanism. The required firmware is
+ usually shipped with the driver, building the driver firmware
+ should only be needed if you have an updated firmware source.
+
+ Firmware should not be being built as part of kernel, these days
+ you should always prevent this and say Y here. There are only two
+ old drivers which enable building of its firmware at kernel build
+ time:
+
+   o CONFIG_WANXL through CONFIG_WANXL_BUILD_FIRMWARE
+   o CONFIG_SCSI_AIC79XX through CONFIG_AIC79XX_BUILD_FIRMWARE
+
+menu "Firmware loader"
 
 config FW_LOADER
-   tristate "Userspace firmware loading support" if EXPERT
+   tristate "Firmware loading facility" if EXPERT
default y
---help---
- This option is provided for the case where none of the in-tree modules
- require userspace firmware loading support, but a module built
- out-of-tree does.
+ This enables the firmware loading facility in the kernel. The kernel
+ will first look for built-in firmware, if it has any. Next, it will
+ look for the requested firmware in a series of filesystem paths:
+
+   o firmware_class path module parameter or kernel boot param
+   o /lib/firmware/updates/UTS_RELEASE
+   o /lib/firmware/updates
+   o /lib/firmware/UTS_RELEASE
+   o /lib/firmware
+
+ Enabling this feature only increases your kernel image by about
+ 828 bytes, enable this option unless you are certain you don't
+ need firmware.
+
+ You typically want this built-in (=y) but you can also enable this
+ as a module, in which case the firmware_class module will be built.
+ You also want to be sure to enable this built-in if you are going to
+ enable built-in firmware (CONFIG_EXTRA_FIRMWARE).
+
+if FW_LOADER
 
 config EXTRA_FIRMWARE
-   string "External firmware blobs to build into the kernel binary"
-   depends on FW_LOADER
+   string "Build these firmware blobs into the kernel binary"
help
- Various drivers in the kernel source tree may require firmware,
- which is generally available in your distribution's linux-firmware
- package.
+ Device drivers which require firmware can typically deal with
+ having the kernel load firmware from the various supported
+ /lib/firmware/ paths. This option enables you to build into the
+ kernel firmware files. Built-in firmware searches are preceeded
+ over firmware lookups using your filesystem over the supported
+ /lib/firmware paths documented on CONFIG_FW_LOADER.
 
- The linux-firmware package should install firmware into
- /lib/firmware/ on your system, so they can be loaded by userspace
- helpers on request.
-
- This option allows firmware to be built into the kernel for the case
- where the user either cannot or doesn't want to provide it from
- userspace at runtime (for example, when the firmware in question is
- required for accessing the boot device, and the user doesn't want to
- use an initrd).
+

Re: [PATCH 5/9] firmware: add function to load firmware without warnings v5

[Luis R. Rodriguez]

On Mon, Apr 23, 2018 at 04:12:01PM -0400, Andres Rodriguez wrote:
> diff --git a/include/linux/firmware.h b/include/linux/firmware.h
> index db8351a42405..a34e16f77f20 100644
> --- a/include/linux/firmware.h
> +++ b/include/linux/firmware.h
> @@ -42,6 +42,8 @@ struct builtin_fw {
>  #if defined(CONFIG_FW_LOADER) || (defined(CONFIG_FW_LOADER_MODULE) && 
> defined(MODULE))
>  int firmware_request(const struct firmware **fw, const char *name,
>struct device *device);
> +int firmware_request_nowarn(const struct firmware **fw, const char *name,
> + struct device *device);
>  int firmware_request_nowait(
>   struct module *module, bool uevent,
>   const char *name, struct device *device, gfp_t gfp, void *context,

You also missed the firmware_request_nowarn() call on the #else. I'll add it
and re-submit myself.

In future patches about firmware please also Cc Mimi Zohar 
,
and Kees Cook . You can also use the long list (modulo, 
not
the EFI list) that Hans used on his EFI patches. I realize its long but its just
to ensure enough folks get to review and eybeball the code.

  Luis

Re: [PATCH 7/9] firmware: use rename fw_sysfs_fallback to use the firmware_ prefix

[Luis R. Rodriguez]

On Mon, Apr 23, 2018 at 04:12:03PM -0400, Andres Rodriguez wrote:
> Use the correct prefix for symbols exported by firmware_loader(). This
> is done since firmware_sysfs_fallback() is now exposed through
> kernel-doc.
> 
> Signed-off-by: Andres Rodriguez 
> ---
>  drivers/base/firmware_loader/fallback.c | 8 
>  drivers/base/firmware_loader/fallback.h | 4 ++--
>  drivers/base/firmware_loader/firmware.h | 6 +++---
>  drivers/base/firmware_loader/main.c | 2 +-
>  4 files changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/base/firmware_loader/fallback.c 
> b/drivers/base/firmware_loader/fallback.c
> index 1a47ddc70c31..fc186b5bccce 100644
> --- a/drivers/base/firmware_loader/fallback.c
> +++ b/drivers/base/firmware_loader/fallback.c
> @@ -661,10 +661,10 @@ static bool fw_run_sysfs_fallback(enum fw_opt opt_flags)
>   return fw_force_sysfs_fallback(opt_flags);
>  }
>  
> -int fw_sysfs_fallback(struct firmware *fw, const char *name,
> -   struct device *device,
> -   enum fw_opt opt_flags,
> -   int ret)
> +int firmware_sysfs_fallback(struct firmware *fw, const char *name,
> + struct device *device,
> + enum fw_opt opt_flags,
> + int ret)

Since we may get more than one fallback later I'll usee the 
firmware_fallback_sysfs() here.
I'll do this change and resubmit myself.

>  {
>   if (!fw_run_sysfs_fallback(opt_flags))
>   return ret;
> diff --git a/drivers/base/firmware_loader/fallback.h 
> b/drivers/base/firmware_loader/fallback.h
> index a3b73a09db6c..8cfaa3299bb7 100644
> --- a/drivers/base/firmware_loader/fallback.h
> +++ b/drivers/base/firmware_loader/fallback.h
> @@ -31,7 +31,7 @@ struct firmware_fallback_config {
>  };
>  
>  #ifdef CONFIG_FW_LOADER_USER_HELPER
> -int fw_sysfs_fallback(struct firmware *fw, const char *name,
> +int firmware_sysfs_fallback(struct firmware *fw, const char *name,
> struct device *device,
> enum fw_opt opt_flags,
> int ret);
> @@ -43,7 +43,7 @@ void fw_fallback_set_default_timeout(void);
>  int register_sysfs_loader(void);
>  void unregister_sysfs_loader(void);
>  #else /* CONFIG_FW_LOADER_USER_HELPER */
> -static inline int fw_sysfs_fallback(struct firmware *fw, const char *name,
> +static inline int firmware_sysfs_fallback(struct firmware *fw, const char 
> *name,
>   struct device *device,
>   enum fw_opt opt_flags,
>   int ret)
> diff --git a/drivers/base/firmware_loader/firmware.h 
> b/drivers/base/firmware_loader/firmware.h
> index a405d400a925..59836d50c5bd 100644
> --- a/drivers/base/firmware_loader/firmware.h
> +++ b/drivers/base/firmware_loader/firmware.h
> @@ -15,12 +15,12 @@
>   * enum fw_opt - options to control firmware loading behaviour
>   *
>   * @FW_OPT_UEVENT: Enables the fallback mechanism to send a kobject uevent
> - *  when the firmware is not found. Userspace is in charge
> - *  to load the firmware using the sysfs loading facility.
> + * when the firmware is not found. Userspace is in charge
> + * to load the firmware using the sysfs loading facility.

This change was really not part of this patch, so I'll merge it in with the
other patch.

>   * @FW_OPT_NOWAIT: Used to describe the firmware request is asynchronous.
>   * @FW_OPT_USERHELPER: Enable the fallback mechanism, in case the direct
>   * filesystem lookup fails at finding the firmware.
> - * For details refer to fw_sysfs_fallback().
> + * For details refer to firmware_sysfs_fallback().
>   * @FW_OPT_NO_WARN: Quiet, avoid printing warning messages.
>   * @FW_OPT_NOCACHE: Disables firmware caching. Firmware caching is used to
>   *  cache the firmware upon suspend, so that upon resume
> diff --git a/drivers/base/firmware_loader/main.c 
> b/drivers/base/firmware_loader/main.c
> index 58aaadf81e12..f009566acd35 100644
> --- a/drivers/base/firmware_loader/main.c
> +++ b/drivers/base/firmware_loader/main.c
> @@ -581,7 +581,7 @@ _firmware_request(const struct firmware **firmware_p, 
> const char *name,
>   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 = firmware_sysfs_fallback(fw, name, device, opt_flags, ret);
>   } else
>   ret = assign_fw(fw, device, opt_flags);
>  
> -- 
> 2.14.1
> 
> 

-- 
Do not panic

Re: [PATCH 6/9] firmware: print firmware name on fallback path

[Luis R. Rodriguez]

On Mon, Apr 23, 2018 at 04:12:02PM -0400, Andres Rodriguez wrote:
> Previously, one could assume the firmware name from the preceding
> message: "Direct firmware load for {name} failed with error %d".
> 
> However, with the new firmware_request_nowarn() entrypoint, the message
> outlined above will not always be printed.

I though the whole point was to not print an error message. What if
we want later to disable this error message? This would prove a bit
pointless.

Let's discuss the exact semantics desired here. Why would only the
fallback be desirable here?

Andres, Kalle?

After we address this I'll address resubmitting this lat patch
along with the last one. For now I'll skip it.

  Luis

> Therefore, we add the firmware name to the fallback path spew in order
> to associate it with the appropriate request.
> 
> Signed-off-by: Andres Rodriguez 
> ---
>  drivers/base/firmware_loader/fallback.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/base/firmware_loader/fallback.c 
> b/drivers/base/firmware_loader/fallback.c
> index e75928458489..1a47ddc70c31 100644
> --- a/drivers/base/firmware_loader/fallback.c
> +++ b/drivers/base/firmware_loader/fallback.c
> @@ -669,6 +669,6 @@ int fw_sysfs_fallback(struct firmware *fw, const char 
> *name,
>   if (!fw_run_sysfs_fallback(opt_flags))
>   return ret;
>  
> - dev_warn(device, "Falling back to user helper\n");
> + dev_warn(device, "Falling back to user helper for %s\n", name);
>   return fw_load_from_user_helper(fw, name, device, opt_flags);
>  }
> -- 
> 2.14.1
> 
> 

-- 
Do not panic

Re: [PATCH 5/9] firmware: add function to load firmware without warnings v5

[Luis R. Rodriguez]

On Mon, Apr 23, 2018 at 04:12:01PM -0400, Andres Rodriguez wrote:
> Currently the firmware loader only exposes one silent path for querying
> optional firmware, and that is firmware_request_direct(). This function
> also disables the fallback path, which might not always be the
> desired behaviour. [0]
> 
> This patch introduces a variations of firmware_request() that enable the
> caller to disable the undesired warning messages. This is equivalent to
> adding FW_OPT_NO_WARN to the old behaviour.
> 
> v2: add header prototype, use updated opt_flags
> v3: split debug message to separate patch
> added _nowait variant
> added documentation
> v4: fix kernel-doc style
> v5: drop _nowait for now, since we lack agreement on the API
> 
> [0]: https://git.kernel.org/linus/c0cc00f250e1
> 
> Signed-off-by: Andres Rodriguez 
> ---
>  .../driver-api/firmware/request_firmware.rst   |  5 +
>  drivers/base/firmware_loader/main.c| 24 
> ++
>  include/linux/firmware.h   |  2 ++
>  3 files changed, 31 insertions(+)
> 
> diff --git a/Documentation/driver-api/firmware/request_firmware.rst 
> b/Documentation/driver-api/firmware/request_firmware.rst
> index 7632f8807472..c8bddbdcfd10 100644
> --- a/Documentation/driver-api/firmware/request_firmware.rst
> +++ b/Documentation/driver-api/firmware/request_firmware.rst
> @@ -20,6 +20,11 @@ firmware_request
>  .. kernel-doc:: drivers/base/firmware_loader/main.c
> :functions: firmware_request
>  
> +firmware_request_nowarn
> +---
> +.. kernel-doc:: drivers/base/firmware_loader/main.c
> +   :functions: firmware_request_nowarn
> +
>  firmware_request_direct
>  ---
>  .. kernel-doc:: drivers/base/firmware_loader/main.c
> diff --git a/drivers/base/firmware_loader/main.c 
> b/drivers/base/firmware_loader/main.c
> index d028cd3257f7..58aaadf81e12 100644
> --- a/drivers/base/firmware_loader/main.c
> +++ b/drivers/base/firmware_loader/main.c
> @@ -631,6 +631,30 @@ firmware_request(const struct firmware **firmware_p, 
> const char *name,
>  }
>  EXPORT_SYMBOL(firmware_request);
>  
> +/**
> + * firmware_request_nowarn() - request for an optional fw module
> + * @firmware: pointer to firmware image
> + * @name: name of firmware file
> + * @device: device for which firmware is being loaded
> + *
> + * This function is similar in behaviour to firmware_request(), except
> + * it doesn't produce warning messages when the file is not found.

It doesn't explain the difference between this and firmware_request_direct()
I'll go ahead and add this and expand also on firmware_request_direct() to be
clearer on the differences to a reader.

I'll queue this up and resubmit myself.

  Luis

> + **/
> +int
> +firmware_request_nowarn(const struct firmware **firmware, const char *name,
> + struct device *device)
> +{
> + int ret;
> +
> + /* Need to pin this module until return */
> + __module_get(THIS_MODULE);
> + ret = _firmware_request(firmware, name, device, NULL, 0,
> + FW_OPT_UEVENT | FW_OPT_NO_WARN);
> + module_put(THIS_MODULE);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(firmware_request_nowarn);
> +
>  /**
>   * firmware_request_direct() - load firmware directly without usermode helper
>   * @firmware_p: pointer to firmware image
> diff --git a/include/linux/firmware.h b/include/linux/firmware.h
> index db8351a42405..a34e16f77f20 100644
> --- a/include/linux/firmware.h
> +++ b/include/linux/firmware.h
> @@ -42,6 +42,8 @@ struct builtin_fw {
>  #if defined(CONFIG_FW_LOADER) || (defined(CONFIG_FW_LOADER_MODULE) && 
> defined(MODULE))
>  int firmware_request(const struct firmware **fw, const char *name,
>struct device *device);
> +int firmware_request_nowarn(const struct firmware **fw, const char *name,
> + struct device *device);
>  int firmware_request_nowait(
>   struct module *module, bool uevent,
>   const char *name, struct device *device, gfp_t gfp, void *context,
> -- 
> 2.14.1
> 
> 

-- 
Do not panic

Re: [PATCH 4/9] firmware: use () to terminate kernel-doc function names

[Luis R. Rodriguez]

On Mon, Apr 23, 2018 at 04:12:00PM -0400, Andres Rodriguez wrote:
> The kernel-doc spec dictates a function name ends in ().
> 
> Signed-off-by: Andres Rodriguez 
> Acked-by: Randy Dunlap 

0-day never got back to me about my full sweep API rename so I think your
changes are best to go in first, I'll do some small changes to account for this
and resubmit to Greg once 0-day gives me its blessing.

  Luis

> ---
>  drivers/base/firmware_loader/fallback.c |  8 
>  drivers/base/firmware_loader/main.c | 22 +++---
>  2 files changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/base/firmware_loader/fallback.c 
> b/drivers/base/firmware_loader/fallback.c
> index ecc49a619298..e75928458489 100644
> --- a/drivers/base/firmware_loader/fallback.c
> +++ b/drivers/base/firmware_loader/fallback.c
> @@ -124,7 +124,7 @@ static ssize_t timeout_show(struct class *class, struct 
> class_attribute *attr,
>  }
>  
>  /**
> - * firmware_timeout_store - set number of seconds to wait for firmware
> + * firmware_timeout_store() - set number of seconds to wait for firmware
>   * @class: device class pointer
>   * @attr: device attribute pointer
>   * @buf: buffer to scan for timeout value
> @@ -238,7 +238,7 @@ static int map_fw_priv_pages(struct fw_priv *fw_priv)
>  }
>  
>  /**
> - * firmware_loading_store - set value in the 'loading' control file
> + * firmware_loading_store() - set value in the 'loading' control file
>   * @dev: device pointer
>   * @attr: device attribute pointer
>   * @buf: buffer to scan for loading control value
> @@ -430,7 +430,7 @@ static int fw_realloc_pages(struct fw_sysfs *fw_sysfs, 
> int min_size)
>  }
>  
>  /**
> - * firmware_data_write - write method for firmware
> + * firmware_data_write() - write method for firmware
>   * @filp: open sysfs file
>   * @kobj: kobject for the device
>   * @bin_attr: bin_attr structure
> @@ -536,7 +536,7 @@ fw_create_instance(struct firmware *firmware, const char 
> *fw_name,
>  }
>  
>  /**
> - * fw_load_sysfs_fallback - load a firmware via the sysfs fallback mechanism
> + * fw_load_sysfs_fallback() - load a firmware via the sysfs fallback 
> mechanism
>   * @fw_sysfs: firmware sysfs information for the firmware to load
>   * @opt_flags: flags of options, FW_OPT_*
>   * @timeout: timeout to wait for the load
> diff --git a/drivers/base/firmware_loader/main.c 
> b/drivers/base/firmware_loader/main.c
> index 5baadad3012d..d028cd3257f7 100644
> --- a/drivers/base/firmware_loader/main.c
> +++ b/drivers/base/firmware_loader/main.c
> @@ -597,7 +597,7 @@ _firmware_request(const struct firmware **firmware_p, 
> const char *name,
>  }
>  
>  /**
> - * firmware_request: - send firmware request and wait for it
> + * firmware_request() - send firmware request and wait for it
>   * @firmware_p: pointer to firmware image
>   * @name: name of firmware file
>   * @device: device for which firmware is being loaded
> @@ -632,7 +632,7 @@ firmware_request(const struct firmware **firmware_p, 
> const char *name,
>  EXPORT_SYMBOL(firmware_request);
>  
>  /**
> - * firmware_request_direct: - load firmware directly without usermode helper
> + * firmware_request_direct() - load firmware directly without usermode helper
>   * @firmware_p: pointer to firmware image
>   * @name: name of firmware file
>   * @device: device for which firmware is being loaded
> @@ -657,7 +657,7 @@ int firmware_request_direct(const struct firmware 
> **firmware_p,
>  EXPORT_SYMBOL_GPL(firmware_request_direct);
>  
>  /**
> - * firmware_request_cache: - cache firmware for suspend so resume can use it
> + * firmware_request_cache() - cache firmware for suspend so resume can use it
>   * @name: name of firmware file
>   * @device: device for which firmware should be cached for
>   *
> @@ -681,7 +681,7 @@ int firmware_request_cache(struct device *device, const 
> char *name)
>  EXPORT_SYMBOL_GPL(firmware_request_cache);
>  
>  /**
> - * firmware_request_into_buf - load firmware into a previously allocated 
> buffer
> + * firmware_request_into_buf() - load firmware into a previously allocated 
> buffer
>   * @firmware_p: pointer to firmware image
>   * @name: name of firmware file
>   * @device: device for which firmware is being loaded and DMA region 
> allocated
> @@ -713,7 +713,7 @@ firmware_request_into_buf(const struct firmware 
> **firmware_p, const char *name,
>  EXPORT_SYMBOL(firmware_request_into_buf);
>  
>  /**
> - * firmware_release: - release the resource associated with a firmware image
> + * firmware_release() - release the resource associated with a firmware image
>   * @fw: firmware resource to release
>   **/
>  void firmware_release(const struct firmware *fw)
> @@ -755,7 +755,7 @@ static void firmware_request_work_func(struct work_struct 
> *work)
>  }
>  
>  /**
> - * firmware_request_nowait - asynchronous version of firmware_request
> + * firmware_request_nowait() - asynchronous version of firmware_request
>   * 

Re: [PATCH 3/9] firmware: add kernel-doc for enum fw_opt

[Luis R. Rodriguez]

On Mon, Apr 23, 2018 at 04:11:59PM -0400, Andres Rodriguez wrote:
> Some basic definitions for the FW_OPT_* values
> 
> v2: Documentation corrections from Luis.

Likewise.

> Signed-off-by: Andres Rodriguez 
> ---
>  drivers/base/firmware_loader/firmware.h | 20 
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/base/firmware_loader/firmware.h 
> b/drivers/base/firmware_loader/firmware.h
> index b252bfa82295..a405d400a925 100644
> --- a/drivers/base/firmware_loader/firmware.h
> +++ b/drivers/base/firmware_loader/firmware.h
> @@ -11,6 +11,26 @@
>  
>  #include 
>  
> +/**
> + * enum fw_opt - options to control firmware loading behaviour
> + *
> + * @FW_OPT_UEVENT: Enables the fallback mechanism to send a kobject uevent
> + *  when the firmware is not found. Userspace is in charge
> + *  to load the firmware using the sysfs loading facility.

The style here is a bit off. I'll just merge this patch with the last one
an change the style a bit to match expectations.

  Luis

> + * @FW_OPT_NOWAIT: Used to describe the firmware request is asynchronous.
> + * @FW_OPT_USERHELPER: Enable the fallback mechanism, in case the direct
> + * filesystem lookup fails at finding the firmware.
> + * For details refer to fw_sysfs_fallback().
> + * @FW_OPT_NO_WARN: Quiet, avoid printing warning messages.
> + * @FW_OPT_NOCACHE: Disables firmware caching. Firmware caching is used to
> + *  cache the firmware upon suspend, so that upon resume
> + *  races against the firmware file lookup on storage is
> + *  avoided. Used for calls where the file may be too
> + *  big, or where the driver takes charge of its own firmware
> + *  caching mechanism.
> + * @FW_OPT_NOFALLBACK: Disable the fallback mechanism. Takes precedence over
> + * _OPT_UEVENT and _OPT_USERHELPER.
> + */
>  enum fw_opt {
>   FW_OPT_UEVENT = BIT(0),
>   FW_OPT_NOWAIT = BIT(1),
> -- 
> 2.14.1
> 
> 

-- 
Do not panic

Re: [PATCH 2/9] firmware: wrap FW_OPT_* into an enum

[Luis R. Rodriguez]

On Mon, Apr 23, 2018 at 04:11:58PM -0400, Andres Rodriguez wrote:
> This should let us associate enum kdoc to these values.
> 
> v2: use BIT() macro

No need to keep the changelog of series here, best to put them below as
I note.
> 
> Signed-off-by: Andres Rodriguez 
> ---

Here is good to add changelog for series.

Anyway this patch can be merged with your next patch. I'll do
this myself and take it in my queue.

  Luis

>  drivers/base/firmware_loader/fallback.c | 12 ++--
>  drivers/base/firmware_loader/fallback.h |  6 --
>  drivers/base/firmware_loader/firmware.h | 18 ++
>  drivers/base/firmware_loader/main.c |  6 +++---
>  4 files changed, 23 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/base/firmware_loader/fallback.c 
> b/drivers/base/firmware_loader/fallback.c
> index da97fc26119c..ecc49a619298 100644
> --- a/drivers/base/firmware_loader/fallback.c
> +++ b/drivers/base/firmware_loader/fallback.c
> @@ -511,7 +511,7 @@ 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, enum fw_opt opt_flags)
>  {
>   struct fw_sysfs *fw_sysfs;
>   struct device *f_dev;
> @@ -544,7 +544,7 @@ fw_create_instance(struct firmware *firmware, const char 
> *fw_name,
>   * In charge of constructing a sysfs fallback interface for firmware loading.
>   **/
>  static int fw_load_sysfs_fallback(struct fw_sysfs *fw_sysfs,
> -   unsigned int opt_flags, long timeout)
> +   enum fw_opt opt_flags, long timeout)
>  {
>   int retval = 0;
>   struct device *f_dev = _sysfs->dev;
> @@ -598,7 +598,7 @@ static int fw_load_sysfs_fallback(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)
> + enum fw_opt opt_flags)
>  {
>   struct fw_sysfs *fw_sysfs;
>   long timeout;
> @@ -639,7 +639,7 @@ static int fw_load_from_user_helper(struct firmware 
> *firmware,
>   return ret;
>  }
>  
> -static bool fw_force_sysfs_fallback(unsigned int opt_flags)
> +static bool fw_force_sysfs_fallback(enum fw_opt opt_flags)
>  {
>   if (fw_fallback_config.force_sysfs_fallback)
>   return true;
> @@ -648,7 +648,7 @@ static bool fw_force_sysfs_fallback(unsigned int 
> opt_flags)
>   return true;
>  }
>  
> -static bool fw_run_sysfs_fallback(unsigned int opt_flags)
> +static bool fw_run_sysfs_fallback(enum fw_opt opt_flags)
>  {
>   if (fw_fallback_config.ignore_sysfs_fallback) {
>   pr_info_once("Ignoring firmware sysfs fallback due to sysctl 
> knob\n");
> @@ -663,7 +663,7 @@ static bool fw_run_sysfs_fallback(unsigned int opt_flags)
>  
>  int fw_sysfs_fallback(struct firmware *fw, const char *name,
> struct device *device,
> -   unsigned int opt_flags,
> +   enum fw_opt opt_flags,
> int ret)
>  {
>   if (!fw_run_sysfs_fallback(opt_flags))
> diff --git a/drivers/base/firmware_loader/fallback.h 
> b/drivers/base/firmware_loader/fallback.h
> index f8255670a663..a3b73a09db6c 100644
> --- a/drivers/base/firmware_loader/fallback.h
> +++ b/drivers/base/firmware_loader/fallback.h
> @@ -5,6 +5,8 @@
>  #include 
>  #include 
>  
> +#include "firmware.h"
> +
>  /**
>   * struct firmware_fallback_config - firmware fallback configuration settings
>   *
> @@ -31,7 +33,7 @@ struct firmware_fallback_config {
>  #ifdef CONFIG_FW_LOADER_USER_HELPER
>  int fw_sysfs_fallback(struct firmware *fw, const char *name,
> struct device *device,
> -   unsigned int opt_flags,
> +   enum fw_opt opt_flags,
> int ret);
>  void kill_pending_fw_fallback_reqs(bool only_kill_custom);
>  
> @@ -43,7 +45,7 @@ void unregister_sysfs_loader(void);
>  #else /* CONFIG_FW_LOADER_USER_HELPER */
>  static inline int fw_sysfs_fallback(struct firmware *fw, const char *name,
>   struct device *device,
> - unsigned int opt_flags,
> + enum fw_opt opt_flags,
>   int ret)
>  {
>   /* Keep carrying over the same error */
> diff --git a/drivers/base/firmware_loader/firmware.h 
> b/drivers/base/firmware_loader/firmware.h
> index d11d37db370b..b252bfa82295 100644
> --- a/drivers/base/firmware_loader/firmware.h
> +++ b/drivers/base/firmware_loader/firmware.h
> @@ -2,6 +2,7 @@
>  #ifndef __FIRMWARE_LOADER_H
>  #define __FIRMWARE_LOADER_H
>  
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -10,13 +11,14 @@
>  
>  #include 
>  

Re: [PATCH 1/9] firmware: some documentation fixes

[Luis R. Rodriguez]

On Mon, Apr 23, 2018 at 04:11:57PM -0400, Andres Rodriguez wrote:
> Including:
>  - Fixup outdated kernel-doc paths
>  - Slightly too short title underline
>  - Some typos
> 
> Signed-off-by: Andres Rodriguez 

This already got merged.

[PATCH v4 0/3] firmware: add firmware_request_cache()

[Luis R. Rodriguez]

Greg,

Here are the last straggler patches to clean up what was left which you
requested and to add the new firmware_request_cache() call to fix a
corner case suspend issue.

The extra space character you found in earlier patches no longer needs
to be fixed as that assignement was replaced with a new line in later
patches which does not have the extra space in the assignment. The only
other straggler fix needed which you had pointed out was the debugfs typo.

The new API call was renamed as suggested to firmware_request_cache(),
with the API name first. I'll use a separate series to start addressing
the firmware API rename for the rest of the callers. That will take some
time as I'm running quite a bit of tests on those changes and I am going
to wait for 0-day to give me its blesssings.

Question, feedback, and specially rants are greatly appreciated.

  Luis

Luis R. Rodriguez (3):
  firmware: fix typo on pr_info_once() when ignore_sysfs_fallback is
used
  firmware: add firmware_request_cache() to help with cache on reboot
  mt7601u: use firmware_request_cache() to address cache on reboot

 .../driver-api/firmware/request_firmware.rst   | 14 +
 drivers/base/firmware_loader/fallback.c|  2 +-
 drivers/base/firmware_loader/main.c| 24 ++
 drivers/net/wireless/mediatek/mt7601u/mcu.c|  2 +-
 include/linux/firmware.h   |  3 +++
 5 files changed, 43 insertions(+), 2 deletions(-)

-- 
2.16.2

[PATCH v4 2/3] firmware: add firmware_request_cache() to help with cache on reboot

[Luis R. Rodriguez]

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

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

Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 .../driver-api/firmware/request_firmware.rst   | 14 +
 drivers/base/firmware_loader/main.c| 24 ++
 include/linux/firmware.h   |  3 +++
 3 files changed, 41 insertions(+)

diff --git a/Documentation/driver-api/firmware/request_firmware.rst 
b/Documentation/driver-api/firmware/request_firmware.rst
index cc0aea880824..cf4516dfbf96 100644
--- a/Documentation/driver-api/firmware/request_firmware.rst
+++ b/Documentation/driver-api/firmware/request_firmware.rst
@@ -44,6 +44,20 @@ request_firmware_nowait
 .. kernel-doc:: drivers/base/firmware_class.c
:functions: request_firmware_nowait
 
+Special optimizations on reboot
+===
+
+Some devices have an optimization in place to enable the firmware to be
+retained during system reboot. When such optimizations are used the driver
+author must ensure the firmware is still available on resume from suspend,
+this can be done with firmware_request_cache() insted of requesting for the
+firmare to be loaded.
+
+firmware_request_cache()
+---
+.. kernel-doc:: drivers/base/firmware_class.c
+   :functions: firmware_request_cache
+
 request firmware API expected driver use
 
 
diff --git a/drivers/base/firmware_loader/main.c 
b/drivers/base/firmware_loader/main.c
index 2913bb0e5e7b..eb34089e4299 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -656,6 +656,30 @@ int request_firmware_direct(const struct firmware 
**firmware_p,
 }
 EXPORT_SYMBOL_GPL(request_firmware_direct);
 
+/**
+ * firmware_request_cache: - cache firmware for suspend so resume can use it
+ * @name: name of firmware file
+ * @device: device for which firmware should be cached for
+ *
+ * There are some devices with an optimization that enables the device to not
+ * require loading firmware on system reboot. This optimization may still
+ * require the firmware present on resume from suspend. This routine can be
+ * used to ensure the firmware is present on resume from suspend in these
+ * situations. This helper is not compatible with drivers which use
+ * request_firmware_into_buf() or request_firmware_nowait() with no uevent set.
+ **/
+int firmware_request_cache(struct device *device, const char *name)
+{
+   int ret;
+
+   mutex_lock(_lock);
+   ret = fw_add_devm_name(device, name);
+   mutex_unlock(_lock);
+
+   return ret;
+}
+EXPORT_SYMBOL_GPL(firmware_request_cache);
+
 /**
  * request_firmware_into_buf - load firmware into a previously allocated buffer
  * @firmware_p: pointer to firmware image
diff --git a/include/linux/firmware.h b/include/linux/firmware.h
index d4508080348d..41050417cafb 100644
--- a/include/linux/firmware.h
+++ b/include/linux/firmware.h
@@ -85,4 +85,7 @@ static inline int request_firmware_into_buf(const struct 
firmware **firmware_p,
 }
 
 #endif
+
+int firmware_request_cache(struct device *device, const char *name);
+
 #endif
-- 
2.16.2

[PATCH v4 1/3] firmware: fix typo on pr_info_once() when ignore_sysfs_fallback is used

[Luis R. Rodriguez]

When the sysctl knob is used ignore the fallback mechanism we pr_info_once()
to ensure its noted the knob was used. The print incorrectly states its a
debugfs knob, its a sysctl knob, so correct this typo.

Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 drivers/base/firmware_loader/fallback.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/base/firmware_loader/fallback.c 
b/drivers/base/firmware_loader/fallback.c
index d231bbcb95d7..31b5015b59fe 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -652,7 +652,7 @@ static bool fw_force_sysfs_fallback(unsigned int opt_flags)
 static bool fw_run_sysfs_fallback(unsigned int opt_flags)
 {
if (fw_fallback_config.ignore_sysfs_fallback) {
-   pr_info_once("Ignoring firmware sysfs fallback due to debugfs 
knob\n");
+   pr_info_once("Ignoring firmware sysfs fallback due to sysctl 
knob\n");
return false;
}
 
-- 
2.16.2

[PATCH v4 3/3] mt7601u: use firmware_request_cache() to address cache on reboot

[Luis R. Rodriguez]

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

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

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

Reported-by: Cantabile <cantabile.d...@gmail.com>
Tested-by: Cantabile <cantabile.d...@gmail.com>
Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 drivers/net/wireless/mediatek/mt7601u/mcu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/mediatek/mt7601u/mcu.c 
b/drivers/net/wireless/mediatek/mt7601u/mcu.c
index d9d6fd7eff5e..61705f679856 100644
--- a/drivers/net/wireless/mediatek/mt7601u/mcu.c
+++ b/drivers/net/wireless/mediatek/mt7601u/mcu.c
@@ -420,7 +420,7 @@ static int mt7601u_load_firmware(struct mt7601u_dev *dev)
 MT_USB_DMA_CFG_TX_BULK_EN));
 
if (firmware_running(dev))
-   return 0;
+   return firmware_request_cache(dev->dev, MT7601U_FIRMWARE);
 
ret = request_firmware(, MT7601U_FIRMWARE, dev->dev);
if (ret)
-- 
2.16.2

Re: [PATCH v3 19/20] firmware: add request_firmware_cache() to help with cache on reboot

[Luis R. Rodriguez]

On Tue, Mar 20, 2018 at 02:54:44PM -0400, Konstantin Ryabitsev wrote:
> On 03/20/18 14:24, Luis R. Rodriguez wrote:
> > *Iff* this seems sensible, this would only mean kernel.org would have to 
> > start
> > accepting git notes long term, for those who optionally want to push them or
> > fetch them.
> 
> We don't disallow them. You just need to make sure you're fetching and
> pushing them, because they aren't by default. You can set this in your
> kernel.org origin section of .git/config:
> 
> [remote origin]
>   ...
>   fetch = +refs/notes/*:refs/notes/*
> 
> And then push them separately as "git push origin refs/notes/*".

Superb, thanks!

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

Good to know thanks, and it looks good!

> If you do start using notes, I strongly suggest you pick a dedicated
> refspace for it instead of putting things into the default
> refs/notes/commits, e.g.:
> 
> git notes --ref crosstree [...]
> 
> This will create refs/notes/crosstree that has less of a chance to be
> clobbered by someone else's use of notes.

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

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

If it was required sed I guess

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

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

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

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

  Luis

Re: [PATCH v3 19/20] firmware: add request_firmware_cache() to help with cache on reboot

[Luis R. Rodriguez]

On Tue, Mar 20, 2018 at 06:38:01PM +0100, Greg KH wrote:
> On Tue, Mar 20, 2018 at 05:34:09PM +0000, Luis R. Rodriguez wrote:
> > On Tue, Mar 20, 2018 at 09:30:55AM +0100, Greg KH wrote:
> > > On Sat, Mar 10, 2018 at 06:15:00AM -0800, Luis R. Rodriguez wrote:
> > > > +EXPORT_SYMBOL_GPL(request_firmware_cache);
> > > 
> > > I know you are just following the existing naming scheme, but please
> > > let's not continue the problem here.  Can you prefix all of the firmware
> > > exported symbols with "firmware_", and then the verb.  So this would be
> > > "firmware_request_cache",
> > 
> > Sure.
> > 
> > >  and the older function would be
> > > "firmware_request_direct".
> > 
> > Sure, so you want to also rename the old exported symbols, and add a macro
> > or static inline to enable use of the older names?
> 
> Renaming is best, let's not keep them around for no good reason :)

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

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

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

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

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

"iso" for isomorphism

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

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

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

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

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

  Luis

Re: [PATCH v3 19/20] firmware: add request_firmware_cache() to help with cache on reboot

[Luis R. Rodriguez]

On Tue, Mar 20, 2018 at 09:30:55AM +0100, Greg KH wrote:
> On Sat, Mar 10, 2018 at 06:15:00AM -0800, Luis R. Rodriguez wrote:
> > +EXPORT_SYMBOL_GPL(request_firmware_cache);
> 
> I know you are just following the existing naming scheme, but please
> let's not continue the problem here.  Can you prefix all of the firmware
> exported symbols with "firmware_", and then the verb.  So this would be
> "firmware_request_cache",

Sure.

>  and the older function would be
> "firmware_request_direct".

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

> I've stopped here in the patch series, applying the others now, so feel
> free to rebase and resend what I've missed, and the minor fixups for the
> prior patches.

Sure thing.

   Luis

Re: [PATCH v3 11/20] firmware: enable to force disable the fallback mechanism at run time

[Luis R. Rodriguez]

On Wed, Mar 14, 2018 at 12:00 PM, Greg KH <gre...@linuxfoundation.org> wrote:
> On Sat, Mar 10, 2018 at 06:14:52AM -0800, Luis R. Rodriguez wrote:
>> You currently need four different kernel builds to test the firmware
>> API fully. By adding a proc knob to force disable the fallback mechanism
>> completely we are able to reduce the amount of kernels you need built
>> to test the firmware API down to two.
>>
>> Acked-by: Kees Cook <keesc...@chromium.org>
>> Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
>> ---
>>  drivers/base/firmware_loader/fallback.c   | 5 +
>>  drivers/base/firmware_loader/fallback.h   | 4 
>>  drivers/base/firmware_loader/fallback_table.c | 9 +
>>  3 files changed, 18 insertions(+)
>>
>> diff --git a/drivers/base/firmware_loader/fallback.c 
>> b/drivers/base/firmware_loader/fallback.c
>> index 45cc40933a47..d6838e7ec00c 100644
>> --- a/drivers/base/firmware_loader/fallback.c
>> +++ b/drivers/base/firmware_loader/fallback.c
>> @@ -643,6 +643,11 @@ static bool fw_force_sysfs_fallback(unsigned int 
>> opt_flags)
>>
>>  static bool fw_run_sysfs_fallback(unsigned int opt_flags)
>>  {
>> + if (fw_fallback_config.ignore_sysfs_fallback) {
>> + pr_info_once("Ignoring firmware sysfs fallback due to debugfs 
>> knob\n");
>
> s/debugfs/sysctl/ right?

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

  Luis

Re: [PATCH v3 05/20] firmware: simplify CONFIG_FW_LOADER_USER_HELPER_FALLBACK further

[Luis R. Rodriguez]

On Wed, Mar 14, 2018 at 11:53 AM, Greg KH <gre...@linuxfoundation.org> wrote:
> On Sat, Mar 10, 2018 at 06:14:46AM -0800, Luis R. Rodriguez wrote:
>> All CONFIG_FW_LOADER_USER_HELPER_FALLBACK really is, is just a bool,
>> initailized at build time. Define it as such. This simplifies the
>> logic even further, removing now all explicit #ifdefs around the code.
>>
>> Acked-by: Kees Cook <keesc...@chromium.org>
>> Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
>> ---
>>  drivers/base/firmware_loader.c | 25 ++---
>>  1 file changed, 18 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/base/firmware_loader.c b/drivers/base/firmware_loader.c
>> index 7dd36ace6152..59dba794ce1a 100644
>> --- a/drivers/base/firmware_loader.c
>> +++ b/drivers/base/firmware_loader.c
>> @@ -266,6 +266,22 @@ static inline bool fw_state_is_aborted(struct fw_priv 
>> *fw_priv)
>>
>>  #ifdef CONFIG_FW_LOADER_USER_HELPER
>>
>> +/**
>> + * struct firmware_fallback_config - firmware fallback configuratioon 
>> settings
>> + *
>> + * Helps describe and fine tune the fallback mechanism.
>> + *
>> + * @force_sysfs_fallback: force the sysfs fallback mechanism to be used
>> + *   as if one had enabled CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y.
>> + */
>> +struct firmware_fallback_config {
>> + bool force_sysfs_fallback;
>> +};
>
> This is C, why are you messing around with a structure and "getters and
> setters" for a set of simple values?
>
>> +static const struct firmware_fallback_config fw_fallback_config = {
>> + .force_sysfs_fallback = 
>> IS_ENABLED(CONFIG_FW_LOADER_USER_HELPER_FALLBACK),
>> +};
>
> static bool force_sysfs_fallback = 
> IS_ENABLED(CONFIG_FW_LOADER_USER_HELPER_FALLBACK);

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

> Then just read/write the foolish thing, don't make this more complex
> than is needed please.

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

> There is a "getter and setters are evil" rant somewhere out there
> online, if you really need me to dig up the old tired arguments :)

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

  Luis

Re: [PATCH v3 07/20] firmware: move loading timeout under struct firmware_fallback_config

[Luis R. Rodriguez]

On Wed, Mar 14, 2018 at 11:56 AM, Greg KH <gre...@linuxfoundation.org> wrote:
> On Sat, Mar 10, 2018 at 06:14:48AM -0800, Luis R. Rodriguez wrote:
>> The timeout is a fallback construct, so we can just stuff the
>> timeout configuration under struct firmware_fallback_config.
>
> Why?  What does it matter?

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

>> While at it, add a few helpers which vets the use of getting or
>> setting the timeout as an int. The main use of the timeout is
>> to set a timeout for completion, and that is used as an unsigned
>> long. There a few cases however where it makes sense to get or
>> set the timeout as an int, the helpers annotate these use cases
>> have been properly vetted for.
>
> This feels really odd to me.  Why would you want to use it as an int,
> just keep it the same "size" everywhere and it should be simpler and
> easier to keep working correctly over time.

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

 Luis

Re: [PATCH v3 00/20] firmware: development for v4.17

[Luis R. Rodriguez]

On Sat, Mar 10, 2018 at 09:16:36AM -0800, Kees Cook wrote:
> On Sat, Mar 10, 2018 at 6:14 AM, Luis R. Rodriguez <mcg...@kernel.org> wrote:
> > Greg,
> >
> > Here's a respin of what I have queued up for v4.17 for the firmware API. It
> > combines the cleanup I've been working on and the addition of the new API 
> > call
> > request_firmware_cache() for fixing a corner case suspend issue on some 
> > type of
> > cards with an optimization in place where the firmware is *not* needed on
> > reboot.
> >
> > The cleanup work allows us to test the firmware API with one kernel
> > configuration. I've addressed Kees' feedback on this respin and
> > combined the code into drivers/base/firmware_class/.
> >
> > I've made one new test_firmware change in consideration for one firmware
> > change, the patch "firmware: ensure the firmware cache is not used on
> > incompatible calls" requires us to modify our tests scripts to use
> > the APIs sanely as well.
> >
> > I've put up these changes on my git tree, refer to the branch
> > "20180307-firmware-dev-for-v4.17" based on linux-next [0] and
> > the same name based on Linus' tree [1].
> >
> > Questions, feedback, and specially rants are always welcomed.
> 
> This all looks good to me! Thanks for respinning. :)

Greg,

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

  Luis

Re: [PATCH] firmware: add a function to load optional firmware v2

[Luis R. Rodriguez]

On Tue, Mar 13, 2018 at 03:16:34PM +0200, Kalle Valo wrote:
> "Luis R. Rodriguez" <mcg...@kernel.org> writes:
> 
> >> +/**
> >> + * request_firmware_optional: - request for an optional fw module
> >> + * @firmware_p: pointer to firmware image
> >> + * @name: name of firmware file
> >> + * @device: device for which firmware is being loaded
> >> + *
> >> + * This function is similar in behaviour to request_firmware(), except
> >> + * it doesn't produce warning messages when the file is not found.
> >> + **/
> >> +int
> >> +request_firmware_optional(const struct firmware **firmware_p, const char 
> >> *name,
> >> + struct device *device)
> >> +{
> >> +   int ret;
> >> +
> >> +   /* 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_NO_WARN );
> >> +   module_put(THIS_MODULE);
> >> +   return ret;
> >> +}
> >> +EXPORT_SYMBOL(request_firmware_optional);
> >
> > New exported symbols for the firmware API should be EXPORT_SYMBOL_GPL().
> 
> To me the word optional feels weird to me. For example, in ath10k I
> suspect we would be only calling request_firmware_optional() with all
> firmware and not request_firmware() at all.
> 
> How about request_firmware_nowarn()? That would even match the
> documentation above.

_nowarn() works with me. Do you at least want the return value to give
an error value if no file was found? This way the driver can decide
when to issue an error if it wants to.

  Luis

Re: [PATCH] firmware: add a function to load optional firmware v2

[Luis R. Rodriguez]

On Tue, Mar 13, 2018 at 03:39:23PM +0200, Kalle Valo wrote:
> "Luis R. Rodriguez" <mcg...@kernel.org> writes:
> 
> > On Mon, Mar 12, 2018 at 12:10:47AM +0100, Arend van Spriel wrote:
> >> On 3/11/2018 5:05 PM, Andres Rodriguez wrote:
> >> > > Your patch series then should also have the driver callers who you
> >> > > want to modify to use this new API. Collect from the 802.11 folks the
> >> > > other drivers which I think they wanted changed as well.
> >> > 
> >> > Arend, Kalle, would love to hear your feedback.
> >> 
> >> I am not sure if it was ath10k, but Kalle will surely know. The other 
> >> driver
> >> firing a whole batch of firmware requests is iwlwifi. These basically try 
> >> to
> >> get latest firmware version and if not there try an older one.
> >
> > Ah I recall now. At least for iwlwifi its that it requests firmware with a
> > range of api files, and we don't need information about files in the middle
> > not found, given all we need to know if is if at lest one file was found
> > or not.
> >
> > I have future code to also enable use of a range request which would replace
> > the recursive nature of iwlwifi's firmware API request, so it simplifies it
> > considerably.
> >
> > Once we get this flag to be silent in, this can be used later. Ie, the new
> > API I'd add would replace the complex api revision thing for an internal 
> > set.
> 
> TBH I doubt we would use this kind of "range" request in ath10k, 

Well it doesn't have the form to use a range either so it wouldn't make sense.

> the
> current code works just fine only if we can get rid of the annoying
> warning from request_firmware(). Unless if it's significantly faster or
> something.

Thanks, I see ath10k uses the sync request_firmware() call, so indeed it
would be a trivial conversion.

Andres can you roll that in for your patch series?

  Luis

Re: [PATCH] firmware: add a function to load optional firmware v2

[Luis R. Rodriguez]

On Mon, Mar 12, 2018 at 12:10:47AM +0100, Arend van Spriel wrote:
> On 3/11/2018 5:05 PM, Andres Rodriguez wrote:
> > > Your patch series then should also have the driver callers who you
> > > want to modify to use this new API. Collect from the 802.11 folks the
> > > other drivers which I think they wanted changed as well.
> > 
> > Arend, Kalle, would love to hear your feedback.
> 
> I am not sure if it was ath10k, but Kalle will surely know. The other driver
> firing a whole batch of firmware requests is iwlwifi. These basically try to
> get latest firmware version and if not there try an older one.

Ah I recall now. At least for iwlwifi its that it requests firmware with a
range of api files, and we don't need information about files in the middle
not found, given all we need to know if is if at lest one file was found
or not.

I have future code to also enable use of a range request which would replace
the recursive nature of iwlwifi's firmware API request, so it simplifies it
considerably.

Once we get this flag to be silent in, this can be used later. Ie, the new
API I'd add would replace the complex api revision thing for an internal set.

> The brcmfmac driver I maintain is slightly different. It downloads two
> distinct pieces of firmware of which one is optional for certain
> configurations. Currently, my driver does two asynchronous requests for it,
> but I consider changing it and only make the first request asynchronous and
> the second request synchronous. You can look at the current code in
> drivers/net/wireless/broadcom/brcmfmac/firmware.c. However, I did quite some
> restructuring last week. Anyway, I probably will end up using the "optional"
> api where appropriate.

Thanks for the details.

  Luis

Re: [PATCH] firmware: add a function to load optional firmware v2

[Luis R. Rodriguez]

First, thanks for your patch!

On Fri, Mar 9, 2018 at 3:09 PM, Andres Rodriguez  wrote:
> Currently the firmware loader only exposes one silent path for querying
> optional firmware, and that is request_firmware_direct(). This function
> also disables the usermodehelper fallback which might not always be the
> desired behaviour.

You should explain on the commit log how at least there is one driver
which needs full silence. Also FYI I think there are others who have
asked for the same before, on the 802.11 world. Kalle, Arend, do you
guys recall if it was a synchronous or async use case?

> This patch introduces request_firmware_optional(), which will not
> produce error/warning messages if the firmware file is not found, but

This looks good for a commit message so far modulo the above.

> will still attempt to query the usermodehelper

The "usermodehelper" is a loose term which I've been trying to fight
in the firmware API. Please refer to this as the fallback mechanism.
The usermode helper actually is kernel/umh.c and in so far as the
firmware API is concerned the kernel/umc.c is used only for the uevent
for the default fallback case. In the custom fallback case there is no
kernel/umc.c API use, and I'm now wondering if use of the UMH lock
doesn't make sense there and we should remove it.

> for the optional
> firmware. Effectively, FW_OPT_UEVENT | FW_OPT_FALLBACK | FW_OPT_NO_WARN.

Can you do me a favor? The above flags are not well documented at all.
I have a big sized cleanup for v4.17 on my , can you base your patch
on top of my tree, modify these flags to become enums, and in the
process kdocify them as part of your work? Refer to
include/uapi/linux/nl80211.h for a good example of how to properly
kdocify enums.

Please base your patch on top of my tree and branch
20180307-firmware-dev-for-v4.17 [0] and resubmit with the above and
below feedback into consideration. You also I take it have users in
mind? I'd like to see at least one user of the API or this fixing a
reported issue. Ie, if users have reported this as issues incorrectly,
referring to those incorrect posts as issues and how this created
confusion would help.

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

> v2: add header prototype, use updated opt_flags
>
> Signed-off-by: Andres Rodriguez 
> ---
>
> Sorry, I messed up the v1 patch and sent the wrong one from before I
> rebased.
>
>  drivers/base/firmware_class.c | 26 +-
>  include/linux/firmware.h  |  2 ++
>  2 files changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index 7dd36ace6152..4e1eddea241b 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -1181,7 +1181,7 @@ static int fw_sysfs_fallback(struct firmware *fw, const 
> char *name,
> if (!fw_run_sysfs_fallback(opt_flags))
> return ret;
>
> -   dev_warn(device, "Falling back to user helper\n");
> +   dev_warn(device, "Falling back to user helper for %s\n", name);

This seems like it should be a separate patch, and it should be
justified, also please modify the language given what I said above
about kernel/umc.c API use. In so far as your actual change is
concerned for this print, *why* do we need another print with the
firmware name on it? The commit log should explain that very well in a
separate patch.

> return fw_load_from_user_helper(fw, name, device, opt_flags);
>  }
>  #else /* CONFIG_FW_LOADER_USER_HELPER */
> @@ -1351,6 +1351,30 @@ request_firmware(const struct firmware **firmware_p, 
> const char *name,
>  }
>  EXPORT_SYMBOL(request_firmware);
>
> +/**
> + * request_firmware_optional: - request for an optional fw module
> + * @firmware_p: pointer to firmware image
> + * @name: name of firmware file
> + * @device: device for which firmware is being loaded
> + *
> + * This function is similar in behaviour to request_firmware(), except
> + * it doesn't produce warning messages when the file is not found.
> + **/
> +int
> +request_firmware_optional(const struct firmware **firmware_p, const char 
> *name,
> + struct device *device)
> +{
> +   int ret;
> +
> +   /* 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_NO_WARN );
> +   module_put(THIS_MODULE);
> +   return ret;
> +}
> +EXPORT_SYMBOL(request_firmware_optional);

New exported symbols for the firmware API should be EXPORT_SYMBOL_GPL().

  Luis

[PATCH v3 00/20] firmware: development for v4.17

[Luis R. Rodriguez]

Greg,

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

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

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

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

Questions, feedback, and specially rants are always welcomed.

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

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

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

-- 
2.16.2

[PATCH v3 03/20] 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.

Acked-by: Kees Cook <keesc...@chromium.org>
Signed-off-by: Luis R. Rodriguez <mcg...@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 bf850050e5e9..323c921a2469 100755
--- a/tools/testing/selftests/firmware/fw_fallback.sh
+++ b/tools/testing/selftests/firmware/fw_fallback.sh
@@ -11,10 +11,7 @@ source $TEST_DIR/fw_lib.sh
 
 check_mods
 
-# CONFIG_FW_LOADER_USER_HELPER has a sysfs class under /sys/class/firmware/
-# These days no one enables CONFIG_FW_LOADER_USER_HELPER so check for that
-# as an indicator for CONFIG_FW_LOADER_USER_HELPER.
-HAS_FW_LOADER_USER_HELPER=$(if [ -d /sys/class/firmware/ ]; then echo yes; 
else echo no; fi)
+HAS_FW_LOADER_USER_HELPER=$(kconfig_has CONFIG_FW_LOADER_USER_HELPER=y)
 HAS_FW_LOADER_USER_HELPER_FALLBACK=$(kconfig_has 
CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y)
 
 if [ "$HAS_FW_LOADER_USER_HELPER" = "yes" ]; then
-- 
2.16.2

[PATCH v3 02/20] 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.

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

Acked-by: Kees Cook <keesc...@chromium.org>
Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 tools/testing/selftests/firmware/config |  4 
 tools/testing/selftests/firmware/fw_fallback.sh |  6 +-
 tools/testing/selftests/firmware/fw_lib.sh  | 24 
 3 files changed, 33 insertions(+), 1 deletion(-)

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 755147a8c967..bf850050e5e9 100755
--- a/tools/testing/selftests/firmware/fw_fallback.sh
+++ b/tools/testing/selftests/firmware/fw_fallback.sh
@@ -15,6 +15,7 @@ check_mods
 # These days no one enables CONFIG_FW_LOADER_USER_HELPER so check for that
 # as an indicator for CONFIG_FW_LOADER_USER_HELPER.
 HAS_FW_LOADER_USER_HELPER=$(if [ -d /sys/class/firmware/ ]; then echo yes; 
else echo no; fi)
+HAS_FW_LOADER_USER_HELPER_FALLBACK=$(kconfig_has 
CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y)
 
 if [ "$HAS_FW_LOADER_USER_HELPER" = "yes" ]; then
OLD_TIMEOUT=$(cat /sys/class/firmware/timeout)
@@ -287,7 +288,10 @@ run_sysfs_custom_load_tests()
fi
 }
 
-run_sysfs_main_tests
+if [ "$HAS_FW_LOADER_USER_HELPER_FALLBACK" = "yes" ]; then
+   run_sysfs_main_tests
+fi
+
 run_sysfs_custom_load_tests
 
 exit 0
diff --git a/tools/testing/selftests/firmware/fw_lib.sh 
b/tools/testing/selftests/firmware/fw_lib.sh
index c14bbca7ecf9..467567c758b9 100755
--- a/tools/testing/selftests/firmware/fw_lib.sh
+++ b/tools/testing/selftests/firmware/fw_lib.sh
@@ -42,3 +42,27 @@ check_mods()
fi
fi
 }
+
+kconfig_has()
+{
+   if [ -f $PROC_CONFIG ]; then
+   if zgrep -q $1 $PROC_CONFIG 2>/dev/null; then
+   echo "yes"
+   else
+   echo "no"
+   fi
+   else
+   # We currently don't have easy heuristics to infer this
+   # so best we can do is just try to use the kernel assuming
+   # you had enabled it. This matches the old behaviour.
+   if [ "$1" = "CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y" ]; then
+   echo "yes"
+   elif [ "$1" = "CONFIG_FW_LOADER_USER_HELPER=y" ]; then
+   if [ -d /sys/class/firmware/ ]; then
+   echo yes
+   else
+   echo no
+   fi
+   fi
+   fi
+}
-- 
2.16.2

[PATCH v3 01/20] test_firmware: add simple firmware firmware test library

[Luis R. Rodriguez]

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

Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 tools/testing/selftests/firmware/fw_fallback.sh   |  7 ++--
 tools/testing/selftests/firmware/fw_filesystem.sh | 20 ++-
 tools/testing/selftests/firmware/fw_lib.sh| 44 +++
 3 files changed, 51 insertions(+), 20 deletions(-)
 create mode 100755 tools/testing/selftests/firmware/fw_lib.sh

diff --git a/tools/testing/selftests/firmware/fw_fallback.sh 
b/tools/testing/selftests/firmware/fw_fallback.sh
index 722cad91df74..755147a8c967 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,9 +6,10 @@
 # won't find so that we can do the load ourself manually.
 set -e
 
-modprobe test_firmware
+TEST_DIR=$(dirname $0)
+source $TEST_DIR/fw_lib.sh
 
-DIR=/sys/devices/virtual/misc/test_firmware
+check_mods
 
 # CONFIG_FW_LOADER_USER_HELPER has a sysfs class under /sys/class/firmware/
 # These days no one enables CONFIG_FW_LOADER_USER_HELPER so check for that
diff --git a/tools/testing/selftests/firmware/fw_filesystem.sh 
b/tools/testing/selftests/firmware/fw_filesystem.sh
index f9508e1a4058..748f2f5737e9 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,24 +6,10 @@
 # know so we can be sure we're not accidentally testing the user helper.
 set -e
 
-DIR=/sys/devices/virtual/misc/test_firmware
 TEST_DIR=$(dirname $0)
+source $TEST_DIR/fw_lib.sh
 
-test_modprobe()
-{
-   if [ ! -d $DIR ]; then
-   echo "$0: $DIR not present"
-   echo "You must have the following enabled in your kernel:"
-   cat $TEST_DIR/config
-   exit 1
-   fi
-}
-
-trap "test_modprobe" EXIT
-
-if [ ! -d $DIR ]; then
-   modprobe test_firmware
-fi
+check_mods
 
 # CONFIG_FW_LOADER_USER_HELPER has a sysfs class under /sys/class/firmware/
 # These days most distros enable CONFIG_FW_LOADER_USER_HELPER but disable
diff --git a/tools/testing/selftests/firmware/fw_lib.sh 
b/tools/testing/selftests/firmware/fw_lib.sh
new file mode 100755
index ..c14bbca7ecf9
--- /dev/null
+++ b/tools/testing/selftests/firmware/fw_lib.sh
@@ -0,0 +1,44 @@
+#!/bin/bash
+# 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
+}
-- 
2.16.2

[PATCH v3 04/20] 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 <mcg...@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.16.2

[PATCH v3 07/20] firmware: move loading timeout under struct firmware_fallback_config

[Luis R. Rodriguez]

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

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

Acked-by: Kees Cook <keesc...@chromium.org>
Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 drivers/base/firmware_loader.c | 46 ++
 1 file changed, 33 insertions(+), 13 deletions(-)

diff --git a/drivers/base/firmware_loader.c b/drivers/base/firmware_loader.c
index 2d819875348d..9757f9fff01e 100644
--- a/drivers/base/firmware_loader.c
+++ b/drivers/base/firmware_loader.c
@@ -266,21 +266,38 @@ static inline bool fw_state_is_aborted(struct fw_priv 
*fw_priv)
  *
  * @force_sysfs_fallback: force the sysfs fallback mechanism to be used
  * as if one had enabled CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y.
+ * @old_timeout: for internal use
+ * @loading_timeout: the timeout to wait for the fallback mechanism before
+ * giving up, in seconds.
  */
 struct firmware_fallback_config {
-   bool force_sysfs_fallback;
+   const bool force_sysfs_fallback;
+   int old_timeout;
+   int loading_timeout;
 };
 
-static const struct firmware_fallback_config fw_fallback_config = {
+static struct firmware_fallback_config fw_fallback_config = {
.force_sysfs_fallback = 
IS_ENABLED(CONFIG_FW_LOADER_USER_HELPER_FALLBACK),
+   .loading_timeout = 60,
+   .old_timeout = 60,
 };
 
-static int old_timeout;
-static int loading_timeout = 60;   /* In seconds */
+/* These getters are vetted to use int properly */
+static inline int __firmware_loading_timeout(void)
+{
+   return fw_fallback_config.loading_timeout;
+}
+
+/* These setters are vetted to use int properly */
+static void __fw_fallback_set_timeout(int timeout)
+{
+   fw_fallback_config.loading_timeout = timeout;
+}
 
 static inline long firmware_loading_timeout(void)
 {
-   return loading_timeout > 0 ? loading_timeout * HZ : MAX_JIFFY_OFFSET;
+   return __firmware_loading_timeout() > 0 ?
+   __firmware_loading_timeout() * HZ : MAX_JIFFY_OFFSET;
 }
 
 /*
@@ -291,14 +308,14 @@ static inline long firmware_loading_timeout(void)
  */
 static void fw_fallback_set_cache_timeout(void)
 {
-   old_timeout = loading_timeout;
-   loading_timeout = 10;
+   fw_fallback_config.old_timeout = __firmware_loading_timeout();
+   __fw_fallback_set_timeout(10);
 }
 
 /* Restores the timeout to the value last configured during normal operation */
 static void fw_fallback_set_default_timeout(void)
 {
-   loading_timeout =  old_timeout;
+   __fw_fallback_set_timeout(fw_fallback_config.old_timeout);
 }
 
 static inline bool fw_sysfs_done(struct fw_priv *fw_priv)
@@ -677,7 +694,7 @@ static void kill_pending_fw_fallback_reqs(bool 
only_kill_custom)
 static ssize_t timeout_show(struct class *class, struct class_attribute *attr,
char *buf)
 {
-   return sprintf(buf, "%d\n", loading_timeout);
+   return sprintf(buf, "%d\n", __firmware_loading_timeout());
 }
 
 /**
@@ -696,9 +713,12 @@ static ssize_t timeout_show(struct class *class, struct 
class_attribute *attr,
 static ssize_t timeout_store(struct class *class, struct class_attribute *attr,
 const char *buf, size_t count)
 {
-   loading_timeout = simple_strtol(buf, NULL, 10);
-   if (loading_timeout < 0)
-   loading_timeout = 0;
+   int tmp_loading_timeout = simple_strtol(buf, NULL, 10);
+
+   if (tmp_loading_timeout < 0)
+   tmp_loading_timeout = 0;
+
+   __fw_fallback_set_timeout(tmp_loading_timeout);
 
return count;
 }
@@ -721,7 +741,7 @@ static int do_firmware_uevent(struct fw_sysfs *fw_sysfs, 
struct kobj_uevent_env
 {
if (add_uevent_var(env, "FIRMWARE=%s", fw_sysfs->fw_priv->fw_name))
return -ENOMEM;
-   if (add_uevent_var(env, "TIMEOUT=%i", loading_timeout))
+   if (add_uevent_var(env, "TIMEOUT=%i", __firmware_loading_timeout()))
return -ENOMEM;
if (add_uevent_var(env, "ASYNC=%d", fw_sysfs->nowait))
return -ENOMEM;
-- 
2.16.2

[PATCH v3 06/20] firmware: use helpers for setting up a temporary cache timeout

[Luis R. Rodriguez]

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

This change introduces no functional changes.

Acked-by: Kees Cook <keesc...@chromium.org>
Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 drivers/base/firmware_loader.c | 49 ++
 1 file changed, 30 insertions(+), 19 deletions(-)

diff --git a/drivers/base/firmware_loader.c b/drivers/base/firmware_loader.c
index 59dba794ce1a..2d819875348d 100644
--- a/drivers/base/firmware_loader.c
+++ b/drivers/base/firmware_loader.c
@@ -191,13 +191,6 @@ static inline bool fw_is_builtin_firmware(const struct 
firmware *fw)
 }
 #endif
 
-static int loading_timeout = 60;   /* In seconds */
-
-static inline long firmware_loading_timeout(void)
-{
-   return loading_timeout > 0 ? loading_timeout * HZ : MAX_JIFFY_OFFSET;
-}
-
 static void fw_state_init(struct fw_priv *fw_priv)
 {
struct fw_state *fw_st = _priv->fw_st;
@@ -282,6 +275,32 @@ static const struct firmware_fallback_config 
fw_fallback_config = {
.force_sysfs_fallback = 
IS_ENABLED(CONFIG_FW_LOADER_USER_HELPER_FALLBACK),
 };
 
+static int old_timeout;
+static int loading_timeout = 60;   /* In seconds */
+
+static inline long firmware_loading_timeout(void)
+{
+   return loading_timeout > 0 ? loading_timeout * HZ : MAX_JIFFY_OFFSET;
+}
+
+/*
+ * use small loading timeout for caching devices' firmware because all these
+ * firmware images have been loaded successfully at lease once, also system is
+ * ready for completing firmware loading now. The maximum size of firmware in
+ * current distributions is about 2M bytes, so 10 secs should be enough.
+ */
+static void fw_fallback_set_cache_timeout(void)
+{
+   old_timeout = loading_timeout;
+   loading_timeout = 10;
+}
+
+/* Restores the timeout to the value last configured during normal operation */
+static void fw_fallback_set_default_timeout(void)
+{
+   loading_timeout =  old_timeout;
+}
+
 static inline bool fw_sysfs_done(struct fw_priv *fw_priv)
 {
return __fw_state_check(fw_priv, FW_STATUS_DONE);
@@ -1206,6 +1225,8 @@ static int fw_sysfs_fallback(struct firmware *fw, const 
char *name,
 }
 
 static inline void kill_pending_fw_fallback_reqs(bool only_kill_custom) { }
+static inline void fw_fallback_set_cache_timeout(void) { }
+static inline void fw_fallback_set_default_timeout(void) { }
 
 static inline int register_sysfs_loader(void)
 {
@@ -1752,7 +1773,6 @@ static void __device_uncache_fw_images(void)
 static void device_cache_fw_images(void)
 {
struct firmware_cache *fwc = _cache;
-   int old_timeout;
DEFINE_WAIT(wait);
 
pr_debug("%s\n", __func__);
@@ -1760,16 +1780,7 @@ static void device_cache_fw_images(void)
/* cancel uncache work */
cancel_delayed_work_sync(>work);
 
-   /*
-* use small loading timeout for caching devices' firmware
-* because all these firmware images have been loaded
-* successfully at lease once, also system is ready for
-* completing firmware loading now. The maximum size of
-* firmware in current distributions is about 2M bytes,
-* so 10 secs should be enough.
-*/
-   old_timeout = loading_timeout;
-   loading_timeout = 10;
+   fw_fallback_set_cache_timeout();
 
mutex_lock(_lock);
fwc->state = FW_LOADER_START_CACHE;
@@ -1779,7 +1790,7 @@ static void device_cache_fw_images(void)
/* wait for completion of caching firmware for all devices */
async_synchronize_full_domain(_cache_domain);
 
-   loading_timeout = old_timeout;
+   fw_fallback_set_default_timeout();
 }
 
 /**
-- 
2.16.2

[PATCH v3 05/20] firmware: simplify CONFIG_FW_LOADER_USER_HELPER_FALLBACK further

[Luis R. Rodriguez]

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

Acked-by: Kees Cook <keesc...@chromium.org>
Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 drivers/base/firmware_loader.c | 25 ++---
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/drivers/base/firmware_loader.c b/drivers/base/firmware_loader.c
index 7dd36ace6152..59dba794ce1a 100644
--- a/drivers/base/firmware_loader.c
+++ b/drivers/base/firmware_loader.c
@@ -266,6 +266,22 @@ static inline bool fw_state_is_aborted(struct fw_priv 
*fw_priv)
 
 #ifdef CONFIG_FW_LOADER_USER_HELPER
 
+/**
+ * struct firmware_fallback_config - firmware fallback configuratioon settings
+ *
+ * Helps describe and fine tune the fallback mechanism.
+ *
+ * @force_sysfs_fallback: force the sysfs fallback mechanism to be used
+ * as if one had enabled CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y.
+ */
+struct firmware_fallback_config {
+   bool force_sysfs_fallback;
+};
+
+static const struct firmware_fallback_config fw_fallback_config = {
+   .force_sysfs_fallback = 
IS_ENABLED(CONFIG_FW_LOADER_USER_HELPER_FALLBACK),
+};
+
 static inline bool fw_sysfs_done(struct fw_priv *fw_priv)
 {
return __fw_state_check(fw_priv, FW_STATUS_DONE);
@@ -1151,19 +1167,14 @@ static int fw_load_from_user_helper(struct firmware 
*firmware,
return ret;
 }
 
-#ifdef CONFIG_FW_LOADER_USER_HELPER_FALLBACK
-static bool fw_force_sysfs_fallback(unsigned int opt_flags)
-{
-   return true;
-}
-#else
 static bool fw_force_sysfs_fallback(unsigned int opt_flags)
 {
+   if (fw_fallback_config.force_sysfs_fallback)
+   return true;
if (!(opt_flags & FW_OPT_USERHELPER))
return false;
return true;
 }
-#endif
 
 static bool fw_run_sysfs_fallback(unsigned int opt_flags)
 {
-- 
2.16.2

[PATCH v3 08/20] firmware: split firmware fallback functionality into its own file

[Luis R. Rodriguez]

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

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

This introduces no functional changes.

Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 drivers/base/Makefile  |   4 +-
 drivers/base/firmware_fallback.c   | 661 +++
 drivers/base/firmware_fallback.h   |  61 +++
 drivers/base/firmware_fallback_table.c |  29 ++
 drivers/base/firmware_loader.c | 803 +
 drivers/base/firmware_loader.h | 115 +
 6 files changed, 874 insertions(+), 799 deletions(-)
 create mode 100644 drivers/base/firmware_fallback.c
 create mode 100644 drivers/base/firmware_fallback.h
 create mode 100644 drivers/base/firmware_fallback_table.c
 create mode 100644 drivers/base/firmware_loader.h

diff --git a/drivers/base/Makefile b/drivers/base/Makefile
index f261143fafbf..b946a408256d 100644
--- a/drivers/base/Makefile
+++ b/drivers/base/Makefile
@@ -5,7 +5,8 @@ obj-y   := component.o core.o bus.o dd.o 
syscore.o \
   driver.o class.o platform.o \
   cpu.o firmware.o init.o map.o devres.o \
   attribute_container.o transport_class.o \
-  topology.o container.o property.o cacheinfo.o
+  topology.o container.o property.o cacheinfo.o \
+  firmware_fallback_table.o
 obj-$(CONFIG_DEVTMPFS) += devtmpfs.o
 obj-$(CONFIG_DMA_CMA) += dma-contiguous.o
 obj-y  += power/
@@ -14,6 +15,7 @@ obj-$(CONFIG_HAVE_GENERIC_DMA_COHERENT) += dma-coherent.o
 obj-$(CONFIG_ISA_BUS_API)  += isa.o
 obj-$(CONFIG_FW_LOADER)+= firmware_class.o
 firmware_class-objs := firmware_loader.o
+firmware_class-$(CONFIG_FW_LOADER_USER_HELPER) += firmware_fallback.o
 obj-$(CONFIG_NUMA) += node.o
 obj-$(CONFIG_MEMORY_HOTPLUG_SPARSE) += memory.o
 ifeq ($(CONFIG_SYSFS),y)
diff --git a/drivers/base/firmware_fallback.c b/drivers/base/firmware_fallback.c
new file mode 100644
index ..47690207e0ee
--- /dev/null
+++ b/drivers/base/firmware_fallback.c
@@ -0,0 +1,661 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "firmware_fallback.h"
+#include "firmware_loader.h"
+
+/*
+ * firmware fallback mechanism
+ */
+
+extern struct firmware_fallback_config fw_fallback_config;
+
+/* These getters are vetted to use int properly */
+static inline int __firmware_loading_timeout(void)
+{
+   return fw_fallback_config.loading_timeout;
+}
+
+/* These setters are vetted to use int properly */
+static void __fw_fallback_set_timeout(int timeout)
+{
+   fw_fallback_config.loading_timeout = timeout;
+}
+
+/*
+ * use small loading timeout for caching devices' firmware because all these
+ * firmware images have been loaded successfully at lease once, also system is
+ * ready for completing firmware loading now. The maximum size of firmware in
+ * current distributions is about 2M bytes, so 10 secs should be enough.
+ */
+void fw_fallback_set_cache_timeout(void)
+{
+   fw_fallback_config.old_timeout = __firmware_loading_timeout();
+   __fw_fallback_set_timeout(10);
+}
+
+/* Restores the timeout to the value last configured during normal operation */
+void fw_fallback_set_default_timeout(void)
+{
+   __fw_fallback_set_timeout(fw_fallback_config.old_timeout);
+}
+
+static long firmware_loading_timeout(void)
+{
+   return __firmware_loading_timeout() > 0 ?
+   __firmware_loading_timeout() * HZ : MAX_JIFFY_OFFSET;
+}
+
+static inline bool fw_sysfs_done(struct fw_priv *fw_priv)
+{
+   return __fw_state_check(fw_priv, FW_STATUS_DONE);
+}
+
+static inline bool fw_sysfs_loading(struct fw_priv *fw_priv)
+{
+   return __fw_state_check(fw_priv, FW_STATUS_LOADING);
+}
+
+static inline int fw_sysfs_wait_timeout(struct fw_priv *fw_priv,  long timeout)
+{
+   return __fw_state_wait_common(fw_priv, timeout);
+}
+
+struct fw_sysfs {
+   bool nowait;
+   struct device dev;
+   struct fw_priv *fw_priv;
+   struct firmware *fw;
+};
+
+static struct fw_sysfs *to_fw_sysfs(struct device *dev)
+{
+   return container_of(dev, struct fw_sysfs, dev);
+}
+
+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_sysfs_done(fw_priv))
+   return;
+
+   list_del_init(_priv->pending

Re: [RFC 0/1] Loading optional firmware

[Luis R. Rodriguez]

On Fri, Mar 9, 2018 at 2:12 PM, Andres Rodriguez  wrote:
> Hi Everyone,
>
> Wanted to inquire your opinions about the following matter.
>
> We are experiencing some end user confusion regarding the following messages
> being printed to dmesg:
>
> [0.571324] amdgpu :01:00.0: Direct firmware load for 
> amdgpu/polaris10_pfp_2.bin failed with error -2
> [0.571338] amdgpu :01:00.0: Direct firmware load for 
> amdgpu/polaris10_me_2.bin failed with error -2
> [0.571348] amdgpu :01:00.0: Direct firmware load for 
> amdgpu/polaris10_ce_2.bin failed with error -2
> [0.571366] amdgpu :01:00.0: Direct firmware load for 
> amdgpu/polaris10_mec_2.bin failed with error -2
> [0.571404] amdgpu :01:00.0: Direct firmware load for 
> amdgpu/polaris10_mec2_2.bin failed with error -2
>
> These firmware blobs are optional. If they aren't available, the graphics card
> can still function normally. But having these messages causes the user to 
> think
> their current problems are related to missing firmware.
>
> It would be great if we could have a mechanism that enabled us to load a
> firmware blob without any dmesg spew in case of file not found 
> errors.Currently
> request_firmware_direct() implements this functionality, but as a drawback it
> forfeits the usermodehelper fallback path.

Yeah, this is a common enough reported type of feature request that I
think it makes sense to add now. I'll reply to your patch with a bit
more details.

 Luis

[PATCH v3 14/20] rename: _request_firmware_load() fw_load_sysfs_fallback()

[Luis R. Rodriguez]

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

Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 Documentation/driver-api/firmware/fallback-mechanisms.rst |  2 +-
 drivers/base/firmware_loader/fallback.c   | 13 ++---
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/Documentation/driver-api/firmware/fallback-mechanisms.rst 
b/Documentation/driver-api/firmware/fallback-mechanisms.rst
index 4055ac76b288..f353783ae0be 100644
--- a/Documentation/driver-api/firmware/fallback-mechanisms.rst
+++ b/Documentation/driver-api/firmware/fallback-mechanisms.rst
@@ -112,7 +112,7 @@ Since a device is created for the sysfs interface to help 
load firmware as a
 fallback mechanism userspace can be informed of the addition of the device by
 relying on kobject uevents. The addition of the device into the device
 hierarchy means the fallback mechanism for firmware loading has been initiated.
-For details of implementation refer to _request_firmware_load(), in particular
+For details of implementation refer to fw_load_sysfs_fallback(), in particular
 on the use of dev_set_uevent_suppress() and kobject_uevent().
 
 The kernel's kobject uevent mechanism is implemented in lib/kobject_uevent.c,
diff --git a/drivers/base/firmware_loader/fallback.c 
b/drivers/base/firmware_loader/fallback.c
index d6838e7ec00c..0a8ec7fec585 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -535,8 +535,15 @@ fw_create_instance(struct firmware *firmware, const char 
*fw_name,
return fw_sysfs;
 }
 
-/* load a firmware via user helper */
-static int _request_firmware_load(struct fw_sysfs *fw_sysfs,
+/**
+ * fw_load_sysfs_fallback - load a firmware via the syfs fallback mechanism
+ * @fw_sysfs: firmware syfs information for the firmware to load
+ * @opt_flags: flags of options, FW_OPT_*
+ * @timeout: timeout to wait for the load
+ *
+ * In charge of constructing a sysfs fallback interface for firmware loading.
+ **/
+static int fw_load_sysfs_fallback(struct fw_sysfs *fw_sysfs,
  unsigned int opt_flags, long timeout)
 {
int retval = 0;
@@ -621,7 +628,7 @@ static int fw_load_from_user_helper(struct firmware 
*firmware,
}
 
fw_sysfs->fw_priv = firmware->priv;
-   ret = _request_firmware_load(fw_sysfs, opt_flags, timeout);
+   ret = fw_load_sysfs_fallback(fw_sysfs, opt_flags, timeout);
 
if (!ret)
ret = assign_fw(firmware, device, opt_flags);
-- 
2.16.2

[PATCH v3 10/20] firmware: enable run time change of forcing fallback loader

[Luis R. Rodriguez]

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

0)
  CONFIG_FW_LOADER=y

1)
  o CONFIG_FW_LOADER=y
  o CONFIG_FW_LOADER_USER_HELPER=y

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

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

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

Acked-by: Kees Cook <keesc...@chromium.org>
Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 drivers/base/firmware_loader/fallback.c   |  1 +
 drivers/base/firmware_loader/fallback.h   |  4 +++-
 drivers/base/firmware_loader/fallback_table.c | 17 +
 kernel/sysctl.c   | 11 +++
 4 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/drivers/base/firmware_loader/fallback.c 
b/drivers/base/firmware_loader/fallback.c
index 9b65837256d6..45cc40933a47 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -7,6 +7,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "fallback.h"
 #include "firmware.h"
diff --git a/drivers/base/firmware_loader/fallback.h 
b/drivers/base/firmware_loader/fallback.h
index 550498c7fa4c..ca7e69a8417b 100644
--- a/drivers/base/firmware_loader/fallback.h
+++ b/drivers/base/firmware_loader/fallback.h
@@ -12,12 +12,14 @@
  *
  * @force_sysfs_fallback: force the sysfs fallback mechanism to be used
  * as if one had enabled CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y.
+ * Useful to help debug a CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y
+ * functionality on a kernel where that config entry has been disabled.
  * @old_timeout: for internal use
  * @loading_timeout: the timeout to wait for the fallback mechanism before
  * giving up, in seconds.
  */
 struct firmware_fallback_config {
-   const bool force_sysfs_fallback;
+   unsigned int force_sysfs_fallback;
int old_timeout;
int loading_timeout;
 };
diff --git a/drivers/base/firmware_loader/fallback_table.c 
b/drivers/base/firmware_loader/fallback_table.c
index 981419044c7e..92365e053e30 100644
--- a/drivers/base/firmware_loader/fallback_table.c
+++ b/drivers/base/firmware_loader/fallback_table.c
@@ -19,6 +19,9 @@
 /* Module or buit-in */
 #ifdef CONFIG_FW_LOADER_USER_HELPER
 
+static unsigned int zero;
+static unsigned int one = 1;
+
 struct firmware_fallback_config fw_fallback_config = {
.force_sysfs_fallback = 
IS_ENABLED(CONFIG_FW_LOADER_USER_HELPER_FALLBACK),
.loading_timeout = 60,
@@ -26,4 +29,18 @@ struct firmware_fallback_config fw_fallback_config = {
 };
 EXPORT_SYMBOL_GPL(fw_fallback_config);
 
+struct ctl_table firmware_config_table[] = {
+   {
+   .procname   = "force_sysfs_fallback",
+   .data   = _fallback_config.force_sysfs_fallback,
+   .maxlen = sizeof(unsigned int),
+   .mode   = 0644,
+   .proc_handler   = proc_douintvec_minmax,
+   .extra1 = ,
+   .extra2 = ,
+   },
+   { }
+};
+EXPORT_SYMBOL_GPL(firmware_config_table);
+
 #endif
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 6c68e771e11d..7a8aa6866764 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -253,6 +253,10 @@ extern struct ctl_table random_table[];
 extern struct ctl_table epoll_table[];
 #endif
 
+#ifdef CONFIG_FW_LOADER_USER_HELPER
+extern struct ctl_table firmware_config_table[];
+#endif
+
 #ifdef HAVE_ARCH_PICK_MMAP_LAYOUT
 int sysctl_legacy_va_layout;
 #endif
@@ -748,6 +752,13 @@ static struct ctl_table kern_table[] = {
.mode   = 0555,
.child  = usermodehelper_table,
},
+#ifdef CONFIG_FW_LOADER_USER_HELPER
+   {
+   .procname   = "firmware_config",
+   .mode   = 0555,
+   .child  = firmware_config_table,
+   },
+#endif
{
.procname   = "overflowuid",
.data   = ,
-- 
2.16.2

[PATCH v3 09/20] firmware: move firmware loader into its own directory

[Luis R. Rodriguez]

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

Suggested-by: Kees Cook <keesc...@chromium.org>
Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 MAINTAINERS   | 2 +-
 drivers/base/Makefile | 7 ++-
 drivers/base/firmware_loader/Makefile | 7 +++
 drivers/base/{firmware_fallback.c => firmware_loader/fallback.c}  | 4 ++--
 drivers/base/{firmware_fallback.h => firmware_loader/fallback.h}  | 0
 .../fallback_table.c} | 4 ++--
 drivers/base/{firmware_loader.h => firmware_loader/firmware.h}| 0
 drivers/base/{firmware_loader.c => firmware_loader/main.c}| 8 
 8 files changed, 18 insertions(+), 14 deletions(-)
 create mode 100644 drivers/base/firmware_loader/Makefile
 rename drivers/base/{firmware_fallback.c => firmware_loader/fallback.c} (99%)
 rename drivers/base/{firmware_fallback.h => firmware_loader/fallback.h} (100%)
 rename drivers/base/{firmware_fallback_table.c => 
firmware_loader/fallback_table.c} (90%)
 rename drivers/base/{firmware_loader.h => firmware_loader/firmware.h} (100%)
 rename drivers/base/{firmware_loader.c => firmware_loader/main.c} (99%)

diff --git a/MAINTAINERS b/MAINTAINERS
index e03a130902cd..6ddd6f4aaffa 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5589,7 +5589,7 @@ M:Luis R. Rodriguez <mcg...@kernel.org>
 L: linux-ker...@vger.kernel.org
 S: Maintained
 F: Documentation/firmware_class/
-F: drivers/base/firmware*.c
+F: drivers/base/firmware_loader/
 F: include/linux/firmware.h
 
 FLASH ADAPTER DRIVER (IBM Flash Adapter 900GB Full Height PCI Flash Card)
diff --git a/drivers/base/Makefile b/drivers/base/Makefile
index b946a408256d..b9539abec675 100644
--- a/drivers/base/Makefile
+++ b/drivers/base/Makefile
@@ -5,17 +5,14 @@ obj-y := component.o core.o bus.o dd.o 
syscore.o \
   driver.o class.o platform.o \
   cpu.o firmware.o init.o map.o devres.o \
   attribute_container.o transport_class.o \
-  topology.o container.o property.o cacheinfo.o \
-  firmware_fallback_table.o
+  topology.o container.o property.o cacheinfo.o
 obj-$(CONFIG_DEVTMPFS) += devtmpfs.o
 obj-$(CONFIG_DMA_CMA) += dma-contiguous.o
 obj-y  += power/
 obj-$(CONFIG_HAS_DMA)  += dma-mapping.o
 obj-$(CONFIG_HAVE_GENERIC_DMA_COHERENT) += dma-coherent.o
 obj-$(CONFIG_ISA_BUS_API)  += isa.o
-obj-$(CONFIG_FW_LOADER)+= firmware_class.o
-firmware_class-objs := firmware_loader.o
-firmware_class-$(CONFIG_FW_LOADER_USER_HELPER) += firmware_fallback.o
+obj-y  += firmware_loader/
 obj-$(CONFIG_NUMA) += node.o
 obj-$(CONFIG_MEMORY_HOTPLUG_SPARSE) += memory.o
 ifeq ($(CONFIG_SYSFS),y)
diff --git a/drivers/base/firmware_loader/Makefile 
b/drivers/base/firmware_loader/Makefile
new file mode 100644
index ..a97eeb0be1d8
--- /dev/null
+++ b/drivers/base/firmware_loader/Makefile
@@ -0,0 +1,7 @@
+# SPDX-License-Identifier: GPL-2.0
+# Makefile for the Linux firmware loader
+
+obj-y  := fallback_table.o
+obj-$(CONFIG_FW_LOADER)+= firmware_class.o
+firmware_class-objs := main.o
+firmware_class-$(CONFIG_FW_LOADER_USER_HELPER) += fallback.o
diff --git a/drivers/base/firmware_fallback.c 
b/drivers/base/firmware_loader/fallback.c
similarity index 99%
rename from drivers/base/firmware_fallback.c
rename to drivers/base/firmware_loader/fallback.c
index 47690207e0ee..9b65837256d6 100644
--- a/drivers/base/firmware_fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -8,8 +8,8 @@
 #include 
 #include 
 
-#include "firmware_fallback.h"
-#include "firmware_loader.h"
+#include "fallback.h"
+#include "firmware.h"
 
 /*
  * firmware fallback mechanism
diff --git a/drivers/base/firmware_fallback.h 
b/drivers/base/firmware_loader/fallback.h
similarity index 100%
rename from drivers/base/firmware_fallback.h
rename to drivers/base/firmware_loader/fallback.h
diff --git a/drivers/base/firmware_fallback_table.c 
b/drivers/base/firmware_loader/fallback_table.c
similarity index 90%
rename from drivers/base/firmware_fallback_table.c
rename to drivers/base/firmware_loader/fallback_table.c
index 53cc4e492520..981419044c7e 100644
--- a/drivers/base/firmware_fallback_table.c
+++ b/drivers/base/firmware_loader/fallback_table.c
@@ -9,8 +9,8 @@
 #include 
 #include 
 
-#include "firmware_fallback.h"
-#include "firmware_loader.h"
+#include "fallback.h"
+#include "firmware.h"
 
 /*
  * firmware fallback configuration table
diff --git a/drivers/base/firmwar

[PATCH v3 11/20] firmware: enable to force disable the fallback mechanism at run time

[Luis R. Rodriguez]

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

Acked-by: Kees Cook <keesc...@chromium.org>
Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 drivers/base/firmware_loader/fallback.c   | 5 +
 drivers/base/firmware_loader/fallback.h   | 4 
 drivers/base/firmware_loader/fallback_table.c | 9 +
 3 files changed, 18 insertions(+)

diff --git a/drivers/base/firmware_loader/fallback.c 
b/drivers/base/firmware_loader/fallback.c
index 45cc40933a47..d6838e7ec00c 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -643,6 +643,11 @@ static bool fw_force_sysfs_fallback(unsigned int opt_flags)
 
 static bool fw_run_sysfs_fallback(unsigned int opt_flags)
 {
+   if (fw_fallback_config.ignore_sysfs_fallback) {
+   pr_info_once("Ignoring firmware sysfs fallback due to debugfs 
knob\n");
+   return false;
+   }
+
if ((opt_flags & FW_OPT_NOFALLBACK))
return false;
 
diff --git a/drivers/base/firmware_loader/fallback.h 
b/drivers/base/firmware_loader/fallback.h
index ca7e69a8417b..dfebc644ed35 100644
--- a/drivers/base/firmware_loader/fallback.h
+++ b/drivers/base/firmware_loader/fallback.h
@@ -14,12 +14,16 @@
  * as if one had enabled CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y.
  * Useful to help debug a CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y
  * functionality on a kernel where that config entry has been disabled.
+ * @ignore_sysfs_fallback: force to disable the sysfs fallback mechanism.
+ * This emulates the behaviour as if we had set the kernel
+ * config CONFIG_FW_LOADER_USER_HELPER=n.
  * @old_timeout: for internal use
  * @loading_timeout: the timeout to wait for the fallback mechanism before
  * giving up, in seconds.
  */
 struct firmware_fallback_config {
unsigned int force_sysfs_fallback;
+   unsigned int ignore_sysfs_fallback;
int old_timeout;
int loading_timeout;
 };
diff --git a/drivers/base/firmware_loader/fallback_table.c 
b/drivers/base/firmware_loader/fallback_table.c
index 92365e053e30..7428659d8df9 100644
--- a/drivers/base/firmware_loader/fallback_table.c
+++ b/drivers/base/firmware_loader/fallback_table.c
@@ -39,6 +39,15 @@ struct ctl_table firmware_config_table[] = {
.extra1 = ,
.extra2 = ,
},
+   {
+   .procname   = "ignore_sysfs_fallback",
+   .data   = _fallback_config.ignore_sysfs_fallback,
+   .maxlen = sizeof(unsigned int),
+   .mode   = 0644,
+   .proc_handler   = proc_douintvec_minmax,
+   .extra1 = ,
+   .extra2 = ,
+   },
{ }
 };
 EXPORT_SYMBOL_GPL(firmware_config_table);
-- 
2.16.2

[PATCH v3 15/20] firmware: fix checking for return values for fw_add_devm_name()

[Luis R. Rodriguez]

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

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

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

Fixes: f531f05ae9437 ("firmware loader: store firmware name into devres list")
Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 drivers/base/firmware_loader/main.c | 12 +---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/base/firmware_loader/main.c 
b/drivers/base/firmware_loader/main.c
index c8966c84bd44..f5046887e362 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -403,7 +403,7 @@ static int fw_add_devm_name(struct device *dev, const char 
*name)
 
fwn = fw_find_devm_name(dev, name);
if (fwn)
-   return 1;
+   return 0;
 
fwn = devres_alloc(fw_name_devm_release, sizeof(struct fw_name_devm),
   GFP_KERNEL);
@@ -431,6 +431,7 @@ int assign_fw(struct firmware *fw, struct device *device,
  unsigned int opt_flags)
 {
struct fw_priv *fw_priv = fw->priv;
+   int ret;
 
mutex_lock(_lock);
if (!fw_priv->size || fw_state_is_aborted(fw_priv)) {
@@ -447,8 +448,13 @@ int assign_fw(struct firmware *fw, struct device *device,
 */
/* don't cache firmware handled without uevent */
if (device && (opt_flags & FW_OPT_UEVENT) &&
-   !(opt_flags & FW_OPT_NOCACHE))
-   fw_add_devm_name(device, fw_priv->fw_name);
+   !(opt_flags & FW_OPT_NOCACHE)) {
+   ret = fw_add_devm_name(device, fw_priv->fw_name);
+   if (ret) {
+   mutex_unlock(_lock);
+   return ret;
+   }
+   }
 
/*
 * After caching firmware image is started, let it piggyback
-- 
2.16.2

[PATCH v3 17/20] test_firmware: modify custom fallback tests to use unique files

[Luis R. Rodriguez]

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

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

Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 tools/testing/selftests/firmware/fw_fallback.sh   | 20 ++--
 tools/testing/selftests/firmware/fw_filesystem.sh | 11 +--
 tools/testing/selftests/firmware/fw_lib.sh| 23 +++
 3 files changed, 46 insertions(+), 8 deletions(-)

diff --git a/tools/testing/selftests/firmware/fw_fallback.sh 
b/tools/testing/selftests/firmware/fw_fallback.sh
index 9337a0328627..8e2e34a2ca69 100755
--- a/tools/testing/selftests/firmware/fw_fallback.sh
+++ b/tools/testing/selftests/firmware/fw_fallback.sh
@@ -238,8 +238,10 @@ run_sysfs_main_tests()
 
 run_sysfs_custom_load_tests()
 {
-   if load_fw_custom "$NAME" "$FW" ; then
-   if ! diff -q "$FW" /dev/test_firmware >/dev/null ; then
+   RANDOM_FILE_PATH=$(setup_random_file)
+   RANDOM_FILE="$(basename $RANDOM_FILE_PATH)"
+   if load_fw_custom "$RANDOM_FILE" "$RANDOM_FILE_PATH" ; then
+   if ! diff -q "$RANDOM_FILE_PATH" /dev/test_firmware >/dev/null 
; then
echo "$0: firmware was not loaded" >&2
exit 1
else
@@ -247,8 +249,10 @@ run_sysfs_custom_load_tests()
fi
fi
 
-   if load_fw_custom "$NAME" "$FW" ; then
-   if ! diff -q "$FW" /dev/test_firmware >/dev/null ; then
+   RANDOM_FILE_PATH=$(setup_random_file)
+   RANDOM_FILE="$(basename $RANDOM_FILE_PATH)"
+   if load_fw_custom "$RANDOM_FILE" "$RANDOM_FILE_PATH" ; then
+   if ! diff -q "$RANDOM_FILE_PATH" /dev/test_firmware >/dev/null 
; then
echo "$0: firmware was not loaded" >&2
exit 1
else
@@ -256,8 +260,12 @@ run_sysfs_custom_load_tests()
fi
fi
 
-   if load_fw_custom_cancel "nope-$NAME" "$FW" ; then
-   if diff -q "$FW" /dev/test_firmware >/dev/null ; then
+   RANDOM_FILE_REAL="$RANDOM_FILE_PATH"
+   FAKE_RANDOM_FILE_PATH=$(setup_random_file_fake)
+   FAKE_RANDOM_FILE="$(basename $FAKE_RANDOM_FILE_PATH)"
+
+   if load_fw_custom_cancel "$FAKE_RANDOM_FILE" "$RANDOM_FILE_REAL" ; then
+   if diff -q "$RANDOM_FILE_PATH" /dev/test_firmware >/dev/null ; 
then
echo "$0: firmware was expected to be cancelled" >&2
exit 1
else
diff --git a/tools/testing/selftests/firmware/fw_filesystem.sh 
b/tools/testing/selftests/firmware/fw_filesystem.sh
index 7f47877fa7fa..6452d2129cd9 100755
--- a/tools/testing/selftests/firmware/fw_filesystem.sh
+++ b/tools/testing/selftests/firmware/fw_filesystem.sh
@@ -230,10 +230,13 @@ test_wait_and_cancel_custom_load()
 test_request_firmware_nowait_custom_nofile()
 {
echo -n "Batched request_firmware_nowait(uevent=false) nofile try #$1: "
+   config_reset
config_unset_uevent
-   config_set_name nope-test-firmware.bin
+   RANDOM_FILE_PATH=$(setup_random_file_fake)
+   RANDOM_FILE="$(basename $RANDOM_FILE_PATH)"
+   config_set_name $RANDOM_FILE
config_trigger_async &
-   test_wait_and_cancel_custom_load nope-test-firmware.bin
+   test_wait_and_cancel_custom_load $RANDOM_FILE
wait
release_all_firmware
echo "OK"
@@ -271,7 +274,11 @@ test_request_firmware_nowait_uevent()
 test_request_firmware_nowait_custom()
 {
echo -n "Batched request_firmware_nowait(uevent=false) try #$1: "
+   config_reset
config_unset_uevent
+   RANDOM_FILE_PATH=$(setup_random_file)
+   RANDOM_FILE="$(basename $RANDOM_FILE_PATH)"
+   config_set_name $RANDOM_FILE
config_trigger_async
release_all_firmware
echo "OK"
diff --git a/tools/testing/selftests/firmware/fw_lib.sh 
b/tools/testing/selftests/firmware/fw_lib.sh
index 98dceb847ba0..9ea31b57d71a 100755
--- a/tools/testing/selftests/firmware/fw_lib.sh
+++ b/tools/testing/selftests/firmware/fw_lib.sh
@@ -104,6 +104,29 @@ setup_tmp_file()
fi
 }
 
+__setup_random_file()
+{
+   R

[PATCH v3 16/20] firmware: add helper to check to see if fw cache is setup

[Luis R. Rodriguez]

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

Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 drivers/base/firmware_loader/main.c | 14 --
 1 file changed, 12 insertions(+), 2 deletions(-)

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

[PATCH v3 18/20] firmware: ensure the firmware cache is not used on incompatible calls

[Luis R. Rodriguez]

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

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

Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 drivers/base/firmware_loader/main.c | 14 ++
 1 file changed, 14 insertions(+)

diff --git a/drivers/base/firmware_loader/main.c 
b/drivers/base/firmware_loader/main.c
index b569d8a09392..2913bb0e5e7b 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -431,6 +431,11 @@ static int fw_add_devm_name(struct device *dev, const char 
*name)
return 0;
 }
 #else
+static bool fw_cache_is_setup(struct device *dev, const char *name)
+{
+   return false;
+}
+
 static int fw_add_devm_name(struct device *dev, const char *name)
 {
return 0;
@@ -672,6 +677,9 @@ request_firmware_into_buf(const struct firmware 
**firmware_p, const char *name,
 {
int ret;
 
+   if (fw_cache_is_setup(device, name))
+   return -EOPNOTSUPP;
+
__module_get(THIS_MODULE);
ret = _request_firmware(firmware_p, name, device, buf, size,
FW_OPT_UEVENT | FW_OPT_NOCACHE);
@@ -769,6 +777,12 @@ request_firmware_nowait(
fw_work->opt_flags = FW_OPT_NOWAIT |
(uevent ? FW_OPT_UEVENT : FW_OPT_USERHELPER);
 
+   if (!uevent && fw_cache_is_setup(device, name)) {
+   kfree_const(fw_work->name);
+   kfree(fw_work);
+   return -EOPNOTSUPP;
+   }
+
if (!try_module_get(module)) {
kfree_const(fw_work->name);
kfree(fw_work);
-- 
2.16.2

[PATCH v3 13/20] test_firmware: test three firmware kernel configs using a proc knob

[Luis R. Rodriguez]

Since we now have knobs to twiddle what used to be set on kernel
configurations we can build one base kernel configuration and modify
behaviour to mimic such kernel configurations to test them.

Provided you build a kernel with:

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

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

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

Acked-by: Kees Cook <keesc...@chromium.org>
Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 tools/testing/selftests/firmware/Makefile|  2 +-
 tools/testing/selftests/firmware/fw_lib.sh   | 51 +
 tools/testing/selftests/firmware/fw_run_tests.sh | 70 
 3 files changed, 122 insertions(+), 1 deletion(-)
 create mode 100755 tools/testing/selftests/firmware/fw_run_tests.sh

diff --git a/tools/testing/selftests/firmware/Makefile 
b/tools/testing/selftests/firmware/Makefile
index 1894d625af2d..826f38d5dd19 100644
--- a/tools/testing/selftests/firmware/Makefile
+++ b/tools/testing/selftests/firmware/Makefile
@@ -3,7 +3,7 @@
 # No binaries, but make sure arg-less "make" doesn't trigger "run_tests"
 all:
 
-TEST_PROGS := fw_filesystem.sh fw_fallback.sh
+TEST_PROGS := fw_run_tests.sh
 
 include ../lib.mk
 
diff --git a/tools/testing/selftests/firmware/fw_lib.sh 
b/tools/testing/selftests/firmware/fw_lib.sh
index e3cc4483fdba..98dceb847ba0 100755
--- a/tools/testing/selftests/firmware/fw_lib.sh
+++ b/tools/testing/selftests/firmware/fw_lib.sh
@@ -47,6 +47,34 @@ check_setup()
 {
HAS_FW_LOADER_USER_HELPER="$(kconfig_has 
CONFIG_FW_LOADER_USER_HELPER=y)"
HAS_FW_LOADER_USER_HELPER_FALLBACK="$(kconfig_has 
CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y)"
+   PROC_FW_IGNORE_SYSFS_FALLBACK="0"
+   PROC_FW_FORCE_SYSFS_FALLBACK="0"
+
+   if [ -z $PROC_SYS_DIR ]; then
+   PROC_SYS_DIR="/proc/sys/kernel"
+   fi
+
+   FW_PROC="${PROC_SYS_DIR}/firmware_config"
+   FW_FORCE_SYSFS_FALLBACK="$FW_PROC/force_sysfs_fallback"
+   FW_IGNORE_SYSFS_FALLBACK="$FW_PROC/ignore_sysfs_fallback"
+
+   if [ -f $FW_FORCE_SYSFS_FALLBACK ]; then
+   PROC_FW_FORCE_SYSFS_FALLBACK="$(cat $FW_FORCE_SYSFS_FALLBACK)"
+   fi
+
+   if [ -f $FW_IGNORE_SYSFS_FALLBACK ]; then
+   PROC_FW_IGNORE_SYSFS_FALLBACK="$(cat $FW_IGNORE_SYSFS_FALLBACK)"
+   fi
+
+   if [ "$PROC_FW_FORCE_SYSFS_FALLBACK" = "1" ]; then
+   HAS_FW_LOADER_USER_HELPER="yes"
+   HAS_FW_LOADER_USER_HELPER_FALLBACK="yes"
+   fi
+
+   if [ "$PROC_FW_IGNORE_SYSFS_FALLBACK" = "1" ]; then
+   HAS_FW_LOADER_USER_HELPER_FALLBACK="no"
+   HAS_FW_LOADER_USER_HELPER="no"
+   fi
 
if [ "$HAS_FW_LOADER_USER_HELPER" = "yes" ]; then
   OLD_TIMEOUT="$(cat /sys/class/firmware/timeout)"
@@ -76,6 +104,28 @@ setup_tmp_file()
fi
 }
 
+proc_set_force_sysfs_fallback()
+{
+   if [ -f $FW_FORCE_SYSFS_FALLBACK ]; then
+   echo -n $1 > $FW_FORCE_SYSFS_FALLBACK
+   check_setup
+   fi
+}
+
+proc_set_ignore_sysfs_fallback()
+{
+   if [ -f $FW_IGNORE_SYSFS_FALLBACK ]; then
+   echo -n $1 > $FW_IGNORE_SYSFS_FALLBACK
+   check_setup
+   fi
+}
+
+proc_restore_defaults()
+{
+   proc_set_force_sysfs_fallback 0
+   proc_set_ignore_sysfs_fallback 0
+}
+
 test_finish()
 {
if [ "$HAS_FW_LOADER_USER_HELPER" = "yes" ]; then
@@ -93,6 +143,7 @@ test_finish()
if [ -d $FWPATH ]; then
rm -rf "$FWPATH"
fi
+   proc_restore_defaults
 }
 
 kconfig_has()
diff --git a/tools/testing/selftests/firmware/fw_run_tests.sh 
b/tools/testing/selftests/firmware/fw_run_tests.sh
new file mode 100755
index ..06d638e9dc62
--- /dev/null
+++ b/tools/testing/selftests/firmware/fw_run_tests.sh
@@ -0,0 +1,70 @@
+#!/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
+
+export HAS_FW_LOADER_USER_HELPER=""
+export HAS_FW_LOADER_USER_HELPER_FALLBACK=""
+
+run_tests()
+{
+   proc_set_force_sysfs_fallback $1
+   proc_set_ig

[PATCH v3 19/20] firmware: add request_firmware_cache() to help with cache on reboot

[Luis R. Rodriguez]

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

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

Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 .../driver-api/firmware/request_firmware.rst   | 14 +
 drivers/base/firmware_loader/main.c| 24 ++
 include/linux/firmware.h   |  3 +++
 3 files changed, 41 insertions(+)

diff --git a/Documentation/driver-api/firmware/request_firmware.rst 
b/Documentation/driver-api/firmware/request_firmware.rst
index cc0aea880824..b554f4338859 100644
--- a/Documentation/driver-api/firmware/request_firmware.rst
+++ b/Documentation/driver-api/firmware/request_firmware.rst
@@ -44,6 +44,20 @@ request_firmware_nowait
 .. kernel-doc:: drivers/base/firmware_class.c
:functions: request_firmware_nowait
 
+Special optimizations on reboot
+===
+
+Some devices have an optimization in place to enable the firmware to be
+retained during system reboot. When such optimizations are used the driver
+author must ensure the firmware is still available on resume from suspend,
+this can be done with request_firmware_cache() insted of requesting for the
+firmare to be loaded.
+
+request_firmware_cache()
+---
+.. kernel-doc:: drivers/base/firmware_class.c
+   :functions: request_firmware_load
+
 request firmware API expected driver use
 
 
diff --git a/drivers/base/firmware_loader/main.c 
b/drivers/base/firmware_loader/main.c
index 2913bb0e5e7b..04bf853f60e9 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -656,6 +656,30 @@ int request_firmware_direct(const struct firmware 
**firmware_p,
 }
 EXPORT_SYMBOL_GPL(request_firmware_direct);
 
+/**
+ * request_firmware_cache: - cache firmware for suspend so resume can use it
+ * @name: name of firmware file
+ * @device: device for which firmware should be cached for
+ *
+ * There are some devices with an optimization that enables the device to not
+ * require loading firmware on system reboot. This optimization may still
+ * require the firmware present on resume from suspend. This routine can be
+ * used to ensure the firmware is present on resume from suspend in these
+ * situations. This helper is not compatible with drivers which use
+ * request_firmware_into_buf() or request_firmware_nowait() with no uevent set.
+ **/
+int request_firmware_cache(struct device *device, const char *name)
+{
+   int ret;
+
+   mutex_lock(_lock);
+   ret = fw_add_devm_name(device, name);
+   mutex_unlock(_lock);
+
+   return ret;
+}
+EXPORT_SYMBOL_GPL(request_firmware_cache);
+
 /**
  * request_firmware_into_buf - load firmware into a previously allocated buffer
  * @firmware_p: pointer to firmware image
diff --git a/include/linux/firmware.h b/include/linux/firmware.h
index d4508080348d..166867910ebc 100644
--- a/include/linux/firmware.h
+++ b/include/linux/firmware.h
@@ -85,4 +85,7 @@ static inline int request_firmware_into_buf(const struct 
firmware **firmware_p,
 }
 
 #endif
+
+int request_firmware_cache(struct device *device, const char *name);
+
 #endif
-- 
2.16.2

[PATCH v3 20/20] mt7601u: use request_firmware_cache() to address cache on reboot

[Luis R. Rodriguez]

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

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

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

Reported-by: Cantabile <cantabile.d...@gmail.com>
Tested-by: Cantabile <cantabile.d...@gmail.com>
Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 drivers/net/wireless/mediatek/mt7601u/mcu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/mediatek/mt7601u/mcu.c 
b/drivers/net/wireless/mediatek/mt7601u/mcu.c
index 65a8004418ea..b90456a4b4d7 100644
--- a/drivers/net/wireless/mediatek/mt7601u/mcu.c
+++ b/drivers/net/wireless/mediatek/mt7601u/mcu.c
@@ -421,7 +421,7 @@ static int mt7601u_load_firmware(struct mt7601u_dev *dev)
 MT_USB_DMA_CFG_TX_BULK_EN));
 
if (firmware_running(dev))
-   return 0;
+   return request_firmware_cache(dev->dev, MT7601U_FIRMWARE);
 
ret = request_firmware(, MT7601U_FIRMWARE, dev->dev);
if (ret)
-- 
2.16.2

[PATCH v3 12/20] test_firmware: expand on library with shared helpers

[Luis R. Rodriguez]

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

Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 tools/testing/selftests/firmware/fw_fallback.sh   | 31 +++---
 tools/testing/selftests/firmware/fw_filesystem.sh | 41 +++---
 tools/testing/selftests/firmware/fw_lib.sh| 52 +++
 3 files changed, 63 insertions(+), 61 deletions(-)

diff --git a/tools/testing/selftests/firmware/fw_fallback.sh 
b/tools/testing/selftests/firmware/fw_fallback.sh
index 323c921a2469..9337a0328627 100755
--- a/tools/testing/selftests/firmware/fw_fallback.sh
+++ b/tools/testing/selftests/firmware/fw_fallback.sh
@@ -6,30 +6,17 @@
 # won't find so that we can do the load ourself manually.
 set -e
 
+TEST_REQS_FW_SYSFS_FALLBACK="yes"
+TEST_REQS_FW_SET_CUSTOM_PATH="no"
 TEST_DIR=$(dirname $0)
 source $TEST_DIR/fw_lib.sh
 
 check_mods
+check_setup
+verify_reqs
+setup_tmp_file
 
-HAS_FW_LOADER_USER_HELPER=$(kconfig_has CONFIG_FW_LOADER_USER_HELPER=y)
-HAS_FW_LOADER_USER_HELPER_FALLBACK=$(kconfig_has 
CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y)
-
-if [ "$HAS_FW_LOADER_USER_HELPER" = "yes" ]; then
-   OLD_TIMEOUT=$(cat /sys/class/firmware/timeout)
-else
-   echo "usermode helper disabled so ignoring test"
-   exit 0
-fi
-
-FWPATH=$(mktemp -d)
-FW="$FWPATH/test-firmware.bin"
-
-test_finish()
-{
-   echo "$OLD_TIMEOUT" >/sys/class/firmware/timeout
-   rm -f "$FW"
-   rmdir "$FWPATH"
-}
+trap "test_finish" EXIT
 
 load_fw()
 {
@@ -168,12 +155,6 @@ load_fw_fallback_with_child()
return $RET
 }
 
-trap "test_finish" EXIT
-
-# This is an unlikely real-world firmware content. :)
-echo "ABCD0123" >"$FW"
-NAME=$(basename "$FW")
-
 test_syfs_timeout()
 {
DEVPATH="$DIR"/"nope-$NAME"/loading
diff --git a/tools/testing/selftests/firmware/fw_filesystem.sh 
b/tools/testing/selftests/firmware/fw_filesystem.sh
index 748f2f5737e9..7f47877fa7fa 100755
--- a/tools/testing/selftests/firmware/fw_filesystem.sh
+++ b/tools/testing/selftests/firmware/fw_filesystem.sh
@@ -6,38 +6,15 @@
 # know so we can be sure we're not accidentally testing the user helper.
 set -e
 
+TEST_REQS_FW_SYSFS_FALLBACK="no"
+TEST_REQS_FW_SET_CUSTOM_PATH="yes"
 TEST_DIR=$(dirname $0)
 source $TEST_DIR/fw_lib.sh
 
 check_mods
-
-# CONFIG_FW_LOADER_USER_HELPER has a sysfs class under /sys/class/firmware/
-# These days most distros enable CONFIG_FW_LOADER_USER_HELPER but disable
-# 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_setup
+verify_reqs
+setup_tmp_file
 
 trap "test_finish" EXIT
 
@@ -46,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
index 467567c758b9..e3cc4483fdba 100755
--- a/tools/testing/selftests/firmware/fw_lib.sh
+++ b/tools/testing/selftests/firmware/fw_lib.sh
@@ -43,6 +43,58 @@ check_mods()
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)"
+

Re: [PATCH] mt7601u: Fix system freeze after resuming from hibernation

[Luis R. Rodriguez]

On Thu, Mar 01, 2018 at 10:11:01PM +0200, cantabile wrote:
> On 01/03/18 19:29, Luis R. Rodriguez wrote:
> > 
> > Correct?
> > 
> 
> The above log is from "20180227-firmware-cache" kernel plus your suggested
> change that makes it always upload the firmware:
> 
> diff --git a/drivers/net/wireless/mediatek/mt7601u/mcu.c
> b/drivers/net/wireless/mediatek/mt7601u/mcu.c
> index 65a8004418ea..04cbffd225a1 100644
> --- a/drivers/net/wireless/mediatek/mt7601u/mcu.c
> +++ b/drivers/net/wireless/mediatek/mt7601u/mcu.c
> @@ -421,7 +421,7 @@ static int mt7601u_load_firmware(struct mt7601u_dev
> *dev)
>MT_USB_DMA_CFG_TX_BULK_EN));
> 
>   if (firmware_running(dev))
> - return 0;
> + pr_info("Firmware already loaded but going to reload...");
> 
>   ret = request_firmware(, MT7601U_FIRMWARE, dev->dev);
>   if (ret)

Huh, if you are using 20180227-firmware-cache, then your diff should instead
be this no?

diff --git a/drivers/net/wireless/mediatek/mt7601u/mcu.c 
b/drivers/net/wireless/mediatek/mt7601u/mcu.c
index b90456a4b4d7..04cbffd225a1 100644
--- a/drivers/net/wireless/mediatek/mt7601u/mcu.c
+++ b/drivers/net/wireless/mediatek/mt7601u/mcu.c
@@ -421,7 +421,7 @@ static int mt7601u_load_firmware(struct mt7601u_dev *dev)
 MT_USB_DMA_CFG_TX_BULK_EN));
 
if (firmware_running(dev))
-   return request_firmware_cache(dev->dev, MT7601U_FIRMWARE);
+   pr_info("Firmware already loaded but going to reload...");
 
ret = request_firmware(, MT7601U_FIRMWARE, dev->dev);
if (ret)


> I recompiled the kernel with CONFIG_DEBUG_INFO=y, but the messages in
> journalctl look pretty much the same as before. There are still question
> marks in the call traces, there are no line numbers, and it still says
> "verbose debug info unavailable". /proc/config.gz contains
> CONFIG_DEBUG_INFO=y. What else do I need to do?

Hrm, odd.

CONFIG_FRAME_POINTER=y

Maybe.

> > But if the patches I posted help, I guess we already have a solution, The 
> > rest of
> > these email exchanges are all just hypothetical exercises.
> > 
> 
> Your patches definitely fix the problem I reported in my initial email.

Ah OK :)

  Luis

Re: [PATCH] mt7601u: Fix system freeze after resuming from hibernation

[Luis R. Rodriguez]

On Thu, Mar 01, 2018 at 04:05:29PM +0200, cantabile wrote:
> On 01/03/18 02:28, Luis R. Rodriguez wrote:
> > On Wed, Feb 28, 2018 at 11:18:59PM +0200, cantabile wrote:
> > 
> > > Feb 28 22:46:19 home kernel: mt7601u 2-3:1.0: Firmware Version: 0.1.00 
> > > Build: 7640 Build time: 201302052146
> > > Feb 28 22:46:19 home kernel: ata1: SATA link up 3.0 Gbps (SStatus 123 
> > > SControl 300)
> > > Feb 28 22:46:19 home kernel: ata1.00: ACPI cmd f5/00:00:00:00:00:a0 
> > > (SECURITY FREEZE LOCK) filtered out
> > > Feb 28 22:46:19 home kernel: ata1.00: ACPI cmd b1/c1:00:00:00:00:a0 
> > > (DEVICE CONFIGURATION OVERLAY) filtered out
> > > Feb 28 22:46:19 home kernel: ata1.00: ACPI cmd c6/00:01:00:00:00:a0 (SET 
> > > MULTIPLE MODE) succeeded
> > > Feb 28 22:46:19 home kernel: ata1.00: ACPI cmd ef/10:03:00:00:00:a0 (SET 
> > > FEATURES) filtered out
> > > Feb 28 22:46:19 home kernel: ata1.00: supports DRM functions and may not 
> > > be fully accessible
> > > Feb 28 22:46:19 home kernel: ata1.00: disabling queued TRIM support
> > > Feb 28 22:46:19 home kernel: ata1.00: ACPI cmd f5/00:00:00:00:00:a0 
> > > (SECURITY FREEZE LOCK) filtered out
> > > Feb 28 22:46:19 home kernel: ata1.00: ACPI cmd b1/c1:00:00:00:00:a0 
> > > (DEVICE CONFIGURATION OVERLAY) filtered out
> > > Feb 28 22:46:19 home kernel: ata1.00: ACPI cmd c6/00:01:00:00:00:a0 (SET 
> > > MULTIPLE MODE) succeeded
> > > Feb 28 22:46:19 home kernel: ata1.00: ACPI cmd ef/10:03:00:00:00:a0 (SET 
> > > FEATURES) filtered out
> > > Feb 28 22:46:19 home kernel: ata1.00: supports DRM functions and may not 
> > > be fully accessible
> > > Feb 28 22:46:19 home kernel: ata1.00: disabling queued TRIM support
> > > Feb 28 22:46:19 home kernel: ata1.00: configured for UDMA/133
> > > Feb 28 22:46:19 home kernel: usb 4-2: reset full-speed USB device number 
> > > 2 using uhci_hcd
> > > Feb 28 22:46:19 home kernel: mt7601u 2-3:1.0: Error: MCU response 
> > > pre-completed!
> > 
> > An issue immediately. So it should mean the firmware load didn't likely work
> > correctly.
> 
> This particular error message appears pretty often after resuming from
> hibernation and loading the firmware, but the device works anyway. 

OK.

> To be
> clear, I'm talking about hibernation attempts from before I made it upload
> the firmware unconditionally.

So you mean to say this happens with the stock kernel (except the debug prints
you added)?

That is, you didn't force to upload the firmware on the above log.

Also, the above log is not from a kernel with the patches I posted to fix the
issue and issue a cache request when the device already has a firmware already
loaded?

Correct?

> > > Feb 28 22:46:19 home systemd[1]: Started Suspend.
> > 
> > Oh another suspend? Or is this just noise from systemd?
> 
> It's just noise. This whole excerpt from journalctl includes only one
> suspend/resume.

Ah, so you really are kicking into high gear suspend/resume or 
hibernation/resume.
Note that there may be some fun races there with the driver perhaps. Who knows.

> > > Feb 28 22:46:20 home kernel: mt7601u 2-3:1.0: Error: MCU response 
> > > pre-completed!
> > 
> > Either way the 802.11 device is barfing.
> > 
> > > Feb 28 22:46:20 home kernel: [ cut here ]
> > > Feb 28 22:46:20 home kernel: Kernel BUG at 9312bc4c [verbose 
> > > debug info unavailable]
> > 
> > And since no debug info is available we ca't do much more to look at this
> > issue. You can recompile with debugging symbols enabled (look online), and
> > it may help further.
> > 
> 
> If I must... :/

To get clear logs you must.

But if the patches I posted help, I guess we already have a solution, The rest 
of
these email exchanges are all just hypothetical exercises.

  Luis

Re: [PATCH] mt7601u: Fix system freeze after resuming from hibernation

[Luis R. Rodriguez]

On Wed, Feb 28, 2018 at 11:18:59PM +0200, cantabile wrote:
> On 28/02/18 20:48, Luis R. Rodriguez wrote:
> > On Wed, Feb 28, 2018 at 08:02:59PM +0200, cantabile wrote:
> > > On 27/02/18 22:42, Luis R. Rodriguez wrote:
> > OK so we know that the optimization is optional, not a requirement.
> > That may be worth extending in documentation on the driver.
> 
> I may have spoken too soon. mt7601u kind of exploded after resuming from
> suspend to RAM. Maybe it has nothing to do with this change, but well. The
> system froze after the KDE lock screen appeared, and a few minutes later the
> Caps Lock LED turned on. I attached journalctl output in case it's
> interesting.

Oh well, the optimization is not optional then :P

> Feb 28 22:46:04 home kernel: PM: suspend entry (deep)
> Feb 28 22:46:04 home plasmashell[576]: QXcbClipboard: SelectionRequest too old
> Feb 28 22:46:04 home ksmserver[552]: CreateNotify: 60817442
> Feb 28 22:46:05 home kernel: PM: Syncing filesystems ... done.
> Feb 28 22:46:05 home kernel: firmware_class: device_cache_fw_images
> Feb 28 22:46:05 home kernel: firmware_class: cache_firmware: 
> iwlwifi-3945-2.ucode
> Feb 28 22:46:05 home kernel: firmware_class: __allocate_fw_priv: 
> fw-iwlwifi-3945-2.ucode fw_priv=eb639dd2
> Feb 28 22:46:05 home kernel: firmware_class: cache_firmware: mt7601u.bin
> Feb 28 22:46:05 home kernel: firmware_class: __allocate_fw_priv: 
> fw-mt7601u.bin fw_priv=4f840abe
> Feb 28 22:46:05 home kernel: (NULL device *): loading 
> /lib/firmware/updates/4.16.0-rc3-g4edf7856bed8/iwlwifi-3945-2.ucode failed 
> with error -2
> Feb 28 22:46:19 home kernel: (NULL device *): loading 
> /lib/firmware/updates/iwlwifi-3945-2.ucode failed with error -2
> Feb 28 22:46:19 home kernel: (NULL device *): loading 
> /lib/firmware/updates/4.16.0-rc3-g4edf7856bed8/mt7601u.bin failed with error 
> -2
> Feb 28 22:46:19 home kernel: (NULL device *): loading 
> /lib/firmware/4.16.0-rc3-g4edf7856bed8/iwlwifi-3945-2.ucode failed with error 
> -2
> Feb 28 22:46:19 home kernel: (NULL device *): loading 
> /lib/firmware/updates/mt7601u.bin failed with error -2
> Feb 28 22:46:19 home kernel: (NULL device *): loading 
> /lib/firmware/4.16.0-rc3-g4edf7856bed8/mt7601u.bin failed with error -2
> Feb 28 22:46:19 home kernel: (NULL device *): direct-loading mt7601u.bin
> Feb 28 22:46:19 home kernel: firmware_class: fw_set_page_data: fw-mt7601u.bin 
> fw_priv=4f840abe data=991ae8e7 size=45412
> Feb 28 22:46:19 home kernel: firmware_class: cache_firmware: mt7601u.bin ret=0
> Feb 28 22:46:19 home kernel: (NULL device *): direct-loading 
> iwlwifi-3945-2.ucode
> Feb 28 22:46:19 home kernel: firmware_class: fw_set_page_data: 
> fw-iwlwifi-3945-2.ucode fw_priv=eb639dd2 data=9d96fe1e 
> size=150100
> Feb 28 22:46:19 home kernel: firmware_class: cache_firmware: 
> iwlwifi-3945-2.ucode ret=0
> Feb 28 22:46:19 home kernel: Freezing user space processes ... (elapsed 0.002 
> seconds) done.
> Feb 28 22:46:19 home kernel: OOM killer disabled.
> Feb 28 22:46:19 home kernel: Freezing remaining freezable tasks ... (elapsed 
> 0.001 seconds) done.
> Feb 28 22:46:19 home kernel: Suspending console(s) (use no_console_suspend to 
> debug)
> Feb 28 22:46:19 home kernel: sd 0:0:0:0: [sda] Synchronizing SCSI cache
> Feb 28 22:46:19 home kernel: sd 0:0:0:0: [sda] Stopping disk
> Feb 28 22:46:19 home kernel: e1000e: EEE TX LPI TIMER: 
> Feb 28 22:46:19 home kernel: ACPI: EC: interrupt blocked
> Feb 28 22:46:19 home kernel: ACPI: Preparing to enter system sleep state S3
> Feb 28 22:46:19 home kernel: ACPI: EC: event blocked
> Feb 28 22:46:19 home kernel: ACPI: EC: EC stopped
> Feb 28 22:46:19 home kernel: PM: Saving platform NVS memory
> Feb 28 22:46:19 home kernel: Disabling non-boot CPUs ...
> Feb 28 22:46:19 home kernel: smpboot: CPU 1 is now offline
> Feb 28 22:46:19 home kernel: ACPI: Low-level resume complete
> Feb 28 22:46:19 home kernel: ACPI: EC: EC started

Ok so suspend..


> Feb 28 22:46:19 home kernel: PM: Restoring platform NVS memory
> Feb 28 22:46:19 home kernel: Enabling non-boot CPUs ...
> Feb 28 22:46:19 home kernel: x86: Booting SMP configuration:
> Feb 28 22:46:19 home kernel: smpboot: Booting Node 0 Processor 1 APIC 0x1
> Feb 28 22:46:19 home kernel:  cache: parent cpu1 should not be sleeping
> Feb 28 22:46:19 home kernel: microcode: sig=0x6fd, pf=0x80, revision=0xa3
> Feb 28 22:46:19 home kernel: microcode: updated to revision 0xa4, date = 
> 2010-10-02
> Feb 28 22:46:19 home kernel: CPU1 is up
> Feb 28 22:46:19 home kernel: ACPI: Waking up from system sleep state S3
> Feb 28 22:46:19 home kernel: ACPI: EC: interrupt unblocked
> Feb 28 22:46:19 home kernel: ACPI: EC: event unblocked
> Feb 28 22:46: