mail archive of the barebox mailing list
 help / color / mirror / Atom feed
* [PATCH 1/2] drivers: caam: add RNG software self-test
@ 2018-11-25 22:59 Roland Hieber
  2018-11-25 22:59 ` [PATCH 2/2] i.MX: HABv4: always print HAB status at boot time Roland Hieber
  2018-11-26  9:01 ` [PATCH 1/2] drivers: caam: add RNG software self-test Sascha Hauer
  0 siblings, 2 replies; 10+ messages in thread
From: Roland Hieber @ 2018-11-25 22:59 UTC (permalink / raw)
  To: barebox; +Cc: Roland Hieber

This patch is based on a vendor patch in U-Boot, taken from
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

|    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 <utkarsh.gupta@nxp.com>

Currently known impacted chips, as determined by NXP, include:
* 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

Port the RNG software self-test from this patch to barebox. It can be
enabled by selecting CRYPTO_DEV_FSL_CAAM_RNG_SELF_TEST in Kconfig.

The original patch included a command line utility to run the self-test,
but we choose a different approach here, and run the software self-test
automatically when the respective HAB events indicating a RNG ROM
self-test failure are found when running habv4_get_status(). Note that
habv4_get_status() must be called by the board code before the CAAM
device driver is probed for this mechanism to work.

Until now there are at least two such known events. The first event was
observed on an i.MX6Solo, silicon revision 1.4; the second event is
mentioned in the original patch description given above. When an event
occured, habv4_get_status() tests if it is one of those known events,
and if so, indicates to the CAAM driver to run the software self-test.
In this case, printing the respective HAB warning is suppressed to
prevent confusion; the software self-test itself will error out in case
of recurring RNG self-test failure.

Signed-off-by: Roland Hieber <r.hieber@pengutronix.de>
---
 drivers/crypto/caam/Kconfig         |  24 +++
 drivers/crypto/caam/Makefile        |   1 +
 drivers/crypto/caam/ctrl.c          |  22 +++
 drivers/crypto/caam/regs.h          |   4 +
 drivers/crypto/caam/rng_self_test.c | 235 ++++++++++++++++++++++++++++
 drivers/crypto/caam/rng_self_test.h |  24 +++
 drivers/hab/habv4.c                 |  46 +++++-
 7 files changed, 353 insertions(+), 3 deletions(-)
 create mode 100644 drivers/crypto/caam/rng_self_test.c
 create mode 100644 drivers/crypto/caam/rng_self_test.h

diff --git a/drivers/crypto/caam/Kconfig b/drivers/crypto/caam/Kconfig
index 2ab509d110..56b90700b8 100644
--- a/drivers/crypto/caam/Kconfig
+++ b/drivers/crypto/caam/Kconfig
@@ -33,3 +33,27 @@ config CRYPTO_DEV_FSL_CAAM_RNG
 	default y
 	help
 	  Selecting this will register the SEC4 hardware rng.
+
+if CRYPTO_DEV_FSL_CAAM_RNG
+
+config CRYPTO_DEV_FSL_CAAM_RNG_SELF_TEST
+	bool "Run RNG software self-test on impacted chips"
+	depends on ARCH_IMX6
+	depends on HABV4
+	default y
+	help
+	  Some chips with HAB >= 4.2.3 have an incorrect implementation of the
+	  RNG self-test in ROM code. In this case, a software self-test should
+	  be run to ensure correctness of the RNG. By enabling this config
+	  option, the software self-test is run automatically when this case
+	  is detected.
+
+	  Currently 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
+
+endif
diff --git a/drivers/crypto/caam/Makefile b/drivers/crypto/caam/Makefile
index 74d32da00e..7bd6f3e23c 100644
--- a/drivers/crypto/caam/Makefile
+++ b/drivers/crypto/caam/Makefile
@@ -3,3 +3,4 @@
 #
 obj-$(CONFIG_CRYPTO_DEV_FSL_CAAM) += ctrl.o error.o jr.o
 obj-$(CONFIG_CRYPTO_DEV_FSL_CAAM_RNG) += caamrng.o
+obj-$(CONFIG_CRYPTO_DEV_FSL_CAAM_RNG_SELF_TEST) += rng_self_test.o
diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c
index 9e62bd6fd6..14dfc7e827 100644
--- a/drivers/crypto/caam/ctrl.c
+++ b/drivers/crypto/caam/ctrl.c
@@ -19,10 +19,13 @@
 #include "desc_constr.h"
 #include "error.h"
 #include "ctrl.h"
+#include "rng_self_test.h"
 
 bool caam_little_end;
 EXPORT_SYMBOL(caam_little_end);
 
+extern bool habv4_need_rng_software_self_test;
+
 /*
  * Descriptor to instantiate RNG State Handle 0 in normal mode and
  * load the JDKEK, TDKEK and TDSK registers
@@ -570,6 +573,25 @@ static int caam_probe(struct device_d *dev)
 
 	cha_vid_ls = rd_reg32(&ctrl->perfmon.cha_id_ls);
 
+	/* habv4_need_rng_software_self_test is determined by habv4_get_status() */
+	if (IS_ENABLED(CONFIG_CRYPTO_DEV_FSL_CAAM_RNG_SELF_TEST) &&
+	    habv4_need_rng_software_self_test) {
+		u8 caam_era;
+		u8 rngvid;
+		u8 rngrev;
+
+		caam_era = (rd_reg32(&ctrl->perfmon.ccb_id) & CCBVID_ERA_MASK) >> CCBVID_ERA_SHIFT;
+		rngvid = (cha_vid_ls & CHAVID_LS_RNGVID_MASK) >> CHAVID_LS_RNGVID_SHIFT;
+		rngrev = (rd_reg32(&ctrl->perfmon.cha_rev_ls) & CRNR_LS_RNGRN_MASK) >> CRNR_LS_RNGRN_SHIFT;
+
+		pr_info("detected failure in CAAM RNG ROM self-test, running software self-test\n");
+		ret = rng_self_test(ctrlpriv->jrpdev[0], caam_era, rngvid, rngrev);
+		if (ret != 0) {
+			caam_remove(dev);
+			return ret;
+		}
+	}
+
 	/*
 	 * If SEC has RNG version >= 4 and RNG state handle has not been
 	 * already instantiated, do RNG instantiation
diff --git a/drivers/crypto/caam/regs.h b/drivers/crypto/caam/regs.h
index 6c9d6d75a0..19e7d6d7e4 100644
--- a/drivers/crypto/caam/regs.h
+++ b/drivers/crypto/caam/regs.h
@@ -279,6 +279,8 @@ struct caam_perfmon {
 
 	/* CAAM Hardware Instantiation Parameters		fa0-fbf */
 	u32 cha_rev_ms;		/* CRNR - CHA Rev No. Most significant half*/
+#define CRNR_LS_RNGRN_SHIFT	16
+#define CRNR_LS_RNGRN_MASK	(0xfull << CRNR_LS_RNGRN_SHIFT)
 	u32 cha_rev_ls;		/* CRNR - CHA Rev No. Least significant half*/
 #define CTPR_MS_QI_SHIFT	25
 #define CTPR_MS_QI_MASK		(0x1ull << CTPR_MS_QI_SHIFT)
