Patchwork clarify calculation in nand_read_subpage and fix possible bug

login
register
mail settings
Submitter Werner Almesberger
Date Nov. 2, 2008, 5:51 a.m.
Message ID <20081102055138.GB5177@almesberger.net>
Download mbox | patch
Permalink /patch/6832/
State New, archived
Headers show

Comments

Werner Almesberger - Nov. 2, 2008, 5:51 a.m.
Alexey,

while debugging a NAND ECC problem, I tried to figure out how the
optimized ECC position calculation in nand_read_subpage worked. A
few pulled hairs later :-) ... I think it could be simplified a
bit, which also helps GCC to generate more compact code for it.

If I understand things correctly, it also seems that the original
algorithm had a small bug that would show with busw = 2 and
non-contiguous ECC blocks. My analysis is below.

Could you please have a look at it and ack it if you think it's
okay ?

Thanks !

- Werner

---------------------------------- cut here -----------------------------------

clarify-nand-ecc-access.patch

The ECC access calculation in nand_read_subpage is quite hard to
understand. This patch makes it more readable.

There is a small change in what the algorithm does: while
if (eccpos[(start_step + num_steps) * chip->ecc.bytes] & (busw - 1))
looks at the position of the ECC byte following the bytes we're
currently reading,
aligned_len = ALIGN(eccfrag_len+(pos-aligned_pos), busw);
only looks at their length plus the additional data we have to read
due to aligning the start position.

I believe the latter to be more correct than the former, since the
next ECC byte could be located anywhere and its location therefore
may not give the alignment information we seek. I'm not even sure
its position is always defined (what if we're reading the last ECC
of the page ?).

The last byte considered in the "gaps" test that checks that all ECC
bytes are contiguous is
  eccfrag_len-2+start_step*ecc.bytes+1 =
  num_steps*ecc.bytes-2+start_step*ecc.bytes+1 =
  (start_step+num+steps)*ecc.bytes-1
so it doesn't include (start_step + num_steps) * chip->ecc.bytes.

The change also saves 44 bytes on ARM.

---
Alexey Korolev - Nov. 6, 2008, 6:23 p.m.
Hi Werner,

Sorry for delay. 
I studied your patch. I liked it. It improves readability.

But I did not understand the note about a bug.
What is the case when bug appears? (What is the distribution of ecc
positions)


Thanks,
Alexey

> Alexey,
> 
> while debugging a NAND ECC problem, I tried to figure out how the
> optimized ECC position calculation in nand_read_subpage worked. A
> few pulled hairs later :-) ... I think it could be simplified a
> bit, which also helps GCC to generate more compact code for it.
> 
> If I understand things correctly, it also seems that the original
> algorithm had a small bug that would show with busw = 2 and
> non-contiguous ECC blocks. My analysis is below.
> 
> Could you please have a look at it and ack it if you think it's
> okay ?
> 
> Thanks !
> 
> - Werner
> 
> ---------------------------------- cut here -----------------------------------
> 
> clarify-nand-ecc-access.patch
> 
> The ECC access calculation in nand_read_subpage is quite hard to
> understand. This patch makes it more readable.
> 
> There is a small change in what the algorithm does: while
> if (eccpos[(start_step + num_steps) * chip->ecc.bytes] & (busw - 1))
> looks at the position of the ECC byte following the bytes we're
> currently reading,
> aligned_len = ALIGN(eccfrag_len+(pos-aligned_pos), busw);
> only looks at their length plus the additional data we have to read
> due to aligning the start position.
> 
> I believe the latter to be more correct than the former, since the
> next ECC byte could be located anywhere and its location therefore
> may not give the alignment information we seek. I'm not even sure
> its position is always defined (what if we're reading the last ECC
> of the page ?).
> 
> The last byte considered in the "gaps" test that checks that all ECC
> bytes are contiguous is
>   eccfrag_len-2+start_step*ecc.bytes+1 =
>   num_steps*ecc.bytes-2+start_step*ecc.bytes+1 =
>   (start_step+num+steps)*ecc.bytes-1
> so it doesn't include (start_step + num_steps) * chip->ecc.bytes.
> 
> The change also saves 44 bytes on ARM.
> 
> ---
> 
> Index: ktrack/drivers/mtd/nand/nand_base.c
> ===================================================================
> --- ktrack.orig/drivers/mtd/nand/nand_base.c	2008-11-02 02:28:19.000000000 -0200
> +++ ktrack/drivers/mtd/nand/nand_base.c	2008-11-02 02:38:22.000000000 -0200
> @@ -851,12 +851,12 @@
>  	} else {
>  		/* send the command to read the particular ecc bytes */
>  		/* take care about buswidth alignment in read_buf */
> -		aligned_pos = eccpos[start_step * chip->ecc.bytes] & ~(busw - 1);
> -		aligned_len = eccfrag_len;
> -		if (eccpos[start_step * chip->ecc.bytes] & (busw - 1))
> -			aligned_len++;
> -		if (eccpos[(start_step + num_steps) * chip->ecc.bytes] & (busw - 1))
> -			aligned_len++;
> +
> +		int pos;
> +
> +		pos = eccpos[start_step * chip->ecc.bytes];
> +		aligned_pos = pos & ~(busw - 1);
> +		aligned_len = ALIGN(eccfrag_len+(pos-aligned_pos), busw);
>  
>  		chip->cmdfunc(mtd, NAND_CMD_RNDOUT, mtd->writesize + aligned_pos, -1);
>  		chip->read_buf(mtd, &chip->oob_poi[aligned_pos], aligned_len);
> 
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
>
Werner Almesberger - Nov. 7, 2008, 5:09 a.m.
Alexey Korolev wrote:
> I studied your patch. I liked it. It improves readability.

Thanks !

> But I did not understand the note about a bug.
> What is the case when bug appears? (What is the distribution of ecc
> positions)

That bug is just for a hypothetical layout. If we assume that the
ECC layout can be completely arbitrary, then we could have a layout
that looks like this:

position	0 1 2 3 4 5 6 7 8 9 10
ECC block	- A A A B B B - C C C

Let's assume we want to read the ECC bytes AAA and BBB. Our bus
width is 2 bytes. With the original algorithm, the unaligned
position would be 1, the unaligned length 6. 

	/* no alignment: pos = 1, length = 6 */

	aligned_pos = eccpos[start_step * chip->ecc.bytes] & ~(busw - 1);

	/* aligned_pos = 0, ok */

	aligned_len = eccfrag_len;

	/* aligned_len = 6 */

	if (eccpos[start_step * chip->ecc.bytes] & (busw - 1))
		aligned_len++;

	/* the original start was unaligned, so aligned_len = 7 */

	if (eccpos[(start_step + num_steps) * chip->ecc.bytes] & (busw - 1))
		aligned_len++;

	/* the next block is aligned, so aligned_len = 7 */

I don't know if this is something one could encounter in real life,
but then, there are lot of people with crazy ideas out there ;-)

- Werner
Alexey Korolev - Nov. 7, 2008, 11:01 a.m.
Hi Werner,

Hah. You are right. But this case is so weird!
Please send patch in form of patch with your Signed-off line and add 
Acked-by: Alexey Korolev <akorolev@infradead.org>

Thank you very much for this patch!

Alexey

> 
> > But I did not understand the note about a bug.
> > What is the case when bug appears? (What is the distribution of ecc
> > positions)
> 
> That bug is just for a hypothetical layout. If we assume that the
> ECC layout can be completely arbitrary, then we could have a layout
> that looks like this:
> 
> position	0 1 2 3 4 5 6 7 8 9 10
> ECC block	- A A A B B B - C C C
> 
> Let's assume we want to read the ECC bytes AAA and BBB. Our bus
> width is 2 bytes. With the original algorithm, the unaligned
> position would be 1, the unaligned length 6. 
> 
> 	/* no alignment: pos = 1, length = 6 */
> 
> 	aligned_pos = eccpos[start_step * chip->ecc.bytes] & ~(busw - 1);
> 
> 	/* aligned_pos = 0, ok */
> 
> 	aligned_len = eccfrag_len;
> 
> 	/* aligned_len = 6 */
> 
> 	if (eccpos[start_step * chip->ecc.bytes] & (busw - 1))
> 		aligned_len++;
> 
> 	/* the original start was unaligned, so aligned_len = 7 */
> 
> 	if (eccpos[(start_step + num_steps) * chip->ecc.bytes] & (busw - 1))
> 		aligned_len++;
> 
> 	/* the next block is aligned, so aligned_len = 7 */
> 
> I don't know if this is something one could encounter in real life,
> but then, there are lot of people with crazy ideas out there ;-)
> 
> - Werner
>

Patch

Index: ktrack/drivers/mtd/nand/nand_base.c
===================================================================
--- ktrack.orig/drivers/mtd/nand/nand_base.c	2008-11-02 02:28:19.000000000 -0200
+++ ktrack/drivers/mtd/nand/nand_base.c	2008-11-02 02:38:22.000000000 -0200
@@ -851,12 +851,12 @@ 
 	} else {
 		/* send the command to read the particular ecc bytes */
 		/* take care about buswidth alignment in read_buf */
-		aligned_pos = eccpos[start_step * chip->ecc.bytes] & ~(busw - 1);
-		aligned_len = eccfrag_len;
-		if (eccpos[start_step * chip->ecc.bytes] & (busw - 1))
-			aligned_len++;
-		if (eccpos[(start_step + num_steps) * chip->ecc.bytes] & (busw - 1))
-			aligned_len++;
+
+		int pos;
+
+		pos = eccpos[start_step * chip->ecc.bytes];
+		aligned_pos = pos & ~(busw - 1);
+		aligned_len = ALIGN(eccfrag_len+(pos-aligned_pos), busw);
 
 		chip->cmdfunc(mtd, NAND_CMD_RNDOUT, mtd->writesize + aligned_pos, -1);
 		chip->read_buf(mtd, &chip->oob_poi[aligned_pos], aligned_len);