| Message ID | 20240722-imx-se-if-v6-5-ee26a87b824a@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | v6: firmware: imx: driver for NXP secure-enclave | expand |
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
> -----Original Message----- > From: Amit Singh Tomar <amitsinght@marvell.com> > Sent: Monday, July 22, 2024 5:08 PM > To: Pankaj Gupta <pankaj.gupta@nxp.com>; Jonathan Corbet > <corbet@lwn.net>; Rob Herring <robh@kernel.org>; Krzysztof Kozlowski > <krzk+dt@kernel.org>; Conor Dooley <conor+dt@kernel.org>; Shawn Guo > <shawnguo@kernel.org>; Sascha Hauer <s.hauer@pengutronix.de>; > Pengutronix Kernel Team <kernel@pengutronix.de>; Fabio Estevam > <festevam@gmail.com>; Rob Herring <robh+dt@kernel.org> > Cc: linux-doc@vger.kernel.org; linux-kernel@vger.kernel.org; > devicetree@vger.kernel.org; imx@lists.linux.dev; linux-arm- > kernel@lists.infradead.org > Subject: [EXT] Re: [EXTERNAL] [PATCH v6 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 > > > 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://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2F > > urldefense.proofpoint.com%2Fv2%2Furl%3Fu%3Dhttps- > 3A__github.com_nxp-2D > > imx_imx-2Dsecure- > 2Denclave.git%26d%3DDwICaQ%26c%3DnKjWec2b6R0mOyPaz7xt > > fQ%26r%3DV_GK7jRuCHDErm6txmgDK1-MbUihtnSQ3gPgB-A- > JKU%26m%3Dhqz6ztDhob0 > > jujItfaaf7PHhtqSHj4aoWie1-b4nAGXTUrSyBQtV9W- > WqLaM_utz%26s%3DC67hc24yMA > > > TzUglvGvywzpn0Efjurb6sOLm2V_9VpsI%26e%3D&data=05%7C02%7Cpanka > j.gupta%4 > > > 0nxp.com%7C5acd31c1cfc14661861c08dcaa42c232%7C686ea1d3bc2b4c6f > a92cd99c > > > 5c301635%7C0%7C0%7C638572450920546605%7CUnknown%7CTWFpbG > Zsb3d8eyJWIjoi > > > MC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0 > %7C%7C%7 > > > C&sdata=cV67vBSDb5uPaABT8RDTmtNOtqePRAALqo7QuUaV4QQ%3D&rese > rved=0 > > <https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Furl > > defense.proofpoint.com%2Fv2%2Furl%3Fu%3Dhttps- > 3A__github.com_nxp-2Dimx > > _imx-2Dsecure- > 2Denclave.git%26d%3DDwICaQ%26c%3DnKjWec2b6R0mOyPaz7xtfQ% > > 26r%3DV_GK7jRuCHDErm6txmgDK1-MbUihtnSQ3gPgB-A- > JKU%26m%3Dhqz6ztDhob0juj > > Itfaaf7PHhtqSHj4aoWie1-b4nAGXTUrSyBQtV9W- > WqLaM_utz%26s%3DC67hc24yMATzU > > > glvGvywzpn0Efjurb6sOLm2V_9VpsI%26e%3D&data=05%7C02%7Cpankaj.gu > pta%40nx > > > p.com%7C5acd31c1cfc14661861c08dcaa42c232%7C686ea1d3bc2b4c6fa92 > cd99c5c3 > > > 01635%7C0%7C0%7C638572450920558539%7CUnknown%7CTWFpbGZsb > 3d8eyJWIjoiMC4 > > > wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C > %7C%7C&s > > > data=yrSEmLSlxZrIZG%2Bk4J%2BbDyvzdEams5ux%2F8nKhQBLq74%3D&rese > rved=0>, > > - i.MX Secure Middle-Ware: > > -- > > > URL:https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2F > > urldefense.proofpoint.com%2Fv2%2Furl%3Fu%3Dhttps- > 3A__github.com_nxp-2D > > imx_imx- > 2Dsmw.git%26d%3DDwICaQ%26c%3DnKjWec2b6R0mOyPaz7xtfQ%26r%3D > V_GK > > 7jRuCHDErm6txmgDK1-MbUihtnSQ3gPgB-A- > JKU%26m%3Dhqz6ztDhob0jujItfaaf7PHh > > tqSHj4aoWie1-b4nAGXTUrSyBQtV9W- > WqLaM_utz%26s%3DNACAFfnEzGKFI7FlqdL4kxl > > > t8PtxeXRorc3IWanqgtY%26e%3D&data=05%7C02%7Cpankaj.gupta%40nxp. > com%7C5a > > > cd31c1cfc14661861c08dcaa42c232%7C686ea1d3bc2b4c6fa92cd99c5c3016 > 35%7C0% > > > 7C0%7C638572450920566218%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiM > C4wLjAwMDAiL > > > CJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdat > a=vuTve > > lCrdOFlGPGGpwpx0YgA6So%2BRIPJQRSzOjfo2LM%3D&reserved=0 > > <https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Furl > > defense.proofpoint.com%2Fv2%2Furl%3Fu%3Dhttps- > 3A__github.com_nxp-2Dimx > > _imx- > 2Dsmw.git%26d%3DDwICaQ%26c%3DnKjWec2b6R0mOyPaz7xtfQ%26r%3D > V_GK7jR > > uCHDErm6txmgDK1-MbUihtnSQ3gPgB-A- > JKU%26m%3Dhqz6ztDhob0jujItfaaf7PHhtqS > > Hj4aoWie1-b4nAGXTUrSyBQtV9W- > WqLaM_utz%26s%3DNACAFfnEzGKFI7FlqdL4kxlt8P > > > txeXRorc3IWanqgtY%26e%3D&data=05%7C02%7Cpankaj.gupta%40nxp.co > m%7C5acd3 > > > 1c1cfc14661861c08dcaa42c232%7C686ea1d3bc2b4c6fa92cd99c5c301635 > %7C0%7C0 > > %7C638572450920572062%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC > 4wLjAwMDAiLCJQ > > > IjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=E > DDL3DFE > > erSUqrBRGQchaAsN3L0H2nkkRw4AsoNBMqA%3D&reserved=0> > > > > 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://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Furldef > ense.proofpoint.com%2Fv2%2Furl%3Fu%3Dhttps-3A__github.com_nxp- > 2Dimx_imx-2Dsecure- > 2Denclave.git%26d%3DDwICaQ%26c%3DnKjWec2b6R0mOyPaz7xtfQ%26r% > 3DV_GK7jRuCHDErm6txmgDK1-MbUihtnSQ3gPgB-A- > JKU%26m%3Dhqz6ztDhob0jujItfaaf7PHhtqSHj4aoWie1- > b4nAGXTUrSyBQtV9W- > WqLaM_utz%26s%3DC67hc24yMATzUglvGvywzpn0Efjurb6sOLm2V_9VpsI% > 26e%3D&data=05%7C02%7Cpankaj.gupta%40nxp.com%7C5acd31c1cfc146 > 61861c08dcaa42c232%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C > 0%7C638572450920577297%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4 > wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C > %7C%7C&sdata=FvSTP3MVF%2BSghK36fmJ8u%2FySJ80DP7VQjNYAytj9gws > %3D&reserved=0 > <https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Furld > efense.proofpoint.com%2Fv2%2Furl%3Fu%3Dhttps-3A__github.com_nxp- > 2Dimx_imx-2Dsecure- > 2Denclave.git%26d%3DDwICaQ%26c%3DnKjWec2b6R0mOyPaz7xtfQ%26r% > 3DV_GK7jRuCHDErm6txmgDK1-MbUihtnSQ3gPgB-A- > JKU%26m%3Dhqz6ztDhob0jujItfaaf7PHhtqSHj4aoWie1- > b4nAGXTUrSyBQtV9W- > WqLaM_utz%26s%3DC67hc24yMATzUglvGvywzpn0Efjurb6sOLm2V_9VpsI% > 26e%3D&data=05%7C02%7Cpankaj.gupta%40nxp.com%7C5acd31c1cfc146 > 61861c08dcaa42c232%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C > 0%7C638572450920582198%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4 > wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C > %7C%7C&sdata=%2Fb342eydDBBpn451JY9h36udCBWaJmzMbMOQcJHWI% > 2BQ%3D&reserved=0>, > > + > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Furldef > ense.proofpoint.com%2Fv2%2Furl%3Fu%3Dhttps-3A__github.com_nxp- > 2Dimx_imx- > 2Dsmw.git%26d%3DDwICaQ%26c%3DnKjWec2b6R0mOyPaz7xtfQ%26r%3D > V_GK7jRuCHDErm6txmgDK1-MbUihtnSQ3gPgB-A- > JKU%26m%3Dhqz6ztDhob0jujItfaaf7PHhtqSHj4aoWie1- > b4nAGXTUrSyBQtV9W- > WqLaM_utz%26s%3DNACAFfnEzGKFI7FlqdL4kxlt8PtxeXRorc3IWanqgtY%26e > %3D&data=05%7C02%7Cpankaj.gupta%40nxp.com%7C5acd31c1cfc146618 > 61c08dcaa42c232%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7 > C638572450920586967%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLj > AwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C > %7C&sdata=sXtVRno2qOn6aGvIr2HpJrB0WbhexELpRMNQE8JPUxY%3D&res > erved=0 > <https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Furld > efense.proofpoint.com%2Fv2%2Furl%3Fu%3Dhttps-3A__github.com_nxp- > 2Dimx_imx- > 2Dsmw.git%26d%3DDwICaQ%26c%3DnKjWec2b6R0mOyPaz7xtfQ%26r%3D > V_GK7jRuCHDErm6txmgDK1-MbUihtnSQ3gPgB-A- > JKU%26m%3Dhqz6ztDhob0jujItfaaf7PHhtqSHj4aoWie1- > b4nAGXTUrSyBQtV9W- > WqLaM_utz%26s%3DNACAFfnEzGKFI7FlqdL4kxlt8PtxeXRorc3IWanqgtY%26e > %3D&data=05%7C02%7Cpankaj.gupta%40nxp.com%7C5acd31c1cfc146618 > 61c08dcaa42c232%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7 > C638572450920591699%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLj > AwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C > %7C&sdata=XVI6X7w5FBckyuDIklwBvWTvYQ69GJPvR%2FnIvgyWulU%3D&r > eserved=0> > > + 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(); > } Checkpatch.pl donot throw either warning or error, for this. Adding the braces to else, do not throw error or warning, either. Thus, will add this change. > > Also, made a similar comment on the earlier version (v5) as well: > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatch > work.kernel.org%2Fproject%2Fimx%2Fpatch%2F20240712-imx-se-if-v5-4- > 66a79903a872%40nxp.com%2F&data=05%7C02%7Cpankaj.gupta%40nxp.c > om%7C5acd31c1cfc14661861c08dcaa42c232%7C686ea1d3bc2b4c6fa92cd > 99c5c301635%7C0%7C0%7C638572450920596406%7CUnknown%7CTWF > pbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJX > VCI6Mn0%3D%7C0%7C%7C%7C&sdata=nnkWdELMd%2BaYqAcqFoeTaSOib > VnSoMBegxXsn8PMePc%3D&reserved=0 > > Thanks > -Amit >
Hi Pankaj, On Mon, Jul 22, 2024 at 10:21:40AM +0530, Pankaj Gupta wrote: > +static int se_ioctl_cmd_snd_rcv_rsp_handler(struct se_if_device_ctx *dev_ctx, > + u64 arg) > +{ > + struct se_if_priv *priv = dev_get_drvdata(dev_ctx->dev); > + 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 (copy_from_user(&cmd_snd_rcv_rsp_info, (u8 *)arg, > + sizeof(cmd_snd_rcv_rsp_info))) { > + dev_err(dev_ctx->priv->dev, > + "%s: Failed to copy cmd_snd_rcv_rsp_info from user\n", > + dev_ctx->miscdev.name); > + err = -EFAULT; > + goto exit; > + } > + > + if (cmd_snd_rcv_rsp_info.tx_buf_sz < SE_MU_HDR_SZ) { > + dev_err(dev_ctx->priv->dev, > + "%s: User buffer too small(%d < %d)\n", > + dev_ctx->miscdev.name, > + cmd_snd_rcv_rsp_info.tx_buf_sz, > + SE_MU_HDR_SZ); > + err = -ENOSPC; > + goto exit; > + } > + > + rx_msg = kzalloc(cmd_snd_rcv_rsp_info.rx_buf_sz, GFP_KERNEL); > + if (!rx_msg) { > + err = -ENOMEM; > + goto exit; > + } > + > + tx_msg = memdup_user(cmd_snd_rcv_rsp_info.tx_buf, > + cmd_snd_rcv_rsp_info.tx_buf_sz); > + if (IS_ERR(tx_msg)) { > + err = PTR_ERR(tx_msg); > + goto exit; > + } > + > + if (tx_msg->header.tag != priv->cmd_tag) { > + err = -EINVAL; > + goto exit; > + } > + > + guard(mutex)(&priv->se_if_cmd_lock); > + priv->waiting_rsp_dev = dev_ctx; > + dev_ctx->temp_resp_size = cmd_snd_rcv_rsp_info.rx_buf_sz; > + > + /* Device Context that is assigned to be a > + * FW's command receiver, has pre-allocated buffer. > + */ > + if (dev_ctx != priv->cmd_receiver_dev) > + dev_ctx->temp_resp = rx_msg; > + > + err = ele_miscdev_msg_send(dev_ctx, > + tx_msg, > + cmd_snd_rcv_rsp_info.tx_buf_sz); > + if (err < 0) > + goto exit; > + > + cmd_snd_rcv_rsp_info.tx_buf_sz = err; > + > + err = ele_miscdev_msg_rcv(dev_ctx, > + cmd_snd_rcv_rsp_info.rx_buf, > + cmd_snd_rcv_rsp_info.rx_buf_sz); Ok, here you now have serialized sending and receiving messages, With this you no longer need priv->waiting_rsp_dev, dev_ctx->temp_resp and dev_ctx->temp_resp_size. Drop these for further cleanup. > +} > + > +static int se_ioctl_get_mu_info(struct se_if_device_ctx *dev_ctx, > + u64 arg) > +{ > + struct se_if_priv *priv = dev_get_drvdata(dev_ctx->dev); > + struct se_if_node_info *if_node_info; > + struct se_ioctl_get_if_info info; > + int err = 0; > + > + if_node_info = (struct se_if_node_info *)priv->info; > + > + info.se_if_id = if_node_info->se_if_id; > + info.interrupt_idx = 0; > + info.tz = 0; > + info.did = if_node_info->se_if_did; > + info.cmd_tag = if_node_info->cmd_tag; > + info.rsp_tag = if_node_info->rsp_tag; > + info.success_tag = if_node_info->success_tag; > + info.base_api_ver = if_node_info->base_api_ver; > + info.fw_api_ver = if_node_info->fw_api_ver; This really shouldn't be here. You pass cmd_tag and rsp_tag to userspace just to guide userspace how to construct a message. This shows that the messages should be constructed in the Kernel rather than in userspace. Just pass the message content from userspace to the kernel and let the kernel build the message on the sender side. > +/* IOCTL entry point of a character device */ > +static long se_ioctl(struct file *fp, unsigned int cmd, unsigned long arg) > +{ > + struct se_if_device_ctx *dev_ctx = container_of(fp->private_data, > + struct se_if_device_ctx, > + miscdev); > + struct se_if_priv *se_if_priv = dev_ctx->priv; > + int err = -EINVAL; > + > + /* Prevent race during change of device context */ > + if (down_interruptible(&dev_ctx->fops_lock)) > + return -EBUSY; > + > + switch (cmd) { > + case SE_IOCTL_ENABLE_CMD_RCV: > + if (!se_if_priv->cmd_receiver_dev) { > + err = 0; > + se_if_priv->cmd_receiver_dev = dev_ctx; > + dev_ctx->temp_resp = kzalloc(MAX_NVM_MSG_LEN, GFP_KERNEL); > + if (!dev_ctx->temp_resp) > + err = -ENOMEM; > + } cmd_receiver_dev isn't locked by anything, still it can be accessed by different userspace processes. Besides, when already another instance is configured for receiving commands I would expect an -EBUSY here instead of silently ignoring the ioctl. > + break; > + case SE_IOCTL_GET_MU_INFO: > + err = se_ioctl_get_mu_info(dev_ctx, arg); > + break; > + case SE_IOCTL_SETUP_IOBUF: > + err = se_ioctl_setup_iobuf_handler(dev_ctx, arg); > + break; > + case SE_IOCTL_GET_SOC_INFO: > + err = se_ioctl_get_se_soc_info_handler(dev_ctx, arg); > + break; > + case SE_IOCTL_CMD_SEND_RCV_RSP: > + err = se_ioctl_cmd_snd_rcv_rsp_handler(dev_ctx, arg); > + break; > + > + default: > + err = -EINVAL; > + dev_dbg(se_if_priv->dev, > + "%s: IOCTL %.8x not supported\n", > + dev_ctx->miscdev.name, > + cmd); > + } > + > + up(&dev_ctx->fops_lock); > + return (long)err; > +} > + ... > +static int init_device_context(struct se_if_priv *priv) > +{ > + const struct se_if_node_info *info = priv->info; > + struct se_if_device_ctx *dev_ctx; > + u8 *devname; > + int ret = 0; > + int i; > + > + priv->ctxs = devm_kzalloc(priv->dev, sizeof(dev_ctx) * priv->max_dev_ctx, > + GFP_KERNEL); > + > + if (!priv->ctxs) { > + ret = -ENOMEM; > + return ret; > + } > + > + /* Create users */ > + for (i = 0; i < priv->max_dev_ctx; i++) { > + dev_ctx = devm_kzalloc(priv->dev, sizeof(*dev_ctx), GFP_KERNEL); > + if (!dev_ctx) { > + ret = -ENOMEM; > + return ret; > + } > + > + dev_ctx->dev = priv->dev; > + dev_ctx->status = SE_IF_CTX_FREE; > + dev_ctx->priv = priv; > + > + priv->ctxs[i] = dev_ctx; > + > + /* Default value invalid for an header. */ > + init_waitqueue_head(&dev_ctx->wq); > + > + INIT_LIST_HEAD(&dev_ctx->pending_out); > + INIT_LIST_HEAD(&dev_ctx->pending_in); > + sema_init(&dev_ctx->fops_lock, 1); > + > + devname = devm_kasprintf(priv->dev, GFP_KERNEL, "%s_ch%d", > + info->se_name, i); > + if (!devname) { > + ret = -ENOMEM; > + return ret; > + } > + > + dev_ctx->miscdev.name = devname; > + dev_ctx->miscdev.minor = MISC_DYNAMIC_MINOR; > + dev_ctx->miscdev.fops = &se_if_fops; > + dev_ctx->miscdev.parent = priv->dev; > + ret = misc_register(&dev_ctx->miscdev); > + if (ret) { > + dev_err(priv->dev, "failed to register misc device %d\n", > + ret); > + return ret; > + } Here you register four character devices which all allow a single open. There's no need to artificially limit the number of users. Just register a single character device, allow it to be opened multiple times and allocate the instance specific context as necessary in se_if_fops_open(). Sascha
> -----Original Message----- > From: Sascha Hauer <s.hauer@pengutronix.de> > Sent: Tuesday, July 23, 2024 7:51 PM > To: Pankaj Gupta <pankaj.gupta@nxp.com> > Cc: Jonathan Corbet <corbet@lwn.net>; Rob Herring <robh@kernel.org>; > Krzysztof Kozlowski <krzk+dt@kernel.org>; Conor Dooley > <conor+dt@kernel.org>; Shawn Guo <shawnguo@kernel.org>; Pengutronix > Kernel Team <kernel@pengutronix.de>; Fabio Estevam > <festevam@gmail.com>; Rob Herring <robh+dt@kernel.org>; linux- > doc@vger.kernel.org; linux-kernel@vger.kernel.org; > devicetree@vger.kernel.org; imx@lists.linux.dev; linux-arm- > kernel@lists.infradead.org > Subject: [EXT] Re: [PATCH v6 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 > > > Hi Pankaj, > > On Mon, Jul 22, 2024 at 10:21:40AM +0530, Pankaj Gupta wrote: > > +static int se_ioctl_cmd_snd_rcv_rsp_handler(struct se_if_device_ctx > *dev_ctx, > > + u64 arg) { > > + struct se_if_priv *priv = dev_get_drvdata(dev_ctx->dev); > > + 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 (copy_from_user(&cmd_snd_rcv_rsp_info, (u8 *)arg, > > + sizeof(cmd_snd_rcv_rsp_info))) { > > + dev_err(dev_ctx->priv->dev, > > + "%s: Failed to copy cmd_snd_rcv_rsp_info from user\n", > > + dev_ctx->miscdev.name); > > + err = -EFAULT; > > + goto exit; > > + } > > + > > + if (cmd_snd_rcv_rsp_info.tx_buf_sz < SE_MU_HDR_SZ) { > > + dev_err(dev_ctx->priv->dev, > > + "%s: User buffer too small(%d < %d)\n", > > + dev_ctx->miscdev.name, > > + cmd_snd_rcv_rsp_info.tx_buf_sz, > > + SE_MU_HDR_SZ); > > + err = -ENOSPC; > > + goto exit; > > + } > > + > > + rx_msg = kzalloc(cmd_snd_rcv_rsp_info.rx_buf_sz, GFP_KERNEL); > > + if (!rx_msg) { > > + err = -ENOMEM; > > + goto exit; > > + } > > + > > + tx_msg = memdup_user(cmd_snd_rcv_rsp_info.tx_buf, > > + cmd_snd_rcv_rsp_info.tx_buf_sz); > > + if (IS_ERR(tx_msg)) { > > + err = PTR_ERR(tx_msg); > > + goto exit; > > + } > > + > > + if (tx_msg->header.tag != priv->cmd_tag) { > > + err = -EINVAL; > > + goto exit; > > + } > > + > > + guard(mutex)(&priv->se_if_cmd_lock); > > + priv->waiting_rsp_dev = dev_ctx; > > + dev_ctx->temp_resp_size = cmd_snd_rcv_rsp_info.rx_buf_sz; > > + > > + /* Device Context that is assigned to be a > > + * FW's command receiver, has pre-allocated buffer. > > + */ > > + if (dev_ctx != priv->cmd_receiver_dev) > > + dev_ctx->temp_resp = rx_msg; > > + > > + err = ele_miscdev_msg_send(dev_ctx, > > + tx_msg, > > + cmd_snd_rcv_rsp_info.tx_buf_sz); > > + if (err < 0) > > + goto exit; > > + > > + cmd_snd_rcv_rsp_info.tx_buf_sz = err; > > + > > + err = ele_miscdev_msg_rcv(dev_ctx, > > + cmd_snd_rcv_rsp_info.rx_buf, > > + cmd_snd_rcv_rsp_info.rx_buf_sz); > > Ok, here you now have serialized sending and receiving messages, > > With this you no longer need priv->waiting_rsp_dev, dev_ctx->temp_resp and > dev_ctx->temp_resp_size. Drop these for further cleanup. It is very much needed. - priv->waiting_rsp_dev, help identify in the callback function that: - the message is targeted for dev_ctx(user space) or dev(kernel space). - the message is targeted for for which dev_ctx. - dev_ctx->temp_resp, this buffer pointer is needed, to receive the message received in call back. - dev_ctx->temp_resp_size, is needed to compare the size of in-coming message. All the three are needed in callback function. > > > +} > > + > > +static int se_ioctl_get_mu_info(struct se_if_device_ctx *dev_ctx, > > + u64 arg) { > > + struct se_if_priv *priv = dev_get_drvdata(dev_ctx->dev); > > + struct se_if_node_info *if_node_info; > > + struct se_ioctl_get_if_info info; > > + int err = 0; > > + > > + if_node_info = (struct se_if_node_info *)priv->info; > > + > > + info.se_if_id = if_node_info->se_if_id; > > + info.interrupt_idx = 0; > > + info.tz = 0; > > + info.did = if_node_info->se_if_did; > > + info.cmd_tag = if_node_info->cmd_tag; > > + info.rsp_tag = if_node_info->rsp_tag; > > + info.success_tag = if_node_info->success_tag; > > + info.base_api_ver = if_node_info->base_api_ver; > > + info.fw_api_ver = if_node_info->fw_api_ver; > > This really shouldn't be here. You pass cmd_tag and rsp_tag to userspace just > to guide userspace how to construct a message. > > This shows that the messages should be constructed in the Kernel rather than > in userspace. Just pass the message content from userspace to the kernel and > let the kernel build the message on the sender side. This will help collecting user-space application logs, with correct tags. This is already used by the customers, for debug. > > > +/* IOCTL entry point of a character device */ static long > > +se_ioctl(struct file *fp, unsigned int cmd, unsigned long arg) { > > + struct se_if_device_ctx *dev_ctx = container_of(fp->private_data, > > + struct se_if_device_ctx, > > + miscdev); > > + struct se_if_priv *se_if_priv = dev_ctx->priv; > > + int err = -EINVAL; > > + > > + /* Prevent race during change of device context */ > > + if (down_interruptible(&dev_ctx->fops_lock)) > > + return -EBUSY; > > + > > + switch (cmd) { > > + case SE_IOCTL_ENABLE_CMD_RCV: > > + if (!se_if_priv->cmd_receiver_dev) { > > + err = 0; > > + se_if_priv->cmd_receiver_dev = dev_ctx; > > + dev_ctx->temp_resp = kzalloc(MAX_NVM_MSG_LEN, > GFP_KERNEL); > > + if (!dev_ctx->temp_resp) > > + err = -ENOMEM; > > + } > > cmd_receiver_dev isn't locked by anything, still it can be accessed by different > userspace processes. It is not accessed by different Userspace processes. It is a slave to FW. FW interacts with it when FW receive a command to do any action, from userspace. Hence, it will be executed under command-lock. > > Besides, when already another instance is configured for receiving commands I > would expect an -EBUSY here instead of silently ignoring the ioctl. Ok. Accepted. > > > + break; > > + case SE_IOCTL_GET_MU_INFO: > > + err = se_ioctl_get_mu_info(dev_ctx, arg); > > + break; > > + case SE_IOCTL_SETUP_IOBUF: > > + err = se_ioctl_setup_iobuf_handler(dev_ctx, arg); > > + break; > > + case SE_IOCTL_GET_SOC_INFO: > > + err = se_ioctl_get_se_soc_info_handler(dev_ctx, arg); > > + break; > > + case SE_IOCTL_CMD_SEND_RCV_RSP: > > + err = se_ioctl_cmd_snd_rcv_rsp_handler(dev_ctx, arg); > > + break; > > + > > + default: > > + err = -EINVAL; > > + dev_dbg(se_if_priv->dev, > > + "%s: IOCTL %.8x not supported\n", > > + dev_ctx->miscdev.name, > > + cmd); > > + } > > + > > + up(&dev_ctx->fops_lock); > > + return (long)err; > > +} > > + > > ... > > > +static int init_device_context(struct se_if_priv *priv) { > > + const struct se_if_node_info *info = priv->info; > > + struct se_if_device_ctx *dev_ctx; > > + u8 *devname; > > + int ret = 0; > > + int i; > > + > > + priv->ctxs = devm_kzalloc(priv->dev, sizeof(dev_ctx) * priv- > >max_dev_ctx, > > + GFP_KERNEL); > > + > > + if (!priv->ctxs) { > > + ret = -ENOMEM; > > + return ret; > > + } > > + > > + /* Create users */ > > + for (i = 0; i < priv->max_dev_ctx; i++) { > > + dev_ctx = devm_kzalloc(priv->dev, sizeof(*dev_ctx), GFP_KERNEL); > > + if (!dev_ctx) { > > + ret = -ENOMEM; > > + return ret; > > + } > > + > > + dev_ctx->dev = priv->dev; > > + dev_ctx->status = SE_IF_CTX_FREE; > > + dev_ctx->priv = priv; > > + > > + priv->ctxs[i] = dev_ctx; > > + > > + /* Default value invalid for an header. */ > > + init_waitqueue_head(&dev_ctx->wq); > > + > > + INIT_LIST_HEAD(&dev_ctx->pending_out); > > + INIT_LIST_HEAD(&dev_ctx->pending_in); > > + sema_init(&dev_ctx->fops_lock, 1); > > + > > + devname = devm_kasprintf(priv->dev, GFP_KERNEL, "%s_ch%d", > > + info->se_name, i); > > + if (!devname) { > > + ret = -ENOMEM; > > + return ret; > > + } > > + > > + dev_ctx->miscdev.name = devname; > > + dev_ctx->miscdev.minor = MISC_DYNAMIC_MINOR; > > + dev_ctx->miscdev.fops = &se_if_fops; > > + dev_ctx->miscdev.parent = priv->dev; > > + ret = misc_register(&dev_ctx->miscdev); > > + if (ret) { > > + dev_err(priv->dev, "failed to register misc device %d\n", > > + ret); > > + return ret; > > + } > > Here you register four character devices which all allow a single open. > > There's no need to artificially limit the number of users. Just register a single > character device, allow it to be opened multiple times and allocate the instance > specific context as necessary in se_if_fops_open(). Accepted. > > Sascha > > -- > Pengutronix e.K. | | > Steuerwalder Str. 21 | > https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww. > pengutronix.de%2F&data=05%7C02%7Cpankaj.gupta%40nxp.com%7C5bb7 > 0a0c3bcb437e2e3808dcab229d77%7C686ea1d3bc2b4c6fa92cd99c5c3016 > 35%7C0%7C0%7C638573412358069325%7CUnknown%7CTWFpbGZsb3d8 > eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3 > D%7C0%7C%7C%7C&sdata=97GKp2ydNvQz0oOwGp0dM3eez3L8IAE1sOqC > 3bhAxd8%3D&reserved=0 | > 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On Thu, Aug 08, 2024 at 10:49:33AM +0000, Pankaj Gupta wrote: > > > + if (tx_msg->header.tag != priv->cmd_tag) { > > > + err = -EINVAL; > > > + goto exit; > > > + } > > > + > > > + guard(mutex)(&priv->se_if_cmd_lock); > > > + priv->waiting_rsp_dev = dev_ctx; > > > + dev_ctx->temp_resp_size = cmd_snd_rcv_rsp_info.rx_buf_sz; > > > + > > > + /* Device Context that is assigned to be a > > > + * FW's command receiver, has pre-allocated buffer. > > > + */ > > > + if (dev_ctx != priv->cmd_receiver_dev) > > > + dev_ctx->temp_resp = rx_msg; > > > + > > > + err = ele_miscdev_msg_send(dev_ctx, > > > + tx_msg, > > > + cmd_snd_rcv_rsp_info.tx_buf_sz); > > > + if (err < 0) > > > + goto exit; > > > + > > > + cmd_snd_rcv_rsp_info.tx_buf_sz = err; > > > + > > > + err = ele_miscdev_msg_rcv(dev_ctx, > > > + cmd_snd_rcv_rsp_info.rx_buf, > > > + cmd_snd_rcv_rsp_info.rx_buf_sz); > > > > Ok, here you now have serialized sending and receiving messages, > > > > With this you no longer need priv->waiting_rsp_dev, dev_ctx->temp_resp and > > dev_ctx->temp_resp_size. Drop these for further cleanup. > > It is very much needed. > - priv->waiting_rsp_dev, help identify in the callback function that: > - the message is targeted for dev_ctx(user space) or dev(kernel space). > - the message is targeted for for which dev_ctx. > - dev_ctx->temp_resp, this buffer pointer is needed, to receive the message received in call back. > - dev_ctx->temp_resp_size, is needed to compare the size of in-coming message. > > All the three are needed in callback function. I think you should throw away ele_miscdev_msg_send() and ele_miscdev_msg_rcv() and instead use ele_msg_send_rcv() instead. This driver contains a whole lot of unneeded complexity up to the point where it's not clear what this driver is actually trying to archieve. Please let's do a step back and try to find out the actual usecases. What I have found out so far is: 1) We can send one message to the ELE and each message is expected to get one response from the ELE. 2) We are not allowed to send another message to the ELE while there is a message in flight that hasn't got a response. 3) Both Kernel and userspace shall be able to send commands and receive its responses. 4) The ELE is able to send a command itself. Is this true? Does this command need a response? Can we continue sending commands to the ELE while the ELE waits for the response to the command? 1) and 2) is covered by ele_msg_send_rcv(). 3) is covered by ele_msg_send_rcv() as well, it can be called directly by kernel code or via an ioctl from userspace. 4) is the most unclear point for me, but 1) 2) and 3) seems straight forward and should be solvable with significantly reduced code size. Am I missing any features that you need as well? > > > > > > +} > > > + > > > +static int se_ioctl_get_mu_info(struct se_if_device_ctx *dev_ctx, > > > + u64 arg) { > > > + struct se_if_priv *priv = dev_get_drvdata(dev_ctx->dev); > > > + struct se_if_node_info *if_node_info; > > > + struct se_ioctl_get_if_info info; > > > + int err = 0; > > > + > > > + if_node_info = (struct se_if_node_info *)priv->info; > > > + > > > + info.se_if_id = if_node_info->se_if_id; > > > + info.interrupt_idx = 0; > > > + info.tz = 0; > > > + info.did = if_node_info->se_if_did; > > > + info.cmd_tag = if_node_info->cmd_tag; > > > + info.rsp_tag = if_node_info->rsp_tag; > > > + info.success_tag = if_node_info->success_tag; > > > + info.base_api_ver = if_node_info->base_api_ver; > > > + info.fw_api_ver = if_node_info->fw_api_ver; > > > > This really shouldn't be here. You pass cmd_tag and rsp_tag to userspace just > > to guide userspace how to construct a message. > > > > This shows that the messages should be constructed in the Kernel rather than > > in userspace. Just pass the message content from userspace to the kernel and > > let the kernel build the message on the sender side. > > This will help collecting user-space application logs, with correct tags. > This is already used by the customers, for debug. I don't bother that you provide this information to userspace. My point is that it shouldn't be needed by userspace to assemble the packets that are sent back to the kernel. Really the packet encapsulation should be done in the kernel and userspace shouldn't be bothered with it. > > > > > > +/* IOCTL entry point of a character device */ static long > > > +se_ioctl(struct file *fp, unsigned int cmd, unsigned long arg) { > > > + struct se_if_device_ctx *dev_ctx = container_of(fp->private_data, > > > + struct se_if_device_ctx, > > > + miscdev); > > > + struct se_if_priv *se_if_priv = dev_ctx->priv; > > > + int err = -EINVAL; > > > + > > > + /* Prevent race during change of device context */ > > > + if (down_interruptible(&dev_ctx->fops_lock)) > > > + return -EBUSY; > > > + > > > + switch (cmd) { > > > + case SE_IOCTL_ENABLE_CMD_RCV: > > > + if (!se_if_priv->cmd_receiver_dev) { > > > + err = 0; > > > + se_if_priv->cmd_receiver_dev = dev_ctx; > > > + dev_ctx->temp_resp = kzalloc(MAX_NVM_MSG_LEN, > > GFP_KERNEL); > > > + if (!dev_ctx->temp_resp) > > > + err = -ENOMEM; > > > + } > > > > cmd_receiver_dev isn't locked by anything, still it can be accessed by different > > userspace processes. > > It is not accessed by different Userspace processes. It is a slave to FW. > FW interacts with it when FW receive a command to do any action, from userspace. > Hence, it will be executed under command-lock. When two userspace programs have a device instance open, then nothing prevents them from calling this ioctl at the same time. You do a if (!se_if_priv->cmd_receiver_dev) se_if_priv->cmd_receiver_dev = dev_ctx; which is executed by two threads simultaneously. It's one of the most classic scenarios that need locking. Sascha
Hi Sascha, Thanks for the detail review. Please find the comments in-line. In our last one to one discussion, there is a specific input to have a common function "ele_msg_send_rcv( prv, tx_msg, rx_msg)" for: - Userspace IOCTL - Kernel message exchange. As discussed, will try to get back to you with my analysis over it. Thanks. Pankaj > -----Original Message----- > From: Sascha Hauer <s.hauer@pengutronix.de> > Sent: Friday, August 9, 2024 12:40 PM > To: Pankaj Gupta <pankaj.gupta@nxp.com> > Cc: Jonathan Corbet <corbet@lwn.net>; Rob Herring <robh@kernel.org>; > Krzysztof Kozlowski <krzk+dt@kernel.org>; Conor Dooley > <conor+dt@kernel.org>; Shawn Guo <shawnguo@kernel.org>; Pengutronix > Kernel Team <kernel@pengutronix.de>; Fabio Estevam > <festevam@gmail.com>; Rob Herring <robh+dt@kernel.org>; linux- > doc@vger.kernel.org; linux-kernel@vger.kernel.org; > devicetree@vger.kernel.org; imx@lists.linux.dev; linux-arm- > kernel@lists.infradead.org > Subject: Re: [EXT] Re: [PATCH v6 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 Thu, Aug 08, 2024 at 10:49:33AM +0000, Pankaj Gupta wrote: > > > > + if (tx_msg->header.tag != priv->cmd_tag) { > > > > + err = -EINVAL; > > > > + goto exit; > > > > + } > > > > + > > > > + guard(mutex)(&priv->se_if_cmd_lock); > > > > + priv->waiting_rsp_dev = dev_ctx; > > > > + dev_ctx->temp_resp_size = cmd_snd_rcv_rsp_info.rx_buf_sz; > > > > + > > > > + /* Device Context that is assigned to be a > > > > + * FW's command receiver, has pre-allocated buffer. > > > > + */ > > > > + if (dev_ctx != priv->cmd_receiver_dev) > > > > + dev_ctx->temp_resp = rx_msg; > > > > + > > > > + err = ele_miscdev_msg_send(dev_ctx, > > > > + tx_msg, > > > > + cmd_snd_rcv_rsp_info.tx_buf_sz); > > > > + if (err < 0) > > > > + goto exit; > > > > + > > > > + cmd_snd_rcv_rsp_info.tx_buf_sz = err; > > > > + > > > > + err = ele_miscdev_msg_rcv(dev_ctx, > > > > + cmd_snd_rcv_rsp_info.rx_buf, > > > > + cmd_snd_rcv_rsp_info.rx_buf_sz); > > > > > > Ok, here you now have serialized sending and receiving messages, > > > > > > With this you no longer need priv->waiting_rsp_dev, > > > dev_ctx->temp_resp and dev_ctx->temp_resp_size. Drop these for > > > further > cleanup. > > > > It is very much needed. > > - priv->waiting_rsp_dev, help identify in the callback function that: > > - the message is targeted for dev_ctx(user space) or dev(kernel space). > > - the message is targeted for for which dev_ctx. > > - dev_ctx->temp_resp, this buffer pointer is needed, to receive the > > message > received in call back. > > - dev_ctx->temp_resp_size, is needed to compare the size of > > in-coming > message. > > > > All the three are needed in callback function. > > I think you should throw away ele_miscdev_msg_send() and > ele_miscdev_msg_rcv() and instead use ele_msg_send_rcv() instead. > Both the API(s): ele_miscdev_msg_send() & ele_miscdev_msg_rcv()are needed. - fops_read API, calls the ele_miscdev_msg_send(), & - fops_write API, calls the ele_miscdev_msg_rcv() > This driver contains a whole lot of unneeded complexity up to the > point where it's not clear what this driver is actually trying to archieve. > > Please let's do a step back and try to find out the actual usecases. > > What I have found out so far is: > > 1) We can send one message to the ELE and each message is expected to get > one response from the ELE. For each message, it is not as simple as to get one response, without any other message exchange. Why? - In order to deliver the response to that message, FW could be exchanging multiple message with its slave called NVM-daemon running at Userspace. - Once enough information is collected from its slave, to prepare the response. It will send the message response. > 2) We are not allowed to send another message to the ELE while there is a > message in flight that hasn't got a response. Here "We" means Userspace application sending the command message on a particular MU. In the case where ELE can receive message over two MU(s), another userspace application can send another command message, via different MU. Hence there can be two command message in flight at a time, via two different MU(s). > 3) Both Kernel and userspace shall be able to send commands and receive > its responses. Yes. > 4) The ELE is able to send a command itself. Is this true? No, rather ELE can send command to its slave, running as a NVM-daemon at userspace. > Does this command need a response? Yes. > Can we continue sending commands to the ELE > while the ELE waits for the response to the command? No. "We" (Application that acts as the command sender, over one MU), the mutex-command-lock is taken by the first command. Hence if "We" tries to send the second command, the lock is not freed to be acquired. At this state, ELE can send command to its slave and can wait for the response from its slave. After collecting information from its slave, ELE will prepare the response to the command sent by "We", to send the response. After the response is received by "We", the mutex-command-lock is freed. > > > 1) and 2) is covered by ele_msg_send_rcv(). 3) is covered by > ele_msg_send_rcv() as well, it can be called directly by kernel code > or via an ioctl from userspace. > > 4) is the most unclear point for me, but 1) 2) and 3) seems straight > forward and should be solvable with significantly reduced code size. > > Am I missing any features that you need as well? Yes. As explained above with each bullet. There is a ELE's slave-daemon running at the userspace, with which ELE exchange messages. Now, it must be clear why? Both the API(s): ele_miscdev_msg_send() & ele_miscdev_msg_rcv()are needed. > > > > > > > > > > > +} > > > > + > > > > +static int se_ioctl_get_mu_info(struct se_if_device_ctx *dev_ctx, > > > > + u64 arg) { > > > > + struct se_if_priv *priv = dev_get_drvdata(dev_ctx->dev); > > > > + struct se_if_node_info *if_node_info; > > > > + struct se_ioctl_get_if_info info; > > > > + int err = 0; > > > > + > > > > + if_node_info = (struct se_if_node_info *)priv->info; > > > > + > > > > + info.se_if_id = if_node_info->se_if_id; > > > > + info.interrupt_idx = 0; > > > > + info.tz = 0; > > > > + info.did = if_node_info->se_if_did; > > > > + info.cmd_tag = if_node_info->cmd_tag; > > > > + info.rsp_tag = if_node_info->rsp_tag; > > > > + info.success_tag = if_node_info->success_tag; > > > > + info.base_api_ver = if_node_info->base_api_ver; > > > > + info.fw_api_ver = if_node_info->fw_api_ver; > > > > > > This really shouldn't be here. You pass cmd_tag and rsp_tag to > > > userspace just to guide userspace how to construct a message. > > > > > > This shows that the messages should be constructed in the Kernel > > > rather than in userspace. Just pass the message content from > > > userspace to the kernel and let the kernel build the message on > > > the sender > side. > > > > This will help collecting user-space application logs, with correct tags. > > This is already used by the customers, for debug. > > I don't bother that you provide this information to userspace. My > point is that it shouldn't be needed by userspace to assemble the > packets that are sent back to the kernel. > > Really the packet encapsulation should be done in the kernel and > userspace shouldn't be bothered with it. Packet encapsulation cannot be removed from the userspace. Only, userspace knows that the current API, that is sent, belongs to which set of API(s): - Set of API(s) supported by FW code. - Set of API(s) supported by ROM Code. Only thing that can be saved is the encapsulating command tag for "We" and response tag for ELE's slave. > > > > > > > > > > +/* IOCTL entry point of a character device */ static long > > > > +se_ioctl(struct file *fp, unsigned int cmd, unsigned long arg) { > > > > + struct se_if_device_ctx *dev_ctx = container_of(fp->private_data, > > > > + struct se_if_device_ctx, > > > > + miscdev); > > > > + struct se_if_priv *se_if_priv = dev_ctx->priv; > > > > + int err = -EINVAL; > > > > + > > > > + /* Prevent race during change of device context */ > > > > + if (down_interruptible(&dev_ctx->fops_lock)) > > > > + return -EBUSY; > > > > + > > > > + switch (cmd) { > > > > + case SE_IOCTL_ENABLE_CMD_RCV: > > > > + if (!se_if_priv->cmd_receiver_dev) { > > > > + err = 0; > > > > + se_if_priv->cmd_receiver_dev = dev_ctx; > > > > + dev_ctx->temp_resp = > > > > + kzalloc(MAX_NVM_MSG_LEN, > > > GFP_KERNEL); > > > > + if (!dev_ctx->temp_resp) > > > > + err = -ENOMEM; > > > > + } > > > > > > cmd_receiver_dev isn't locked by anything, still it can be > > > accessed by different userspace processes. > > > > It is not accessed by different Userspace processes. It is a slave to FW. > > FW interacts with it when FW receive a command to do any action, > > from > userspace. > > Hence, it will be executed under command-lock. > > When two userspace programs have a device instance open, then nothing > prevents them from calling this ioctl at the same time. You do a > > if (!se_if_priv->cmd_receiver_dev) > se_if_priv->cmd_receiver_dev = dev_ctx; > > which is executed by two threads simultaneously. It's one of the most > classic scenarios that need locking. This IOCTL is not to be called by user-application. It is to be called by one application(called ELE's Slave NVM-Daemon) implemented as part of secure-enclave library code-base. This case will never be occurring. I will update the SE-DEV text file against this ioctl. > > Sascha > > -- > Pengutronix e.K. | | > Steuerwalder Str. 21 | > https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww. > pengutronix.de%2F&data=05%7C02%7Cpankaj.gupta%40nxp.com%7Ca46d > d4dd6a0542c2848608dcb842477e%7C686ea1d3bc2b4c6fa92cd99c5c3016 > 35%7C0%7C0%7C638587842010417199%7CUnknown%7CTWFpbGZsb3d8 > eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3 > D%7C0%7C%7C%7C&sdata=mslWc%2F%2Bp4PKtth3htkmdAJ0xHFh5MlCkcj > %2FKNw7Tg5U%3D&reserved=0 | > 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
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://github.com/nxp-imx/imx-secure-enclave.git, + https://github.com/nxp-imx/imx-smw.git + 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); + + /* NVM buffer size are not known prior receiving it from FW. + */ + dev_ctx->temp_resp_size = rx_msg_sz; + + /* Allow user to read */ + dev_ctx->pending_hdr = 1; + wake_up_interruptible(&dev_ctx->wq); + + return; + } else if (header->tag == priv->rsp_tag) { + if (priv->waiting_rsp_dev) { + dev_dbg(dev, "Selecting rsp waiter\n"); + dev_ctx = priv->waiting_rsp_dev; + if (rx_msg_sz != dev_ctx->temp_resp_size + && !exception_for_size(priv, header)) + dev_err(dev, + "%s: Msg RSP hdr(0x%x) with different sz(%d != %d).\n", + dev_ctx->miscdev.name, + *(u32 *) header, + rx_msg_sz, dev_ctx->temp_resp_size); + else + memcpy(dev_ctx->temp_resp, msg, rx_msg_sz); + + /* Allow user to read */ + dev_ctx->pending_hdr = 1; + wake_up_interruptible(&dev_ctx->wq); + } else { /* * Reading the EdgeLock Enclave response * to the command, sent by other @@ -132,8 +318,8 @@ void se_if_rx_callback(struct mbox_client *mbox_cl, void *msg) complete(&priv->done); spin_unlock(&priv->lock); - return; } + return; } dev_err(dev, "Failed to select a device for message: %.8x\n", diff --git a/drivers/firmware/imx/ele_common.h b/drivers/firmware/imx/ele_common.h index 5ef775a42ab3..7b1c6bfc138b 100644 --- a/drivers/firmware/imx/ele_common.h +++ b/drivers/firmware/imx/ele_common.h @@ -17,6 +17,10 @@ uint32_t se_add_msg_crc(uint32_t *msg, uint32_t msg_len); int ele_msg_rcv(struct se_if_priv *priv); int ele_msg_send(struct se_if_priv *priv, void *tx_msg); int ele_msg_send_rcv(struct se_if_priv *priv, void *tx_msg, void *rx_msg); +int ele_miscdev_msg_rcv(struct se_if_device_ctx *dev_ctx, + void *rx_msg, int rx_msg_sz); +int ele_miscdev_msg_send(struct se_if_device_ctx *dev_ctx, + void *tx_msg, int tx_msg_sz); void se_if_rx_callback(struct mbox_client *mbox_cl, void *msg); int se_val_rsp_hdr_n_status(struct se_if_priv *priv, struct se_msg_hdr *header, diff --git a/drivers/firmware/imx/se_ctrl.c b/drivers/firmware/imx/se_ctrl.c index 23065db5ac17..b12a9795b495 100644 --- a/drivers/firmware/imx/se_ctrl.c +++ b/drivers/firmware/imx/se_ctrl.c @@ -23,6 +23,7 @@ #include <linux/slab.h> #include <linux/string.h> #include <linux/sys_soc.h> +#include <uapi/linux/se_ioctl.h> #include "ele_base_msg.h" #include "ele_common.h" @@ -197,6 +198,588 @@ static int se_soc_info(struct se_if_priv *priv, return 0; } +static int se_ioctl_cmd_snd_rcv_rsp_handler(struct se_if_device_ctx *dev_ctx, + u64 arg) +{ + struct se_if_priv *priv = dev_get_drvdata(dev_ctx->dev); + 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 (copy_from_user(&cmd_snd_rcv_rsp_info, (u8 *)arg, + sizeof(cmd_snd_rcv_rsp_info))) { + dev_err(dev_ctx->priv->dev, + "%s: Failed to copy cmd_snd_rcv_rsp_info from user\n", + dev_ctx->miscdev.name); + err = -EFAULT; + goto exit; + } + + if (cmd_snd_rcv_rsp_info.tx_buf_sz < SE_MU_HDR_SZ) { + dev_err(dev_ctx->priv->dev, + "%s: User buffer too small(%d < %d)\n", + dev_ctx->miscdev.name, + cmd_snd_rcv_rsp_info.tx_buf_sz, + SE_MU_HDR_SZ); + err = -ENOSPC; + goto exit; + } + + rx_msg = kzalloc(cmd_snd_rcv_rsp_info.rx_buf_sz, GFP_KERNEL); + if (!rx_msg) { + err = -ENOMEM; + goto exit; + } + + tx_msg = memdup_user(cmd_snd_rcv_rsp_info.tx_buf, + cmd_snd_rcv_rsp_info.tx_buf_sz); + if (IS_ERR(tx_msg)) { + err = PTR_ERR(tx_msg); + goto exit; + } + + if (tx_msg->header.tag != priv->cmd_tag) { + err = -EINVAL; + goto exit; + } + + guard(mutex)(&priv->se_if_cmd_lock); + priv->waiting_rsp_dev = dev_ctx; + dev_ctx->temp_resp_size = cmd_snd_rcv_rsp_info.rx_buf_sz; + + /* Device Context that is assigned to be a + * FW's command receiver, has pre-allocated buffer. + */ + if (dev_ctx != priv->cmd_receiver_dev) + dev_ctx->temp_resp = rx_msg; + + err = ele_miscdev_msg_send(dev_ctx, + tx_msg, + cmd_snd_rcv_rsp_info.tx_buf_sz); + if (err < 0) + goto exit; + + cmd_snd_rcv_rsp_info.tx_buf_sz = err; + + err = ele_miscdev_msg_rcv(dev_ctx, + cmd_snd_rcv_rsp_info.rx_buf, + cmd_snd_rcv_rsp_info.rx_buf_sz); + + if (err < 0) + goto exit; + + cmd_snd_rcv_rsp_info.rx_buf_sz = err; + +exit: + dev_ctx->temp_resp_size = 0; + priv->waiting_rsp_dev = NULL; + if (dev_ctx != priv->cmd_receiver_dev) + dev_ctx->temp_resp = NULL; + + if (copy_to_user((void *)arg, &cmd_snd_rcv_rsp_info, + sizeof(cmd_snd_rcv_rsp_info))) { + dev_err(dev_ctx->priv->dev, + "%s: Failed to copy cmd_snd_rcv_rsp_info from user\n", + dev_ctx->miscdev.name); + err = -EFAULT; + } + return err; +} + +/* + * File operations for user-space + */ + +/* Write a message to the MU. */ +static ssize_t se_if_fops_write(struct file *fp, const char __user *buf, + size_t size, loff_t *ppos) +{ + struct se_api_msg *tx_msg __free(kfree) = NULL; + struct se_if_device_ctx *dev_ctx; + struct se_if_priv *priv; + int err; + + dev_ctx = container_of(fp->private_data, + struct se_if_device_ctx, + miscdev); + priv = dev_ctx->priv; + dev_dbg(priv->dev, + "%s: write from 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 (size < SE_MU_HDR_SZ) { + dev_err(priv->dev, + "%s: User buffer too small(%zu < %d)\n", + dev_ctx->miscdev.name, + size, SE_MU_HDR_SZ); + err = -ENOSPC; + goto exit; + } + tx_msg = memdup_user(buf, size); + if (IS_ERR(tx_msg)) { + err = PTR_ERR(tx_msg); + goto exit; + } + + print_hex_dump_debug("from user ", DUMP_PREFIX_OFFSET, 4, 4, + tx_msg, size, false); + + err = ele_miscdev_msg_send(dev_ctx, tx_msg, size); + +exit: + up(&dev_ctx->fops_lock); + return err; +} + +/* + * Read a message from the MU. + * Blocking until a message is available. + */ +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; + struct se_if_priv *priv; + int err; + + dev_ctx = container_of(fp->private_data, + struct se_if_device_ctx, + miscdev); + priv = dev_ctx->priv; + 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; + } + + err = ele_miscdev_msg_rcv(dev_ctx, buf, size); + +exit: + up(&dev_ctx->fops_lock); + return err; +} + +static int se_ioctl_get_mu_info(struct se_if_device_ctx *dev_ctx, + u64 arg) +{ + struct se_if_priv *priv = dev_get_drvdata(dev_ctx->dev); + struct se_if_node_info *if_node_info; + struct se_ioctl_get_if_info info; + int err = 0; + + if_node_info = (struct se_if_node_info *)priv->info; + + info.se_if_id = if_node_info->se_if_id; + info.interrupt_idx = 0; + info.tz = 0; + info.did = if_node_info->se_if_did; + info.cmd_tag = if_node_info->cmd_tag; + info.rsp_tag = if_node_info->rsp_tag; + info.success_tag = if_node_info->success_tag; + info.base_api_ver = if_node_info->base_api_ver; + info.fw_api_ver = if_node_info->fw_api_ver; + + dev_dbg(priv->dev, + "%s: info [se_if_id: %d, irq_idx: %d, tz: 0x%x, did: 0x%x]\n", + dev_ctx->miscdev.name, + info.se_if_id, info.interrupt_idx, info.tz, info.did); + + if (copy_to_user((u8 *)arg, &info, sizeof(info))) { + dev_err(dev_ctx->priv->dev, + "%s: Failed to copy mu info to user\n", + dev_ctx->miscdev.name); + err = -EFAULT; + goto exit; + } + +exit: + return err; +} + +/* Need to copy the output data to user-device context. + */ +int se_dev_ctx_cpy_out_data(struct se_if_device_ctx *dev_ctx, bool do_cpy) +{ + struct se_buf_desc *b_desc, *temp; + + list_for_each_entry_safe(b_desc, temp, &dev_ctx->pending_out, link) { + if (b_desc->usr_buf_ptr && b_desc->shared_buf_ptr && do_cpy) { + + dev_dbg(dev_ctx->dev, + "%s: Copy output data to user\n", + dev_ctx->miscdev.name); + if (copy_to_user(b_desc->usr_buf_ptr, + b_desc->shared_buf_ptr, + b_desc->size)) { + dev_err(dev_ctx->dev, + "%s: Failure copying output data to user.", + dev_ctx->miscdev.name); + return -EFAULT; + } + } + + if (b_desc->shared_buf_ptr) + memset(b_desc->shared_buf_ptr, 0, b_desc->size); + + list_del(&b_desc->link); + kfree(b_desc); + } + return 0; +} + +/* + * Clean the used Shared Memory space, + * whether its Input Data copied from user buffers, or + * Data received from FW. + */ +void se_dev_ctx_shared_mem_cleanup(struct se_if_device_ctx *dev_ctx) +{ + struct list_head *dev_ctx_lists[] = {&dev_ctx->pending_in, + &dev_ctx->pending_out}; + struct se_buf_desc *b_desc, *temp; + int i; + + for (i = 0; i < 2; i++) { + list_for_each_entry_safe(b_desc, temp, + dev_ctx_lists[i], link) { + + if (b_desc->shared_buf_ptr) + memset(b_desc->shared_buf_ptr, 0, b_desc->size); + + list_del(&b_desc->link); + kfree(b_desc); + } + } +} + +/* + * Copy a buffer of data to/from the user and return the address to use in + * messages + */ +static int se_ioctl_setup_iobuf_handler(struct se_if_device_ctx *dev_ctx, + u64 arg) +{ + struct se_shared_mem *shared_mem = NULL; + struct se_ioctl_setup_iobuf io = {0}; + struct se_buf_desc *b_desc = NULL; + int err = 0; + u32 pos; + + if (copy_from_user(&io, (u8 *)arg, sizeof(io))) { + dev_err(dev_ctx->priv->dev, + "%s: Failed copy iobuf config from user\n", + dev_ctx->miscdev.name); + err = -EFAULT; + goto exit; + } + + dev_dbg(dev_ctx->priv->dev, + "%s: io [buf: %p(%d) flag: %x]\n", + dev_ctx->miscdev.name, + io.user_buf, io.length, io.flags); + + if (io.length == 0 || !io.user_buf) { + /* + * Accept NULL pointers since some buffers are optional + * in FW commands. In this case we should return 0 as + * pointer to be embedded into the message. + * Skip all data copy part of code below. + */ + io.ele_addr = 0; + goto copy; + } + + /* No specific requirement for this buffer. */ + shared_mem = &dev_ctx->non_secure_mem; + + /* Check there is enough space in the shared memory. */ + if (shared_mem->size < shared_mem->pos || + round_up(io.length, 8u) >= (shared_mem->size - shared_mem->pos)) { + dev_err(dev_ctx->priv->dev, + "%s: Not enough space in shared memory\n", + dev_ctx->miscdev.name); + err = -ENOMEM; + goto exit; + } + + /* Allocate space in shared memory. 8 bytes aligned. */ + pos = shared_mem->pos; + shared_mem->pos += round_up(io.length, 8u); + io.ele_addr = (u64)shared_mem->dma_addr + pos; + + memset(shared_mem->ptr + pos, 0, io.length); + if ((io.flags & SE_IO_BUF_FLAGS_IS_INPUT) || + (io.flags & SE_IO_BUF_FLAGS_IS_IN_OUT)) { + /* + * buffer is input: + * copy data from user space to this allocated buffer. + */ + if (copy_from_user(shared_mem->ptr + pos, io.user_buf, + io.length)) { + dev_err(dev_ctx->priv->dev, + "%s: Failed copy data to shared memory\n", + dev_ctx->miscdev.name); + err = -EFAULT; + goto exit; + } + } + + b_desc = kzalloc(sizeof(*b_desc), GFP_KERNEL); + if (!b_desc) { + err = -ENOMEM; + goto exit; + } + +copy: + /* Provide the EdgeLock Enclave address to user space only if success.*/ + if (copy_to_user((u8 *)arg, &io, sizeof(io))) { + dev_err(dev_ctx->priv->dev, + "%s: Failed to copy iobuff setup to user\n", + dev_ctx->miscdev.name); + kfree(b_desc); + err = -EFAULT; + goto exit; + } + + if (b_desc) { + b_desc->shared_buf_ptr = shared_mem->ptr + 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, &dev_ctx->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, &dev_ctx->pending_out); + } + } + +exit: + return err; +} + +/* IOCTL to provide SoC information */ +static int se_ioctl_get_se_soc_info_handler(struct se_if_device_ctx *dev_ctx, + u64 arg) +{ + const struct se_if_node_info_list *info_list; + struct se_ioctl_get_soc_info soc_info; + int err = -EINVAL; + + info_list = device_get_match_data(dev_ctx->priv->dev); + if (!info_list) + goto exit; + + soc_info.soc_id = info_list->soc_id; + soc_info.soc_rev = dev_ctx->priv->soc_rev; + + err = (int)copy_to_user((u8 *)arg, (u8 *)(&soc_info), sizeof(soc_info)); + if (err) { + dev_err(dev_ctx->priv->dev, + "%s: Failed to copy soc info to user\n", + dev_ctx->miscdev.name); + err = -EFAULT; + goto exit; + } + +exit: + return err; +} + +/* 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); + int err = 0; + + /* Avoid race if opened at the same time */ + if (down_trylock(&dev_ctx->fops_lock)) + return -EBUSY; + + /* Authorize only 1 instance. */ + if (dev_ctx->status != SE_IF_CTX_FREE) { + err = -EBUSY; + goto exit; + } + + /* + * Allocate some memory for data exchanges with S40x. + * This will be used for data not requiring secure memory. + */ + dev_ctx->non_secure_mem.ptr = dmam_alloc_coherent(dev_ctx->dev, + MAX_DATA_SIZE_PER_USER, + &dev_ctx->non_secure_mem.dma_addr, + GFP_KERNEL); + if (!dev_ctx->non_secure_mem.ptr) { + err = -ENOMEM; + goto exit; + } + + dev_ctx->non_secure_mem.size = MAX_DATA_SIZE_PER_USER; + dev_ctx->non_secure_mem.pos = 0; + dev_ctx->status = SE_IF_CTX_OPENED; + + dev_ctx->pending_hdr = 0; + + goto exit; + + dmam_free_coherent(dev_ctx->priv->dev, MAX_DATA_SIZE_PER_USER, + dev_ctx->non_secure_mem.ptr, + dev_ctx->non_secure_mem.dma_addr); + +exit: + up(&dev_ctx->fops_lock); + 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 = container_of(fp->private_data, + struct se_if_device_ctx, + miscdev); + 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_dev == dev_ctx) { + priv->cmd_receiver_dev = NULL; + kfree(dev_ctx->temp_resp); + } + + /* check if this device was registered as waiting response. */ + if (priv->waiting_rsp_dev == dev_ctx) { + priv->waiting_rsp_dev = NULL; + mutex_unlock(&priv->se_if_cmd_lock); + } + + /* Unmap secure memory shared buffer. */ + if (dev_ctx->secure_mem.ptr) + devm_iounmap(dev_ctx->dev, dev_ctx->secure_mem.ptr); + + dev_ctx->secure_mem.ptr = NULL; + dev_ctx->secure_mem.dma_addr = 0; + dev_ctx->secure_mem.size = 0; + dev_ctx->secure_mem.pos = 0; + + /* Free non-secure shared buffer. */ + dmam_free_coherent(dev_ctx->priv->dev, MAX_DATA_SIZE_PER_USER, + dev_ctx->non_secure_mem.ptr, + dev_ctx->non_secure_mem.dma_addr); + + dev_ctx->non_secure_mem.ptr = NULL; + dev_ctx->non_secure_mem.dma_addr = 0; + dev_ctx->non_secure_mem.size = 0; + dev_ctx->non_secure_mem.pos = 0; + se_dev_ctx_shared_mem_cleanup(dev_ctx); + dev_ctx->status = SE_IF_CTX_FREE; + +exit: + up(&dev_ctx->fops_lock); + return 0; +} + +/* IOCTL entry point of a character device */ +static long se_ioctl(struct file *fp, unsigned int cmd, unsigned long arg) +{ + struct se_if_device_ctx *dev_ctx = container_of(fp->private_data, + struct se_if_device_ctx, + miscdev); + struct se_if_priv *se_if_priv = dev_ctx->priv; + int err = -EINVAL; + + /* Prevent race during change of device context */ + if (down_interruptible(&dev_ctx->fops_lock)) + return -EBUSY; + + switch (cmd) { + case SE_IOCTL_ENABLE_CMD_RCV: + if (!se_if_priv->cmd_receiver_dev) { + err = 0; + se_if_priv->cmd_receiver_dev = dev_ctx; + dev_ctx->temp_resp = kzalloc(MAX_NVM_MSG_LEN, GFP_KERNEL); + if (!dev_ctx->temp_resp) + err = -ENOMEM; + } + break; + case SE_IOCTL_GET_MU_INFO: + err = se_ioctl_get_mu_info(dev_ctx, arg); + break; + case SE_IOCTL_SETUP_IOBUF: + err = se_ioctl_setup_iobuf_handler(dev_ctx, arg); + break; + case SE_IOCTL_GET_SOC_INFO: + err = se_ioctl_get_se_soc_info_handler(dev_ctx, arg); + break; + case SE_IOCTL_CMD_SEND_RCV_RSP: + err = se_ioctl_cmd_snd_rcv_rsp_handler(dev_ctx, arg); + break; + + default: + err = -EINVAL; + dev_dbg(se_if_priv->dev, + "%s: IOCTL %.8x not supported\n", + dev_ctx->miscdev.name, + cmd); + } + + up(&dev_ctx->fops_lock); + return (long)err; +} + +/* Char driver setup */ +static const struct file_operations se_if_fops = { + .open = se_if_fops_open, + .owner = THIS_MODULE, + .release = se_if_fops_close, + .unlocked_ioctl = se_ioctl, + .read = se_if_fops_read, + .write = se_if_fops_write, +}; + +/* interface for managed res to unregister a character device */ +static void if_misc_deregister(void *miscdevice) +{ + misc_deregister(miscdevice); +} + /* interface for managed res to free a mailbox channel */ static void if_mbox_free_channel(void *mbox_chan) { @@ -234,6 +817,7 @@ static void se_if_probe_cleanup(struct platform_device *pdev) { struct device *dev = &pdev->dev; struct se_if_priv *priv; + int i; priv = dev_get_drvdata(dev); @@ -253,6 +837,17 @@ static void se_if_probe_cleanup(struct platform_device *pdev) priv->imem.buf = NULL; } + if (priv->ctxs) { + for (i = 0; i < priv->max_dev_ctx; i++) { + if (priv->ctxs[i]) { + devm_remove_action(dev, + if_misc_deregister, + &priv->ctxs[i]->miscdev); + misc_deregister(&priv->ctxs[i]->miscdev); + } + } + } + /* No need to check, if reserved memory is allocated * before calling for its release. Or clearing the * un-set bit. @@ -260,6 +855,74 @@ static void se_if_probe_cleanup(struct platform_device *pdev) of_reserved_mem_device_release(dev); } +static int init_device_context(struct se_if_priv *priv) +{ + const struct se_if_node_info *info = priv->info; + struct se_if_device_ctx *dev_ctx; + u8 *devname; + int ret = 0; + int i; + + priv->ctxs = devm_kzalloc(priv->dev, sizeof(dev_ctx) * priv->max_dev_ctx, + GFP_KERNEL); + + if (!priv->ctxs) { + ret = -ENOMEM; + return ret; + } + + /* Create users */ + for (i = 0; i < priv->max_dev_ctx; i++) { + dev_ctx = devm_kzalloc(priv->dev, sizeof(*dev_ctx), GFP_KERNEL); + if (!dev_ctx) { + ret = -ENOMEM; + return ret; + } + + dev_ctx->dev = priv->dev; + dev_ctx->status = SE_IF_CTX_FREE; + dev_ctx->priv = priv; + + priv->ctxs[i] = dev_ctx; + + /* Default value invalid for an header. */ + init_waitqueue_head(&dev_ctx->wq); + + INIT_LIST_HEAD(&dev_ctx->pending_out); + INIT_LIST_HEAD(&dev_ctx->pending_in); + sema_init(&dev_ctx->fops_lock, 1); + + devname = devm_kasprintf(priv->dev, GFP_KERNEL, "%s_ch%d", + info->se_name, i); + if (!devname) { + ret = -ENOMEM; + return ret; + } + + dev_ctx->miscdev.name = devname; + dev_ctx->miscdev.minor = MISC_DYNAMIC_MINOR; + dev_ctx->miscdev.fops = &se_if_fops; + dev_ctx->miscdev.parent = priv->dev; + ret = misc_register(&dev_ctx->miscdev); + if (ret) { + dev_err(priv->dev, "failed to register misc device %d\n", + ret); + return ret; + } + + ret = devm_add_action(priv->dev, if_misc_deregister, + &dev_ctx->miscdev); + if (ret) { + dev_err(priv->dev, + "failed[%d] to add action to the misc-dev\n", + ret); + return ret; + } + } + + return ret; +} + static void se_load_firmware(const struct firmware *fw, void *context) { struct se_if_priv *priv = (struct se_if_priv *) context; @@ -459,6 +1122,16 @@ static int se_if_probe(struct platform_device *pdev) ret = 0; } + if (info->max_dev_ctx) { + ret = init_device_context(priv); + if (ret) { + dev_err(dev, + "Failed[0x%x] to create device contexts.\n", + ret); + goto exit; + } + } + dev_info(dev, "i.MX secure-enclave: %s interface to firmware, configured.\n", info->se_name); return ret; @@ -499,6 +1172,10 @@ static int se_resume(struct device *dev) { struct se_if_priv *priv = dev_get_drvdata(dev); const struct se_if_node_info *info = priv->info; + int i; + + for (i = 0; i < priv->max_dev_ctx; i++) + wake_up_interruptible(&priv->ctxs[i]->wq); if (info && info->imem_mgmt_file_in_rfs) se_restore_imem_state(priv); diff --git a/drivers/firmware/imx/se_ctrl.h b/drivers/firmware/imx/se_ctrl.h index f6008481408a..4f119a979a04 100644 --- a/drivers/firmware/imx/se_ctrl.h +++ b/drivers/firmware/imx/se_ctrl.h @@ -13,6 +13,8 @@ #define MAX_FW_LOAD_RETRIES 50 #define RES_STATUS(x) FIELD_GET(0x000000ff, x) +#define MAX_DATA_SIZE_PER_USER (65 * 1024) +#define MAX_NVM_MSG_LEN (256) #define MESSAGING_VERSION_6 0x6 #define MESSAGING_VERSION_7 0x7 @@ -26,6 +28,48 @@ struct se_imem_buf { u32 state; }; +struct se_buf_desc { + u8 *shared_buf_ptr; + u8 *usr_buf_ptr; + u32 size; + struct list_head link; +}; + +/* Status of a char device */ +enum se_if_dev_ctx_status_t { + SE_IF_CTX_FREE, + SE_IF_CTX_OPENED +}; + +struct se_shared_mem { + dma_addr_t dma_addr; + u32 size; + u32 pos; + u8 *ptr; +}; + +/* Private struct for each char device instance. */ +struct se_if_device_ctx { + struct device *dev; + struct se_if_priv *priv; + struct miscdevice miscdev; + + enum se_if_dev_ctx_status_t status; + wait_queue_head_t wq; + struct semaphore fops_lock; + + u32 pending_hdr; + struct list_head pending_in; + struct list_head pending_out; + + struct se_shared_mem secure_mem; + struct se_shared_mem non_secure_mem; + + struct se_api_msg *temp_resp; + u32 temp_resp_size; + struct notifier_block se_notify; +}; + /* Header of the messages exchange with the EdgeLock Enclave */ struct se_msg_hdr { u8 ver; @@ -84,4 +128,6 @@ struct se_if_priv { struct se_imem_buf imem; }; +int se_dev_ctx_cpy_out_data(struct se_if_device_ctx *dev_ctx, bool do_cpy); +void se_dev_ctx_shared_mem_cleanup(struct se_if_device_ctx *dev_ctx); #endif diff --git a/include/uapi/linux/se_ioctl.h b/include/uapi/linux/se_ioctl.h new file mode 100644 index 000000000000..c2d0a92ef626 --- /dev/null +++ b/include/uapi/linux/se_ioctl.h @@ -0,0 +1,94 @@ +/* SPDX-License-Identifier: (GPL-2.0 WITH Linux-syscall-note) OR BSD-3-Clause*/ +/* + * Copyright 2024 NXP + */ + +#ifndef SE_IOCTL_H +#define SE_IOCTL_H + +/* IOCTL definitions. */ + +struct se_ioctl_setup_iobuf { + u8 *user_buf; + u32 length; + u32 flags; + u64 ele_addr; +}; + +struct se_ioctl_shared_mem_cfg { + u32 base_offset; + u32 size; +}; + +struct se_ioctl_get_if_info { + u8 se_if_id; + u8 interrupt_idx; + u8 tz; + u8 did; + u8 cmd_tag; + u8 rsp_tag; + u8 success_tag; + u8 base_api_ver; + u8 fw_api_ver; +}; + +struct se_ioctl_cmd_snd_rcv_rsp_info { + u32 *tx_buf; + int tx_buf_sz; + u32 *rx_buf; + int rx_buf_sz; +}; + +struct se_ioctl_get_soc_info { + u16 soc_id; + u16 soc_rev; +}; + +/* IO Buffer Flags */ +#define SE_IO_BUF_FLAGS_IS_OUTPUT (0x00u) +#define SE_IO_BUF_FLAGS_IS_INPUT (0x01u) +#define SE_IO_BUF_FLAGS_USE_SEC_MEM (0x02u) +#define SE_IO_BUF_FLAGS_USE_SHORT_ADDR (0x04u) +#define SE_IO_BUF_FLAGS_IS_IN_OUT (0x10u) + +/* IOCTLS */ +#define SE_IOCTL 0x0A /* like MISC_MAJOR. */ + +/* + * ioctl to designated the current fd as logical-reciever. + * This is ioctl is send when the nvm-daemon, a slave to the + * firmware is started by the user. + */ +#define SE_IOCTL_ENABLE_CMD_RCV _IO(SE_IOCTL, 0x01) + +/* + * ioctl to get the buffer allocated from the memory, which is shared + * between kernel and FW. + * Post allocation, the kernel tagged the allocated memory with: + * Output + * Input + * Input-Output + * Short address + * Secure-memory + */ +#define SE_IOCTL_SETUP_IOBUF _IOWR(SE_IOCTL, 0x03, \ + struct se_ioctl_setup_iobuf) + +/* + * ioctl to get the mu information, that is used to exchange message + * with FW, from user-spaced. + */ +#define SE_IOCTL_GET_MU_INFO _IOR(SE_IOCTL, 0x04, \ + struct se_ioctl_get_if_info) +/* + * ioctl to get SoC Info from user-space. + */ +#define SE_IOCTL_GET_SOC_INFO _IOR(SE_IOCTL, 0x06, \ + struct se_ioctl_get_soc_info) + +/* + * ioctl to send command and receive response from user-space. + */ +#define SE_IOCTL_CMD_SEND_RCV_RSP _IOWR(SE_IOCTL, 0x07, \ + struct se_ioctl_cmd_snd_rcv_rsp_info) +#endif
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@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(-)