[PATCH] mtd-utils: ubiformat: Add fastmap support

[PATCH] mtd-utils: ubiformat: Add fastmap support

[Sascha Hauer]

When flashing an image with ubiformat fastmap is not supported since
the first 64 eraseblocks are occupied with data, so the fastmap code
does not find a place to put the fastmap anchor. This series changes
this and allows to use fastmap with ubiformat flashed images.

Sascha Hauer (2):
      ubiformat: Factor out a write_eraseblock function
      ubiformat: Leave space for fastmap anchor

 ubi-utils/ubiformat.c | 251 +++++++++++++++++++++++++++++++++++++++++++++++++++++--------------------------------------------------------
 1 file changed, 121 insertions(+), 130 deletions(-)

[PATCH 1/2] ubiformat: Factor out a write_eraseblock function

[Sascha Hauer]

The format and the flash_image function share some code. Factor out a
write_erasblock function. This makes the code a bit shorter and makes
it easy to skip flashing a block for fastmap support.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 ubi-utils/ubiformat.c | 241 +++++++++++++++++++++++---------------------------
 1 file changed, 111 insertions(+), 130 deletions(-)

diff --git a/ubi-utils/ubiformat.c b/ubi-utils/ubiformat.c
index 21409ca..2269cdf 100644
--- a/ubi-utils/ubiformat.c
+++ b/ubi-utils/ubiformat.c
@@ -415,6 +415,105 @@ static int mark_bad(const struct mtd_dev_info *mtd, struct ubi_scan_info *si, in
 	return consecutive_bad_check(eb);
 }
 
