RE: RE: RE: RE: [EXT] Re: [PATCH 4/4] firmware: imx: add driver for NXP EdgeLock Enclave

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

 




> -----Original Message-----
> From: Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx>
> Sent: Wednesday, May 22, 2024 4:41 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>; Sascha Hauer
> <s.hauer@xxxxxxxxxxxxxx>; Pengutronix Kernel Team
> <kernel@xxxxxxxxxxxxxx>; Fabio Estevam <festevam@xxxxxxxxx>; linux-
> doc@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx; imx@xxxxxxxxxxxxxxx; linux-arm-
> kernel@xxxxxxxxxxxxxxxxxxx
> Subject: Re: RE: RE: RE: [EXT] Re: [PATCH 4/4] firmware: imx: add driver for
> NXP EdgeLock Enclave
> 
> On 22.05.2024 10:46:10, Pankaj Gupta wrote:
> > > > > > > > +
> > > > > > > > +	memset(s_info, 0x0, sizeof(*s_info));
> > > > > > > > +
> > > > > > > > +	if (priv->mem_pool_name)
> > > > > > > > +		get_info_data = get_phy_buf_mem_pool(dev,
> > > > > > > > +						     priv-
> > > >mem_pool_name,
> > > > > > > > +						     &get_info_addr,
> > > > > > > > +
> > > ELE_GET_INFO_BUFF_SZ);
> > > > > > > > +	else
> > > > > > > > +		get_info_data = dmam_alloc_coherent(dev,
> > > > > > > > +
> > > ELE_GET_INFO_BUFF_SZ,
> > > > > > > > +						    &get_info_addr,
> > > > > > > > +						    GFP_KERNEL);
> > > > > > >
> > > > > > > It's better style to move the init of the dma memory into
> > > > > > > the probe function.
> > > > > >
> > > > > > It is not DMA init. It is DMA allocation.
> > > > >
> > > > > It's better style to move the allocation of the dma memory into
> > > > > the probe function.
> > > > >
> > > > The buffer 'get_info_data', is allocated and freed within this function.
> > > > This API is called multiple times:
> > > > - as part of probe.
> > > > - as part of suspend/resume.
> > > >
> > > > Why to keep the memory retained?
> > >
> > > I see. Then why do you allocate with dmam_alloc_coherent()?
> >
> > Because this memory is written by Firmware. It should be either from
> > SRAM Or from reserved memory region, accessible to FW.
> 
> It's about managed resources. Why don't you use dma_alloc_coherent()?

Sorry to miss out the point, you trying to make.
Accepted. Will change it in v2.

