Re: [EXTERNAL] [PATCH v6 5/5] firmware: imx: adds miscdev

[Amit Singh Tomar <amitsinght@marvell.com>]

Hi Pankaj,
> 
> Adds the driver for communication interface to secure-enclave,
> for exchanging messages with NXP secure enclave HW IP(s) like
> EdgeLock Enclave from:
> - User-Space Applications via character driver.
> 
> ABI documentation for the NXP secure-enclave driver.
> 
> User-space library using this driver:
> - i.MX Secure Enclave library:
>     -- URL:https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_nxp-2Dimx_imx-2Dsecure-2Denclave.git&d=DwICaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=V_GK7jRuCHDErm6txmgDK1-MbUihtnSQ3gPgB-A-JKU&m=hqz6ztDhob0jujItfaaf7PHhtqSHj4aoWie1-b4nAGXTUrSyBQtV9W-WqLaM_utz&s=C67hc24yMATzUglvGvywzpn0Efjurb6sOLm2V_9VpsI&e=  <https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_nxp-2Dimx_imx-2Dsecure-2Denclave.git&d=DwICaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=V_GK7jRuCHDErm6txmgDK1-MbUihtnSQ3gPgB-A-JKU&m=hqz6ztDhob0jujItfaaf7PHhtqSHj4aoWie1-b4nAGXTUrSyBQtV9W-WqLaM_utz&s=C67hc24yMATzUglvGvywzpn0Efjurb6sOLm2V_9VpsI&e=>,
> - i.MX Secure Middle-Ware:
>     -- URL:https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_nxp-2Dimx_imx-2Dsmw.git&d=DwICaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=V_GK7jRuCHDErm6txmgDK1-MbUihtnSQ3gPgB-A-JKU&m=hqz6ztDhob0jujItfaaf7PHhtqSHj4aoWie1-b4nAGXTUrSyBQtV9W-WqLaM_utz&s=NACAFfnEzGKFI7FlqdL4kxlt8PtxeXRorc3IWanqgtY&e=  <https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_nxp-2Dimx_imx-2Dsmw.git&d=DwICaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=V_GK7jRuCHDErm6txmgDK1-MbUihtnSQ3gPgB-A-JKU&m=hqz6ztDhob0jujItfaaf7PHhtqSHj4aoWie1-b4nAGXTUrSyBQtV9W-WqLaM_utz&s=NACAFfnEzGKFI7FlqdL4kxlt8PtxeXRorc3IWanqgtY&e=>
> 
> Signed-off-by: Pankaj Gupta <pankaj.gupta@nxp.com>
> ---
>    Documentation/ABI/testing/se-cdev |  43 +++
>    drivers/firmware/imx/ele_common.c | 192 ++++++++++-
>    drivers/firmware/imx/ele_common.h |   4 +
>    drivers/firmware/imx/se_ctrl.c    | 677 ++++++++++++++++++++++++++++++++++++++
>    drivers/firmware/imx/se_ctrl.h    |  46 +++
>    include/uapi/linux/se_ioctl.h     |  94 ++++++
>    6 files changed, 1053 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/se-cdev b/Documentation/ABI/testing/se-cdev
> new file mode 100644
> index 000000000000..3451c909ccc4
> --- /dev/null
> +++ b/Documentation/ABI/testing/se-cdev
> @@ -0,0 +1,43 @@
> +What:		/dev/<se>_mu[0-9]+_ch[0-9]+
> +Date:		May 2024
> +KernelVersion:	6.8
> +Contact:	linux-imx@nxp.com, pankaj.gupta@nxp.com
> +Description:
> +		NXP offers multiple hardware IP(s) for secure enclaves like EdgeLock-
> +		Enclave(ELE), SECO. The character device file descriptors
> +		/dev/<se>_mu*_ch* are the interface between userspace NXP's secure-
> +		enclave shared library and the kernel driver.
> +
> +		The ioctl(2)-based ABI is defined and documented in
> +		[include]<linux/firmware/imx/ele_mu_ioctl.h>.
> +		ioctl(s) are used primarily for:
> +			- shared memory management
> +			- allocation of I/O buffers
> +			- getting mu info
> +			- setting a dev-ctx as receiver to receive all the commands from FW
> +			- getting SoC info
> +			- send command and receive command response
> +
> +		The following file operations are supported:
> +
> +		open(2)
> +		  Currently the only useful flags are O_RDWR.
> +
> +		read(2)
> +		  Every read() from the opened character device context is waiting on
> +		  wait_event_interruptible, that gets set by the registered mailbox callback
> +		  function, indicating a message received from the firmware on message-
> +		  unit.
> +
> +		write(2)
> +		  Every write() to the opened character device context needs to acquire
> +		  mailbox_lock before sending message on to the message unit.
> +
> +		close(2)
> +		  Stops and frees up the I/O contexts that were associated
> +		  with the file descriptor.
> +
> +Users:		https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_nxp-2Dimx_imx-2Dsecure-2Denclave.git&d=DwICaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=V_GK7jRuCHDErm6txmgDK1-MbUihtnSQ3gPgB-A-JKU&m=hqz6ztDhob0jujItfaaf7PHhtqSHj4aoWie1-b4nAGXTUrSyBQtV9W-WqLaM_utz&s=C67hc24yMATzUglvGvywzpn0Efjurb6sOLm2V_9VpsI&e=  <https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_nxp-2Dimx_imx-2Dsecure-2Denclave.git&d=DwICaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=V_GK7jRuCHDErm6txmgDK1-MbUihtnSQ3gPgB-A-JKU&m=hqz6ztDhob0jujItfaaf7PHhtqSHj4aoWie1-b4nAGXTUrSyBQtV9W-WqLaM_utz&s=C67hc24yMATzUglvGvywzpn0Efjurb6sOLm2V_9VpsI&e=>,
> +		https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_nxp-2Dimx_imx-2Dsmw.git&d=DwICaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=V_GK7jRuCHDErm6txmgDK1-MbUihtnSQ3gPgB-A-JKU&m=hqz6ztDhob0jujItfaaf7PHhtqSHj4aoWie1-b4nAGXTUrSyBQtV9W-WqLaM_utz&s=NACAFfnEzGKFI7FlqdL4kxlt8PtxeXRorc3IWanqgtY&e=  <https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_nxp-2Dimx_imx-2Dsmw.git&d=DwICaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=V_GK7jRuCHDErm6txmgDK1-MbUihtnSQ3gPgB-A-JKU&m=hqz6ztDhob0jujItfaaf7PHhtqSHj4aoWie1-b4nAGXTUrSyBQtV9W-WqLaM_utz&s=NACAFfnEzGKFI7FlqdL4kxlt8PtxeXRorc3IWanqgtY&e=>
> +		crypto/skcipher,
> +		drivers/nvmem/imx-ocotp-ele.c
> diff --git a/drivers/firmware/imx/ele_common.c b/drivers/firmware/imx/ele_common.c
> index 3a6584d6f6f2..8167ae201b83 100644
> --- a/drivers/firmware/imx/ele_common.c
> +++ b/drivers/firmware/imx/ele_common.c
> @@ -78,6 +78,149 @@ int ele_msg_send_rcv(struct se_if_priv *priv, void *tx_msg, void *rx_msg)
>    	return err;
>    }
>    
> +int ele_miscdev_msg_rcv(struct se_if_device_ctx *dev_ctx,
> +			void *rx_buf,
> +			int rx_buf_sz)
> +{
> +	struct se_msg_hdr *header;
> +	int err;
> +
> +	err = wait_event_interruptible(dev_ctx->wq, dev_ctx->pending_hdr != 0);
> +	if (err) {
> +		dev_err(dev_ctx->dev,
> +			"%s: Err[0x%x]:Interrupted by signal.\n",
> +			dev_ctx->miscdev.name, err);
> +		goto exit;
> +	}
> +
> +	header = (struct se_msg_hdr *) dev_ctx->temp_resp;
> +
> +	if (header->tag == dev_ctx->priv->rsp_tag) {
> +		if (dev_ctx->priv->waiting_rsp_dev && dev_ctx->priv->waiting_rsp_dev != dev_ctx) {
> +			dev_warn(dev_ctx->dev,
> +			"Dev-ctx waiting for response mismatch (%s != %s).\n",
> +			dev_ctx->miscdev.name, dev_ctx->priv->waiting_rsp_dev->miscdev.name);
> +			err = -EPERM;
> +			goto exit;
> +		}
> +	}
> +
> +	dev_dbg(dev_ctx->dev,
> +		"%s: %s %s\n",
> +		dev_ctx->miscdev.name,
> +		__func__,
> +		"message received, start transmit to user");
> +
> +	/*
> +	 * Check that the size passed as argument is larger than
> +	 * the one carried in the message.
> +	 *
> +	 * In case of US-command/response, the dev_ctx->temp_resp_size
> +	 * is set before sending the command.
> +	 *
> +	 * In case of NVM Slave-command/response, the dev_ctx->temp_resp_size
> +	 * is set after receing the message from mailbox.
> +	 */
> +	if (dev_ctx->temp_resp_size > rx_buf_sz) {
> +		dev_err(dev_ctx->dev,
> +			"%s: User buffer too small (%d < %d)\n",
> +			dev_ctx->miscdev.name,
> +			rx_buf_sz, dev_ctx->temp_resp_size);
> +		 dev_ctx->temp_resp_size = rx_buf_sz;
> +	}
> +
> +	/* We may need to copy the output data to user before
> +	 * delivering the completion message.
> +	 */
> +	err = se_dev_ctx_cpy_out_data(dev_ctx, true);
> +	if (err < 0)
> +		goto exit;
> +
> +	/* Copy data from the buffer */
> +	print_hex_dump_debug("to user ", DUMP_PREFIX_OFFSET, 4, 4,
> +			     dev_ctx->temp_resp, dev_ctx->temp_resp_size, false);
> +	if (copy_to_user(rx_buf, dev_ctx->temp_resp, dev_ctx->temp_resp_size)) {
> +		dev_err(dev_ctx->dev,
> +			"%s: Failed to copy to user\n",
> +			dev_ctx->miscdev.name);
> +		err = -EFAULT;
> +		goto exit;
> +	}
> +
> +	err = dev_ctx->temp_resp_size;
> +exit:
> +	if (err < 0)
> +		se_dev_ctx_cpy_out_data(dev_ctx, false);
> +
> +	/* free memory allocated on the shared buffers. */
> +	dev_ctx->secure_mem.pos = 0;
> +	dev_ctx->non_secure_mem.pos = 0;
> +
> +	dev_ctx->pending_hdr = 0;
> +	se_dev_ctx_shared_mem_cleanup(dev_ctx);
> +
> +	return err;
> +}
> +
> +int ele_miscdev_msg_send(struct se_if_device_ctx *dev_ctx,
> +			 void *tx_msg, int tx_msg_sz)
> +{
> +	struct se_if_priv *priv = dev_ctx->priv;
> +	struct se_msg_hdr *header;
> +	u32 size_to_send;
> +	int err;
> +
> +	header = (struct se_msg_hdr *) tx_msg;
> +
> +	/*
> +	 * Check that the size passed as argument matches the size
> +	 * carried in the message.
> +	 */
> +	size_to_send = header->size << 2;
> +
> +	if (size_to_send != tx_msg_sz) {
> +		err = -EINVAL;
> +		dev_err(priv->dev,
> +			"%s: User buf hdr(0x%x) sz mismatced with input-sz (%d != %d).\n",
> +			dev_ctx->miscdev.name, *(u32 *)header, size_to_send, tx_msg_sz);
> +		goto exit;
> +	}
> +
> +	/* Check the message is valid according to tags */
> +	if (header->tag == priv->rsp_tag) {
> +		/* Check the device context can send the command */
> +		if (dev_ctx != priv->cmd_receiver_dev) {
> +			dev_err(priv->dev,
> +				"%s: Channel not configured to send resp to FW.",
> +				dev_ctx->miscdev.name);
> +			err = -EPERM;
> +			goto exit;
> +		}
> +	} else if (header->tag == priv->cmd_tag) {
> +		if (priv->waiting_rsp_dev != dev_ctx) {
> +			dev_err(priv->dev,
> +				"%s: Channel not configured to send cmd to FW.",
> +				dev_ctx->miscdev.name);
> +			err = -EPERM;
> +			goto exit;
> +		}
> +		lockdep_assert_held(&priv->se_if_cmd_lock);
> +	} else {
> +		dev_err(priv->dev,
> +			"%s: The message does not have a valid TAG\n",
> +			dev_ctx->miscdev.name);
> +		err = -EINVAL;
> +		goto exit;
> +	}
> +	err = ele_msg_send(priv, tx_msg);
> +	if (err < 0)
> +		goto exit;
> +
> +	err = size_to_send;
> +exit:
> +	return err;
> +}
> +
>    static bool exception_for_size(struct se_if_priv *priv,
>    				struct se_msg_hdr *header)
>    {
> @@ -99,6 +242,7 @@ static bool exception_for_size(struct se_if_priv *priv,
>    void se_if_rx_callback(struct mbox_client *mbox_cl, void *msg)
>    {
>    	struct device *dev = mbox_cl->dev;
> +	struct se_if_device_ctx *dev_ctx;
>    	struct se_if_priv *priv;
>    	struct se_msg_hdr *header;
>    	u32 rx_msg_sz;
> @@ -114,8 +258,50 @@ void se_if_rx_callback(struct mbox_client *mbox_cl, void *msg)
>    	header = msg;
>    	rx_msg_sz = header->size << 2;
>    
> -	if (header->tag == priv->rsp_tag) {
> -		if (!priv->waiting_rsp_dev) {
> +	/* Incoming command: wake up the receiver if any. */
> +	if (header->tag == priv->cmd_tag) {
> +		dev_dbg(dev, "Selecting cmd receiver\n");
> +		dev_ctx = priv->cmd_receiver_dev;
> +		/* Pre-allocated buffer of MAX_NVM_MSG_LEN
> +		 * as the NVM command are initiated by FW.
> +		 * Size is revealed as part of this call function.
> +		 */
> +		if (rx_msg_sz > MAX_NVM_MSG_LEN) {
> +			dev_err(dev,
> +				"%s: Msg recvd hdr(0x%x) with greater[%d] than allocated buf-sz.\n",
> +				dev_ctx->miscdev.name,
> +				*(u32 *) header,
> +				rx_msg_sz);
> +		} else
> +			memcpy(dev_ctx->temp_resp, msg, rx_msg_sz);

It is categorically stated (in the Linux kernel coding style guide) that 
this rule does not apply if only one branch of a conditional statement 
consists of a single statement. In such cases, you should categorically 
use braces for both branches of the conditional statement:

if (condition) {
         do_this();
         do_that();
} else {
         otherwise();
}

Also, made a similar comment on the earlier version (v5) as well:
https://patchwork.kernel.org/project/imx/patch/20240712-imx-se-if-v5-4-66a79903a872@nxp.com/

Thanks
-Amit