From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751256AbdK3UfT (ORCPT ); Thu, 30 Nov 2017 15:35:19 -0500 Received: from mx2.suse.de ([195.135.220.15]:49887 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750890AbdK3UfS (ORCPT ); Thu, 30 Nov 2017 15:35:18 -0500 Date: Thu, 30 Nov 2017 21:35:16 +0100 From: "Luis R. Rodriguez" To: Greg KH Cc: "Luis R. Rodriguez" , 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 Message-ID: <20171130203516.GN729@wotan.suse.de> References: <20171120182409.27348-1-mcgrof@kernel.org> <20171120182409.27348-20-mcgrof@kernel.org> <20171129102804.GA12916@kroah.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20171129102804.GA12916@kroah.com> User-Agent: Mutt/1.6.0 (2016-04-01) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > > +#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 > > > > #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