@@ -311,6 +313,8 @@ struct caam_perfmon {
 #define CCBVID_ERA_SHIFT	24
 	u32 ccb_id;		/* CCBVID - CCB Version ID	*/
 	u32 cha_id_ms;		/* CHAVID - CHA Version ID Most Significant*/
+#define CHAVID_LS_RNGVID_SHIFT	16
+#define CHAVID_LS_RNGVID_MASK	(0xfull << CHAVID_LS_RNGVID_SHIFT)
 	u32 cha_id_ls;		/* CHAVID - CHA Version ID Least Significant*/
 	u32 cha_num_ms;		/* CHANUM - CHA Number Most Significant	*/
 	u32 cha_num_ls;		/* CHANUM - CHA Number Least Significant*/
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 <r.hieber@pengutronix.de>
+ *
+ * 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 <utkarsh.gupta@nxp.com>
+ * |    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 <utkarsh.gupta@nxp.com>
+ *
+ * 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
+ */
+
+#define pr_fmt(fmt) "rng_self_test: " fmt
+
+#include <dma.h>
+#include <common.h>
+#include "error.h"
+#include "regs.h"
+#include "jr.h"
+
+static inline u32 sec_in32(void *addr)
+{
+	return readl(addr);
+}
+
+#define DSC1SIZE 0x36
+#define DSC2SIZE 0x3A
+#define RESULT_SIZE 32
+
+static const u32 rng_dsc1[DSC1SIZE] = {
+	0xb0800036, 0x04800010, 0x3c85a15b, 0x50a9d0b1,
+	0x71a09fee, 0x2eecf20b, 0x02800020, 0xb267292e,
+	0x85bf712d, 0xe85ff43a, 0xa716b7fb, 0xc40bb528,
+	0x27b6f564, 0x8821cb5d, 0x9b5f6c26, 0x12a00020,
+	0x0a20de17, 0x6529357e, 0x316277ab, 0x2846254e,
+	0x34d23ba5, 0x6f5e9c32, 0x7abdc1bb, 0x0197a385,
+	0x82500405, 0xa2000001, 0x10880004, 0x00000005,
+	0x12820004, 0x00000020, 0x82500001, 0xa2000001,
+	0x10880004, 0x40000045, 0x02800020, 0x8f389cc7,
+	0xe7f7cbb0, 0x6bf2073d, 0xfc380b6d, 0xb22e9d1a,
+	0xee64fcb7, 0xa2b48d49, 0xdf9bc3a4, 0x82500009,
+	0xa2000001, 0x10880004, 0x00000005, 0x82500001,
+	0x60340020, 0xFFFFFFFF, 0xa2000001, 0x10880004,
+	0x00000005, 0x8250000d
+};
+
+static const u32 rng_result1[RESULT_SIZE] = {
+	0x3a, 0xfe, 0x2c, 0x87, 0xcc, 0xb6, 0x44, 0x49,
+	0x19, 0x16, 0x9a, 0x74, 0xa1, 0x31, 0x8b, 0xef,
+	0xf4, 0x86, 0x0b, 0xb9, 0x5e, 0xee, 0xae, 0x91,
+	0x92, 0xf4, 0xa9, 0x8f, 0xb0, 0x37, 0x18, 0xa4
+};
+
+static const u32 rng_dsc2[DSC2SIZE] = {
+	0xb080003a, 0x04800020, 0x27b73130, 0x30b4b10f,
+	0x7c62b1ad, 0x77abe899, 0x67452301, 0xefcdab89,
+	0x98badcfe, 0x10325476, 0x02800020, 0x63f757cf,
+	0xb9165584, 0xc3c1b407, 0xcc4ce8ad, 0x1ffe8a58,
+	0xfb4fa893, 0xbb5f4af0, 0x3fb946a1, 0x12a00020,
+	0x56cbcaa5, 0xfff3adad, 0xe804dcbf, 0x9a900c71,
+	0xa42017e3, 0x826948e2, 0xd0cfeb3e, 0xaf1a136a,
+	0x82500405, 0xa2000001, 0x10880004, 0x00000005,
+	0x12820004, 0x00000020, 0x82500001, 0xa2000001,
+	0x10880004, 0x40000045, 0x02800020, 0x2e882f8a,
+	0xe929943e, 0x8132c0a8, 0x12037f90, 0x809fbd66,
+	0x8684ea04, 0x00cbafa7, 0x7b82d12a, 0x82500009,
+	0xa2000001, 0x10880004, 0x00000005, 0x82500001,
+	0x60340020, 0xFFFFFFFF, 0xa2000001, 0x10880004,
+	0x00000005, 0x8250000d
+};
+
+static const u32 rng_result2[RESULT_SIZE] = {
+	0x76, 0x87, 0x66, 0x4e, 0xd8, 0x1d, 0x1f, 0x43,
+	0x76, 0x50, 0x85, 0x5d, 0x1e, 0x1d, 0x9d, 0x0f,
+	0x93, 0x75, 0x83, 0xff, 0x9a, 0x9b, 0x61, 0xa9,
+	0xa5, 0xeb, 0xa3, 0x28, 0x2a, 0x15, 0xc1, 0x57
+};
+
+/*
+ * construct_rng_self_test_jobdesc() - Implement destination address in RNG self test descriptors
+ * Returns zero on success, and negative on error.
+ */
+static int construct_rng_self_test_jobdesc(u32 *desc, const u32 *rng_st_dsc, u8 *res_addr, int desc_size)
+{
+	int result_addr_idx = desc_size - 5;
+	int i = 0;
+
+	if (!desc) {
+		return -EINVAL;
+	}
+
+	for (i = 0; i < desc_size; i++) {
+		desc[i] = rng_st_dsc[i];
+	}
+
+	/* Replace destination address in the descriptor */
+	desc[result_addr_idx] = (u32)res_addr;
+
+	pr_vdebug("RNG SELF TEST DESCRIPTOR:\n");
+	for (i = 0; i < desc_size; i++) {
+		pr_vdebug("0x%08X\n", desc[i]);
+	}
+
+	return 0;
+}
+
+/* rng_self_test_done() - callback for caam_jr_enqueue */
+static void rng_self_test_done(struct device_d *dev, u32 *desc, u32 err, void *arg)
+{
+	int * job_err = (int *)arg;
+	BUG_ON(!job_err);
+	*job_err = err;
+}
+
+/*
+ * 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)
+{
+	int ret, i, desc_size = 0, job_err = 0;
+	const u32 *rng_st_dsc, *exp_result;
+	u32 *desc = 0;
+	u8 *result = 0;
+
+	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;
+	}
+
+	result = dma_alloc(sizeof(uint32_t) * RESULT_SIZE);
+	if (!result) {
+		pr_err("Not enough memory for result buffer allocation\n");
+		return -ENOMEM;
+	}
+
+	desc = dma_alloc(sizeof(uint32_t) * desc_size);
+	if (!desc) {
+		pr_err("Not enough memory for descriptor allocation\n");
+		return -ENOMEM;
+	}
+
+	ret = construct_rng_self_test_jobdesc(desc, rng_st_dsc, result, desc_size);
+	if (ret) {
+		pr_err("Error in Job Descriptor Construction: %d %s\n",
+				ret, strerror(ret));
+		goto err;
+	} else {
+		dma_sync_single_for_device((unsigned long)desc,
+				desc_size * sizeof(uint32_t), DMA_TO_DEVICE);
+		dma_sync_single_for_device((unsigned long)result,
+				RESULT_SIZE * sizeof(uint32_t), DMA_FROM_DEVICE);
+
+		/* wait for job completion */
+		ret = caam_jr_enqueue(dev, desc, rng_self_test_done, &job_err);
+		if (ret) {
+			pr_err("Error while running RNG self-test descriptor: %d %s\n",
+					ret, strerror(ret));
+			goto err;
+		}
+		if (job_err) {
+			pr_err("Job Error:\n");
+			caam_jr_strstatus(dev, job_err);
+			goto err;
+		}
+	}
+
+	dma_sync_single_for_cpu((unsigned long)result, RESULT_SIZE * sizeof(uint32_t),
+			DMA_FROM_DEVICE);
+
+	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]);
+	}
+
+	for (i = 0; i < RESULT_SIZE; i++) {
+		if (result[i] != exp_result[i]) {
+			pr_err("RNG self-test failed with unexpected result\n");
+			ret = -ERANGE;
+			goto err;
+		}
+	}
+	pr_notice("RNG software self-test passed\n");
+	ret = 0;
+
+err:
+	dma_free(desc);
+	return ret;
+}
diff --git a/drivers/crypto/caam/rng_self_test.h b/drivers/crypto/caam/rng_self_test.h
new file mode 100644
index 0000000000..67356fe93b
--- /dev/null
+++ b/drivers/crypto/caam/rng_self_test.h
@@ -0,0 +1,24 @@
+/*
+ * CAAM RNG self test
+ *
+ * Copyright (C) 2018 Pengutronix, Roland Hieber <r.hieber@pengutronix.de>
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef RNG_SELF_TEST_H
+#define RNG_SELF_TEST_H
+
+int rng_self_test(struct device_d *dev, const u8 caam_era, const u8 rngvid, const u8 rngrev);
+
+#endif /* RNG_SELF_TEST_H */
diff --git a/drivers/hab/habv4.c b/drivers/hab/habv4.c
index 28fd42ecd7..e64f34870e 100644
--- a/drivers/hab/habv4.c
+++ b/drivers/hab/habv4.c
@@ -387,6 +387,27 @@ static void habv4_display_event(uint8_t *data, uint32_t len)
 	habv4_display_event_record((struct hab_event_record *)data);
 }
 
