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

From: Pankaj Gupta
Date: Thu Aug 08 2024 - 06:49:59 EST

Next message: James Clark: "Re: [PATCH] perf scripts python arm-cs-trace-disasm.py: Skip disasm if address continuity is broken"
Previous message: Krzysztof Kozlowski: "Re: [PATCH v2 2/2] dt-bindings: misc: aspeed,ast2400-cvic: Convert to DT schema"
Next in thread: Sascha Hauer: "Re: [EXT] Re: [PATCH v6 5/5] firmware: imx: adds miscdev"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

> -----Original Message-----
> From: Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>
> Sent: Tuesday, July 23, 2024 7:51 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 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 |

Next message: James Clark: "Re: [PATCH] perf scripts python arm-cs-trace-disasm.py: Skip disasm if address continuity is broken"
Previous message: Krzysztof Kozlowski: "Re: [PATCH v2 2/2] dt-bindings: misc: aspeed,ast2400-cvic: Convert to DT schema"
Next in thread: Sascha Hauer: "Re: [EXT] Re: [PATCH v6 5/5] firmware: imx: adds miscdev"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]