Re: [PATCH v28 4/7] firmware: imx: device context dedicated to priv

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Data race in `ele_msg_send_rcv()` when initializing `waiting_rsp_clbk_hdl` state, leading to a potential NULL pointer dereference in the RX interrupt handler.
- [Low] Unused variable `flags` in `ele_msg_send_rcv()`.
--

--- Patch [4]: [PATCH v28 4/7] firmware: imx: device context dedicated to priv ---
commit 64e554919e156aa88c2007ef9dd9fafde735e583
Author: Pankaj Gupta <pankaj.gupta@nxp.com>

firmware: imx: device context dedicated to priv

Add priv_dev_ctx to prepare enabling misc-device context based send-receive
path, to communicate with FW. No functionality change.

Closes: https://sashiko.dev/#/patchset/20260715-imx-se-if-v27-0-bb7c45952f06@nxp.com?part=4

> diff --git a/drivers/firmware/imx/ele_common.c b/drivers/firmware/imx/ele_common.c
> index ca8a32a82689b..1ca22c53b55c4 100644
> --- a/drivers/firmware/imx/ele_common.c
> +++ b/drivers/firmware/imx/ele_common.c

[ ... ]

> @@ -149,34 +151,38 @@ static void ele_msg_send_rcv_cleanup(struct se_if_priv *priv)
>  	unsigned long flags;
>  
>  	spin_lock_irqsave(&priv->waiting_rsp_clbk_hdl.clbk_rx_lock, flags);
> +	priv->waiting_rsp_clbk_hdl.dev_ctx = NULL;
>  	priv->waiting_rsp_clbk_hdl.rx_msg = NULL;
>  	priv->waiting_rsp_clbk_hdl.rx_msg_sz = 0;
>  	spin_unlock_irqrestore(&priv->waiting_rsp_clbk_hdl.clbk_rx_lock, flags);
>  }
>  
>  /* API used for send/receive blocking call. */
> -int ele_msg_send_rcv(struct se_if_priv *priv, void *tx_msg, int tx_msg_sz,
> -		     void *rx_msg, int exp_rx_msg_sz)
> +int ele_msg_send_rcv(struct se_if_device_ctx *dev_ctx, void *tx_msg,
> +		     int tx_msg_sz, void *rx_msg, int exp_rx_msg_sz)
>  {
> +	struct se_if_priv *priv = dev_ctx->priv;
> +	unsigned long flags;

[Severity: Low]
Is this flags variable used anywhere in this function?

It appears to be declared but never utilized in the function scope, which
will trigger a -Wunused-variable compiler warning.

>  	int err;
>  
>  	guard(mutex)(&priv->se_if_cmd_lock);
>  
>  	if (atomic_read(&priv->fw_busy)) {
> -		dev_dbg(priv->dev, "ELE became unresponsive.\n");
> +		dev_dbg(priv->dev, "%s: ELE became unresponsive.\n", dev_ctx->devname);
>  		return -EBUSY;
>  	}
>  	reinit_completion(&priv->waiting_rsp_clbk_hdl.done);
> +	priv->waiting_rsp_clbk_hdl.dev_ctx = dev_ctx;
>  	priv->waiting_rsp_clbk_hdl.rx_msg_sz = exp_rx_msg_sz;
>  	priv->waiting_rsp_clbk_hdl.rx_msg = rx_msg;

[Severity: High]
Could these lockless updates result in a data race with the interrupt
handler?

They are updated here without acquiring clbk_rx_lock. If a delayed firmware
response from a previous command arrives concurrently, the interrupt handler
se_if_rx_callback() will execute.

Due to CPU reordering or compiler optimizations, can the interrupt handler
observe the new rx_msg pointer before the new dev_ctx pointer becomes
visible?

> -	err = ele_msg_send(priv, tx_msg, tx_msg_sz);
> +	err = ele_msg_send(dev_ctx, tx_msg, tx_msg_sz);

[ ... ]

> @@ -302,8 +310,8 @@ void se_if_rx_callback(struct mbox_client *mbox_cl, void *msg)
>  			dev_info(dev, "ELE responded (late), recovery FW available.");
>  			return;
>  		}
> -		dev_dbg(dev, "Selecting resp waiter: for mesg header:0x%x.",
> -			*(u32 *)header);
> +		dev_dbg(dev, "Selecting resp waiter:%s for mesg header:0x%x.",
> +			se_clbk_hdl->dev_ctx->devname, *(u32 *)header);

If the interrupt handler sees a non-NULL rx_msg, it assumes the transaction
is active and attempts to access se_clbk_hdl->dev_ctx->devname here. If
dev_ctx is still NULL from the previous cleanup, won't this result in a NULL
pointer dereference?

>  
>  		/*
>  		 * For rsp_tag traffic, the sender provides the expected response

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260717-imx-se-if-v28-0-0a9659c7e69d@nxp.com?part=4