+/* Some chips with HAB >= 4.2.3 have an incorrect implementation of the RNG
+ * self-test in ROM code. In this case, an HAB event is generated, and a
+ * software self-test should be run. This variable is set to @c true by
+ * habv4_get_status() when this occurs. */
+bool habv4_need_rng_software_self_test = false;
+EXPORT_SYMBOL(habv4_need_rng_software_self_test);
+
+#define RNG_FAIL_EVENT_SIZE 36
+static uint8_t habv4_known_rng_fail_events[][RNG_FAIL_EVENT_SIZE] = {
+	{ 0xdb, 0x00, 0x24, 0x42,  0x69, 0x30, 0xe1, 0x1d,
+	  0x00, 0x80, 0x00, 0x02,  0x40, 0x00, 0x36, 0x06,
+	  0x55, 0x55, 0x00, 0x03,  0x00, 0x00, 0x00, 0x00,
+	  0x00, 0x00, 0x00, 0x00,  0x00, 0x00, 0x00, 0x00,
+	  0x00, 0x00, 0x00, 0x01 },
+	{ 0xdb, 0x00, 0x24, 0x42,  0x69, 0x30, 0xe1, 0x1d,
+	  0x00, 0x04, 0x00, 0x02,  0x40, 0x00, 0x36, 0x06,
+	  0x55, 0x55, 0x00, 0x03,  0x00, 0x00, 0x00, 0x00,
+	  0x00, 0x00, 0x00, 0x00,  0x00, 0x00, 0x00, 0x00,
+	  0x00, 0x00, 0x00, 0x01 },
+};
+
 static int habv4_get_status(const struct habv4_rvt *rvt)
 {
 	uint8_t data[256];
@@ -413,10 +434,29 @@ static int habv4_get_status(const struct habv4_rvt *rvt)
 
 	len = sizeof(data);
 	while (rvt->report_event(HAB_STATUS_WARNING, index, data, &len) == HAB_STATUS_SUCCESS) {
-		pr_err("-------- HAB warning Event %d --------\n", index);
-		pr_err("event data:\n");
 
-		habv4_display_event(data, len);
+		/* suppress RNG self-test fail events if they can be handled in software */
+		bool is_rng_fail_event = false;
+		if (IS_ENABLED(CONFIG_CRYPTO_DEV_FSL_CAAM_RNG_SELF_TEST)) {
+			int i;
+			for (i = 0; i < ARRAY_SIZE(habv4_known_rng_fail_events); i++) {
+				if (memcmp(data, habv4_known_rng_fail_events[i],
+					   min(len, (uint32_t)RNG_FAIL_EVENT_SIZE)) == 0) {
+					is_rng_fail_event = true;
+					break;
+				}
+			}
+		}
+
+		if (is_rng_fail_event) {
+			pr_warning("RNG self-test failure detected, will run software self-test\n");
+			habv4_need_rng_software_self_test = true;
+		} else {
+			pr_err("-------- HAB warning Event %d --------\n", index);
+			pr_err("event data:\n");
+			habv4_display_event(data, len);
+		}
+
 		len = sizeof(data);
 		index++;
 	}
-- 
2.19.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 2/2] i.MX: HABv4: always print HAB status at boot time
  2018-11-25 22:59 [PATCH 1/2] drivers: caam: add RNG software self-test Roland Hieber
@ 2018-11-25 22:59 ` Roland Hieber
  2018-11-26  7:56   ` Denis OSTERLAND
  2018-12-03 10:17   ` Denis OSTERLAND
  2018-11-26  9:01 ` [PATCH 1/2] drivers: caam: add RNG software self-test Sascha Hauer
  1 sibling, 2 replies; 10+ messages in thread
From: Roland Hieber @ 2018-11-25 22:59 UTC (permalink / raw)
  To: barebox; +Cc: Roland Hieber

Currently, board code needs to call habv4_get_status() explicitely, but
there is no reason that it cannot be called automatically at startup
when HABv4 is enabled. This way the call cannot be forgotten and we can
make sure to report all potentially occuring HAB warnings and errors.

Signed-off-by: Roland Hieber <r.hieber@pengutronix.de>
---
 drivers/hab/habv3.c |  4 ++++
 drivers/hab/habv4.c | 41 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+)

diff --git a/drivers/hab/habv3.c b/drivers/hab/habv3.c
index 82ae245f8a..c190c78d40 100644
--- a/drivers/hab/habv3.c
+++ b/drivers/hab/habv3.c
@@ -78,5 +78,9 @@ int imx_habv3_get_status(uint32_t status)
 
 int imx25_hab_get_status(void)
 {
+	if (!cpu_is_mx25()) {
+		return 0;
+	}
 	return imx_habv3_get_status(readl(IOMEM(0x780018d4)));
 }
+postmmu_initcall(imx25_hab_get_status);
diff --git a/drivers/hab/habv4.c b/drivers/hab/habv4.c
index e64f34870e..09f598631b 100644
--- a/drivers/hab/habv4.c
+++ b/drivers/hab/habv4.c
@@ -20,6 +20,7 @@
 
 #include <common.h>
 #include <hab.h>
+#include <init.h>
 #include <types.h>
 
 #include <mach/generic.h>
@@ -500,9 +501,49 @@ int imx6_hab_get_status(void)
 	return -EINVAL;
 }
 
+static int init_imx6_hab_get_status(void)
+{
+	int ret = 0;
+
+	if (!cpu_is_mx6())
+		/* can happen in multi-image builds and is not an error */
+		return 0;
+
+	ret = imx6_hab_get_status();
+
+	/* nobody will check the return value if there were HAB errors, but the
+	 * initcall will fail spectaculously with a strange error message. */
+	if (ret == -EPERM)
+		return 0;
+	return ret;
+}
+/* need to run before MMU setup because i.MX6 ROM code is mapped near 0x0,
+ * which will no longer be accessible when the MMU sets the zero page to
+ * faulting. */
+postconsole_initcall(init_imx6_hab_get_status);
+
 int imx28_hab_get_status(void)
 {
 	const struct habv4_rvt *rvt = (void *)HABV4_RVT_IMX28;
 
 	return habv4_get_status(rvt);
 }
+
+static int init_imx28_hab_get_status(void)
+{
+	int ret = 0;
+
+	if (!cpu_is_mx28())
+		/* can happen in multi-image builds and is not an error */
+		return 0;
+
+	ret = imx28_hab_get_status();
+
+	/* nobody will check the return value if there were HAB errors, but the
+	 * initcall will fail spectaculously with a strange error message. */
+	if (ret == -EPERM)
+		return 0;
+	return ret;
+}
+/* i.MX28 ROM code can be run after MMU setup to make use of caching */
+postmmu_initcall(init_imx28_hab_get_status);
-- 
2.19.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] i.MX: HABv4: always print HAB status at boot time
  2018-11-25 22:59 ` [PATCH 2/2] i.MX: HABv4: always print HAB status at boot time Roland Hieber
@ 2018-11-26  7:56   ` Denis OSTERLAND
  2018-12-03 10:17   ` Denis OSTERLAND
  1 sibling, 0 replies; 10+ messages in thread
From: Denis OSTERLAND @ 2018-11-26  7:56 UTC (permalink / raw)
  To: barebox

Am Sonntag, den 25.11.2018, 23:59 +0100 schrieb Roland Hieber:
> Currently, board code needs to call habv4_get_status() explicitely, but
> there is no reason that it cannot be called automatically at startup
> when HABv4 is enabled. This way the call cannot be forgotten and we can
> make sure to report all potentially occuring HAB warnings and errors.
Looks like the easier way, than trying to get this to work as a command.
I hope I found some time to test it.

Regards Denis

> 
> Signed-off-by: Roland Hieber <r.hieber@pengutronix.de>
> ---
>  drivers/hab/habv3.c |  4 ++++
>  drivers/hab/habv4.c | 41 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 45 insertions(+)
> 
> diff --git a/drivers/hab/habv3.c b/drivers/hab/habv3.c
> index 82ae245f8a..c190c78d40 100644
> --- a/drivers/hab/habv3.c
> +++ b/drivers/hab/habv3.c
> @@ -78,5 +78,9 @@ int imx_habv3_get_status(uint32_t status)
>  
>  int imx25_hab_get_status(void)
>  {
> +	if (!cpu_is_mx25()) {
> +		return 0;
> +	}
>  	return imx_habv3_get_status(readl(IOMEM(0x780018d4)));
>  }
> +postmmu_initcall(imx25_hab_get_status);
> diff --git a/drivers/hab/habv4.c b/drivers/hab/habv4.c
> index e64f34870e..09f598631b 100644
> --- a/drivers/hab/habv4.c
> +++ b/drivers/hab/habv4.c
> @@ -20,6 +20,7 @@
>  
>  #include <common.h>
>  #include <hab.h>
> +#include <init.h>
>  #include <types.h>
>  
>  #include <mach/generic.h>
> @@ -500,9 +501,49 @@ int imx6_hab_get_status(void)
>  	return -EINVAL;
>  }
>  
> +static int init_imx6_hab_get_status(void)
> +{
> +	int ret = 0;
> +
> +	if (!cpu_is_mx6())
> +		/* can happen in multi-image builds and is not an error */
> +		return 0;
> +
> +	ret = imx6_hab_get_status();
> +
> +	/* nobody will check the return value if there were HAB errors, but the
> +	 * initcall will fail spectaculously with a strange error message. */
> +	if (ret == -EPERM)
> +		return 0;
> +	return ret;
> +}
> +/* need to run before MMU setup because i.MX6 ROM code is mapped near 0x0,
> + * which will no longer be accessible when the MMU sets the zero page to
> + * faulting. */
> +postconsole_initcall(init_imx6_hab_get_status);
> +
>  int imx28_hab_get_status(void)
>  {
>  	const struct habv4_rvt *rvt = (void *)HABV4_RVT_IMX28;
>  
>  	return habv4_get_status(rvt);
>  }
> +
> +static int init_imx28_hab_get_status(void)
> +{
> +	int ret = 0;
> +
> +	if (!cpu_is_mx28())
> +		/* can happen in multi-image builds and is not an error */
> +		return 0;
> +
> +	ret = imx28_hab_get_status();
> +
> +	/* nobody will check the return value if there were HAB errors, but the
> +	 * initcall will fail spectaculously with a strange error message. */
> +	if (ret == -EPERM)
> +		return 0;
> +	return ret;
> +}
> +/* i.MX28 ROM code can be run after MMU setup to make use of caching */
> +postmmu_initcall(init_imx28_hab_get_status);

Diehl Connectivity Solutions GmbH
Geschäftsführung: Horst Leonberger
Sitz der Gesellschaft: Nürnberg - Registergericht: Amtsgericht
Nürnberg: HRB 32315
___________________________________________________________________________________________________

Der Inhalt der vorstehenden E-Mail ist nicht rechtlich bindend. Diese E-Mail enthaelt vertrauliche und/oder rechtlich geschuetzte Informationen.
Informieren Sie uns bitte, wenn Sie diese E-Mail faelschlicherweise erhalten haben. Bitte loeschen Sie in diesem Fall die Nachricht.
Jede unerlaubte Form der Reproduktion, Bekanntgabe, Aenderung, Verteilung und/oder Publikation dieser E-Mail ist strengstens untersagt.
The contents of the above mentioned e-mail is not legally binding. This e-mail contains confidential and/or legally protected information. Please inform us if you have received this e-mail by
mistake and delete it in such a case. Each unauthorized reproduction, disclosure, alteration, distribution and/or publication of this e-mail is strictly prohibited. 
_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] drivers: caam: add RNG software self-test
  2018-11-25 22:59 [PATCH 1/2] drivers: caam: add RNG software self-test Roland Hieber
  2018-11-25 22:59 ` [PATCH 2/2] i.MX: HABv4: always print HAB status at boot time Roland Hieber
@ 2018-11-26  9:01 ` Sascha Hauer
  2018-11-26 10:53   ` Roland Hieber
  2018-11-29 10:17   ` Roland Hieber
  1 sibling, 2 replies; 10+ messages in thread
From: Sascha Hauer @ 2018-11-26  9:01 UTC (permalink / raw)
  To: Roland Hieber; +Cc: barebox

Hi Roland,

On Sun, Nov 25, 2018 at 11:59:51PM +0100, Roland Hieber wrote:
> diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c
> index 9e62bd6fd6..14dfc7e827 100644
> --- a/drivers/crypto/caam/ctrl.c
> +++ b/drivers/crypto/caam/ctrl.c
> @@ -19,10 +19,13 @@
>  #include "desc_constr.h"
>  #include "error.h"
>  #include "ctrl.h"
> +#include "rng_self_test.h"
>  
>  bool caam_little_end;
>  EXPORT_SYMBOL(caam_little_end);
>  
> +extern bool habv4_need_rng_software_self_test;

Put the declaration into include/hab.h

> +
>  /*
>   * Descriptor to instantiate RNG State Handle 0 in normal mode and
>   * load the JDKEK, TDKEK and TDSK registers
> @@ -570,6 +573,25 @@ static int caam_probe(struct device_d *dev)
>  
>  	cha_vid_ls = rd_reg32(&ctrl->perfmon.cha_id_ls);
>  
> +	/* habv4_need_rng_software_self_test is determined by habv4_get_status() */
> +	if (IS_ENABLED(CONFIG_CRYPTO_DEV_FSL_CAAM_RNG_SELF_TEST) &&
> +	    habv4_need_rng_software_self_test) {
> +		u8 caam_era;
> +		u8 rngvid;
> +		u8 rngrev;
> +
> +		caam_era = (rd_reg32(&ctrl->perfmon.ccb_id) & CCBVID_ERA_MASK) >> CCBVID_ERA_SHIFT;
> +		rngvid = (cha_vid_ls & CHAVID_LS_RNGVID_MASK) >> CHAVID_LS_RNGVID_SHIFT;
> +		rngrev = (rd_reg32(&ctrl->perfmon.cha_rev_ls) & CRNR_LS_RNGRN_MASK) >> CRNR_LS_RNGRN_SHIFT;
> +
> +		pr_info("detected failure in CAAM RNG ROM self-test, running software self-test\n");

This looks unnecessary. You already printed the HAB failure event and
the rng_self_test() will output the result.

> +		ret = rng_self_test(ctrlpriv->jrpdev[0], caam_era, rngvid, rngrev);
> +		if (ret != 0) {
> +			caam_remove(dev);
> +			return ret;
> +		}
> +	}
> +
>  	/*
>  	 * If SEC has RNG version >= 4 and RNG state handle has not been
>  	 * already instantiated, do RNG instantiation
> diff --git a/drivers/crypto/caam/regs.h b/drivers/crypto/caam/regs.h
> index 6c9d6d75a0..19e7d6d7e4 100644
> --- a/drivers/crypto/caam/regs.h
> +++ b/drivers/crypto/caam/regs.h
> @@ -279,6 +279,8 @@ struct caam_perfmon {
>  
>  	/* CAAM Hardware Instantiation Parameters		fa0-fbf */
>  	u32 cha_rev_ms;		/* CRNR - CHA Rev No. Most significant half*/
> +#define CRNR_LS_RNGRN_SHIFT	16
> +#define CRNR_LS_RNGRN_MASK	(0xfull << CRNR_LS_RNGRN_SHIFT)
>  	u32 cha_rev_ls;		/* CRNR - CHA Rev No. Least significant half*/
>  #define CTPR_MS_QI_SHIFT	25
>  #define CTPR_MS_QI_MASK		(0x1ull << CTPR_MS_QI_SHIFT)
> @@ -311,6 +313,8 @@ struct caam_perfmon {
>  #define CCBVID_ERA_SHIFT	24
>  	u32 ccb_id;		/* CCBVID - CCB Version ID	*/
>  	u32 cha_id_ms;		/* CHAVID - CHA Version ID Most Significant*/
> +#define CHAVID_LS_RNGVID_SHIFT	16
> +#define CHAVID_LS_RNGVID_MASK	(0xfull << CHAVID_LS_RNGVID_SHIFT)
>  	u32 cha_id_ls;		/* CHAVID - CHA Version ID Least Significant*/
>  	u32 cha_num_ms;		/* CHANUM - CHA Number Most Significant	*/
>  	u32 cha_num_ls;		/* CHANUM - CHA Number Least Significant*/
> 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 <r.hieber@pengutronix.de>
> + *
> + * 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 <utkarsh.gupta@nxp.com>
> + * |    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 <utkarsh.gupta@nxp.com>
> + *
> + * 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
> + */
> +
> +#define pr_fmt(fmt) "rng_self_test: " fmt
> +
> +#include <dma.h>
> +#include <common.h>
> +#include "error.h"
> +#include "regs.h"
> +#include "jr.h"
> +
> +static inline u32 sec_in32(void *addr)
> +{
> +	return readl(addr);
> +}

This function is unused.

> +
> +#define DSC1SIZE 0x36
> +#define DSC2SIZE 0x3A
> +#define RESULT_SIZE 32
> +
> +static const u32 rng_dsc1[DSC1SIZE] = {
> +	0xb0800036, 0x04800010, 0x3c85a15b, 0x50a9d0b1,
> +	0x71a09fee, 0x2eecf20b, 0x02800020, 0xb267292e,
> +	0x85bf712d, 0xe85ff43a, 0xa716b7fb, 0xc40bb528,
> +	0x27b6f564, 0x8821cb5d, 0x9b5f6c26, 0x12a00020,
> +	0x0a20de17, 0x6529357e, 0x316277ab, 0x2846254e,
> +	0x34d23ba5, 0x6f5e9c32, 0x7abdc1bb, 0x0197a385,
> +	0x82500405, 0xa2000001, 0x10880004, 0x00000005,
> +	0x12820004, 0x00000020, 0x82500001, 0xa2000001,
> +	0x10880004, 0x40000045, 0x02800020, 0x8f389cc7,
> +	0xe7f7cbb0, 0x6bf2073d, 0xfc380b6d, 0xb22e9d1a,
> +	0xee64fcb7, 0xa2b48d49, 0xdf9bc3a4, 0x82500009,
> +	0xa2000001, 0x10880004, 0x00000005, 0x82500001,
> +	0x60340020, 0xFFFFFFFF, 0xa2000001, 0x10880004,
> +	0x00000005, 0x8250000d
> +};

Making this an array with undetermined size and using ARRAY_SIZE on the
array would make counting the elements unnecessary.

> +
> +static const u32 rng_result1[RESULT_SIZE] = {
> +	0x3a, 0xfe, 0x2c, 0x87, 0xcc, 0xb6, 0x44, 0x49,
> +	0x19, 0x16, 0x9a, 0x74, 0xa1, 0x31, 0x8b, 0xef,
> +	0xf4, 0x86, 0x0b, 0xb9, 0x5e, 0xee, 0xae, 0x91,
> +	0x92, 0xf4, 0xa9, 0x8f, 0xb0, 0x37, 0x18, 0xa4
> +};

same here.

> +
> +static const u32 rng_dsc2[DSC2SIZE] = {
> +	0xb080003a, 0x04800020, 0x27b73130, 0x30b4b10f,
> +	0x7c62b1ad, 0x77abe899, 0x67452301, 0xefcdab89,
> +	0x98badcfe, 0x10325476, 0x02800020, 0x63f757cf,
> +	0xb9165584, 0xc3c1b407, 0xcc4ce8ad, 0x1ffe8a58,
> +	0xfb4fa893, 0xbb5f4af0, 0x3fb946a1, 0x12a00020,
> +	0x56cbcaa5, 0xfff3adad, 0xe804dcbf, 0x9a900c71,
> +	0xa42017e3, 0x826948e2, 0xd0cfeb3e, 0xaf1a136a,
> +	0x82500405, 0xa2000001, 0x10880004, 0x00000005,
> +	0x12820004, 0x00000020, 0x82500001, 0xa2000001,
> +	0x10880004, 0x40000045, 0x02800020, 0x2e882f8a,
> +	0xe929943e, 0x8132c0a8, 0x12037f90, 0x809fbd66,
> +	0x8684ea04, 0x00cbafa7, 0x7b82d12a, 0x82500009,
> +	0xa2000001, 0x10880004, 0x00000005, 0x82500001,
> +	0x60340020, 0xFFFFFFFF, 0xa2000001, 0x10880004,
> +	0x00000005, 0x8250000d
> +};
> +
> +static const u32 rng_result2[RESULT_SIZE] = {
> +	0x76, 0x87, 0x66, 0x4e, 0xd8, 0x1d, 0x1f, 0x43,
> +	0x76, 0x50, 0x85, 0x5d, 0x1e, 0x1d, 0x9d, 0x0f,
> +	0x93, 0x75, 0x83, 0xff, 0x9a, 0x9b, 0x61, 0xa9,
> +	0xa5, 0xeb, 0xa3, 0x28, 0x2a, 0x15, 0xc1, 0x57
> +};

Why is this u32 when only the lower 8 bits are ever used? Changing this
to u8 would allow you to use memcmp to compare the result with the
expeceted one.

> +
> +/*
> + * construct_rng_self_test_jobdesc() - Implement destination address in RNG self test descriptors
> + * Returns zero on success, and negative on error.
> + */
> +static int construct_rng_self_test_jobdesc(u32 *desc, const u32 *rng_st_dsc, u8 *res_addr, int desc_size)
> +{
> +	int result_addr_idx = desc_size - 5;
> +	int i = 0;
> +
> +	if (!desc) {
> +		return -EINVAL;
> +	}

No need to check, this is already done in the caller.

> +
> +	for (i = 0; i < desc_size; i++) {
> +		desc[i] = rng_st_dsc[i];
> +	}
> +
> +	/* Replace destination address in the descriptor */
> +	desc[result_addr_idx] = (u32)res_addr;
> +
> +	pr_vdebug("RNG SELF TEST DESCRIPTOR:\n");
> +	for (i = 0; i < desc_size; i++) {
> +		pr_vdebug("0x%08X\n", desc[i]);
> +	}
> +
> +	return 0;
> +}
> +
> +/* rng_self_test_done() - callback for caam_jr_enqueue */
> +static void rng_self_test_done(struct device_d *dev, u32 *desc, u32 err, void *arg)
> +{
> +	int * job_err = (int *)arg;

unnecessary cast.

> +	BUG_ON(!job_err);

This is unnecessary because you will get a nive crash dump in the next
line when you dereference the NULL pointer.

> +	*job_err = err;
> +}
> +
> +/*
> + * 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".

> +
> +	result = dma_alloc(sizeof(uint32_t) * RESULT_SIZE);
> +	if (!result) {
> +		pr_err("Not enough memory for result buffer allocation\n");
> +		return -ENOMEM;
> +	}

Please no messages for usual allocation failures.

"result" is never freed.

> +
> +	desc = dma_alloc(sizeof(uint32_t) * desc_size);
> +	if (!desc) {
> +		pr_err("Not enough memory for descriptor allocation\n");
> +		return -ENOMEM;
> +	}
> +
> +	ret = construct_rng_self_test_jobdesc(desc, rng_st_dsc, result, desc_size);
> +	if (ret) {
> +		pr_err("Error in Job Descriptor Construction: %d %s\n",
> +				ret, strerror(ret));
> +		goto err;
> +	} else {

Not necessary to put the below code into an else {} path when you error
out above.

> +		dma_sync_single_for_device((unsigned long)desc,
> +				desc_size * sizeof(uint32_t), DMA_TO_DEVICE);
> +		dma_sync_single_for_device((unsigned long)result,
> +				RESULT_SIZE * sizeof(uint32_t), DMA_FROM_DEVICE);
> +
> +		/* wait for job completion */
> +		ret = caam_jr_enqueue(dev, desc, rng_self_test_done, &job_err);
> +		if (ret) {
> +			pr_err("Error while running RNG self-test descriptor: %d %s\n",
> +					ret, strerror(ret));
> +			goto err;
> +		}
> +		if (job_err) {
> +			pr_err("Job Error:\n");
> +			caam_jr_strstatus(dev, job_err);
> +			goto err;
> +		}
> +	}
> +
> +	dma_sync_single_for_cpu((unsigned long)result, RESULT_SIZE * sizeof(uint32_t),
> +			DMA_FROM_DEVICE);
> +
> +	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.

> +
> +	for (i = 0; i < RESULT_SIZE; i++) {
> +		if (result[i] != exp_result[i]) {
> +			pr_err("RNG self-test failed with unexpected result\n");
> +			ret = -ERANGE;
> +			goto err;
> +		}
> +	}

memcmp, as mentioned above.

> +	pr_notice("RNG software self-test passed\n");
> +	ret = 0;
> +
> +err:
> +	dma_free(desc);
> +	return ret;
> +}
> diff --git a/drivers/crypto/caam/rng_self_test.h b/drivers/crypto/caam/rng_self_test.h
> new file mode 100644
> index 0000000000..67356fe93b
> --- /dev/null
> +++ b/drivers/crypto/caam/rng_self_test.h
> @@ -0,0 +1,24 @@
> +/*
> + * CAAM RNG self test
> + *
> + * Copyright (C) 2018 Pengutronix, Roland Hieber <r.hieber@pengutronix.de>
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#ifndef RNG_SELF_TEST_H
> +#define RNG_SELF_TEST_H
> +
> +int rng_self_test(struct device_d *dev, const u8 caam_era, const u8 rngvid, const u8 rngrev);
> +
> +#endif /* RNG_SELF_TEST_H */
> diff --git a/drivers/hab/habv4.c b/drivers/hab/habv4.c
> index 28fd42ecd7..e64f34870e 100644
> --- a/drivers/hab/habv4.c
> +++ b/drivers/hab/habv4.c
> @@ -387,6 +387,27 @@ static void habv4_display_event(uint8_t *data, uint32_t len)
>  	habv4_display_event_record((struct hab_event_record *)data);
>  }
>  
> +/* Some chips with HAB >= 4.2.3 have an incorrect implementation of the RNG
> + * self-test in ROM code. In this case, an HAB event is generated, and a
> + * software self-test should be run. This variable is set to @c true by
> + * habv4_get_status() when this occurs. */
> +bool habv4_need_rng_software_self_test = false;
> +EXPORT_SYMBOL(habv4_need_rng_software_self_test);
> +
> +#define RNG_FAIL_EVENT_SIZE 36

ARRAY_SIZE is your friend.

> +static uint8_t habv4_known_rng_fail_events[][RNG_FAIL_EVENT_SIZE] = {
> +	{ 0xdb, 0x00, 0x24, 0x42,  0x69, 0x30, 0xe1, 0x1d,
> +	  0x00, 0x80, 0x00, 0x02,  0x40, 0x00, 0x36, 0x06,
> +	  0x55, 0x55, 0x00, 0x03,  0x00, 0x00, 0x00, 0x00,
> +	  0x00, 0x00, 0x00, 0x00,  0x00, 0x00, 0x00, 0x00,
> +	  0x00, 0x00, 0x00, 0x01 },
> +	{ 0xdb, 0x00, 0x24, 0x42,  0x69, 0x30, 0xe1, 0x1d,
> +	  0x00, 0x04, 0x00, 0x02,  0x40, 0x00, 0x36, 0x06,
> +	  0x55, 0x55, 0x00, 0x03,  0x00, 0x00, 0x00, 0x00,
> +	  0x00, 0x00, 0x00, 0x00,  0x00, 0x00, 0x00, 0x00,
> +	  0x00, 0x00, 0x00, 0x01 },
> +};
> +
>  static int habv4_get_status(const struct habv4_rvt *rvt)
>  {
>  	uint8_t data[256];
> @@ -413,10 +434,29 @@ static int habv4_get_status(const struct habv4_rvt *rvt)
>  
>  	len = sizeof(data);
>  	while (rvt->report_event(HAB_STATUS_WARNING, index, data, &len) == HAB_STATUS_SUCCESS) {
> -		pr_err("-------- HAB warning Event %d --------\n", index);
> -		pr_err("event data:\n");
>  
> -		habv4_display_event(data, len);
> +		/* suppress RNG self-test fail events if they can be handled in software */
> +		bool is_rng_fail_event = false;
> +		if (IS_ENABLED(CONFIG_CRYPTO_DEV_FSL_CAAM_RNG_SELF_TEST)) {
> +			int i;
> +			for (i = 0; i < ARRAY_SIZE(habv4_known_rng_fail_events); i++) {
> +				if (memcmp(data, habv4_known_rng_fail_events[i],
> +					   min(len, (uint32_t)RNG_FAIL_EVENT_SIZE)) == 0) {
> +					is_rng_fail_event = true;
> +					break;
> +				}
> +			}
> +		}

You could put that into a separate function to make this loop body and
also the test better readable.

> +
> +		if (is_rng_fail_event) {
> +			pr_warning("RNG self-test failure detected, will run software self-test\n");

pr_warning is a bit harsh when we atually expect it. pr_info maybe?

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] drivers: caam: add RNG software self-test
  2018-11-26  9:01 ` [PATCH 1/2] drivers: caam: add RNG software self-test Sascha Hauer
@ 2018-11-26 10:53   ` Roland Hieber
  2018-11-29 14:11     ` Roland Hieber
  2018-11-29 10:17   ` Roland Hieber
  1 sibling, 1 reply; 10+ messages in thread
From: Roland Hieber @ 2018-11-26 10:53 UTC (permalink / raw)
  To: Sascha Hauer; +Cc: barebox

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 <r.hieber@pengutronix.de>
> > + *
> > + * 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 <utkarsh.gupta@nxp.com>
> > + * |    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 <utkarsh.gupta@nxp.com>
> > + *
> > + * 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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] drivers: caam: add RNG software self-test
  2018-11-26  9:01 ` [PATCH 1/2] drivers: caam: add RNG software self-test Sascha Hauer
  2018-11-26 10:53   ` Roland Hieber
@ 2018-11-29 10:17   ` Roland Hieber
  1 sibling, 0 replies; 10+ messages in thread
From: Roland Hieber @ 2018-11-29 10:17 UTC (permalink / raw)
  To: Sascha Hauer; +Cc: barebox

On Mon, Nov 26, 2018 at 10:01:40AM +0100, Sascha Hauer wrote:
> On Sun, Nov 25, 2018 at 11:59:51PM +0100, Roland Hieber wrote:
[...]
> > +	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".

I asked back to Utkarsh Gupta at NXP, the original author of this
self-test. His reply was:

    (If condition) Legacy i.MX chipsets which are effected with this
    issue contain CAAM version below 8 and have RNG VID=4 and RNG
    REV <3.

    (else if condition) Current i.MX chipsets which are effected with
    this issue contain CAAM version greater than or equal to 8 and have
    RNG4.3 or above.

and he also agreed that the code could be more clear here.
If you do the math, the else-if clause only fires if

        !(caam_era < 8 && rngvid == 4 && rngrev < 3) && (rngvid != 3)
<=> [using De Morgan's laws]
        (caam_era >= 8 || rngvid != 4 || rngrev >= 3) && (rngvid != 3)

Note that (rngvid != 3) is always true because rngvid can only be 2 or
4. But he also mentions that the patch was meant to be run only on the
affected chips anyway, which rules out the cases rngvid == 2:

	(caam_era >= 8 || false || rngrev >= 3)
<=> 	(caam_era >= 8 || rngrev >= 3)

which is kind of what his formulation in natural language says.

I will use a clearer else-if condition instead in v2.

[...]
> > diff --git a/drivers/hab/habv4.c b/drivers/hab/habv4.c
> > index 28fd42ecd7..e64f34870e 100644
> > --- a/drivers/hab/habv4.c
> > +++ b/drivers/hab/habv4.c
> > @@ -387,6 +387,27 @@ static void habv4_display_event(uint8_t *data, uint32_t len)
> >  	habv4_display_event_record((struct hab_event_record *)data);
> >  }
> >  
> > +/* Some chips with HAB >= 4.2.3 have an incorrect implementation of the RNG
> > + * self-test in ROM code. In this case, an HAB event is generated, and a
> > + * software self-test should be run. This variable is set to @c true by
> > + * habv4_get_status() when this occurs. */
> > +bool habv4_need_rng_software_self_test = false;
> > +EXPORT_SYMBOL(habv4_need_rng_software_self_test);
> > +
> > +#define RNG_FAIL_EVENT_SIZE 36
> 
> ARRAY_SIZE is your friend.
> 
> > +static uint8_t habv4_known_rng_fail_events[][RNG_FAIL_EVENT_SIZE] = {
> > +	{ 0xdb, 0x00, 0x24, 0x42,  0x69, 0x30, 0xe1, 0x1d,
> > +	  0x00, 0x80, 0x00, 0x02,  0x40, 0x00, 0x36, 0x06,
> > +	  0x55, 0x55, 0x00, 0x03,  0x00, 0x00, 0x00, 0x00,
> > +	  0x00, 0x00, 0x00, 0x00,  0x00, 0x00, 0x00, 0x00,
> > +	  0x00, 0x00, 0x00, 0x01 },
> > +	{ 0xdb, 0x00, 0x24, 0x42,  0x69, 0x30, 0xe1, 0x1d,
> > +	  0x00, 0x04, 0x00, 0x02,  0x40, 0x00, 0x36, 0x06,
> > +	  0x55, 0x55, 0x00, 0x03,  0x00, 0x00, 0x00, 0x00,
> > +	  0x00, 0x00, 0x00, 0x00,  0x00, 0x00, 0x00, 0x00,
> > +	  0x00, 0x00, 0x00, 0x01 },
> > +};

No, ARRAY_SIZE won't work here:

drivers/hab/habv4.c:399:16: error: array type has incomplete element type 'uint8_t[] {aka unsigned char[]}'
 static uint8_t habv4_known_rng_fail_events[][] = {
                ^~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/hab/habv4.c:399:16: note: declaration of 'habv4_known_rng_fail_events' as multidimensional array must have bounds for all dimensions except the first
 
 - 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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] drivers: caam: add RNG software self-test
  2018-11-26 10:53   ` Roland Hieber
@ 2018-11-29 14:11     ` Roland Hieber
  2018-12-03  7:45       ` Sascha Hauer
  0 siblings, 1 reply; 10+ messages in thread
From: Roland Hieber @ 2018-11-29 14:11 UTC (permalink / raw)
  To: Sascha Hauer; +Cc: barebox

On Mon, Nov 26, 2018 at 11:53:47AM +0100, Roland Hieber wrote:
> > > +	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.

I have several solutions in mind: one would be to introduce a function
log_level() to get the current log level and decide on that whether to
call memory_display. However then memory dump won't pick up the pr_*
prefixes and use plain printf nevertheless. So my second thought is to
add a callback function to memory_display to pass pr_debug or else to it
which it can use to print stuff. However then we need to remodel
memory_display so it always prints full lines (or uses pr_cont which it
would need to know about, and you wanted to get rid of pr_cont anyway).
We could also use a third way and implement a custom printf escape, say
%md, that gets replaced with memory_display output when calling
pr_printk, and move the core formatting of memory_display into a helper.

I'm not sure which one is best.

-  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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] drivers: caam: add RNG software self-test
  2018-11-29 14:11     ` Roland Hieber
@ 2018-12-03  7:45       ` Sascha Hauer
  2018-12-03  9:41         ` Roland Hieber
  0 siblings, 1 reply; 10+ messages in thread
From: Sascha Hauer @ 2018-12-03  7:45 UTC (permalink / raw)
  To: Roland Hieber; +Cc: barebox

On Thu, Nov 29, 2018 at 03:11:00PM +0100, Roland Hieber wrote:
> On Mon, Nov 26, 2018 at 11:53:47AM +0100, Roland Hieber wrote:
> > > > +	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.
> 
> I have several solutions in mind: one would be to introduce a function
> log_level() to get the current log level and decide on that whether to
> call memory_display. However then memory dump won't pick up the pr_*
> prefixes and use plain printf nevertheless. So my second thought is to
> add a callback function to memory_display to pass pr_debug or else to it
> which it can use to print stuff. However then we need to remodel
> memory_display so it always prints full lines (or uses pr_cont which it
> would need to know about, and you wanted to get rid of pr_cont anyway).
> We could also use a third way and implement a custom printf escape, say
> %md, that gets replaced with memory_display output when calling
> pr_printk, and move the core formatting of memory_display into a helper.

Just posted another possibility. See if it fits your needs.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] drivers: caam: add RNG software self-test
  2018-12-03  7:45       ` Sascha Hauer
@ 2018-12-03  9:41         ` Roland Hieber
  0 siblings, 0 replies; 10+ messages in thread
From: Roland Hieber @ 2018-12-03  9:41 UTC (permalink / raw)
  To: Sascha Hauer; +Cc: barebox

On Mon, Dec 03, 2018 at 08:45:12AM +0100, Sascha Hauer wrote:
> On Thu, Nov 29, 2018 at 03:11:00PM +0100, Roland Hieber wrote:
> > On Mon, Nov 26, 2018 at 11:53:47AM +0100, Roland Hieber wrote:
> > > > > +	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.
> > 
> > I have several solutions in mind: one would be to introduce a function
> > log_level() to get the current log level and decide on that whether to
> > call memory_display. However then memory dump won't pick up the pr_*
> > prefixes and use plain printf nevertheless. So my second thought is to
> > add a callback function to memory_display to pass pr_debug or else to it
> > which it can use to print stuff. However then we need to remodel
> > memory_display so it always prints full lines (or uses pr_cont which it
> > would need to know about, and you wanted to get rid of pr_cont anyway).
> > We could also use a third way and implement a custom printf escape, say
> > %md, that gets replaced with memory_display output when calling
> > pr_printk, and move the core formatting of memory_display into a helper.
> 
> Just posted another possibility. See if it fits your needs.

I would have solved it a bit differently with the callback, but yours I
find much more elegant as the whole pr_memory_dump can be optimised out.
Thanks!

 - 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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] i.MX: HABv4: always print HAB status at boot time
  2018-11-25 22:59 ` [PATCH 2/2] i.MX: HABv4: always print HAB status at boot time Roland Hieber
  2018-11-26  7:56   ` Denis OSTERLAND
@ 2018-12-03 10:17   ` Denis OSTERLAND
  1 sibling, 0 replies; 10+ messages in thread
From: Denis OSTERLAND @ 2018-12-03 10:17 UTC (permalink / raw)
  To: barebox

Am Sonntag, den 25.11.2018, 23:59 +0100 schrieb Roland Hieber:
> Currently, board code needs to call habv4_get_status() explicitely, but
> there is no reason that it cannot be called automatically at startup
> when HABv4 is enabled. This way the call cannot be forgotten and we can
> make sure to report all potentially occuring HAB warnings and errors.
This fully fits my needs.
Tested on i.MX6.
> 
> Signed-off-by: Roland Hieber <r.hieber@pengutronix.de>
Acked-by: Denis Osterland <Denis Osterland@diehl.com>
> ---
>  drivers/hab/habv3.c |  4 ++++
>  drivers/hab/habv4.c | 41 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 45 insertions(+)
> 
> diff --git a/drivers/hab/habv3.c b/drivers/hab/habv3.c
> index 82ae245f8a..c190c78d40 100644
> --- a/drivers/hab/habv3.c
> +++ b/drivers/hab/habv3.c
> @@ -78,5 +78,9 @@ int imx_habv3_get_status(uint32_t status)
>  
>  int imx25_hab_get_status(void)
>  {
> +	if (!cpu_is_mx25()) {
> +		return 0;
> +	}
>  	return imx_habv3_get_status(readl(IOMEM(0x780018d4)));
>  }
> +postmmu_initcall(imx25_hab_get_status);
> diff --git a/drivers/hab/habv4.c b/drivers/hab/habv4.c
> index e64f34870e..09f598631b 100644
> --- a/drivers/hab/habv4.c
> +++ b/drivers/hab/habv4.c
> @@ -20,6 +20,7 @@
>  
>  #include <common.h>
>  #include <hab.h>
> +#include <init.h>
>  #include <types.h>
>  
>  #include <mach/generic.h>
> @@ -500,9 +501,49 @@ int imx6_hab_get_status(void)
>  	return -EINVAL;
>  }
>  
> +static int init_imx6_hab_get_status(void)
> +{
> +	int ret = 0;
> +
> +	if (!cpu_is_mx6())
> +		/* can happen in multi-image builds and is not an error */
> +		return 0;
> +
> +	ret = imx6_hab_get_status();
> +
> +	/* nobody will check the return value if there were HAB errors, but the
> +	 * initcall will fail spectaculously with a strange error message. */
> +	if (ret == -EPERM)
> +		return 0;
> +	return ret;
> +}
> +/* need to run before MMU setup because i.MX6 ROM code is mapped near 0x0,
> + * which will no longer be accessible when the MMU sets the zero page to
> + * faulting. */
> +postconsole_initcall(init_imx6_hab_get_status);
> +
>  int imx28_hab_get_status(void)
>  {
>  	const struct habv4_rvt *rvt = (void *)HABV4_RVT_IMX28;
>  
>  	return habv4_get_status(rvt);
>  }
> +
> +static int init_imx28_hab_get_status(void)
> +{
> +	int ret = 0;
> +
> +	if (!cpu_is_mx28())
> +		/* can happen in multi-image builds and is not an error */
> +		return 0;
> +
> +	ret = imx28_hab_get_status();
> +
> +	/* nobody will check the return value if there were HAB errors, but the
> +	 * initcall will fail spectaculously with a strange error message. */
> +	if (ret == -EPERM)
> +		return 0;
> +	return ret;
> +}
> +/* i.MX28 ROM code can be run after MMU setup to make use of caching */
> +postmmu_initcall(init_imx28_hab_get_status);

Diehl Connectivity Solutions GmbH
Geschäftsführung: Horst Leonberger
Sitz der Gesellschaft: Nürnberg - Registergericht: Amtsgericht
Nürnberg: HRB 32315
___________________________________________________________________________________________________

Der Inhalt der vorstehenden E-Mail ist nicht rechtlich bindend. Diese E-Mail enthaelt vertrauliche und/oder rechtlich geschuetzte Informationen.
Informieren Sie uns bitte, wenn Sie diese E-Mail faelschlicherweise erhalten haben. Bitte loeschen Sie in diesem Fall die Nachricht.
Jede unerlaubte Form der Reproduktion, Bekanntgabe, Aenderung, Verteilung und/oder Publikation dieser E-Mail ist strengstens untersagt.
The contents of the above mentioned e-mail is not legally binding. This e-mail contains confidential and/or legally protected information. Please inform us if you have received this e-mail by
mistake and delete it in such a case. Each unauthorized reproduction, disclosure, alteration, distribution and/or publication of this e-mail is strictly prohibited. 
_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2018-12-03 10:18 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-25 22:59 [PATCH 1/2] drivers: caam: add RNG software self-test Roland Hieber
2018-11-25 22:59 ` [PATCH 2/2] i.MX: HABv4: always print HAB status at boot time Roland Hieber
2018-11-26  7:56   ` Denis OSTERLAND
2018-12-03 10:17   ` Denis OSTERLAND
2018-11-26  9:01 ` [PATCH 1/2] drivers: caam: add RNG software self-test Sascha Hauer
2018-11-26 10:53   ` Roland Hieber
2018-11-29 14:11     ` Roland Hieber
2018-12-03  7:45       ` Sascha Hauer
2018-12-03  9:41         ` Roland Hieber
2018-11-29 10:17   ` Roland Hieber

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox