Re: [PATCH v2 13/16] mtd/docg3: add ECC correction code

[Robert Jarzmik <robert.jarzmik@free.fr>]

Mike Dunn <mikedunn@newsguy.com> writes:

> On 11/10/2011 12:05 AM, Robert Jarzmik wrote:
>>  
>> +if MTD_DOCG3
>> +config BCH_CONST_M
>> +	default 14
>> +config BCH_CONST_T
>> +	default 4
>> +endif
>
>
> It might be better to let the user set this in the kernel config.  Doing it here
> precludes the use of the algorithm by any other module that needs to use it with
> different parameters.
You're right.
I'll shift that to mioa701 board code, where I'm sure no other BCH is necessary.

>> +static int doc_ecc_bch_fix_data(struct docg3 *docg3, void *buf, u8 *hwecc)
> Nit: function name in comment is inconsistent with its actual name.
Good catch. Will fix.
>> +	for (i = 0; i < numerrs; i++)
>> +		if (errorpos[i] < DOC_ECC_BCH_COVERED_BYTES*8)
>> +			/* error is located in data, correct it */
>> +			change_bit(errorpos[i], buf);
>> +out:
>> +	doc_dbg("doc_ecc_bch_fix_data: flipped %d bits\n", numerrs);
>> +	return numerrs;
>> +}
>
>
> Where do you check for reads of a blank page?
On stack frame above. Look at doc_read_oob():
		if ((block0 >= DOC_LAYOUT_BLOCK_FIRST_DATA) &&
		    (eccconf1 & DOC_ECCCONF1_BCH_SYNDROM_ERR) &&
		    (eccconf1 & DOC_ECCCONF1_PAGE_IS_WRITTEN) &&
                                \---> this is the key
		    (ops->mode != MTD_OPS_RAW) &&
		    (nbdata == DOC_LAYOUT_PAGE_SIZE)) {
			ret = doc_ecc_bch_fix_data(docg3, buf, hwecc);

Here you see that I'll make the error correction only if the page was written
before. If it's blank, I continue reading without attempting ECC correction.

> Not specifically related to this patch, but... are you sure you want to
> initialize the ecc on every read?  I'm sure it's not necessary; you can just
> leave it on; maybe turn it off if doing raw reads.  I know this is the case for
> both the P3 and G4 when running under PalmOS / TrueFFS library.  I notice that
> this function has delays and polls the status register in between calls to
> cpu_relax(), so the performance hit is probably not insignificant, especiallu
> when done for every 512 byte page.
Well, that's some info.
And yes, it adds some latency.
Now for the necessity, I'm not fully convinced. I know that the ECC register is
set up differently for reads and writes (that's the
DOC_ECCCONF0_READ_MODE). When doc_read_oob() is called, I don't know if the
previous action was a read or a write ...

What I could do to improve performance would be to store in the docg3 private
data if last action was a read or a write, and perform the doc_*_page_ecc_init()
only if action changes. What do you think ?

> Another nit (also not specifically related to this patch): bad name for this
> function.  The ecc being read is not the BCH syndrome, as we now know.  This is
> a pet peeve of mine; M-sys abused that word by misapplying it to the byts read
> from the ecc hw, which confused the hell out of me as I tried to understand what
> the hw was generating.
Yes, I'll change the name to hw_ecc or something like that.

Thanks for the review.

-- 
Robert