Re: [PATCH v2 3/3] i2c: qcom-geni: Add bus recovery support for GPI DMA mode

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] i2c: qcom-geni: unreachable -EAGAIN/-EPROTO check in geni_i2c_gpi_bus_cmd()
- [Low] i2c: qcom-geni: misplaced XFER_TIMEOUT fix in geni_i2c_fifo_bus_cmd()
--

commit 0f096af4f3272f27200e96084308f4f7a864dd9b
Author: Aniket Randive <aniket.randive@oss.qualcomm.com>

i2c: qcom-geni: Add bus recovery support for GPI DMA mode

This patch adds I2C bus recovery support in GPI DMA mode by issuing
I2C_BUS_CLEAR and I2C_STOP_ON_BUS commands over the GPI TX DMA channel,
and checks SE_GENI_IOS I2C_BUS_FREE on -EIO completions before triggering
recovery. It also updates geni_i2c_fifo_bus_cmd() to use gi2c->adap.timeout.

> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> index 065d7240de04d..31f811bdc0513 100644
> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
> @@ -1034,7 +1034,7 @@ static int geni_i2c_fifo_bus_cmd(struct geni_i2c_dev *gi2c, u32 cmd)
>  	reinit_completion(&gi2c->done);
>  
>  	geni_se_setup_m_cmd(&gi2c->se, cmd, 0);
> -	time_left = wait_for_completion_timeout(&gi2c->done, XFER_TIMEOUT);
> +	time_left = wait_for_completion_timeout(&gi2c->done, gi2c->adap.timeout);
>  	if (!time_left) {
>  		dev_err(gi2c->se.dev, "timeout waiting for bus cmd %u\n", cmd);
>  		geni_i2c_abort_xfer(gi2c);

[Severity: Low]
Earlier in the history, commit "i2c: qcom-geni: Add dynamic transfer timeout
based on transfer length and frequency" removed the XFER_TIMEOUT macro from
drivers/i2c/busses/i2c-qcom-geni.c in favor of gi2c->adap.timeout.

The preceding patch in this series, "i2c: qcom-geni: Add bus recovery
support for FIFO mode", introduced geni_i2c_fifo_bus_cmd() using XFER_TIMEOUT,
which causes a build failure with CONFIG_I2C_QCOM_GENI enabled at that commit
before this hunk replaces it with gi2c->adap.timeout.

Should this change in geni_i2c_fifo_bus_cmd() be squashed into "i2c:
qcom-geni: Add bus recovery support for FIFO mode" to preserve git
bisectability across the series?

> @@ -1065,13 +1065,82 @@ static bool geni_i2c_needs_bus_recovery(struct geni_i2c_dev *gi2c, int ret)

[ ... ]

> +static int geni_i2c_gpi_bus_cmd(struct geni_i2c_dev *gi2c, enum i2c_op cmd)
> +{

[ ... ]

> +	time_left = wait_for_completion_timeout(&gi2c->done, gi2c->adap.timeout);
> +	if (!time_left) {
> +		dev_err(gi2c->se.dev, "timeout waiting for GPI bus cmd %u\n", cmd);
> +		dmaengine_terminate_sync(gi2c->tx_c);
> +		return -ETIMEDOUT;
> +	}
> +
> +	/* ARB_LOST and BUS_PROTO may be expected during recovery; treat as success */
> +	if (gi2c->err == -EAGAIN || gi2c->err == -EPROTO)
> +		return 0;
> +
> +	return gi2c->err;
> +}

[Severity: Low]
Can gi2c->err ever be -EAGAIN or -EPROTO here in geni_i2c_gpi_bus_cmd()?

When gi2c->gpi_mode is enabled, setup_gpi_dma() selects GENI_GPI_DMA mode,
which disables SE_IRQ_EN and routes completion events through the GPI DMA
callback i2c_gpi_cb_result() rather than geni_i2c_irq().

In drivers/i2c/busses/i2c-qcom-geni.c, i2c_gpi_cb_result() sets gi2c->err
only to -EIO on transfer failure:

	if (result->result != DMA_TRANS_NOERROR) {
		dev_err(gi2c->se.dev, "DMA txn failed:%d\n", result->result);
		gi2c->err = -EIO;
	}

As noted in the comments in geni_i2c_needs_bus_recovery() and
geni_i2c_recover_bus(), GPI mode reports NACK, BUS_PROTO, and ARB_LOST alike
as -EIO and cannot distinguish benign recovery events on the opcode return
path. Is this -EAGAIN and -EPROTO check in geni_i2c_gpi_bus_cmd() unreachable
code left over from geni_i2c_fifo_bus_cmd()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261023-20260826_aniket_randive_oss_qualcomm_com-v2-0-93572b453d6f@oss.qualcomm.com?part=3