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
