All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/2] Support skipping bad blocks when seeking to start address
@ 2017-01-04 14:18 Mike Crowe
  2017-01-04 14:18 ` [PATCH 1/2] nandwrite: Add --skip-bad-blocks-to-start option Mike Crowe
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Mike Crowe @ 2017-01-04 14:18 UTC (permalink / raw)
  To: linux-mtd; +Cc: Mike Crowe

I found myself needing to write and verify part of a partition that
will be read by the bootloader by skipping blocks from the start of
the partition. Initially I added separate --input-skip and
--output-skip options but David Oberhollenzer suggested in
<cdfdbe5d-1aa8-e424-7691-338d0d63f7b1@sigma-star.at> that a simple
flag to indicate that the start address should skip bad blocks would
avoid ambiguity.

David also suggested that I skip the bad blocks in a separate step
before the already-cluttered main loop.

Mike Crowe (2):
  nandwrite: Add --skip-bad-blocks-to-start option
  nanddump: Add --skip-bad-blocks-to-start option

 nand-utils/nanddump.c  | 23 +++++++++++++++++++++++
 nand-utils/nandwrite.c | 26 ++++++++++++++++++++++++++
 2 files changed, 49 insertions(+)

-- 
2.1.4

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 1/2] nandwrite: Add --skip-bad-blocks-to-start option
  2017-01-04 14:18 [PATCH 0/2] Support skipping bad blocks when seeking to start address Mike Crowe
@ 2017-01-04 14:18 ` Mike Crowe
  2017-01-04 14:18 ` [PATCH 2/2] nanddump: " Mike Crowe
  2017-01-05 13:38 ` [PATCH 0/2] Support skipping bad blocks when seeking to start address goliath
  2 siblings, 0 replies; 12+ messages in thread
From: Mike Crowe @ 2017-01-04 14:18 UTC (permalink / raw)
  To: linux-mtd; +Cc: Mike Crowe

The --skip-bad-blocks-to-start option will increase the seek offset by the
size of each bad block encountered between the start of the partition and
the specified start address.

This can be useful when writing part way through a partition that will be
read using a simple bad-block-skipping algorithm.
---
 nand-utils/nandwrite.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/nand-utils/nandwrite.c b/nand-utils/nandwrite.c
index 9602a6e..880990f 100644
--- a/nand-utils/nandwrite.c
+++ b/nand-utils/nandwrite.c
@@ -56,6 +56,8 @@ static void display_help(int status)
 "  -o, --oob               Input contains oob data\n"
 "  -O, --onlyoob           Input contains oob data and only write the oob part\n"
 "  -s addr, --start=addr   Set output start address (default is 0)\n"
+"  --skip-bad-blocks-to-start"
+"                          Skip bad blocks when seeking to the start address\n"
 "  -p, --pad               Pad writes to page size\n"
 "  -b, --blockalign=1|2|4  Set multiple of eraseblocks to align to\n"
 "      --input-skip=length Skip |length| bytes of the input file\n"
@@ -96,6 +98,7 @@ static bool		autoplace = false;
 static bool		skipallffs = false;
 static bool		noskipbad = false;
 static bool		pad = false;
+static bool		skip_bad_blocks_to_start = false;
 static int		blockalign = 1; /* default to using actual block size */
 
 static void process_options(int argc, char * const argv[])
@@ -110,6 +113,7 @@ static void process_options(int argc, char * const argv[])
 			{"version", no_argument, 0, 'V'},
 			{"input-skip", required_argument, 0, 0},
 			{"input-size", required_argument, 0, 0},
+			{"skip-bad-blocks-to-start", no_argument, 0, 0},
 			{"help", no_argument, 0, 'h'},
 			{"blockalign", required_argument, 0, 'b'},
 			{"markbad", no_argument, 0, 'm'},
@@ -139,6 +143,9 @@ static void process_options(int argc, char * const argv[])
 			case 2: /* --input-size */
 				inputsize = simple_strtoll(optarg, &error);
 				break;
