| Message ID | 20171120182409.27348-20-mcgrof@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Nov 20, 2017 at 10:24:05AM -0800, Luis R. Rodriguez wrote: > The CONFIG_FW_LOADER_USER_HELPER_FALLBACK kernel config is not > enabled my major distros, however its known to be enabled on > some Android kernels. Testing the firmware API properly then > means to test at least two different kernel configurations. > > Since CONFIG_FW_LOADER_USER_HELPER_FALLBACK only does a simple > enablement under certain situations we can just emulate this > behaviour through a debugfs knob. This enables testing two of > the tree possible kernel configs with the firmware loader with > one kernel build. > > Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org> > --- > drivers/base/Kconfig | 6 ++++++ > drivers/base/Makefile | 1 + > drivers/base/firmware_debug.c | 34 ++++++++++++++++++++++++++++++++ > drivers/base/firmware_debug.h | 44 ++++++++++++++++++++++++++++++++++++++++++ > drivers/base/firmware_loader.c | 13 ++++++++++++- > 5 files changed, 97 insertions(+), 1 deletion(-) > create mode 100644 drivers/base/firmware_debug.c > create mode 100644 drivers/base/firmware_debug.h > > diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig > index 2f6614c9a229..ab6889ac6787 100644 > --- a/drivers/base/Kconfig > +++ b/drivers/base/Kconfig > @@ -149,6 +149,12 @@ config EXTRA_FIRMWARE_DIR > config FW_LOADER_USER_HELPER > bool > > +config FW_LOADER_DEBUG > + bool "Firmware loader debugging interface" > + help > + If you are hacking on the firmware API of the kernel, firmware_class, > + you can enable this to help debug the kernel. > + > config FW_LOADER_USER_HELPER_FALLBACK > bool "Fallback user-helper invocation for firmware loading" > depends on FW_LOADER > diff --git a/drivers/base/Makefile b/drivers/base/Makefile > index f261143fafbf..64eaa4f9cb02 100644 > --- a/drivers/base/Makefile > +++ b/drivers/base/Makefile > @@ -14,6 +14,7 @@ obj-$(CONFIG_HAVE_GENERIC_DMA_COHERENT) += dma-coherent.o > obj-$(CONFIG_ISA_BUS_API) += isa.o > obj-$(CONFIG_FW_LOADER) += firmware_class.o > firmware_class-objs := firmware_loader.o > +firmware_class-$(CONFIG_FW_LOADER_DEBUG) += firmware_debug.o > obj-$(CONFIG_NUMA) += node.o > obj-$(CONFIG_MEMORY_HOTPLUG_SPARSE) += memory.o > ifeq ($(CONFIG_SYSFS),y) > diff --git a/drivers/base/firmware_debug.c b/drivers/base/firmware_debug.c > new file mode 100644 > index 000000000000..f2817eb6f480 > --- /dev/null > +++ b/drivers/base/firmware_debug.c > @@ -0,0 +1,34 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* Firmware dubugging interface */ > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > + > +#include <linux/debugfs.h> > +#include "firmware_debug.h" > + > +struct firmware_debug fw_debug; > + > +static struct dentry *debugfs_firmware; > + > +int __init register_fw_debugfs(void) > +{ > + debugfs_firmware = debugfs_create_dir("firmware", NULL); > + if (!debugfs_firmware) > + return -ENOMEM; You never need to check the return value of a debugfs call, you should not care about it, nor do anything different in your code. The value returned can always be passed back into any other debugfs call when needed. > + > + if (!debugfs_create_bool("force_sysfs_fallback", S_IRUSR | S_IWUSR, > + debugfs_firmware, > + &fw_debug.force_sysfs_fallback)) Same here, you don't care. > + goto err_out; > + > + return 0; > +err_out: > + debugfs_remove_recursive(debugfs_firmware); You didn't create any files, why recursive? Anyway, not needed. > + debugfs_firmware = NULL; > + return -ENOMEM; > +} > + > +void unregister_fw_debugfs(void) > +{ > + debugfs_remove_recursive(debugfs_firmware); > + debugfs_firmware = NULL; Why set this to NULL? > +} > diff --git a/drivers/base/firmware_debug.h b/drivers/base/firmware_debug.h > new file mode 100644 > index 000000000000..bc41bbca9a54 > --- /dev/null > +++ b/drivers/base/firmware_debug.h > @@ -0,0 +1,44 @@ > +// SPDX-License-Identifier: GPL-2.0 > +#ifndef __FIRMWARE_DEBUG_H > +#define __FIRMWARE_DEBUG_H > + > +#ifdef CONFIG_FW_LOADER_DEBUG > +/** > + * struct firmware_debug - firmware debugging configuration > + * > + * Provided to help debug the firmware API. > + * > + * @force_sysfs_fallback: force the sysfs fallback mechanism to be used > + * as if one had enabled CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y. > + * Useful to help debug a CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y > + * functionality on a kernel where that config entry has been disabled. > + */ > +struct firmware_debug { > + bool force_sysfs_fallback; > +}; > + > +extern struct firmware_debug fw_debug; > + > +int register_fw_debugfs(void); > +void unregister_fw_debugfs(void); > + > +static inline bool fw_debug_force_sysfs_fallback(void) > +{ > + return fw_debug.force_sysfs_fallback; > +} > +#else > +static inline int register_fw_debugfs(void) > +{ > + return 0; > +} > +static inline void unregister_fw_debugfs(void) > +{ > +} > + > +static inline bool fw_debug_force_sysfs_fallback(void) > +{ > + return false; > +} > +#endif /* CONFIG_FW_LOADER_DEBUG */ > + > +#endif /* __FIRMWARE_DEBUG_H */ > diff --git a/drivers/base/firmware_loader.c b/drivers/base/firmware_loader.c > index 43b97a8137f7..b2b52ba9f245 100644 > --- a/drivers/base/firmware_loader.c > +++ b/drivers/base/firmware_loader.c > @@ -36,6 +36,7 @@ > #include <generated/utsrelease.h> > > #include "base.h" > +#include "firmware_debug.h" > > MODULE_AUTHOR("Manuel Estrada Sainz"); > MODULE_DESCRIPTION("Multi purpose firmware loading support"); > @@ -1158,6 +1159,9 @@ static bool fw_force_sysfs_fallback(unsigned int opt_flags) > #else > static bool fw_force_sysfs_fallback(unsigned int opt_flags) > { > + if (fw_debug_force_sysfs_fallback()) > + return true; > + > if (!(opt_flags & FW_OPT_USERHELPER)) > return false; > return true; > @@ -1913,10 +1917,14 @@ static int __init firmware_class_init(void) > /* No need to unfold these on exit */ > fw_cache_init(); > > - ret = register_fw_pm_ops(); > + ret = register_fw_debugfs(); > if (ret) > return ret; Again you don't care about the state of debugfs. Did you test this on a system without CONFIG_DEBUGFS enabled? thanks, greg k-h
On Wed, Nov 29, 2017 at 11:28:04AM +0100, Greg KH wrote: > On Mon, Nov 20, 2017 at 10:24:05AM -0800, Luis R. Rodriguez wrote: > > diff --git a/drivers/base/firmware_debug.c b/drivers/base/firmware_debug.c > > new file mode 100644 > > index 000000000000..f2817eb6f480 > > --- /dev/null > > +++ b/drivers/base/firmware_debug.c > > @@ -0,0 +1,34 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* Firmware dubugging interface */ > > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > > + > > +#include <linux/debugfs.h> > > +#include "firmware_debug.h" > > + > > +struct firmware_debug fw_debug; > > + > > +static struct dentry *debugfs_firmware; > > + > > +int __init register_fw_debugfs(void) > > +{ > > + debugfs_firmware = debugfs_create_dir("firmware", NULL); > > + if (!debugfs_firmware) > > + return -ENOMEM; > > You never need to check the return value of a debugfs call, you should > not care about it, nor do anything different in your code. The value > returned can always be passed back into any other debugfs call when > needed. Neat, so all uses as in the above are wrong eh? > > + > > + if (!debugfs_create_bool("force_sysfs_fallback", S_IRUSR | S_IWUSR, > > + debugfs_firmware, > > + &fw_debug.force_sysfs_fallback)) > > Same here, you don't care. OK! > > > + goto err_out; > > + > > + return 0; > > +err_out: > > + debugfs_remove_recursive(debugfs_firmware); > > You didn't create any files, why recursive? > Anyway, not needed. Wouldn't this take care of removing the two bool files I add in one shot later? > > + debugfs_firmware = NULL; > > + return -ENOMEM; > > +} > > + > > +void unregister_fw_debugfs(void) > > +{ > > + debugfs_remove_recursive(debugfs_firmware); > > + debugfs_firmware = NULL; > > Why set this to NULL? OK, will avoid. > > diff --git a/drivers/base/firmware_loader.c b/drivers/base/firmware_loader.c > > index 43b97a8137f7..b2b52ba9f245 100644 > > --- a/drivers/base/firmware_loader.c > > +++ b/drivers/base/firmware_loader.c > > @@ -36,6 +36,7 @@ > > #include <generated/utsrelease.h> > > > > #include "base.h" > > +#include "firmware_debug.h" > > > > MODULE_AUTHOR("Manuel Estrada Sainz"); > > MODULE_DESCRIPTION("Multi purpose firmware loading support"); > > @@ -1158,6 +1159,9 @@ static bool fw_force_sysfs_fallback(unsigned int opt_flags) > > #else > > static bool fw_force_sysfs_fallback(unsigned int opt_flags) > > { > > + if (fw_debug_force_sysfs_fallback()) > > + return true; > > + > > if (!(opt_flags & FW_OPT_USERHELPER)) > > return false; > > return true; > > @@ -1913,10 +1917,14 @@ static int __init firmware_class_init(void) > > /* No need to unfold these on exit */ > > fw_cache_init(); > > > > - ret = register_fw_pm_ops(); > > + ret = register_fw_debugfs(); > > if (ret) > > return ret; > > Again you don't care about the state of debugfs. Did you test this on a > system without CONFIG_DEBUGFS enabled? Hrm, nope. Will fix it up. Thanks for the review. Luis
diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig index 2f6614c9a229..ab6889ac6787 100644 --- a/drivers/base/Kconfig +++ b/drivers/base/Kconfig @@ -149,6 +149,12 @@ config EXTRA_FIRMWARE_DIR config FW_LOADER_USER_HELPER bool +config FW_LOADER_DEBUG + bool "Firmware loader debugging interface" + help + If you are hacking on the firmware API of the kernel, firmware_class, + you can enable this to help debug the kernel. + config FW_LOADER_USER_HELPER_FALLBACK bool "Fallback user-helper invocation for firmware loading" depends on FW_LOADER diff --git a/drivers/base/Makefile b/drivers/base/Makefile index f261143fafbf..64eaa4f9cb02 100644 --- a/drivers/base/Makefile +++ b/drivers/base/Makefile @@ -14,6 +14,7 @@ obj-$(CONFIG_HAVE_GENERIC_DMA_COHERENT) += dma-coherent.o obj-$(CONFIG_ISA_BUS_API) += isa.o obj-$(CONFIG_FW_LOADER) += firmware_class.o firmware_class-objs := firmware_loader.o +firmware_class-$(CONFIG_FW_LOADER_DEBUG) += firmware_debug.o obj-$(CONFIG_NUMA) += node.o obj-$(CONFIG_MEMORY_HOTPLUG_SPARSE) += memory.o ifeq ($(CONFIG_SYSFS),y) diff --git a/drivers/base/firmware_debug.c b/drivers/base/firmware_debug.c new file mode 100644 index 000000000000..f2817eb6f480 --- /dev/null +++ b/drivers/base/firmware_debug.c @@ -0,0 +1,34 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Firmware dubugging interface */ +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt + +#include <linux/debugfs.h> +#include "firmware_debug.h" + +struct firmware_debug fw_debug; + +static struct dentry *debugfs_firmware; + +int __init register_fw_debugfs(void) +{ + debugfs_firmware = debugfs_create_dir("firmware", NULL); + if (!debugfs_firmware) + return -ENOMEM; + + if (!debugfs_create_bool("force_sysfs_fallback", S_IRUSR | S_IWUSR, + debugfs_firmware, + &fw_debug.force_sysfs_fallback)) + goto err_out; + + return 0; +err_out: + debugfs_remove_recursive(debugfs_firmware); + debugfs_firmware = NULL; + return -ENOMEM; +} + +void unregister_fw_debugfs(void) +{ + debugfs_remove_recursive(debugfs_firmware); + debugfs_firmware = NULL; +} diff --git a/drivers/base/firmware_debug.h b/drivers/base/firmware_debug.h new file mode 100644 index 000000000000..bc41bbca9a54 --- /dev/null +++ b/drivers/base/firmware_debug.h @@ -0,0 +1,44 @@ +// SPDX-License-Identifier: GPL-2.0 +#ifndef __FIRMWARE_DEBUG_H +#define __FIRMWARE_DEBUG_H + +#ifdef CONFIG_FW_LOADER_DEBUG +/** + * struct firmware_debug - firmware debugging configuration + * + * Provided to help debug the firmware API. + * + * @force_sysfs_fallback: force the sysfs fallback mechanism to be used + * as if one had enabled CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y. + * Useful to help debug a CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y + * functionality on a kernel where that config entry has been disabled. + */ +struct firmware_debug { + bool force_sysfs_fallback; +}; + +extern struct firmware_debug fw_debug; + +int register_fw_debugfs(void); +void unregister_fw_debugfs(void); + +static inline bool fw_debug_force_sysfs_fallback(void) +{ + return fw_debug.force_sysfs_fallback; +} +#else +static inline int register_fw_debugfs(void) +{ + return 0; +} +static inline void unregister_fw_debugfs(void) +{ +} + +static inline bool fw_debug_force_sysfs_fallback(void) +{ + return false; +} +#endif /* CONFIG_FW_LOADER_DEBUG */ + +#endif /* __FIRMWARE_DEBUG_H */ diff --git a/drivers/base/firmware_loader.c b/drivers/base/firmware_loader.c index 43b97a8137f7..b2b52ba9f245 100644 --- a/drivers/base/firmware_loader.c +++ b/drivers/base/firmware_loader.c @@ -36,6 +36,7 @@ #include <generated/utsrelease.h> #include "base.h" +#include "firmware_debug.h" MODULE_AUTHOR("Manuel Estrada Sainz"); MODULE_DESCRIPTION("Multi purpose firmware loading support"); @@ -1158,6 +1159,9 @@ static bool fw_force_sysfs_fallback(unsigned int opt_flags) #else static bool fw_force_sysfs_fallback(unsigned int opt_flags) { + if (fw_debug_force_sysfs_fallback()) + return true; + if (!(opt_flags & FW_OPT_USERHELPER)) return false; return true; @@ -1913,10 +1917,14 @@ static int __init firmware_class_init(void) /* No need to unfold these on exit */ fw_cache_init(); - ret = register_fw_pm_ops(); + ret = register_fw_debugfs(); if (ret) return ret; + ret = register_fw_pm_ops(); + if (ret) + goto out_debugfs; + ret = register_reboot_notifier(&fw_shutdown_nb); if (ret) goto out; @@ -1925,6 +1933,8 @@ static int __init firmware_class_init(void) out: unregister_fw_pm_ops(); +out_debugfs: + unregister_fw_debugfs(); return ret; } @@ -1933,6 +1943,7 @@ static void __exit firmware_class_exit(void) unregister_fw_pm_ops(); unregister_reboot_notifier(&fw_shutdown_nb); unregister_sysfs_loader(); + unregister_fw_debugfs(); } fs_initcall(firmware_class_init);
The CONFIG_FW_LOADER_USER_HELPER_FALLBACK kernel config is not enabled my major distros, however its known to be enabled on some Android kernels. Testing the firmware API properly then means to test at least two different kernel configurations. Since CONFIG_FW_LOADER_USER_HELPER_FALLBACK only does a simple enablement under certain situations we can just emulate this behaviour through a debugfs knob. This enables testing two of the tree possible kernel configs with the firmware loader with one kernel build. Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org> --- drivers/base/Kconfig | 6 ++++++ drivers/base/Makefile | 1 + drivers/base/firmware_debug.c | 34 ++++++++++++++++++++++++++++++++ drivers/base/firmware_debug.h | 44 ++++++++++++++++++++++++++++++++++++++++++ drivers/base/firmware_loader.c | 13 ++++++++++++- 5 files changed, 97 insertions(+), 1 deletion(-) create mode 100644 drivers/base/firmware_debug.c create mode 100644 drivers/base/firmware_debug.h