Re: [PATCH v46 3/7] firmware: imx: add driver for NXP EdgeLock Enclave

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Deadlock in suspend/resume path due to GFP_KERNEL memory allocations in ele_get_info().
--

commit 05cadf4c7e4a0295c86129fe662754b87efdff7e
Author: Pankaj Gupta <pankaj.gupta@nxp.com>

firmware: imx: add driver for NXP EdgeLock Enclave

Add MU-based communication interface for secure enclave. NXP hardware IP(s)
for secure-enclaves like Edgelock Enclave (ELE) are embedded in the SoC to
support features like HSM, SHE & V2X. The secure enclave FW communicates
with Linux over single or multiple dedicated messaging unit (MU) based
interface(s).

Closes: https://sashiko.dev/#/patchset/20260902-imx-se-if-v45-0-6e400879d8ec@nxp.com?part=3

> diff --git a/drivers/firmware/imx/ele_base_msg.c b/drivers/firmware/imx/ele_base_msg.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/firmware/imx/ele_base_msg.c

[ ... ]

> +int ele_get_info(struct se_if_priv *priv, struct ele_dev_info *s_info)
> +{
> +	dma_addr_t get_info_addr = 0;
> +	void *get_info_data = NULL;
> +	u32 get_info_len;
> +	int ret;
> +
> +	if (!priv)
> +		return -EINVAL;
> +
> +	memset(s_info, 0x0, sizeof(*s_info));
> +
> +	struct se_api_msg *tx_msg __free(kfree) =
> +		kzalloc(ELE_GET_INFO_REQ_MSG_SZ, GFP_KERNEL);
> +	if (!tx_msg)
> +		return -ENOMEM;
> +
> +	struct se_api_msg *rx_msg __free(kfree) =
> +		kzalloc(ELE_GET_INFO_RSP_MSG_SZ, GFP_KERNEL);
> +	if (!rx_msg)
> +		return -ENOMEM;
> +
> +	get_info_len = ELE_GET_INFO_BUFF_SZ;
> +	if (priv->mem_pool)
> +		get_info_data = gen_pool_dma_alloc(priv->mem_pool,
> +						   get_info_len,
> +						   &get_info_addr);
> +	else
> +		get_info_data = dma_alloc_coherent(priv->dev,
> +						   get_info_len,
> +						   &get_info_addr,
> +						   GFP_KERNEL);

[Severity: High]
Can these GFP_KERNEL allocations cause a system deadlock during suspend?

The commit message notes that ele_service_swap() was updated to use GFP_NOIO 
because it runs in the suspend/resume path, and direct memory reclaim could 
trigger I/O to a suspended storage device. 

However, it appears ele_get_info() is also executed synchronously in this 
exact same suspend/resume path:

> diff --git a/drivers/firmware/imx/ele_common.c b/drivers/firmware/imx/ele_common.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/firmware/imx/ele_common.c

[ ... ]

> +int se_save_imem_state(struct se_if_priv *priv, struct se_imem_buf *imem)
> +{
> +	struct ele_dev_info s_info = {0};
> +	int ret;
> +
> +	ret = ele_get_info(priv, &s_info);
> +	if (ret) {
> +		dev_err(priv->dev, "Failed to get info from ELE.\n");
> +		return ret;
> +	}

Since se_suspend() unconditionally calls se_save_imem_state(), which in turn 
calls ele_get_info(), won't the GFP_KERNEL allocations for tx_msg, rx_msg, 
and get_info_data bypass the fix you made in ele_service_swap() and still 
risk the same I/O reclaim deadlock?

Should these allocations in ele_get_info() also be changed to use GFP_NOIO 
to ensure the entire suspend/resume chain avoids I/O triggers?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903-imx-se-if-v46-0-aefaab525034@nxp.com?part=3