+			case 3: /* --skip-bad-blocks-to-start */
+				skip_bad_blocks_to_start = true;
+				break;
 			}
 			break;
 		case 'V':
@@ -349,6 +356,25 @@ int main(int argc, char * const argv[])
 		goto closeall;
 	}
 
+	/* Skip bad blocks on the way to the start address if necessary */
+	if (skip_bad_blocks_to_start) {
+		unsigned long long bbs_offset = 0;
+		while (bbs_offset < mtdoffset) {
+			ret = mtd_is_bad(&mtd, fd, bbs_offset / ebsize_aligned);
+			if (ret < 0) {
+				sys_errmsg("%s: MTD get bad block failed", mtd_device);
+				goto closeall;
+			} else if (ret == 1) {
+				if (!quiet)
+					fprintf(stderr, "Bad block at %llx, %u block(s) "
+						"from %llx will be skipped\n",
+						bbs_offset, blockalign, bbs_offset);
+				mtdoffset += ebsize_aligned;
+			}
+			bbs_offset += ebsize_aligned;
+		}
+	}
+
 	/* Check, if length fits into device */
 	if ((imglen / pagelen) * mtd.min_io_size > mtd.size - mtdoffset) {
 		fprintf(stderr, "Image %lld bytes, NAND page %d bytes, OOB area %d"
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 2/2] nanddump: Add --skip-bad-blocks-to-start option
  2017-01-04 14:18 [PATCH 0/2] Support skipping bad blocks when seeking to start address Mike Crowe
  2017-01-04 14:18 ` [PATCH 1/2] nandwrite: Add --skip-bad-blocks-to-start option Mike Crowe
@ 2017-01-04 14:18 ` Mike Crowe
  2017-01-05 13:38 ` [PATCH 0/2] Support skipping bad blocks when seeking to start address goliath
  2 siblings, 0 replies; 12+ messages in thread
From: Mike Crowe @ 2017-01-04 14:18 UTC (permalink / raw)
  To: linux-mtd; +Cc: Mike Crowe

The --skip-bad-blocks-to-start option will increase the start address by
the size of each bad block encountered between the start of the partition
and the specified start address.

This can be useful if other readers of the partition will be reading using
a simple bad-block-skipping algorithm.
---
 nand-utils/nanddump.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/nand-utils/nanddump.c b/nand-utils/nanddump.c
index a8ad334..eaaf331 100644
--- a/nand-utils/nanddump.c
+++ b/nand-utils/nanddump.c
@@ -52,6 +52,8 @@ static void display_help(int status)
 "-p         --prettyprint        Print nice (hexdump)\n"
 "-q         --quiet              Don't display progress and status messages\n"
 "-s addr    --startaddress=addr  Start address\n"
+"           --skip-bad-blocks-to-start\n"
+"                                Skip bad blocks when seeking to the start address\n"
 "\n"
 "--bb=METHOD, where METHOD can be `padbad', `dumpbad', or `skipbad':\n"
 "    padbad:  dump flash data, substituting 0xFF for any bad blocks\n"
@@ -86,6 +88,7 @@ static const char		*dumpfile;		// dump file name
 static bool			quiet = false;		// suppress diagnostic output
 static bool			canonical = false;	// print nice + ascii
 static bool			forcebinary = false;	// force printing binary to tty
+static bool			skip_bad_blocks_to_start = false;
 
 static enum {
 	padbad,   // dump flash data, substituting 0xFF for any bad blocks
@@ -105,6 +108,7 @@ static void process_options(int argc, char * const argv[])
 			{"version", no_argument, 0, 'V'},
 			{"bb", required_argument, 0, 0},
 			{"omitoob", no_argument, 0, 0},
+			{"skip-bad-blocks-to-start", no_argument, 0, 0 },
 			{"help", no_argument, 0, 'h'},
 			{"forcebinary", no_argument, 0, 'a'},
 			{"canonicalprint", no_argument, 0, 'c'},
@@ -146,6 +150,9 @@ static void process_options(int argc, char * const argv[])
 							errmsg_die("--oob and --oomitoob are mutually exclusive");
 						}
 						break;
+					case 3: /* --skip-bad-blocks-to-start */
+						skip_bad_blocks_to_start = true;
+						break;
 				}
 				break;
 			case 'V':
@@ -397,6 +404,22 @@ int main(int argc, char * const argv[])
 				mtd.min_io_size);
 		goto closeall;
 	}
+	if (skip_bad_blocks_to_start) {
+		unsigned long long bbs_offset = 0;
+		while (bbs_offset < start_addr) {
+			err = mtd_is_bad(&mtd, fd, bbs_offset / mtd.eb_size);
+			if (err < 0) {
+				sys_errmsg("%s: MTD get bad block failed", mtddev);
+				goto closeall;
+			} else if (err == 1) {
+				if (!quiet)
+					fprintf(stderr, "Bad block at %llx\n", bbs_offset);
+				start_addr += mtd.eb_size;
+			}
+			bbs_offset += mtd.eb_size;
+		}
+	}
+
 	if (length)
 		end_addr = start_addr + length;
 	if (!length || end_addr > mtd.size)
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH 0/2] Support skipping bad blocks when seeking to start address
  2017-01-04 14:18 [PATCH 0/2] Support skipping bad blocks when seeking to start address Mike Crowe
  2017-01-04 14:18 ` [PATCH 1/2] nandwrite: Add --skip-bad-blocks-to-start option Mike Crowe
  2017-01-04 14:18 ` [PATCH 2/2] nanddump: " Mike Crowe
@ 2017-01-05 13:38 ` goliath
  2017-01-05 14:18   ` Mike Crowe
  2 siblings, 1 reply; 12+ messages in thread
From: goliath @ 2017-01-05 13:38 UTC (permalink / raw)
  To: Mike Crowe, linux-mtd; +Cc: Richard Weinberger, Boris Brezillon

Hi,

not being between two releases this time, I had some more time to
think about your patch and had a brief discussion with Richard and
subsequently with Boris on the MTD IRC channel.

Some concerns have been raised regarding how your specific boot
loader clutch works and whether we should really add tool support
for it.

If your boot loader seeks to a specific erase block by counting
only good blocks (to load the kernel or some second stage?), what
happens if another block in between goes bad? Does that just brick
your board or do you have to move the data around all the time to
account for that?

What kind of boot loader is this? Is it widely used or is that
algorithm used by a number of other boot loaders? In that case
it would make sense to add support for this. However, if it is
just a one-time fix for a vendor specific boot loader, it might
not be reasonable to merge it into upstream mtd-utils. Or is
there some other use case for this?


Regards,

David

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 0/2] Support skipping bad blocks when seeking to start address
  2017-01-05 13:38 ` [PATCH 0/2] Support skipping bad blocks when seeking to start address goliath
@ 2017-01-05 14:18   ` Mike Crowe
  2017-01-05 14:48     ` Boris Brezillon
  0 siblings, 1 reply; 12+ messages in thread
From: Mike Crowe @ 2017-01-05 14:18 UTC (permalink / raw)
  To: goliath; +Cc: linux-mtd, Richard Weinberger, Boris Brezillon

On Thursday 05 January 2017 at 14:38:23 +0100, goliath wrote:
> Some concerns have been raised regarding how your specific boot
> loader clutch works and whether we should really add tool support
> for it.
> 
> If your boot loader seeks to a specific erase block by counting
> only good blocks (to load the kernel or some second stage?), what
> happens if another block in between goes bad? Does that just brick
> your board or do you have to move the data around all the time to
> account for that?

The blocks in-between don't get written so they can't be marked bad. If
they develop bit errors that can't be corrected then the board will stop
booting because the bootloader can't be loaded.

> What kind of boot loader is this? Is it widely used or is that
> algorithm used by a number of other boot loaders? In that case
> it would make sense to add support for this. However, if it is
> just a one-time fix for a vendor specific boot loader, it might
> not be reasonable to merge it into upstream mtd-utils. Or is
> there some other use case for this?

The bootloader is provided by our SoC vendor and only used with their
family of chips. It has various parts that are loaded independently and we
need to upgrade some of them. In an ideal world these would be in separate
partitions and we wouldn't have this problem, but we need to maintain
backward compatibility.

I've used other bootloaders in the past that also have two stages, and the
second stage is merely appended at the next good block.

mtd-utils already supports reading and writing flash whilst skipping
blocks. Adding the ability to start reading or writing at an offset that
takes into account previous bad blocks seems to be a generically-useful
feature to me.

However, if this is considered to be too niche a use case then we can just
continue to apply the patch ourselves.

Thanks for the review.

Mike.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 0/2] Support skipping bad blocks when seeking to start address
  2017-01-05 14:18   ` Mike Crowe
@ 2017-01-05 14:48     ` Boris Brezillon
  2017-01-05 15:04       ` Mike Crowe
  0 siblings, 1 reply; 12+ messages in thread
From: Boris Brezillon @ 2017-01-05 14:48 UTC (permalink / raw)
  To: Mike Crowe; +Cc: goliath, linux-mtd, Richard Weinberger

Hi Mike,

On Thu, 5 Jan 2017 14:18:34 +0000
Mike Crowe <mac@mcrowe.com> wrote:

> On Thursday 05 January 2017 at 14:38:23 +0100, goliath wrote:
> > Some concerns have been raised regarding how your specific boot
> > loader clutch works and whether we should really add tool support
> > for it.
> > 
> > If your boot loader seeks to a specific erase block by counting
> > only good blocks (to load the kernel or some second stage?), what
> > happens if another block in between goes bad? Does that just brick
> > your board or do you have to move the data around all the time to
> > account for that?  
> 
> The blocks in-between don't get written so they can't be marked bad.

Maybe not the block in between, but the first block used to store your
2nd stage bootloader (or whatever your store at this offset) might
become bad, which means your data will be moved to the next block (or
possibly farther).

> If
> they develop bit errors that can't be corrected then the board will stop
> booting because the bootloader can't be loaded.

Okay.

> 
> > What kind of boot loader is this? Is it widely used or is that
> > algorithm used by a number of other boot loaders? In that case
> > it would make sense to add support for this. However, if it is
> > just a one-time fix for a vendor specific boot loader, it might
> > not be reasonable to merge it into upstream mtd-utils. Or is
> > there some other use case for this?  
> 
> The bootloader is provided by our SoC vendor and only used with their
> family of chips. It has various parts that are loaded independently and we
> need to upgrade some of them. In an ideal world these would be in separate
> partitions and we wouldn't have this problem, but we need to maintain
> backward compatibility.
> 
> I've used other bootloaders in the past that also have two stages, and the
> second stage is merely appended at the next good block.

So the 1st stage bootloader is trying to load data from the next good
block. I guess the 1st stage bootloader is also programmed using this
approach (skip bad blocks if any), which would explain why you need
this 'adjust offset after skipping bad blocks' feature.

> 
> mtd-utils already supports reading and writing flash whilst skipping
> blocks. Adding the ability to start reading or writing at an offset that
> takes into account previous bad blocks seems to be a generically-useful
> feature to me.
> 
> However, if this is considered to be too niche a use case then we can just
> continue to apply the patch ourselves.

I don't think that's the idea here. We just wanted to be sure what was
the exact use case you had.
I think this request is acceptable (even if I don't like the option
name :-)), but I'll let Richard and David decide what to do with this
patch.

Thanks for the explanation.

Boris

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 0/2] Support skipping bad blocks when seeking to start address
  2017-01-05 14:48     ` Boris Brezillon
