Re: [PATCH RFC v3 07/14] i2c: core: implement i2c_transfer_v2() - transfer with detailed reporting

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] i2c: core: out-of-bounds msgs[] access when tracing in __i2c_transfer_v2()
- [Medium] i2c: core: unupdated report and skipped tracing in __i2c_transfer_v2()
- [Medium] i2c: core: unreset transfer report on retry in __i2c_transfer_v2()
- [Low] i2c: core: wrong function name in kernel-doc comment for i2c_transfer()
--

commit d51afdaed0f512f5afd35f1c0e4f6000a4fd5c18
Author: Dmitry Guzman <Dmitry.Guzman@mobileye.com>

i2c: core: implement i2c_transfer_v2() - transfer with detailed reporting

This commit introduces i2c_transfer_v2() and __i2c_transfer_v2() to support
detailed transfer reporting via struct i2c_transfer_report and the
algo->xfer_v2() callback, while keeping i2c_transfer() and __i2c_transfer()
as wrappers passing a NULL report pointer.

> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 23ff02a1e532d..e8449f55f0452 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -2288,22 +2288,37 @@ static struct i2c_msg *i2c_check_for_quirks(struct i2c_adapter *adap,
>  }
>  
>  /**
> - * __i2c_transfer - unlocked flavor of i2c_transfer
> + * __i2c_transfer_v2 - unlocked flavor of i2c_transfer_v2
>   * @adap: Handle to I2C bus
>   * @msgs: One or more messages to execute before STOP is issued to
>   *	terminate the operation; each message begins with a START.
>   * @num: Number of messages to be executed.
> + * @report: The buffer for detailed transfer report (may be NULL).
>   *
>   * Returns negative errno, else the number of messages executed.
> + * Writes the detailed transfer report to the structure pointed by @report.
>   *
>   * Adapter lock must be held when calling this function. No debug logging
>   * takes place.
>   */
> -int __i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
> +int __i2c_transfer_v2(struct i2c_adapter *adap, struct i2c_msg *msgs, int num,
> +		      struct i2c_transfer_report *report)
>  {
>  	unsigned long orig_jiffies;
>  	int ret, try;
>  
> +	if (report) {
> +		/* Initialize the report structure with -EOPNOTSUPP in all fields.
> +		 * This differentiates from the case when the adapter supports
> +		 * fault reporting and returns -EOPNOTSUPP because it cannot transfer
> +		 * a certain message.
> +		 */
> +		i2c_transfer_report_init(report);
> +
> +		if (!adap->algo->xfer_v2)
> +			return -EOPNOTSUPP;
> +	}

[ ... ]

> @@ -2336,10 +2359,14 @@ int __i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
>  	/* Retry automatically on arbitration loss */
>  	orig_jiffies = jiffies;
>  	for (ret = 0, try = 0; try <= adap->retries; try++) {
> -		if (i2c_in_atomic_xfer_mode() && adap->algo->master_xfer_atomic)
> +		if (i2c_in_atomic_xfer_mode() && adap->algo->master_xfer_atomic) {
>  			ret = adap->algo->master_xfer_atomic(adap, msgs, num);

[Severity: Medium]
What happens in __i2c_transfer_v2() when i2c_in_atomic_xfer_mode() is true
on an adapter that provides both xfer_v2 and master_xfer_atomic, and the
caller passes a non-NULL report pointer?

At the start of __i2c_transfer_v2(), i2c_transfer_report_init(report) sets
report->msgs_cplt, report->fault_msg_idx, and report->bytes_cplt to
-EOPNOTSUPP. The transfer loop then invokes adap->algo->master_xfer_atomic(),
which does not take a report pointer. Even when master_xfer_atomic() succeeds
and returns num, report is never updated with i2c_transfer_report_success(),
so all fields of report remain -EOPNOTSUPP in violation of the struct
i2c_transfer_report contract.

Because report->msgs_cplt stays -EOPNOTSUPP, the tracepoint loop below also
sets n = report->msgs_cplt and skips trace_i2c_reply() for all completed
I2C_M_RD messages. Should __i2c_transfer_v2() update report (or reject
atomic transfers when report is non-NULL)?

> -		else
> -			ret = adap->algo->master_xfer(adap, msgs, num);
> +		} else {
> +			if (report)
> +				ret = adap->algo->xfer_v2(adap, msgs, num, report);
> +			else
> +				ret = adap->algo->master_xfer(adap, msgs, num);
> +		}
>  
>  		if (ret != -EAGAIN)
>  			break;

[Severity: Medium]
If adap->algo->xfer_v2() partially populates report on try == 0 and returns
-EAGAIN on arbitration loss, doesn't the next iteration of the retry loop in
__i2c_transfer_v2() pass that dirty report structure into xfer_v2() without
re-initializing it via i2c_transfer_report_init(report)?

If a subsequent attempt fails before overwriting all fields of report, or if
the bus driver expects report->msgs_cplt to be initialized to -EOPNOTSUPP on
entry to xfer_v2() (such as i2c_dw_fault_report() checking report->msgs_cplt < 0
and accumulating msgs_in_prev_parts), stale values from the earlier attempt
will corrupt the final report.

Also, in i2c_transfer_v2(), if __i2c_lock_bus_helper(adap) returns -EAGAIN
before __i2c_transfer_v2() is called, i2c_transfer_report_init(report) is
not called at all, leaving the caller's report structure uninitialized.
Should i2c_transfer_report_init(report) be called on entry to
i2c_transfer_v2() and before each retry in __i2c_transfer_v2()?

>  		if (time_after(jiffies, orig_jiffies + adap->timeout))
>  			break;
>  	}
>  
>  	if (static_branch_unlikely(&i2c_trace_msg_key)) {
> -		int i;
> -		for (i = 0; i < ret; i++)
> +		int i, n;
> +
> +		if (report)
> +			n = report->msgs_cplt;
> +		else
> +			n = ret;
> +		for (i = 0; i < n; i++)
>  			if (msgs[i].flags & I2C_M_RD)
>  				trace_i2c_reply(adap, &msgs[i], msgs[i].len, i);
> +		if (report && report->bytes_cplt > 0 && msgs[n].flags & I2C_M_RD)
> +			trace_i2c_reply(adap, &msgs[n], report->bytes_cplt, n);

[Severity: High]
Can this index msgs[n] out of bounds when i2c_trace_msg_key is enabled in
__i2c_transfer_v2()?

Here n is set to report->msgs_cplt, and if report->bytes_cplt > 0, the code
evaluates msgs[n].flags and calls trace_i2c_reply(adap, &msgs[n],
report->bytes_cplt, n) without checking that n >= 0 and n < num (or that n <=
num for the loop and report->bytes_cplt <= msgs[n].len).

By definition in struct i2c_transfer_report, report->msgs_cplt can be a
negative error code (when the driver cannot determine the number of
completed messages) or equal to num (or even >= num if a multi-part transfer
accumulates msgs_in_prev_parts across retries). If report->bytes_cplt > 0 at
the same time, indexing msgs[n] reads out of bounds on the msgs[] array and
trace_i2c_reply() can copy from an out-of-bounds msgs[n].buf pointer.

>  		trace_i2c_result(adap, num, ret);
>  	}
>  
>  	return ret;
>  }

[ ... ]

> +/**
> + * i2c_transfer_v2 - execute a single or combined I2C message
> + * @adap: Handle to I2C bus
> + * @msgs: One or more messages to execute before STOP is issued to
> + *	terminate the operation; each message begins with a START.
> + * @num: Number of messages to be executed.
> + *
> + * Returns negative errno, else the number of messages executed.
> + *
> + * Note that there is no requirement that each message be sent to
> + * the same slave address, although that is the most common model.
> + */
> +int i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)

[Severity: Low]
Should the kernel-doc function name above i2c_transfer() be i2c_transfer
instead of i2c_transfer_v2? Having i2c_transfer_v2 here causes a kernel-doc
prototype mismatch warning when building with W=1 or building documentation.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105-i2c-fault-reporting-v3-0-515ad74ac4c9@mobileye.com?part=7