[U-Boot] [PATCH 1/2] mtd: vf610_nfc: mark page as dirty on block erase

[Scott Wood]

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 at 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 at 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 at 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