> -----Original Message----- > From: Sascha Hauer <s.hauer@xxxxxxxxxxxxxx> > Sent: Friday, May 24, 2024 6:14 PM > To: Pankaj Gupta <pankaj.gupta@xxxxxxx> > Cc: Jonathan Corbet <corbet@xxxxxxx>; Rob Herring <robh+dt@xxxxxxxxxx>; > Krzysztof Kozlowski <krzysztof.kozlowski+dt@xxxxxxxxxx>; Conor Dooley > <conor+dt@xxxxxxxxxx>; Shawn Guo <shawnguo@xxxxxxxxxx>; Pengutronix > Kernel Team <kernel@xxxxxxxxxxxxxx>; Fabio Estevam > <festevam@xxxxxxxxx>; Rob Herring <robh@xxxxxxxxxx>; Krzysztof Kozlowski > <krzk+dt@xxxxxxxxxx>; linux-doc@xxxxxxxxxxxxxxx; linux- > kernel@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; imx@xxxxxxxxxxxxxxx; > linux-arm-kernel@xxxxxxxxxxxxxxxxxxx > Subject: Re: [EXT] Re: [PATCH v2 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 Fri, May 24, 2024 at 12:03:35PM +0000, Pankaj Gupta wrote: > > > > > > > -----Original Message----- > > > From: Sascha Hauer <s.hauer@xxxxxxxxxxxxxx> > > > Sent: Friday, May 24, 2024 1:53 PM > > > To: Pankaj Gupta <pankaj.gupta@xxxxxxx> > > > Cc: Jonathan Corbet <corbet@xxxxxxx>; Rob Herring > > > <robh+dt@xxxxxxxxxx>; Krzysztof Kozlowski > > > <krzysztof.kozlowski+dt@xxxxxxxxxx>; Conor Dooley > > > <conor+dt@xxxxxxxxxx>; Shawn Guo <shawnguo@xxxxxxxxxx>; > Pengutronix > > > Kernel Team <kernel@xxxxxxxxxxxxxx>; Fabio Estevam > > > <festevam@xxxxxxxxx>; Rob Herring <robh@xxxxxxxxxx>; Krzysztof > > > Kozlowski <krzk+dt@xxxxxxxxxx>; linux-doc@xxxxxxxxxxxxxxx; linux- > > > kernel@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; > > > imx@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx > > > Subject: [EXT] Re: [PATCH v2 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, May 23, 2024 at 04:19:36PM +0530, Pankaj Gupta wrote: > > > > +int imx_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; > > > > + int err; > > > > + > > > > + header = (struct se_msg_hdr *) tx_msg; > > > > + > > > > + /* > > > > + * Check that the size passed as argument matches the size > > > > + * carried in the message. > > > > + */ > > > > + err = header->size << 2; > > > > + > > > > + if (err != tx_msg_sz) { > > > > + err = -EINVAL; > > > > + dev_err(priv->dev, > > > > + "%s: User buffer too small\n", > > > > + dev_ctx->miscdev.name); > > > > + goto exit; > > > > + } > > > > + /* Check the message is valid according to tags */ > > > > + if (header->tag == priv->cmd_tag) { > > > > + mutex_lock(&priv->se_if_cmd_lock); > > > > > > Grabbing a mutex in a character devices write fop and releasing it > > > in the read fop is really calling for undesired race conditions. > > > > Condition is: > > - Only one command is allowed to be in flight, at a time per interface. > > -- Second command is not allowed, when one command is in flight. > > - Duration of the flight is till the time the response is not received from the > FW. > > > > Command lock is grabbed and then released in process context only. > > > > > > > > If sending a command and receiving the response shall be an atomic > > > operation then you should really consider turning this into an ioctl > > > and just not implement read/write on the character device. With this > > > you'll be able to get rid of several oddities in this drivers locking. > > > > > > > It is not an atomic operation. It can be pre-empted. > > I didn't mean atomic in the sense of being non preemptable. > > > But it cannot be pre-empted to send another command on the same > interface. > > > > As only one command is allowed to be executed at one point in time, > through an interface. > > I meant atomic in the sense that only one command may be in flight: Send a > message and do not allow to send another message until the answer to the > first one is received. > > Using an ioctl you can just use imx_ele_msg_send_rcv() which takes a mutex > during the whole send/receive process and have no need for such a strange > locking construct. > > > > > + /* > > > > + * We may need to copy the output data to user before > > > > + * delivering the completion message. > > > > + */ > > > > + while (!list_empty(&dev_ctx->pending_out)) { > > > > + b_desc = list_first_entry_or_null(&dev_ctx->pending_out, > > > > + struct se_buf_desc, > > > > + link); > > > > + if (!b_desc) > > > > + continue; > > > > > > b_desc will never be NULL because otherwise you wouldn't be in the > > > loop anymore. The usual way to iterate over a list is to use > > > list_for_each_entry() or > > > list_for_each_entry_safe() in case you delete entries in the loop body. > > > > > > > Will remove the NULL check. > > if (!b_desc) > > continue; > > Please don't. Use list_for_each_entry_safe() which is the normal way to > iterate over a list. This change will add few more cpu cycles. Will add this in v3. > > > > > +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 imx_se_node_info *if_node_info; > > > > + struct se_ioctl_get_if_info info; > > > > + int err = 0; > > > > + > > > > + if_node_info = (struct imx_se_node_info *)priv->info; > > > > > > priv->info is of type const void *. You are casting away the the 'const' > > > here. Either it is const, then it should stay const, or not, in > > > which case it shouldn't be declared const. Also why isn't priv->info > > > of type struct imx_se_node_info * in the first place? > > > > This struct definition is local to the file se_ctrl.c. > > Declaration of imx_se_node_info, is fixed by adding const in the whole file. > > Add a > > struct imx_se_node_info; > > to se_ctrl.h and you're done. The scope of this structure will remain to se_ctrl.c only. If you suggest, will replace the info structure with secure-enclave interface index(u8 if_idx). > > > > > > > + err = -EFAULT; > > > > + goto exit; > > > > + } else { > > > > + /* 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 > > > > + || io.length >= shared_mem->size - shared_mem->pos) { Will update this check to replace io.length with round_up(io.length). Will correct in v3. > > > > + 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); > > > > > > You are checking if there's enough space in the shared memory > > > without taking this round_up into account. > > > > Yes. It is initializing the local variable 'pos', with last store value of > shared_mem->pos. > > Your check is: > > if (shared_mem->size < shared_mem->pos || io.length >= shared_mem- > >size - shared_mem->pos) > > Afterwards you do a: > > shared_mem->pos += round_up(io.length, 8u); > > This invalidates the check. You have to honor the potential padding in your > check as well. > See the comment above. > Sascha > > -- > Pengutronix e.K. | | > Steuerwalder Str. 21 | > http://www.pe/ > ngutronix.de%2F&data=05%7C02%7Cpankaj.gupta%40nxp.com%7Cbd91341 > 92f06423be17c08dc7bef290e%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C > 0%7C0%7C638521514310084478%7CUnknown%7CTWFpbGZsb3d8eyJWIjoi > MC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0% > 7C%7C%7C&sdata=iVTkihKZ0F9ATk%2FTXpXMqmj8tJq8ufKijglcPWegjA4%3D& > reserved=0 | > 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |