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

[Stefan Wahren]

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@xxxxxxxxx>
> Closes:https://lore.kernel.org/oe-kbuild-all/202304120902.bP52A56z-lkp@xxxxxxxxx
> Signed-off-by: Pankaj Gupta <pankaj.gupta@xxxxxxx>
> ---
>   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;
> +}