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

From: "Luis R. Rodriguez" <mcgrof@kernel.org>
To: "Luis R. Rodriguez" <mcgrof@kernel.org>
Cc: Greg KH <gregkh@linuxfoundation.org>,
	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: Fri, 1 Dec 2017 00:54:25 +0100	[thread overview]
Message-ID: <20171130235425.GO729@wotan.suse.de> (raw)
In-Reply-To: <20171130203516.GN729@wotan.suse.de>

On Thu, Nov 30, 2017 at 09:35:16PM +0100, Luis R. Rodriguez wrote:
> 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?

You know, I'm wondering if it just makes sense to go straight into making
CONFIG_FW_LOADER_USER_HELPER_FALLBACK nothing but a setting a bool on a
config to true.

diff --git a/drivers/base/firmware_loader.c b/drivers/base/firmware_loader.c
index 43b97a8137f7..d3f2aabfc41d 100644
--- a/drivers/base/firmware_loader.c
+++ b/drivers/base/firmware_loader.c
@@ -117,6 +117,12 @@ struct fw_name_devm {
 	const char *name;
 };
 
+struct firmware_config {
+	bool force_sysfs_fallback;
+};
+
+static struct firmware_config fw_config;
+
 static inline struct fw_priv *to_fw_priv(struct kref *ref)
 {
 	return container_of(ref, struct fw_priv, ref);
@@ -1151,18 +1157,25 @@ static int fw_load_from_user_helper(struct firmware *firmware,
 }
 
 #ifdef CONFIG_FW_LOADER_USER_HELPER_FALLBACK
-static bool fw_force_sysfs_fallback(unsigned int opt_flags)
+static void __init fw_config_init(void)
 {
-	return true;
+	fw_config.force_sysfs_fallback = true;
 }
+
 #else
+static void __init fw_config_init(void)
+{
+}
+#endif
+
 static bool fw_force_sysfs_fallback(unsigned int opt_flags)
 {
+	if (fw_config.force_sysfs_fallback)
+		return true;
 	if (!(opt_flags & FW_OPT_USERHELPER))
 		return false;
 	return true;
 }
-#endif
 
 static bool fw_run_sysfs_fallback(unsigned int opt_flags)
 {
@@ -1911,6 +1924,7 @@ static int __init firmware_class_init(void)
 	int ret;
 
 	/* No need to unfold these on exit */
+	fw_config_init();
 	fw_cache_init();
 
 	ret = register_fw_pm_ops();

After which we can add two generic syfs firmware knobs to help do the same as I did
for debugfs, only we actually support it as proper API. Thoughts?

For instance for changing to force the usermode helper:

diff --git a/drivers/base/firmware_loader.c b/drivers/base/firmware_loader.c
index d3f2aabfc41d..659db28f5c02 100644
--- a/drivers/base/firmware_loader.c
+++ b/drivers/base/firmware_loader.c
@@ -41,6 +41,9 @@ MODULE_AUTHOR("Manuel Estrada Sainz");
 MODULE_DESCRIPTION("Multi purpose firmware loading support");
 MODULE_LICENSE("GPL");
 
+static unsigned int zero;
+static unsigned int one = 1;
+
 enum fw_status {
 	FW_STATUS_UNKNOWN,
 	FW_STATUS_LOADING,
@@ -1919,6 +1922,19 @@ static struct notifier_block fw_shutdown_nb = {
 	.notifier_call = fw_shutdown_notify,
 };
 
+struct ctl_table firmware_config_table[] = {
+	{
+		.procname	= "force_sysfs_fallback",
+		.data		= &fw_config.force_sysfs_fallback,
+		.maxlen         = sizeof(unsigned int),
+		.mode           = 0644,
+		.proc_handler   = proc_douintvec_minmax,
+		.extra1		= &zero,
+		.extra2		= &one,
+	},
+	{ }
+};
+
 static int __init firmware_class_init(void)
 {
 	int ret;
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 557d46728577..202442f3c58c 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -251,6 +251,10 @@ extern struct ctl_table random_table[];
 extern struct ctl_table epoll_table[];
 #endif
 
+#ifdef CONFIG_FW_LOADER
+extern struct ctl_table firmware_config_table[];
+#endif
+
 #ifdef HAVE_ARCH_PICK_MMAP_LAYOUT
 int sysctl_legacy_va_layout;
 #endif
@@ -746,6 +750,13 @@ static struct ctl_table kern_table[] = {
 		.mode		= 0555,
 		.child		= usermodehelper_table,
 	},
+#ifdef CONFIG_FW_LOADER
+	{
+		.procname	= "firmware_config",
+		.mode		= 0555,
+		.child		= firmware_config_table,
+	},
+#endif
 	{
 		.procname	= "overflowuid",
 		.data		= &overflowuid,

Thoughts, preferences?

  Luis
