Re: [PATCH v7 5/5] firmware: imx: adds miscdev

From: Sascha Hauer
Date: Mon Sep 09 2024 - 08:22:39 EST

Next message: Connor Abbott: "Re: [PATCH v3 04/10] drm/msm/A6xx: Implement preemption for A7XX targets"
Previous message: Andi Shyti: "Re: [PATCH v2 7/7] i2c: designware: Group all DesignWare drivers under a single option"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Wed, Sep 04, 2024 at 04:21:21PM +0530, Pankaj Gupta wrote:
> 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://github.com/nxp-imx/imx-secure-enclave.git,
> - i.MX Secure Middle-Ware:
> -- URL: https://github.com/nxp-imx/imx-smw.git
>
> Signed-off-by: Pankaj Gupta <pankaj.gupta@xxxxxxx>
> ---
> Documentation/ABI/testing/se-cdev | 43 ++
> drivers/firmware/imx/ele_base_msg.c | 8 +-
> drivers/firmware/imx/ele_common.c | 36 +-
> drivers/firmware/imx/ele_common.h | 6 +-
> drivers/firmware/imx/se_ctrl.c | 790 ++++++++++++++++++++++++++++++++++++
> drivers/firmware/imx/se_ctrl.h | 52 +++
> include/uapi/linux/se_ioctl.h | 94 +++++
> 7 files changed, 1010 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/firmware/imx/ele_base_msg.c b/drivers/firmware/imx/ele_base_msg.c
> index e3e570a25e85..ae200ce64100 100644
> --- a/drivers/firmware/imx/ele_base_msg.c
> +++ b/drivers/firmware/imx/ele_base_msg.c
> @@ -68,7 +68,7 @@ int ele_get_info(struct device *dev, struct ele_dev_info *s_info)
> tx_msg->data[0] = upper_32_bits(get_info_addr);
> tx_msg->data[1] = lower_32_bits(get_info_addr);
> tx_msg->data[2] = sizeof(*s_info);
> - ret = ele_msg_send_rcv(priv,
> + ret = ele_msg_send_rcv(priv->priv_dev_ctx,
> tx_msg,
> ELE_GET_INFO_REQ_MSG_SZ,
> rx_msg,
> @@ -150,7 +150,7 @@ int ele_ping(struct device *dev)
> goto exit;
> }
>
> - ret = ele_msg_send_rcv(priv,
> + ret = ele_msg_send_rcv(priv->priv_dev_ctx,
> tx_msg,
> ELE_PING_REQ_SZ,
> rx_msg,
> @@ -206,7 +206,7 @@ int ele_service_swap(struct device *dev,
> tx_msg->data[3] = lower_32_bits(addr);
> tx_msg->data[4] = se_add_msg_crc((uint32_t *)&tx_msg[0],
> ELE_SERVICE_SWAP_REQ_MSG_SZ);
> - ret = ele_msg_send_rcv(priv,
> + ret = ele_msg_send_rcv(priv->priv_dev_ctx,
> tx_msg,
> ELE_SERVICE_SWAP_REQ_MSG_SZ,
> rx_msg,
> @@ -268,7 +268,7 @@ int ele_fw_authenticate(struct device *dev, phys_addr_t addr)
> tx_msg->data[0] = lower_32_bits(addr);
> tx_msg->data[2] = addr;
>
> - ret = ele_msg_send_rcv(priv,
> + ret = ele_msg_send_rcv(priv->priv_dev_ctx,
> tx_msg,
> ELE_FW_AUTH_REQ_SZ,
> rx_msg,
> diff --git a/drivers/firmware/imx/ele_common.c b/drivers/firmware/imx/ele_common.c
> index a06d7015d3f1..9114c3594567 100644
> --- a/drivers/firmware/imx/ele_common.c
> +++ b/drivers/firmware/imx/ele_common.c
> @@ -18,16 +18,18 @@ u32 se_add_msg_crc(u32 *msg, u32 msg_len)
> return crc;
> }
>
> -int ele_msg_rcv(struct se_if_priv *priv,
> +int ele_msg_rcv(struct se_if_device_ctx *dev_ctx,
> struct se_clbk_handle *se_clbk_hdl)
> {
> + struct se_if_priv *priv = dev_ctx->priv;
> int err = 0;
>
> err = wait_event_interruptible(se_clbk_hdl->wq,
> atomic_read(&se_clbk_hdl->pending_hdr) != 0);
> if (err < 0)
> dev_err(priv->dev,
> - "Err[0x%x]:Interrupted by signal.\n",
> + "%s: Err[0x%x]:Interrupted by signal.\n",
> + se_clbk_hdl->dev_ctx->devname,
> err);
> else
> err = se_clbk_hdl->rx_msg_sz;
> @@ -37,10 +39,11 @@ int ele_msg_rcv(struct se_if_priv *priv,
> return err;
> }
>
> -int ele_msg_send(struct se_if_priv *priv,
> +int ele_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;
> int err;
>
> @@ -53,7 +56,8 @@ int ele_msg_send(struct se_if_priv *priv,
> if (header->size << 2 != tx_msg_sz) {
> err = -EINVAL;
> dev_err(priv->dev,
> - "User buf hdr: 0x%x, sz mismatced with input-sz (%d != %d).",
> + "%s: User buf hdr: 0x%x, sz mismatced with input-sz (%d != %d).",
> + dev_ctx->devname,
> *(u32 *)header,
> header->size << 2, tx_msg_sz);
> goto exit;
> @@ -62,7 +66,9 @@ int ele_msg_send(struct se_if_priv *priv,
>
> err = mbox_send_message(priv->tx_chan, tx_msg);
> if (err < 0) {
> - dev_err(priv->dev, "Error: mbox_send_message failure.\n");
> + dev_err(priv->dev,
> + "%s: Error: mbox_send_message failure.",
> + dev_ctx->devname);
> return err;
> }
> err = tx_msg_sz;
> @@ -72,24 +78,26 @@ int ele_msg_send(struct se_if_priv *priv,
> }
>
> /* API used for send/receive blocking call. */
> -int ele_msg_send_rcv(struct se_if_priv *priv,
> +int ele_msg_send_rcv(struct se_if_device_ctx *dev_ctx,

You are heavily patching a file you introduced just in the last patch.
It seems you have messed up while rebasing. Please cleanup.

> +static int se_ioctl_cmd_snd_rcv_rsp_handler(struct se_if_device_ctx *dev_ctx,
> + u64 arg)
> +{
> + struct se_if_priv *priv = dev_ctx->priv;
> + struct se_ioctl_cmd_snd_rcv_rsp_info cmd_snd_rcv_rsp_info;
> + struct se_api_msg *tx_msg __free(kfree) = NULL;
> + struct se_api_msg *rx_msg __free(kfree) = NULL;
> + int err = 0;
> +
> + if (down_interruptible(&dev_ctx->fops_lock))
> + return -EBUSY;
> +
> + if (dev_ctx->status != SE_IF_CTX_OPENED) {
> + err = -EINVAL;
> + goto exit;
> + }

You don't need this. Just trust Linux that it won't call your
ioctl/read/write ops when the device is not opened.

> +
> + if (copy_from_user(&cmd_snd_rcv_rsp_info, (u8 *)arg,
> + sizeof(cmd_snd_rcv_rsp_info))) {
> + dev_err(dev_ctx->priv->dev,

You have priv as a variable already, please use it throughout this
function.

> +static ssize_t se_if_fops_read(struct file *fp, char __user *buf,
> + size_t size, loff_t *ppos)
> +{
> + struct se_if_device_ctx *dev_ctx = fp->private_data;
> + struct se_if_priv *priv = dev_ctx->priv;
> + int err;
> +
> + dev_dbg(priv->dev,
> + "%s: read to buf %p(%zu), ppos=%lld\n",
> + dev_ctx->miscdev.name,
> + buf, size, ((ppos) ? *ppos : 0));
> +
> + if (down_interruptible(&dev_ctx->fops_lock))
> + return -EBUSY;
> +
> + if (dev_ctx->status != SE_IF_CTX_OPENED) {
> + err = -EINVAL;
> + goto exit;
> + }
> +
> + if (dev_ctx != priv->cmd_receiver_clbk_hdl.dev_ctx) {
> + err = -EINVAL;
> + goto exit;
> + }
> +
> + err = ele_msg_rcv(dev_ctx, &priv->cmd_receiver_clbk_hdl);
> + if (err < 0) {
> + dev_err(dev_ctx->priv->dev,
> + "%s: Err[0x%x]:Interrupted by signal.\n",
> + dev_ctx->devname, err);
> + dev_err(dev_ctx->priv->dev,
> + "Current active dev-ctx count = %d.\n",
> + dev_ctx->priv->active_devctx_count);

active_devctx_count is never used except for printing this message. I
don't think it provides meaningful information for userspace. Just dorp
it.

> + dma_free_coherent(priv->dev, MAX_DATA_SIZE_PER_USER,
> + se_shared_mem_mgmt->non_secure_mem.ptr,
> + se_shared_mem_mgmt->non_secure_mem.dma_addr);
> +
> + se_shared_mem_mgmt->non_secure_mem.ptr = NULL;
> + se_shared_mem_mgmt->non_secure_mem.dma_addr = 0;
> + se_shared_mem_mgmt->non_secure_mem.size = 0;
> + se_shared_mem_mgmt->non_secure_mem.pos = 0;
> +}
> +
> +/* Need to copy the output data to user-device context.
> + */
> +int se_dev_ctx_cpy_out_data(struct se_shared_mem_mgmt_info *se_shared_mem_mgmt)

Only used in this file. Make it static.

> +{
> + struct se_if_device_ctx *dev_ctx = container_of(se_shared_mem_mgmt,
> + typeof(*dev_ctx),
> + se_shared_mem_mgmt);

This function is only ever called like:

se_dev_ctx_cpy_out_data(&dev_ctx->se_shared_mem_mgmt);

Instead of this back and forth you could pass a struct se_if_device_ctx *
directly.

> +static int add_b_desc_to_pending_list(void *shared_ptr_with_pos,
> + struct se_ioctl_setup_iobuf *io,
> + struct se_shared_mem_mgmt_info *se_shared_mem_mgmt)
> +{
> + struct se_buf_desc *b_desc = NULL;
> +
> + b_desc = kzalloc(sizeof(*b_desc), GFP_KERNEL);
> + if (!b_desc)
> + return -ENOMEM;
> +
> + if (b_desc) {

b_desc is not NULL here.

> + b_desc->shared_buf_ptr = shared_ptr_with_pos;
> + b_desc->usr_buf_ptr = io->user_buf;
> + b_desc->size = io->length;
> +
> + if (io->flags & SE_IO_BUF_FLAGS_IS_INPUT) {
> + /*
> + * buffer is input:
> + * add an entry in the "pending input buffers" list so
> + * that copied data can be cleaned from shared memory
> + * later.
> + */
> + list_add_tail(&b_desc->link, &se_shared_mem_mgmt->pending_in);
> + } else {
> + /*
> + * buffer is output:
> + * add an entry in the "pending out buffers" list so data
> + * can be copied to user space when receiving Secure-Enclave
> + * response.
> + */
> + list_add_tail(&b_desc->link, &se_shared_mem_mgmt->pending_out);
> + }
> + }
> +
> +
> + return 0;
> +}
> +
> +/* Open a character device. */
> +static int se_if_fops_open(struct inode *nd, struct file *fp)
> +{
> + struct se_if_device_ctx *dev_ctx = container_of(fp->private_data,
> + struct se_if_device_ctx,
> + miscdev);
> + struct se_if_priv *priv = dev_ctx->priv;
> + int err = 0;
> +
> + priv->dev_ctx_mono_count++;
> + err = init_device_context(priv,
> + priv->dev_ctx_mono_count ?
> + priv->dev_ctx_mono_count
> + : priv->dev_ctx_mono_count++,

priv->dev_ctx_mono_count won't be 0 here as you just increased it.

> + &dev_ctx);
> + if (err) {
> + dev_err(priv->dev,
> + "Failed[0x%x] to create device contexts.\n",
> + err);
> + goto exit;
> + }
> +
> + dev_ctx->status = SE_IF_CTX_OPENED;
> +
> + fp->private_data = dev_ctx;
> +
> +exit:
> + up(&dev_ctx->fops_lock);

You never acquired this semaphore which is likely not what you want.

> + return err;
> +}
> +
> +/* Close a character device. */
> +static int se_if_fops_close(struct inode *nd, struct file *fp)
> +{
> + struct se_if_device_ctx *dev_ctx = fp->private_data;
> + struct se_if_priv *priv = dev_ctx->priv;
> +
> + /* Avoid race if closed at the same time */
> + if (down_trylock(&dev_ctx->fops_lock))
> + return -EBUSY;
> +
> + /* The device context has not been opened */
> + if (dev_ctx->status != SE_IF_CTX_OPENED)
> + goto exit;
> +
> + /* check if this device was registered as command receiver. */
> + if (priv->cmd_receiver_clbk_hdl.dev_ctx == dev_ctx) {

This is racy. priv->cmd_receiver_clbk_hdl can be accessed by multiple
tasks without locking.

> + priv->cmd_receiver_clbk_hdl.dev_ctx = NULL;
> + kfree(priv->cmd_receiver_clbk_hdl.rx_msg);
> + priv->cmd_receiver_clbk_hdl.rx_msg = NULL;
> + atomic_set(&priv->cmd_receiver_clbk_hdl.pending_hdr, 0);
> + }
> +
> + /* check if this device was registered as waiting response. */
> + if (priv->waiting_rsp_clbk_hdl.dev_ctx == dev_ctx) {

> + priv->waiting_rsp_clbk_hdl.dev_ctx = NULL;
> + mutex_unlock(&priv->se_if_cmd_lock);

This mutex will never be locked here.

> + }
> +
> + se_dev_ctx_shared_mem_cleanup(&dev_ctx->se_shared_mem_mgmt);
> + cleanup_se_shared_mem(&dev_ctx->se_shared_mem_mgmt);
> + dev_ctx->status = SE_IF_CTX_FREE;
> +
> + priv->active_devctx_count--;
> + list_del(&dev_ctx->link);
> +exit:
> + up(&dev_ctx->fops_lock);
> + kfree(dev_ctx->devname);
> + kfree(dev_ctx);
> +
> + return 0;
> +}
> +
> static void se_if_probe_cleanup(struct platform_device *pdev)
> {
> + struct se_if_device_ctx *dev_ctx, *t_dev_ctx;
> struct device *dev = &pdev->dev;
> struct se_if_priv *priv;
>
> @@ -247,6 +939,14 @@ static void se_if_probe_cleanup(struct platform_device *pdev)
> priv->imem.buf = NULL;
> }
>
> + list_for_each_entry_safe(dev_ctx, t_dev_ctx, &priv->dev_ctx_list, link) {
> + list_del(&dev_ctx->link);
> + priv->active_devctx_count--;
> + }

The only place where this list is ever iterated over is here. active_devctx_count
is never used except for printing a message (see other comment).
Just drop this list.

> +
> + devm_remove_action(dev, if_misc_deregister, &priv->priv_dev_ctx->miscdev);
> + misc_deregister(&priv->priv_dev_ctx->miscdev);
> +
> /* No need to check, if reserved memory is allocated
> * before calling for its release. Or clearing the
> * un-set bit.
> @@ -254,6 +954,87 @@ static void se_if_probe_cleanup(struct platform_device *pdev)
> of_reserved_mem_device_release(dev);
> }
>
> +int init_device_context(struct se_if_priv *priv, int ch_id,
> + struct se_if_device_ctx **new_dev_ctx)
> +{

static.

> + const struct se_if_node_info *info = priv->info;
> + struct se_if_device_ctx *dev_ctx;
> + int ret = 0;
> +
> + if (ch_id)
> + dev_ctx = kzalloc(sizeof(*dev_ctx), GFP_KERNEL);
> + else
> + dev_ctx = devm_kzalloc(priv->dev, sizeof(*dev_ctx), GFP_KERNEL);
> +
> + if (!dev_ctx) {
> + ret = -ENOMEM;
> + return ret;
> + }
> +
> + dev_ctx->status = SE_IF_CTX_FREE;
> + dev_ctx->priv = priv;
> +
> + if (ch_id)
> + dev_ctx->devname = kasprintf(GFP_KERNEL, "%s_ch%d",
> + info->se_name, ch_id);
> + else
> + dev_ctx->devname = devm_kasprintf(priv->dev,
> + GFP_KERNEL, "%s_ch%d",
> + info->se_name, ch_id);
> + if (!dev_ctx->devname) {
> + ret = -ENOMEM;
> + if (ch_id)
> + kfree(dev_ctx);
> +
> + return ret;
> + }
> +
> + sema_init(&dev_ctx->fops_lock, 1);

This semaphore only counts up to one in your code which basically makes
it a mutex. Just use a mutex instead.

> +/* Private struct for each char device instance. */
> +struct se_if_device_ctx {
> + struct se_if_priv *priv;
> + struct miscdevice miscdev;
> + const char *devname;
> +
> + enum se_if_dev_ctx_status_t status;
> + struct semaphore fops_lock;
> +
> + struct se_shared_mem_mgmt_info se_shared_mem_mgmt;
> + struct notifier_block se_notify;

se_notify is never used.

Sascha

--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |

Next message: Connor Abbott: "Re: [PATCH v3 04/10] drm/msm/A6xx: Implement preemption for A7XX targets"
Previous message: Andi Shyti: "Re: [PATCH v2 7/7] i2c: designware: Group all DesignWare drivers under a single option"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]