Re: [PATCH 3/3] i2c: omap: add DMA support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] i2c: omap: stall and -ETIMEDOUT on bus error in omap_i2c_xfer_msg()
- [Medium] i2c: omap: DMA channel leak in omap_i2c_probe() on adapter add error
- [Medium] i2c: omap: active PIO FIFO interrupts during DMA in omap_i2c_xfer_msg()
--

commit 4941414830d919c66f3a4ea17fab3a4dcf5a305e
Author: Antoni Pokusinski <apokusinski01@gmail.com>

i2c: omap: add DMA support

This commit adds TX and RX DMA support to the OMAP I2C controller driver for
messages whose length is at least 8 bytes and a multiple of the configured
FIFO threshold.

> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index f02d294db42a6..59e633c80236b 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c

[ ... ]

> @@ -742,8 +928,17 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
>  	 * into arbitration and we're currently unable to recover from it.
>  	 */
>  	if (!polling) {
> -		time_left = wait_for_completion_timeout(&omap->cmd_complete,
> -							OMAP_I2C_TIMEOUT);
> +		time_left = OMAP_I2C_TIMEOUT;
> +
> +		if (use_dma)
> +			time_left = wait_for_completion_timeout(
> +						&omap->dma->complete,
> +						time_left);
> +
> +		if (time_left)
> +			time_left = wait_for_completion_timeout(
> +						&omap->cmd_complete,
> +						time_left);

[Severity: High]
What happens in omap_i2c_xfer_msg() in drivers/i2c/busses/i2c-omap.c if an
I2C bus error such as a NACK (OMAP_I2C_STAT_NACK) or arbitration loss
(OMAP_I2C_STAT_AL) occurs during a DMA transfer?

When the target device NACKs or arbitration is lost before all msg->len bytes
are transferred, the controller stops generating FIFO DMA requests, so
omap_i2c_dma_callback() will not be invoked to complete omap->dma->complete.

Meanwhile, the interrupt handler path:

omap_i2c_isr_thread()
  -> omap_i2c_xfer_data()
  -> omap_i2c_complete_cmd()

records the error in omap->cmd_err and signals omap->cmd_complete, but does
not signal omap->dma->complete:

static inline void
omap_i2c_complete_cmd(struct omap_i2c_dev *omap, u16 err)
{
	omap->cmd_err |= err;
	complete(&omap->cmd_complete);
}

Won't waiting on omap->dma->complete first cause omap_i2c_xfer_msg() to stall
for the full OMAP_I2C_TIMEOUT, reset the controller, and return -ETIMEDOUT
instead of handling omap->cmd_err and returning -EREMOTEIO, -EAGAIN, or 0
(for I2C_M_IGNORE_NAK)?

>  	} else {
>  		do {
>  			omap_i2c_wait(omap);
> @@ -753,6 +948,15 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
>  		time_left = !ret;
>  	}
>  
> +	if (use_dma) {
> +		omap_i2c_dma_disable_req(omap);
> +
> +		if (!time_left || omap->cmd_err)
> +			dmaengine_terminate_sync(omap->dma->chan_using);
> +
> +		omap_i2c_dma_cleanup(omap, time_left && !omap->cmd_err);
> +	}

[Severity: Medium]
Could disabling DMA requests here race with the PIO interrupt handler in
drivers/i2c/busses/i2c-omap.c?

During a DMA transfer, omap_i2c_xfer_msg() still sets omap->buf = msg->buf
and omap->buf_len = msg->len, and OMAP_I2C_IE_XRDY, OMAP_I2C_IE_RRDY,
OMAP_I2C_IE_XDR, and OMAP_I2C_IE_RDR remain enabled in OMAP_I2C_IE_REG.

If a DMA transfer times out or fails while the controller is still enabled
(prior to omap_i2c_reset()), omap_i2c_dma_disable_req() clears
OMAP_I2C_BUF_RDMA_EN or OMAP_I2C_BUF_XDMA_EN in OMAP_I2C_BUF_REG. Once the
DMA request bit is cleared in OMAP_I2C_BUF_REG, can pending FIFO threshold
conditions trigger XRDY/RRDY/XDR/RDR interrupts and cause
omap_i2c_isr_thread() -> omap_i2c_xfer_data() to read or write
OMAP_I2C_DATA_REG and omap->buf concurrently with omap_i2c_dma_cleanup() and
omap_i2c_reset()?

[ ... ]

> @@ -1478,6 +1742,22 @@ omap_i2c_probe(struct platform_device *pdev)
>  		goto err_unuse_clocks;
>  	}
>  
> +	if (omap->scheme != OMAP_I2C_SCHEME_0) {
> +		omap->dma = omap_i2c_dma_init(omap, (dma_addr_t)res->start);
> +		if (PTR_ERR(omap->dma) == -EPROBE_DEFER) {
> +			r = -EPROBE_DEFER;
> +			dev_err_probe(&pdev->dev, r,
> +				      "can't get DMA channels\n");
> +			goto err_unuse_clocks;
> +		} else if (PTR_ERR(omap->dma) == -ENODEV) {
> +			dev_dbg(omap->dev, "DMA setup failed\n");
> +			omap->dma = NULL;
> +		} else if (IS_ERR(omap->dma)) {
> +			r = PTR_ERR(omap->dma);
> +			goto err_unuse_clocks;
> +		}
> +	}
> +
>  	adap = &omap->adapter;
>  	i2c_set_adapdata(adap, omap);
>  	adap->owner = THIS_MODULE;

[Severity: Medium]
Does omap_i2c_probe() in drivers/i2c/busses/i2c-omap.c leak dma->chan_tx and
dma->chan_rx if i2c_add_numbered_adapter() fails?

omap_i2c_dma_init() acquires dma->chan_tx and dma->chan_rx using
dma_request_chan(). Later in omap_i2c_probe():

	r = i2c_add_numbered_adapter(adap);
	if (r)
		goto err_unuse_clocks;

The err_unuse_clocks error path does not call omap_i2c_dma_free(omap) when
omap->dma is set:

err_unuse_clocks:
	omap_i2c_write_reg(omap, OMAP_I2C_CON_REG, 0);
err_put_pm:
	pm_runtime_put_sync(omap->dev);
err_disable_pm:
	pm_runtime_dont_use_autosuspend(omap->dev);
	pm_runtime_disable(&pdev->dev);

	return r;

Should the error unwind path release the DMA channels with
omap_i2c_dma_free(omap) when omap->dma is non-NULL?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105200556.34240-2-apokusinski01@gmail.com?part=3