LinuxLists.cc - [PATCH 0/4] ARM: cleanup debugfs usage

2019-01-22 14:42:52

Subject: [PATCH 0/4] ARM: cleanup debugfs usage

When calling debugfs code, there is no need to ever check the return
value of the call, as no logic should ever change if a call works
properly or not. Fix up a bunch of x86-specific code to not care about
the results of debugfs

Greg Kroah-Hartman (4):
arm64: dump: no need to check return value of debugfs_create functions
arm: dump: no need to check return value of debugfs_create functions
arm: omap1: no need to check return value of debugfs_create functions
arm: omap2: no need to check return value of debugfs_create functions

arch/arm/include/asm/ptdump.h | 9 ++---
arch/arm/mach-omap1/clock.c | 63 +++++++--------------------------
arch/arm/mach-omap1/pm.c | 7 ++--
arch/arm/mach-omap2/pm-debug.c | 15 ++++----
arch/arm/mm/dump.c | 4 +--
arch/arm/mm/ptdump_debugfs.c | 8 ++---
arch/arm64/include/asm/ptdump.h | 9 ++---
arch/arm64/mm/dump.c | 4 +--
arch/arm64/mm/ptdump_debugfs.c | 7 ++--
9 files changed, 34 insertions(+), 92 deletions(-)

--
2.20.1

2019-01-22 14:43:15

by Greg Kroah-Hartman

[permalink] [raw]

Subject: [PATCH 2/4] arm: dump: no need to check return value of debugfs_create functions

2019-01-22 14:43:23

by Greg Kroah-Hartman

[permalink] [raw]

Subject: [PATCH 3/4] arm: omap1: no need to check return value of debugfs_create functions

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: Paul Walmsley <[email protected]>
Cc: Aaro Koskinen <[email protected]>
Cc: Tony Lindgren <[email protected]>
Cc: Russell King <[email protected]>
Cc: Kevin Hilman <[email protected]>
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Greg Kroah-Hartman <[email protected]>
---
arch/arm/mach-omap1/clock.c | 63 +++++++------------------------------
arch/arm/mach-omap1/pm.c | 7 ++---
2 files changed, 14 insertions(+), 56 deletions(-)

diff --git a/arch/arm/mach-omap1/clock.c b/arch/arm/mach-omap1/clock.c
index c8c6fe88b2d6..3d7ab2bcf46c 100644
--- a/arch/arm/mach-omap1/clock.c
+++ b/arch/arm/mach-omap1/clock.c
@@ -990,84 +990,45 @@ static int debug_clock_show(struct seq_file *s, void *unused)

DEFINE_SHOW_ATTRIBUTE(debug_clock);

-static int clk_debugfs_register_one(struct clk *c)
+static void clk_debugfs_register_one(struct clk *c)
{
- int err;
struct dentry *d;
struct clk *pa = c->parent;

d = debugfs_create_dir(c->name, pa ? pa->dent : clk_debugfs_root);
- if (!d)
- return -ENOMEM;
c->dent = d;

- d = debugfs_create_u8("usecount", S_IRUGO, c->dent, &c->usecount);
- if (!d) {
- err = -ENOMEM;
- goto err_out;
- }
- d = debugfs_create_ulong("rate", S_IRUGO, c->dent, &c->rate);
- if (!d) {
- err = -ENOMEM;
- goto err_out;
- }
- d = debugfs_create_x8("flags", S_IRUGO, c->dent, &c->flags);
- if (!d) {
- err = -ENOMEM;
- goto err_out;
- }
- return 0;
-
-err_out:
- debugfs_remove_recursive(c->dent);
- return err;
+ debugfs_create_u8("usecount", S_IRUGO, c->dent, &c->usecount);
+ debugfs_create_ulong("rate", S_IRUGO, c->dent, &c->rate);
+ debugfs_create_x8("flags", S_IRUGO, c->dent, &c->flags);
}

-static int clk_debugfs_register(struct clk *c)
+static void clk_debugfs_register(struct clk *c)
{
int err;
struct clk *pa = c->parent;

- if (pa && !pa->dent) {
- err = clk_debugfs_register(pa);
- if (err)
- return err;
- }
+ if (pa && !pa->dent)
+ clk_debugfs_register(pa);

- if (!c->dent) {
- err = clk_debugfs_register_one(c);
- if (err)
- return err;
- }
- return 0;
+ if (!c->dent)
+ clk_debugfs_register_one(c);
}

