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 <mcg...@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
