mtd: no need to check return value of debugfs_create functions

PATCH Applied Greg Kroah-Hartman 2 participants 5 messages Nov 7, 2019 – Nov 12, 2019

Subsystems (2)

Memory technology devices (MTD) Unsorted block images (UBI)

lore

Share & tools

lore

Greg Kroah-Hartman

author

2019-11-07 08:51 UTC #0 lore raw

PATCH 1/1 mtd: 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 David Woodhouse <[email protected]>

Cc Brian Norris <[email protected]>

Cc Marek Vasut <[email protected]>

Cc Miquel Raynal <[email protected]>

Cc Richard Weinberger <[email protected]>

Cc Vignesh Raghavendra <[email protected]>

Cc Artem Bityutskiy <[email protected]>

Cc [email protected]

Signed-off-by Greg Kroah-Hartman <[email protected]>

---
 drivers/mtd/mtdcore.c   |  26 +++-----
 drivers/mtd/mtdswap.c   |   8 +--
 drivers/mtd/ubi/debug.c | 128 +++++++++++++---------------------------
 3 files changed, 49 insertions(+), 113 deletions(-)

drivers/mtd/mtdcore.c +7 ~~-19~~

index 6cc7ecb0c788..5fac4355b9c2 100644
@@ -382,33 +382,21 @@ static struct dentry *dfs_dir_mtd;
 static void mtd_debugfs_populate(struct mtd_info *mtd)
 {
 	struct device *dev = &mtd->dev;
-	struct dentry *root, *dent;
+	struct dentry *root;
 
 	if (IS_ERR_OR_NULL(dfs_dir_mtd))
 		return;
 
 	root = debugfs_create_dir(dev_name(dev), dfs_dir_mtd);
-	if (IS_ERR_OR_NULL(root)) {
-		dev_dbg(dev, "won't show data in debugfs\n");
-		return;
-	}
-
 	mtd->dbg.dfs_dir = root;
 
-	if (mtd->dbg.partid) {
-		dent = debugfs_create_file("partid", 0400, root, mtd,
-					   &mtd_partid_debug_fops);
-		if (IS_ERR_OR_NULL(dent))
-			dev_err(dev, "can't create debugfs entry for partid\n");
-	}
+	if (mtd->dbg.partid)
+		debugfs_create_file("partid", 0400, root, mtd,
+				    &mtd_partid_debug_fops);
 
-	if (mtd->dbg.partname) {
-		dent = debugfs_create_file("partname", 0400, root, mtd,
-					   &mtd_partname_debug_fops);
-		if (IS_ERR_OR_NULL(dent))
-			dev_err(dev,
-				"can't create debugfs entry for partname\n");
-	}
+	if (mtd->dbg.partname)
+		debugfs_create_file("partname", 0400, root, mtd,
+				    &mtd_partname_debug_fops);
 }
 
 #ifndef CONFIG_MMU

drivers/mtd/mtdswap.c +1 -7

index f92414eb4c86..58eefa43af14 100644
@@ -1257,7 +1257,6 @@ DEFINE_SHOW_ATTRIBUTE(mtdswap);
 static int mtdswap_add_debugfs(struct mtdswap_dev *d)
 {
 	struct dentry *root = d->mtd->dbg.dfs_dir;
-	struct dentry *dent;
 
 	if (!IS_ENABLED(CONFIG_DEBUG_FS))
 		return 0;

@@ -1265,12 +1264,7 @@ static int mtdswap_add_debugfs(struct mtdswap_dev *d)
 	if (IS_ERR_OR_NULL(root))
 		return -1;
 
-	dent = debugfs_create_file("mtdswap_stats", S_IRUSR, root, d,
-				&mtdswap_fops);
-	if (!dent) {
-		dev_err(d->dev, "debugfs_create_file failed\n");
-		return -1;
-	}
+	debugfs_create_file("mtdswap_stats", S_IRUSR, root, d, &mtdswap_fops);
 
 	return 0;
 }

drivers/mtd/ubi/debug.c +41 ~~-87~~

144 lines not on this page · raw message

Signature · 1 line

2.23.0

Miquel Raynal

maintainer

2019-11-07 09:09 UTC #1 lore raw

Hi Greg,

Greg Kroah-Hartman <[email protected]> wrote on Thu, 7 Nov 2019 09:51:11 +0100:

Quoted 3 lines

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.

I didn't know about this. Is this something new or has it been the rule since the beginning? In the case, don't we need a Fixes tag here?

Quoted 10 lines

Cc David Woodhouse <[email protected]>
Cc Brian Norris <[email protected]>
Cc Marek Vasut <[email protected]>
Cc Miquel Raynal <[email protected]>
Cc Richard Weinberger <[email protected]>
Cc Vignesh Raghavendra <[email protected]>
Cc Artem Bityutskiy <[email protected]>
Cc [email protected]
Cc [email protected]
Signed-off-by Greg Kroah-Hartman <[email protected]>

[...]

drivers/mtd/ubi/debug.c int ubi_debugfs_init_dev(struct ubi_device *ubi) · line 555

Quoted 10 lines

drivers/mtd/ubi/debug.c +10 -0

+
+	d->dfs_emulate_io_failures = debugfs_create_file("tst_emulate_io_failures",
+							 S_IWUSR, d->dfs_dir,
+							 (void *)ubi_num,
+							 &dfs_fops);
+
+	d->dfs_emulate_power_cut = debugfs_create_file("tst_emulate_power_cut",
+						       S_IWUSR, d->dfs_dir,
+						       (void *)ubi_num,
+						       &dfs_fops);

Nitpick: I think we miss an empty line here. I can fix it when applying.

Quoted 5 lines

drivers/mtd/ubi/debug.c +5 -0

