Re: [PATCH v2 2/2] i2c: imx-lpi2c: use dmaengine_prep_submit() to simple code

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [Medium] Incomplete implementation of the refactoring pattern intended by the commit.
- [Low] The fallback declaration for `dmaengine_prep_submit_slave_single` contains semicolons instead of commas, breaking the build when CONFIG_DMA_ENGINE is disabled.

Pre-existing issues:
- [Critical] A critical double free vulnerability can occur on `dma->rx_cmd_buf` in the error handling path of `lpi2c_imx_dma_xfer()`.
- [High] DMA channels requested during probe are never released on device removal or on a subsequent probe failure.
--

commit c9c1ee53a58bc638a612c603e4f9002874839ea5
Author: Frank Li <Frank.Li@nxp.com>

i2c: imx-lpi2c: use dmaengine_prep_submit() to simple code
    
Use dmaengine_prep_submit() to simple code. No functional change.

> Use dmaengine_prep_submit() to simple code. No functional change.

[Severity: Medium]
Is the refactoring incomplete here? The commit message states it simplifies the
code by using dmaengine_prep_submit(), but it only updates
lpi2c_dma_rx_cmd_submit().

An identical DMA preparation sequence exists just below it in lpi2c_dma_submit(),
which continues to manually call dmaengine_prep_slave_single() and
dmaengine_submit(). Should lpi2c_dma_submit() be updated as well for
consistency?

> diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
> index 2a0962a0b4417..c90f72eec8498 100644
> --- a/drivers/i2c/busses/i2c-imx-lpi2c.c
> +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
> @@ -733,15 +732,11 @@ static int lpi2c_dma_rx_cmd_submit(struct lpi2c_imx_struct *lpi2c_imx)
>  		return -EINVAL;
>  	}
>  
> -	rx_cmd_desc = dmaengine_prep_slave_single(txchan, dma->dma_tx_addr,
> -						  dma->rx_cmd_buf_len, DMA_MEM_TO_DEV,
> -						  DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
> -	if (!rx_cmd_desc) {
> -		dev_err(&lpi2c_imx->adapter.dev, "DMA prep slave sg failed, use pio\n");
> -		goto desc_prepare_err_exit;
> -	}
> -
> -	cookie = dmaengine_submit(rx_cmd_desc);
> +	cookie = dmaengine_prep_submit_slave_single(txchan, NULL, NULL,
> +						    dma->dma_tx_addr,
> +						    dma->rx_cmd_buf_len,
> +						    DMA_MEM_TO_DEV,
> +						    DMA_PREP_INTERRUPT | DMA_CTRL_ACK);

[Severity: Low]
Will this break the build when CONFIG_DMA_ENGINE is disabled?

In include/linux/dmaengine.h, the !CONFIG_DMA_ENGINE stub for
dmaengine_prep_submit_slave_single() contains semicolons instead of commas:

static inline dma_cookie_t
dmaengine_prep_submit_slave_single(struct dma_chan *chan,
				   dma_async_tx_callback cb, void *cb_param;
				   dma_addr_t buf, size_t len,
				   enum dma_transfer_direction dir,
				   unsigned long flags);
{
	return -ENODEV;
}

This syntax error will cause a compilation failure for any driver calling it
when CONFIG_DMA_ENGINE is not set.

>  	if (dma_submit_error(cookie)) {
>  		dev_err(&lpi2c_imx->adapter.dev, "submitting DMA failed, use pio\n");
>  		goto submit_err_exit;

[ ... ]

[Severity: Critical]
This isn't a bug introduced by this patch, but could this result in a double
free on dma->rx_cmd_buf in the error handling path of lpi2c_imx_dma_xfer()?

When a read transfer (I2C_M_RD) finishes, dma->rx_cmd_buf is freed at the
disable_dma label:

	if (dma->dma_msg_flag & I2C_M_RD)
		kfree(dma->rx_cmd_buf);

However, the pointer is not set to NULL. If a subsequent read transfer fails
early (e.g., if lpi2c_dma_config() or lpi2c_dma_submit() fails), the code jumps
to disable_dma before lpi2c_imx_alloc_rx_cmd_buf() can allocate a new buffer.

Since dma->dma_msg_flag & I2C_M_RD is true, wouldn't kfree() be called a second
time on the dangling pointer from the previous transfer?

[Severity: High]
This is a pre-existing issue, but are the DMA channels requested during probe
leaking on device removal or probe failure?

In lpi2c_dma_init(), the "tx" and "rx" DMA channels are acquired using the
non-managed dma_request_chan() API. However, the driver's remove function
lpi2c_imx_remove() does not call dma_release_channel() to free them.

Furthermore, if i2c_add_adapter() fails during lpi2c_imx_probe(), the error path
(rpm_disable) fails to release the DMA channels that were successfully acquired
moments earlier. Should these paths be updated to release the channels?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260522-dma_prep_submit-v2-0-7a87a5a29525@nxp.com?part=2