From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from metis.ext.pengutronix.de ([2001:67c:670:201:290:27ff:fe1d:cc33]) by bombadil.infradead.org with esmtps (Exim 4.90_1 #2 (Red Hat Linux)) id 1gREWd-0003p0-FV for barebox@lists.infradead.org; Mon, 26 Nov 2018 10:54:01 +0000 Date: Mon, 26 Nov 2018 11:53:47 +0100 From: Roland Hieber Message-ID: <20181126105347.dlfdxjwdas3s57ia@pengutronix.de> References: <20181125225952.16159-1-r.hieber@pengutronix.de> <20181126090140.k443kixpzmcxhjim@pengutronix.de> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20181126090140.k443kixpzmcxhjim@pengutronix.de> List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "barebox" Errors-To: barebox-bounces+u.kleine-koenig=pengutronix.de@lists.infradead.org Subject: Re: [PATCH 1/2] drivers: caam: add RNG software self-test To: Sascha Hauer Cc: barebox@lists.infradead.org Hi Sascha, thanks for the feedback so far! Will fix things in v2. On Mon, Nov 26, 2018 at 10:01:40AM +0100, Sascha Hauer wrote: [...] > > diff --git a/drivers/crypto/caam/rng_self_test.c b/drivers/crypto/caam/rng_self_test.c > > new file mode 100644 > > index 0000000000..aff8f26cfd > > --- /dev/null > > +++ b/drivers/crypto/caam/rng_self_test.c > > @@ -0,0 +1,235 @@ > > +// SPDX-License-Identifier: GPL-2.0-or-later > > +/* > > + * Copyright 2018 NXP > > + * Copyright (C) 2018 Pengutronix, Roland Hieber > > + * > > + * CAAM RNG self-test -- based on the vendor patch for U-Boot: > > + * https://portland.source.codeaurora.org/patches/external/imxsupport/uboot-imx/imx_v2016.03_4.1.15_2.0.0_ga/HAB-238-Run-RNG-self-test-for-impacted-i.MX-chips.zip > > + * > > + * | From: Utkarsh Gupta > > + * | Subject: [PATCH] HAB-238 Run RNG self test for impacted i.MX chips > > + * | > > + * | Patch is only applicable to imx_v2016.03_4.1.15_2.0.0_ga branch of u-boot. > > + * | Please adapt the patch for your respective release version. > > + * | > > + * | Background: > > + * | Few i.MX chips which have HAB 4.2.3 or beyond, have oberserved following > > + * | warning message generated by HAB due to incorrect implementation of drng > > + * | self test in boot ROM. > > + * | > > + * | Event |0xdb|0x0024|0x42| SRCE Field: 69 30 e1 1d > > + * | | | | | STS = HAB_WARNING (0x69) > > + * | | | | | RSN = HAB_ENG_FAIL (0x30) > > + * | | | | | CTX = HAB_CTX_ENTRY (0xE1) > > + * | | | | | ENG = HAB_ENG_CAAM (0x1D) > > + * | | | | | Evt Data (hex): > > + * | | | | | 00 08 00 02 40 00 36 06 55 55 00 03 00 00 00 00 > > + * | | | | | 00 00 00 00 00 00 00 00 00 00 00 01 > > + * | > > + * | It is recommended to run this rng self test before any RNG related crypto > > + * | implementations are done. > > + * [...] > > + * | > > + * | Signed-off-by: Utkarsh Gupta > > + * > > + * Known impacted chips: > > + * > > + * - i.MX6DQ+ silicon revision 1.1 > > + * - i.MX6DQ silicon revision 1.6 > > + * - i.MX6DLS silicon revision 1.4 > > + * - i.MX6SX silicon revision 1.4 > > + * - i.MX6UL silicon revision 1.2 > > + * - i.MX67SD silicon revision 1.3 > > + */ > > + [...] > > +/* > > + * rng_self_test() - Perform RNG self test > > + * Returns zero on success, and negative on error. > > + */ > > +int rng_self_test(struct device_d *dev, const u8 caam_era, const u8 rngvid, const u8 rngrev) > > +{ > > Please add a caam_ prefix. > > > + int ret, i, desc_size = 0, job_err = 0; > > + const u32 *rng_st_dsc, *exp_result; > > + u32 *desc = 0; > > No need to initialize. > > > + u8 *result = 0; > > ditto. > > > + if (caam_era < 8 && rngvid == 4 && rngrev < 3) { > > + rng_st_dsc = rng_dsc1; > > + desc_size = DSC1SIZE; > > + exp_result = rng_result1; > > + } else if (rngvid != 3) { > > + rng_st_dsc = rng_dsc2; > > + desc_size = DSC2SIZE; > > + exp_result = rng_result2; > > + } else { > > + pr_err("Error: Invalid CAAM ERA (%d) or RNG Version ID (%d) or RNG revision (%d)\n", > > + caam_era, rngvid, rngrev); > > + return -EINVAL; > > Is this test really correct? Basically it says "We accept everything > except rngvid == 3". It also errors out when caam_era > 8 or rngrev >= 3. I'm not sure about the implementation details here why this was done, but that's literally what the NXP-authored code from U-Boot does. [...] > > + pr_vdebug("Result\n"); > > + for (i = 0; i < RESULT_SIZE; i++) { > > + pr_vdebug("%02X\n", result[i]); > > + } > > + > > + pr_vdebug("Expected Result:\n"); > > + for (i = 0; i < RESULT_SIZE; i++) { > > + pr_vdebug("%02X\n", exp_result[i]); > > + } > > Use memory_display to display this. Also this is only interesting if > both differ, so better print it in the failure path. I thought about that too, but I didn't see a way to make memory_display use pr_vdebug, or otherwise make its output depend on the debug level. - Roland -- Roland Hieber | r.hieber@pengutronix.de | Pengutronix e.K. | https://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim | Phone: +49-5121-206917-5086 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox