Re: [PATCH] [MTD] [OneNAND] Do not stop reading for ECC errors

[Adrian Hunter <ext-adrian.hunter@nokia.com>]

ext Jörn Engel wrote:
> On Mon, 5 November 2007 16:46:13 +0200, Adrian Hunter wrote:
>> -EBADMSG is an ECC error.
>>
>> ret must be non-zero to indicate that the bufferram is invalid.  Otherwise
>> if we read from the same location again, the data would be taken from 
>> bufferram
>> and no ECC error would be reported.  However we need to keep reading (see 
>> nand_base.c)
>> so ret is set to zero as though no error occured.  Finally 
>> mtd->ecc_stats.failed
>> is examined to determine ECC errors. i.e.
> 
> Hmm.
> 
>> static int onenand_read(struct mtd_info *mtd, loff_t from, size_t len,
>> 	size_t *retlen, u_char *buf)
>> {
>> 	...
>> 	stats = mtd->ecc_stats;
>> 	...
>> 	while (!ret) {
>> 		...
>> 		ret = this->wait(mtd, FL_READING);
>> 		onenand_update_bufferram(mtd, from, !ret);
>> 		if (ret == -EBADMSG)
>> 			ret = 0;
>> 	}
>> 	*retlen = read;
>> 	if (mtd->ecc_stats.failed - stats.failed)
>> 		return -EBADMSG;
>> 	if (ret)
>> 		return ret;
> 
> Those two conditionals should have reversed order, imo.

Very true.  I have added that fix to the patch also - see next mail.

> 
>>> What about -EUCLEAN?
>> The wait function doesn't return -EUCLEAN.  It increments 
>> mtd->ecc_stats.corrected and returns
>> zero for correctable ECC errors.
>>
>> File systems (like say JFFS2 for example) don't expect -EUCLEAN from 
>> mtd->read_oob and would treat
>> it as a fatal error, so it must not be returned in this case.
> 
> Such an interface sure looks strange.  But since read_oob is strange
> anyway I don't care very much either way.
> 
> Jörn
>