Re: [PATCH v2 07/12] i2c: nomadik: add support for I2C_XFER_V2 - detailed fault reporting

[sashiko-bot@kernel.org]

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

New issues:
- [High] The migration to the I2C_XFER_V2 API silently removed the hardcoded 3-attempt retry loop for transfers, breaking the driver's resilience against transient bus errors.
- [High] Removing `.xfer` from `i2c_algorithm` breaks downstream subsystems (like I2C multiplexers) that strictly check `.master_xfer`.
- [High] Heap out-of-bounds write in `i2cdev_ioctl_rdwr_v2` when updating the length of a partially transferred message.
- [High] Unconditional memory leak of I2C message buffers upon transfer failure in `i2cdev_ioctl_rdwr_v2`.
- [Medium] Unprotected lockless read of `priv->cli.xfer_bytes` in process context races with the hardirq handler.
- [Medium] Interrupt handler resets controller state before process context can read it, defeating the new byte-level reporting.

Pre-existing issues:
- [High] Missing return value check for pm_runtime_get_sync(), which can lead to a kernel crash if resume fails.
- [High] Missing synchronize_irq() in the timeout path leads to a Use-After-Free of message buffers by the interrupt handler.
--

commit 6e00e021c9abe420f0965de298c6d92c4d01241e
Author: Dmitry Guzman <Dmitry.Guzman@mobileye.com>

i2c: nomadik: add support for I2C_XFER_V2 - detailed fault reporting

I2C_XFER_V2 is a new API that allows I2C clients to get the detailed
report in case of transmission failure. Previously, the only information
returned by I2C bus controller was the error code, without exposing
how many messages or bytes had been sent. This commit introduces support
of this feature in the i2c-nomadik driver.

> diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
> index c36e9961e0a39..8b0ab191f7c1f 100644
> --- a/drivers/i2c/busses/i2c-nomadik.c
> +++ b/drivers/i2c/busses/i2c-nomadik.c
[ ... ]
> @@ -678,6 +683,16 @@ static int nmk_i2c_xfer_one(struct nmk_i2c_dev *priv, u16 flags)
>  			status = priv->result;
>  		}
>  
> +		if (flags & I2C_M_RD) {
> +			/* For READ messages, return the number of bytes read from FIFO */
> +			priv->bytes_cplt = priv->cli.xfer_bytes;

[Severity: Medium]
Is it safe to perform an unprotected lockless read of priv->cli.xfer_bytes
here? If the interrupt handler updates priv->cli.xfer_bytes concurrently on
an error or timeout path, could this regression cause the process context to
read a stale or inconsistent value?

> +		} else {
> +			/* For WRITE messages, return the number of bytes sent on bus */
> +			priv->bytes_cplt = FIELD_GET(I2C_SR_LENGTH, i2c_sr);

[Severity: Medium]
When an I2C abort error occurs, the interrupt handler calls init_hw(),
which resets the I2C_SR register before process context can read it.
Does this race condition cause the newly introduced byte-level error
reporting to silently fail by returning 0 for bytes_cplt and missing
the abort cause printout?

> +			/* LENGTH value includes the last byte that has not been sent or ACKed */
> +			if (priv->bytes_cplt > 0)
> +				priv->bytes_cplt--;
> +		}
>  		init_hw(priv);

[Severity: High]
This is a pre-existing issue and was not introduced by the patch under
review, but missing synchronize_irq() in the timeout path leads to a
use-after-free. If a timeout occurs during an I2C transfer, this code path
frees the message buffer, but the interrupt handler could fire concurrently
on another CPU:

i2c_irq_handler()
    *priv->cli.buffer = nmk_i2c_readb(priv, I2C_RFR);

Since init_hw() calls disable_all_interrupts() which only masks the IP block
but does not wait for running handlers, can this cause memory corruption by
writing to the concurrently freed buffer?

>  
>  		status = status ? status : priv->result;
[ ... ]
> @@ -733,14 +749,16 @@ static int nmk_i2c_xfer_one(struct nmk_i2c_dev *priv, u16 flags)
>   * please use the i2c_smbus_read_i2c_block_data()
>   * or i2c_smbus_write_i2c_block_data() API
>   */
> -static int nmk_i2c_xfer(struct i2c_adapter *i2c_adap,
> -		struct i2c_msg msgs[], int num_msgs)
> +static int nmk_i2c_xfer_v2(struct i2c_adapter *i2c_adap,
> +		struct i2c_msg msgs[], int num_msgs,
> +		struct i2c_transfer_report *report)
>  {

[Severity: High]
The migration to the I2C_XFER_V2 API silently removed the hardcoded 3-attempt
retry loop for transfers. Does this regression break the driver's resilience
against transient bus errors like lost arbitration or NACKs, causing failed
I2C transfers for device drivers?

>  	int status = 0;
>  	int i;
>  	struct nmk_i2c_dev *priv = i2c_get_adapdata(i2c_adap);
>  
>  	pm_runtime_get_sync(&priv->adev->dev);

[Severity: High]
This is a pre-existing issue and was not introduced by the patch under
review, but the return value of pm_runtime_get_sync() is unchecked here. If
the power domain fails to resume and returns an error, execution continues
to setup_i2c_controller(), which could lead to a kernel panic from accessing
unclocked or unpowered registers. Should we check the return value before
proceeding?

> +	priv->bytes_cplt = 0;
>  
>  	/* setup the i2c controller */
[ ... ]
> @@ -1014,7 +1039,7 @@ static unsigned int nmk_i2c_functionality(struct i2c_adapter *adap)
>  }
>  
>  static const struct i2c_algorithm nmk_i2c_algo = {
> -	.xfer = nmk_i2c_xfer,
> +	.xfer_v2 = nmk_i2c_xfer_v2,

[Severity: High]
Does removing .xfer from i2c_algorithm break downstream subsystems like I2C
multiplexers? When a multiplexer attached to this bus attempts to register
its transfer methods, it strictly checks parent->algo->master_xfer. Since
.xfer is removed, the .master_xfer field (which is aliased via a union)
evaluates to NULL, causing multiplexed I2C transfers to consistently fail
with -EOPNOTSUPP.

>  	.functionality = nmk_i2c_functionality
>  };


Looking at i2cdev_ioctl_rdwr_v2() in drivers/i2c/i2c-dev.c introduced earlier
in this series:

drivers/i2c/i2c-dev.c:i2cdev_ioctl_rdwr_v2() {
    ...
    if (user_report) {
        res = i2c_transfer_v2(client->adapter, msgs, nmsgs, &report);
        i = report.msgs_cplt;
    } else {
        res = i2c_transfer(client->adapter, msgs, nmsgs);
        if (res < 0)
            i = 0;
        else
            i = nmsgs;
    }
    ...
    while (i-- > 0) {
        if (msgs[i].flags & I2C_M_RD) {
            if (copy_to_user(data_ptrs[i], msgs[i].buf,
                             msgs[i].len))
                res = -EFAULT;
        }
        kfree(msgs[i].buf);
    }
    ...
}

[Severity: High]
Does this regression leak the un-transferred message buffers? When an I2C
transfer involving multiple messages fails partway through, the loop index i
is reassigned to the number of completed messages. This truncates the cleanup
loop, skipping the kfree(msgs[i].buf) for all subsequent allocated message
buffers.


Further down in i2cdev_ioctl_rdwr_v2():

drivers/i2c/i2c-dev.c:i2cdev_ioctl_rdwr_v2() {
    ...
    /* Number of messages transferred completely or partially */
    if (report.bytes_cplt > 0) {
        i++;
        msgs[i].len = report.bytes_cplt;
    }
    ...
}

[Severity: High]
Can this regression corrupt memory with an out-of-bounds write? The code
incorrectly increments i before writing report.bytes_cplt to msgs[i].len.
If nmsgs == 1 and the only message faults, this writes entirely outside the
msgs array, or otherwise corrupts the length of the next message.

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