[PATCH 0/5] firmware: few fixes for v4.15

[PATCH 0/5] firmware: few fixes for v4.15

[Luis R. Rodriguez]

Greg,

Here's a few fixes that I came up with over the weekend for the
firmware loader while doing some other development. I realize we
have a short merge window this time due to the upcoming holidays,
so I understand if its too late, but considering they are simple
fixes I figured I'd let you decide if this should wait for v4.16
or be folded in for v4.15. The fixes are not critical as its
very improbably you'll run into issues with them. The code
folding helps make the fixes easier to manage and read.

I let 0-day test over the weekend and no build issues were found.

  Luis

Luis R. Rodriguez (5):
  firmware: add helper to unregister pm ops
  firmware: fix capturing errors on fw_cache_init() on early init
  firmware: provide helpers for registering the syfs loader
  firmware: fix detecting error on register_reboot_notifier()
  test_firmware: fix setting old custom fw path back on exit

 drivers/base/firmware_class.c                     | 95 +++++++++++++++++------
 tools/testing/selftests/firmware/fw_filesystem.sh |  5 +-
 2 files changed, 74 insertions(+), 26 deletions(-)

-- 
2.15.0

[PATCH 1/5] firmware: add helper to unregister pm ops

[Luis R. Rodriguez]

This will be used later to unfold on error on init.

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

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 4b57cf5bc81d..149413a376cf 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -1767,11 +1767,20 @@ static int fw_suspend(void)
 static struct syscore_ops fw_syscore_ops = {
 	.suspend = fw_suspend,
 };
+
+static inline void unregister_fw_pm_ops(void)
+{
+	unregister_syscore_ops(&fw_syscore_ops);
+	unregister_pm_notifier(&fw_cache.pm_notify);
+}
 #else
 static int fw_cache_piggyback_on_request(const char *name)
 {
 	return 0;
 }
+static inline void unregister_fw_pm_ops(void)
+{
+}
 #endif
 
 static void __init fw_cache_init(void)
