[PATCH] firmware: change kernel read fail to dev_dbg()

[PATCH] firmware: change kernel read fail to dev_dbg()

[Luis R. Rodriguez]

When we now use the new kernel_read_file_from_path() we
are reporting a failure when we iterate over all the paths
possible for firmware. Before using kernel_read_file_from_path()
we only reported a failure once we confirmed a file existed
with filp_open() but failed with fw_read_file_contents().

With kernel_read_file_from_path() both are done for us and
we obviously are now reporting too much information given that
some optional paths will always fail and clutter the logs.

fw_get_filesystem_firmware() already has a check for failure
and uses an internal flag, FW_OPT_NO_WARN, to let users
warn or not warn. For instance request_firmware_direct()
does not warn as this can be used for optional firmware
as it has no usermode helper fallback. In the future we
may want to change this, given everyone is disabling the
usermode helper anyway now, but for now keep reporting
only as was designed. request_firmware_direct() will
continue to not report errors as it was designed not to.

Reported-by: Heiner Kallweit <hkallweit1@gmail.com>
Cc: Heiner Kallweit <hkallweit1@gmail.com>
Cc: Mimi Zohar <zohar@linux.vnet.ibm.com>
Cc: Kees Cook <keescook@chromium.org>
Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---

Note, this is a fix for a small issue reported by Heiner after
Mimi's common kernel file loader changes were merged on James tree.
I'm following up through James. If this should go through someone
else's tree feel free to coordiante accordingly.

 drivers/base/firmware_class.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 1cff832ab74e..b1cf4d61ffc9 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -328,7 +328,7 @@ static int fw_get_filesystem_firmware(struct device *device,
 		rc = kernel_read_file_from_path(path, &buf->data, &size,
 						INT_MAX, READING_FIRMWARE);
 		if (rc) {
-			dev_warn(device, "loading %s failed with error %d\n",
+			dev_dbg(device, "loading %s failed with error %d\n",
 				 path, rc);
 			continue;
 		}
-- 
2.7.0

Re: [PATCH] firmware: change kernel read fail to dev_dbg()

[Luis R. Rodriguez]

On Sat, Feb 27, 2016 at 03:16:03PM -0800, Luis R. Rodriguez wrote:
> When we now use the new kernel_read_file_from_path() we
> are reporting a failure when we iterate over all the paths
> possible for firmware. Before using kernel_read_file_from_path()
> we only reported a failure once we confirmed a file existed
> with filp_open() but failed with fw_read_file_contents().
> 
> With kernel_read_file_from_path() both are done for us and
> we obviously are now reporting too much information given that
> some optional paths will always fail and clutter the logs.
> 
> fw_get_filesystem_firmware() already has a check for failure
> and uses an internal flag, FW_OPT_NO_WARN, to let users
> warn or not warn. For instance request_firmware_direct()
> does not warn as this can be used for optional firmware
> as it has no usermode helper fallback. In the future we
> may want to change this, given everyone is disabling the
> usermode helper anyway now, but for now keep reporting
> only as was designed. request_firmware_direct() will
> continue to not report errors as it was designed not to.
> 
> Reported-by: Heiner Kallweit <hkallweit1@gmail.com>
> Cc: Heiner Kallweit <hkallweit1@gmail.com>
> Cc: Mimi Zohar <zohar@linux.vnet.ibm.com>
> Cc: Kees Cook <keescook@chromium.org>
> Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
> ---
> 
> Note, this is a fix for a small issue reported by Heiner after
> Mimi's common kernel file loader changes were merged on James tree.
> I'm following up through James. If this should go through someone
> else's tree feel free to coordiante accordingly.

I'll be replacing this shortly with a v2.

  Luis

[PATCH v2] firmware: change kernel read fail to dev_dbg()

[Luis R. Rodriguez]

When we now use the new kernel_read_file_from_path() we
are reporting a failure when we iterate over all the paths
possible for firmware. Before using kernel_read_file_from_path()
we only reported a failure once we confirmed a file existed
with filp_open() but failed with fw_read_file_contents().

With kernel_read_file_from_path() both are done for us and
we obviously are now reporting too much information given that
some optional paths will always fail and clutter the logs.

fw_get_filesystem_firmware() already has a check for failure
and uses an internal flag, FW_OPT_NO_WARN, but this does not
let us capture other unxpected errors. This enables that
as changed by Neil via commit:

"firmware: Be a bit more verbose about direct firmware loading failure"

Reported-by: Heiner Kallweit <hkallweit1@gmail.com>
Cc: Neil Horman <nhorman@tuxdriver.com>
Cc: Heiner Kallweit <hkallweit1@gmail.com>
Cc: Mimi Zohar <zohar@linux.vnet.ibm.com>
Cc: Kees Cook <keescook@chromium.org>
Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 drivers/base/firmware_class.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 1cff832ab74e..9503a88b189b 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -328,8 +328,12 @@ static int fw_get_filesystem_firmware(struct device *device,
 		rc = kernel_read_file_from_path(path, &buf->data, &size,
 						INT_MAX, READING_FIRMWARE);
 		if (rc) {
-			dev_warn(device, "loading %s failed with error %d\n",
-				 path, rc);
+			if (rc == -ENOENT)
+				dev_dbg(device, "loading %s failed with error %d\n",
+					 path, rc);
+			else
+				dev_warn(device, "loading %s failed with error %d\n",
+					 path, rc);
 			continue;
 		}
 		dev_dbg(device, "direct-loading %s\n", buf->fw_id);
-- 
2.7.0