On Mon, 2015-03-30 at 22:14 +0200, Stefan Agner wrote:
On 2015-03-30 19:02, Bill Pringlemeir wrote:
On 24 Mar 2015, stefan@agner.ch wrote:
The driver tries to re-use the page buffer by storing the page number of the current page in the buffer. The page is only read if the requested page number is not currently in the buffer. When a block is erased, the page number is marked as invalid if the erased page equals the one currently in the cache. However, since a erase block consists of multiple pages, also other page numbers could be affected.
The commands to reproduce this issue (on a written page):
nand dump 0x800 nand erase 0x0 0x20000 nand dump 0x800
The second nand dump command returns the data from the buffer, while in fact the page is erased (0xff).
Avoid the hassle to calculate whether the page is affected or not, but set the page buffer unconditionally to invalid instead.
Signed-off-by: Stefan Agner stefan@agner.ch
This are two bug fixes which would be nice if they would still make it into 2015.04...
drivers/mtd/nand/vf610_nfc.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/mtd/nand/vf610_nfc.c b/drivers/mtd/nand/vf610_nfc.c index 928d58b..9de971c 100644 --- a/drivers/mtd/nand/vf610_nfc.c +++ b/drivers/mtd/nand/vf610_nfc.c @@ -369,8 +369,7 @@ static void vf610_nfc_command(struct mtd_info *mtd, unsigned command, break;
case NAND_CMD_ERASE1: - if (nfc->page == page) - nfc->page = -1; + nfc->page = -1; vf610_nfc_send_commands(nfc->regs, command, NAND_CMD_ERASE2, ERASE_CMD_CODE); vf610_nfc_addr_cycle(mtd, column, page);
This change looks sensible. It is also possible that because sub-pages were removed that we could just remove the caching all together. It is possible that a higher layer may intentionally want to program and then do a read to verify.
Hm, I now realize that this actually happened somewhat by accident. Back when I migrated the driver to U-Boot, I removed the hwctl function since it was an empty function. This probably lead to the problem with sub page writes, which is why sub-page writes has been removed (by adding NAND_NO_SUBPAGE_WRITE).
Subpages can be supported without hwctl, by implementing the appropriate commands -- if the hardware supports it (e.g. some controllers only want to do ECC on full pages). There's a comment in the driver saying that "NFC does not support sub-page reads and writes".
If hwctl was empty it probably means that this controller does not expose an interface that matches well with hwctl.
However, in order to avoid a full page getting allocated and memcpy'ed when only writing a part of a page, we actually could re-enable that feature. But I'm not sure about the semantics of a subpage writes: Does the stack makes sure that the rest of the bytes are in the cache (e.g. read before write)? From what I understand, a subpage write would only copy (via vf610_nfc_write_buf) the data required into the the page cache, which then gets written to the device. Who makes sure that the rest of the page contains sound data?
If the rest of the page is all 0xff it shouldn't affect the existing data -- as long as the controller isn't writing some non-0xff ECC bytes as a result.
It's moot if subpage writes are disabled, though.
I guess we want to stay the same as the mainline Linux you are submitting. I haven't benchmarked the updates since sub-pages were removed to see if this is worth it. I think it was only ~10-20% in some benchmark I was doing with the 'caching'.
I think it mainly makes sense when the higher layer reads OOB and then the main data or visa-verse. I saw such access patterns at least in U-Boot.
Wouldn't it make more sense to not read a full page every time OOB is read?
At least in the small, this is a minimal change that is correct.
Ack-by: Bill Pringlemeir bpringlemeir@nbsps.com
Thanks for the Ack. If still possible, it would be nice to see that in 2015.04... :-)
I'd rather see the caching removed entirely.
-Scott