@@ -1823,10 +1832,7 @@ static int __init firmware_class_init(void)
 
 static void __exit firmware_class_exit(void)
 {
-#ifdef CONFIG_PM_SLEEP
-	unregister_syscore_ops(&fw_syscore_ops);
-	unregister_pm_notifier(&fw_cache.pm_notify);
-#endif
+	unregister_fw_pm_ops();
 	unregister_reboot_notifier(&fw_shutdown_nb);
 #ifdef CONFIG_FW_LOADER_USER_HELPER
 	class_unregister(&firmware_class);
-- 
2.15.0

[PATCH 2/5] firmware: fix capturing errors on fw_cache_init() on early init

[Luis R. Rodriguez]

register_pm_notifier() can technically fail, caputure this.
Note that register_syscore_ops() cannot fail given it just
adds an element to a linked list. This has been broken since
v3.7. Chances of this failing however are slim.

To improve code readability move the code folded under CONFIG_PM_SLEEP
into a helper.

Fixes: 07646d9c0938d ("firmware loader: cache devices firmware during suspend/resume cycle")
Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 drivers/base/firmware_class.c | 45 ++++++++++++++++++++++++++++++-------------
 1 file changed, 32 insertions(+), 13 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 149413a376cf..c8033c5488f9 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -1768,6 +1768,26 @@ static struct syscore_ops fw_syscore_ops = {
 	.suspend = fw_suspend,
 };
 
+static int __init register_fw_pm_ops(void)
+{
+	int ret;
+
+	spin_lock_init(&fw_cache.name_lock);
+	INIT_LIST_HEAD(&fw_cache.fw_names);
+
+	INIT_DELAYED_WORK(&fw_cache.work,
+			  device_uncache_fw_images_work);
+
+	fw_cache.pm_notify.notifier_call = fw_pm_notify;
+	ret = register_pm_notifier(&fw_cache.pm_notify);
+	if (ret)
+		return ret;
+
+	register_syscore_ops(&fw_syscore_ops);
+
+	return ret;
+}
+
 static inline void unregister_fw_pm_ops(void)
 {
 	unregister_syscore_ops(&fw_syscore_ops);
@@ -1778,6 +1798,10 @@ static int fw_cache_piggyback_on_request(const char *name)
 {
 	return 0;
 }
+static inline int register_fw_pm_ops(void)
+{
+	return 0;
+}
 static inline void unregister_fw_pm_ops(void)
 {
 }
@@ -1788,19 +1812,6 @@ static void __init fw_cache_init(void)
 	spin_lock_init(&fw_cache.lock);
 	INIT_LIST_HEAD(&fw_cache.head);
 	fw_cache.state = FW_LOADER_NO_CACHE;
-
-#ifdef CONFIG_PM_SLEEP
-	spin_lock_init(&fw_cache.name_lock);
-	INIT_LIST_HEAD(&fw_cache.fw_names);
-
-	INIT_DELAYED_WORK(&fw_cache.work,
-			  device_uncache_fw_images_work);
-
-	fw_cache.pm_notify.notifier_call = fw_pm_notify;
-	register_pm_notifier(&fw_cache.pm_notify);
-
-	register_syscore_ops(&fw_syscore_ops);
-#endif
 }
 
 static int fw_shutdown_notify(struct notifier_block *unused1,
@@ -1821,7 +1832,15 @@ static struct notifier_block fw_shutdown_nb = {
 
 static int __init firmware_class_init(void)
 {
+	int ret;
+
+	/* No need to unfold these on exit */
 	fw_cache_init();
+
+	ret = register_fw_pm_ops();
+	if (ret)
+		return ret;
+
 	register_reboot_notifier(&fw_shutdown_nb);
 #ifdef CONFIG_FW_LOADER_USER_HELPER
 	return class_register(&firmware_class);
-- 
2.15.0

[PATCH 3/5] firmware: provide helpers for registering the syfs loader

[Luis R. Rodriguez]

This makes init / exit much easier to read, and we can later
reuse this code on other errors not captured yet.

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

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index c8033c5488f9..3340a17e0499 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -687,6 +687,16 @@ static struct class firmware_class = {
 	.dev_release	= fw_dev_release,
 };
 
+static inline int register_sysfs_loader(void)
+{
+	return class_register(&firmware_class);
+}
+
+static inline void unregister_sysfs_loader(void)
+{
+	class_unregister(&firmware_class);
+}
+
 static ssize_t firmware_loading_show(struct device *dev,
 				     struct device_attribute *attr, char *buf)
 {
@@ -1124,6 +1134,15 @@ fw_load_from_user_helper(struct firmware *firmware, const char *name,
 
 static inline void kill_pending_fw_fallback_reqs(bool only_kill_custom) { }
 
+static inline int register_sysfs_loader(void)
+{
+	return 0;
+}
+
+static inline void unregister_sysfs_loader(void)
+{
+}
+
 #endif /* CONFIG_FW_LOADER_USER_HELPER */
 
 /* prepare firmware and firmware_buf structs;
@@ -1842,20 +1861,14 @@ static int __init firmware_class_init(void)
 		return ret;
 
 	register_reboot_notifier(&fw_shutdown_nb);
-#ifdef CONFIG_FW_LOADER_USER_HELPER
-	return class_register(&firmware_class);
-#else
-	return 0;
-#endif
+	return register_sysfs_loader();
 }
 
 static void __exit firmware_class_exit(void)
 {
 	unregister_fw_pm_ops();
 	unregister_reboot_notifier(&fw_shutdown_nb);
-#ifdef CONFIG_FW_LOADER_USER_HELPER
-	class_unregister(&firmware_class);
-#endif
+	unregister_sysfs_loader();
 }
 
 fs_initcall(firmware_class_init);
-- 
2.15.0

[PATCH 4/5] firmware: fix detecting error on register_reboot_notifier()

[Luis R. Rodriguez]

register_reboot_notifier() can fail, detect this and address this
failure. This has been broken since v3.11, however the chances of
this failing here is really low.

Fixes: fe304143b0c3d ("firmware: Avoid deadlock of usermodehelper lock at shutdown")
Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 drivers/base/firmware_class.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 3340a17e0499..7ab54f2b032f 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -1860,8 +1860,15 @@ static int __init firmware_class_init(void)
 	if (ret)
 		return ret;
 
-	register_reboot_notifier(&fw_shutdown_nb);
+	ret = register_reboot_notifier(&fw_shutdown_nb);
+	if (ret)
+		goto out;
+
 	return register_sysfs_loader();
+
+out:
+	unregister_fw_pm_ops();
+	return ret;
 }
 
 static void __exit firmware_class_exit(void)
-- 
2.15.0

[PATCH 5/5] test_firmware: fix setting old custom fw path back on exit

[Luis R. Rodriguez]

The file /sys/module/firmware_class/parameters/path can be used
to set a custom firmware path. The fw_filesystem.sh script creates
a temporary directory to add a test firmware file to be used during
testing, in order for this to work it uses the custom path syfs file
and it was supposed to reset back the file on execution exit. The
script failed to do this due to a typo, it was using OLD_PATH instead
of OLD_FWPATH, since its inception since v3.17.

Its not as easy to just keep the old setting, it turns out that
resetting an empty setting won't actually do what we want, we need
to check if it was empty and set an empty space.

Without this we end up having the temporary path always set after
we run these tests.

Fixes: 0a8adf58475 ("test: add firmware_class loader test")
Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 tools/testing/selftests/firmware/fw_filesystem.sh | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/firmware/fw_filesystem.sh b/tools/testing/selftests/firmware/fw_filesystem.sh
index b1f20fef36c7..f9508e1a4058 100755
--- a/tools/testing/selftests/firmware/fw_filesystem.sh
+++ b/tools/testing/selftests/firmware/fw_filesystem.sh
@@ -45,7 +45,10 @@ test_finish()
 	if [ "$HAS_FW_LOADER_USER_HELPER" = "yes" ]; then
 		echo "$OLD_TIMEOUT" >/sys/class/firmware/timeout
 	fi
-	echo -n "$OLD_PATH" >/sys/module/firmware_class/parameters/path
+	if [ "$OLD_FWPATH" = "" ]; then
+		OLD_FWPATH=" "
+	fi
+	echo -n "$OLD_FWPATH" >/sys/module/firmware_class/parameters/path
 	rm -f "$FW"
 	rmdir "$FWPATH"
 }
-- 
2.15.0

Re: [PATCH 0/5] firmware: few fixes for v4.15

[Greg KH]

On Mon, Nov 20, 2017 at 09:45:30AM -0800, Luis R. Rodriguez wrote:
> Greg,
> 
> Here's a few fixes that I came up with over the weekend for the
> firmware loader while doing some other development. I realize we
> have a short merge window this time due to the upcoming holidays,
> so I understand if its too late, but considering they are simple
> fixes I figured I'd let you decide if this should wait for v4.16
> or be folded in for v4.15. The fixes are not critical as its
> very improbably you'll run into issues with them. The code
> folding helps make the fixes easier to manage and read.
> 
> I let 0-day test over the weekend and no build issues were found.

As these don't fix any reported issues, I'll just queue them up for
4.16-rc1.

thanks,

greg k-h