core:driver implementation NXP CAAM Driver - RNG/HASH by cneveux · Pull Request #3228 · OP-TEE/optee_os

cneveux · 2019-08-29T08:35:22Z

Add the NXP CAAM drivers:

Random generator (instantiation and random generation)
Hash

Add a generic cryptographic driver interface connecting
TEE Crypto generic APIs to HW driver interface

jenswi-linaro

Comments for the commit "core: driver: generic resources for crypto device driver"

With comment addressed:
Reviewed-by: Jens Wiklander <[email protected]>

jenswi-linaro · 2019-08-29T12:35:40Z

@@ -0,0 +1,47 @@
+// SPDX-License-Identifier: BSD-2-Clause


A separate directory for this file seems a bit too much. How about putting this file directly below crypto_api?

May be but I'll classified all algorithms in a specific directory. For 2 raisons:

it is easier to find files associated to the algorithms.

it facilitate the build by including of not the directory.

At the end we will have following directories (at least):

acipher (ECC/DH/RSA/DSA)

cipher (AES/DES/DES3)

hash (HASH)

mac (HMAC/Cipher MAC/CMAC)

etienne-lms

some minor comments for "core: driver: generic resources for crypto device driver"

etienne-lms · 2019-08-29T10:13:02Z

+
+/*
+ * Table of Cryptographic modules
+ */


minor: remove the comment and init value { 0 } for crypt_algo[], implicit.

etienne-lms · 2019-08-29T10:15:51Z

+
+	CRYPTO_TRACE("hash alloc_ctx algo 0x%" PRIX32, algo);
+
+	/* Check the parameters */


remove this comment

etienne-lms · 2019-08-29T10:15:57Z

+	algo_id = TEE_ALG_GET_MAIN_ALG(algo);
+
+	if (algo_op == TEE_OPERATION_DIGEST &&
+	    (algo_id >= TEE_MAIN_ALGO_MD5 && algo_id <= TEE_MAIN_ALGO_SHA512))


that's strange. in drvcrypt_hash_alloc_ctx() we do expect to handle a hashing operations, hence call the expected allocation handler. Why test here the algo/op?

etienne-lms · 2019-08-29T10:20:25Z

 TEE_Result crypto_aes_gcm_alloc_ctx(struct crypto_authenc_ctx **ctx);
+
+#ifdef CFG_CRYPTO_DRV_HASH
+/* Cryptographic HASH driver context allocation */


remove comment

jenswi-linaro · 2019-08-29T12:45:11Z

+$(call force, CFG_JR_BLOCK_SIZE,0x1000)
+
+$(call force, CFG_JR_INDEX,0)  # Default JR index used
+$(call force, CFG_JR_IRQ,137)  # Default JR IT Number (105 + 32) = 137


Since it's sometimes IRQ and sometimes FIQ in secure world I'd prefer calling it interrupt when being unspecific. When being more specific are secure interrupts called native interrupts and non-secure interrupts called foreign interrupts.

What's interesting from OP-TEE point of view is if OP-TEE itself will handle the interrupt or if it's to be forwarded.

ok, as you want, renamed to CFG_JR_INT.
Point of view functional and security, we have to ensure that the Job Ring used into Secure side is not caught by Non-secure side. For that, as all secure interrupts on ARM are Secure FIQ, we configure the Job Ring interrupt to be secured FIQ. That way the non-secure can not enable/disable it and neither intercept it.
In case this interrupt is handled in the Non-secure, the crypto operation can be disturbed or hacked.

jenswi-linaro · 2019-08-29T12:47:34Z

+{
+	TEE_Result retresult = TEE_ERROR_GENERIC;
+	enum CAAM_Status retstatus = CAAM_FAILURE;
+	struct caam_jrcfg jrcfg = { 0 };


Structs are normally zero initialized as { }.

It doesn't cost anything to write it { 0 } and it's more friendly to read ;)

jenswi-linaro · 2019-08-29T12:50:57Z

+	itr_disable(handler->it);
+
+	/* Send a signal to exit WFE loop */
+	sev();


Who's waiting?

the enqueue loop do_jr_enqueue.
The justification to switch the CPU in WFE is to reduce power consumption and to not overload HW by pulling register

jenswi-linaro · 2019-08-29T12:51:49Z

