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 <[email protected]> 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/[email protected]?part=12
