clarify calculation in nand_read_subpage and fix possible bug

Submitted by Werner Almesberger on Nov. 2, 2008, 5:51 a.m.

Details

Message ID 20081102055138.GB5177@almesberger.net
State New, archived
Headers show

Commit Message

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.

---

Comments

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 hide | download patch | download mbox

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);