On 24.01.19 13:18, Boris Brezillon wrote:
<snip>
As it seems, adding "column" to the buffer address is not a good
idea. Here a short log with some debugging:
Oh, you're right, buf points to the OOB portion in case of OOB-only
accesses.
[ 27.721104] spinand_read_from_cache_op (241): ret=32 nbytes=128 column=2048
This small patch below of your last one, seems to fix this issue
for me:
root@mt7688:~# cat /sys/class/mtd/mtd5/bad_blocks
2
root@mt7688:~# ./nandbiterrs /dev/mtd5 -i
incremental biterrors test
Successfully corrected 0 bit errors per subpage
Inserted biterror @ 1/7
Read reported 4 corrected bit errors
Successfully corrected 1 bit errors per subpage
Inserted biterror @ 3/7
Read reported 4 corrected bit errors
Successfully corrected 2 bit errors per subpage
Inserted biterror @ 5/7
Read reported 4 corrected bit errors
Successfully corrected 3 bit errors per subpage
Inserted biterror @ 7/7
Read reported 4 corrected bit errors
Successfully corrected 4 bit errors per subpage
Inserted biterror @ 8/7
Read reported 5 corrected bit errors
Successfully corrected 5 bit errors per subpage
Inserted biterror @ 10/7
Read reported 6 corrected bit errors
Successfully corrected 6 bit errors per subpage
Inserted biterror @ 12/7
Read reported 7 corrected bit errors
Successfully corrected 7 bit errors per subpage
Inserted biterror @ 14/7
Read reported 8 corrected bit errors
Successfully corrected 8 bit errors per subpage
Inserted biterror @ 17/7
Failed to recover 1 bitflips
Read error after 9 bit errors per page
What do you think? Could you fold it into your patch if its
acceptable?
Actually, I asked Miquel to drop/revert the initial patch, so I'll
squash whatever makes it work for you in the original commit and send a
new version.
Thanks,
Stefan
--->8---
diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
index 659a0e716ee0..a5ccd4adc30f 100644
--- a/drivers/mtd/nand/spi/core.c
+++ b/drivers/mtd/nand/spi/core.c
@@ -218,6 +218,7 @@ static int spinand_read_from_cache_op(struct spinand_device *spinand,
unsigned int nbytes = 0;
void *buf = NULL;
u16 column = 0;
+ int offs = 0;
ssize_t ret;
if (req->datalen) {
@@ -237,7 +238,7 @@ static int spinand_read_from_cache_op(struct spinand_device *spinand,
rdesc = spinand->dirmaps[req->pos.plane].rdesc;
while (nbytes) {
- ret = spi_mem_dirmap_read(rdesc, column, nbytes, buf + column);
+ ret = spi_mem_dirmap_read(rdesc, column, nbytes, buf + offs);
if (ret < 0)
return ret;
@@ -246,6 +247,7 @@ static int spinand_read_from_cache_op(struct spinand_device *spinand,
nbytes -= ret;
column += ret;
+ offs += ret;
How about incrementing buf directly?
buf += ret;
Sure.
}
if (req->datalen)
@@ -273,6 +275,7 @@ static int spinand_write_to_cache_op(struct spinand_device *spinand,
struct mtd_info *mtd = nanddev_to_mtd(nand);
struct spi_mem_dirmap_desc *wdesc;
unsigned int nbytes, column = 0;
+ int offs = 0;
ssize_t ret;
nbytes = nanddev_page_size(nand) + nanddev_per_page_oobsize(nand);
@@ -297,7 +300,7 @@ static int spinand_write_to_cache_op(struct spinand_device *spinand,
while (nbytes) {
ret = spi_mem_dirmap_write(wdesc, column, nbytes,
- spinand->databuf + column);
+ spinand->databuf + offs);
In the write case 'spinand->databuf + column' works because we always
write the page entirely, but making the write path consistent with
the read one is a good thing, so let's add a buf pointer.
if (ret < 0)
return ret;
@@ -306,6 +309,7 @@ static int spinand_write_to_cache_op(struct spinand_device *spinand,
nbytes -= ret;
column += ret;
+ offs += ret;
}
return 0;
How about the following diff (hopefully the last one :-))?
--->8---
diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
index 52f17fc42daa..98e4ad74df7f 100644
--- a/drivers/mtd/nand/spi/core.c
+++ b/drivers/mtd/nand/spi/core.c
@@ -238,14 +238,16 @@ static int spinand_read_from_cache_op(struct spinand_device *spinand,
while (nbytes) {
while (nbytes) {
ret = spi_mem_dirmap_read(rdesc, column, nbytes, buf);
- if (!ret || ret > nbytes)
- ret = -EIO;
-
if (ret < 0)
return ret;
+ if (!ret || ret > nbytes)
+ return -EIO;
+
+
Double empty line.
nbytes -= ret;
column += ret;
+ buf += ret;
}
if (req->datalen)
@@ -273,6 +275,7 @@ static int spinand_write_to_cache_op(struct spinand_device *spinand,
struct mtd_info *mtd = nanddev_to_mtd(nand);
struct spi_mem_dirmap_desc *wdesc;
unsigned int nbytes, column = 0;
+ void *buf = spinand->databuf;
ssize_t ret;
nbytes = nanddev_page_size(nand) + nanddev_per_page_oobsize(nand);
@@ -296,16 +299,16 @@ static int spinand_write_to_cache_op(struct spinand_device *spinand,
wdesc = spinand->dirmaps[req->pos.plane].wdesc;
while (nbytes) {
- ret = spi_mem_dirmap_read(wdesc, column, nbytes,
- spinand->databuf + column);
- if (!ret || ret > nbytes)
- ret = -EIO;
-
+ ret = spi_mem_dirmap_write(wdesc, column, nbytes, buf);
if (ret < 0)
return ret;
+ if (!ret || ret > nbytes)
+ return -EIO;
+
nbytes -= ret;
column += ret;
+ buf += ret;
}
return 0;
@@ -761,21 +764,6 @@ static void spinand_destroy_dirmaps(struct spinand_device *spinand)
spinand_destroy_dirmap(spinand, i);
}
-const struct spi_mem_op *
-spinand_find_supported_op(struct spinand_device *spinand,
- const struct spi_mem_op *ops,
- unsigned int nops)
-{
- unsigned int i;
-
- for (i = 0; i < nops; i++) {
- if (spi_mem_supports_op(spinand->spimem, &ops[i]))
- return &ops[i];
- }
-
- return NULL;
-}
-
static const struct nand_ops spinand_ops = {
.erase = spinand_erase,
.markbad = spinand_markbad,
This one works as well. Feel free to add my:
Reviewed-by: Stefan Roese <sr@xxxxxxx>
Tested-by: Stefan Roese <sr@xxxxxxx>
tags if appropriate.
Another question though regarding older kernel versions (e.g. v4.19 LTS).
Could you perhaps push your fix [1] from yesterday as well?
Many thanks again for all your help here,
Stefan
[1] http://lists.infradead.org/pipermail/linux-mtd/2019-January/086931.html
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
