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 <[email protected]> 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/[email protected]?part=3