@ 2017-01-05 15:04       ` Mike Crowe
  2017-01-09 12:18         ` David Oberhollenzer
  0 siblings, 1 reply; 12+ messages in thread
From: Mike Crowe @ 2017-01-05 15:04 UTC (permalink / raw)
  To: Boris Brezillon; +Cc: goliath, linux-mtd, Richard Weinberger

On Thursday 05 January 2017 at 15:48:24 +0100, Boris Brezillon wrote:
> On Thu, 5 Jan 2017 14:18:34 +0000
> Mike Crowe <mac@mcrowe.com> wrote:
> > On Thursday 05 January 2017 at 14:38:23 +0100, goliath wrote:
> > > If your boot loader seeks to a specific erase block by counting
> > > only good blocks (to load the kernel or some second stage?), what
> > > happens if another block in between goes bad? Does that just brick
> > > your board or do you have to move the data around all the time to
> > > account for that?  
> > 
> > The blocks in-between don't get written so they can't be marked bad.
> 
> Maybe not the block in between, but the first block used to store your
> 2nd stage bootloader (or whatever your store at this offset) might
> become bad, which means your data will be moved to the next block (or
> possibly farther).

In our case we always write the remainder of the partition so any bad
blocks that develop will shift everything else down too. You're correct
that this would be a problem if we attempted to write in the middle though.

