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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Uninitialized stack variable `stat` is evaluated if `regmap_read` fails.
- [Medium] i2c_dw_fault_report incorrectly reports transmission progress on non-abort errors, and unconditional addition corrupts potential error returns.
--

commit ace9d554c2f401ddd92040765e5f36fab0bfd74c
Author: Dmitry Guzman <Dmitry.Guzman@mobileye.com>
Subject: i2c: designware: add support for I2C_XFER_V2 - detailed fault reporting

This commit introduces support for I2C_XFER_V2 in the i2c-designware driver.
It enables clients to get detailed reports on transmission failures by
detecting the number of un-sent bytes via the FLUSH_CNT field and checking
abort flags after FIFO reads and writes.

> diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c
> index f029aa1eadbf8..bc23c3fdaaf2e 100644
> --- a/drivers/i2c/busses/i2c-designware-master.c
> +++ b/drivers/i2c/busses/i2c-designware-master.c

[ ... ]

> @@ -33,6 +34,16 @@
>  #define AMD_TIMEOUT_MAX_US	250
>  #define AMD_MASTERCFG_MASK	GENMASK(15, 0)
>  
> +static int i2c_dw_fault_report(struct dw_i2c_dev *dev, struct i2c_transfer_report *report);
> +
> +static inline bool i2c_dw_check_abort_flag(struct dw_i2c_dev *dev)
> +{
> +	u32 stat;
> +
> +	regmap_read(dev->map, DW_IC_RAW_INTR_STAT, &stat);
> +	return (!!(stat & DW_IC_INTR_TX_ABRT));
> +}

[Severity: Medium]
If regmap_read() fails (for example, due to a bus error or PM state),
could this result in evaluating an uninitialized stack variable?

If stat contains a garbage value, this might spuriously detect a TX abort,
prematurely breaking the FIFO read/write loops and causing non-deterministic
transfer behavior.

[ ... ]

> @@ -918,16 +980,83 @@ i2c_dw_xfer_common(struct dw_i2c_dev *dev, struct i2c_msg msgs[], int num)
>  
>  	i2c_dw_release_lock(dev);
>  
> -	if (ret < 0)
> +	if (ret < 0) {
> +		if (report) {
> +			i2c_dw_fault_report(dev, report);
> +			report->msgs_cplt += msgs_in_prev_parts;
> +			report->fault_msg_idx += msgs_in_prev_parts;
> +		}
>  		return ret;
> +	}

[Severity: Medium]
Will this unconditional addition corrupt a negative error code if
i2c_dw_fault_report() sets report->msgs_cplt to a negative value to indicate
an indeterminate state?

[ ... ]

> +static int i2c_dw_fault_report(struct dw_i2c_dev *dev, struct i2c_transfer_report *report)
> +{
> +	int idx;
> +	int n_flushed = FIELD_GET(DW_IC_TX_ABRT_SOURCE_FLUSH_CNT_MASK, dev->abort_source);

[Severity: Medium]
Does this code correctly handle non-abort errors like timeouts?

If a timeout occurs and dev->abort_source is 0, n_flushed will be 0. Could
this cause bytes stuck in the FIFO to be incorrectly reported as successfully
sent on the wire?

[ ... ]

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