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