Re: [PATCH 2/2] mtd: nand: nand_mxs: Fix readtotal calculation

[Stefan Lengfeld <s.lengfeld@phytec.de>]

Hi Sascha,

On Fri, Oct 21, 2016 at 09:18:08AM +0200, Sascha Hauer wrote:
> On Thu, Oct 20, 2016 at 03:40:07PM +0200, Christian Hemp wrote:
> > The calculation of readtotal must be bit alligend. If not the bch core
> > finds bit flips in every page, because readtotal is too small.
> > This bug was mostly introduced since commit "51061a9 mtd: nand: nand_mxs:
> > Add subpage read support".
> 
> Is this somehow related to:
> 
> http://lists.infradead.org/pipermail/linux-mtd/2016-June/068243.html
> 
> Or is this another topic?
> 
> Sascha

Yes, this patch fixes the same wrong assumption as in the patch above.  In the
above patch on the kernel list the assumption is that every block of
chunk+eccbits is byte aligned on the flash. So if the eccbits are not divisible
by 8, there are unused bits in the last byte of every block of chunk+eccbits.
That's not the case. In fact the second, third,... chunk will not be byte
aligned on the flash anymore. The blocks are bit shifted.

    AFAIR, the GPMI/ECC engine operates at the bit level (which is a pain
    to deal with BTW), and is only requiring a byte alignment on the total
    number of ECC bits. So here, DIV_ROUND_UP(18 * 13 * 4, 8) = 117, which
    fits in the 118 bytes (128 bytes - 10 bytes of 'metadata').
    [Source: http://lists.infradead.org/pipermail/linux-mtd/2016-June/068244.html]


In the patch below the line

> > -	ecc_parity_size = 13 * eccstrength / 8;

is the culprit. If the eccbits per chunk are not divisible by 8, a rounding
error occurs. Since C integer division truncates the fractional part, the byte
size in 'ecc_parity_size' underestimates the real size of the eccbits on the
flash.

The correct solution is to calculate all sizes in bits and then round up to the
next byte. So the code only overestimates the required read size.

I can only agree to Boris statement:

    AFAIR, the GPMI/ECC engine operates at the bit level (which is a pain
    to deal with BTW),

Dealing with bit aligned stuff is really awful.

Mit freundlichen Grüßen / Kind regards,
	Stefan Lengfeld

> 
> > 
> > Tested with:
> > nand: NAND device: Manufacturer ID: 0x01, Chip ID: 0xd3 (AMD/Spansion
> > S34ML08G2), 1024MiB, page size: 2048, OOB size: 128
> > 
> > nand: NAND device: Manufacturer ID: 0x2c, Chip ID: 0xdc (Micron
> > MT29F4G08ABADAWP), 512MiB, page size: 2048, OOB size: 64
> > 
> > nand: NAND device: Manufacturer ID: 0xec, Chip ID: 0xd3 (Samsung NAND
> > 1GiB 3,3V 8-bit), 1024MiB, page size: 2048, OOB size: 64
> > 
> > Signed-off-by: Christian Hemp <c.hemp@phytec.de>
> > Signed-off-by: Stefan Lengfeld <s.lengfeld@phytec.de>
> > ---
> >  drivers/mtd/nand/nand_mxs.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/nand_mxs.c b/drivers/mtd/nand/nand_mxs.c
> > index fe955e8..cba0bee 100644
> > --- a/drivers/mtd/nand/nand_mxs.c
> > +++ b/drivers/mtd/nand/nand_mxs.c
> > @@ -728,15 +728,15 @@ static int __mxs_nand_ecc_read_page(struct mtd_info *mtd, struct nand_chip *nand
> >  	uint32_t corrected = 0, failed = 0;
> >  	uint8_t	*status;
> >  	unsigned int  max_bitflips = 0;
> > -	int i, ret, readtotal, nchunks, eccstrength, ecc_parity_size;
> > +	int i, ret, readtotal, nchunks, eccstrength;
> >  
> >  	eccstrength = mxs_nand_get_ecc_strength(mtd->writesize, mtd->oobsize);
> >  
> >  	readlen = roundup(readlen, MXS_NAND_CHUNK_DATA_CHUNK_SIZE);
> >  	nchunks = mxs_nand_ecc_chunk_cnt(readlen);
> > -	ecc_parity_size = 13 * eccstrength / 8;
> > -	readtotal = MXS_NAND_METADATA_SIZE +
> > -		(MXS_NAND_CHUNK_DATA_CHUNK_SIZE + ecc_parity_size) * nchunks;
> > +	readtotal =  MXS_NAND_METADATA_SIZE;
> > +	readtotal += MXS_NAND_CHUNK_DATA_CHUNK_SIZE * nchunks;
> > +	readtotal += DIV_ROUND_UP(13 * eccstrength * nchunks, 8);
> >  
> >  	mxs_nand_config_bch(mtd, readtotal);
> >  
> > -- 
> > 1.9.1
> > 
> > 
> > _______________________________________________
> > barebox mailing list
> > barebox@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/barebox
> > 
> 
> -- 
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
> 
> _______________________________________________
> barebox mailing list
> barebox@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/barebox

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox