RE: [EXT] Re: [PATCH v12 4/5] firmware: imx: add driver for NXP EdgeLock Enclave

Pankaj Gupta
2025-01-22T11:13:41Z

[-- Attachment #1: Type: text/plain, Size: 13009 bytes --]

-----Original Message-----
From: Krzysztof Kozlowski <krzk@kernel.org>>
Sent: Monday, January 20, 2025 5:54 PM
To: Pankaj Gupta <pankaj.gupta@nxp.com>>; Jonathan Corbet <corbet@lwn.net>>;
Rob Herring <robh@kernel.org>>; Krzysztof Kozlowski <krzk+dt@kernel.org>>;
Conor Dooley <conor+dt@kernel.org>>; Shawn Guo <shawnguo@kernel.org>>;
Sascha Hauer <s.hauer@pengutronix.de>>; Pengutronix Kernel Team
<kernel@pengutronix.de>>; Fabio Estevam <festevam@gmail.com>>
Cc: linux-doc@vger.kernel.org; linux-kernel@vger.kernel.org;
devicetree@vger.kernel.org; imx@lists.linux.dev;
linux-arm-kernel@lists.infradead.org
Subject: [EXT] Re: [PATCH v12 4/5] firmware: imx: add driver for NXP
EdgeLock Enclave

Caution: This is an external email. Please take care when clicking links or
opening attachments. When in doubt, report the message using the 'Report
this email' button

On 20/01/2025 17:52, Pankaj Gupta wrote:
>> NXP hardware IP(s) for secure-enclaves like Edgelock Enclave(ELE), are
>> embedded in the SoC to support the features like HSM, SHE & V2X, using
>> message based communication interface.

> Fix your machine so this is not a "future" work.
Accepted. Correct the m/c time.

>>
>> The secure enclave FW communicates on a dedicated messaging unit(MU)
>> based interface(s) with application core, where kernel is running.
>> It exists on specific i.MX processors. e.g. i.MX8ULP, i.MX93.
>>
>> This patch adds the driver for communication interface to
>> secure-enclave,

>Please do not use "This commit/patch/change", but imperative mood. See
longer explanation here:
https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Felixir.boo
tlin.com%2Flinux%2Fv5.17.1%2Fsource%2FDocumentation%2Fprocess%2Fsubmitting-p
atches.rst%23L95&data=05%7C02%7Cpankaj.gupta%40nxp.com%7C82dee31b12c7489b3fc
408dd394d46d1%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63872972623236648
5%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOi
JXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=rzrVqx%2FAmPgI
r7iq4y1duYLgkB4jMm1RVYJfRMSmtJ4%3D&reserved=0

Accepted.
Will replace "This patch adds the driver for communication interface to
secure-enclave", with
"Adds the driver for communication interface to secure-enclave firmware."

>> for exchanging messages with NXP secure enclave HW IP(s) like EdgeLock
>> Enclave (ELE) from Kernel-space, used by kernel management layers like
>> - DM-Crypt.
>>
>> Signed-off-by: Pankaj Gupta <pankaj.gupta@nxp.com>>
>> ---

>> +int ele_fetch_soc_info(struct se_if_priv *priv, void *data) {
>> + int err;
>> +
>> + err = ele_get_info(priv, data);
>> + if (err < 0)
>> + return err;
>> +
>> + return err;
>> +}
>> +
>> +int ele_ping(struct se_if_priv *priv) {
>> + struct se_api_msg *tx_msg __free(kfree) = NULL;
>> + struct se_api_msg *rx_msg __free(kfree) = NULL;
>> + int ret = 0;
>> +
>> + if (!priv) {
>> + ret = -EINVAL;
>> + goto exit;

> This does not make sense. return.... but is this even possible?
This is added as part of previously received comment.

>> + }
>> +
>> + tx_msg = kzalloc(ELE_PING_REQ_SZ, GFP_KERNEL);
>> + if (!tx_msg) {
>> + ret = -ENOMEM;

> return -ENOMEM.
Accepted.
For all the similar changes throughout the driver, will replace this.

>> + goto exit;

> Please read in coding style how gotos are supposed to be used.
Accepted.
For all the similar changes throughout the driver, will replace this.

>> + }
>> +
>> + rx_msg = kzalloc(ELE_PING_RSP_SZ, GFP_KERNEL);
>> + if (!rx_msg) {
>> + ret = -ENOMEM;
>> + goto exit;
>> + }

Accepted.
For all the similar changes throughout the driver, will replace this.

>> +
>> + ret = se_fill_cmd_msg_hdr(priv,
>> + (struct se_msg_hdr
*)&tx_msg->>header,
>> + ELE_PING_REQ, ELE_PING_REQ_SZ,
>> + true);

> Fix your coding style - run checkpatch strict on this.
Accepted.
Ran the checkpatch with --strict option and fixed the issues.

>> + if (ret) {
>> + dev_err(priv->>dev, "Error: se_fill_cmd_msg_hdr
failed.\n");
>> + goto exit;
>> + }
>> +

...

>> +int ele_get_info(struct se_if_priv *priv, struct ele_dev_info
>> +*s_info); int ele_fetch_soc_info(struct se_if_priv *priv, void
>> +*data); int ele_ping(struct se_if_priv *priv); int
>> +ele_service_swap(struct se_if_priv *priv,
>> + phys_addr_t addr,
>> + u32 addr_size, u16 flag); int
>> +ele_fw_authenticate(struct se_if_priv *priv, phys_addr_t addr);
>> +#endif
>> diff --git a/drivers/firmware/imx/ele_common.c
>> b/drivers/firmware/imx/ele_common.c
>> new file mode 100644
>> index 000000000000..67d1fa761172
>> --- /dev/null
>> +++ b/drivers/firmware/imx/ele_common.c
>> @@ -0,0 +1,324 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * Copyright 2024 NXP
>> + */
>> +
>> +#include "ele_base_msg.h"
>> +#include "ele_common.h"
>> +
>> +u32 se_add_msg_crc(u32 *msg, u32 msg_len) {
>> + u32 nb_words = msg_len / (u32)sizeof(u32);
>> + u32 crc = 0;
>> + u32 i;
>> +
>> + for (i = 0; i < nb_words - 1; i++)
>> + crc ^= *(msg + i);
>> +
>> + return crc;
>> +}
>> +
>> +int ele_msg_rcv(struct se_if_priv *priv,
>> + struct se_clbk_handle *se_clbk_hdl) {
>> + int err = 0;
>> +
>> + do {
>> + /* If callback is executed before entrying to wait
>> + state,

> It is not a networking device. Use Linux coding style.
> You already got such comment long time ago and not much improved.

Will remove the comments from the function body.

>> +
>> +static int se_if_probe(struct platform_device *pdev) {
>> + const struct se_if_node_info_list *info_list;
>> + const struct se_if_node_info *info;
>> + struct device *dev = &pdev->>dev;
>> + struct se_fw_load_info *load_fw;
>> + struct se_if_priv *priv;
>> + u32 idx;
>> + int ret;
>> +q
>> + idx = GET_IDX_FROM_DEV_NODE_NAME(dev->>of_node);

> NAK. Node can be called firmware and your entire driver collapes.
The macro is updated to verify the correct-ness of node-name.

+ (!memcmp(dev_of_node->full_name, NODE_NAME,
strlen(NODE_NAME)) ?\
((strlen(dev_of_node->full_name) > strlen(NODE_NAME)) ?\
GET_ASCII_TO_U8((strlen(dev_of_node->full_name) -
strlen(NODE_NAME)),\
dev_of_node->full_name[strlen(NODE_NAME) +
1], \
- dev_of_node->full_name[strlen(NODE_NAME) +
2]) : 0)
+ dev_of_node->full_name[strlen(NODE_NAME) +
2]) : 0) : -EINVAL)

>> + info_list = device_get_match_data(dev);
>> + if (idx >>= info_list->>num_mu) {
>> + dev_err(dev,
>> + "Incorrect node name :%s\n",
>> + dev->>of_node->>full_name);

> Nope. "firmware" or "secure" are correct node names.
New check is added to validate the correctness of the node name for this
driver.
Replaced the message of " Incorrect node name..", with the help message.
- u32 idx;
+ int idx;
int ret;

idx = GET_IDX_FROM_DEV_NODE_NAME(dev->of_node);
+ if (idx < 0) {
+ dev_err(dev,
+ "Use \"secure-enclave-n\" as node name, where n = 0,
1, 2, ... is node-index.");
+ return -EINVAL;
+ }

> Where did you document this ABI?
Will Add new ABI document: "
Documentation/ABI/testing/sysfs-firmware-fsl-se", to reflect this.

+What: /sys/firmware/devicetree/base/firmware/secure-enclave-[0-9]
+Date: Jan 2025
+KernelVersion: 6.13
+Contact: linux-imx@nxp.com, pankaj.gupta@nxp.com
+Description:
+ NXP offers multiple hardware IP(s) for secure enclaves like
EdgeLock-
+ Enclave(ELE), SECO, v2x. The device node must be defined
with name as:
+ "secure-enclave-n", where n is 0, 1, 2, 3 ... index of the
node.

>> + dev_err(dev,
>> + "%s-<index>>, acceptable index range is 0..%d\n",
>> + dev->>of_node->>name,
>> + info_list->>num_mu - 1);
>> + ret = -EINVAL;
>> + return ret;
>> + }
>> +
>> + info = &info_list->>info[idx];
>> + if (!info) {
>> + ret = -EINVAL;
>> + goto exit;
>> + }
>> +
>> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> + if (!priv) {
>> + ret = -ENOMEM;
>> + goto exit;

> Nope, You don't get how common exit works. You are supposed to clean up in
comon exit paths, not print error paths, especially ones which are not
welcomed - like here.
Accepted.
Replaced "ret = -ENOMEM; goto exit;" with "return -ENOMEM;"

>> + }
>> +
>> + priv->>dev = dev;
>> + priv->>if_defs = &info->>if_defs;
>> + dev_set_drvdata(dev, priv);
>> +
>> + ret = devm_add_action(dev, se_if_probe_cleanup, pdev);
>> + if (ret)
>> + goto exit;
>> +
>> +
>> + /* Mailbox client configuration */
>> + priv->>se_mb_cl.dev = dev;
>> + priv->>se_mb_cl.tx_block = false;
>> + priv->>se_mb_cl.knows_txdone = true;
>> + priv->>se_mb_cl.rx_callback = se_if_rx_callback;
>> +
>> + ret = se_if_request_channel(dev, &priv->>tx_chan,
>> + &priv->>se_mb_cl, MBOX_TX_NAME);
>> + if (ret)
>> + goto exit;
>> +
>> + ret = se_if_request_channel(dev, &priv->>rx_chan,
>> + &priv->>se_mb_cl, MBOX_RX_NAME);
>> + if (ret)
>> + goto exit;
>> +
>> + mutex_init(&priv->>se_if_cmd_lock);
>> +
>> + init_completion(&priv->>waiting_rsp_clbk_hdl.done);
>> + init_completion(&priv->>cmd_receiver_clbk_hdl.done);
>> +
>> + if (info->>pool_name) {
>> + priv->>mem_pool = of_gen_pool_get(dev->>of_node,
>> + info->>pool_name,
0);
>> + if (!priv->>mem_pool) {
>> + dev_err(dev,
>> + "Unable to get sram pool = %s\n",
>> + info->>pool_name);
>> + goto exit;

> Why do you print erros twice?
Accepted.
Moved the dev_err to dev_dbg.

>> + }
>> + }
>> +
>> + if (info->>reserved_dma_ranges) {
>> + ret = of_reserved_mem_device_init(dev);
>> + if (ret) {
>> + dev_err(dev,
>> + "failed to init reserved memory region
%d\n",
>> + ret);
>> + goto exit;
>> + }
>> + }
>> +
>> + if (info->>if_defs.se_if_type == SE_TYPE_ID_HSM) {
>> + ret = se_soc_info(priv);
>> + if (ret) {
>> + dev_err(dev,
>> + "failed[%pe] to fetch SoC Info\n",
ERR_PTR(ret));
>> + goto exit;
>> + }
>> + }
>> +
>> + /* By default, there is no pending FW to be loaded.*/
>> + if (info_list->>se_fw_img_nm.prim_fw_nm_in_rfs ||
>> + info_list->>se_fw_img_nm.seco_fw_nm_in_rfs) {
>> + load_fw = get_load_fw_instance(priv);
>> + load_fw->>se_fw_img_nm = &info_list->>se_fw_img_nm;
>> + load_fw->>is_fw_loaded = false;
>> +
>> + if (info_list->>se_fw_img_nm.prim_fw_nm_in_rfs) {
>> + /* allocate buffer where SE store encrypted IMEM */
>> + load_fw->>imem.buf =
dmam_alloc_coherent(priv->>dev, ELE_IMEM_SIZE,
>> +
&load_fw->>imem.phyaddr,
>> +
GFP_KERNEL);
>> + if (!load_fw->>imem.buf) {
>> + dev_err(priv->>dev,
>> + "dmam-alloc-failed: To store
encr-IMEM.\n");
>> + ret = -ENOMEM;
>> + goto exit;
>> + }
>> + load_fw->>imem_mgmt = true;
>> + }
>> + }
>> + dev_info(dev, "i.MX secure-enclave: %s%d interface to firmware,
configured.\n",
>> + SE_TYPE_STR_HSM,
>> + priv->>if_defs->>se_instance_id);

> Drop probe success. Useless.
There are multiple SE interfaces.
This message confirms SE communication interface to FW, is successfully
established and configured.

>> + return ret;
>> +
>> +exit:
>> + /* if execution control reaches here, if probe fails.
>> + */

> Obvious comment.
Accepted. Removed.

>> + return dev_err_probe(dev, ret, "%s: Probe failed.", __func__);

> Drop. I think I asked already long time - like 10 revisiosn ago - to drop
simple function debug messages. Look at other drivers how exit paths are
handled.

Accepted.

Will replace "%s: Probe failed.", with meaningful info "Failed to configure:
i.MX secure-enclave: %s%d interface."

Best regards,
Krzysztof

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 11094 bytes --]