Re: [PATCH v5 06/11] firmware: imx: add driver for NXP EdgeLock Enclave

Stefan Wahren
2023-08-25T10:29:37Z

Hi Pankaj,

Am 23.08.23 um 09:33 schrieb Pankaj Gupta:
> The Edgelock Enclave , is the secure enclave embedded in the SoC
> to support the features like HSM, SHE & V2X, using message based
> communication channel.
>
> ELE FW communicates on a dedicated MU with application core where
> kernel is running. It exists on specific i.MX processors. e.g.
> i.MX8ULP, i.MX93.
>
> Reported-by: kernel test robot <lkp@intel.com>
> Closes:https://lore.kernel.org/oe-kbuild-all/202304120902.bP52A56z-lkp@intel.com
> Signed-off-by: Pankaj Gupta <pankaj.gupta@nxp.com>
> ---
> Documentation/ABI/testing/se-cdev | 29 +
> drivers/firmware/imx/Kconfig | 12 +
> drivers/firmware/imx/Makefile | 2 +
> drivers/firmware/imx/ele_base_msg.c | 62 ++
> drivers/firmware/imx/ele_common.c | 34 +
> drivers/firmware/imx/ele_common.h | 21 +
> drivers/firmware/imx/se_fw.c | 1201 +++++++++++++++++++++
> drivers/firmware/imx/se_fw.h | 168 +++
> include/linux/firmware/imx/ele_base_msg.h | 37 +
> include/linux/firmware/imx/ele_mu_ioctl.h | 52 +
> 10 files changed, 1618 insertions(+)
> create mode 100644 Documentation/ABI/testing/se-cdev
> create mode 100644 drivers/firmware/imx/ele_base_msg.c
> create mode 100644 drivers/firmware/imx/ele_common.c
> create mode 100644 drivers/firmware/imx/ele_common.h
> create mode 100644 drivers/firmware/imx/se_fw.c
> create mode 100644 drivers/firmware/imx/se_fw.h
> create mode 100644 include/linux/firmware/imx/ele_base_msg.h
> create mode 100644 include/linux/firmware/imx/ele_mu_ioctl.h
...
> +
> +int ele_get_info(struct device *dev, phys_addr_t addr, u32 data_size)
> +{
> + struct ele_mu_priv *priv = dev_get_drvdata(dev);
> + int ret;
> + unsigned int tag, command, size, ver, status;
> +
> + ret = plat_fill_cmd_msg_hdr(priv,
> + (struct mu_hdr *)&priv->tx_msg.header,
> + ELE_GET_INFO_REQ, 16);
> + if (ret)
> + return ret;
> +
> + priv->tx_msg.data[0] = upper_32_bits(addr);
> + priv->tx_msg.data[1] = lower_32_bits(addr);
> + priv->tx_msg.data[2] = data_size;
> + ret = imx_ele_msg_send_rcv(priv);
> + if (ret < 0)
> + return ret;
> +
> + tag = MSG_TAG(priv->rx_msg.header);
> + command = MSG_COMMAND(priv->rx_msg.header);
> + size = MSG_SIZE(priv->rx_msg.header);
> + ver = MSG_VER(priv->rx_msg.header);
> + status = RES_STATUS(priv->rx_msg.data[0]);
> + if (tag == priv->rsp_tag &&
> + command == ELE_GET_INFO_REQ &&
> + size == ELE_GET_INFO_REQ_MSG_SZ &&
> + ver == ELE_BASE_API_VERSION &&
> + status == priv->success_tag)
> + return 0;
except of the coding style, i won't recommend this error handling. In
case a user report a failure of ele_get_info(), we need to figure out
which of these conditions failed. Why not check the conditions step by
step and give a detailed error message.

The same applies to the rest of the series.

Best regards
> +
> + return -EINVAL;
> +}
>