Re: [PATCH v4 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() overwrites bus command errors with 0
--

commit cc96ce79094e98763abdf6d3ca4ab22eeb1119f1
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 using the
I2C_BUS_CLEAR and I2C_STOP_ON_BUS hardware opcodes when SDA is held low.
It automatically triggers recovery from geni_i2c_xfer() on -EPROTO,
-ETIMEDOUT, and -EAGAIN errors and registers geni_i2c_recover_bus() via
i2c_bus_recovery_info.

> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> index 60b6273f538c2..b08353616adc8 100644
> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
[ ... ]
> @@ -1013,6 +1015,112 @@ 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)
> +{
[ ... ]
> +	/*
> +	 * 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.
> +	 */
> +	ret = geni_i2c_bus_is_free(gi2c) ? 0 : (ret ?: -EBUSY);

[Severity: Medium]
As reported by Sashiko on v3 [1], can this overwrite a real command error
or timeout in ret with 0?

In geni_i2c_fifo_bus_cmd(), expected transient recovery errors (-EAGAIN
and -EPROTO) are already cleared to 0, so a non-zero return value
indicates a command timeout (-ETIMEDOUT) or a controller error such as
-EIO.

Once geni_i2c_fifo_bus_cmd(gi2c, I2C_BUS_CLEAR) clocks SCL until the
target releases SDA, both SDA and SCL are high in SE_GENI_IOS, so
geni_i2c_bus_is_free(gi2c) is already true before I2C_STOP_ON_BUS runs.
If geni_i2c_fifo_bus_cmd(gi2c, I2C_STOP_ON_BUS) then fails or times out,
or if I2C_BUS_CLEAR itself times out after advancing the target to a high
data bit (skipping I2C_STOP_ON_BUS), evaluating geni_i2c_bus_is_free(gi2c)
first unconditionally overwrites the negative error in ret with 0 even
though no STOP condition was generated on the bus.

Note that a subsequent patch in the series (commit 8e94719d9d80, "i2c:
qcom-geni: Add bus recovery support for GPI DMA mode") adds explicit
if (ret == -ETIMEDOUT) goto out_pm checks in the GPI DMA branch, but
leaves this FIFO branch unchanged. Should this preserve non-zero ret values
and only check geni_i2c_bus_is_free(gi2c) when the commands succeeded (for
example, if (!ret && !geni_i2c_bus_is_free(gi2c)) ret = -EBUSY;)?

[1] https://lore.kernel.org/r/20261027-20260826_aniket_randive_oss_qualcomm_com-v3-2-7f2a6e572de9@oss.qualcomm.com

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103-20260826_aniket_randive_oss_qualcomm_com-v4-0-59b3ce96dc4b@oss.qualcomm.com?part=2