Re: [V3, 2/2] mtd: brcmnand: Detect sticky ucorr ecc error on dma reads

Boris Brezillon
2016-07-11T06:49:31Z

On Sun, 10 Jul 2016 22:04:52 -0700
Brian Norris <computersforpeace@gmail.com> wrote:

> Hi,
>
> Looking through Boris's pull request, I noticed an issue:
>
> On Thu, Jun 09, 2016 at 05:17:55PM -0400, Kamal Dasu wrote:
> > This change provides a fix for controller bug where nand
> > controller could have a possible sticky error after a PIO
> > followed by a DMA read. The fix retries a read if we see
> > a uncorr_ecc after read to detect such sticky errors.
> > The fix applies to only controller version 7.0 and 7.1.
> >
> > Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com>
> > ---
> > V3 changes
> > none
> > V2 Changes
> > Restrict controller bug workaround to version 7.0 and 7.1
> > ---
> > drivers/mtd/nand/brcmnand/brcmnand.c | 18 ++++++++++++++++++
> > 1 file changed, 18 insertions(+)
> >
> > diff --git a/drivers/mtd/nand/brcmnand/brcmnand.c b/drivers/mtd/nand/brcmnand/brcmnand.c
> > index 7ee9617..1156495 100644
> > --- a/drivers/mtd/nand/brcmnand/brcmnand.c
> > +++ b/drivers/mtd/nand/brcmnand/brcmnand.c
> > @@ -1602,9 +1602,11 @@ static int brcmnand_read(struct mtd_info *mtd, struct nand_chip *chip,
> > struct brcmnand_controller *ctrl = host->ctrl;
> > u64 err_addr = 0;
> > int err;
> > + static bool retry = true;
>
> Is this intentionally static? That means your retry will only be
> performed exactly once *ever*. Probably not what you expected?
>
> Boris, if this is indeed unintended, would you rather fix this up in the
> original patch and send me a new pull request?

I'll fix the initial commit and send a new PR once Kamal has confirmed
this variable should not be static (which is also my opinion).

> Or have my apply the
> trivial fixup (i.e., s/static//) as a separate patch on top?

Thanks,

Boris