> I think this request is acceptable (even if I don't like the option
> name :-)), but I'll let Richard and David decide what to do with this
> patch.

I'm not particularly attached to the option name. I chose
"--skip-bad-blocks-to-start" in the hope of being as clear as possible, but
I agree that it is a mouthful! The confusion over the meaning of the option
was also one of the reasons why I originally implemented the feature using
a separate offset.

Thanks for the review.

Mike.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 0/2] Support skipping bad blocks when seeking to start address
  2017-01-05 15:04       ` Mike Crowe
@ 2017-01-09 12:18         ` David Oberhollenzer
  2017-01-09 14:51           ` Mike Crowe
  0 siblings, 1 reply; 12+ messages in thread
From: David Oberhollenzer @ 2017-01-09 12:18 UTC (permalink / raw)
  To: Mike Crowe, Boris Brezillon; +Cc: linux-mtd, Richard Weinberger

On 01/05/2017 04:04 PM, Mike Crowe wrote:
>> I think this request is acceptable (even if I don't like the option
>> name :-)), but I'll let Richard and David decide what to do with this
>> patch.
> 
> I'm not particularly attached to the option name. I chose
> "--skip-bad-blocks-to-start" in the hope of being as clear as possible, but
> I agree that it is a mouthful! The confusion over the meaning of the option
> was also one of the reasons why I originally implemented the feature using
> a separate offset.
As I mentioned in my initial suggestion, this is IMO much clearer than
having two different offset options and avoids the confusing corner case
of having both set.

I would gladly accept this patch series (apart from the missing signed-off-by),
however what actually sparked the discussion above was this snippet here:

On 01/04/2017 03:18 PM, Mike Crowe wrote:
> diff --git a/nand-utils/nandwrite.c b/nand-utils/nandwrite.c
> index 9602a6e..880990f 100644
> --- a/nand-utils/nandwrite.c
> +++ b/nand-utils/nandwrite.c
> @@ -349,6 +356,25 @@ int main(int argc, char * const argv[])
>  		goto closeall;
>  	}
>  
> +	/* Skip bad blocks on the way to the start address if necessary */
> +	if (skip_bad_blocks_to_start) {
> +		unsigned long long bbs_offset = 0;
> +		while (bbs_offset < mtdoffset) {
> +			ret = mtd_is_bad(&mtd, fd, bbs_offset / ebsize_aligned);
> +			if (ret < 0) {
> +				sys_errmsg("%s: MTD get bad block failed", mtd_device);
> +				goto closeall;
> +			} else if (ret == 1) {
> +				if (!quiet)
> +					fprintf(stderr, "Bad block at %llx, %u block(s) "
> +						"from %llx will be skipped\n",
> +						bbs_offset, blockalign, bbs_offset);
> +				mtdoffset += ebsize_aligned;
> +			}
> +			bbs_offset += ebsize_aligned;
> +		}
> +	}
> +
>  	/* Check, if length fits into device */
>  	if ((imglen / pagelen) * mtd.min_io_size > mtd.size - mtdoffset) {
>  		fprintf(stderr, "Image %lld bytes, NAND page %d bytes, OOB area %d"
> 
The nandwrite program supports working on clusters of erase blocks aka
"virtual erase blocks", because JFFS2 supports that.

The variable ebsize_aligned is used all over the place in nandwrite and
can hold a multiple of the actual erase block size.

Your patch skips a virtual block only if the first block is bad. IMO it
would make more sense to check if any of the clustered blocks is bad in
the case where ebsize_alligned is larger than a single erase block.

This prompts the questions of how your bootload would deal with that,
on the one heand, and how JFFS2 deals with bad blocks within a virtual
erase block.


Regards,

David

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 0/2] Support skipping bad blocks when seeking to start address
  2017-01-09 12:18         ` David Oberhollenzer
