* Re: [PATCH 2/2] mtd: spi-nand: Add read retry support
@ 2024-09-09 12:34 kernel test robot
0 siblings, 0 replies; 7+ messages in thread
From: kernel test robot @ 2024-09-09 12:34 UTC (permalink / raw)
To: oe-kbuild; +Cc: lkp, Dan Carpenter
BCC: lkp@intel.com
CC: oe-kbuild-all@lists.linux.dev
In-Reply-To: <20240905055333.2363358-3-linchengming884@gmail.com>
References: <20240905055333.2363358-3-linchengming884@gmail.com>
TO: Cheng Ming Lin <linchengming884@gmail.com>
TO: miquel.raynal@bootlin.com
TO: vigneshr@ti.com
TO: linux-mtd@lists.infradead.org
TO: linux-kernel@vger.kernel.org
CC: richard@nod.at
CC: alvinzhou@mxic.com.tw
CC: leoyu@mxic.com.tw
CC: Cheng Ming Lin <chengminglin@mxic.com.tw>
Hi Cheng,
kernel test robot noticed the following build warnings:
[auto build test WARNING on mtd/nand/next]
[also build test WARNING on linus/master v6.11-rc7]
[cannot apply to next-20240909]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Cheng-Ming-Lin/mtd-spi-nand-Add-fixups-for-read-retry/20240905-135719
base: https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next
patch link: https://lore.kernel.org/r/20240905055333.2363358-3-linchengming884%40gmail.com
patch subject: [PATCH 2/2] mtd: spi-nand: Add read retry support
:::::: branch date: 4 days ago
:::::: commit date: 4 days ago
config: x86_64-randconfig-161-20240909 (https://download.01.org/0day-ci/archive/20240909/202409092034.1htCE96j-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <error27@gmail.com>
| Closes: https://lore.kernel.org/r/202409092034.1htCE96j-lkp@intel.com/
smatch warnings:
drivers/mtd/nand/spi/core.c:692 spinand_mtd_read() error: we previously assumed 'spinand->info->fixups' could be null (see line 666)
vim +692 drivers/mtd/nand/spi/core.c
7529df4652482c Peter Pan 2018-06-22 632
7529df4652482c Peter Pan 2018-06-22 633 static int spinand_mtd_read(struct mtd_info *mtd, loff_t from,
7529df4652482c Peter Pan 2018-06-22 634 struct mtd_oob_ops *ops)
7529df4652482c Peter Pan 2018-06-22 635 {
7529df4652482c Peter Pan 2018-06-22 636 struct spinand_device *spinand = mtd_to_spinand(mtd);
7529df4652482c Peter Pan 2018-06-22 637 struct nand_device *nand = mtd_to_nanddev(mtd);
7bea6056927727 Michał Kępień 2022-06-29 638 struct mtd_ecc_stats old_stats;
7529df4652482c Peter Pan 2018-06-22 639 unsigned int max_bitflips = 0;
7529df4652482c Peter Pan 2018-06-22 640 struct nand_io_iter iter;
3d1f08b032dc4e Miquel Raynal 2020-10-01 641 bool disable_ecc = false;
7529df4652482c Peter Pan 2018-06-22 642 bool ecc_failed = false;
ec2cf155ae799c Cheng Ming Lin 2024-09-05 643 u8 retry_mode = 0;
7529df4652482c Peter Pan 2018-06-22 644 int ret = 0;
7529df4652482c Peter Pan 2018-06-22 645
3d1f08b032dc4e Miquel Raynal 2020-10-01 646 if (ops->mode == MTD_OPS_RAW || !spinand->eccinfo.ooblayout)
3d1f08b032dc4e Miquel Raynal 2020-10-01 647 disable_ecc = true;
7529df4652482c Peter Pan 2018-06-22 648
7529df4652482c Peter Pan 2018-06-22 649 mutex_lock(&spinand->lock);
7529df4652482c Peter Pan 2018-06-22 650
7bea6056927727 Michał Kępień 2022-06-29 651 old_stats = mtd->ecc_stats;
7bea6056927727 Michał Kępień 2022-06-29 652
701981cab01696 Miquel Raynal 2020-08-27 653 nanddev_io_for_each_page(nand, NAND_PAGE_READ, from, ops, &iter) {
3d1f08b032dc4e Miquel Raynal 2020-10-01 654 if (disable_ecc)
3d1f08b032dc4e Miquel Raynal 2020-10-01 655 iter.req.mode = MTD_OPS_RAW;
7529df4652482c Peter Pan 2018-06-22 656
3d1f08b032dc4e Miquel Raynal 2020-10-01 657 ret = spinand_select_target(spinand, iter.req.pos.target);
7529df4652482c Peter Pan 2018-06-22 658 if (ret)
7529df4652482c Peter Pan 2018-06-22 659 break;
7529df4652482c Peter Pan 2018-06-22 660
ec2cf155ae799c Cheng Ming Lin 2024-09-05 661 read_retry:
3d1f08b032dc4e Miquel Raynal 2020-10-01 662 ret = spinand_read_page(spinand, &iter.req);
7529df4652482c Peter Pan 2018-06-22 663 if (ret < 0 && ret != -EBADMSG)
7529df4652482c Peter Pan 2018-06-22 664 break;
7529df4652482c Peter Pan 2018-06-22 665
ec2cf155ae799c Cheng Ming Lin 2024-09-05 @666 if (ret == -EBADMSG && spinand->info->fixups) {
ec2cf155ae799c Cheng Ming Lin 2024-09-05 667 if (spinand->read_retries && ((retry_mode + 1) < spinand->read_retries)) {
ec2cf155ae799c Cheng Ming Lin 2024-09-05 668 retry_mode++;
ec2cf155ae799c Cheng Ming Lin 2024-09-05 669 ret = spinand->info->fixups->setup_read_retry(spinand, retry_mode);
ec2cf155ae799c Cheng Ming Lin 2024-09-05 670 if (ret < 0)
ec2cf155ae799c Cheng Ming Lin 2024-09-05 671 break;
ec2cf155ae799c Cheng Ming Lin 2024-09-05 672
ec2cf155ae799c Cheng Ming Lin 2024-09-05 673 /* Reset ecc_stats; retry */
ec2cf155ae799c Cheng Ming Lin 2024-09-05 674 mtd->ecc_stats = old_stats;
ec2cf155ae799c Cheng Ming Lin 2024-09-05 675 goto read_retry;
ec2cf155ae799c Cheng Ming Lin 2024-09-05 676 } else {
ec2cf155ae799c Cheng Ming Lin 2024-09-05 677 /* No more retry modes; real failure */
7529df4652482c Peter Pan 2018-06-22 678 ecc_failed = true;
ec2cf155ae799c Cheng Ming Lin 2024-09-05 679 }
ec2cf155ae799c Cheng Ming Lin 2024-09-05 680 } else if (ret == -EBADMSG) {
ec2cf155ae799c Cheng Ming Lin 2024-09-05 681 ecc_failed = true;
ec2cf155ae799c Cheng Ming Lin 2024-09-05 682 } else {
7529df4652482c Peter Pan 2018-06-22 683 max_bitflips = max_t(unsigned int, max_bitflips, ret);
ec2cf155ae799c Cheng Ming Lin 2024-09-05 684 }
7529df4652482c Peter Pan 2018-06-22 685
b83408b580eccf WeiXiong Liao 2019-06-28 686 ret = 0;
7529df4652482c Peter Pan 2018-06-22 687 ops->retlen += iter.req.datalen;
7529df4652482c Peter Pan 2018-06-22 688 ops->oobretlen += iter.req.ooblen;
ec2cf155ae799c Cheng Ming Lin 2024-09-05 689
ec2cf155ae799c Cheng Ming Lin 2024-09-05 690 /* Reset to retry mode 0*/
ec2cf155ae799c Cheng Ming Lin 2024-09-05 691 if (retry_mode) {
ec2cf155ae799c Cheng Ming Lin 2024-09-05 @692 ret = spinand->info->fixups->setup_read_retry(spinand, 0);
ec2cf155ae799c Cheng Ming Lin 2024-09-05 693 if (ret < 0)
ec2cf155ae799c Cheng Ming Lin 2024-09-05 694 break;
ec2cf155ae799c Cheng Ming Lin 2024-09-05 695 retry_mode = 0;
ec2cf155ae799c Cheng Ming Lin 2024-09-05 696 }
7529df4652482c Peter Pan 2018-06-22 697 }
7529df4652482c Peter Pan 2018-06-22 698
ec2cf155ae799c Cheng Ming Lin 2024-09-05 699
7bea6056927727 Michał Kępień 2022-06-29 700 if (ops->stats) {
7bea6056927727 Michał Kępień 2022-06-29 701 ops->stats->uncorrectable_errors +=
7bea6056927727 Michał Kępień 2022-06-29 702 mtd->ecc_stats.failed - old_stats.failed;
7bea6056927727 Michał Kępień 2022-06-29 703 ops->stats->corrected_bitflips +=
7bea6056927727 Michał Kępień 2022-06-29 704 mtd->ecc_stats.corrected - old_stats.corrected;
7bea6056927727 Michał Kępień 2022-06-29 705 }
7bea6056927727 Michał Kępień 2022-06-29 706
7529df4652482c Peter Pan 2018-06-22 707 mutex_unlock(&spinand->lock);
7529df4652482c Peter Pan 2018-06-22 708
7529df4652482c Peter Pan 2018-06-22 709 if (ecc_failed && !ret)
7529df4652482c Peter Pan 2018-06-22 710 ret = -EBADMSG;
7529df4652482c Peter Pan 2018-06-22 711
7529df4652482c Peter Pan 2018-06-22 712 return ret ? ret : max_bitflips;
7529df4652482c Peter Pan 2018-06-22 713 }
7529df4652482c Peter Pan 2018-06-22 714
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH 0/2] mtd: spi-nand: Add support for read retry
@ 2024-09-05 5:53 Cheng Ming Lin
2024-09-05 5:53 ` Cheng Ming Lin
0 siblings, 1 reply; 7+ messages in thread
From: Cheng Ming Lin @ 2024-09-05 5:53 UTC (permalink / raw)
To: miquel.raynal, vigneshr, linux-mtd, linux-kernel
Cc: richard, alvinzhou, leoyu, Cheng Ming Lin
From: Cheng Ming Lin <chengminglin@mxic.com.tw>
When the host ECC fails to correct the data error of NAND device,
there's a special read for data recovery method which host executes
the Special Read for Data Recovery operation and may recover the
lost data by host ECC again.
For more detailed information, please refer to page 27 in the link below:
Link: https://www.macronix.com/Lists/Datasheet/Attachments/9034/MX35LF1G24AD,%203V,%201Gb,%20v1.4.pdf
Cheng Ming Lin (2):
mtd: spi-nand: Add fixups for read retry
mtd: spi-nand: Add read retry support
drivers/mtd/nand/spi/core.c | 33 +++++++++++++-
drivers/mtd/nand/spi/macronix.c | 79 ++++++++++++++++++++++++++-------
include/linux/mtd/spinand.h | 17 +++++++
3 files changed, 112 insertions(+), 17 deletions(-)
--
2.25.1
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 2/2] mtd: spi-nand: Add read retry support
2024-09-05 5:53 [PATCH 0/2] mtd: spi-nand: Add support for read retry Cheng Ming Lin
@ 2024-09-05 5:53 ` Cheng Ming Lin
0 siblings, 0 replies; 7+ messages in thread
From: Cheng Ming Lin @ 2024-09-05 5:53 UTC (permalink / raw)
To: miquel.raynal, vigneshr, linux-mtd, linux-kernel
Cc: richard, alvinzhou, leoyu, Cheng Ming Lin
From: Cheng Ming Lin <chengminglin@mxic.com.tw>
When the host ECC fails to correct the data error of NAND device,
there's a special read for data recovery method which host setups
for the next read retry mode and may recover the lost data by host
ECC again.
Signed-off-by: Cheng Ming Lin <chengminglin@mxic.com.tw>
---
drivers/mtd/nand/spi/core.c | 33 +++++++++++++++++++++++++++++++--
1 file changed, 31 insertions(+), 2 deletions(-)
diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
index e0b6715e5dfe..2f21ea926132 100644
--- a/drivers/mtd/nand/spi/core.c
+++ b/drivers/mtd/nand/spi/core.c
@@ -640,6 +640,7 @@ static int spinand_mtd_read(struct mtd_info *mtd, loff_t from,
struct nand_io_iter iter;
bool disable_ecc = false;
bool ecc_failed = false;
+ u8 retry_mode = 0;
int ret = 0;
if (ops->mode == MTD_OPS_RAW || !spinand->eccinfo.ooblayout)
@@ -657,20 +658,45 @@ static int spinand_mtd_read(struct mtd_info *mtd, loff_t from,
if (ret)
break;
+read_retry:
ret = spinand_read_page(spinand, &iter.req);
if (ret < 0 && ret != -EBADMSG)
break;
- if (ret == -EBADMSG)
+ if (ret == -EBADMSG && spinand->info->fixups) {
+ if (spinand->read_retries && ((retry_mode + 1) < spinand->read_retries)) {
+ retry_mode++;
+ ret = spinand->info->fixups->setup_read_retry(spinand, retry_mode);
+ if (ret < 0)
+ break;
+
+ /* Reset ecc_stats; retry */
+ mtd->ecc_stats = old_stats;
+ goto read_retry;
+ } else {
+ /* No more retry modes; real failure */
+ ecc_failed = true;
+ }
+ } else if (ret == -EBADMSG) {
ecc_failed = true;
- else
+ } else {
max_bitflips = max_t(unsigned int, max_bitflips, ret);
+ }
ret = 0;
ops->retlen += iter.req.datalen;
ops->oobretlen += iter.req.ooblen;
+
+ /* Reset to retry mode 0*/
+ if (retry_mode) {
+ ret = spinand->info->fixups->setup_read_retry(spinand, 0);
+ if (ret < 0)
+ break;
+ retry_mode = 0;
+ }
}
+
if (ops->stats) {
ops->stats->uncorrectable_errors +=
mtd->ecc_stats.failed - old_stats.failed;
@@ -1095,6 +1121,9 @@ int spinand_match_and_init(struct spinand_device *spinand,
spinand->flags = table[i].flags;
spinand->id.len = 1 + table[i].devid.len;
spinand->select_target = table[i].select_target;
+ spinand->info = info;
+ if (spinand->info->fixups && spinand->info->fixups->init_read_retry)
+ spinand->read_retries = spinand->info->fixups->init_read_retry(spinand);
op = spinand_select_op_variant(spinand,
info->op_variants.read_cache);
--
2.25.1
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH 2/2] mtd: spi-nand: Add read retry support
@ 2024-09-05 5:53 ` Cheng Ming Lin
0 siblings, 0 replies; 7+ messages in thread
From: Cheng Ming Lin @ 2024-09-05 5:53 UTC (permalink / raw)
To: miquel.raynal, vigneshr, linux-mtd, linux-kernel
Cc: richard, alvinzhou, leoyu, Cheng Ming Lin
From: Cheng Ming Lin <chengminglin@mxic.com.tw>
When the host ECC fails to correct the data error of NAND device,
there's a special read for data recovery method which host setups
for the next read retry mode and may recover the lost data by host
ECC again.
Signed-off-by: Cheng Ming Lin <chengminglin@mxic.com.tw>
---
drivers/mtd/nand/spi/core.c | 33 +++++++++++++++++++++++++++++++--
1 file changed, 31 insertions(+), 2 deletions(-)
diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
index e0b6715e5dfe..2f21ea926132 100644
--- a/drivers/mtd/nand/spi/core.c
+++ b/drivers/mtd/nand/spi/core.c
@@ -640,6 +640,7 @@ static int spinand_mtd_read(struct mtd_info *mtd, loff_t from,
struct nand_io_iter iter;
bool disable_ecc = false;
bool ecc_failed = false;
+ u8 retry_mode = 0;
int ret = 0;
if (ops->mode == MTD_OPS_RAW || !spinand->eccinfo.ooblayout)
@@ -657,20 +658,45 @@ static int spinand_mtd_read(struct mtd_info *mtd, loff_t from,
if (ret)
break;
+read_retry:
ret = spinand_read_page(spinand, &iter.req);
if (ret < 0 && ret != -EBADMSG)
break;
- if (ret == -EBADMSG)
+ if (ret == -EBADMSG && spinand->info->fixups) {
+ if (spinand->read_retries && ((retry_mode + 1) < spinand->read_retries)) {
+ retry_mode++;
+ ret = spinand->info->fixups->setup_read_retry(spinand, retry_mode);
+ if (ret < 0)
+ break;
+
+ /* Reset ecc_stats; retry */
+ mtd->ecc_stats = old_stats;
+ goto read_retry;
+ } else {
+ /* No more retry modes; real failure */
+ ecc_failed = true;
+ }
+ } else if (ret == -EBADMSG) {
ecc_failed = true;
- else
+ } else {
max_bitflips = max_t(unsigned int, max_bitflips, ret);
+ }
ret = 0;
ops->retlen += iter.req.datalen;
ops->oobretlen += iter.req.ooblen;
+
+ /* Reset to retry mode 0*/
+ if (retry_mode) {
+ ret = spinand->info->fixups->setup_read_retry(spinand, 0);
+ if (ret < 0)
+ break;
+ retry_mode = 0;
+ }
}
+
if (ops->stats) {
ops->stats->uncorrectable_errors +=
mtd->ecc_stats.failed - old_stats.failed;
@@ -1095,6 +1121,9 @@ int spinand_match_and_init(struct spinand_device *spinand,
spinand->flags = table[i].flags;
spinand->id.len = 1 + table[i].devid.len;
spinand->select_target = table[i].select_target;
+ spinand->info = info;
+ if (spinand->info->fixups && spinand->info->fixups->init_read_retry)
+ spinand->read_retries = spinand->info->fixups->init_read_retry(spinand);
op = spinand_select_op_variant(spinand,
info->op_variants.read_cache);
--
2.25.1
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH 2/2] mtd: spi-nand: Add read retry support
2024-09-05 5:53 ` Cheng Ming Lin
@ 2024-10-01 10:17 ` Miquel Raynal
-1 siblings, 0 replies; 7+ messages in thread
From: Miquel Raynal @ 2024-10-01 10:17 UTC (permalink / raw)
To: Cheng Ming Lin
Cc: vigneshr, linux-mtd, linux-kernel, richard, alvinzhou, leoyu,
Cheng Ming Lin
Hi Cheng Ming,
linchengming884@gmail.com wrote on Thu, 5 Sep 2024 13:53:33 +0800:
> From: Cheng Ming Lin <chengminglin@mxic.com.tw>
>
> When the host ECC fails to correct the data error of NAND device,
> there's a special read for data recovery method which host setups
> for the next read retry mode and may recover the lost data by host
> ECC again.
>
> Signed-off-by: Cheng Ming Lin <chengminglin@mxic.com.tw>
> ---
> drivers/mtd/nand/spi/core.c | 33 +++++++++++++++++++++++++++++++--
> 1 file changed, 31 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
> index e0b6715e5dfe..2f21ea926132 100644
> --- a/drivers/mtd/nand/spi/core.c
> +++ b/drivers/mtd/nand/spi/core.c
> @@ -640,6 +640,7 @@ static int spinand_mtd_read(struct mtd_info *mtd, loff_t from,
> struct nand_io_iter iter;
> bool disable_ecc = false;
> bool ecc_failed = false;
> + u8 retry_mode = 0;
> int ret = 0;
>
> if (ops->mode == MTD_OPS_RAW || !spinand->eccinfo.ooblayout)
> @@ -657,20 +658,45 @@ static int spinand_mtd_read(struct mtd_info *mtd, loff_t from,
This will no longer apply with continuous support added in. However
please make this only in the non continuous path. I don't think it can
fly in the other.
> if (ret)
> break;
>
> +read_retry:
> ret = spinand_read_page(spinand, &iter.req);
> if (ret < 0 && ret != -EBADMSG)
> break;
>
> - if (ret == -EBADMSG)
> + if (ret == -EBADMSG && spinand->info->fixups) {
> + if (spinand->read_retries && ((retry_mode + 1) < spinand->read_retries)) {
++retry_mode?
> + retry_mode++;
So this can be dropped.
> + ret = spinand->info->fixups->setup_read_retry(spinand, retry_mode);
> + if (ret < 0)
> + break;
No, you need to set ecc_failed here.
> +
> + /* Reset ecc_stats; retry */
> + mtd->ecc_stats = old_stats;
> + goto read_retry;
> + } else {
> + /* No more retry modes; real failure */
> + ecc_failed = true;
> + }
> + } else if (ret == -EBADMSG) {
> ecc_failed = true;
> - else
> + } else {
> max_bitflips = max_t(unsigned int, max_bitflips, ret);
> + }
>
> ret = 0;
> ops->retlen += iter.req.datalen;
> ops->oobretlen += iter.req.ooblen;
> +
> + /* Reset to retry mode 0*/
> + if (retry_mode) {
retry_mode = 0;
> + ret = spinand->info->fixups->setup_read_retry(spinand, 0);
retry_mode);
> + if (ret < 0)
> + break;
this if clause is useless.
> + retry_mode = 0;
And then this can be dropped from here.
> + }
> }
>
> +
Spurious line
> if (ops->stats) {
> ops->stats->uncorrectable_errors +=
> mtd->ecc_stats.failed - old_stats.failed;
> @@ -1095,6 +1121,9 @@ int spinand_match_and_init(struct spinand_device *spinand,
> spinand->flags = table[i].flags;
> spinand->id.len = 1 + table[i].devid.len;
> spinand->select_target = table[i].select_target;
> + spinand->info = info;
> + if (spinand->info->fixups && spinand->info->fixups->init_read_retry)
> + spinand->read_retries = spinand->info->fixups->init_read_retry(spinand);
Now I get you init. Ok, fine.
>
> op = spinand_select_op_variant(spinand,
> info->op_variants.read_cache);
Thanks,
Miquèl
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH 2/2] mtd: spi-nand: Add read retry support
@ 2024-10-01 10:17 ` Miquel Raynal
0 siblings, 0 replies; 7+ messages in thread
From: Miquel Raynal @ 2024-10-01 10:17 UTC (permalink / raw)
To: Cheng Ming Lin
Cc: vigneshr, linux-mtd, linux-kernel, richard, alvinzhou, leoyu,
Cheng Ming Lin
Hi Cheng Ming,
linchengming884@gmail.com wrote on Thu, 5 Sep 2024 13:53:33 +0800:
> From: Cheng Ming Lin <chengminglin@mxic.com.tw>
>
> When the host ECC fails to correct the data error of NAND device,
> there's a special read for data recovery method which host setups
> for the next read retry mode and may recover the lost data by host
> ECC again.
>
> Signed-off-by: Cheng Ming Lin <chengminglin@mxic.com.tw>
> ---
> drivers/mtd/nand/spi/core.c | 33 +++++++++++++++++++++++++++++++--
> 1 file changed, 31 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
> index e0b6715e5dfe..2f21ea926132 100644
> --- a/drivers/mtd/nand/spi/core.c
> +++ b/drivers/mtd/nand/spi/core.c
> @@ -640,6 +640,7 @@ static int spinand_mtd_read(struct mtd_info *mtd, loff_t from,
> struct nand_io_iter iter;
> bool disable_ecc = false;
> bool ecc_failed = false;
> + u8 retry_mode = 0;
> int ret = 0;
>
> if (ops->mode == MTD_OPS_RAW || !spinand->eccinfo.ooblayout)
> @@ -657,20 +658,45 @@ static int spinand_mtd_read(struct mtd_info *mtd, loff_t from,
This will no longer apply with continuous support added in. However
please make this only in the non continuous path. I don't think it can
fly in the other.
> if (ret)
> break;
>
> +read_retry:
> ret = spinand_read_page(spinand, &iter.req);
> if (ret < 0 && ret != -EBADMSG)
> break;
>
> - if (ret == -EBADMSG)
> + if (ret == -EBADMSG && spinand->info->fixups) {
> + if (spinand->read_retries && ((retry_mode + 1) < spinand->read_retries)) {
++retry_mode?
> + retry_mode++;
So this can be dropped.
> + ret = spinand->info->fixups->setup_read_retry(spinand, retry_mode);
> + if (ret < 0)
> + break;
No, you need to set ecc_failed here.
> +
> + /* Reset ecc_stats; retry */
> + mtd->ecc_stats = old_stats;
> + goto read_retry;
> + } else {
> + /* No more retry modes; real failure */
> + ecc_failed = true;
> + }
> + } else if (ret == -EBADMSG) {
> ecc_failed = true;
> - else
> + } else {
> max_bitflips = max_t(unsigned int, max_bitflips, ret);
> + }
>
> ret = 0;
> ops->retlen += iter.req.datalen;
> ops->oobretlen += iter.req.ooblen;
> +
> + /* Reset to retry mode 0*/
> + if (retry_mode) {
retry_mode = 0;
> + ret = spinand->info->fixups->setup_read_retry(spinand, 0);
retry_mode);
> + if (ret < 0)
> + break;
this if clause is useless.
> + retry_mode = 0;
And then this can be dropped from here.
> + }
> }
>
> +
Spurious line
> if (ops->stats) {
> ops->stats->uncorrectable_errors +=
> mtd->ecc_stats.failed - old_stats.failed;
> @@ -1095,6 +1121,9 @@ int spinand_match_and_init(struct spinand_device *spinand,
> spinand->flags = table[i].flags;
> spinand->id.len = 1 + table[i].devid.len;
> spinand->select_target = table[i].select_target;
> + spinand->info = info;
> + if (spinand->info->fixups && spinand->info->fixups->init_read_retry)
> + spinand->read_retries = spinand->info->fixups->init_read_retry(spinand);
Now I get you init. Ok, fine.
>
> op = spinand_select_op_variant(spinand,
> info->op_variants.read_cache);
Thanks,
Miquèl
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH 2/2] mtd: spi-nand: Add read retry support
2024-10-01 10:17 ` Miquel Raynal
@ 2024-10-07 5:53 ` Cheng Ming Lin
-1 siblings, 0 replies; 7+ messages in thread
From: Cheng Ming Lin @ 2024-10-07 5:53 UTC (permalink / raw)
To: Miquel Raynal
Cc: vigneshr, linux-mtd, linux-kernel, richard, alvinzhou, leoyu,
Cheng Ming Lin
Hi Miquel,
Miquel Raynal <miquel.raynal@bootlin.com> 於 2024年10月1日 週二 下午6:17寫道：
>
> Hi Cheng Ming,
>
> linchengming884@gmail.com wrote on Thu, 5 Sep 2024 13:53:33 +0800:
>
> > From: Cheng Ming Lin <chengminglin@mxic.com.tw>
> >
> > When the host ECC fails to correct the data error of NAND device,
> > there's a special read for data recovery method which host setups
> > for the next read retry mode and may recover the lost data by host
> > ECC again.
> >
> > Signed-off-by: Cheng Ming Lin <chengminglin@mxic.com.tw>
> > ---
> > drivers/mtd/nand/spi/core.c | 33 +++++++++++++++++++++++++++++++--
> > 1 file changed, 31 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
> > index e0b6715e5dfe..2f21ea926132 100644
> > --- a/drivers/mtd/nand/spi/core.c
> > +++ b/drivers/mtd/nand/spi/core.c
> > @@ -640,6 +640,7 @@ static int spinand_mtd_read(struct mtd_info *mtd, loff_t from,
> > struct nand_io_iter iter;
> > bool disable_ecc = false;
> > bool ecc_failed = false;
> > + u8 retry_mode = 0;
> > int ret = 0;
> >
> > if (ops->mode == MTD_OPS_RAW || !spinand->eccinfo.ooblayout)
> > @@ -657,20 +658,45 @@ static int spinand_mtd_read(struct mtd_info *mtd, loff_t from,
>
> This will no longer apply with continuous support added in. However
> please make this only in the non continuous path. I don't think it can
> fly in the other.
Thanks for your helpful suggestions.
>
> > if (ret)
> > break;
> >
> > +read_retry:
> > ret = spinand_read_page(spinand, &iter.req);
> > if (ret < 0 && ret != -EBADMSG)
> > break;
> >
> > - if (ret == -EBADMSG)
> > + if (ret == -EBADMSG && spinand->info->fixups) {
> > + if (spinand->read_retries && ((retry_mode + 1) < spinand->read_retries)) {
>
> ++retry_mode?
> > + retry_mode++;
>
> So this can be dropped.
>
> > + ret = spinand->info->fixups->setup_read_retry(spinand, retry_mode);
> > + if (ret < 0)
> > + break;
>
> No, you need to set ecc_failed here.
>
> > +
> > + /* Reset ecc_stats; retry */
> > + mtd->ecc_stats = old_stats;
> > + goto read_retry;
> > + } else {
> > + /* No more retry modes; real failure */
> > + ecc_failed = true;
> > + }
> > + } else if (ret == -EBADMSG) {
> > ecc_failed = true;
> > - else
> > + } else {
> > max_bitflips = max_t(unsigned int, max_bitflips, ret);
> > + }
> >
> > ret = 0;
> > ops->retlen += iter.req.datalen;
> > ops->oobretlen += iter.req.ooblen;
> > +
> > + /* Reset to retry mode 0*/
> > + if (retry_mode) {
>
> retry_mode = 0;
>
> > + ret = spinand->info->fixups->setup_read_retry(spinand, 0);
>
> retry_mode);
>
> > + if (ret < 0)
> > + break;
>
> this if clause is useless.
>
> > + retry_mode = 0;
>
> And then this can be dropped from here.
>
> > + }
> > }
> >
> > +
>
> Spurious line
>
> > if (ops->stats) {
> > ops->stats->uncorrectable_errors +=
> > mtd->ecc_stats.failed - old_stats.failed;
> > @@ -1095,6 +1121,9 @@ int spinand_match_and_init(struct spinand_device *spinand,
> > spinand->flags = table[i].flags;
> > spinand->id.len = 1 + table[i].devid.len;
> > spinand->select_target = table[i].select_target;
> > + spinand->info = info;
> > + if (spinand->info->fixups && spinand->info->fixups->init_read_retry)
> > + spinand->read_retries = spinand->info->fixups->init_read_retry(spinand);
>
> Now I get you init. Ok, fine.
>
> >
> > op = spinand_select_op_variant(spinand,
> > info->op_variants.read_cache);
>
Thank you for all your suggestions.
I will modify the patch according to your advice.
>
> Thanks,
> Miquèl
Thanks,
Cheng Ming Lin
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH 2/2] mtd: spi-nand: Add read retry support
@ 2024-10-07 5:53 ` Cheng Ming Lin
0 siblings, 0 replies; 7+ messages in thread
From: Cheng Ming Lin @ 2024-10-07 5:53 UTC (permalink / raw)
To: Miquel Raynal
Cc: vigneshr, linux-mtd, linux-kernel, richard, alvinzhou, leoyu,
Cheng Ming Lin
Hi Miquel,
Miquel Raynal <miquel.raynal@bootlin.com> 於 2024年10月1日 週二 下午6:17寫道：
>
> Hi Cheng Ming,
>
> linchengming884@gmail.com wrote on Thu, 5 Sep 2024 13:53:33 +0800:
>
> > From: Cheng Ming Lin <chengminglin@mxic.com.tw>
> >
> > When the host ECC fails to correct the data error of NAND device,
> > there's a special read for data recovery method which host setups
> > for the next read retry mode and may recover the lost data by host
> > ECC again.
> >
> > Signed-off-by: Cheng Ming Lin <chengminglin@mxic.com.tw>
> > ---
> > drivers/mtd/nand/spi/core.c | 33 +++++++++++++++++++++++++++++++--
> > 1 file changed, 31 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
> > index e0b6715e5dfe..2f21ea926132 100644
> > --- a/drivers/mtd/nand/spi/core.c
> > +++ b/drivers/mtd/nand/spi/core.c
> > @@ -640,6 +640,7 @@ static int spinand_mtd_read(struct mtd_info *mtd, loff_t from,
> > struct nand_io_iter iter;
> > bool disable_ecc = false;
> > bool ecc_failed = false;
> > + u8 retry_mode = 0;
> > int ret = 0;
> >
> > if (ops->mode == MTD_OPS_RAW || !spinand->eccinfo.ooblayout)
> > @@ -657,20 +658,45 @@ static int spinand_mtd_read(struct mtd_info *mtd, loff_t from,
>
> This will no longer apply with continuous support added in. However
> please make this only in the non continuous path. I don't think it can
> fly in the other.
Thanks for your helpful suggestions.
>
> > if (ret)
> > break;
> >
> > +read_retry:
> > ret = spinand_read_page(spinand, &iter.req);
> > if (ret < 0 && ret != -EBADMSG)
> > break;
> >
> > - if (ret == -EBADMSG)
> > + if (ret == -EBADMSG && spinand->info->fixups) {
> > + if (spinand->read_retries && ((retry_mode + 1) < spinand->read_retries)) {
>
> ++retry_mode?
> > + retry_mode++;
>
> So this can be dropped.
>
> > + ret = spinand->info->fixups->setup_read_retry(spinand, retry_mode);
> > + if (ret < 0)
> > + break;
>
> No, you need to set ecc_failed here.
>
> > +
> > + /* Reset ecc_stats; retry */
> > + mtd->ecc_stats = old_stats;
> > + goto read_retry;
> > + } else {
> > + /* No more retry modes; real failure */
> > + ecc_failed = true;
> > + }
> > + } else if (ret == -EBADMSG) {
> > ecc_failed = true;
> > - else
> > + } else {
> > max_bitflips = max_t(unsigned int, max_bitflips, ret);
> > + }
> >
> > ret = 0;
> > ops->retlen += iter.req.datalen;
> > ops->oobretlen += iter.req.ooblen;
> > +
> > + /* Reset to retry mode 0*/
> > + if (retry_mode) {
>
> retry_mode = 0;
>
> > + ret = spinand->info->fixups->setup_read_retry(spinand, 0);
>
> retry_mode);
>
> > + if (ret < 0)
> > + break;
>
> this if clause is useless.
>
> > + retry_mode = 0;
>
> And then this can be dropped from here.
>
> > + }
> > }
> >
> > +
>
> Spurious line
>
> > if (ops->stats) {
> > ops->stats->uncorrectable_errors +=
> > mtd->ecc_stats.failed - old_stats.failed;
> > @@ -1095,6 +1121,9 @@ int spinand_match_and_init(struct spinand_device *spinand,
> > spinand->flags = table[i].flags;
> > spinand->id.len = 1 + table[i].devid.len;
> > spinand->select_target = table[i].select_target;
> > + spinand->info = info;
> > + if (spinand->info->fixups && spinand->info->fixups->init_read_retry)
> > + spinand->read_retries = spinand->info->fixups->init_read_retry(spinand);
>
> Now I get you init. Ok, fine.
>
> >
> > op = spinand_select_op_variant(spinand,
> > info->op_variants.read_cache);
>
Thank you for all your suggestions.
I will modify the patch according to your advice.
>
> Thanks,
> Miquèl
Thanks,
Cheng Ming Lin
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2024-10-07 5:53 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-09-09 12:34 [PATCH 2/2] mtd: spi-nand: Add read retry support kernel test robot
-- strict thread matches above, loose matches on Subject: below --
2024-09-05 5:53 [PATCH 0/2] mtd: spi-nand: Add support for read retry Cheng Ming Lin
2024-09-05 5:53 ` [PATCH 2/2] mtd: spi-nand: Add read retry support Cheng Ming Lin
2024-09-05 5:53 ` Cheng Ming Lin
2024-10-01 10:17 ` Miquel Raynal
2024-10-01 10:17 ` Miquel Raynal
2024-10-07 5:53 ` Cheng Ming Lin
2024-10-07 5:53 ` Cheng Ming Lin
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.