static int __init clk_debugfs_init(void)
{
struct clk *c;
struct dentry *d;
- int err;

d = debugfs_create_dir("clock", NULL);
- if (!d)
- return -ENOMEM;
clk_debugfs_root = d;

- list_for_each_entry(c, &clocks, node) {
- err = clk_debugfs_register(c);
- if (err)
- goto err_out;
- }
+ list_for_each_entry(c, &clocks, node)
+ clk_debugfs_register(c);

- d = debugfs_create_file("summary", S_IRUGO,
- d, NULL, &debug_clock_fops);
- if (!d)
- return -ENOMEM;
+ debugfs_create_file("summary", S_IRUGO, d, NULL, &debug_clock_fops);

return 0;
-err_out:
- debugfs_remove_recursive(clk_debugfs_root);
- return err;
}
late_initcall(clk_debugfs_init);

diff --git a/arch/arm/mach-omap1/pm.c b/arch/arm/mach-omap1/pm.c
index 998075d3ef86..d068958d6f8a 100644
--- a/arch/arm/mach-omap1/pm.c
+++ b/arch/arm/mach-omap1/pm.c
@@ -539,11 +539,8 @@ static void omap_pm_init_debugfs(void)
struct dentry *d;

d = debugfs_create_dir("pm_debug", NULL);
- if (!d)
- return;
-
- (void) debugfs_create_file("omap_pm", S_IWUSR | S_IRUGO,
- d, NULL, &omap_pm_debug_fops);
+ debugfs_create_file("omap_pm", S_IWUSR | S_IRUGO, d, NULL,
+ &omap_pm_debug_fops);
}

#endif /* CONFIG_DEBUG_FS */
--
2.20.1

2019-01-22 14:43:35

by Greg Kroah-Hartman

[permalink] [raw]

Subject: [PATCH 4/4] arm: omap2: no need to check return value of debugfs_create functions

2019-01-22 14:44:12

by Greg Kroah-Hartman

[permalink] [raw]

Subject: [PATCH 1/4] arm64: dump: no need to check return value of debugfs_create functions

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: Catalin Marinas <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: Marc Zyngier <[email protected]>
Cc: Peng Donglin <[email protected]>
Cc: <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
---
arch/arm64/include/asm/ptdump.h | 9 +++------
arch/arm64/mm/dump.c | 4 ++--
arch/arm64/mm/ptdump_debugfs.c | 7 ++-----
3 files changed, 7 insertions(+), 13 deletions(-)

diff --git a/arch/arm64/include/asm/ptdump.h b/arch/arm64/include/asm/ptdump.h
index 6afd8476c60c..9e948a93d26c 100644
--- a/arch/arm64/include/asm/ptdump.h
+++ b/arch/arm64/include/asm/ptdump.h
@@ -34,13 +34,10 @@ struct ptdump_info {

void ptdump_walk_pgd(struct seq_file *s, struct ptdump_info *info);
#ifdef CONFIG_ARM64_PTDUMP_DEBUGFS
-int ptdump_debugfs_register(struct ptdump_info *info, const char *name);
+void ptdump_debugfs_register(struct ptdump_info *info, const char *name);
#else
-static inline int ptdump_debugfs_register(struct ptdump_info *info,
- const char *name)
-{
- return 0;
-}
+static inline void ptdump_debugfs_register(struct ptdump_info *info,
+ const char *name) { }
#endif
void ptdump_check_wx(void);
#endif /* CONFIG_ARM64_PTDUMP_CORE */
diff --git a/arch/arm64/mm/dump.c b/arch/arm64/mm/dump.c
index fcb1f2a6d7c6..08c250350b8a 100644
--- a/arch/arm64/mm/dump.c
+++ b/arch/arm64/mm/dump.c
@@ -407,7 +407,7 @@ void ptdump_check_wx(void)
static int ptdump_init(void)
{
ptdump_initialize();
- return ptdump_debugfs_register(&kernel_ptdump_info,
- "kernel_page_tables");
+ ptdump_debugfs_register(&kernel_ptdump_info, "kernel_page_tables");
+ return 0;
}
device_initcall(ptdump_init);
diff --git a/arch/arm64/mm/ptdump_debugfs.c b/arch/arm64/mm/ptdump_debugfs.c
index 24d786fc3a4c..064163f25592 100644
--- a/arch/arm64/mm/ptdump_debugfs.c
+++ b/arch/arm64/mm/ptdump_debugfs.c
@@ -12,10 +12,7 @@ static int ptdump_show(struct seq_file *m, void *v)
}
DEFINE_SHOW_ATTRIBUTE(ptdump);

