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

From: Marc Kleine-Budde
Date: Wed May 22 2024 - 03:48:17 EST

On 21.05.2024 11:57:04, 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()?

[...]

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

But why can't you use the same lock in imx_ele_miscdev_msg_send()?


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

[...]

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

[...]

> > > > > +
> > > > > + info_list->soc_rev = s_info.soc_rev;
> > > > > +
> > > > > + if (!info->soc_register)
> > > > > + return 0;
> > > > > +
> > > > > + attr = devm_kzalloc(dev, sizeof(*attr), GFP_KERNEL);
> > > > > + if (!attr)
> > > > > + return -ENOMEM;
> > > > > +
> > > > > + if (s_info.minor_ver)
> > > > > + attr->revision = devm_kasprintf(dev, GFP_KERNEL, "%x.%x",
> > > > > + s_info.major_ver,
> > > > > + s_info.minor_ver);
> > > > > + else
> > > > > + attr->revision = devm_kasprintf(dev, GFP_KERNEL, "%x",
> > > > > + s_info.major_ver);
> > > > > +
> > > > > + switch (s_info.soc_id) {
> > > > > + case SOC_ID_OF_IMX8ULP:
> > > > > + attr->soc_id = devm_kasprintf(dev, GFP_KERNEL,
> > > > > + "i.MX8ULP");
> > > > > + break;
> > > > > + case SOC_ID_OF_IMX93:
> > > > > + attr->soc_id = devm_kasprintf(dev, GFP_KERNEL,
> > > > > + "i.MX93");
> > > > > + break;
> > > > > + }
> > > > > +
> > > > > + err = of_property_read_string(of_root, "model",
> > > > > + &attr->machine);
> > > > > + if (err) {
> > > > > + devm_kfree(dev, attr);
> > > >
> > > > Why do you do a manual cleanup of devm managed resources? Same
> > > > applies to the other devm managed resources, too.
> > > >
> > > Used devm managed memory, as this function is called as part probe.
> > > Post device registration, this devm managed memory is un-necessarily
> > > blocked. It is better to release it as part of clean-up, under this
> > > function only.
> >
> > Why do you allocate the memory with devm in the first place, if it's not
> > needed after probe?
>
> Sorry to confuse you. Actually the devm_memory will be needed for the case of soc_registration.
> Meaning, memory with devm, will be needed post probing as well.
>
> If this function fails, the probing will fail too. It will be auto cleaned.
>
> Accepted, will remove the devm_free in v2.

If you don't need the memory past probe() allocate with kzalloc() and
use kfree(). Only used managed resources for lifetimes beyond the probe
function.

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 |

Attachment: signature.asc
Description: PGP signature