-----Original Message----- From: Sascha Hauer <s.hauer@xxxxxxxxxxxxxx> Sent: Monday, September 9, 2024 5:52 PM To: Pankaj Gupta <pankaj.gupta@xxxxxxx> Cc: Jonathan Corbet <corbet@xxxxxxx>; Rob Herring <robh@xxxxxxxxxx>; Krzysztof Kozlowski <krzk+dt@xxxxxxxxxx>; Conor Dooley <conor+dt@xxxxxxxxxx>; Shawn Guo <shawnguo@xxxxxxxxxx>; Pengutronix Kernel Team <kernel@xxxxxxxxxxxxxx>; Fabio Estevam <festevam@xxxxxxxxx>; Rob Herring <robh+dt@xxxxxxxxxx>; linux-doc@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; imx@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx Subject: [EXT] Re: [PATCH v7 5/5] firmware: imx: adds miscdev 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 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://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgith > ub.com%2Fnxp-imx%2Fimx-secure-enclave.git&data=05%7C02%7Cpankaj.gupta% > 40nxp.com%7Ca61c8211ef7548dfde0008dcd0ca0bec%7C686ea1d3bc2b4c6fa92cd99 > c5c301635%7C0%7C0%7C638614813421798949%7CUnknown%7CTWFpbGZsb3d8eyJWIjo > iMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C% > 7C&sdata=DQ2%2Fv%2BuCZRlBD4xHiKwGOnicwv84g6rXZ2j%2BjMolt%2FE%3D&reserv > ed=0, > - i.MX Secure Middle-Ware: > -- URL: > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgith > ub.com%2Fnxp-imx%2Fimx-smw.git&data=05%7C02%7Cpankaj.gupta%40nxp.com%7 > Ca61c8211ef7548dfde0008dcd0ca0bec%7C686ea1d3bc2b4c6fa92cd99c5c301635%7 > C0%7C0%7C638614813421814882%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMD > AiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=TL > 3gmQWdzNkseInz3DQ%2B4eOO1iMpgD8QYIldXIaKIgA%3D&reserved=0 > > 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. [PG] There is no new file introduced in the whole patch-set series. > It seems you have messed up while rebasing. Please cleanup. [PG] Instead of " struct se_if_priv *priv" in 4/5, I have used " struct se_if_device_ctx *dev_ctx", to have Device-name printed. > +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. Ok will remove this in V8. > + > + 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. Ok. Will correct this at every places in V8. > +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. [PG] Like to maintain the count of active dev_context. This will be later useful for debugging and in case of suspend/resume scenarios. > + 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. Ok. Accepted will correct in V8. > +{ > + 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. Accepted. Will correct in V8. > +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. Will correct this in V8. > + 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. [PG] In case it got wrap around to zero, then to avoid it open a device with name appended with zero, hence incrementing 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. [PG] Agree. Will fix it in v8. > + 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. It is not accessed by multiple task. It is slave to the FW. The "cmd_receiver_clbk_hdl.dev_ctx" can be only one per 'priv'. This is accessed by Callback function only, to receive any command from FW. Not more than one such device, allowed to be opened. > + 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. Accepted will correct in V8. > + } > + > + 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. Like to keep the active_devctx_count, for debugging the stale dev_ctx in the system. Will replace the dev_err with dev_dbg. > + > + 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. Will remove it in V8. > + 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. Accepted. Will change in V8. > +/* 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. This will be used in next platform/feature addition. For now, it is not needed. Accepted, will remove this in V8. Sascha -- Pengutronix e.K. | | Steuerwalder Str. 21 | https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.pengutr onix.de%2F&data=05%7C02%7Cpankaj.gupta%40nxp.com%7Ca61c8211ef7548dfde0008dcd 0ca0bec%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638614813421828163%7CUn known%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJX VCI6Mn0%3D%7C0%7C%7C%7C&sdata=9%2BsmzPhaxGzr3vn1pD5V81XEqxY%2BEz%2FfFFZugiHY %2B08%3D&reserved=0 | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Attachment:
smime.p7s
Description: S/MIME cryptographic signature
