[PATCH] mtd: docg3 fix in-middle of blocks reads

[PATCH] mtd: docg3 fix in-middle of blocks reads

[Robert Jarzmik]

Corner case reads do not work, and return false data and
ECC. This case is typically seen in a ubifs usage, with a
read of type:
 - docg3 docg3: doc_read_oob(from=14882415, mode=1,
 data=(c30eca40:12), oob=(  (null):0))

This results in the following reads:
 - docg3 docg3: doc_read_data_area(buf=  (null), len=111)
 - docg3 docg3: doc_read_data_area(buf=c30eca40, len=12)
 - docg3 docg3: doc_read_data_area(buf=  (null), len=389)
 - docg3 docg3: doc_read_data_area(buf=  (null), len=0)
 - docg3 docg3: doc_read_data_area(buf=  (null), len=16)

If we suppose that the pages content is :
 - bytes 0 .. 111   : 0x0a
 - bytes 112 .. 255 : 0x0f
Then the returned bytes will be :
 - 111 times 0x0a (correct)
 - 0x0a 2 times and 0x0f 10 times (incorrect, should be
 0x0a,0x0f)
 - 0x0f 389 times (correct)
 - nothing
 - correct OOB

The reason seams that the first 111 bytes read ends between
the 2 docg3 planes, and that the first following read (in
the 12 bytes sequence, read of 16 bit word) returns the byte
of the rightmost plane duplicated in high and lower byte of
the word.

Fix this behaviour by ensuring that if the previous read
ended up in-between the 2 planes, there will be a first 1
byte read to get back to the beginning of leftmost plane.

Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
---
 drivers/mtd/devices/docg3.c |   34 +++++++++++++++++++++++-----------
 1 files changed, 23 insertions(+), 11 deletions(-)

diff --git a/drivers/mtd/devices/docg3.c b/drivers/mtd/devices/docg3.c
index 8272c02..3414031 100644
--- a/drivers/mtd/devices/docg3.c
+++ b/drivers/mtd/devices/docg3.c
@@ -227,7 +227,7 @@ static void doc_read_data_area(struct docg3 *docg3, void *buf, int len,
 	u8 data8, *dst8;
 
 	doc_dbg("doc_read_data_area(buf=%p, len=%d)\n", buf, len);
-	cdr = len & 0x3;
+	cdr = len & 0x1;
 	len4 = len - cdr;
 
 	if (first)
@@ -732,12 +732,24 @@ err:
  * @len: the number of bytes to be read (must be a multiple of 4)
  * @buf: the buffer to be filled in (or NULL is forget bytes)
  * @first: 1 if first time read, DOC_READADDRESS should be set
+ * @last_odd: 1 if last read ended up on an odd byte
+ *
+ * Reads bytes from a prepared page. There is a trickery here : if the last read
+ * ended up on an odd offset in the 1024 bytes double page, ie. between the 2
+ * planes, the first byte must be read apart. If a word (16bit) read was used,
+ * the read would return the byte of plane 2 as low *and* high endian, which
+ * will mess the read.
  *
  */
 static int doc_read_page_getbytes(struct docg3 *docg3, int len, u_char *buf,
-				  int first)
+				  int first, int last_odd)
 {
-	doc_read_data_area(docg3, buf, len, first);
+	if (last_odd && len > 0) {
+		doc_read_data_area(docg3, buf, 1, first);
+		doc_read_data_area(docg3, buf ? buf + 1 : buf, len - 1, 0);
+	} else {
+		doc_read_data_area(docg3, buf, len, first);
+	}
 	doc_delay(docg3, 2);
 	return len;
 }
@@ -887,20 +899,20 @@ static int doc_read_oob(struct mtd_info *mtd, loff_t from,
 		ret = doc_read_page_ecc_init(docg3, DOC_ECC_BCH_TOTAL_BYTES);
 		if (ret < 0)
 			goto err_in_read;
-		ret = doc_read_page_getbytes(docg3, skip, NULL, 1);
+		ret = doc_read_page_getbytes(docg3, skip, NULL, 1, 0);
 		if (ret < skip)
 			goto err_in_read;
-		ret = doc_read_page_getbytes(docg3, nbdata, buf, 0);
+		ret = doc_read_page_getbytes(docg3, nbdata, buf, 0, skip % 2);
 		if (ret < nbdata)
 			goto err_in_read;
 		doc_read_page_getbytes(docg3,
 				       DOC_LAYOUT_PAGE_SIZE - nbdata - skip,
-				       NULL, 0);
-		ret = doc_read_page_getbytes(docg3, nboob, oobbuf, 0);
+				       NULL, 0, (skip + nbdata) % 2);
+		ret = doc_read_page_getbytes(docg3, nboob, oobbuf, 0, 0);
 		if (ret < nboob)
 			goto err_in_read;
 		doc_read_page_getbytes(docg3, DOC_LAYOUT_OOB_SIZE - nboob,
-				       NULL, 0);
+				       NULL, 0, nboob % 2);
 
 		doc_get_bch_hw_ecc(docg3, hwecc);
 		eccconf1 = doc_register_readb(docg3, DOC_ECCCONF1);
@@ -1004,7 +1016,7 @@ static int doc_reload_bbt(struct docg3 *docg3)
 						     DOC_LAYOUT_PAGE_SIZE);
 		if (!ret)
 			doc_read_page_getbytes(docg3, DOC_LAYOUT_PAGE_SIZE,
-					       buf, 1);
+					       buf, 1, 0);
 		buf += DOC_LAYOUT_PAGE_SIZE;
 	}
 	doc_read_page_finish(docg3);
