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

[Stefan Agner]

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).
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...? However, if removing the caching in the
driver would lead to a performance drop, I would rather prefer to keep
it...

>> I guess we want to stay the same as the mainline Linux you are
>> submitting.
> 
> So fix Linux. :-)

The driver is actually not yet in Linux. That change however is included
in the latest revision. Hence, yes, we will :-)

--
Stefan