| Message ID | 1441296222-14989-6-git-send-email-boris.brezillon@free-electrons.com |
|---|---|
| State | Superseded |
| Headers | show |
+ others On Thu, Sep 03, 2015 at 06:03:42PM +0200, Boris Brezillon wrote: > Some drivers are manually checking for 'erased pages' while correcting > ECC bytes. > This logic is now done by the core infrastructure, and can thus be removed > from those drivers. > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> > --- > drivers/mtd/nand/davinci_nand.c | 8 -------- > drivers/mtd/nand/diskonchip.c | 32 ++------------------------------ > drivers/mtd/nand/jz4740_nand.c | 18 ------------------ > 3 files changed, 2 insertions(+), 56 deletions(-) > > diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c > index 816ef53..0d6adbe 100644 > --- a/drivers/mtd/nand/davinci_nand.c > +++ b/drivers/mtd/nand/davinci_nand.c > @@ -316,14 +316,6 @@ static int nand_davinci_correct_4bit(struct mtd_info *mtd, > unsigned num_errors, corrected; > unsigned long timeo; > > - /* All bytes 0xff? It's an erased page; ignore its ECC. */ > - for (i = 0; i < 10; i++) { > - if (ecc_code[i] != 0xff) > - goto compare; > - } > - return 0; > - > -compare: > /* Unpack ten bytes into eight 10 bit values. We know we're > * little-endian, and use type punning for less shifting/masking. > */ > diff --git a/drivers/mtd/nand/diskonchip.c b/drivers/mtd/nand/diskonchip.c > index 7da266a..eb65769 100644 > --- a/drivers/mtd/nand/diskonchip.c > +++ b/drivers/mtd/nand/diskonchip.c > @@ -936,37 +936,9 @@ static int doc200x_correct_data(struct mtd_info *mtd, u_char *dat, > calc_ecc[i] = ReadDOC_(docptr, DoC_Mplus_ECCSyndrome0 + i); > else > calc_ecc[i] = ReadDOC_(docptr, DoC_ECCSyndrome0 + i); > - if (calc_ecc[i] != empty_read_syndrome[i]) > - emptymatch = 0; > } > - /* If emptymatch=1, the read syndrome is consistent with an > - all-0xff data and stored ecc block. Check the stored ecc. */ > - if (emptymatch) { > - for (i = 0; i < 6; i++) { > - if (read_ecc[i] == 0xff) > - continue; > - emptymatch = 0; > - break; > - } > - } > - /* If emptymatch still =1, check the data block. */ > - if (emptymatch) { > - /* Note: this somewhat expensive test should not be triggered > - often. It could be optimized away by examining the data in > - the readbuf routine, and remembering the result. */ > - for (i = 0; i < 512; i++) { > - if (dat[i] == 0xff) > - continue; > - emptymatch = 0; > - break; > - } > - } > - /* If emptymatch still =1, this is almost certainly a freshly- > - erased block, in which case the ECC will not come out right. > - We'll suppress the error and tell the caller everything's > - OK. Because it is. */ > - if (!emptymatch) > - ret = doc_ecc_decode(rs_decoder, dat, calc_ecc); > + > + ret = doc_ecc_decode(rs_decoder, dat, calc_ecc); > if (ret > 0) > printk(KERN_ERR "doc200x_correct_data corrected %d errors\n", ret); > } I see new warnings: drivers/mtd/nand/diskonchip.c: At top level: drivers/mtd/nand/diskonchip.c: In function ‘doc200x_correct_data’: drivers/mtd/nand/diskonchip.c:915:6: warning: unused variable ‘emptymatch’ [-Wunused-variable] int emptymatch = 1; ^ drivers/mtd/nand/diskonchip.c:79:15: warning: ‘empty_read_syndrome’ defined but not used [-Wunused-variable] static u_char empty_read_syndrome[6] = { 0x26, 0xff, 0x6d, 0x47, 0x73, 0x7a }; ^ I'd also like test results for these drivers before doing this. And I'm not sure how to find good testers for these drivers, even it users still exist. > diff --git a/drivers/mtd/nand/jz4740_nand.c b/drivers/mtd/nand/jz4740_nand.c > index ba44af3..4d73276 100644 > --- a/drivers/mtd/nand/jz4740_nand.c > +++ b/drivers/mtd/nand/jz4740_nand.c > @@ -224,24 +224,6 @@ static int jz_nand_correct_ecc_rs(struct mtd_info *mtd, uint8_t *dat, > uint32_t t; > unsigned int timeout = 1000; > > - t = read_ecc[0]; > - > - if (t == 0xff) { > - for (i = 1; i < 9; ++i) > - t &= read_ecc[i]; > - > - t &= dat[0]; > - t &= dat[nand->chip.ecc.size / 2]; > - t &= dat[nand->chip.ecc.size - 1]; > - > - if (t == 0xff) { > - for (i = 1; i < nand->chip.ecc.size - 1; ++i) > - t &= dat[i]; > - if (t == 0xff) > - return 0; > - } > - } > - > for (i = 0; i < 9; ++i) > writeb(read_ecc[i], nand->base + JZ_REG_NAND_PAR0 + i); > > -- > 1.9.1 >
Hi Brian, On Mon, 21 Sep 2015 16:28:32 -0700 Brian Norris <computersforpeace@gmail.com> wrote: > + others > > On Thu, Sep 03, 2015 at 06:03:42PM +0200, Boris Brezillon wrote: > > Some drivers are manually checking for 'erased pages' while correcting > > ECC bytes. > > This logic is now done by the core infrastructure, and can thus be removed > > from those drivers. > > > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> > > --- > > drivers/mtd/nand/davinci_nand.c | 8 -------- > > drivers/mtd/nand/diskonchip.c | 32 ++------------------------------ > > drivers/mtd/nand/jz4740_nand.c | 18 ------------------ > > 3 files changed, 2 insertions(+), 56 deletions(-) > > > > diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c > > index 816ef53..0d6adbe 100644 > > --- a/drivers/mtd/nand/davinci_nand.c > > +++ b/drivers/mtd/nand/davinci_nand.c > > @@ -316,14 +316,6 @@ static int nand_davinci_correct_4bit(struct mtd_info *mtd, > > unsigned num_errors, corrected; > > unsigned long timeo; > > > > - /* All bytes 0xff? It's an erased page; ignore its ECC. */ > > - for (i = 0; i < 10; i++) { > > - if (ecc_code[i] != 0xff) > > - goto compare; > > - } > > - return 0; > > - > > -compare: > > /* Unpack ten bytes into eight 10 bit values. We know we're > > * little-endian, and use type punning for less shifting/masking. > > */ > > diff --git a/drivers/mtd/nand/diskonchip.c b/drivers/mtd/nand/diskonchip.c > > index 7da266a..eb65769 100644 > > --- a/drivers/mtd/nand/diskonchip.c > > +++ b/drivers/mtd/nand/diskonchip.c > > @@ -936,37 +936,9 @@ static int doc200x_correct_data(struct mtd_info *mtd, u_char *dat, > > calc_ecc[i] = ReadDOC_(docptr, DoC_Mplus_ECCSyndrome0 + i); > > else > > calc_ecc[i] = ReadDOC_(docptr, DoC_ECCSyndrome0 + i); > > - if (calc_ecc[i] != empty_read_syndrome[i]) > > - emptymatch = 0; > > } > > - /* If emptymatch=1, the read syndrome is consistent with an > > - all-0xff data and stored ecc block. Check the stored ecc. */ > > - if (emptymatch) { > > - for (i = 0; i < 6; i++) { > > - if (read_ecc[i] == 0xff) > > - continue; > > - emptymatch = 0; > > - break; > > - } > > - } > > - /* If emptymatch still =1, check the data block. */ > > - if (emptymatch) { > > - /* Note: this somewhat expensive test should not be triggered > > - often. It could be optimized away by examining the data in > > - the readbuf routine, and remembering the result. */ > > - for (i = 0; i < 512; i++) { > > - if (dat[i] == 0xff) > > - continue; > > - emptymatch = 0; > > - break; > > - } > > - } > > - /* If emptymatch still =1, this is almost certainly a freshly- > > - erased block, in which case the ECC will not come out right. > > - We'll suppress the error and tell the caller everything's > > - OK. Because it is. */ > > - if (!emptymatch) > > - ret = doc_ecc_decode(rs_decoder, dat, calc_ecc); > > + > > + ret = doc_ecc_decode(rs_decoder, dat, calc_ecc); > > if (ret > 0) > > printk(KERN_ERR "doc200x_correct_data corrected %d errors\n", ret); > > } > > I see new warnings: > > drivers/mtd/nand/diskonchip.c: At top level: > drivers/mtd/nand/diskonchip.c: In function ‘doc200x_correct_data’: > drivers/mtd/nand/diskonchip.c:915:6: warning: unused variable ‘emptymatch’ [-Wunused-variable] > int emptymatch = 1; > ^ > drivers/mtd/nand/diskonchip.c:79:15: warning: ‘empty_read_syndrome’ defined but not used [-Wunused-variable] > static u_char empty_read_syndrome[6] = { 0x26, 0xff, 0x6d, 0x47, 0x73, 0x7a }; > ^ Oops, I'll fix those warnings. > > I'd also like test results for these drivers before doing this. And I'm > not sure how to find good testers for these drivers, even it users still > exist. Yep, the goal of this patch was to let people owning boards embedding those chips test the changes. If we don't get anyone to test it, maybe we should leave those drivers unchanged at the expense of doing twice the test in case of bitflips in erased pages or uncorrectable bitflips (which is really unlikely). Best Regards, Boris
diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c index 816ef53..0d6adbe 100644 --- a/drivers/mtd/nand/davinci_nand.c +++ b/drivers/mtd/nand/davinci_nand.c @@ -316,14 +316,6 @@ static int nand_davinci_correct_4bit(struct mtd_info *mtd, unsigned num_errors, corrected; unsigned long timeo; - /* All bytes 0xff? It's an erased page; ignore its ECC. */ - for (i = 0; i < 10; i++) { - if (ecc_code[i] != 0xff) - goto compare; - } - return 0; - -compare: /* Unpack ten bytes into eight 10 bit values. We know we're * little-endian, and use type punning for less shifting/masking. */ diff --git a/drivers/mtd/nand/diskonchip.c b/drivers/mtd/nand/diskonchip.c index 7da266a..eb65769 100644 --- a/drivers/mtd/nand/diskonchip.c +++ b/drivers/mtd/nand/diskonchip.c @@ -936,37 +936,9 @@ static int doc200x_correct_data(struct mtd_info *mtd, u_char *dat, calc_ecc[i] = ReadDOC_(docptr, DoC_Mplus_ECCSyndrome0 + i); else calc_ecc[i] = ReadDOC_(docptr, DoC_ECCSyndrome0 + i); - if (calc_ecc[i] != empty_read_syndrome[i]) - emptymatch = 0; } - /* If emptymatch=1, the read syndrome is consistent with an - all-0xff data and stored ecc block. Check the stored ecc. */ - if (emptymatch) { - for (i = 0; i < 6; i++) { - if (read_ecc[i] == 0xff) - continue; - emptymatch = 0; - break; - } - } - /* If emptymatch still =1, check the data block. */ - if (emptymatch) { - /* Note: this somewhat expensive test should not be triggered - often. It could be optimized away by examining the data in - the readbuf routine, and remembering the result. */ - for (i = 0; i < 512; i++) { - if (dat[i] == 0xff) - continue; - emptymatch = 0; - break; - } - } - /* If emptymatch still =1, this is almost certainly a freshly- - erased block, in which case the ECC will not come out right. - We'll suppress the error and tell the caller everything's - OK. Because it is. */ - if (!emptymatch) - ret = doc_ecc_decode(rs_decoder, dat, calc_ecc); + + ret = doc_ecc_decode(rs_decoder, dat, calc_ecc); if (ret > 0) printk(KERN_ERR "doc200x_correct_data corrected %d errors\n", ret); } diff --git a/drivers/mtd/nand/jz4740_nand.c b/drivers/mtd/nand/jz4740_nand.c index ba44af3..4d73276 100644 --- a/drivers/mtd/nand/jz4740_nand.c +++ b/drivers/mtd/nand/jz4740_nand.c @@ -224,24 +224,6 @@ static int jz_nand_correct_ecc_rs(struct mtd_info *mtd, uint8_t *dat, uint32_t t; unsigned int timeout = 1000; - t = read_ecc[0]; - - if (t == 0xff) { - for (i = 1; i < 9; ++i) - t &= read_ecc[i]; - - t &= dat[0]; - t &= dat[nand->chip.ecc.size / 2]; - t &= dat[nand->chip.ecc.size - 1]; - - if (t == 0xff) { - for (i = 1; i < nand->chip.ecc.size - 1; ++i) - t &= dat[i]; - if (t == 0xff) - return 0; - } - } - for (i = 0; i < 9; ++i) writeb(read_ecc[i], nand->base + JZ_REG_NAND_PAR0 + i);
Some drivers are manually checking for 'erased pages' while correcting ECC bytes. This logic is now done by the core infrastructure, and can thus be removed from those drivers. Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> --- drivers/mtd/nand/davinci_nand.c | 8 -------- drivers/mtd/nand/diskonchip.c | 32 ++------------------------------ drivers/mtd/nand/jz4740_nand.c | 18 ------------------ 3 files changed, 2 insertions(+), 56 deletions(-)