[PATCH v2 1/3] crypto: mxs-dcp: Add support for hardware provided keys

[David Gstir]

Hi Jarkko,

thanks for the review!

> On 12.09.2023, at 19:32, Jarkko Sakkinen <jarkko at kernel.org> wrote:
> 
> On Tue Sep 12, 2023 at 2:11 PM EEST, David Gstir wrote:

[...]

>> - /* 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.


>> +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.


>> 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 at sigma-star.at>
>> + *          Richard Weinberger <richard at 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