@ 2017-01-09 14:51           ` Mike Crowe
  2017-01-11 16:22             ` Mike Crowe
  0 siblings, 1 reply; 12+ messages in thread
From: Mike Crowe @ 2017-01-09 14:51 UTC (permalink / raw)
  To: David Oberhollenzer; +Cc: Boris Brezillon, linux-mtd, Richard Weinberger

On Monday 09 January 2017 at 13:18:53 +0100, David Oberhollenzer wrote:
> On 01/05/2017 04:04 PM, Mike Crowe wrote:
> >> I think this request is acceptable (even if I don't like the option
> >> name :-)), but I'll let Richard and David decide what to do with this
> >> patch.
> > 
> > I'm not particularly attached to the option name. I chose
> > "--skip-bad-blocks-to-start" in the hope of being as clear as possible, but
> > I agree that it is a mouthful! The confusion over the meaning of the option
> > was also one of the reasons why I originally implemented the feature using
> > a separate offset.
> As I mentioned in my initial suggestion, this is IMO much clearer than
> having two different offset options and avoids the confusing corner case
> of having both set.
> 
> I would gladly accept this patch series (apart from the missing signed-off-by),
> however what actually sparked the discussion above was this snippet here:

I spotted the lack of sign-off just after I sent the patch. I'll make sure
that future versions include one.

