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

From: "Luis R. Rodriguez" <mcgrof@kernel.org>
To: Greg KH <gregkh@linuxfoundation.org>
Cc: "Luis R. Rodriguez" <mcgrof@kernel.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: Thu, 30 Nov 2017 21:35:16 +0100	[thread overview]
Message-ID: <20171130203516.GN729@wotan.suse.de> (raw)
In-Reply-To: <20171129102804.GA12916@kroah.com>

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