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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




> -----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 |





[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux