Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 21 May 2019 10:47:13 +0200
From:   Miquel Raynal <miquel.raynal@bootlin.com>
To:     masonccyang@mxic.com.tw
Cc:     bbrezillon@kernel.org, computersforpeace@gmail.com,
        dwmw2@infradead.org, frieder.schrempf@kontron.de,
        juliensu@mxic.com.tw, linux-kernel@vger.kernel.org,
        linux-mtd@lists.infradead.org, marek.vasut@gmail.com,
        richard@nod.at, vigneshr@ti.com, zhengxunli@mxic.com.tw
Subject: Re: [PATCH v2] mtd: rawnand: Add Macronix NAND read retry support
Message-ID: <20190521104713.4b3a7769@xps13>
In-Reply-To: <OFDCB9EA90.C6F8EA4C-ON48258401.000981AF-48258401.000ED713@mxic.com.tw>
References: <1558076001-29579-1-git-send-email-masonccyang@mxic.com.tw>
        <20190520143438.46248bfc@xps13>
        <OFDCB9EA90.C6F8EA4C-ON48258401.000981AF-48258401.000ED713@mxic.com.tw>
Organization: Bootlin
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8BIT
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi masonccyang@mxic.com.tw,

masonccyang@mxic.com.tw wrote on Tue, 21 May 2019 10:42:06 +0800:

> Hi Miquel,
>  
> > > Add support for Macronix NAND read retry.
> > > 
> > > Macronix NANDs support specific read operation for data recovery,
> > > which can be enabled/disabled with a SET/GET_FEATURE.
> > > Driver checks byte 167 of Vendor Blocks in ONFI parameter page table
> > > to see if this high-reliability function is supported.
> > > 
> > > Signed-off-by: Mason Yang <masonccyang@mxic.com.tw>
> > > ---
> > >  drivers/mtd/nand/raw/nand_macronix.c | 57   
> ++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 57 insertions(+)
> > > 
> > > diff --git a/drivers/mtd/nand/raw/nand_macronix.c   
> b/drivers/mtd/nand/raw/
> > nand_macronix.c  
> > > index e287e71..1a4dc92 100644
> > > --- a/drivers/mtd/nand/raw/nand_macronix.c
> > > +++ b/drivers/mtd/nand/raw/nand_macronix.c
> > > @@ -17,6 +17,62 @@
> > > 
> > >  #include "internals.h"
> > > 
> > > +#define MACRONIX_READ_RETRY_BIT BIT(0)
> > > +#define MACRONIX_READ_RETRY_MODE 6  
> > 
> > Can you name this define MACRONIX_NUM_READ_RETRY_MODES?  
> 
> okay, will fix.
> 
> >   
> > > +
> > > +struct nand_onfi_vendor_macronix {
> > > +   u8 reserved[1];  
> > 
> > Do you need this "[1]" ?  
> 
> okay, just u8 reserved;
> 
> >   
> > > +   u8 reliability_func;
> > > +} __packed;
> > > +
> > > +/*
> > > + * Macronix NANDs support using SET/GET_FEATURES to enter/exit read   
> retry mode
> > > + */
> > > +static int macronix_nand_setup_read_retry(struct nand_chip *chip, int   
> mode)
> > > +{
> > > +   u8 feature[ONFI_SUBFEATURE_PARAM_LEN];
> > > +   int ret, feature_addr = ONFI_FEATURE_ADDR_READ_RETRY;
> > > +
> > > +   if (chip->parameters.supports_set_get_features &&
> > > +       test_bit(feature_addr, chip->parameters.set_feature_list) &&
> > > +       test_bit(feature_addr, chip->parameters.get_feature_list)) {
> > > +      feature[0] = mode;
> > > +      ret =  nand_set_features(chip, feature_addr, feature);
> > > +      if (ret)
> > > +         pr_err("Failed to set read retry moded:%d\n", mode);  
> > 
> > Do you have to call nand_get_features() on error?  
> 
> okay
> 
> >   
> > > +
> > > +      ret =  nand_get_features(chip, feature_addr, feature);
> > > +      if (ret || feature[0] != mode)
> > > +         pr_err("Failed to verify read retry moded:%d(%d)\n",
> > > +                mode, feature[0]);  
> > 
> > if ret == 0 but feature[0] != mode, shouldn't you return an error?  
> 
> okay, will fix.
> 
> >   
> > > +   }
> > > +
> > > +   return ret;  
> > 
> > This will produce a Warning at compile time (ret may be used
> > uninitialized). Have you tested it?  
> 
> Tool chain I used is "gcc-arm-linux-gnueabi" and no Warning at compile 
> time.

What's the output of:
gcc-arm-linux-gnueabi -v
?

> 
> Patch it to:
> ----------------------------------------------------------------------------->  
>  static int macronix_nand_setup_read_retry(struct nand_chip *chip, int 
> mode)
>  {
>          u8 feature[ONFI_SUBFEATURE_PARAM_LEN];
>          int ret, feature_addr = ONFI_FEATURE_ADDR_READ_RETRY;
> 
>          if (chip->parameters.supports_set_get_features &&
>              test_bit(feature_addr, chip->parameters.set_feature_list) &&
>              test_bit(feature_addr, chip->parameters.get_feature_list)) {
> 
>                  feature[0] = mode;
>                  ret =  nand_set_features(chip, feature_addr, feature);

                         ^ extra space, please be careful with the
                         typos, and run checkpatch.pl --strict before
                         sending patches.

>                  if (ret) {
>                          pr_err("Failed to set read retry moded:%d\n", 
> mode);
>                          goto err_out;
>                  }
> 
>                  ret =  nand_get_features(chip, feature_addr, feature);
>                  if (ret) {
>                          pr_err("Failed to get read retry moded:%d\n", 
> mode);
>                          goto err_out;
>                  } else if (feature[0] != mode) {
>                          pr_err("Failed to verify read retry 
> moded:%d(%d)\n",
>                                  mode, feature[0]);
>                          return -EIO;

That's not what I meant. You can keep the former condition but if !ret
then ret = -EIO for instance.

>                  }
>          }
> 
>  err_out:
>          return ret;

Again, do not jump to a single return call, directly do the return from
the point where you want to quit the function.

The problem should be that ret may be used uninitialized, the compiler
should tell you that.

Thanks,
Miquèl