Hi Jarkko,

thanks for the review!

On 12.09.2023, at 19:32, Jarkko Sakkinen [off-list ref] wrote:

On Tue Sep 12, 2023 at 2:11 PM EEST, David Gstir wrote:

[...]

quoted

- /* Payload contains the key. */
- desc->control0 |= MXS_DCP_CONTROL0_PAYLOAD_KEY;
+ if (key_referenced) {
+ /* Set OTP key bit to select the key via KEY_SELECT. */
+ desc->control0 |= MXS_DCP_CONTROL0_OTP_KEY;
+ } else {
+ /* Payload contains the key. */
+ desc->control0 |= MXS_DCP_CONTROL0_PAYLOAD_KEY;
+ }

Remove curly braces (coding style).

Will fix that and all similar cases for v3.

quoted

+static int mxs_dcp_aes_setrefkey(struct crypto_skcipher *tfm, const u8 *key,
+ unsigned int len)
+{
+ struct dcp_async_ctx *actx = crypto_skcipher_ctx(tfm);
+
+ if (len != DCP_PAES_KEYSIZE)
+ return -EINVAL;
+
+ switch (key[0]) {
+ case DCP_PAES_KEY_SLOT0:
+ case DCP_PAES_KEY_SLOT1:
+ case DCP_PAES_KEY_SLOT2:
+ case DCP_PAES_KEY_SLOT3:
+ case DCP_PAES_KEY_UNIQUE:
+ case DCP_PAES_KEY_OTP:
+ memcpy(actx->key, key, len);
+ break;

I don't understand why the "commit" is split into two parts
(memcpy and assignments in different code blocks). You should
probably rather:

switch (key[0]) {
case DCP_PAES_KEY_SLOT0:
case DCP_PAES_KEY_SLOT1:
case DCP_PAES_KEY_SLOT2:
case DCP_PAES_KEY_SLOT3:
case DCP_PAES_KEY_UNIQUE:
case DCP_PAES_KEY_OTP:
memcpy(actx->key, key, len);
actx->key_len = len;
actx->refkey = true;
return 0;

default:
return -EINVAL;
}
}

Or alternatively you can move all operations after the switch-case
statement. IMHO, any state change is better to put into a singular
location.

Makes sense. I’ll change that.

quoted

diff --git a/include/soc/fsl/dcp.h b/include/soc/fsl/dcp.h
new file mode 100644
index 000000000000..df6678ee10a1
--- /dev/null
+++ b/include/soc/fsl/dcp.h

@@ -0,0 +1,19 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (C) 2021 sigma star gmbh
+ * Authors: David Gstir <david@sigma-star.at>
+ *          Richard Weinberger <richard@sigma-star.at>

Git already has author-field and commit can have co-developed-by so
this is totally obsolete.

Also noted for v3.

Thanks,
- David

--
sigma star gmbh | Eduard-Bodem-Gasse 6, 6020 Innsbruck, Austria
UID/VAT Nr: ATU 66964118 | FN: 374287y