| Message ID | 20190122145742.11292-2-gregkh@linuxfoundation.org (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | mips: cleanup debugfs usage | expand |
Hi, On Tue, Jan 22, 2019 at 03:57:38PM +0100, Greg Kroah-Hartman wrote: > -static int init_debufs(void) > +static void init_debugfs(void) > { > - struct dentry *show_dentry; > dir = debugfs_create_dir("oct_ilm", 0); > - if (!dir) { > - pr_err("oct_ilm: failed to create debugfs entry oct_ilm\n"); > - return -1; > - } > - > - show_dentry = debugfs_create_file("statistics", 0222, dir, NULL, > - &oct_ilm_ops); > - if (!show_dentry) { > - pr_err("oct_ilm: failed to create debugfs entry oct_ilm/statistics\n"); > - return -1; > - } > - > - show_dentry = debugfs_create_file("reset", 0222, dir, NULL, > - &reset_statistics_ops); > - if (!show_dentry) { > - pr_err("oct_ilm: failed to create debugfs entry oct_ilm/reset\n"); > - return -1; > - } > - > + debugfs_create_file("statistics", 0222, dir, NULL, &oct_ilm_ops); > + debugfs_create_file("reset", 0222, dir, NULL, &reset_statistics_ops); > return 0; The return needs to be deleted now that the function is void. A.
Hello, On Tue, Jan 22, 2019 at 08:48:56PM +0200, Aaro Koskinen wrote: > On Tue, Jan 22, 2019 at 03:57:38PM +0100, Greg Kroah-Hartman wrote: > > -static int init_debufs(void) > > +static void init_debugfs(void) > > { > > - struct dentry *show_dentry; > > dir = debugfs_create_dir("oct_ilm", 0); > > - if (!dir) { > > - pr_err("oct_ilm: failed to create debugfs entry oct_ilm\n"); > > - return -1; > > - } > > - > > - show_dentry = debugfs_create_file("statistics", 0222, dir, NULL, > > - &oct_ilm_ops); > > - if (!show_dentry) { > > - pr_err("oct_ilm: failed to create debugfs entry oct_ilm/statistics\n"); > > - return -1; > > - } > > - > > - show_dentry = debugfs_create_file("reset", 0222, dir, NULL, > > - &reset_statistics_ops); > > - if (!show_dentry) { > > - pr_err("oct_ilm: failed to create debugfs entry oct_ilm/reset\n"); > > - return -1; > > - } > > - > > + debugfs_create_file("statistics", 0222, dir, NULL, &oct_ilm_ops); > > + debugfs_create_file("reset", 0222, dir, NULL, &reset_statistics_ops); > > return 0; > > The return needs to be deleted now that the function is void. Well spotted - I'm happy to fix this up whilst applying the patch. Thanks, Paul
On Tue, Jan 22, 2019 at 07:22:57PM +0000, Paul Burton wrote: > Hello, > > On Tue, Jan 22, 2019 at 08:48:56PM +0200, Aaro Koskinen wrote: > > On Tue, Jan 22, 2019 at 03:57:38PM +0100, Greg Kroah-Hartman wrote: > > > -static int init_debufs(void) > > > +static void init_debugfs(void) > > > { > > > - struct dentry *show_dentry; > > > dir = debugfs_create_dir("oct_ilm", 0); > > > - if (!dir) { > > > - pr_err("oct_ilm: failed to create debugfs entry oct_ilm\n"); > > > - return -1; > > > - } > > > - > > > - show_dentry = debugfs_create_file("statistics", 0222, dir, NULL, > > > - &oct_ilm_ops); > > > - if (!show_dentry) { > > > - pr_err("oct_ilm: failed to create debugfs entry oct_ilm/statistics\n"); > > > - return -1; > > > - } > > > - > > > - show_dentry = debugfs_create_file("reset", 0222, dir, NULL, > > > - &reset_statistics_ops); > > > - if (!show_dentry) { > > > - pr_err("oct_ilm: failed to create debugfs entry oct_ilm/reset\n"); > > > - return -1; > > > - } > > > - > > > + debugfs_create_file("statistics", 0222, dir, NULL, &oct_ilm_ops); > > > + debugfs_create_file("reset", 0222, dir, NULL, &reset_statistics_ops); > > > return 0; > > > > The return needs to be deleted now that the function is void. > > Well spotted - I'm happy to fix this up whilst applying the patch. The fact that 0-day didn't catch this makes me worried, is this platform/driver not being built there? Thanks for catching this, sorry for the mistake. greg k-h
Hi Greg, On Tue, Jan 22, 2019 at 08:29:16PM +0100, Greg Kroah-Hartman wrote: > On Tue, Jan 22, 2019 at 07:22:57PM +0000, Paul Burton wrote: > > On Tue, Jan 22, 2019 at 08:48:56PM +0200, Aaro Koskinen wrote: > > > On Tue, Jan 22, 2019 at 03:57:38PM +0100, Greg Kroah-Hartman wrote: > > > > -static int init_debufs(void) > > > > +static void init_debugfs(void) > > > > { > > > > - struct dentry *show_dentry; > > > > dir = debugfs_create_dir("oct_ilm", 0); > > > > - if (!dir) { > > > > - pr_err("oct_ilm: failed to create debugfs entry oct_ilm\n"); > > > > - return -1; > > > > - } > > > > - > > > > - show_dentry = debugfs_create_file("statistics", 0222, dir, NULL, > > > > - &oct_ilm_ops); > > > > - if (!show_dentry) { > > > > - pr_err("oct_ilm: failed to create debugfs entry oct_ilm/statistics\n"); > > > > - return -1; > > > > - } > > > > - > > > > - show_dentry = debugfs_create_file("reset", 0222, dir, NULL, > > > > - &reset_statistics_ops); > > > > - if (!show_dentry) { > > > > - pr_err("oct_ilm: failed to create debugfs entry oct_ilm/reset\n"); > > > > - return -1; > > > > - } > > > > - > > > > + debugfs_create_file("statistics", 0222, dir, NULL, &oct_ilm_ops); > > > > + debugfs_create_file("reset", 0222, dir, NULL, &reset_statistics_ops); > > > > return 0; > > > > > > The return needs to be deleted now that the function is void. > > > > Well spotted - I'm happy to fix this up whilst applying the patch. > > The fact that 0-day didn't catch this makes me worried, is this > platform/driver not being built there? It looks like this code ought to be built as a module for cavium_octeon_defconfig: $ grep oct_ilm arch/mips/cavium-octeon/Makefile obj-$(CONFIG_OCTEON_ILM) += oct_ilm.o $ grep OCTEON_ILM arch/mips/configs/cavium_octeon_defconfig CONFIG_OCTEON_ILM=m Fengguang or others - does 0-day build cavium_octeon_defconfig? If so, does it build modules? Thanks, Paul
diff --git a/arch/mips/cavium-octeon/oct_ilm.c b/arch/mips/cavium-octeon/oct_ilm.c index 2d68a39f1443..0b6ec4c17896 100644 --- a/arch/mips/cavium-octeon/oct_ilm.c +++ b/arch/mips/cavium-octeon/oct_ilm.c @@ -63,31 +63,12 @@ static int reset_statistics(void *data, u64 value) DEFINE_SIMPLE_ATTRIBUTE(reset_statistics_ops, NULL, reset_statistics, "%llu\n"); -static int init_debufs(void) +static void init_debugfs(void) { - struct dentry *show_dentry; dir = debugfs_create_dir("oct_ilm", 0); - if (!dir) { - pr_err("oct_ilm: failed to create debugfs entry oct_ilm\n"); - return -1; - } - - show_dentry = debugfs_create_file("statistics", 0222, dir, NULL, - &oct_ilm_ops); - if (!show_dentry) { - pr_err("oct_ilm: failed to create debugfs entry oct_ilm/statistics\n"); - return -1; - } - - show_dentry = debugfs_create_file("reset", 0222, dir, NULL, - &reset_statistics_ops); - if (!show_dentry) { - pr_err("oct_ilm: failed to create debugfs entry oct_ilm/reset\n"); - return -1; - } - + debugfs_create_file("statistics", 0222, dir, NULL, &oct_ilm_ops); + debugfs_create_file("reset", 0222, dir, NULL, &reset_statistics_ops); return 0; - } static void init_latency_info(struct latency_info *li, int startup) @@ -169,11 +150,7 @@ static __init int oct_ilm_module_init(void) int rc; int irq = OCTEON_IRQ_TIMER0 + TIMER_NUM; - rc = init_debufs(); - if (rc) { - WARN(1, "Could not create debugfs entries"); - return rc; - } + init_debugfs(); rc = request_irq(irq, cvm_oct_ciu_timer_interrupt, IRQF_NO_THREAD, "oct_ilm", 0);
When calling debugfs functions, there is no need to ever check the return value. The function can work or not, but the code logic should never do something different based on this. Cc: Ralf Baechle <ralf@linux-mips.org> Cc: Paul Burton <paul.burton@mips.com> Cc: James Hogan <jhogan@kernel.org> Cc: linux-mips@vger.kernel.org Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> --- arch/mips/cavium-octeon/oct_ilm.c | 31 ++++--------------------------- 1 file changed, 4 insertions(+), 27 deletions(-)