Re: [PATCH v3 2/3] i2c: qcom-geni: Add bus recovery support for FIFO mode

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] i2c: qcom-geni: geni_i2c_recover_bus() clobbers bus command errors with 0
--

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

i2c: qcom-geni: Add bus recovery support for FIFO mode

This commit adds I2C bus recovery support for FIFO mode in the Qualcomm GENI
I2C driver using the I2C_BUS_CLEAR and I2C_STOP_ON_BUS hardware opcodes. It
triggers recovery automatically in geni_i2c_xfer() when a transfer fails with
-EPROTO, -ETIMEDOUT, or -EAGAIN while the bus is not free, and registers the
recovery callback in i2c_bus_recovery_info.

> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> index 60b6273f538c2..d9e86f110ae9b 100644
> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> +++ b/drivers/i2c/busses/i2c-qcom-geni.c

[ ... ]

> @@ -1013,6 +1015,115 @@ static int geni_i2c_fifo_xfer(struct geni_i2c_dev *gi2c,
>  	return num;
>  }
>  
> +static int geni_i2c_fifo_bus_cmd(struct geni_i2c_dev *gi2c, u32 cmd)
> +{
> +	unsigned long time_left;
> +	unsigned long flags;
> +
> +	/*
> +	 * Clear cur so the IRQ handler does not attempt FIFO watermark
> +	 * filling or draining while the recovery opcode is in flight.
> +	 * cur and err are shared with geni_i2c_irq(), which reads cur and
> +	 * writes err under gi2c->lock, so take the lock around this reset.
> +	 */
> +	spin_lock_irqsave(&gi2c->lock, flags);
> +	gi2c->cur = NULL;
> +	gi2c->err = 0;
> +	spin_unlock_irqrestore(&gi2c->lock, flags);
> +	geni_se_select_mode(&gi2c->se, GENI_SE_FIFO);
> +	reinit_completion(&gi2c->done);
> +
> +	geni_se_setup_m_cmd(&gi2c->se, cmd, 0);
> +	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);
> +
> +		return -ETIMEDOUT;
> +	}
> +
> +	/*
> +	 * ARB_LOST and BUS_PROTO interrupts may be reported while the bus
> +	 * transitions from stuck to idle during the recovery sequence.
> +	 * The opcode completed successfully so treat these as success.
> +	 */
> +	if (gi2c->err == -EAGAIN || gi2c->err == -EPROTO)
> +		return 0;
> +
> +	return gi2c->err;
> +}

[ ... ]

> +static int geni_i2c_recover_bus(struct i2c_adapter *adap)
> +{

[ ... ]

> +	ret = geni_i2c_fifo_bus_cmd(gi2c, I2C_BUS_CLEAR);
> +	if (!ret)
> +		ret = geni_i2c_fifo_bus_cmd(gi2c, I2C_STOP_ON_BUS);
> +
> +	/*
> +	 * Recovery succeeds only once both bus lines are released. The bus
> +	 * state is the authority, while a clean opcode status with either
> +	 * line low is a failed recovery.
> +	 */
> +	if (geni_i2c_bus_is_free(gi2c))
> +		ret = 0;
> +	else if (!ret)
> +		ret = -EBUSY;

[Severity: Medium]
Can this check in geni_i2c_recover_bus() overwrite a genuine command error or
timeout stored in ret with 0?

In geni_i2c_fifo_bus_cmd(), expected transient recovery errors (-EAGAIN and
-EPROTO) are already converted to 0 upon completion, so a non-zero return
value indicates that the recovery opcode failed or timed out.

When geni_i2c_fifo_bus_cmd(gi2c, I2C_BUS_CLEAR) succeeds, the target has
released SDA and both SDA (RX_DATA_IN) and SCL (IO2_DATA_IN) read high,
meaning geni_i2c_bus_is_free(gi2c) is already true. If the subsequent
geni_i2c_fifo_bus_cmd(gi2c, I2C_STOP_ON_BUS) call fails or times out,
geni_i2c_bus_is_free(gi2c) will still evaluate to true and clobber the error
in ret with 0, leaving the bus without a transmitted stop condition while
reporting a successful recovery to callers of i2c_recover_bus().

Similarly, if I2C_BUS_CLEAR itself times out or fails while SDA happens to be
high, I2C_STOP_ON_BUS is skipped by if (!ret), and the geni_i2c_bus_is_free()
check discards the error and returns 0.

Should geni_i2c_recover_bus() preserve non-zero errors from
geni_i2c_fifo_bus_cmd() and only check !geni_i2c_bus_is_free(gi2c) when ret
is 0?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261027-20260826_aniket_randive_oss_qualcomm_com-v3-0-7f2a6e572de9@oss.qualcomm.com?part=2