-----Original Message----- From: Krzysztof Kozlowski <krzk@xxxxxxxxxx>> Sent: Monday, January 20, 2025 5:54 PM To: Pankaj Gupta <pankaj.gupta@xxxxxxx>>; Jonathan Corbet <corbet@xxxxxxx>>; Rob Herring <robh@xxxxxxxxxx>>; Krzysztof Kozlowski <krzk+dt@xxxxxxxxxx>>; Conor Dooley <conor+dt@xxxxxxxxxx>>; Shawn Guo <shawnguo@xxxxxxxxxx>>; Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>>; Pengutronix Kernel Team <kernel@xxxxxxxxxxxxxx>>; Fabio Estevam <festevam@xxxxxxxxx>> Cc: linux-doc@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; imx@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx 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@xxxxxxx>> >> --- >> +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@xxxxxxx, pankaj.gupta@xxxxxxx +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:
smime.p7s
Description: S/MIME cryptographic signature
