Re: [PATCH v2] mtd: rawnand: Add Macronix NAND read retry support

[masonccyang@xxxxxxxxxxx]

Hi Miquel,


> > > 
> > > > +
> > > > +      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
> ?
> 

Oops, it's gcc 4.8.3 20131111 and kind of obsolete.
That's why no Warning at compile time.

> > 
> > 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.

got it and thanks for your review.

> 
> Thanks,
> Miquèl

best regards,
Mason

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information 
and/or personal data, which is protected by applicable laws. Please be 
reminded that duplication, disclosure, distribution, or use of this e-mail 
(and/or its attachments) or any part thereof is prohibited. If you receive 
this e-mail in error, please notify us immediately and delete this mail as 
well as its attachment(s) from your system. In addition, please be 
informed that collection, processing, and/or use of personal data is 
prohibited unless expressly permitted by personal data protection laws. 
Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================



============================================================================

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information and/or personal data, which is protected by applicable laws. Please be reminded that duplication, disclosure, distribution, or use of this e-mail (and/or its attachments) or any part thereof is prohibited. If you receive this e-mail in error, please notify us immediately and delete this mail as well as its attachment(s) from your system. In addition, please be informed that collection, processing, and/or use of personal data is prohibited unless expressly permitted by personal data protection laws. Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/