+/*
+ * Write a single eraseblock. When buf is given its content is written to the
+ * eraseblock. When buf is NULL only the ec header is written. Returns 0 on
+ * success, 1 if this block should be skipped or -1 when an error occured.
+ */
+static int write_eraseblock(libmtd_t libmtd, const struct mtd_dev_info *mtd,
+		const struct ubigen_info *ui, struct ubi_scan_info *si, int eb,
+		void *buf, int written_ebs)
+{
+	int err, write_size;
+	struct ubi_ec_hdr *hdr = NULL;
+	long long ec;
+
+	if (args.override_ec)
+		ec = args.ec;
+	else if (si->ec[eb] <= EC_MAX)
+		ec = si->ec[eb] + 1;
+	else
+		ec = si->mean_ec;
+
+	if (args.verbose) {
+		normsg_cont("eraseblock %d: erase", eb);
+		fflush(stdout);
+	}
+
+	err = mtd_erase(libmtd, mtd, args.node_fd, eb);
+	if (err) {
+		if (!args.quiet)
+			printf("\n");
+
+		sys_errmsg("failed to erase eraseblock %d", eb);
+		if (errno != EIO)
+			return -1;
+
+		if (mark_bad(mtd, si, eb))
+			return -1;
+
+		return 1;
+	}
+
+	if (buf) {
+		write_size = mtd->eb_size;
+		err = change_ech((struct ubi_ec_hdr *)buf, ui->image_seq, ec);
+		if (err) {
+			errmsg("bad EC header at eraseblock %d of \"%s\"",
+			       written_ebs, args.image);
+			return -1;
+		}
+		write_size = drop_ffs(mtd, buf, mtd->eb_size);
+	} else {
+		write_size = UBI_EC_HDR_SIZE + mtd->subpage_size - 1;
+		write_size /= mtd->subpage_size;
+		write_size *= mtd->subpage_size;
+
+		hdr = malloc(write_size);
+		if (!hdr)
+			return sys_errmsg("cannot allocate %d bytes of memory", write_size);
+		memset(hdr, 0xFF, write_size);
+
+		ubigen_init_ec_hdr(ui, hdr, ec);
+
+		buf = hdr;
+	}
+
+	if (args.verbose) {
+		printf(", write %sEC %lld\n", hdr ? "" : "data, ", ec);
+		fflush(stdout);
+	}
+
+	err = mtd_write(libmtd, mtd, args.node_fd, eb, 0, buf,
+			write_size, NULL, 0, 0);
+	if (err) {
+		if (!args.quiet && !args.verbose)
+			printf("\n");
+		sys_errmsg("cannot write eraseblock %d", eb);
+
+		if (errno != EIO) {
+			if (args.subpage_size != mtd->min_io_size)
+				normsg("may be sub-page size is "
+				       "incorrect?");
+			err = -1;
+			goto out;
+		}
+
+		err = mtd_torture(libmtd, mtd, args.node_fd, eb);
+		if (err) {
+			if (mark_bad(mtd, si, eb)) {
+				err = -1;
+				goto out;
+			}
+		}
+		err = 1;
+	}
+
+out:
+	free(hdr);
+	return err;
+}
+
 static int flash_image(libmtd_t libmtd, const struct mtd_dev_info *mtd,
 		       const struct ubigen_info *ui, struct ubi_scan_info *si)
 {
@@ -442,9 +541,8 @@ static int flash_image(libmtd_t libmtd, const struct mtd_dev_info *mtd,
 	verbose(args.verbose, "will write %d eraseblocks", img_ebs);
 	divisor = img_ebs;
 	for (eb = 0; eb < mtd->eb_cnt; eb++) {
-		int err, new_len;
+		int err;
 		char buf[mtd->eb_size];
-		long long ec;
 
 		if (!args.quiet && !args.verbose) {
 			printf("\r" PROGRAM_NAME ": flashing eraseblock %d -- %2lld %% complete  ",
@@ -457,26 +555,6 @@ static int flash_image(libmtd_t libmtd, const struct mtd_dev_info *mtd,
 			continue;
 		}
 
-		if (args.verbose) {
-			normsg_cont("eraseblock %d: erase", eb);
-			fflush(stdout);
-		}
-
-		err = mtd_erase(libmtd, mtd, args.node_fd, eb);
-		if (err) {
-			if (!args.quiet)
-				printf("\n");
-			sys_errmsg("failed to erase eraseblock %d", eb);
-
-			if (errno != EIO)
-				goto out_close;
-
-			if (mark_bad(mtd, si, eb))
-				goto out_close;
-
-			continue;
-		}
-
 		if (!skip_data_read) {
 			err = read_all(fd, buf, mtd->eb_size);
 			if (err) {
@@ -487,46 +565,11 @@ static int flash_image(libmtd_t libmtd, const struct mtd_dev_info *mtd,
 		}
 		skip_data_read = 0;
 
-		if (args.override_ec)
-			ec = args.ec;
-		else if (si->ec[eb] <= EC_MAX)
-			ec = si->ec[eb] + 1;
-		else
-			ec = si->mean_ec;
-
-		if (args.verbose) {
-			printf(", change EC to %lld", ec);
-			fflush(stdout);
-		}
-
-		err = change_ech((struct ubi_ec_hdr *)buf, ui->image_seq, ec);
-		if (err) {
-			errmsg("bad EC header at eraseblock %d of \"%s\"",
-			       written_ebs, args.image);
+		err = write_eraseblock(libmtd, mtd, ui, si, eb, buf, written_ebs);
+		if (err < 0)
 			goto out_close;
-		}
-
-		if (args.verbose) {
-			printf(", write data\n");
-			fflush(stdout);
-		}
-
-		new_len = drop_ffs(mtd, buf, mtd->eb_size);
-
-		err = mtd_write(libmtd, mtd, args.node_fd, eb, 0, buf, new_len,
-				NULL, 0, 0);
-		if (err) {
-			sys_errmsg("cannot write eraseblock %d", eb);
-
-			if (errno != EIO)
-				goto out_close;
-
-			err = mtd_torture(libmtd, mtd, args.node_fd, eb);
-			if (err) {
-				if (mark_bad(mtd, si, eb))
-					goto out_close;
-			}
 
+		if (err > 0) {
 			/*
 			 * We have to make sure that we do not read next block
 			 * of data from the input image or stdin - we have to
@@ -553,20 +596,11 @@ static int format(libmtd_t libmtd, const struct mtd_dev_info *mtd,
 		  const struct ubigen_info *ui, struct ubi_scan_info *si,
 		  int start_eb, int novtbl)
 {
-	int eb, err, write_size;
-	struct ubi_ec_hdr *hdr;
+	int eb, err;
 	struct ubi_vtbl_record *vtbl;
 	int eb1 = -1, eb2 = -1;
 	long long ec1 = -1, ec2 = -1;
 
-	write_size = UBI_EC_HDR_SIZE + mtd->subpage_size - 1;
-	write_size /= mtd->subpage_size;
-	write_size *= mtd->subpage_size;
-	hdr = malloc(write_size);
-	if (!hdr)
-		return sys_errmsg("cannot allocate %d bytes of memory", write_size);
-	memset(hdr, 0xFF, write_size);
-
 	for (eb = start_eb; eb < mtd->eb_cnt; eb++) {
 		long long ec;
 
@@ -579,33 +613,6 @@ static int format(libmtd_t libmtd, const struct mtd_dev_info *mtd,
 		if (si->ec[eb] == EB_BAD)
 			continue;
 
-		if (args.override_ec)
-			ec = args.ec;
-		else if (si->ec[eb] <= EC_MAX)
-			ec = si->ec[eb] + 1;
-		else
-			ec = si->mean_ec;
-		ubigen_init_ec_hdr(ui, hdr, ec);
-
-		if (args.verbose) {
-			normsg_cont("eraseblock %d: erase", eb);
-			fflush(stdout);
-		}
-
-		err = mtd_erase(libmtd, mtd, args.node_fd, eb);
-		if (err) {
-			if (!args.quiet)
-				printf("\n");
-
-			sys_errmsg("failed to erase eraseblock %d", eb);
-			if (errno != EIO)
-				goto out_free;
-
-			if (mark_bad(mtd, si, eb))
-				goto out_free;
-			continue;
-		}
-
 		if ((eb1 == -1 || eb2 == -1) && !novtbl) {
 			if (eb1 == -1) {
 				eb1 = eb;
@@ -619,33 +626,9 @@ static int format(libmtd_t libmtd, const struct mtd_dev_info *mtd,
 			continue;
 		}
 
-		if (args.verbose) {
-			printf(", write EC %lld\n", ec);
-			fflush(stdout);
-		}
-
-		err = mtd_write(libmtd, mtd, args.node_fd, eb, 0, hdr,
-				write_size, NULL, 0, 0);
-		if (err) {
-			if (!args.quiet && !args.verbose)
-				printf("\n");
-			sys_errmsg("cannot write EC header (%d bytes buffer) to eraseblock %d",
-				   write_size, eb);
-
-			if (errno != EIO) {
-				if (args.subpage_size != mtd->min_io_size)
-					normsg("may be sub-page size is incorrect?");
-				goto out_free;
-			}
-
-			err = mtd_torture(libmtd, mtd, args.node_fd, eb);
-			if (err) {
-				if (mark_bad(mtd, si, eb))
-					goto out_free;
-			}
-			continue;
-
-		}
+		err = write_eraseblock(libmtd, mtd, ui, si, eb, NULL, 0);
+		if (err < 0)
+			return -1;
 	}
 
 	if (!args.quiet && !args.verbose)
@@ -654,28 +637,26 @@ static int format(libmtd_t libmtd, const struct mtd_dev_info *mtd,
 	if (!novtbl) {
 		if (eb1 == -1 || eb2 == -1) {
 			errmsg("no eraseblocks for volume table");
-			goto out_free;
+			goto out;
 		}
 
 		verbose(args.verbose, "write volume table to eraseblocks %d and %d", eb1, eb2);
 		vtbl = ubigen_create_empty_vtbl(ui);
 		if (!vtbl)
-			goto out_free;
+			goto out;
 
 		err = ubigen_write_layout_vol(ui, eb1, eb2, ec1,  ec2, vtbl,
 					      args.node_fd);
 		free(vtbl);
 		if (err) {
 			errmsg("cannot write layout volume");
-			goto out_free;
+			goto out;
 		}
 	}
 
-	free(hdr);
 	return 0;
 
-out_free:
-	free(hdr);
+out:
 	return -1;
 }
 
-- 
2.1.1

[PATCH 2/2] ubiformat: Leave space for fastmap anchor

[Sascha Hauer]

The fastmap code needs a free eraseblock in the first 64 erasblocks
to write a fastmap anchor. Since ubiformat continuously writes the
image to the flash the fastmap code won't find a free block and
fastmap will be disabled. With this patch ubiformat skips flashing
a block at the beginning thus allowing the fastmap code to write
an anchor.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 ubi-utils/ubiformat.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/ubi-utils/ubiformat.c b/ubi-utils/ubiformat.c
index 2269cdf..df43df3 100644
--- a/ubi-utils/ubiformat.c
+++ b/ubi-utils/ubiformat.c
@@ -518,6 +518,7 @@ static int flash_image(libmtd_t libmtd, const struct mtd_dev_info *mtd,
 		       const struct ubigen_info *ui, struct ubi_scan_info *si)
 {
 	int fd, img_ebs, eb, written_ebs = 0, divisor, skip_data_read = 0;
+	int fastmap_anchor_done = 0;
 	off_t st_size;
 
 	fd = open_file(&st_size);
@@ -544,6 +545,15 @@ static int flash_image(libmtd_t libmtd, const struct mtd_dev_info *mtd,
 		int err;
 		char buf[mtd->eb_size];
 
+		if (!fastmap_anchor_done) {
+			err = write_eraseblock(libmtd, mtd, ui, si, eb, NULL, 0);
+			if (err < 0)
+				goto out_close;
+			if (err == 0)
+				fastmap_anchor_done = 1;
+			continue;
+		}
+
 		if (!args.quiet && !args.verbose) {
 			printf("\r" PROGRAM_NAME ": flashing eraseblock %d -- %2lld %% complete  ",
 			       eb, (long long)(eb + 1) * 100 / divisor);
-- 
2.1.1

Re: [PATCH 2/2] ubiformat: Leave space for fastmap anchor

[Richard Weinberger]

Am 22.10.2014 um 09:48 schrieb Sascha Hauer:
> The fastmap code needs a free eraseblock in the first 64 erasblocks
> to write a fastmap anchor. Since ubiformat continuously writes the
> image to the flash the fastmap code won't find a free block and
> fastmap will be disabled. 

If UBI is unable to write a fastmap it will try again later.
So, it will be not disabled.

> With this patch ubiformat skips flashing
> a block at the beginning thus allowing the fastmap code to write
> an anchor.

Hmm, this is a bit hacky. What prevents UBI itself from using this free PEB
after the first attach? I.e. if a bitflip happens?
The in kernel code has already a mechanism to move used PEBs < 64 to make space
for the anchor.
IMHO the only sane approach is to create a fastmap enabled image directly with ubiformat.
While creating the initial fastmap code I had the plan to make mtd-tools fastmap aware
but simply run out of budget and so far nobody cared enough.

Thanks,
//richard

> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  ubi-utils/ubiformat.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/ubi-utils/ubiformat.c b/ubi-utils/ubiformat.c
> index 2269cdf..df43df3 100644
> --- a/ubi-utils/ubiformat.c
> +++ b/ubi-utils/ubiformat.c
> @@ -518,6 +518,7 @@ static int flash_image(libmtd_t libmtd, const struct mtd_dev_info *mtd,
>  		       const struct ubigen_info *ui, struct ubi_scan_info *si)
>  {
>  	int fd, img_ebs, eb, written_ebs = 0, divisor, skip_data_read = 0;
> +	int fastmap_anchor_done = 0;
>  	off_t st_size;
>  
>  	fd = open_file(&st_size);
> @@ -544,6 +545,15 @@ static int flash_image(libmtd_t libmtd, const struct mtd_dev_info *mtd,
>  		int err;
>  		char buf[mtd->eb_size];
>  
> +		if (!fastmap_anchor_done) {
> +			err = write_eraseblock(libmtd, mtd, ui, si, eb, NULL, 0);
> +			if (err < 0)
> +				goto out_close;
> +			if (err == 0)
> +				fastmap_anchor_done = 1;
> +			continue;
> +		}
> +
>  		if (!args.quiet && !args.verbose) {
>  			printf("\r" PROGRAM_NAME ": flashing eraseblock %d -- %2lld %% complete  ",
>  			       eb, (long long)(eb + 1) * 100 / divisor);
> 

Re: [PATCH 2/2] ubiformat: Leave space for fastmap anchor

[Tanya Brokhman]

On 10/22/2014 11:15 AM, Richard Weinberger wrote:
> Am 22.10.2014 um 09:48 schrieb Sascha Hauer:
>> The fastmap code needs a free eraseblock in the first 64 erasblocks
>> to write a fastmap anchor. Since ubiformat continuously writes the
>> image to the flash the fastmap code won't find a free block and
>> fastmap will be disabled.
>
> If UBI is unable to write a fastmap it will try again later.
> So, it will be not disabled.
>
>> With this patch ubiformat skips flashing
>> a block at the beginning thus allowing the fastmap code to write
>> an anchor.
>
> Hmm, this is a bit hacky. What prevents UBI itself from using this free PEB
> after the first attach? I.e. if a bitflip happens?
> The in kernel code has already a mechanism to move used PEBs < 64 to make space
> for the anchor.
> IMHO the only sane approach is to create a fastmap enabled image directly with ubiformat.
> While creating the initial fastmap code I had the plan to make mtd-tools fastmap aware
> but simply run out of budget and so far nobody cared enough.

I care :) Its in my TODO to update ubinize to leave space for fastmap 
anchor (and perhaps generate a fastmap data as well but this is just a 
thought).


Thanks,
Tanya Brokhman
-- 
Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum, a Linux Foundation Collaborative Project

Re: [PATCH 2/2] ubiformat: Leave space for fastmap anchor

[Richard Weinberger]

Am 22.10.2014 um 10:19 schrieb Tanya Brokhman:
> On 10/22/2014 11:15 AM, Richard Weinberger wrote:
>> Am 22.10.2014 um 09:48 schrieb Sascha Hauer:
>>> The fastmap code needs a free eraseblock in the first 64 erasblocks
>>> to write a fastmap anchor. Since ubiformat continuously writes the
>>> image to the flash the fastmap code won't find a free block and
>>> fastmap will be disabled.
>>
>> If UBI is unable to write a fastmap it will try again later.
>> So, it will be not disabled.
>>
>>> With this patch ubiformat skips flashing
>>> a block at the beginning thus allowing the fastmap code to write
>>> an anchor.
>>
>> Hmm, this is a bit hacky. What prevents UBI itself from using this free PEB
>> after the first attach? I.e. if a bitflip happens?
>> The in kernel code has already a mechanism to move used PEBs < 64 to make space
>> for the anchor.
>> IMHO the only sane approach is to create a fastmap enabled image directly with ubiformat.
>> While creating the initial fastmap code I had the plan to make mtd-tools fastmap aware
>> but simply run out of budget and so far nobody cared enough.
> 
> I care :) Its in my TODO to update ubinize to leave space for fastmap anchor (and perhaps generate a fastmap data as well but this is just a thought).

Good to know. :)
Please generate the fastmap data such that upon the very first attach fastmap can be used.
As I said leaving space for fastmap is hacky and still cannot guarantee that fastmap can find
a free PEB for its anchor.

Thanks,
//richard

Re: [PATCH 2/2] ubiformat: Leave space for fastmap anchor

[Richard Weinberger]

Am 22.10.2014 um 10:19 schrieb Tanya Brokhman:
> I care :) Its in my TODO to update ubinize to leave space for fastmap anchor (and perhaps generate a fastmap data as well but this is just a thought).

While talking about fastmap TODOs, here you can find my fastmap patch queue:
https://git.kernel.org/cgit/linux/kernel/git/rw/misc.git/log/?h=ubi-current

These patches are *not* ready for mainline, some of them need a rewrite
to meet all design goals of UBI. But maybe the one or other fixes an issue
which is on your TODO list. :-)

Thanks,
//richard

Re: [PATCH 2/2] ubiformat: Leave space for fastmap anchor

[Sascha Hauer]

On Wed, Oct 22, 2014 at 10:15:01AM +0200, Richard Weinberger wrote:
> Am 22.10.2014 um 09:48 schrieb Sascha Hauer:
> > The fastmap code needs a free eraseblock in the first 64 erasblocks
> > to write a fastmap anchor. Since ubiformat continuously writes the
> > image to the flash the fastmap code won't find a free block and
> > fastmap will be disabled. 
> 
> If UBI is unable to write a fastmap it will try again later.
> So, it will be not disabled.
> 
> > With this patch ubiformat skips flashing
> > a block at the beginning thus allowing the fastmap code to write
> > an anchor.
> 
> Hmm, this is a bit hacky. What prevents UBI itself from using this free PEB
> after the first attach? I.e. if a bitflip happens?
> The in kernel code has already a mechanism to move used PEBs < 64 to make space
> for the anchor.

Ah, indeed. I wasn't aware of this. I think we were confused by not
having fm_autoconvert enabled in the kernel and using ubiformat and UBI
from barebox.
So right now I tested that the kernel makes space for the anchor and
has fastmap support on the second attach of an mtd device.

So it seems my problem is solved without patching ubiformat. Nice ;)

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 2/2] ubiformat: Leave space for fastmap anchor

[Tanya Brokhman]

On 10/22/2014 11:34 AM, Richard Weinberger wrote:
> Am 22.10.2014 um 10:19 schrieb Tanya Brokhman:
>> I care :) Its in my TODO to update ubinize to leave space for fastmap anchor (and perhaps generate a fastmap data as well but this is just a thought).
>
> While talking about fastmap TODOs, here you can find my fastmap patch queue:
> https://git.kernel.org/cgit/linux/kernel/git/rw/misc.git/log/?h=ubi-current
>
> These patches are *not* ready for mainline, some of them need a rewrite
> to meet all design goals of UBI. But maybe the one or other fixes an issue
> which is on your TODO list. :-)
>
> Thanks,
> //richard
>

thanks! it's very helpful! and just to combine the reply in one email:

"Please generate the fastmap data such that upon the very first attach 
fastmap can be used."

This is what I was thinking. Its doable, right? since in ubinize we have 
all data required for constructing fastmap.

 From all your discussions with Artem I figure we can expect quite a few 
changes to the fastmap implementation. Is the layout also going to 
change? I remember you mentioning such idea in one of the emails.


Thanks,
Tanya Brokhman
-- 
Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum, a Linux Foundation Collaborative Project

Re: [PATCH 2/2] ubiformat: Leave space for fastmap anchor

[Richard Weinberger]

Am 22.10.2014 um 12:48 schrieb Tanya Brokhman:
> On 10/22/2014 11:34 AM, Richard Weinberger wrote:
>> Am 22.10.2014 um 10:19 schrieb Tanya Brokhman:
>>> I care :) Its in my TODO to update ubinize to leave space for fastmap anchor (and perhaps generate a fastmap data as well but this is just a thought).
>>
>> While talking about fastmap TODOs, here you can find my fastmap patch queue:
>> https://git.kernel.org/cgit/linux/kernel/git/rw/misc.git/log/?h=ubi-current
>>
>> These patches are *not* ready for mainline, some of them need a rewrite
>> to meet all design goals of UBI. But maybe the one or other fixes an issue
>> which is on your TODO list. :-)
>>
>> Thanks,
>> //richard
>>
> 
> thanks! it's very helpful! and just to combine the reply in one email:
> 
> "Please generate the fastmap data such that upon the very first attach fastmap can be used."
> 
> This is what I was thinking. Its doable, right? since in ubinize we have all data required for constructing fastmap.

Of course it is doable. :)

> From all your discussions with Artem I figure we can expect quite a few changes to the fastmap implementation. Is the layout also going to change? I remember you mentioning such
> idea in one of the emails.

So far the layout is not going to change.

Thanks,
//richard