Re: [PATCH v2 19/23] firmware: add debug facility to emulate forcing sysfs fallback

From: Greg KH <gregkh@linuxfoundation.org>
To: "Luis R. Rodriguez" <mcgrof@kernel.org>
Cc: akpm@linux-foundation.org, keescook@chromium.org,
	mfuzzey@parkeon.com, zohar@linux.vnet.ibm.com,
	dhowells@redhat.com, pali.rohar@gmail.com, tiwai@suse.de,
	arend.vanspriel@broadcom.com, zajec5@gmail.com, nbroeking@me.com,
	markivx@codeaurora.org, stephen.boyd@linaro.org,
	broonie@kernel.org, dmitry.torokhov@gmail.com,
	dwmw2@infradead.org, torvalds@linux-foundation.org,
	Abhay_Salunke@dell.com, bjorn.andersson@linaro.org,
	jewalt@lgsinnovations.com, linux-kernel@vger.kernel.org,
	linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH v2 19/23] firmware: add debug facility to emulate forcing sysfs fallback
Date: Wed, 29 Nov 2017 11:28:04 +0100	[thread overview]
Message-ID: <20171129102804.GA12916@kroah.com> (raw)
In-Reply-To: <20171120182409.27348-20-mcgrof@kernel.org>

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