> On 01/04/2017 03:18 PM, Mike Crowe wrote:
> > diff --git a/nand-utils/nandwrite.c b/nand-utils/nandwrite.c
> > index 9602a6e..880990f 100644
> > --- a/nand-utils/nandwrite.c
> > +++ b/nand-utils/nandwrite.c
> > @@ -349,6 +356,25 @@ int main(int argc, char * const argv[])
> >  		goto closeall;
> >  	}
> >  
> > +	/* Skip bad blocks on the way to the start address if necessary */
> > +	if (skip_bad_blocks_to_start) {
> > +		unsigned long long bbs_offset = 0;
> > +		while (bbs_offset < mtdoffset) {
> > +			ret = mtd_is_bad(&mtd, fd, bbs_offset / ebsize_aligned);
> > +			if (ret < 0) {
> > +				sys_errmsg("%s: MTD get bad block failed", mtd_device);
> > +				goto closeall;
> > +			} else if (ret == 1) {
> > +				if (!quiet)
> > +					fprintf(stderr, "Bad block at %llx, %u block(s) "
> > +						"from %llx will be skipped\n",
> > +						bbs_offset, blockalign, bbs_offset);
> > +				mtdoffset += ebsize_aligned;
> > +			}
> > +			bbs_offset += ebsize_aligned;
> > +		}
> > +	}
> > +
> >  	/* Check, if length fits into device */
> >  	if ((imglen / pagelen) * mtd.min_io_size > mtd.size - mtdoffset) {
> >  		fprintf(stderr, "Image %lld bytes, NAND page %d bytes, OOB area %d"
> > 
> The nandwrite program supports working on clusters of erase blocks aka
> "virtual erase blocks", because JFFS2 supports that.
> 
> The variable ebsize_aligned is used all over the place in nandwrite and
> can hold a multiple of the actual erase block size.
> 
> Your patch skips a virtual block only if the first block is bad. IMO it
> would make more sense to check if any of the clustered blocks is bad in
> the case where ebsize_alligned is larger than a single erase block.

To be honest, I didn't entirely understand that feature. I just tried to
make sure I treated ebsize_aligned in the same way as the rest of the code.

> This prompts the questions of how your bootload would deal with that,
> on the one heand, and how JFFS2 deals with bad blocks within a virtual
> erase block.

We're not using virtual erase blocks and I don't believe our bootloader
supports them either.

If erase blocks are going to be combined into larger virtual erase blocks
then I believe the only sensible way forward is to treat the virtual erase
block as bad if any of the blocks within it are bad.

However, I can't say I understand how the main loop in nandwrite.c can work
if blockalign > 1. It contains the following lines of code:

|  ebsize_aligned = mtd.eb_size * blockalign;
|  ...
|  ret = mtd_is_bad(&mtd, fd, offs / ebsize_aligned);
|  ...		   
|  offs +=  ebsize_aligned / blockalign;

The implementation of mtd_is_bad in libmtd.c contains:

|  seek = (loff_t)eb * mtd->eb_size;

These four statements seem to be incompatible to me. mtd_is_bad expects a
physical erase block index, but it is being passed a virtual erase block
index. Furthermore, the offset is being incremented by the physical erase
block size, but this won't have any effect on the virtual erase block index
passed to mtd_is_bad until blockalign loops have been executed.

Nevertheless, I can make the initial bad-block-skipping code skip virtual
erase blocks if any physical erase block within is bad if that is deemed to
be correct.

Thanks for the review.

Mike.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 0/2] Support skipping bad blocks when seeking to start address
  2017-01-09 14:51           ` Mike Crowe
@ 2017-01-11 16:22             ` Mike Crowe
  0 siblings, 0 replies; 12+ messages in thread
From: Mike Crowe @ 2017-01-11 16:22 UTC (permalink / raw)
  To: David Oberhollenzer; +Cc: Boris Brezillon, linux-mtd, Richard Weinberger

On Monday 09 January 2017 at 14:51:35 +0000, Mike Crowe wrote:
> However, I can't say I understand how the main loop in nandwrite.c can work
> if blockalign > 1. It contains the following lines of code:
> 
> |  ebsize_aligned = mtd.eb_size * blockalign;
> |  ...
> |  ret = mtd_is_bad(&mtd, fd, offs / ebsize_aligned);

I believe that the above line should read:

 ret = mtd_is_bad(&mtd, fd, offs / mtd.eb_size);

so that...

> |  ...		   
> |  offs +=  ebsize_aligned / blockalign;
> 
> The implementation of mtd_is_bad in libmtd.c contains:
> 
> |  seek = (loff_t)eb * mtd->eb_size;

...the above line works correctly.

This seems to have been introduced back in 2010 in
15d811481cf1cf61ae23fabbd1e191ebdbcf3881 when a direct ioctl call was
replaced by the call to mtd_is_bad.

But, if it really has been broken since then, I think it's worth
questioning how many people can actually be using the blockalign feature.

Thanks.

Mike.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 0/2] Support skipping bad blocks when seeking to start address
  2017-01-17 11:54 Mike Crowe