-int ptdump_debugfs_register(struct ptdump_info *info, const char *name)
+void ptdump_debugfs_register(struct ptdump_info *info, const char *name)
{
- struct dentry *pe;
- pe = debugfs_create_file(name, 0400, NULL, info, &ptdump_fops);
- return pe ? 0 : -ENOMEM;
-
+ debugfs_create_file(name, 0400, NULL, info, &ptdump_fops);
}
--
2.20.1

2019-01-22 21:28:16

by Kees Cook

[permalink] [raw]

Subject: Re: [PATCH 2/4] arm: dump: no need to check return value of debugfs_create functions

On Wed, Jan 23, 2019 at 3:41 AM Greg Kroah-Hartman
<[email protected]> wrote:
>
> 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: Russell King <[email protected]>
> Cc: Jinbum Park <[email protected]>
> Cc: Kees Cook <[email protected]>
> Cc: Laura Abbott <[email protected]>
> Cc: [email protected]
> Signed-off-by: Greg Kroah-Hartman <[email protected]>

Do you mean that it bubbles up to an initcall, and the return value is
meaningless there? This seems fine to me...

Reviewed-by: Kees Cook <[email protected]>

-Kees

> ---
> arch/arm/include/asm/ptdump.h | 9 +++------
> arch/arm/mm/dump.c | 4 ++--
> arch/arm/mm/ptdump_debugfs.c | 8 ++------
> 3 files changed, 7 insertions(+), 14 deletions(-)
>
> diff --git a/arch/arm/include/asm/ptdump.h b/arch/arm/include/asm/ptdump.h
> index 3ebf9718288d..0c2d3d0d4cc6 100644
> --- a/arch/arm/include/asm/ptdump.h
> +++ b/arch/arm/include/asm/ptdump.h
> @@ -21,13 +21,10 @@ struct ptdump_info {
>
> void ptdump_walk_pgd(struct seq_file *s, struct ptdump_info *info);
> #ifdef CONFIG_ARM_PTDUMP_DEBUGFS
> -int ptdump_debugfs_register(struct ptdump_info *info, const char *name);
> +void ptdump_debugfs_register(struct ptdump_info *info, const char *name);
> #else
> -static inline int ptdump_debugfs_register(struct ptdump_info *info,
> - const char *name)
> -{
> - return 0;
> -}
> +static inline void ptdump_debugfs_register(struct ptdump_info *info,
> + const char *name) { }
> #endif /* CONFIG_ARM_PTDUMP_DEBUGFS */
>
> void ptdump_check_wx(void);
> diff --git a/arch/arm/mm/dump.c b/arch/arm/mm/dump.c
> index 084779c5c893..eb385a500ed0 100644
> --- a/arch/arm/mm/dump.c
> +++ b/arch/arm/mm/dump.c
> @@ -450,7 +450,7 @@ void ptdump_check_wx(void)
> static int ptdump_init(void)
> {
> ptdump_initialize();
> - return ptdump_debugfs_register(&kernel_ptdump_info,
> - "kernel_page_tables");
> + ptdump_debugfs_register(&kernel_ptdump_info, "kernel_page_tables");
> + return 0;
> }
> __initcall(ptdump_init);
> diff --git a/arch/arm/mm/ptdump_debugfs.c b/arch/arm/mm/ptdump_debugfs.c
> index be8d87be4b93..598b636615a2 100644
> --- a/arch/arm/mm/ptdump_debugfs.c
> +++ b/arch/arm/mm/ptdump_debugfs.c
> @@ -24,11 +24,7 @@ static const struct file_operations ptdump_fops = {
> .release = single_release,
> };
>
> -int ptdump_debugfs_register(struct ptdump_info *info, const char *name)
> +void ptdump_debugfs_register(struct ptdump_info *info, const char *name)
> {
> - struct dentry *pe;
> -
> - pe = debugfs_create_file(name, 0400, NULL, info, &ptdump_fops);
> - return pe ? 0 : -ENOMEM;
> -
> + debugfs_create_file(name, 0400, NULL, info, &ptdump_fops);
> }
> --
> 2.20.1
>

--
Kees Cook

2019-01-22 23:10:07

by Tony Lindgren

[permalink] [raw]

Subject: Re: [PATCH 3/4] arm: omap1: no need to check return value of debugfs_create functions

* Greg Kroah-Hartman <[email protected]> [190122 14:41]:
> 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.

Acked-by: Tony Lindgren <[email protected]>

2019-01-22 23:10:10

by Tony Lindgren

[permalink] [raw]

Subject: Re: [PATCH 4/4] arm: omap2: no need to check return value of debugfs_create functions

2019-01-23 09:44:59

by Laura Abbott

[permalink] [raw]

Subject: Re: [PATCH 2/4] arm: dump: no need to check return value of debugfs_create functions

On 1/22/19 6:41 AM, Greg Kroah-Hartman wrote:
> 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: Russell King <[email protected]>
> Cc: Jinbum Park <[email protected]>
> Cc: Kees Cook <[email protected]>
> Cc: Laura Abbott <[email protected]>

Acked-by: Laura Abbott <[email protected]>

> Cc: [email protected]
> Signed-off-by: Greg Kroah-Hartman <[email protected]>
> ---
> arch/arm/include/asm/ptdump.h | 9 +++------
> arch/arm/mm/dump.c | 4 ++--
> arch/arm/mm/ptdump_debugfs.c | 8 ++------
> 3 files changed, 7 insertions(+), 14 deletions(-)
>
> diff --git a/arch/arm/include/asm/ptdump.h b/arch/arm/include/asm/ptdump.h
> index 3ebf9718288d..0c2d3d0d4cc6 100644
> --- a/arch/arm/include/asm/ptdump.h
> +++ b/arch/arm/include/asm/ptdump.h
> @@ -21,13 +21,10 @@ struct ptdump_info {
>
> void ptdump_walk_pgd(struct seq_file *s, struct ptdump_info *info);
> #ifdef CONFIG_ARM_PTDUMP_DEBUGFS
> -int ptdump_debugfs_register(struct ptdump_info *info, const char *name);
> +void ptdump_debugfs_register(struct ptdump_info *info, const char *name);
> #else
> -static inline int ptdump_debugfs_register(struct ptdump_info *info,
> - const char *name)
> -{
> - return 0;
> -}
> +static inline void ptdump_debugfs_register(struct ptdump_info *info,
> + const char *name) { }
> #endif /* CONFIG_ARM_PTDUMP_DEBUGFS */
>
> void ptdump_check_wx(void);
> diff --git a/arch/arm/mm/dump.c b/arch/arm/mm/dump.c
> index 084779c5c893..eb385a500ed0 100644
> --- a/arch/arm/mm/dump.c
> +++ b/arch/arm/mm/dump.c
> @@ -450,7 +450,7 @@ void ptdump_check_wx(void)
> static int ptdump_init(void)
> {
> ptdump_initialize();
> - return ptdump_debugfs_register(&kernel_ptdump_info,
> - "kernel_page_tables");
> + ptdump_debugfs_register(&kernel_ptdump_info, "kernel_page_tables");
> + return 0;
> }
> __initcall(ptdump_init);
> diff --git a/arch/arm/mm/ptdump_debugfs.c b/arch/arm/mm/ptdump_debugfs.c
> index be8d87be4b93..598b636615a2 100644
> --- a/arch/arm/mm/ptdump_debugfs.c
> +++ b/arch/arm/mm/ptdump_debugfs.c
> @@ -24,11 +24,7 @@ static const struct file_operations ptdump_fops = {
> .release = single_release,
> };
>
> -int ptdump_debugfs_register(struct ptdump_info *info, const char *name)
> +void ptdump_debugfs_register(struct ptdump_info *info, const char *name)
> {
> - struct dentry *pe;
> -
> - pe = debugfs_create_file(name, 0400, NULL, info, &ptdump_fops);
> - return pe ? 0 : -ENOMEM;
> -
> + debugfs_create_file(name, 0400, NULL, info, &ptdump_fops);
> }
>

2019-01-25 18:14:14

by Catalin Marinas

[permalink] [raw]

Subject: Re: [PATCH 1/4] arm64: dump: no need to check return value of debugfs_create functions

On Tue, Jan 22, 2019 at 03:41:11PM +0100, Greg Kroah-Hartman wrote:
> 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: Catalin Marinas <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: Marc Zyngier <[email protected]>
> Cc: Peng Donglin <[email protected]>
> Cc: <[email protected]>
> Signed-off-by: Greg Kroah-Hartman <[email protected]>

Acked-by: Catalin Marinas <[email protected]>

2019-01-30 18:21:47

by Will Deacon

[permalink] [raw]

Subject: Re: [PATCH 1/4] arm64: dump: no need to check return value of debugfs_create functions

Hi Greg,

On Tue, Jan 22, 2019 at 03:41:11PM +0100, Greg Kroah-Hartman wrote:
> 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: Catalin Marinas <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: Marc Zyngier <[email protected]>
> Cc: Peng Donglin <[email protected]>
> Cc: <[email protected]>
> Signed-off-by: Greg Kroah-Hartman <[email protected]>
> ---
> arch/arm64/include/asm/ptdump.h | 9 +++------
> arch/arm64/mm/dump.c | 4 ++--
> arch/arm64/mm/ptdump_debugfs.c | 7 ++-----
> 3 files changed, 7 insertions(+), 13 deletions(-)

Are you taking this via your tree, or would you like us to queue it in
arm64?

Will

2019-01-30 19:34:59

by Greg Kroah-Hartman

[permalink] [raw]

Subject: Re: [PATCH 1/4] arm64: dump: no need to check return value of debugfs_create functions

On Wed, Jan 30, 2019 at 06:21:02PM +0000, Will Deacon wrote:
> Hi Greg,
>
> On Tue, Jan 22, 2019 at 03:41:11PM +0100, Greg Kroah-Hartman wrote:
> > 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: Catalin Marinas <[email protected]>
> > Cc: Will Deacon <[email protected]>
> > Cc: Marc Zyngier <[email protected]>
> > Cc: Peng Donglin <[email protected]>
> > Cc: <[email protected]>
> > Signed-off-by: Greg Kroah-Hartman <[email protected]>
> > ---
> > arch/arm64/include/asm/ptdump.h | 9 +++------
> > arch/arm64/mm/dump.c | 4 ++--
> > arch/arm64/mm/ptdump_debugfs.c | 7 ++-----
> > 3 files changed, 7 insertions(+), 13 deletions(-)
>
> Are you taking this via your tree, or would you like us to queue it in
> arm64?

What ever is easiest for you is fine with me, I can handle it either
way, just let me know.

thanks,

greg k-h

2019-01-31 13:58:26

by Will Deacon

[permalink] [raw]

Subject: Re: [PATCH 1/4] arm64: dump: no need to check return value of debugfs_create functions

On Wed, Jan 30, 2019 at 08:33:48PM +0100, Greg Kroah-Hartman wrote:
> On Wed, Jan 30, 2019 at 06:21:02PM +0000, Will Deacon wrote:
> > Hi Greg,
> >
> > On Tue, Jan 22, 2019 at 03:41:11PM +0100, Greg Kroah-Hartman wrote:
> > > 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: Catalin Marinas <[email protected]>
> > > Cc: Will Deacon <[email protected]>
> > > Cc: Marc Zyngier <[email protected]>
> > > Cc: Peng Donglin <[email protected]>
> > > Cc: <[email protected]>
> > > Signed-off-by: Greg Kroah-Hartman <[email protected]>
> > > ---
> > > arch/arm64/include/asm/ptdump.h | 9 +++------
> > > arch/arm64/mm/dump.c | 4 ++--
> > > arch/arm64/mm/ptdump_debugfs.c | 7 ++-----
> > > 3 files changed, 7 insertions(+), 13 deletions(-)
> >
> > Are you taking this via your tree, or would you like us to queue it in
> > arm64?
>
> What ever is easiest for you is fine with me, I can handle it either
> way, just let me know.

It's easy enough for us to merge the arm64 bits, so we'll pick that up for
5.1.

Cheers,

Will