| Message ID | 20081102055138.GB5177@almesberger.net |
|---|---|
| State | New, archived |
| Headers | show |
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/ >
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
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 >
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);