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@xxxxxxxxxxxxxx>
> Sent: Friday, August 9, 2024 12:40 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: 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 |