+				      uint32_t *job_id)
+{
+	enum CAAM_Status retstatus = CAAM_BUSY;
+


Empty line not needed.

jenswi-linaro · 2019-08-29T12:58:14Z

+	 * also touching it
+	 */
+	cpu_spin_lock(&jr_privdata->callers_lock);
+	for ((idx_jr = 0), (found = false);


All these redundant () are a bit annoying, how about writing it as (relying on found already being false):

for (idx_jr = 0; idx_jr < jr_privdata->nb_jobs && !found; idx_jr++) {

jenswi-linaro · 2019-08-29T19:39:53Z

+ * @job_ids     Job Ids Mask
+ * @timeout_ms  Timeout in millisecond
+ */
+enum CAAM_Status caam_jr_dequeue(uint32_t job_ids, uint32_t timeout_ms);


Could some other type than a fixed width type be used for timeout_ms, the 32-bit width seems rather arbitrary.

Using a 8, 16 or 32 bits value will not change anything. At compilation the ARM register R1 (32 bits) will be used. Then as it's internal to the CAAM driver, the person calling this function is supposed to know what to do.

Yes, but that's not the point, by the way there's 64-bit platforms to consider too.

With fixed width types you signal that you care about the specific properties of that type, how it overflows etc. At certain places it will stand out a bit.

You can keep it as it is if you think it's important.

ok so is unsigned int timeout_ms better?

jenswi-linaro · 2019-08-29T19:40:50Z

+ *
+ * @nbentries  Number of descriptor entries
+ */
+uint32_t *caam_alloc_desc(uint8_t nbentries);


Why the uint8_t type?

The number of entry in a descriptor is limited to 64. Using 8 bits will limit the allocation buffer to 256 * 4 bytes even if it's too much.

So if this function is called as caam_alloc_desc(258), it's OK to get a smaller allocation than expected? This doesn't seem right to me. Using size_t instead would be more robust.

I would agree with you if the function was called dynamically and outside the CAAM driver which is not the case.
Here if the descriptor allocated is too small, the cryptographic operation is failing because the CAAM HW is not getting the correct descriptor and in plus there will be a buffer overflow because the driver filling the descriptor will write somewhere not allocated to it.
If such error raised after release it means that the developer didn't validate the work done which is a big fault.
Then size_t is really too big data variable in this particular case (size_t is generally a long int), don't you think?
If we want to be stronger, we have to add in any internal function parameters check which will be a mess.

jenswi-linaro · 2019-08-29T19:42:21Z

+ * Recalculate the pointer p address to start on a align modulus
+ * The p address is round up to the next offset (align modulus)
+ */
+static inline vaddr_t align_ptr(vaddr_t p, uint32_t align)


How about using ROUNDUP() instead?

jenswi-linaro · 2019-08-29T19:44:35Z

+int caam_realloc_align(void *orig, struct caambuf *dst, size_t size)
+{
+	enum teecore_memtypes mtype = MEM_AREA_MAXTYPE;
+	uint32_t isCached = false;


Please drop the camel case.

jenswi-linaro · 2019-08-29T19:53:01Z

+ */
+void caam_cache_op_sgt(enum utee_cache_operation op, struct caamsgtbuf *insgt)
+{
+	uint8_t idx;


How about using unsigned int instead of a fixed width type?
Please initialize

jenswi-linaro

More comments for the commit "core: driver: implementation NXP CAAM Driver"

jenswi-linaro · 2019-08-30T08:21:51Z

+ */
+uint32_t *caam_alloc_desc(uint8_t nbentries)
+{
+	return mem_alloc(DESC_SZBYTES(nbentries),


Checking for overflow would be nice. It can't happen with a uint8_t, but with a size_t instead it would be possible to catch it.

As explain I don't want to add the overflow check for this particular function which is intern to the NXP CAAM driver. Overflow must not raised because driver has to be validated before released and this function is not called with dynamic size.

jenswi-linaro · 2019-08-30T08:24:15Z

+
+	if (type & MEM_TYPE_ALIGN) {
+		cacheline_size = read_cacheline_size();
+		alloc_size += ROUNDUP(alloc_size, cacheline_size);


alloc_size = ...
I think we should check for overflow also.

jenswi-linaro · 2019-08-30T08:26:45Z

+	 * |  Buffer    |
+	 * --------------
+	 */
+	alloc_size = size + MEM_INFO_SIZE;


Check for overflow, with ADD_OVERFLOW() it would become:

if (ADD_OVERFLOW(size, MEM_INFO_SIZE, &alloc_res)) return NULL;

jenswi-linaro · 2019-08-30T08:27:29Z

+	if (type & MEM_TYPE_ALIGN) {
+		/*
+		 * In case of the buffer must be aligned on a cache
+		 * ligne, ensure that the size is aligned on a cache line


s/ligne/line

jenswi-linaro · 2019-08-30T08:29:11Z

+		 * otherwise the total buffer size will be too small
+		 */
+		cacheline_size = read_cacheline_size();
+		alloc_size +=


I'd prefer avoiding the ternary operator here and use a classic if statement instead.
Need to check for overflow.

jenswi-linaro · 2019-08-30T08:31:29Z

+		cacheline_size = read_cacheline_size();
+		alloc_size +=
+			(alloc_size == cacheline_size) ? cacheline_size : 0;
+		alloc_size += ROUNDUP(alloc_size, cacheline_size);


alloc_size = ...
Need to check for overflow.

jenswi-linaro · 2019-08-30T08:50:33Z

+ * 1    if reallocation of the buffer
+ * (-1) if allocation error
+ */
+int caam_realloc_align(void *orig, struct caambuf *dst, size_t size)


The name of this function is a bit unfortunate, the "realloc" tricked me into thinking that it behaved somewhat like realloc(). How about "caam_init_or_alloc_align" or something like that instead.

or rename to caam_set_or_alloc_align_buf

jenswi-linaro · 2019-08-30T09:41:30Z

+	uint8_t *data;   /* Data buffer */
+	paddr_t paddr;   /* Physical address of the buffer */
+	size_t length;   /* Number of bytes in the data buffer */
+	uint8_t nocache; /* =1 if buffer is not cacheable */


Negative logic usually make it harder to understand the code. I'd prefer bool cached. Keep it as it is if you think it's important.

Yes but I'll keep it as is for the simple reason that be default with the variable initialization at 0's the default nocache is as expected without adding a new code line to set the value.

jenswi-linaro · 2019-09-02T08:47:42Z

There's a bunch of comment under "hidden conversions" when I'm viewing this PR which seem to have been missed as they are not address or answered.

etienne-lms

mainly minor comments

etienne-lms · 2019-09-02T07:24:25Z

+TEE_Result drvcrypt_register(enum drvcrypt_algo_id algo_id, void *ops)
+{
+	if (!crypt_algo[algo_id]) {
+		CRYPTO_TRACE("Registering module id %d with 0x%" PRIxPTR,


minor: replace "... %0x" PRIxPTR, ..., (uinptr_t)ops)
with "... %p", ..., ops)

Same at line 28, 41.

etienne-lms · 2019-09-02T07:28:28Z

+/*
+ * Allocates the Software Hashing Context function of the algorithm
+ * and if the HW handles it. Else return on error and let the
+ * global cryptographic core module to call SW library enabled.


could simplify: "Allocate context for hashing operation if a drvcrypt hash driver if registered."

etienne-lms · 2019-09-02T07:32:16Z

+ * Modify the Cryptographic algorithm in the table of module
+ *
+ * @algo_id  ID of the Cryptographic module
+ * @ops      Reference to the cryptographic module


prefer use the same description as for drvcrypt_register().

etienne-lms · 2019-09-02T07:32:37Z

+TEE_Result drvcrypt_register(enum drvcrypt_algo_id algo_id, void *ops);
+
+/*
+ * Modify the Cryptographic algorithm in the table of module


s/module/modules/

etienne-lms · 2019-09-02T07:34:50Z

 CFG_MMAP_REGIONS ?= 24
+
+# Almost all platforms include CAAM HW Modules, except the
+# one forced to be disabled


s/one/ones/

etienne-lms · 2019-09-02T08:23:04Z

+int caam_set_or_alloc_align_buf(void *orig, struct caambuf *dst, size_t size)
+{
+	enum teecore_memtypes mtype = MEM_AREA_MAXTYPE;
+	uint32_t is_cached = false;


etienne-lms · 2019-09-02T08:24:37Z

+enum CAAM_Status caam_cpy_block_src(struct caamblock *block,
+				    struct caambuf *src, size_t offset)
+{
+	enum CAAM_Status ret;


default init value missing

etienne-lms · 2019-09-02T08:26:35Z

+		  block->max);
+
+	cpy_size = block->max - block->filled;
+	cpy_size = MIN(cpy_size, (src->length - offset));


minor: remove inner parentheses.

etienne-lms · 2019-09-02T08:27:14Z

+	unsigned int idx = 0;
+
+	cache_operation(TEE_CACHECLEAN, (void *)insgt->sgt,
+			(insgt->number * sizeof(struct caamsgt)));


minor: remove parentheses.

etienne-lms · 2019-09-02T08:29:47Z

+TEE_Result job_status_to_tee_result(uint32_t status)
+{
+	/*
+	 * Add all status code that can be translate


s/translate/translated/

etienne-lms · 2019-09-03T09:30:32Z

+
+#define MEM_TYPE_ZEROED	BIT(0)
+#define MEM_TYPE_NORMAL	BIT(1)
+#define MEM_TYPE_ALIGN	BIT(2)


MEM_TYPE_NORMAL seems useless, or aybe lacks a description.
MEM_TYPE_ALIGN should be named MEM_TYPE_CACHE_ALIGN. (or maybe add an inline description comment stating it's about cache line alignment)

etienne-lms · 2019-09-03T09:52:20Z

+ * Crypto Library Algorithm enumeration
+ */
+enum drvcrypt_algo_id {
+	CRYPTO_HASH = 0,     /* HASH driver */


minor: s/HASH/Hash/

etienne-lms · 2019-09-03T09:56:07Z

+{
+	struct crypto_hash *hash = NULL;
+	struct hashdata *hashdata = NULL;
+	uint8_t algo = (hash_id - TEE_MAIN_ALGO_MD5);


minor: as Jens pointed, using uint8_t adds nothing but maybe few instructions to handle the single-byte format of the data. Suggest to replace with unsigned int.
Can remove parentheses.

etienne-lms · 2019-09-03T10:00:02Z

+	if (do_rng_read(&data, 1))
+		return data;
+
+	return 0;


hazardous to return a deterministic value here. Suggest panic();

etienne-lms · 2019-09-03T11:48:09Z

+ * Allocate internal driver buffer aligned with a cache line
+ *
+ * @size  size in bytes of the memory to allocate
+ * @buf   [out] buffer allocated


etienne-lms · 2019-09-03T11:48:21Z

+ * Allocate internal driver buffer and initialize it with 0s
+ *
+ * @size  size in bytes of the memory to allocate
+ * @buf   [out] buffer allocated


etienne-lms · 2019-09-03T11:51:36Z

+static inline void dump_desc(uint32_t *desc)
+{
+	size_t idx;
+	size_t len;


default init

etienne-lms · 2019-09-03T11:53:43Z

+#define CLASS_DECO		0x3
+
+#define	CMD_SGT			BIT32(24)
+#define	CMD_IMM			BIT32(23)


prefer a space instead of 1st tab

etienne-lms · 2019-09-03T11:57:15Z

+}
+
+/*
+ * Registration of the HASH Driver


s/HASH Driver/hash driver/
Few other occurrences of HASH to lowercase in inline comments.

Add a generic cryptographic driver interface connecting TEE Crypto generic APIs to HW driver interface The Generic Crypto Driver interface in the core/driver/crypto/crypto_api is implemented to be able to use a HW driver. Signed-off-by: Cedric Neveux <[email protected]> Reviewed-by: Jens Wiklander <[email protected]>

jenswi-linaro · 2019-09-05T08:23:34Z

All this force pushing and rebasing makes it harder than necessary to follow the changes you make. Unless rebasing and squashing in changes are needed to get a better view it should normally be avoided during the review. Instead you can push incremental changes which later are squashed in when the review is done. I'm sure you've noticed that pattern in other reviews we've done.

I don't think I can help improving the commit "core: driver: implementation NXP CAAM Driver" much more now so please apply:
Acked-by: Jens Wiklander <[email protected]>
for that commit.

Add the NXP CAAM drivers: - Random generator (instantiation and random generation) - Hash Signed-off-by: Cedric Neveux <[email protected]> Acked-by: Jens Wiklander <[email protected]>

Emantor · 2019-09-06T07:38:13Z

+#include <libfdt.h>
+
+static const char *dt_ctrl_match_table = {
+	"fsl,sec-v4.0-ctrl",


This node is not found in any upstream device tree, breaking boot on any board with a mainline kernel and dt:

I/TC: OP-TEE version: 3.6.0-141-ga41d3b18 (gcc version 8.2.1 20181130 (OSELAS.Toolchain-2018.12.0 8-20181130)) #1 Fri Sep 6 07:31:41 UTC 2019 arm D/TC:0 0 check_ta_store:638 TA store: "early TA" D/TC:0 0 check_ta_store:638 TA store: "Secure Storage TA" D/TC:0 0 check_ta_store:638 TA store: "REE [buffered]" D/TC:0 0 early_ta_init:255 Early TA bc50d971-d4c9-42c4-82cb-343fb7f37896 size 632165 (compressed, uncompressed 1037980) D/TC:0 0 mobj_mapped_shm_init:447 Shared memory address range: 4e400000, 50400000 E/TC:0 0 plat_rng_init:354 Warning: seeding RNG with zeroes D/TC:0 0 imx_wdog_base:142 path: /soc/aips-bus@2000000/wdog@20bc000 E/TC:0 0 caam_hal_cfg_get_conf:79 Caam Node not found err = 0xFFFFFFFF E/TC:0 0 crypto_driver_init:79 CAAM Driver initialization (0xffff000a) E/TC:0 0 Panic at core/drivers/crypto/caam/caam_ctrl.c:80 <crypto_driver_init> E/TC:0 0 Call stack: E/TC:0 0 0x4e0056fd print_kernel_stack at /ptx/work/WORK_BEEMI/rcz/backup/git/trees/optee/imx6q/core/arch/arm/kernel/unwind_arm32.c:450

@Emantor,

You can find the linux version in Code Aurora

My expectation was that an upstream OP-TEE works with an upstream kernel and does not require device-tree properties not documented anywhere.

ok, I have to think how to deal with. I'll submit a new commit (thru a new PR) for that.

IMO this is not the way forward here. Working platforms should still work after the PR is merged, so the necessary change should be included with the commit which introduces the dependency on the device tree node.

I tend to agree with @Emantor on this, we should be very careful with DT bindings and specs, otherwise things will become unmanageable (I mean difficult to use and possibly inconsistent).

ff you could get the DT stuff and linux drivers to be accepted into upstream Linux at about the same time we accept the secure drivers here, that would be great.

Reminds me of how we dealt with the secure console binding (/secure-chosen/stdout-path). I first created a PR here, then proposed the binding to Linux. Rob H. made some suggestions and accepted the binding as long as I showed it was about to be used by OP-TEE and we were done.

etienne-lms · 2019-09-06T07:47:45Z

@cneveux, I have gone again through the whole P-R, ending around 40 minor comments, hence at least a bunch of cleanup stuff, that actually could be postponed.

few remaining CamelCase (CAAM_Status, waitJobIds, fullSize)
inline function description both in C and header files (I didn't check whether they are consistent)
few inline comments not accurate.
some parentheses removal, instruction compaction, local var init, ...
also few rename suggestion (callbk=>callback, drvcrypt_getmod()=>drvcrypt_get_ops()).

All in one, nothing harmful.

I won't push straight these additional comments right now, not to put noise in this review. I understood you have other CAAM features waiting for this initial P-R to land.
@jenswi-linaro, @jforissier, would you prefer we step ahead in this drvcrypto and merge this P-R (leaving cleanup to later P-Rs)?

rework the hal_cfg.c and hal_cfg_dt.c file to ensure that either DTB configuration is used if defined or Hard coded configuration is used Signed-off-by: Cedric Neveux <[email protected]>

etienne-lms

Specifically for commit "[rework] CAAM Configuration with/without DTB file"

etienne-lms · 2019-09-05T06:43:45Z

-		return TEE_ERROR_NOT_IMPLEMENTED;
+	/*
+	 * If a Cryptographic driver is supported, call first
+	 * the driver's allocation function. If return no success


You should replace "no success" with something else.
I would propose to shorten the description: Use default crypto implement if no matching drvcrypt device.

I guess such comment illustrates why inline comment should be used only when implementation is not self sufficient.

etienne-lms · 2019-09-05T06:46:14Z

+ *
+ * @algo_id  ID of the Cryptographic module
+ */
+void *drvcrypt_getmod(enum drvcrypt_algo_id algo_id)


suggest to rename drvcrypt_get_ops(). As per description referring to a operation ref.
(if agree, better do it now than once merged)

etienne-lms · 2019-09-05T06:54:35Z

+typedef TEE_Result (*hw_hash_allocate)(struct crypto_hash_ctx **ctx,
+				       uint8_t hash_id);
+
+static inline TEE_Result drvcrypt_register_hash(hw_hash_allocate allocate)


Add description, as this a formal API of drvcrypt:

/* * Register a hash processing driver in the crypto API * * @allocate - Callback for driver context allocation in the crypto layer */

etienne-lms · 2019-09-05T07:08:12Z

+ * Crypto Library Hash driver allocation function prototype
+ */
+typedef TEE_Result (*hw_hash_allocate)(struct crypto_hash_ctx **ctx,
+				       uint8_t hash_id);


I though hash_id here is not very explicit. Actually, you should move to there description from next commit in the series 947fbf3#diff-ceb9dd6899902415c951f995eddc5ce1R570.

I think the API should receive the full algo, not only the main algorithm. There is no strong reason the filter info. I guess other drvcrypt drivers will also get the full 32bit Algorithm Identifier.

if you want

etienne-lms · 2019-09-05T15:37:38Z

+
+	reg = io_caam_read32(baseaddr + RNG_STA);
+
+	return (((reg & RNG_STA_SKVN) == RNG_STA_SKVN) ? true : false);


return io_caam_read32(baseaddr + RNG_STA) & RNG_STA_SKVN: straight

etienne-lms · 2019-09-06T13:49:20Z

+caam_hal_cfg_get_jobring_dt(void *fdt __unused,
+			    struct caam_jrcfg *jrcfg __unused)
+{
+}


suggest memset(jrcfg, 0, sizeof(*jrcfg));

only offset and it_num will be set to 0. one of the field (base) is already set.

etienne-lms · 2019-09-06T13:51:02Z

+void caam_hal_cfg_get_jobring_dt(void *fdt, struct caam_jrcfg *jrcfg);
+
+/*
+ * Disable the Job Ring used for the Secure environment into the DTB


maybe Disable the DT node related to the Job Ring used by secure world

Note this description comment is both in the .c and .h files, which is not convenient for maintenance.

etienne-lms · 2019-09-06T13:51:53Z

 /*
- * Returns the Job Ring configuration to be used by the TEE
+ * Finds the CAAM Controller definition in the DTB.
+ * If found, ensures it reserved for the Secure OS and disabled for the RichOS.


s/it/it is/
s/reserved/enabled/

function will be removed

etienne-lms · 2019-09-06T13:52:47Z

+	/*
+	 * Node is defined, check if the CAAM Controller is defined. If not
+	 * use HW hard coded value
+	 */


I think this comment does not belong to this function.

function will be removed

etienne-lms · 2019-09-06T13:54:31Z


 /*
- * Returns the Job Ring configuration to be used by the TEE
+ * Finds the CAAM Controller definition in the DTB.


prefer: Return the CAAM controller node found in the DTB.

function will be removed

etienne-lms · 2019-09-06T15:25:17Z

ooh! sorry. Pushing comments for "Specifically for commit "[rework] CAAM Configuration with/without DTB file" also pushed all my pending comments. Well... I guess it's a bit late now. Sorry for the noise.

etienne-lms · 2019-09-07T16:15:29Z

@cneveux, I suggest you push a P-R for the 1st commit: generic drvcrypt only. Should land quick.
Then let's add hashes above and random. and others, prefer one by one, generic and HW specific preferably in specific commits.

etienne-lms · 2019-09-09T08:54:01Z

+	({                                                                     \
+		__typeof__(size) _size = (size);                               \
+		__typeof__(size) _sizeup = 0;                                  \
+		_sizeup = ROUNDUP(_size, align);                               \


Please add a empty escaped line here:

__typeof__(size) _size = (size); \ __typeof__(size) _sizeup = 0; \ + \ _sizeup = ROUNDUP(_size, align); \

etienne-lms · 2019-09-09T08:54:33Z

+
+/*
+ * Allocate normal memory.
+ * Depending on the MEM_TYPE_DEFAULT value initialized it to 0's.


Well, that make sure not to change the API there caller really expects 0 init buffer.

etienne-lms · 2019-09-09T08:57:18Z

+
+	if ((size_topost) && (data)) {
+		struct caambuf indata = { .data = (uint8_t *)data,
+					  .length = inlen };


ok, right. sorry.

etienne-lms · 2019-09-09T09:00:07Z

+	({                                                                     \
+		__typeof__(buf) _buf = (buf);                                  \
+		__typeof__(len) _len = (len);                                  \
+		DRV_TRACE("%s @0x%" PRIxPTR ": %zu", title, (uintptr_t)_buf,   \


coding style: add an empty escaped line between variable definition and instructions:

({ \ __typeof__(buf) _buf = (buf); \ __typeof__(len) _len = (len); \ + \ DRV_TRACE("%s @0x%" PRIxPTR ": %zu", title, (uintptr_t)_buf, \

cneveux · 2019-09-10T11:02:28Z

Replace with PR #3259

jenswi-linaro reviewed Aug 29, 2019

View reviewed changes

etienne-lms reviewed Aug 29, 2019

View reviewed changes

jenswi-linaro reviewed Aug 29, 2019

View reviewed changes

cneveux force-pushed the feature/patch_caam_hash_2 branch from c0791a7 to ecd1e96 Compare August 30, 2019 07:39

jenswi-linaro reviewed Aug 30, 2019

View reviewed changes

cneveux force-pushed the feature/patch_caam_hash_2 branch 2 times, most recently from 4ee903e to 64ad3db Compare September 2, 2019 08:40

etienne-lms reviewed Sep 2, 2019

View reviewed changes

cneveux force-pushed the feature/patch_caam_hash_2 branch 2 times, most recently from f5fca66 to c4a2a1c Compare September 3, 2019 07:32

etienne-lms reviewed Sep 3, 2019

View reviewed changes

Comment thread core/drivers/crypto/crypto_api/include/drvcrypt.h

etienne-lms reviewed Sep 3, 2019

View reviewed changes

cneveux force-pushed the feature/patch_caam_hash_2 branch from c4a2a1c to 63a3a41 Compare September 3, 2019 13:53

cneveux force-pushed the feature/patch_caam_hash_2 branch 2 times, most recently from c73e55a to 947fbf3 Compare September 5, 2019 06:23

cneveux force-pushed the feature/patch_caam_hash_2 branch from 947fbf3 to a41d3b1 Compare September 5, 2019 11:16

core: driver: implementation NXP CAAM Driver

a41d3b1

Add the NXP CAAM drivers: - Random generator (instantiation and random generation) - Hash Signed-off-by: Cedric Neveux <[email protected]> Acked-by: Jens Wiklander <[email protected]>

jenswi-linaro mentioned this pull request Sep 5, 2019

Adding hardware accelerated crypto #3251

Closed

etienne-lms mentioned this pull request Sep 6, 2019

Minimal HUK implementation without full CAAM driver #3160

Closed

Emantor reviewed Sep 6, 2019

View reviewed changes

[rework] CAAM Configuration with/without DTB file

7a878e6

rework the hal_cfg.c and hal_cfg_dt.c file to ensure that either DTB configuration is used if defined or Hard coded configuration is used Signed-off-by: Cedric Neveux <[email protected]>

etienne-lms reviewed Sep 6, 2019

View reviewed changes

etienne-lms reviewed Sep 9, 2019

View reviewed changes

cneveux closed this Sep 10, 2019

jockebech mentioned this pull request Oct 7, 2019

Update CHANGELOG for 3.7.0 #3309

Merged

cneveux deleted the feature/patch_caam_hash_2 branch October 13, 2020 12:55


		CRYPTO_TRACE("hash alloc_ctx algo 0x%" PRIX32, algo);

		/* Check the parameters */


		reg = io_caam_read32(baseaddr + RNG_STA);

		return (((reg & RNG_STA_SKVN) == RNG_STA_SKVN) ? true : false);

Conversation

cneveux commented Aug 29, 2019

Uh oh!

jenswi-linaro left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

etienne-lms left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jenswi-linaro left a comment