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

[Scott Wood]

On Mon, 2015-03-30 at 22:40 +0200, Stefan Agner wrote:
> On 2015-03-30 22:34, Scott Wood wrote:
> > On Mon, 2015-03-30 at 13:02 -0400, 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.
> > 
> > It's more than possible -- Peter Tyser posted patches to do exactly that
> > for command-line NAND writes.
> > 
> >> I had seen that different FS seem to do 'write' and then immediately
> >> follow with a read.  If you believe the controller and the write status
> >> was ok, then I think it is fine to reuse the existing buffer and keep
> >> this caching.
> > 
> > If the upper layers want to cache then let them cache.
> > 
> 
> In this case, the lower layer is doing the caching (the driver).

It shouldn't.

> Depending on what the idea was behind the reread (e.g. check the amount
> of ECC erros just after write?), this caching inside the driver might
> miss lead the upper layers...?

Yes.  It's also extra complexity that has already led to problems, as
this patchset demonstrates.

>  However, if removing the caching in the driver would lead to a
> performance drop, I would rather prefer to keep it...

What is special about this controller, that caching makes sense here but
not on other controllers?  If it makes sense everywhere, then the upper
layer is the place to do it.

-Scott