[PATCH mtd-utils] nandmarkbad: new util to mark blocks as bad

[PATCH mtd-utils] nandmarkbad: new util to mark blocks as bad

[Uwe Kleine-König]

---
 .gitignore               |  1 +
 nand-utils/Makemodule.am |  5 ++-
 nand-utils/nandmarkbad.c | 95 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 100 insertions(+), 1 deletion(-)
 create mode 100644 nand-utils/nandmarkbad.c

diff --git a/.gitignore b/.gitignore
index 38bd04deb34e..754da97b43bf 100644
--- a/.gitignore
+++ b/.gitignore
@@ -36,6 +36,7 @@ mkfs.jffs2
 mtd_debug
 mtdpart
 nanddump
+nandmarkbad
 nandtest
 nandwrite
 nftl_format
diff --git a/nand-utils/Makemodule.am b/nand-utils/Makemodule.am
index d75b0cb3e36c..c31dcb01f06e 100644
--- a/nand-utils/Makemodule.am
+++ b/nand-utils/Makemodule.am
@@ -7,6 +7,9 @@ nandwrite_LDADD = libmtd.a
 nandtest_SOURCES = nand-utils/nandtest.c
 nandtest_LDADD = libmtd.a
 
+nandmarkbad_SOURCES = nand-utils/nandmarkbad.c
+nandmarkbad_LDADD = libmtd.a
+
 nftldump_SOURCES = nand-utils/nftldump.c
 nftldump_LDADD = libmtd.a
 
@@ -14,7 +17,7 @@ nftl_format_SOURCES = nand-utils/nftl_format.c
 nftl_format_LDADD = libmtd.a
 
 NAND_BINS = \
-	nanddump nandwrite nandtest nftldump nftl_format
+	nanddump nandwrite nandtest nandmarkbad nftldump nftl_format
 
 NAND_SH = \
 	nand-utils/load_nandsim.sh
diff --git a/nand-utils/nandmarkbad.c b/nand-utils/nandmarkbad.c
new file mode 100644
index 000000000000..023bdbe50c4d
--- /dev/null
+++ b/nand-utils/nandmarkbad.c
@@ -0,0 +1,95 @@
+#define PROGRAM_NAME "nandmarkbad"
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <getopt.h>
+
+#include "common.h"
+#include <libmtd.h>
+
+void usage(int status)
+{
+	fprintf(status ? stderr : stdout,
+		"usage: %s [OPTIONS] <device>\n\n"
+		"  -b, --markbad=blockno        Mark block bad\n"
+		PROGRAM_NAME);
+	exit(status);
+}
+
+/*
+ * Main program
+ */
+int main(int argc, char **argv)
+{
+	loff_t mark_bad[32];
+	unsigned cnt_bad = 0;
+	struct mtd_dev_info mtd;
+	libmtd_t mtd_desc;
+	int fd;
+	int error = 0;
+	int ret;
+	unsigned int i;
+
+	for (;;) {
+		static const char short_options[] = "b:h";
+		static const struct option long_options[] = {
+			{ "markbad", required_argument, 0, 'b' },
+			{0, 0, 0, 0},
+		};
+		int option_index = 0;
+		int c = getopt_long(argc, argv, short_options, long_options,
+				    &option_index);
+		if (c == EOF)
+			break;
+
+		switch (c) {
+		case 'h':
+			usage(EXIT_SUCCESS);
+			break;
+		case '?':
+			usage(EXIT_FAILURE);
+			break;
+
+		case 'b':
+			if (cnt_bad < ARRAY_SIZE(mark_bad)) {
+				mark_bad[cnt_bad] =
+					simple_strtoll(optarg, &error);
+				++cnt_bad;
+			} else {
+				errmsg_die("Can't handle so many bad blocks\n");
+			}
+
+			break;
+		}
+	}
+
+	argc -= optind;
+	argv += optind;
+
+	if (argc != 1 || error)
+		usage(EXIT_FAILURE);
+
+	if (!cnt_bad)
+		errmsg_die("You must specify at least one block to mark bad\n");
+
+	fd = open(argv[0], O_RDWR);
+	if (fd < 0)
+		sys_errmsg_die("Failed to open mtd device\n");
+
+	mtd_desc = libmtd_open();
+	if (!mtd_desc)
+		errmsg_die("Can't initialize libmtd");
+
+	if (mtd_get_dev_info(mtd_desc, argv[0], &mtd) < 0)
+		errmsg_die("mtd_get_dev_info failed");
+
+	for (i = 0; i < cnt_bad; ++i) {
+		ret = mtd_mark_bad(&mtd, fd, mark_bad[i]);
+		if (ret)
+			sys_errmsg_die("%s: MTD Mark bad block failure",
+				       argv[0]);
+	}
+
+	return EXIT_SUCCESS;
+}
-- 
2.14.1

