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
