Re: [PATCH v2] mtd: rawnand: Add Macronix NAND read retry support
From: masonccyang
Date: Fri May 24 2019 - 03:44:01 EST
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.
=====================================================================