Re: [PATCH v2] i2c: imx: add I2C_M_REV_DIR_ADDR and I2C_M_NOSTART

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] 0-length standard I2C reads cause an integer underflow, leading to an infinite read loop and catastrophic kernel buffer overflow or NULL pointer dereference.
- [High] DMA mappings are leaked on the error path if wait_for_completion_timeout() times out.
--

--- Patch [1]: [PATCH v2] i2c: imx: add I2C_M_REV_DIR_ADDR and I2C_M_NOSTART ---
commit d4f0c04249e7ead7a9fd8aedf2465f7f1895d284
Author: Vincent Jardin <vjardin@free.fr>
Subject: i2c: imx: add I2C_M_REV_DIR_ADDR and I2C_M_NOSTART

This patch adds support for I2C_M_REV_DIR_ADDR and I2C_M_NOSTART flags to the
i2c-imx driver. It allows inverting the transmitted R/W bit on a write message
and continuing a frame without re-addressing on subsequent reads, which is
needed for devices like the Realtek RTL8366SE SMI switch.

Link: https://lore.kernel.org/r/20260807-for-upstream-i2c-imx-lx2160-reverse-v1-1-f276ce8cb660@free.fr

> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index 9477d814fde94..7dc9c43673eee 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
[ ... ]
> @@ -1000,16 +1000,10 @@ static inline int i2c_imx_isr_write(struct imx_i2c_struct *i2c_imx)
>  	return 1;
>  }
>  
> -static inline int i2c_imx_isr_read(struct imx_i2c_struct *i2c_imx)
> +static inline void i2c_imx_setup_read(struct imx_i2c_struct *i2c_imx)
>  {
> -	int result;
>  	unsigned int temp;
>  
> -	result = i2c_imx_isr_acked(i2c_imx);
> -	if (result)
> -		return result;
> -
> -	/* setup bus to read data */
>  	temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
>  	temp &= ~I2CR_MTX;
>  	if ((i2c_imx->msg->len - 1) || (i2c_imx->msg->flags & I2C_M_RECV_LEN))

[Severity: Critical]
This is a pre-existing issue, but does a 0-length standard I2C read cause an
integer underflow here?

When processing a standard 0-length I2C read message, such as I2C_SMBUS_QUICK
with read intent, i2c_imx->msg->len is 0. Does subtracting 1 trigger an
underflow evaluating to 0xFFFF?

If so, does this cause the driver to incorrectly clear the I2CR_TXAK flag
and instruct the hardware to ACK instead of NACK, leading to an infinite read
loop in i2c_imx_isr_read_continue() where msg_buf_idx increments indefinitely?

> @@ -1017,6 +1011,18 @@ static inline int i2c_imx_isr_read(struct imx_i2c_struct *i2c_imx)
[ ... ]
> @@ -1200,7 +1216,7 @@ static int i2c_imx_dma_write(struct imx_i2c_struct *i2c_imx,
>  	 * Write slave address.
>  	 * The first byte must be transmitted by the CPU.
>  	 */
> -	imx_i2c_write_reg(i2c_8bit_addr_from_msg(msgs), i2c_imx, IMX_I2C_I2DR);
> +	imx_i2c_write_reg(i2c_imx_addr_byte(msgs), i2c_imx, IMX_I2C_I2DR);
>  	time_left = wait_for_completion_timeout(
>  				&i2c_imx->dma->cmd_complete,
>  				msecs_to_jiffies(DMA_TIMEOUT));

[Severity: High]
This isn't a bug introduced by this patch, but do the error paths after this
timeout leak DMA mappings?

In i2c_imx_dma_write() and i2c_imx_dma_read(), the buffer is mapped for
DMA inside i2c_imx_dma_xfer(). If wait_for_completion_timeout() returns 0,
the code calls dmaengine_terminate_sync() and returns -ETIMEDOUT without
explicitly unmapping the buffer.

Since dmaengine_terminate_sync() does not invoke the DMA completion callback
where dma_unmap_single() normally resides, does this permanently leak the
mapping and IOMMU resources if the I2C bus hangs or times out?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260812-for-upstream-i2c-imx-lx2160-reverse-v2-1-f1343714c5a9@free.fr?part=1