linux-kernel - Re: [PATCH v2 1/2] mtd: spi-nand: Add read retry support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAAyq3SYMh+DDB6Wzh623-0YcBXQ8UoQq4nK5+g5wmy4HyCY1uQ@mail.gmail.com>
Date: Fri, 22 Nov 2024 13:39:08 +0800
From: Cheng Ming Lin <linchengming884@...il.com>
To: Miquel Raynal <miquel.raynal@...tlin.com>
Cc: vigneshr@...com, linux-mtd@...ts.infradead.org, 
	linux-kernel@...r.kernel.org, richard@....at, alvinzhou@...c.com.tw, 
	leoyu@...c.com.tw, Cheng Ming Lin <chengminglin@...c.com.tw>
Subject: Re: [PATCH v2 1/2] mtd: spi-nand: Add read retry support

Hi Miquel,

Miquel Raynal <miquel.raynal@...tlin.com> 於 2024年11月18日 週一 下午6:31寫道：
>
>
> Hi Cheng Ming,
>
> On 14/11/2024 at 10:35:27 +08, Cheng Ming Lin <linchengming884@...il.com> wrote:
>
> > From: Cheng Ming Lin <chengminglin@...c.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
>
> Here is a suggestion for rewording the second part of your commit log:
>
> ... which can be setup by the host for the next read. There are several
> retry levels that can be attempted until the lost data is recovered or
> definitely assumed lost.
>

Thank you for your suggestion. I will make adjustments based on it.

> > for the next read retry mode and may recover the lost data by host
> > ECC again.
> >
> > Signed-off-by: Cheng Ming Lin <chengminglin@...c.com.tw>
> > ---
> >  drivers/mtd/nand/spi/core.c | 35 +++++++++++++++++++++++++++++++++--
> >  include/linux/mtd/spinand.h | 14 ++++++++++++++
> >  2 files changed, 47 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
> > index 4d76f9f71a0e..bd5339a308aa 100644
> > --- a/drivers/mtd/nand/spi/core.c
> > +++ b/drivers/mtd/nand/spi/core.c
> > @@ -672,10 +672,14 @@ static int spinand_mtd_regular_page_read(struct mtd_info *mtd, loff_t from,
> >       struct spinand_device *spinand = mtd_to_spinand(mtd);
> >       struct nand_device *nand = mtd_to_nanddev(mtd);
> >       struct nand_io_iter iter;
> > +     struct mtd_ecc_stats old_stats;
>
> Reverse christmas tree is nicer :)

Got it! I'll make the changes.

>
> >       bool disable_ecc = false;
> >       bool ecc_failed = false;
> > +     unsigned int retry_mode = 0;
> >       int ret;
> >
> > +     old_stats = mtd->ecc_stats;
> > +
> >       if (ops->mode == MTD_OPS_RAW || !mtd->ooblayout)
> >               disable_ecc = true;
> >
> > @@ -687,18 +691,43 @@ static int spinand_mtd_regular_page_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->set_read_retry) {
> > +                     if (spinand->read_retries && (++retry_mode < spinand->read_retries)) {
> > +                             ret = spinand->set_read_retry(spinand, retry_mode);
> > +                             if (ret < 0) {
> > +                                     ecc_failed = true;
> > +                                     break;
>
> What is this break gonna do? You're not in a loop. I don't think breaks
> have any effect on if blocks.

There's a nanddev_io_for_each_page iteration above, so the break
was intended to terminate that loop. However, I realized that break
should be replaced with return ret because if the set feature operation
fails, it should return the error.

>
> > +                             }
> > +
> > +                             /* 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);
> > +             }
> >
>
> Thanks,
> Miquèl

Thanks,
Cheng Ming Lin