+	d->dfs_power_cut_min = debugfs_create_file("tst_emulate_power_cut_min",
+						   S_IWUSR, d->dfs_dir,
+						   (void *)ubi_num, &dfs_fops);
+
+	d->dfs_power_cut_max = debugfs_create_file("tst_emulate_power_cut_max",

drivers/mtd/ubi/debug.c int ubi_debugfs_init_dev(struct ubi_device *ubi) · line 570

Quoted 10 lines

drivers/mtd/ubi/debug.c +7 -0

+						   S_IWUSR, d->dfs_dir,
+						   (void *)ubi_num, &dfs_fops);
+
+	debugfs_create_file("detailed_erase_block_info", S_IRUSR, d->dfs_dir,
+			    (void *)ubi_num, &eraseblk_count_fops);
+
+	return 0;
 }
 
 /**

Thanks,
Miquèl

Greg Kroah-Hartman

author

2019-11-07 09:15 UTC #2 lore raw

Quoted 11 lines · Miquel Raynal

Hi Greg,
Greg Kroah-Hartman <[email protected]> wrote on Thu, 7 Nov 2019 09:51:11 +0100:
Quoted 3 lines
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.
I didn't know about this. Is this something new or has it been the rule since the beginning? In the case, don't we need a Fixes tag here?

It's been the way always, but as of a few kernel releases ago, debugfs is even more "fault-tolerant" of stuff like this.

And there's no need for a "Fixes:" as this is just work to clean up the debugfs api and usage (I have a lot more work to do after these types of changes.)

Quoted 14 lines

Quoted 11 lines
Cc David Woodhouse <[email protected]>
Cc Brian Norris <[email protected]>
Cc Marek Vasut <[email protected]>
Cc Miquel Raynal <[email protected]>
Cc Richard Weinberger <[email protected]>
Cc Vignesh Raghavendra <[email protected]>
Cc Artem Bityutskiy <[email protected]>
Cc [email protected]
Cc [email protected]
Signed-off-by Greg Kroah-Hartman <[email protected]>
[...]

Quoted 12 lines

drivers/mtd/ubi/debug.c int ubi_debugfs_init_dev(struct ubi_device *ubi) · line 555
Quoted 10 lines
drivers/mtd/ubi/debug.c +10 -0
+
+	d->dfs_emulate_io_failures = debugfs_create_file("tst_emulate_io_failures",
+							 S_IWUSR, d->dfs_dir,
+							 (void *)ubi_num,
+							 &dfs_fops);
+
+	d->dfs_emulate_power_cut = debugfs_create_file("tst_emulate_power_cut",
+						       S_IWUSR, d->dfs_dir,
+						       (void *)ubi_num,
+						       &dfs_fops);
Nitpick: I think we miss an empty line here. I can fix it when applying.

Ah, oops, sorry about that.

thanks,

greg k-h

Miquel Raynal

maintainer

2019-11-07 10:13 UTC #3 lore raw

Hi Greg,

Greg Kroah-Hartman <[email protected]> wrote on Thu, 7 Nov 2019 10:15:18 +0100:

Quoted 13 lines · Miquel Raynal

Quoted 11 lines
Hi Greg,
Greg Kroah-Hartman <[email protected]> wrote on Thu, 7 Nov 2019 09:51:11 +0100:
Quoted 3 lines
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.
I didn't know about this. Is this something new or has it been the rule since the beginning? In the case, don't we need a Fixes tag here?

Quoted 6 lines

It's been the way always, but as of a few kernel releases ago, debugfs is even more "fault-tolerant" of stuff like this.
And there's no need for a "Fixes:" as this is just work to clean up the debugfs api and usage (I have a lot more work to do after these types of changes.)

Ok, thanks for the clarification.

Cheers!
Miquèl

Quoted 33 lines

Quoted 15 lines
Quoted 12 lines
Cc David Woodhouse <[email protected]>
Cc Brian Norris <[email protected]>
Cc Marek Vasut <[email protected]>
Cc Miquel Raynal <[email protected]>
Cc Richard Weinberger <[email protected]>
Cc Vignesh Raghavendra <[email protected]>
Cc Artem Bityutskiy <[email protected]>
Cc [email protected]
Cc [email protected]
Signed-off-by Greg Kroah-Hartman <[email protected]>
[...]
Quoted 12 lines
drivers/mtd/ubi/debug.c int ubi_debugfs_init_dev(struct ubi_device *ubi) · line 555
Quoted 10 lines
drivers/mtd/ubi/debug.c +10 -0
+
+	d->dfs_emulate_io_failures = debugfs_create_file("tst_emulate_io_failures",
+							 S_IWUSR, d->dfs_dir,
+							 (void *)ubi_num,
+							 &dfs_fops);
+
+	d->dfs_emulate_power_cut = debugfs_create_file("tst_emulate_power_cut",
+						       S_IWUSR, d->dfs_dir,
+						       (void *)ubi_num,
+						       &dfs_fops);  
Nitpick: I think we miss an empty line here. I can fix it when applying.
Ah, oops, sorry about that.
thanks,
greg k-h

Miquel Raynal

maintainer Applied

2019-11-12 14:19 UTC #4 lore raw

In reply to Greg Kroah-Hartman

Quoted 4 lines · Greg Kroah-Hartman

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.

Quoted 10 lines

Cc David Woodhouse <[email protected]>
Cc Brian Norris <[email protected]>
Cc Marek Vasut <[email protected]>
Cc Miquel Raynal <[email protected]>
Cc Richard Weinberger <[email protected]>
Cc Vignesh Raghavendra <[email protected]>
Cc Artem Bityutskiy <[email protected]>
Cc [email protected]
Cc [email protected]
Signed-off-by Greg Kroah-Hartman <[email protected]>

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/next, thanks.

Miquel