@@ -1064,10 +1076,10 @@ static int doc_get_erase_count(struct docg3 *docg3, loff_t from)
 	ret = doc_reset_seq(docg3);
 	if (!ret)
 		ret = doc_read_page_prepare(docg3, block0, block1, page,
-					    ofs + DOC_LAYOUT_WEAR_OFFSET);
+					    ofs + DOC_LAYOUT_WEAR_OFFSET, 0);
 	if (!ret)
 		ret = doc_read_page_getbytes(docg3, DOC_LAYOUT_WEAR_SIZE,
-					     buf, 1);
+					     buf, 1, 0);
 	doc_read_page_finish(docg3);
 
 	if (ret || (buf[0] != DOC_ERASE_MARK) || (buf[2] != DOC_ERASE_MARK))
-- 
1.7.5.4

Re: [PATCH] mtd: docg3 fix in-middle of blocks reads

[Robert Jarzmik]

Hi Artem,

Any chance this fix makes it into 3.4-rcN ?

Cheers.

--
Robert

Re: [PATCH] mtd: docg3 fix in-middle of blocks reads

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 806 bytes --]

On Mon, 2012-04-09 at 13:19 +0200, Robert Jarzmik wrote:
> Corner case reads do not work, and return false data and
> ECC. This case is typically seen in a ubifs usage, with a
> read of type:
>  - docg3 docg3: doc_read_oob(from=14882415, mode=1,
>  data=(c30eca40:12), oob=(  (null):0))
> 
> This results in the following reads:
>  - docg3 docg3: doc_read_data_area(buf=  (null), len=111)
>  - docg3 docg3: doc_read_data_area(buf=c30eca40, len=12)
>  - docg3 docg3: doc_read_data_area(buf=  (null), len=389)
>  - docg3 docg3: doc_read_data_area(buf=  (null), len=0)
>  - docg3 docg3: doc_read_data_area(buf=  (null), len=16)

I usually go my e-mails bottom-up, but since you pinged me I jumped at
your patch right away. Pushed to l2-mtd.git, thanks!

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] mtd: docg3 fix in-middle of blocks reads

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 337 bytes --]

On Thu, 2012-04-12 at 21:17 +0200, Robert Jarzmik wrote:
> Hi Artem,
> 
> Any chance this fix makes it into 3.4-rcN ?

Hmm, not sure about 3.4-rcN - it is up to him. But I'll try to ping him,
although I am going to be away for few weeks soon, so no guarantees.
Better ping him yourself.


-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] mtd: docg3 fix in-middle of blocks reads

[Robert Jarzmik]

Artem Bityutskiy <dedekind1@gmail.com> writes:
> Hmm, not sure about 3.4-rcN - it is up to him. But I'll try to ping him,
> although I am going to be away for few weeks soon, so no guarantees.
> Better ping him yourself.

OK.
But no stress, we're at rc2/3, that leaves me 3 weeks. I'm not totally convinced
he'll pick a single patch from anybody else than a maintainer in the -rc series
anyway.
But I'll try to ping him in 1 or 2 weeks, let's see what happens then.

Have a good vacation !

-- 
Robert

Re: [PATCH] mtd: docg3 fix in-middle of blocks reads

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 705 bytes --]

On Mon, 2012-04-09 at 13:19 +0200, Robert Jarzmik wrote:
> 
> The reason seams that the first 111 bytes read ends between
> the 2 docg3 planes, and that the first following read (in
> the 12 bytes sequence, read of 16 bit word) returns the byte
> of the rightmost plane duplicated in high and lower byte of
> the word.
> 
> Fix this behaviour by ensuring that if the previous read
> ended up in-between the 2 planes, there will be a first 1
> byte read to get back to the beginning of leftmost plane. 

Um, this seems complex. Wouldn't it be easier just to say "Don't Do That
Then" — always read a multiple of two bytes, even if the caller doesn't
care about the last byte?

-- 
dwmw2

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6171 bytes --]

Re: [PATCH] mtd: docg3 fix in-middle of blocks reads

[Robert Jarzmik]

David Woodhouse <dwmw2@infradead.org> writes:

> On Mon, 2012-04-09 at 13:19 +0200, Robert Jarzmik wrote:
>> 
>> The reason seams that the first 111 bytes read ends between
>> the 2 docg3 planes, and that the first following read (in
>> the 12 bytes sequence, read of 16 bit word) returns the byte
>> of the rightmost plane duplicated in high and lower byte of
>> the word.
>> 
>> Fix this behaviour by ensuring that if the previous read
>> ended up in-between the 2 planes, there will be a first 1
>> byte read to get back to the beginning of leftmost plane. 
>
> Um, this seems complex. Wouldn't it be easier just to say "Don't Do That
> Then" — always read a multiple of two bytes, even if the caller doesn't
> care about the last byte?

Hi David,

I don't get the "Don't Do That Then".

The caller here, UBIFS, wants to read 12 bytes at offset 111 => [
byte111..byte122]. Note that the number of wanted bytes is even, and it's the
offset which is _odd_.

The problem for the driver is to read the 111 first bytes and forget them, and
then store the next 12 bytes. I don't see an alternative here, apart from having
a bounce buffer to read the bytes [byte110..byte122], and then copy into the
destination buffer. And I really don't want the bounce buffer.

Do you see any other alternative ?

Cheers.

-- 
Robert