@ 2017-01-23  6:47 ` David Oberhollenzer
  0 siblings, 0 replies; 12+ messages in thread
From: David Oberhollenzer @ 2017-01-23  6:47 UTC (permalink / raw)
  To: Mike Crowe, linux-mtd; +Cc: richard

On 01/17/2017 12:54 PM, Mike Crowe wrote:
> I found myself needing to write and verify part of a partition that
> will be read by the bootloader by skipping blocks from the start of
> the partition. Initially I added separate --input-skip and
> --output-skip options but David Oberhollenzer suggested in
> <cdfdbe5d-1aa8-e424-7691-338d0d63f7b1 at sigma-star.at>
> http://lists.infradead.org/pipermail/linux-mtd/2016-December/070855.html
> that a simple flag to indicate that the start address should skip bad
> blocks would avoid ambiguity.
> 
> David also suggested that I skip the bad blocks in a separate step
> before the already-cluttered main loop.
> 
> This version uses is_virt_block_bad in nandwrite, so the series now depends
> on David Oberhollenzer's virtual block handling fixes from
> <20170112102828.13124-1-david.oberhollenzer@sigma-star.at>
> http://lists.infradead.org/pipermail/linux-mtd/2017-January/071544.html
> 
> Mike Crowe (2):
>   nandwrite: Add --skip-bad-blocks-to-start option
>   nanddump: Add --skip-bad-blocks-to-start option
> 
>  nand-utils/nanddump.c  | 23 +++++++++++++++++++++++
>  nand-utils/nandwrite.c | 26 ++++++++++++++++++++++++++
>  2 files changed, 49 insertions(+)
> 
Applied to mtd-utils.git.


Thanks,

David

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 0/2] Support skipping bad blocks when seeking to start address
@ 2017-01-17 11:54 Mike Crowe
  2017-01-23  6:47 ` David Oberhollenzer
  0 siblings, 1 reply; 12+ messages in thread
