diff mbox

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

Message ID 20171120182409.27348-20-mcgrof@kernel.org
State New, archived
Headers show

Commit Message

Luis Chamberlain Nov. 20, 2017, 6:24 p.m. UTC
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

Comments

Greg Kroah-Hartman Nov. 29, 2017, 10:28 a.m. UTC | #1
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
Luis Chamberlain Nov. 30, 2017, 8:35 p.m. UTC | #2
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 mbox

Patch

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);