Skip to contentSkip to conversation

firmware: few fixes for v4.15

PATCH In review Luis R. Rodriguez 2 participants 7 messages Nov 20, 2017 – Nov 29, 2017

Subsystems (3)
VFS Kselftest Driver core
lore
Share & tools
lore

Luis R. Rodriguez

author
#0 lore raw

cover firmware: few fixes for v4.15

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(-)
Signature · 1 line
2.15.0
author
#1 lore raw

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

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

Signed-off-by Luis R. Rodriguez <[email protected]>
---
 drivers/base/firmware_class.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)
drivers/base/firmware_class.c +10 -4
index 4b57cf5bc81d..149413a376cf 100644
@@ -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);
Signature · 1 line
2.15.0
author
#2 lore raw

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

In reply to 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 <[email protected]>
---
 drivers/base/firmware_class.c | 45 ++++++++++++++++++++++++++++++-------------
 1 file changed, 32 insertions(+), 13 deletions(-)
drivers/base/firmware_class.c +32 -13
index 149413a376cf..c8033c5488f9 100644
@@ -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);
Signature · 1 line
2.15.0
author
#3 lore raw

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

In reply to 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 <[email protected]>
---
 drivers/base/firmware_class.c | 29 +++++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)
drivers/base/firmware_class.c +21 -8
index c8033c5488f9..3340a17e0499 100644
@@ -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);
Signature · 1 line
2.15.0
author
#4 lore raw

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

In reply to 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 <[email protected]>
---
 drivers/base/firmware_class.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)
drivers/base/firmware_class.c +8 -1
index 3340a17e0499..7ab54f2b032f 100644
@@ -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)
Signature · 1 line
2.15.0
author
#5 lore raw

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

In reply to 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 <[email protected]>
---
 tools/testing/selftests/firmware/fw_filesystem.sh | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
tools/testing/selftests/firmware/fw_filesystem.sh +4 -1
index b1f20fef36c7..f9508e1a4058 100755
@@ -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"
 }
Signature · 1 line
2.15.0
maintainer
#6 lore raw
In reply to Luis R. Rodriguez
Quoted 11 lines · 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.

Quoted 1 line

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