From: Mike Crowe @ 2017-01-17 11:54 UTC (permalink / raw)
  To: linux-mtd; +Cc: Mike Crowe, David Oberhollenzer, richard

I found myself needing to write and verify part of a partition that
will be read by the bootloader by skipping blocks from the start of
the partition. Initially I added separate --input-skip and
--output-skip options but David Oberhollenzer suggested in
<cdfdbe5d-1aa8-e424-7691-338d0d63f7b1 at sigma-star.at>
http://lists.infradead.org/pipermail/linux-mtd/2016-December/070855.html
that a simple flag to indicate that the start address should skip bad
blocks would avoid ambiguity.

David also suggested that I skip the bad blocks in a separate step
before the already-cluttered main loop.

This version uses is_virt_block_bad in nandwrite, so the series now depends
on David Oberhollenzer's virtual block handling fixes from
<20170112102828.13124-1-david.oberhollenzer@sigma-star.at>
http://lists.infradead.org/pipermail/linux-mtd/2017-January/071544.html

Mike Crowe (2):
  nandwrite: Add --skip-bad-blocks-to-start option
  nanddump: Add --skip-bad-blocks-to-start option

 nand-utils/nanddump.c  | 23 +++++++++++++++++++++++
 nand-utils/nandwrite.c | 26 ++++++++++++++++++++++++++
 2 files changed, 49 insertions(+)

-- 
2.1.4

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2017-01-23  6:49 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-01-04 14:18 [PATCH 0/2] Support skipping bad blocks when seeking to start address Mike Crowe
2017-01-04 14:18 ` [PATCH 1/2] nandwrite: Add --skip-bad-blocks-to-start option Mike Crowe
2017-01-04 14:18 ` [PATCH 2/2] nanddump: " Mike Crowe
2017-01-05 13:38 ` [PATCH 0/2] Support skipping bad blocks when seeking to start address goliath
2017-01-05 14:18   ` Mike Crowe
2017-01-05 14:48     ` Boris Brezillon
2017-01-05 15:04       ` Mike Crowe
2017-01-09 12:18         ` David Oberhollenzer
2017-01-09 14:51           ` Mike Crowe
2017-01-11 16:22             ` Mike Crowe
2017-01-17 11:54 Mike Crowe
2017-01-23  6:47 ` David Oberhollenzer

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.