Re: [PATCH mtd-utils] nandmarkbad: new util to mark blocks as bad

[David Oberhollenzer]

Hi,


On 08/24/2017 05:22 PM, Uwe Kleine-König wrote:
> +void usage(int status)
> +{
> +	fprintf(status ? stderr : stdout,
> +		"usage: %s [OPTIONS] <device>\n\n"
> +		"  -b, --markbad=blockno        Mark block bad\n"
> +		PROGRAM_NAME);
I assume there's a comma missing? Running the program without
arguments segfaults immediately on my system.


> +	exit(status);
> +}
> +
> +/*
> + * Main program
> + */
> +int main(int argc, char **argv)
> +{
> +	loff_t mark_bad[32];
> +	unsigned cnt_bad = 0;
> +	struct mtd_dev_info mtd;
> +	libmtd_t mtd_desc;
> +	int fd;
> +	int error = 0;
> +	int ret;
> +	unsigned int i;
> +
> +	for (;;) {
> +		static const char short_options[] = "b:h";
> +		static const struct option long_options[] = {
> +			{ "markbad", required_argument, 0, 'b' },
> +			{0, 0, 0, 0},
> +		};
> +		int option_index = 0;
> +		int c = getopt_long(argc, argv, short_options, long_options,
> +				    &option_index);
> +		if (c == EOF)
> +			break;

A "--help" long option in addition to the "-h" short option would be nice.
And additionally a "--version" long option with a "-V" short option.

There is a helper function called "common_print_version" that basically prints
'PROGRAM_NAME (mtd-utils) PACKAGE_VERSION'.


Thanks,

David

[PATCH mtd-utils v2] nandmarkbad: new util to mark blocks as bad

[Uwe Kleine-König]

---
changes since (implicit) v1 sent with Message-Id: 20170824152227.30394-1-ukleinek@debian.org:

 - fix printf format resulting in crash
   (suggested by gcc and David Oberhollenzer)
 - add options --help, --version, -V (suggested by David Oberhollenzer)
 - add options --i-know-what-i-do, -y (suggested by Richard Weinberger)
 - add an error message if no device was given
 - make usage static
 - use right author email address

 .gitignore               |  1 +
 nand-utils/Makemodule.am |  5 ++-
 nand-utils/nandmarkbad.c | 95 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 100 insertions(+), 1 deletion(-)
 create mode 100644 nand-utils/nandmarkbad.c

diff --git a/.gitignore b/.gitignore
index 38bd04deb34e..754da97b43bf 100644
--- a/.gitignore
+++ b/.gitignore
@@ -36,6 +36,7 @@ mkfs.jffs2
 mtd_debug
 mtdpart
 nanddump
+nandmarkbad
 nandtest
 nandwrite
 nftl_format
diff --git a/nand-utils/Makemodule.am b/nand-utils/Makemodule.am
index d75b0cb3e36c..c31dcb01f06e 100644
--- a/nand-utils/Makemodule.am
+++ b/nand-utils/Makemodule.am
@@ -7,6 +7,9 @@ nandwrite_LDADD = libmtd.a
 nandtest_SOURCES = nand-utils/nandtest.c
 nandtest_LDADD = libmtd.a
 
+nandmarkbad_SOURCES = nand-utils/nandmarkbad.c
+nandmarkbad_LDADD = libmtd.a
+
 nftldump_SOURCES = nand-utils/nftldump.c
 nftldump_LDADD = libmtd.a
 
@@ -14,7 +17,7 @@ nftl_format_SOURCES = nand-utils/nftl_format.c
 nftl_format_LDADD = libmtd.a
 
 NAND_BINS = \
-	nanddump nandwrite nandtest nftldump nftl_format
+	nanddump nandwrite nandtest nandmarkbad nftldump nftl_format
 
 NAND_SH = \
 	nand-utils/load_nandsim.sh
diff --git a/nand-utils/nandmarkbad.c b/nand-utils/nandmarkbad.c
new file mode 100644
index 000000000000..023bdbe50c4d
--- /dev/null
+++ b/nand-utils/nandmarkbad.c
@@ -0,0 +1,95 @@
+#define PROGRAM_NAME "nandmarkbad"
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <getopt.h>
+
+#include "common.h"
+#include <libmtd.h>
+
+void usage(int status)
+{
+	fprintf(status ? stderr : stdout,
+		"usage: %s [OPTIONS] <device>\n\n"
+		"  -b, --markbad=blockno        Mark block bad\n"
+		PROGRAM_NAME);
+	exit(status);
+}
+
+/*
+ * Main program
+ */
+int main(int argc, char **argv)
+{
+	loff_t mark_bad[32];
+	unsigned cnt_bad = 0;
+	struct mtd_dev_info mtd;
+	libmtd_t mtd_desc;
+	int fd;
+	int error = 0;
+	int ret;
+	unsigned int i;
+
+	for (;;) {
+		static const char short_options[] = "b:h";
+		static const struct option long_options[] = {
+			{ "markbad", required_argument, 0, 'b' },
+			{0, 0, 0, 0},
+		};
+		int option_index = 0;
+		int c = getopt_long(argc, argv, short_options, long_options,
+				    &option_index);
+		if (c == EOF)
+			break;
+
+		switch (c) {
+		case 'h':
+			usage(EXIT_SUCCESS);
+			break;
+		case '?':
+			usage(EXIT_FAILURE);
+			break;
+
+		case 'b':
+			if (cnt_bad < ARRAY_SIZE(mark_bad)) {
+				mark_bad[cnt_bad] =
+					simple_strtoll(optarg, &error);
+				++cnt_bad;
+			} else {
+				errmsg_die("Can't handle so many bad blocks\n");
+			}
+
+			break;
+		}
+	}
+
+	argc -= optind;
+	argv += optind;
+
+	if (argc != 1 || error)
+		usage(EXIT_FAILURE);
+
+	if (!cnt_bad)
+		errmsg_die("You must specify at least one block to mark bad\n");
+
+	fd = open(argv[0], O_RDWR);
+	if (fd < 0)
+		sys_errmsg_die("Failed to open mtd device\n");
+
+	mtd_desc = libmtd_open();
+	if (!mtd_desc)
+		errmsg_die("Can't initialize libmtd");
+
+	if (mtd_get_dev_info(mtd_desc, argv[0], &mtd) < 0)
+		errmsg_die("mtd_get_dev_info failed");
+
+	for (i = 0; i < cnt_bad; ++i) {
+		ret = mtd_mark_bad(&mtd, fd, mark_bad[i]);
+		if (ret)
+			sys_errmsg_die("%s: MTD Mark bad block failure",
+				       argv[0]);
+	}
+
+	return EXIT_SUCCESS;
+}
-- 
2.14.1

[PATCH mtd-utils v3] nandmarkbad: new util to mark blocks as bad

[Uwe Kleine-König]

---
changes since v2, sent with Message-Id: 20170830112145.26586-1-u.kleine-koenig@pengutronix.de:
 - actually committed the changes advertised for v2 (sigh)

changes since (implicit) v1 sent with Message-Id: 20170824152227.30394-1-ukleinek@debian.org:

 - fix printf format resulting in crash
   (suggested by gcc and David Oberhollenzer)
 - add options --help, --version, -V (suggested by David Oberhollenzer)
 - add options --i-know-what-i-do, -y (suggested by Richard Weinberger)
 - add an error message if no device was given
 - make usage static
 - use right author email address

 .gitignore               |   1 +
 nand-utils/Makemodule.am |   5 +-
 nand-utils/nandmarkbad.c | 119 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 124 insertions(+), 1 deletion(-)
 create mode 100644 nand-utils/nandmarkbad.c

diff --git a/.gitignore b/.gitignore
index 38bd04deb34e..754da97b43bf 100644
--- a/.gitignore
+++ b/.gitignore
@@ -36,6 +36,7 @@ mkfs.jffs2
 mtd_debug
 mtdpart
 nanddump
+nandmarkbad
 nandtest
 nandwrite
 nftl_format
diff --git a/nand-utils/Makemodule.am b/nand-utils/Makemodule.am
index d75b0cb3e36c..c31dcb01f06e 100644
--- a/nand-utils/Makemodule.am
+++ b/nand-utils/Makemodule.am
@@ -7,6 +7,9 @@ nandwrite_LDADD = libmtd.a
 nandtest_SOURCES = nand-utils/nandtest.c
 nandtest_LDADD = libmtd.a
 
+nandmarkbad_SOURCES = nand-utils/nandmarkbad.c
+nandmarkbad_LDADD = libmtd.a
+
 nftldump_SOURCES = nand-utils/nftldump.c
 nftldump_LDADD = libmtd.a
 
@@ -14,7 +17,7 @@ nftl_format_SOURCES = nand-utils/nftl_format.c
 nftl_format_LDADD = libmtd.a
 
 NAND_BINS = \
-	nanddump nandwrite nandtest nftldump nftl_format
+	nanddump nandwrite nandtest nandmarkbad nftldump nftl_format
 
 NAND_SH = \
 	nand-utils/load_nandsim.sh
diff --git a/nand-utils/nandmarkbad.c b/nand-utils/nandmarkbad.c
new file mode 100644
index 000000000000..cf05698c3609
--- /dev/null
+++ b/nand-utils/nandmarkbad.c
@@ -0,0 +1,119 @@
+#define PROGRAM_NAME "nandmarkbad"
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <getopt.h>
+
+#include "common.h"
+#include <libmtd.h>
+
+static void usage(int status)
+{
+	fprintf(status ? stderr : stdout,
+		"usage: %s [OPTIONS] <device>\n\n"
+		"  -b, --markbad=blockno        Mark block bad\n"
+		"  -h, --help                   Display this help and exit\n"
+		"  -V, --version                Output version information and exit\n"
+		"  -y, --i-know-what-i-do       really do mark blocks as bad\n",
+		PROGRAM_NAME);
+	exit(status);
+}
+
+/*
+ * Main program
+ */
+int main(int argc, char **argv)
+{
+	loff_t mark_bad[32];
+	unsigned cnt_bad = 0;
+	struct mtd_dev_info mtd;
+	libmtd_t mtd_desc;
+	int fd;
+	int error = 0;
+	int ret;
+	unsigned int i;
+	int iknowwhatido = 0;
+
+	for (;;) {
+		static const char short_options[] = "b:hVy";
+		static const struct option long_options[] = {
+			{ "help", no_argument, 0, 'h' },
+			{ "markbad", required_argument, 0, 'b' },
+			{ "version", no_argument, 0, 'V'},
+			{ "i-know-what-i-do", no_argument, 0, 'y' },
+			{0, 0, 0, 0},
+		};
+		int option_index = 0;
+		int c = getopt_long(argc, argv, short_options, long_options,
+				    &option_index);
+		if (c == EOF)
+			break;
+
+		switch (c) {
+		case '?':
+			usage(EXIT_FAILURE);
+			break;
+
+		case 'b':
+			if (cnt_bad < ARRAY_SIZE(mark_bad)) {
+				mark_bad[cnt_bad] =
+					simple_strtoll(optarg, &error);
+				++cnt_bad;
+			} else {
+				errmsg_die("Can't handle so many bad blocks\n");
+			}
+
+			break;
+
+		case 'h':
+			usage(EXIT_SUCCESS);
+			break;
+
+		case 'V':
+			common_print_version();
+			return EXIT_SUCCESS;
+
+		case 'y':
+			iknowwhatido = 1;
+			break;
+		}
+	}
+
+	argc -= optind;
+	argv += optind;
+
+	if (error)
+		usage(EXIT_FAILURE);
+
+	if (argc != 1)
+		errmsg_die("You must specify a device to operate on\n");
+
+	if (!cnt_bad)
+		errmsg_die("You must specify at least one block to mark bad\n");
+
+	if (!iknowwhatido)
+		errmsg_die(PROGRAM_NAME " does things that are hard to undo.\n"
+			   "\tPlease convince yourself you understand the risks,\n"
+			   "\tthen add --i-know-what-i-do to the options.\n");
+
+	fd = open(argv[0], O_RDWR);
+	if (fd < 0)
+		sys_errmsg_die("Failed to open mtd device\n");
+
+	mtd_desc = libmtd_open();
+	if (!mtd_desc)
+		errmsg_die("Can't initialize libmtd");
+
+	if (mtd_get_dev_info(mtd_desc, argv[0], &mtd) < 0)
+		errmsg_die("mtd_get_dev_info failed");
+
+	for (i = 0; i < cnt_bad; ++i) {
+		ret = mtd_mark_bad(&mtd, fd, mark_bad[i]);
+		if (ret)
+			sys_errmsg_die("%s: MTD Mark bad block failure",
+				       argv[0]);
+	}
+
+	return EXIT_SUCCESS;
+}
-- 
2.14.1

Re: [PATCH mtd-utils v3] nandmarkbad: new util to mark blocks as bad

[David Oberhollenzer]

Hi,


sorry for the late reply. After requesting the initial changes, I caught up on
the IRC discussion and came under the impression that it was pretty clear to
everyone that there are a few major issues with this kind of tool.

First of all, marking a block bad can currently not be undone. The changes this
tool makes are thus both destructive and irreversible.

If the MTD contains UBI data, UBI has no way of moving the data somewhere else
if a block just randomly goes bad completely out of the blue.

Besides, if the MTD is attached to an UBI device, UBI already takes care of
detecting blocks that go bad *over time* and takes care of marking them as
bad after moving the data somewhere else.

It might be interesting to have such a tool if nand scrub functionality was
added to the kernel (such as in [1]), allowing the changes done by the tool
to be undone. But even then, what would be the practical use case for
such a tool?


Thanks,

David


[1] http://patchwork.ozlabs.org/patch/764978/

Re: [PATCH mtd-utils v3] nandmarkbad: new util to mark blocks as bad

[Uwe Kleine-König]

Hello David,

On Wed, Sep 13, 2017 at 11:39:31AM +0200, David Oberhollenzer wrote:
> 
> sorry for the late reply. After requesting the initial changes, I caught up on
> the IRC discussion and came under the impression that it was pretty clear to
> everyone that there are a few major issues with this kind of tool.
> 
> First of all, marking a block bad can currently not be undone. The changes this
> tool makes are thus both destructive and irreversible.
> 
> If the MTD contains UBI data, UBI has no way of moving the data somewhere else
> if a block just randomly goes bad completely out of the blue.
> 
> Besides, if the MTD is attached to an UBI device, UBI already takes care of
> detecting blocks that go bad *over time* and takes care of marking them as
> bad after moving the data somewhere else.
> 
> It might be interesting to have such a tool if nand scrub functionality was
> added to the kernel (such as in [1]), allowing the changes done by the tool
> to be undone. But even then, what would be the practical use case for
> such a tool?

I needed it as part of reproducing a problem I was told to fix. I only
used it on a nandsim device for testing. I could imagine though that the
tool could also come handy while developping a mtd driver. But yes, it
would be more useful in combination with a possibility to unmark a bad
block.

In sum: I don't care much if you take it or not. I just thought it might
come handy for someone else and so I shared my work.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |