Re: [PATCH v2 03/12] i2c: nomadik: do not try to retransmit I2C message series on errors

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Lack of synchronization between process context timeout handling and the IRQ handler causes unprotected hardware access and Use-After-Free memory corruption.
- [High] Missing return value check for pm_runtime_get_sync() in nmk_i2c_xfer() / nmk_i2c_xfer_v2() allows hardware access while the device is suspended.
- [High] Wait queue head `xfer_wq` is incorrectly re-initialized per-transfer, risking data races with delayed interrupts.
- [High] flush_i2c_fifo() executes a massive, non-yielding busy-wait loop inside an IRQ handler, risking an NMI watchdog timeout.
--

commit 5d412f1d8843a1e97e53a226d012fcb387ee76b5
Author: Dmitry Guzman <Dmitry.Guzman@mobileye.com>
Subject: i2c: nomadik: do not try to retransmit I2C message series on errors

This commit modifies the i2c-nomadik driver to avoid retransmitting the
entire message series upon an error in the xfer callback. It removes the
triple retry loop to prevent hiding hardware faults and avoiding issues
with non-idempotent messages.

> diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
> index 45b4815b09452..ca9366e09e712 100644
> --- a/drivers/i2c/busses/i2c-nomadik.c
> +++ b/drivers/i2c/busses/i2c-nomadik.c
> @@ -716,27 +716,21 @@ static int nmk_i2c_xfer(struct i2c_adapter *i2c_adap,
>  	int status = 0;
>  	int i;
>  	struct nmk_i2c_dev *priv = i2c_get_adapdata(i2c_adap);
> -	int j;
>  
>  	pm_runtime_get_sync(&priv->adev->dev);
>  
> -	/* Attempt three times to send the message queue */
> -	for (j = 0; j < 3; j++) {
> -		/* setup the i2c controller */
> -		setup_i2c_controller(priv);

[Severity: High]
This isn't a regression introduced by this patch, but pm_runtime_get_sync()
can fail (e.g., if the clock fails to enable or the power domain is off).
Since this return value is not checked, does this unconditionally proceed to
access hardware registers via setup_i2c_controller() even if the device
failed to resume?

If the clock remains gated, could accessing the I/O memory cause a kernel
panic (synchronous external abort or bus fault)?

> -		for (i = 0; i < num_msgs; i++) {
[ ... ]

Looking at read_i2c() in drivers/i2c/busses/i2c-nomadik.c:

> static int read_i2c(struct nmk_i2c_dev *priv, u16 flags)
> {
[ ... ]
> 	/* enable the controller */
> 	i2c_set_bit(priv->virtbase + I2C_CR, I2C_CR_PE);
> 
> 	init_waitqueue_head(&priv->xfer_wq);
> 	priv->xfer_done = false;

[Severity: High]
This isn't a regression introduced by this patch, but init_waitqueue_head()
is called at the start of every transfer. Wait queues should typically be
initialized exactly once during device probe.

If a previous transfer timed out and its interrupt was not successfully
disabled (e.g., if flush_i2c_fifo() times out), could a delayed hardware
interrupt fire concurrently with the next transfer's initialization?

The delayed i2c_irq_handler() executes:

> 		priv->xfer_done = true;
> 		wake_up(&priv->xfer_wq);

Could wake_up() execute exactly while init_waitqueue_head() is
re-initializing the waitqueue's spinlock and list head on another CPU,
leading to spinlock and linked list corruption?

Looking at flush_i2c_fifo() in drivers/i2c/busses/i2c-nomadik.c:

> static int flush_i2c_fifo(struct nmk_i2c_dev *priv)
> {
> #define LOOP_ATTEMPTS 10
> 	ktime_t timeout;
> 	int i;
[ ... ]
> 	for (i = 0; i < LOOP_ATTEMPTS; i++) {
> 		timeout = ktime_add_us(ktime_get(), priv->timeout_usecs);
> 
> 		while (ktime_after(timeout, ktime_get())) {
> 			if ((readl(priv->virtbase + I2C_CR) &
> 				(I2C_CR_FTX | I2C_CR_FRX)) == 0)
> 				return 0;
> 		}
> 	}

[Severity: High]
This isn't a regression introduced by this patch, but flush_i2c_fifo() uses
a polling loop checking FIFO status using ktime_after() without any
cpu_relax() or sleep. 

Since flush_i2c_fifo() is called by init_hw(), which is triggered by the
i2c_irq_handler() on error paths (such as I2C_IT_MAL or I2C_IT_BERR),
is this blocking nested loop executing inside a hard IRQ handler context?

The timeout duration priv->timeout_usecs (defaulting to 200 ms) is nested
inside an outer loop running LOOP_ATTEMPTS (10) times. If the hardware is
stuck, could the IRQ handler continuously spin in hard IRQ context for
2 seconds, triggering a system hard lockup and NMI watchdog panic?

Looking at nmk_i2c_xfer_one() in drivers/i2c/busses/i2c-nomadik.c:

> static int nmk_i2c_xfer_one(struct nmk_i2c_dev *priv, u16 flags)
> {
[ ... ]
> 	if (status || priv->result) {
> 		u32 i2c_sr;
> 		u32 cause;
[ ... ]
> 		init_hw(priv);
> 
> 		status = status ? status : priv->result;
> 	}
> 
> 	return status;
> }

[Severity: Critical]
This isn't a regression introduced by this patch, but if an I2C transfer
times out in wait_event_timeout() within read_i2c(), process context
attempts recovery by calling init_hw() without any lock to serialize against
the IRQ handler.

Looking at init_hw():

> static int init_hw(struct nmk_i2c_dev *priv)
> {
> 	int stat;
> 
> 	stat = flush_i2c_fifo(priv);
> 	if (stat)
> 		goto exit;
> 
> 	/* disable the controller */
> 	i2c_clr_bit(priv->virtbase + I2C_CR, I2C_CR_PE);
> 
> 	disable_all_interrupts(priv);

If flush_i2c_fifo() also times out, it exits early via goto exit and skips
disabling the controller and its interrupts. 

When the process returns -ETIMEDOUT to the I2C core, the user-provided
transfer buffer is freed. Since the hardware and interrupts were left
enabled, could a delayed interrupt later complete the transaction and fire?

The IRQ handler executes locklessly and enters the Rx interrupt case:

> 	case I2C_IT_RXFNF:
> 		for (count = rft; count > 0; count--) {
> 			/* Read the Rx FIFO */
> 			*priv->cli.buffer = nmk_i2c_readb(priv, I2C_RFR);
> 			priv->cli.buffer++;
> 		}

Could this dereference the stale priv->cli.buffer, resulting in a
use-after-free and memory corruption by writing received hardware data
directly into freed memory?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903-i2c-fault-reporting-v2-0-fedeb91792e6@mobileye.com?part=3