Message-ID: <ZrXAv79KFCSyB3U_@pengutronix.de>
Date: Fri, 9 Aug 2024 09:09:51 +0200
From: Sascha Hauer <s.hauer@...gutronix.de>
To: Pankaj Gupta <pankaj.gupta@....com>
Cc: Jonathan Corbet <corbet@....net>, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>, Shawn Guo <shawnguo@...nel.org>,
	Pengutronix Kernel Team <kernel@...gutronix.de>,
	Fabio Estevam <festevam@...il.com>,
	Rob Herring <robh+dt@...nel.org>,
	"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"imx@...ts.linux.dev" <imx@...ts.linux.dev>,
	"linux-arm-kernel@...ts.infradead.org" <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [EXT] Re: [PATCH v6 5/5] firmware: imx: adds miscdev

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

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Powered by blists - more mailing lists