lkml.org 
[lkml]   [2017]   [Nov]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patches in this message
    /
    Date
    From
    SubjectRe: [PATCH v2 19/23] firmware: add debug facility to emulate forcing sysfs fallback
    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

    \
     
     \ /
      Last update: 2017-12-01 00:55    [W:2.485 / U:0.096 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site