Re: [PATCH v1 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 2 potential issue(s) to consider:

New issues:
- [High] Compilation error and broken synchronization logic in the command abort path.

Pre-existing issues:
- [High] A NACK interrupt during bus recovery causes a fatal NULL pointer dereference in the interrupt handler.
--

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

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

This commit adds bus recovery support using I2C_BUS_CLEAR and
I2C_STOP_ON_BUS hardware opcodes to restore a stuck bus to an idle
state in FIFO mode. It automatically triggers recovery on bus errors
and registers a recovery callback via i2c_bus_recovery_info.

> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> +++ b/drivers/i2c/busses/i2c-qcom-geni.c

[ ... ]

> @@ -956,6 +957,90 @@ 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);

[Severity: High]
This is a pre-existing issue, but by explicitly clearing gi2c->cur here,
can a NACK interrupt during bus recovery cause a NULL pointer dereference
in the interrupt handler?

If a NACK interrupt occurs during this sequence, geni_i2c_irq() routes
it to geni_i2c_check_addr_data_nack() which unconditionally checks the
flags on the current message:

drivers/i2c/busses/i2c-qcom-geni.c:geni_i2c_check_addr_data_nack() {
    ...
    else if (!(gi2c->cur->flags & I2C_M_RD))
        geni_i2c_err(gi2c, DATA_NACK);
    ...
}

> +	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, XFER_TIMEOUT);
> +	if (!time_left) {
> +		dev_err(gi2c->se.dev, "timeout waiting for bus cmd %u\n", cmd);
> +		gi2c->abort_done = false;
> +		geni_se_abort_m_cmd(&gi2c->se);
> +		time_left = ABORT_TIMEOUT;
> +		do {
> +			time_left = wait_for_completion_timeout(&gi2c->done, time_left);
> +		} while (!gi2c->abort_done && time_left);

[Severity: High]
Is gi2c->abort_done declared as a struct completion? If so, assigning false
to it and checking it with a logical NOT will cause a compilation error.

Additionally, does this wait on the correct completion variable? The loop
calls wait_for_completion_timeout(&gi2c->done, time_left) but then checks
!gi2c->abort_done. It appears it should wait on &gi2c->abort_done instead.

Lastly, does the hardware abort command geni_se_abort_m_cmd() need to be
issued while holding gi2c->lock to prevent racing with the concurrent
interrupt handler?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260826-i2c_bus_recovery-v1-0-203f0ec76f84@oss.qualcomm.com?part=2