> 
> > > > > > > > +int imx_ele_msg_send(struct se_if_priv *priv, void *mssg) {
> > > > > > > > +	bool is_cmd_lock_tobe_taken = false;
> > > > > > > > +	int err;
> > > > > > > > +
> > > > > > > > +	if (!priv->waiting_rsp_dev || priv->no_dev_ctx_used) {
> > > > > > > > +		is_cmd_lock_tobe_taken = true;
> > > > > > > > +		mutex_lock(&priv->se_if_cmd_lock);
> > > > > > > > +	}
> > > > > > > > +	scoped_guard(mutex, &priv->se_if_lock);
> > > > > > > > +
> > > > > > > > +	err = mbox_send_message(priv->tx_chan, mssg);
> > > > > > > > +	if (err < 0) {
> > > > > > > > +		dev_err(priv->dev, "Error: mbox_send_message
> > > failure.\n");
> > > > > > > > +		if (is_cmd_lock_tobe_taken)
> > > > > > > > +			mutex_unlock(&priv->se_if_cmd_lock);
> > > > > > >
> > > > > > > Only dropping the lock in case of failure doesn't look right to me.
> > > > > >
> > > > > > The callers of this function, takes the execution flow to
> > > > > > aborting the operation on getting return code < 0. No next
> > > > > > action is expected under this aborted operation. Unlocking the
> > > > > > lock here is not an issue
> > > > > >
> > > > > > > It seems you should better move the lock to the callers of this
> function.
> > > > > >
> > > > > > Accepted, and moved to the caller of the function for:
> > > > > >    - locking
> > > > > >    - unlocking in case of error.
> > > > > >
> > > > > > Unlocking in the read API, once response is successfully
> > > > > > received and read.
> > > > >
> > > > > A better design would be: imx_ele_msg_rcv() imx_ele_msg_send()
> > > > > are expected to be called locked. Add lockdep_assert_held() to
> > > > > these function to document/check this.
> > > > >
> > > > > The callers of imx_ele_msg_rcv() and imx_ele_msg_send() have to
> > > > > take care of the locking.
> > > > >
> > > > > [...]
> > > > >
> > > > The locking/unlocking of se_if_cmd_lock, is taken care by the callers
> only:
> > > > - imx_ele_msg_send_rcv calls both the functions:
> > > >   --imx_ele_msg_send.
> > > >   --imx_ele_msg_rcv.
> > > >
> > > > But the lockdep_assert_held, cannot be added to imx_ele_msg_send,
> > > > as its another caller function imx_ele_miscdev_msg_send calls if
> > > > for
> > > > sending:
> > > >  --- command (here command lock is taken).
> > > >  --- response to a command (here command lock is not taken).
> > >
> > > miscdev is another patch.
> > Will try to split it.
> >
> > >
> > > But why can't you use the same lock in imx_ele_miscdev_msg_send()?
> > Using the same lock "se_if_cmd_lock", in imx_ele_miscdev_msg_send.
> > This function is called from fops_write. This lock is taken conditionally taken
> depending on the kind of message:
> >
> >   --- Message containing command (here command lock is taken).
> >   --- Message containing response to a command (here command lock is not
> taken).
> 
> Let's design a proper the kernel internal interface first. For simplicity reasons
> the misc dev should be out of scope first

Ok, will split the patch.
In patch 4/5 the lockdep_assert_held will be added to imx_ele_msg_send, and in the patch 5/5, it will be removed or I need to create duplicate function without lockdep_assert_held.

> 
> > > > > > > > +static const struct imx_se_node_info_list imx93_info = {
> > > > > > > > +	.num_mu = 1,
> > > > > > > > +	.soc_id = SOC_ID_OF_IMX93,
> > > > > > > > +	.info = {
> > > > > > > > +			{
> > > > > > > > +				.se_if_id = 2,
> > > > > > > > +				.se_if_did = 3,
> > > > > > > > +				.max_dev_ctx = 4,
> > > > > > > > +				.cmd_tag = 0x17,
> > > > > > > > +				.rsp_tag = 0xe1,
> > > > > > > > +				.success_tag = 0xd6,
> > > > > > > > +				.base_api_ver =
> > > MESSAGING_VERSION_6,
> > > > > > > > +				.fw_api_ver =
> > > MESSAGING_VERSION_7,
> > > > > > > > +				.se_name = "hsm1",
> > > > > > > > +				.mbox_tx_name = "tx",
> > > > > > > > +				.mbox_rx_name = "rx",
> > > > > > > > +				.reserved_dma_ranges = true,
> > > > > > > > +				.imem_mgmt = true,
> > > > > > > > +				.soc_register = true,
> > > > > > > > +			},
> > > > > > > > +	},
> > > > > > >
> > > > > > >
> > > > > > > Some (most?) members of these structs are the same. Why do
> > > > > > > you have this abstraction if it's not needed right now?
> > > > > >
> > > > > > It is needed as the values is different for different NXP SoC
> > > > > > compatible. It will be needed for NXP i.MX95 platform, whose
> > > > > > code will be next in pipeline.
> > > > >
> > > > > How does the imx95 .info look like?
> > > > >
> > > > Copied from the internal repo.
> > > > static const struct imx_info_list imx95_info = {
> > > >         .num_mu = 4,
> > > >         .soc_id = SOC_ID_OF_IMX95,
> > > >         .info = {
> > > >                         {
> > > >                                 .socdev = false,
> > > >                                 .mu_id = 2,
> > > >                                 .mu_did = 3,
> > > >                                 .max_dev_ctx = 4,
> > > >                                 .cmd_tag = 0x17,
> > > >                                 .rsp_tag = 0xe1,
> > > >                                 .success_tag = 0xd6,
> > > >                                 .base_api_ver = MESSAGING_VERSION_6,
> > > >                                 .fw_api_ver = MESSAGING_VERSION_7,
> > > >                                 .se_name = "hsm1",
> > > >                                 .mbox_tx_name = "tx",
> > > >                                 .mbox_rx_name = "rx",
> > > >                                 .pool_name = NULL,
> > > >                                 .reserved_dma_ranges = false,
> > > >                                 .init_fw = true,
> > > >                                 .v2x_state_check = true,
> > > >                                 .start_rng = ele_start_rng,
> > > >                                 .enable_ele_trng = true,
> > > >                                 .imem_mgmt = false,
> > > >                                 .mu_buff_size = 0,
> > > >                                 .fw_name_in_rfs = NULL,
> > > >                         },
> > > >                         {
> > > >                                 .socdev = false,
> > > >                                 .mu_id = 0,
> > > >                                 .mu_did = 0,
> > > >                                 .max_dev_ctx = 0,
> > > >                                 .cmd_tag = 0x17,
> > > >                                 .rsp_tag = 0xe1,
> > > >                                 .success_tag = 0xd6,
> > > >                                 .base_api_ver = 0x2,
> > > >                                 .fw_api_ver = 0x2,
> > > >                                 .se_name = "v2x_dbg",
> > > >                                 .pool_name = NULL,
> > > >                                 .mbox_tx_name = "tx",
> > > >                                 .mbox_rx_name = "rx",
> > > >                                 .reserved_dma_ranges = false,
> > > >                                 .init_fw = false,
> > > >                                 .v2x_state_check = true,
> > > >                                 .start_rng = v2x_start_rng,
> > > >                                 .enable_ele_trng = false,
> > > >                                 .imem_mgmt = false,
> > > >                                 .mu_buff_size = 0,
> > > >                                 .fw_name_in_rfs = NULL,
> > > >                         },
> > > >                         {
> > > >                                 .socdev = false,
> > > >                                 .mu_id = 4,
> > > >                                 .mu_did = 0,
> > > >                                 .max_dev_ctx = 4,
> > > >                                 .cmd_tag = 0x18,
> > > >                                 .rsp_tag = 0xe2,
> > > >                                 .success_tag = 0xd6,
> > > >                                 .base_api_ver = 0x2,
> > > >                                 .fw_api_ver = 0x2,
> > > >                                 .se_name = "v2x_sv0",
> > > >                                 .pool_name = NULL,
> > > >                                 .mbox_tx_name = "tx",
> > > >                                 .mbox_rx_name = "rx",
> > > >                                 .reserved_dma_ranges = false,
> > > >                                 .init_fw = false,
> > > >                                 .v2x_state_check = true,
> > > >                                 .start_rng = NULL,
> > > >                                 .enable_ele_trng = false,
> > > >                                 .imem_mgmt = false,
> > > >                                 .mu_buff_size = 16,
> > > >                                 .fw_name_in_rfs = NULL,
> > > >                         },
> > > >                         {
> > > >                                 .socdev = false,
> > > >                                 .mu_id = 6,
> > > >                                 .mu_did = 0,
> > > >                                 .max_dev_ctx = 4,
> > > >                                 .cmd_tag = 0x1a,
> > > >                                 .rsp_tag = 0xe4,
> > > >                                 .success_tag = 0xd6,
> > > >                                 .base_api_ver = 0x2,
> > > >                                 .fw_api_ver = 0x2,
> > > >                                 .se_name = "v2x_she",
> > > >                                 .pool_name = NULL,
> > > >                                 .mbox_tx_name = "tx",
> > > > 		   .mbox_rx_name = "rx",
> > > >                                 .reserved_dma_ranges = false,
> > > >                                 .init_fw = false,
> > > >                                 .v2x_state_check = true,
> > > >                                 .start_rng = NULL,
> > > >                                 .enable_ele_trng = false,
> > > >                                 .imem_mgmt = false,
> > > >                                 .mu_buff_size = 16,
> > > >                                 .fw_name_in_rfs = NULL,
> > > >                         },
> > > >                         {
> > > >                                 .socdev = false,
> > > >                                 .mu_id = 6,
> > > >                                 .mu_did = 0,
> > > >                                 .max_dev_ctx = 4,
> > > >                                 .cmd_tag = 0x1a,
> > > >                                 .rsp_tag = 0xe4,
> > > >                                 .success_tag = 0xd6,
> > > >                                 .base_api_ver = 0x2,
> > > >                                 .fw_api_ver = 0x2,
> > > >                                 .se_name = "v2x_she",
> > > >                                 .pool_name = NULL,
> > > >                                 .mbox_tx_name = "tx",
> > > >                                 .mbox_rx_name = "rx",
> > > >                                 .reserved_dma_ranges = false,
> > > >                                 .init_fw = false,
> > > >                                 .v2x_state_check = true,
> > > >                                 .start_rng = NULL,
> > > >                                 .enable_ele_trng = false,
> > > >                                 .imem_mgmt = false,
> > > >                                 .mu_buff_size = 256,
> > > >                                 .fw_name_in_rfs = NULL,
> > > >                         },
> > > >         }
> > > > };
> > >
> > > Just looking at _some_, the .cmd_tag, .rsp_tag and .success_tag look
> > > the same for all SoCs.
> > .cmd_tag & .rsp_tag is varying for each: .se_name = "v2x_dbg",
> > .se_name = "v2x_she" and .se_name = "v2x_sv0",
> >
> > .success_tag is going to be different for i.MX8DXL. It will be zero
> > for i.MX8DXL, as compared to current 0xD6, for i.MX8ULP, 93, 95
> >
> >
> > >
> > > [...]
> > >
> > > > Created a static variable g_soc_rev in the se_ctrl.c.
> > > > Accepted and will correct it in v2.
> > > >
> > > > >
> > > > > > > > +	if (info_list->soc_rev)
> > > > > > > > +		return err;
> > > > > > >
> > > > Will change the above condition to g_soc_rev.
> > >
> > > "g_" as is global? Don't do that. Use your priv!
> > Yes, soc_rev can be put under priv.
> > It is proposed like this as it is used only once, that too in this file only.
> >
> > Will do this in V2.
> 
> If it's only used once, pass it via a function parameter. If you need it past
> probe, put in priv.
> 
Need it past probe.

> Marc
> 
> --
> Pengutronix e.K.                 | Marc Kleine-Budde          |
> Embedded Linux                   | https://www.pengutronix.de |
> Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
> Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |




[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux