linux-kernel - Re: [PATCH v3] mtd: rawnand: Add support for Macronix NAND randomizer

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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <OFF66743F5.5B74B775-ON48258493.003332F7-48258493.0033F1E2@mxic.com.tw>
Date:   Mon, 14 Oct 2019 17:27:22 +0800
From:   masonccyang@...c.com.tw
To:     "Miquel Raynal" <miquel.raynal@...tlin.com>
Cc:     bbrezillon@...nel.org, computersforpeace@...il.com,
        dwmw2@...radead.org, frieder.schrempf@...tron.de,
        juliensu@...c.com.tw, linux-kernel@...r.kernel.org,
        linux-mtd@...ts.infradead.org, marek.vasut@...il.com,
        richard@....at, tglx@...utronix.de, vigneshr@...com
Subject: Re: [PATCH v3] mtd: rawnand: Add support for Macronix NAND randomizer


Hi Miquel,


> > changelog
> > v3:
> > To enable randomizer by specific DT property in children nodes,
> > mxic,enable-randomizer-otp;
> > 
> > v2:
> > To enable randomizer by checking chip options NAND_NO_SUBPAGE_WRITE
> > 
> > v1:
> > To enable randomizer by sys-fs
> > 
> > Signed-off-by: Mason Yang <masonccyang@...c.com.tw>
> > ---
> >  drivers/mtd/nand/raw/nand_macronix.c | 64 
++++++++++++++++++++++++++++++++++++
> 
> As long as you modify bindings, you should write a separate patch to
> update the documentation and get it acked by Rob Herring.

Do you mean I have to create a new device tree binding file by 
nand_macronix.txt 
for raw NAND device ?

> 
> >  1 file changed, 64 insertions(+)
> > 
> > diff --git a/drivers/mtd/nand/raw/nand_macronix.c 
b/drivers/mtd/nand/raw/
> nand_macronix.c
> > index 58511ae..d5df09a 100644
> > --- a/drivers/mtd/nand/raw/nand_macronix.c
> > +++ b/drivers/mtd/nand/raw/nand_macronix.c
> > @@ -11,6 +11,13 @@
> >  #define MACRONIX_READ_RETRY_BIT BIT(0)
> >  #define MACRONIX_NUM_READ_RETRY_MODES 6
> > 
> > +#define MACRONIX_RANDOMIZER_BIT BIT(1)
> > +#define ONFI_FEATURE_ADDR_MXIC_RANDOMIZER 0xB0
> > +#define MACRONIX_RANDOMIZER_ENPGM BIT(0)
> > +#define MACRONIX_RANDOMIZER_RANDEN BIT(1)
> > +#define MACRONIX_RANDOMIZER_RANDOPT BIT(2)
> > +#define MACRONIX_RANDOMIZER_MODE_EXIT ~MACRONIX_RANDOMIZER_ENPGM
> 
> I would rather prefer a 
> 
> #define ...RANDOMISER_MODE_ENTER (ENGPM | RANDEN | RANDOPT)
> #define ...RANDOMISER_MODE_EXIT (RANDEN | RANDOPT)
> 

okay.

> > +
> >  struct nand_onfi_vendor_macronix {
> >     u8 reserved;
> >     u8 reliability_func;
> > @@ -29,15 +36,72 @@ static int macronix_nand_setup_read_retry(struct 
> nand_chip *chip, int mode)
> >     return nand_set_features(chip, ONFI_FEATURE_ADDR_READ_RETRY, 
feature);
> >  }
> > 
> > +static void macronix_nand_randomizer_check_enable(struct nand_chip 
*chip)
> 
> You should return something and check it from the calling function.

okay, will fix it.

> 
> > +{
> > +   u8 feature[ONFI_SUBFEATURE_PARAM_LEN];
> > +   int ret;
> > +
> > +   ret = nand_get_features(chip, ONFI_FEATURE_ADDR_MXIC_RANDOMIZER,
> > +            feature);
> > +   if (feature[0]) {
> > +      pr_info("Macronix NAND randomizer enabled:0x%x\n", feature[0]);
> > +      return;
> > +   }
> > +
> > +   feature[0] = MACRONIX_RANDOMIZER_ENPGM | 
MACRONIX_RANDOMIZER_RANDEN |
> > +           MACRONIX_RANDOMIZER_RANDOPT;
> > +   ret = nand_set_features(chip, ONFI_FEATURE_ADDR_MXIC_RANDOMIZER,
> > +            feature);
> > +   if (ret)
> > +      goto err;
> > +
> > +   feature[0] = 0x0;
> > +   ret = nand_prog_page_op(chip, 0, 0, feature, 1);
> 
> What is this? A comment is needed.

it's needed for a OTP bit programming flow.

> 
> > +   if (ret)
> > +      goto err;
> > +
> > +   ret = nand_get_features(chip, ONFI_FEATURE_ADDR_MXIC_RANDOMIZER,
> > +            feature);
> > +   if (ret)
> > +      goto err;
> > +
> > +   feature[0] &= MACRONIX_RANDOMIZER_MODE_EXIT;
> > +   ret = nand_set_features(chip, ONFI_FEATURE_ADDR_MXIC_RANDOMIZER,
> > +            feature);
> > +   if (ret)
> > +      goto err;
> > +
> > +   pr_info("Macronix NAND randomizer enable ok\n");
> 
> The pr_info "ok" could be dropped, the "failed" one would go in
> nand_onfi_init() after a check on the return code.
> 
> Then, no more goto's.

got it, will fix it.

thanks for your time and comments.

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.

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