[PATCH RFC v3 00/14] I2C - detailed transfer reporting in case of a fault

[PATCH RFC v3 00/14] I2C - detailed transfer reporting in case of a fault

[Dmitry Guzman]

The existing API has function i2c_transfer() that transfers one or more
messages, and it only returns a single error code if the transfer was
failed. It doesn't allow to know how many of the messages were
transferred successfully, neither how many bytes were transferred in the
message that caused the fault, and also it drops all data received from
target device before the fault. There is a comment about this in
drivers/i2c/i2c-core-base.c: "REVISIT the fault reporting model here is
weak".

This patch series introduces new API function i2c_transfer_v2() that does
the same as i2c_transfer() but also returns detailed transfer report,
including number of messages and bytes transferred before the fault. This
also allows caller to get the bytes read from the target before the fault
occurred.

For user space clients, new ioctl `I2C_RDWR_V2` is introduced.

Patch for `i2c-tools` user space utility that introduces support of this
feature:

https://lore.kernel.org/linux-i2c/20260623-i2c_detailed_report-v1-1-b67259a31a9d@mobileye.com/

Below there are several usage examples with modified i2c-tools.

- Unsupported message (max length supported by I2C controller is 2047):

    # i2ctransfer -y 0 -r w1@0x19 0 r8000
    Error: Operation not supported at message 1
    0 out of 2 messages sent
    #

- First two messages transferred successfully, the third one is sent to
  non-existent target address and thus is not ACKed. The data read in the
second message is available for the caller:

    # i2ctransfer -y 0 -r w1@0x19 0 r1 w1@0x1a 0
    Error: No such device or address at message 2
    2 out of 3 messages sent
    0x32
    #

- Target ACKs first two bytes but sends NACK on the third byte:

    # i2ctransfer -r -y 0 w4@0x09 0x80 0x80 0x55 0x55
    Error: Input/output error at message 0
    0 out of 1 messages sent
    Message 0: 2 out of 4 bytes sent
    #

- Message reading is not finished due to timeout. The data read before
  timeout is available:

    # i2ctransfer -y 0 -r r100@0x19
    Error: Connection timed out at message 0
    0 out of 1 messages sent
    Message 0: 16 out of 100 bytes sent
    0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00
    #

- Driver does not support detailed fault reporting:

    # i2ctransfer -y 0 -r r1@0x19
    Error: Sending messages failed: Operation not supported
    #

In this patchset, the introduced functionality is implemented in
i2c-core and in two bus drivers: i2c-nomadik and i2c-designware.

Patches #11..#14 that implement the new feature in the bus drivers depend
on two other patch sets:

https://lore.kernel.org/linux-i2c/20261103-i2c-designware-fixes-v1-1-f83dd71f988c@mobileye.com

https://lore.kernel.org/linux-i2c/20261103-i2c-nomadik-fixes-v1-0-655c6af2cb70@mobileye.com

that contain other improvements of these two drivers. I submit them
separately from this patchset because they can be applied independently
of remaining patches in the set and because v3 of this patch set becomes
too long with them.

i2c-nomadik driver has been tested on EyeQ6H.

i2c-designware driver has been tested on EyeQ6L+.

The implementation is split up into patches:

Patch #1 Add definition for I2C transfer report structure.
Patch #2 Add xfer_v2() method to i2c_algorithm structure.
Patch #3 Add helper functions for transfer report generation.
Patch #4 Add i2c_xfer_to_xfer_v2() helper function for drivers that do
	not have specific implementation for xfer() method but have
	xfer_v2().
Patch #5 Pass message length separately from i2c_msg structure in
	trace_i2c_reply().
Patch #6 Make i2c_check_for_quirks() return the pointer to the message
	that cannot be sent.
Patch #7 Implement i2c_transfer_v2() in I2C core.
Patch #8 Add definitions for I2C_XFER_V2 ioctl for using the new feature
	from user space.
Patch #9 Implement I2C_XFER_V2 ioctl in I2C core.
Patch #10 Add documentation for the new feature.
Patch #11 Save I2C_SR register value before resetting hardware in
	interrupt handler in i2c-nomadik.
Patch #12 Add support for xfer_v2() in i2c-nomadik.
Patch #13 Separate i2c_algorithm structure for AMD_NAVI_GPU variant of
	  designware controller.
Patch #14 Add support for xfer_v2() in i2c-designware driver.

Signed-off-by: Dmitry Guzman <Dmitry.Guzman@mobileye.com>
---
Changes in v3:
- Split a single big patch #1 with all i2c-core changes into smaller
  patches (#1..#10).
- Separate fixes of pre-existing issues in i2c-nomadik and
  i2c-designware drivers into their own patch sets.
- Fix issues found in v2 by AI review bot.
- Improve comments, fix typos.
- Added more usage examples in the cover letter.
- Link to v2: https://lore.kernel.org/r/20260903-i2c-fault-reporting-v2-0-fedeb91792e6@mobileye.com

Changes in v2:
- Add support for detailed transfer reporting in designware driver (patches
  #8..#12).
- Change `unsigned char` into `u8` in patch #2.
- Added code-review trailers for patches #3..#7.
- Rebase on new kernel version (no conflicting code changes).
- Link to v1: https://lore.kernel.org/r/20260623-i2c-fault-reporting-v1-0-6db1a8aabf18@mobileye.com

To: Andi Shyti <andi.shyti@kernel.org>
To: Steven Rostedt <rostedt@goodmis.org>
To: Masami Hiramatsu <mhiramat@kernel.org>
To: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
To: Linus Walleij <linusw@kernel.org>
To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To: Mika Westerberg <mika.westerberg@linux.intel.com>
Cc: linux-i2c@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-trace-kernel@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org

---
Dmitry Guzman (14):
      i2c: uapi: add i2c_transfer_report structure
      i2c: core: add xfer_v2() to i2c_algorithm structure
      i2c: core: add helper functions for transfer report generation
      i2c: core: add i2c_xfer_to_xfer_v2() helper function
      i2c: trace: pass message length separately in trace_i2c_reply()
      i2c: core: make i2c_check_for_quirks() return pointer to the message
      i2c: core: implement i2c_transfer_v2() - transfer with detailed reporting
      i2c: uapi: add I2C_XFER_V2 ioctl - detailed fault reporting
      i2c: core: implement I2C_RDWR_V2 ioctl - transfer with detailed report
      i2c: doc: add section for I2C_RDWR_V2 ioctl
      i2c: nomadik: save I2C_CR register before resetting HW in IRQ handler
      i2c: nomadik: add support for I2C_XFER_V2 - detailed fault reporting
      i2c: designware: use separate `i2c_algorithm` for AMD_NAVI_GPU
      i2c: designware: add support for I2C_XFER_V2 - detailed fault reporting

 Documentation/i2c/dev-interface.rst        |  48 ++++++++++
 drivers/i2c/busses/i2c-designware-common.c |  15 ++-
 drivers/i2c/busses/i2c-designware-core.h   |  13 +++
 drivers/i2c/busses/i2c-designware-master.c | 145 +++++++++++++++++++++++++++--
 drivers/i2c/busses/i2c-nomadik.c           |  67 ++++++++++---
 drivers/i2c/i2c-core-base.c                | 135 ++++++++++++++++++++-------
 drivers/i2c/i2c-dev.c                      | 123 ++++++++++++++++++++----
 include/linux/i2c.h                        |  35 +++++++
 include/trace/events/i2c.h                 |  10 +-
 include/uapi/linux/i2c-dev.h               |   9 ++
 include/uapi/linux/i2c.h                   |  21 +++++
 11 files changed, 538 insertions(+), 83 deletions(-)
---
base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4
change-id: 20260623-i2c-fault-reporting-9236c9affc2d
prerequisite-change-id: 20261025-i2c-designware-fixes-35d79db08689:v1
prerequisite-patch-id: dfc2af82eb4d654796ee4222fd86651c624b3305
prerequisite-change-id: 20261025-i2c-nomadik-fixes-73121cc61408:v1
prerequisite-patch-id: 04275f1ef88d65148fc56f1392942dbf3d523cf9
prerequisite-patch-id: 40eac74ab9e214ef658e5061368df45b719247f0
prerequisite-patch-id: 450e16ffa1879c1903048486aefa5a35c1e71f3c
prerequisite-patch-id: 1ea98272288b495cf185d7760b68d78982448f0f
prerequisite-patch-id: 8fd5cc8ebc4572189389c645f5fd939429118324
prerequisite-patch-id: fe840395009e945fc2442d4d7d3d07a426d48d54
prerequisite-patch-id: 0f6884cf9b38bd015cd7a23ba85cc0e239c3a49c
prerequisite-patch-id: 5f5ffed672373fc7f141bca2a9b605a31b6148ef

Best regards,
--  
Dmitry Guzman <Dmitry.Guzman@mobileye.com>

[PATCH RFC v3 01/14] i2c: uapi: add i2c_transfer_report structure

[Dmitry Guzman]

Add i2c_transfer_report structure declaration for detailed transfer
reporting in case of a fault.

The structure has three fields:

- fault_msg_idx - index of message that caused fault;
- msgs_cplt - number of messages transferred successfully;
- bytes_cplt - number of bytes transferred in the last message if it was
  completed partially.

Signed-off-by: Dmitry Guzman <Dmitry.Guzman@mobileye.com>
---
 include/uapi/linux/i2c.h | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/include/uapi/linux/i2c.h b/include/uapi/linux/i2c.h
index 2a226657d9f8..5e8e7d3536c8 100644
--- a/include/uapi/linux/i2c.h
+++ b/include/uapi/linux/i2c.h
@@ -135,6 +135,27 @@ struct i2c_msg {
 					 I2C_FUNC_SMBUS_READ_BLOCK_DATA | \
 					 I2C_FUNC_SMBUS_BLOCK_PROC_CALL)
 
+/* Detailed transfer report */
+
+struct i2c_transfer_report {
+	__s32 fault_msg_idx;		/* In case of a fault, index of the message that caused
+					 * the fault. If the bus driver cannot determine it, it
+					 * puts a negative error code. If there is no fault, the
+					 * value is equal to number of messages transferred.
+					 */
+	__s32 msgs_cplt;		/* Number of messages that are known to be transferred
+					 * successfully. If the bus driver cannot determine it, it
+					 * puts a negative error code. If there is no fault, the
+					 * value is equal to number of messages transferred.
+					 */
+	__s32 bytes_cplt;		/* In case of a fault, number of bytes in the message at
+					 * index `msgs_cplt` that are known to be transferred
+					 * successfully. If the bus driver cannot determine the
+					 * number of bytes, it puts a negative error value.
+					 * If there is no fault, the value is 0.
+					 */
+};
+
 /*
  * Data for SMBus Messages
  */

-- 
2.43.0

[PATCH RFC v3 02/14] i2c: core: add xfer_v2() to i2c_algorithm structure

[Dmitry Guzman]

In struct i2c_algorithm, add xfer_v2() - a new method that is
similar to xfer() but accepts in addition pointer to struct
i2c_transfer_report and puts the detailed transfer report into it.

With this method, it case of a fault, it is possible to get not only
the error code, but also the number of messages and bytes transferred
before the fault. If there were read messages before the fault, the read
data may also be used by the client.

Signed-off-by: Dmitry Guzman <Dmitry.Guzman@mobileye.com>
---
 include/linux/i2c.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 6a847fc88d25..bf7dc12f7264 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -537,6 +537,8 @@ i2c_register_board_info(int busnum, struct i2c_board_info const *info,
  *   into I2C transfers instead.
  * @smbus_xfer_atomic: Same as @smbus_xfer. Yet, only using atomic context
  *   so e.g. PMICs can be accessed very late before shutdown. Optional.
+ * @xfer_v2: Same as @xfer, but provides detailed report in case of a fault.
+ *   Optional. If it is present, @xfer must be also present.
  * @functionality: Return the flags that this algorithm/adapter pair supports
  *   from the ``I2C_FUNC_*`` flags.
  * @reg_target: Register given client to local target mode of this adapter
@@ -582,6 +584,8 @@ struct i2c_algorithm {
 				 unsigned short flags, char read_write,
 				 u8 command, int size, union i2c_smbus_data *data);
 
+	int (*xfer_v2)(struct i2c_adapter *adap, struct i2c_msg *msgs,
+		       int num, struct i2c_transfer_report *report);
 	/* To determine what the adapter supports */
 	u32 (*functionality)(struct i2c_adapter *adap);
 

-- 
2.43.0

[PATCH RFC v3 03/14] i2c: core: add helper functions for transfer report generation

[Dmitry Guzman]

Add three helper functions for I2C transfer report generation:

- i2c_transfer_report_init() that puts -EOPNOTSUPP in all struct fields
  (this should be used if the defailed reporting is not supported);

- i2c_transfer_report_success() that is used to generate the report in
  success case (all messages are transferred successfully);

- i2c_transfer_report_bad_msg() that is used if one of messages cannot
  be transferred and no data were actually transferred.

Signed-off-by: Dmitry Guzman <Dmitry.Guzman@mobileye.com>
---
 include/linux/i2c.h | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index bf7dc12f7264..9bee726592fc 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -601,6 +601,27 @@ struct i2c_algorithm {
 #endif
 };
 
+static inline void i2c_transfer_report_init(struct i2c_transfer_report *report)
+{
+	report->msgs_cplt = -EOPNOTSUPP;
+	report->fault_msg_idx = -EOPNOTSUPP;
+	report->bytes_cplt = -EOPNOTSUPP;
+}
+
+static inline void i2c_transfer_report_success(struct i2c_transfer_report *report, int num)
+{
+	report->msgs_cplt = num;
+	report->fault_msg_idx = num;
+	report->bytes_cplt = 0;
+}
+
+static inline void i2c_transfer_report_bad_msg(struct i2c_transfer_report *report, int i)
+{
+	report->msgs_cplt = 0;
+	report->bytes_cplt = 0;
+	report->fault_msg_idx = i;
+}
+
 /**
  * struct i2c_lock_operations - represent I2C locking operations
  * @lock_bus: Get exclusive access to an I2C bus segment

-- 
2.43.0

[PATCH RFC v3 04/14] i2c: core: add i2c_xfer_to_xfer_v2() helper function

[Dmitry Guzman]

If an I2C bus driver implements i2c_xfer_v2() callback for multi-message
transfer with detailed reporting in its struct i2c_algorithm, it still
must have old xfer() callback for backward compatibility because this
callback sometimes is checked or called directly, without using
i2c_transfer().

To avoid code duplication for drivers that do not need different code
for transfer with and without detailed reporting enabled, provide a
helper function i2c_xfer_to_xfer_v2() that calls xfer_v2() with pointer
to a dummy struct i2c_transfer_report that will be dropped after using.

It should be used in xfer field of struct i2c_algorithm.

Signed-off-by: Dmitry Guzman <Dmitry.Guzman@mobileye.com>
---
 drivers/i2c/i2c-core-base.c | 8 ++++++++
 include/linux/i2c.h         | 2 ++
 2 files changed, 10 insertions(+)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 165ec14260c8..12b49c01baed 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -2400,6 +2400,14 @@ int i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 }
 EXPORT_SYMBOL(i2c_transfer);
 
+int i2c_xfer_to_xfer_v2(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
+{
+	struct i2c_transfer_report dummy_report;
+
+	return adap->algo->xfer_v2(adap, msgs, num, &dummy_report);
+}
+EXPORT_SYMBOL(i2c_xfer_to_xfer_v2);
+
 /**
  * i2c_transfer_buffer_flags - issue a single I2C message transferring data
  *			       to/from a buffer
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 9bee726592fc..6a4df8562ab4 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -601,6 +601,8 @@ struct i2c_algorithm {
 #endif
 };
 
+int i2c_xfer_to_xfer_v2(struct i2c_adapter *adap, struct i2c_msg *msgs, int num);
+
 static inline void i2c_transfer_report_init(struct i2c_transfer_report *report)
 {
 	report->msgs_cplt = -EOPNOTSUPP;

-- 
2.43.0

[PATCH RFC v3 05/14] i2c: trace: pass message length separately in trace_i2c_reply()

[Dmitry Guzman]

In trace_i2c_reply(), add separate argument for message length and use it
instead of taking the message length from struct i2c_msg. This allows to
trace the read data of an interrupted read message.

This is preparation for implementation of detailed transfer reporting
feature.

Signed-off-by: Dmitry Guzman <Dmitry.Guzman@mobileye.com>
---
 drivers/i2c/i2c-core-base.c |  2 +-
 include/trace/events/i2c.h  | 10 +++++-----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 12b49c01baed..3dd7ef02c562 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -2349,7 +2349,7 @@ int __i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 		int i;
 		for (i = 0; i < ret; i++)
 			if (msgs[i].flags & I2C_M_RD)
-				trace_i2c_reply(adap, &msgs[i], i);
+				trace_i2c_reply(adap, &msgs[i], msgs[i].len, i);
 		trace_i2c_result(adap, num, ret);
 	}
 
diff --git a/include/trace/events/i2c.h b/include/trace/events/i2c.h
index 142a23c6593c..b3267dcec420 100644
--- a/include/trace/events/i2c.h
+++ b/include/trace/events/i2c.h
@@ -88,22 +88,22 @@ TRACE_EVENT_FN(i2c_read,
  */
 TRACE_EVENT_FN(i2c_reply,
 	       TP_PROTO(const struct i2c_adapter *adap, const struct i2c_msg *msg,
-			int num),
-	       TP_ARGS(adap, msg, num),
+			int data_len, int num),
+	       TP_ARGS(adap, msg, data_len, num),
 	       TP_STRUCT__entry(
 		       __field(int,	adapter_nr		)
 		       __field(__u16,	msg_nr			)
 		       __field(__u16,	addr			)
 		       __field(__u16,	flags			)
 		       __field(__u16,	len			)
-		       __dynamic_array(__u8, buf, msg->len)	),
+		       __dynamic_array(__u8, buf, data_len)	),
 	       TP_fast_assign(
 		       __entry->adapter_nr = adap->nr;
 		       __entry->msg_nr = num;
 		       __entry->addr = msg->addr;
 		       __entry->flags = msg->flags;
-		       __entry->len = msg->len;
-		       memcpy(__get_dynamic_array(buf), msg->buf, msg->len);
+		       __entry->len = data_len;
+		       memcpy(__get_dynamic_array(buf), msg->buf, data_len);
 			      ),
 	       TP_printk("i2c-%d #%u a=%03x f=%04x l=%u [%*phD]",
 			 __entry->adapter_nr,

-- 
2.43.0

[PATCH RFC v3 06/14] i2c: core: make i2c_check_for_quirks() return pointer to the message

[Dmitry Guzman]

i2c_check_for_quirks() checks if there are messages in msgs array that
cannot be transferred by the adapter. If there is such a message, it
returns -EOPNOTSUPP.

The error code in the return value is not needed because __i2c_transfer()
does not use it and explicitly returns -EOPNOTSUPP if
i2c_check_for_quirks() return value is not zero. So, instead of returning
the error code, return the pointer to the first message that cannot be
transferred by the controller, to provide the possibility to pass the
message index to the client.

This is preparation for implementation of detailed transfer reporting.

Signed-off-by: Dmitry Guzman <Dmitry.Guzman@mobileye.com>
---
 drivers/i2c/i2c-core-base.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 3dd7ef02c562..23ff02a1e532 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -2223,15 +2223,17 @@ module_exit(i2c_exit);
 /* Check if val is exceeding the quirk IFF quirk is non 0 */
 #define i2c_quirk_exceeded(val, quirk) ((quirk) && ((val) > (quirk)))
 
-static int i2c_quirk_error(struct i2c_adapter *adap, struct i2c_msg *msg, char *err_msg)
+static struct i2c_msg *i2c_quirk_error(struct i2c_adapter *adap,
+				       struct i2c_msg *msg, char *err_msg)
 {
 	dev_err_ratelimited(&adap->dev, "adapter quirk: %s (addr 0x%04x, size %u, %s)\n",
 			    err_msg, msg->addr, msg->len,
 			    str_read_write(msg->flags & I2C_M_RD));
-	return -EOPNOTSUPP;
+	return msg;
 }
 
-static int i2c_check_for_quirks(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
+static struct i2c_msg *i2c_check_for_quirks(struct i2c_adapter *adap,
+					    struct i2c_msg *msgs, int num)
 {
 	const struct i2c_adapter_quirks *q = adap->quirks;
 	int max_num = q->max_num_msgs, i;
@@ -2282,7 +2284,7 @@ static int i2c_check_for_quirks(struct i2c_adapter *adap, struct i2c_msg *msgs,
 		}
 	}
 
-	return 0;
+	return NULL;
 }
 
 /**

-- 
2.43.0

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

[Dmitry Guzman]

In __i2c_transfer() and i2c_transfer(), add one more parameter - pointer
to transfer report. In __i2c_transfer(), if the pointer is not NULL,
use algo->xfer_v2() callback instead of algo->xfer() in order to get the
detailed transfer report. If the adapter does not have xfer_v2(), return
-EOPNOTSUPP.

If the adapter has any quirks that make impossible to transfer requested
message or messages, put the index of the first such message to the
transfer report structure.

If the report is not NULL, algo->xfer_v2() returned an error and there
are any read messages, trace the read data using `trace_i2c_reply(). If
the last read message is incomplete (transfer was interrupted in the
middle of a read message), trace the read portion of data also.

Rename __i2c_transfer() to __i2c_transfer_v2() and i2c_transfer() to
i2c_transfer_v2().

For backward compatibility, add __i2c_transfer() and i2c_transfer() that
call __i2c_transfer_v2() and i2c_transfer_v2() respectively with null
report pointer.

Remove comment "REVISIT the fault reporting model here is weak".

Signed-off-by: Dmitry Guzman <Dmitry.Guzman@mobileye.com>
---
 drivers/i2c/i2c-core-base.c | 115 ++++++++++++++++++++++++++++++++------------
 include/linux/i2c.h         |   8 +++
 2 files changed, 93 insertions(+), 30 deletions(-)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 23ff02a1e532..e8449f55f045 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;
+	}
+
 	if (!adap->algo->master_xfer) {
 		dev_dbg(&adap->dev, "I2C level transfers not supported\n");
 		return -EOPNOTSUPP;
@@ -2316,8 +2331,16 @@ int __i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 	if (ret)
 		return ret;
 
-	if (adap->quirks && i2c_check_for_quirks(adap, msgs, num))
-		return -EOPNOTSUPP;
+	if (adap->quirks) {
+		struct i2c_msg *bad_msg;
+
+		bad_msg = i2c_check_for_quirks(adap, msgs, num);
+		if (bad_msg) {
+			if (report)
+				i2c_transfer_report_bad_msg(report, bad_msg - msgs);
+			return -EOPNOTSUPP;
+		}
+	}
 
 	/*
 	 * i2c_trace_msg_key gets enabled when tracepoint i2c_transfer gets
@@ -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);
-		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;
@@ -2348,58 +2375,86 @@ int __i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 	}
 
 	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);
 		trace_i2c_result(adap, num, ret);
 	}
 
 	return ret;
 }
+EXPORT_SYMBOL(__i2c_transfer_v2);
+
+/**
+ * __i2c_transfer - unlocked flavor of i2c_transfer
+ * @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.
+ *
+ * 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)
+{
+	return __i2c_transfer_v2(adap, msgs, num, NULL);
+}
 EXPORT_SYMBOL(__i2c_transfer);
 
 /**
- * i2c_transfer - execute a single or combined I2C message
+ * i2c_transfer_v2 - execute combined I2C message with detailed report
  * @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.
  *
- * 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)
+int i2c_transfer_v2(struct i2c_adapter *adap, struct i2c_msg *msgs, int num,
+		    struct i2c_transfer_report *report)
 {
 	int ret;
 
-	/* REVISIT the fault reporting model here is weak:
-	 *
-	 *  - When we get an error after receiving N bytes from a slave,
-	 *    there is no way to report "N".
-	 *
-	 *  - When we get a NAK after transmitting N bytes to a slave,
-	 *    there is no way to report "N" ... or to let the master
-	 *    continue executing the rest of this combined message, if
-	 *    that's the appropriate response.
-	 *
-	 *  - When for example "num" is two and we successfully complete
-	 *    the first message but get an error part way through the
-	 *    second, it's unclear whether that should be reported as
-	 *    one (discarding status on the second message) or errno
-	 *    (discarding status on the first one).
-	 */
 	ret = __i2c_lock_bus_helper(adap);
 	if (ret)
 		return ret;
 
-	ret = __i2c_transfer(adap, msgs, num);
+	ret = __i2c_transfer_v2(adap, msgs, num, report);
 	i2c_unlock_bus(adap, I2C_LOCK_SEGMENT);
 
 	return ret;
 }
+EXPORT_SYMBOL(i2c_transfer_v2);
+
+/**
+ * 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)
+{
+	return i2c_transfer_v2(adap, msgs, num, NULL);
+}
 EXPORT_SYMBOL(i2c_transfer);
 
 int i2c_xfer_to_xfer_v2(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 6a4df8562ab4..ec5f5d6792a8 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -131,6 +131,14 @@ int i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num);
 /* Unlocked flavor */
 int __i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num);
 
+/* Transfer with detailed transfer reporting.
+ */
+int i2c_transfer_v2(struct i2c_adapter *adap, struct i2c_msg *msgs, int num,
+		    struct i2c_transfer_report *report);
+/* Unlocked flavor */
+int __i2c_transfer_v2(struct i2c_adapter *adap, struct i2c_msg *msgs, int num,
+		      struct i2c_transfer_report *report);
+
 /* This is the very generalized SMBus access routine. You probably do not
    want to use this, though; one of the functions below may be much easier,
    and probably just as fast.

-- 
2.43.0

[PATCH RFC v3 08/14] i2c: uapi: add I2C_XFER_V2 ioctl - detailed fault reporting

[Dmitry Guzman]

For user-space access to I2C multi-message transfer with detailed
reporting functionality, add I2C_XFER_V2 ioctl and corresponding
struct i2c_rdwr_v2_ioctl_data.

The structure consists of struct i2c_rdwr_ioctl_data that used in the
same way as in I2C_XFER, and struct i2c_transfer_report that the
detailed transfer report is put in.

Signed-off-by: Dmitry Guzman <Dmitry.Guzman@mobileye.com>
---
 include/uapi/linux/i2c-dev.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/include/uapi/linux/i2c-dev.h b/include/uapi/linux/i2c-dev.h
index 1c4cec4ddd84..5097568a3149 100644
--- a/include/uapi/linux/i2c-dev.h
+++ b/include/uapi/linux/i2c-dev.h
@@ -11,11 +11,13 @@
 
 #include <linux/types.h>
 #include <linux/compiler.h>
+#include <linux/i2c.h>
 
 /* /dev/i2c-X ioctl commands.  The ioctl's parameter is always an
  * unsigned long, except for:
  *	- I2C_FUNCS, takes pointer to an unsigned long
  *	- I2C_RDWR, takes pointer to struct i2c_rdwr_ioctl_data
+ *	- I2C_RDWR_V2, takes pointer to struct i2c_rdwr_v2_ioctl_data
  *	- I2C_SMBUS, takes pointer to struct i2c_smbus_ioctl_data
  */
 #define I2C_RETRIES	0x0701	/* number of times a device address should
@@ -33,6 +35,7 @@
 #define I2C_FUNCS	0x0705	/* Get the adapter functionality mask */
 
 #define I2C_RDWR	0x0707	/* Combined R/W transfer (one STOP only) */
+#define I2C_RDWR_V2	0x0709  /* I2C_RDWR with detailed fault reporting */
 
 #define I2C_PEC		0x0708	/* != 0 to use PEC with SMBus */
 #define I2C_SMBUS	0x0720	/* SMBus transfer */
@@ -52,6 +55,12 @@ struct i2c_rdwr_ioctl_data {
 	__u32 nmsgs;			/* number of i2c_msgs */
 };
 
+/* This is the structure as used in the I2C_RDWR_V2 ioctl call */
+struct i2c_rdwr_v2_ioctl_data {
+	struct i2c_rdwr_ioctl_data rdwr_data;
+	struct i2c_transfer_report report;
+};
+
 #define  I2C_RDWR_IOCTL_MAX_MSGS	42
 /* Originally defined with a typo, keep it for compatibility */
 #define  I2C_RDRW_IOCTL_MAX_MSGS	I2C_RDWR_IOCTL_MAX_MSGS

-- 
2.43.0

[PATCH RFC v3 09/14] i2c: core: implement I2C_RDWR_V2 ioctl - transfer with detailed report

[Dmitry Guzman]

I2C_RDWR_V2 is a new ioctl for I2C controller devices. It has the same
functionality as I2C_RDWR, with additional feature - in case of a fault
during transfer, it provides information how many messages and bytes were
successfully transferred and which message caused the fault. If there is
any read data, it also may be used.

This feature is available on drivers that implement i2c_xfer_v2() method
in their i2c_algorithm.

Signed-off-by: Dmitry Guzman <Dmitry.Guzman@mobileye.com>
---
 drivers/i2c/i2c-dev.c | 123 ++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 104 insertions(+), 19 deletions(-)

diff --git a/drivers/i2c/i2c-dev.c b/drivers/i2c/i2c-dev.c
index d219eb89f978..14fe1dd7eecf 100644
--- a/drivers/i2c/i2c-dev.c
+++ b/drivers/i2c/i2c-dev.c
@@ -240,11 +240,19 @@ static int i2cdev_check_addr(struct i2c_adapter *adapter, unsigned int addr)
 	return result;
 }
 
-static noinline int i2cdev_ioctl_rdwr(struct i2c_client *client,
-		unsigned nmsgs, struct i2c_msg *msgs)
+static noinline int i2cdev_ioctl_rdwr_v2(struct i2c_client *client,
+					 unsigned int nmsgs, struct i2c_msg *msgs,
+					 struct i2c_transfer_report __user *user_report)
 {
+	struct i2c_transfer_report report;
 	u8 __user **data_ptrs;
-	int i, res;
+	int i, res, msgs_done;
+
+	i2c_transfer_report_init(&report);
+
+	if (user_report)
+		if (copy_to_user(user_report, &report, sizeof(report)))
+			return -EFAULT;
 
 	/* Adapter must support I2C transfers */
 	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
@@ -259,6 +267,7 @@ static noinline int i2cdev_ioctl_rdwr(struct i2c_client *client,
 		/* Limit the size of the message to a sane amount */
 		if (msgs[i].len > 8192) {
 			res = -EINVAL;
+			i2c_transfer_report_bad_msg(&report, i);
 			break;
 		}
 
@@ -266,6 +275,7 @@ static noinline int i2cdev_ioctl_rdwr(struct i2c_client *client,
 		msgs[i].buf = memdup_user(data_ptrs[i], msgs[i].len);
 		if (IS_ERR(msgs[i].buf)) {
 			res = PTR_ERR(msgs[i].buf);
+			i2c_transfer_report_bad_msg(&report, i);
 			break;
 		}
 		/* memdup_user allocates with GFP_KERNEL, so DMA is ok */
@@ -287,6 +297,7 @@ static noinline int i2cdev_ioctl_rdwr(struct i2c_client *client,
 			    msgs[i].len < 1 || msgs[i].buf[0] < 1 ||
 			    msgs[i].len < msgs[i].buf[0] +
 					     I2C_SMBUS_BLOCK_MAX) {
+				i2c_transfer_report_bad_msg(&report, i);
 				i++;
 				res = -EINVAL;
 				break;
@@ -300,12 +311,44 @@ static noinline int i2cdev_ioctl_rdwr(struct i2c_client *client,
 		for (j = 0; j < i; ++j)
 			kfree(msgs[j].buf);
 		kfree(data_ptrs);
+		if (user_report && copy_to_user(user_report, &report, sizeof(report)))
+			res = -EFAULT;
 		return res;
 	}
 
-	res = i2c_transfer(client->adapter, msgs, nmsgs);
+	if (user_report) {
+		res = i2c_transfer_v2(client->adapter, msgs, nmsgs, &report);
+		msgs_done = report.msgs_cplt;
+	} else {
+		res = i2c_transfer(client->adapter, msgs, nmsgs);
+		if (res < 0)
+			msgs_done = 0;
+		else
+			msgs_done = nmsgs;
+	}
+
+	if (user_report && copy_to_user(user_report, &report, sizeof(report)))
+		res = -EFAULT;
+
+	if (msgs_done > (int)nmsgs) {
+		//fixme - use dev_err?
+		pr_err("Bad i2c_transfer_report: msgs_cplt = %i, bytes_cplt = %i, nmsgs = %i\n",
+		       report.msgs_cplt, report.bytes_cplt, nmsgs);
+		msgs_done = nmsgs;
+	}
+
+	if (report.bytes_cplt > 0) {
+		if (msgs_done < nmsgs && msgs_done >= 0) {
+			msgs[msgs_done].len = report.bytes_cplt;
+			msgs_done++;
+		} else {
+			pr_err("Bad i2c_transfer_report: msgs_cplt = %i, bytes_cplt = %i, nmsgs = %i\n",
+				report.msgs_cplt, report.bytes_cplt, nmsgs);
+		}
+	}
+
 	while (i-- > 0) {
-		if (res >= 0 && (msgs[i].flags & I2C_M_RD)) {
+		if (msgs[i].flags & I2C_M_RD && i < msgs_done) {
 			if (copy_to_user(data_ptrs[i], msgs[i].buf,
 					 msgs[i].len))
 				res = -EFAULT;
@@ -439,18 +482,39 @@ static long i2cdev_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 		funcs = i2c_get_functionality(client->adapter);
 		return put_user(funcs, (unsigned long __user *)arg);
 
-	case I2C_RDWR: {
+	case I2C_RDWR:
+	case I2C_RDWR_V2:
+	{
+		struct i2c_rdwr_ioctl_data __user *user_arg;
+		struct i2c_transfer_report __user *user_rep;
 		struct i2c_rdwr_ioctl_data rdwr_arg;
 		struct i2c_msg *rdwr_pa;
 		int res;
 
-		if (copy_from_user(&rdwr_arg,
-				   (struct i2c_rdwr_ioctl_data __user *)arg,
-				   sizeof(rdwr_arg)))
+		if (cmd == I2C_RDWR_V2) {
+			user_arg = &((struct i2c_rdwr_v2_ioctl_data __user *)arg)->rdwr_data;
+			user_rep = &((struct i2c_rdwr_v2_ioctl_data __user *)arg)->report;
+		} else {
+			user_arg = (struct i2c_rdwr_ioctl_data __user *)arg;
+			user_rep = NULL;
+		}
+
+		if (copy_from_user(&rdwr_arg, user_arg, sizeof(rdwr_arg)))
 			return -EFAULT;
 
-		if (!rdwr_arg.msgs || rdwr_arg.nmsgs == 0)
-			return -EINVAL;
+		if (!rdwr_arg.msgs || rdwr_arg.nmsgs == 0) {
+			/*
+			 * I2C_RDWR_V2 ioctl with nmsgs == 0 is used for
+			 * discovering of the controller capability to return
+			 * detailed fault reports.
+			 */
+			if (cmd == I2C_RDWR)
+				return -EINVAL;
+			if (client->adapter->algo->xfer_v2)
+				return 0;
+			else
+				return -EOPNOTSUPP;
+		}
 
 		/*
 		 * Put an arbitrary limit on the number of messages that can
@@ -464,7 +528,7 @@ static long i2cdev_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 		if (IS_ERR(rdwr_pa))
 			return PTR_ERR(rdwr_pa);
 
-		res = i2cdev_ioctl_rdwr(client, rdwr_arg.nmsgs, rdwr_pa);
+		res = i2cdev_ioctl_rdwr_v2(client, rdwr_arg.nmsgs, rdwr_pa, user_rep);
 		kfree(rdwr_pa);
 		return res;
 	}
@@ -531,6 +595,11 @@ struct i2c_rdwr_ioctl_data32 {
 	u32 nmsgs;
 };
 
+struct i2c_rdwr_v2_ioctl_data32 {
+	struct i2c_rdwr_ioctl_data32 rdwr_data;
+	struct i2c_transfer_report report;
+};
+
 static long compat_i2cdev_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
 	struct i2c_client *client = file->private_data;
@@ -539,19 +608,35 @@ static long compat_i2cdev_ioctl(struct file *file, unsigned int cmd, unsigned lo
 	case I2C_FUNCS:
 		funcs = i2c_get_functionality(client->adapter);
 		return put_user(funcs, (compat_ulong_t __user *)arg);
-	case I2C_RDWR: {
+	case I2C_RDWR:
+	case I2C_RDWR_V2:
+	{
+		struct i2c_rdwr_ioctl_data32 __user *user_arg;
+		struct i2c_transfer_report __user *user_rep;
 		struct i2c_rdwr_ioctl_data32 rdwr_arg;
 		struct i2c_msg32 __user *p;
 		struct i2c_msg *rdwr_pa;
 		int i, res;
 
-		if (copy_from_user(&rdwr_arg,
-				   (struct i2c_rdwr_ioctl_data32 __user *)arg,
-				   sizeof(rdwr_arg)))
+		if (cmd == I2C_RDWR_V2) {
+			user_arg = &((struct i2c_rdwr_v2_ioctl_data32 __user *)arg)->rdwr_data;
+			user_rep = &((struct i2c_rdwr_v2_ioctl_data32 __user *)arg)->report;
+		} else {
+			user_arg = (struct i2c_rdwr_ioctl_data32 __user *)arg;
+			user_rep = NULL;
+		}
+
+		if (copy_from_user(&rdwr_arg, user_arg, sizeof(rdwr_arg)))
 			return -EFAULT;
 
-		if (!rdwr_arg.msgs || rdwr_arg.nmsgs == 0)
-			return -EINVAL;
+		if (!rdwr_arg.msgs || rdwr_arg.nmsgs == 0) {
+			if (cmd == I2C_RDWR)
+				return -EINVAL;
+			if (client->adapter->algo->xfer_v2)
+				return 0;
+			else
+				return -EOPNOTSUPP;
+		}
 
 		if (rdwr_arg.nmsgs > I2C_RDWR_IOCTL_MAX_MSGS)
 			return -EINVAL;
@@ -575,7 +660,7 @@ static long compat_i2cdev_ioctl(struct file *file, unsigned int cmd, unsigned lo
 			};
 		}
 
-		res = i2cdev_ioctl_rdwr(client, rdwr_arg.nmsgs, rdwr_pa);
+		res = i2cdev_ioctl_rdwr_v2(client, rdwr_arg.nmsgs, rdwr_pa, user_rep);
 		kfree(rdwr_pa);
 		return res;
 	}

-- 
2.43.0

[PATCH RFC v3 10/14] i2c: doc: add section for I2C_RDWR_V2 ioctl

[Dmitry Guzman]

I2C_RDWR_V2 is a new ioctl for I2C controller devices. It has the same
functionality as I2C_RDWR, with additional feature - in case of a fault
during transfer, it provides information how many messages and bytes were
successfully transferred and which message caused the fault.

Add documentation section about the new ioctl and the structures it uses.

Signed-off-by: Dmitry Guzman <Dmitry.Guzman@mobileye.com>
---
 Documentation/i2c/dev-interface.rst | 48 +++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/Documentation/i2c/dev-interface.rst b/Documentation/i2c/dev-interface.rst
index c277a8e1202b..95e60bdb2f4d 100644
--- a/Documentation/i2c/dev-interface.rst
+++ b/Documentation/i2c/dev-interface.rst
@@ -140,6 +140,54 @@ The following IOCTLs are defined:
   The slave address and whether to use ten bit address mode has to be
   set in each message, overriding the values set with the above ioctl's.
 
+``ioctl(file, I2C_RDWR_V2, struct i2c_rdwr_v2_ioctl_data *msgset)``
+  Does the same combined read/write transaction as I2C_RDWR, but also
+  provides detailed fault report. The argument is a pointer to a::
+
+    struct i2c_rdwr_v2_ioctl_data {
+	    struct i2c_rdwr_ioctl_data rdwr_data;
+	    struct i2c_transfer_report report;
+    };
+
+  rdwr_data is the same structure as the argument for I2C_RDWR ioctl.
+  report is the structure that the transfer report is written to::
+
+    struct i2c_transfer_report {
+      __s32 fault_msg_idx;
+      __s32 msgs_cplt;
+      __s32 bytes_cplt;
+    };
+
+  msgs_cplt is the number of messages that has been sent or received
+  successfully. If there are read messages within this range, the returned
+  data is guaranteed to be valid. If a message has been read from the
+  device but the read data is lost (for example, FIFO is flushed before
+  CPU read it), this message must not be counted.  If the controller cannot
+  determine the number of completed messages, the value is -EOPNOTSUPP.
+
+  fault_msg_idx is the number of messages that caused a fault. In case of a
+  fault, it is not necessary equal to msgs_cplt. For example, if the driver
+  validates the whole batch before starting transmission, detects that it
+  cannot send it, it returns -EOPNOTSUPP error immediately, so msgs_cplt is 0,
+  while fault_msg_idx points to the message that cannot be sent. Another
+  example when these two value may be different is I2C controller that
+  flushes RX FIFO when an error is detected before CPU reads data from it.
+
+  If there is no fault, the fault_msg_idx value is equal to msgs_cplt.
+
+  bytes_cplt indicates the number of bytes sent/received in the message at
+  index msgs_cplt. If this is a read message, it is guaranteed that these
+  bytes in the message data buffer are valid. If the controller cannot
+  determine the byte count, the value should be -EOPNOTSUPP. If there was
+  no fault or no data was transferred successfully in the faulty message
+  (for example, NACK on target address or first data byte), the value should
+  be 0.
+
+  To discover whether the device supports detailed fault reporting, use
+  I2C_RDWR_V2 ioctl with nmsgs = 0. If the driver supports it, the return
+  value should be 0. If the driver supports only plain I2C_RDWR, the return
+  value should be -EOPNOTSUPP. In any case, nothing is done on wire.
+
 ``ioctl(file, I2C_SMBUS, struct i2c_smbus_ioctl_data *args)``
   If possible, use the provided ``i2c_smbus_*`` methods described below instead
   of issuing direct ioctls.

-- 
2.43.0

[PATCH RFC v3 11/14] i2c: nomadik: save I2C_CR register before resetting HW in IRQ handler

[Dmitry Guzman]

On some interrupts, such as Bus Error interrupt or Arbitration Lost
interrupt, the interrupt handler calls init_hw() to reset the I2C
controller. This clears I2C_SR register value that might be used for
fault reporting.

Add i2c_sr field to nmk_i2c_dev structure, store there I2C_SR register
value before resetting the hardware, and use this value instead of
reading it directly in nmk_i2c_xfer_one() if it was read in IRQ handler.

Signed-off-by: Dmitry Guzman <Dmitry.Guzman@mobileye.com>
---
 drivers/i2c/busses/i2c-nomadik.c | 30 ++++++++++++++++++++++--------
 1 file changed, 22 insertions(+), 8 deletions(-)

diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
index b9f839d5a6fa..f68d4cf7c0e7 100644
--- a/drivers/i2c/busses/i2c-nomadik.c
+++ b/drivers/i2c/busses/i2c-nomadik.c
@@ -193,6 +193,7 @@ struct i2c_nmk_client {
  * @xfer_done: xfer done boolean.
  * @has_32b_bus: controller is on a bus that only supports 32-bit accesses.
  * @timeout_usecs: Slave response timeout
+ * @i2c_sr: last read I2C_SR register value
  * @sm: speed mode
  * @stop: stop condition.
  * @xfer_wq: xfer done wait queue.
@@ -212,6 +213,7 @@ struct nmk_i2c_dev {
 	bool				xfer_done;
 	bool				has_32b_bus;
 	u32				timeout_usecs;
+	u32				i2c_sr;
 	enum i2c_freq_mode		sm;
 	int				stop;
 	struct wait_queue_head		xfer_wq;
@@ -652,6 +654,7 @@ static int nmk_i2c_xfer_one(struct nmk_i2c_dev *priv, u16 flags)
 {
 	int status;
 
+	priv->i2c_sr = 0;
 	if (flags & I2C_M_RD) {
 		/* read operation */
 		priv->cli.operation = I2C_READ;
@@ -663,12 +666,22 @@ static int nmk_i2c_xfer_one(struct nmk_i2c_dev *priv, u16 flags)
 	}
 
 	if (status || priv->result) {
-		u32 i2c_sr;
 		u32 cause;
+		/*
+		 * On some errors, the interrupt handler reads I2C_SR register value,
+		 * stores it into priv->i2c_sr and resets the hardware with init_hw().
+		 * If it is done, we use this value. The value cannot be 0 because
+		 * I2C_SR_STATUS after transfer start may be I2C_ON_GOING, I2C_OK or
+		 * I2C_ABORT but not I2C_NOP.
+		 * If priv->i2c_sr == 0, it was not updated since transmission start, so
+		 * init_hw() was not called and the hardware register I2C_SR still holds
+		 * correct value.
+		 */
+		if (!priv->i2c_sr)
+			priv->i2c_sr = readl(priv->virtbase + I2C_SR);
 
-		i2c_sr = readl(priv->virtbase + I2C_SR);
-		if (FIELD_GET(I2C_SR_STATUS, i2c_sr) == I2C_ABORT) {
-			cause = FIELD_GET(I2C_SR_CAUSE, i2c_sr);
+		if (FIELD_GET(I2C_SR_STATUS, priv->i2c_sr) == I2C_ABORT) {
+			cause = FIELD_GET(I2C_SR_CAUSE, priv->i2c_sr);
 			dev_dbg(&priv->adev->dev, "%s\n",
 				cause >= ARRAY_SIZE(abort_causes) ?
 				"unknown reason" :
@@ -879,6 +892,7 @@ static irqreturn_t i2c_irq_handler(int irq, void *arg)
 			priv->result = -EIO;
 			dev_err(dev, "%lu bytes still remain to be xfered\n",
 				priv->cli.count);
+			priv->i2c_sr = readl(priv->virtbase + I2C_SR);
 			init_hw(priv);
 		}
 		priv->xfer_done = true;
@@ -890,6 +904,7 @@ static irqreturn_t i2c_irq_handler(int irq, void *arg)
 	/* Master Arbitration lost interrupt */
 	case I2C_IT_MAL:
 		priv->result = -EAGAIN;
+		priv->i2c_sr = readl(priv->virtbase + I2C_SR);
 		init_hw(priv);
 
 		i2c_set_bit(priv->virtbase + I2C_ICR, I2C_IT_MAL);
@@ -906,11 +921,9 @@ static irqreturn_t i2c_irq_handler(int irq, void *arg)
 	 */
 	case I2C_IT_BERR:
 	{
-		u32 sr;
-
-		sr = readl(priv->virtbase + I2C_SR);
+		priv->i2c_sr = readl(priv->virtbase + I2C_SR);
 		priv->result = -EIO;
-		if (FIELD_GET(I2C_SR_STATUS, sr) == I2C_ABORT)
+		if (FIELD_GET(I2C_SR_STATUS, priv->i2c_sr) == I2C_ABORT)
 			init_hw(priv);
 
 		i2c_set_bit(priv->virtbase + I2C_ICR, I2C_IT_BERR);
@@ -927,6 +940,7 @@ static irqreturn_t i2c_irq_handler(int irq, void *arg)
 	 */
 	case I2C_IT_TXFOVR:
 		priv->result = -EIO;
+		priv->i2c_sr = readl(priv->virtbase + I2C_SR);
 		init_hw(priv);
 
 		dev_err(dev, "Tx Fifo Over run\n");

-- 
2.43.0

[PATCH RFC v3 12/14] i2c: nomadik: add support for I2C_XFER_V2 - detailed fault reporting

[Dmitry Guzman]

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; there was no way to
find out how many messages or bytes in a certain message have been sent
or received until the fault condition occurred.

This commit introduces support of this feature in i2c-nomadik driver.

Signed-off-by: Dmitry Guzman <Dmitry.Guzman@mobileye.com>
---
 drivers/i2c/busses/i2c-nomadik.c | 37 +++++++++++++++++++++++++++++++------
 1 file changed, 31 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
index f68d4cf7c0e7..9f43ab792ae2 100644
--- a/drivers/i2c/busses/i2c-nomadik.c
+++ b/drivers/i2c/busses/i2c-nomadik.c
@@ -198,6 +198,7 @@ struct i2c_nmk_client {
  * @stop: stop condition.
  * @xfer_wq: xfer done wait queue.
  * @result: controller propogated result.
+ * @bytes_cplt: number of bytes completed in the message that caused a fault.
  */
 struct nmk_i2c_dev {
 	struct i2c_vendor_data		*vendor;
@@ -218,6 +219,7 @@ struct nmk_i2c_dev {
 	int				stop;
 	struct wait_queue_head		xfer_wq;
 	int				result;
+	int				bytes_cplt;
 };
 
 /* controller's abort causes */
@@ -530,6 +532,8 @@ static int read_i2c(struct nmk_i2c_dev *priv, u16 flags)
 	int status = 0;
 	bool xfer_done;
 
+	priv->cli.xfer_bytes = 0;
+
 	mcr = load_i2c_mcr_reg(priv, flags);
 	writel(mcr, priv->virtbase + I2C_MCR);
 
@@ -655,6 +659,7 @@ static int nmk_i2c_xfer_one(struct nmk_i2c_dev *priv, u16 flags)
 	int status;
 
 	priv->i2c_sr = 0;
+	priv->bytes_cplt = 0;
 	if (flags & I2C_M_RD) {
 		/* read operation */
 		priv->cli.operation = I2C_READ;
@@ -690,6 +695,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;
+		} else {
+			/* For WRITE messages, return the number of bytes sent on bus */
+			priv->bytes_cplt = FIELD_GET(I2C_SR_LENGTH, priv->i2c_sr);
+			/* LENGTH value includes the last byte that has not been sent or ACKed */
+			if (priv->bytes_cplt > 0)
+				priv->bytes_cplt--;
+		}
 		init_hw(priv);
 
 		status = status ? status : priv->result;
@@ -699,10 +714,11 @@ static int nmk_i2c_xfer_one(struct nmk_i2c_dev *priv, u16 flags)
 }
 
 /**
- * nmk_i2c_xfer() - I2C transfer function used by kernel framework
+ * nmk_i2c_xfer_v2() - I2C transfer function used by kernel framework
  * @i2c_adap: Adapter pointer to the controller
  * @msgs: Pointer to data to be written.
  * @num_msgs: Number of messages to be executed
+ * @report: Pointer to transfer report to be written.
  *
  * This is the function called by the generic kernel i2c_transfer()
  * or i2c_smbus...() API calls. Note that this code is protected by the
@@ -745,8 +761,9 @@ 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)
 {
 	int status = 0;
 	int i;
@@ -756,6 +773,8 @@ static int nmk_i2c_xfer(struct i2c_adapter *i2c_adap,
 	if (i)
 		return i;
 
+	priv->bytes_cplt = 0;
+
 	/* setup the i2c controller */
 	setup_i2c_controller(priv);
 
@@ -774,10 +793,15 @@ static int nmk_i2c_xfer(struct i2c_adapter *i2c_adap,
 	pm_runtime_put_sync(&priv->adev->dev);
 
 	/* return the no. messages processed */
-	if (status)
+	if (status) {
+		report->msgs_cplt = i;
+		report->bytes_cplt = priv->bytes_cplt;
+		report->fault_msg_idx = i;
 		return status;
-	else
+	} else {
+		i2c_transfer_report_success(report, num_msgs);
 		return num_msgs;
+	}
 }
 
 /**
@@ -1029,7 +1053,8 @@ static unsigned int nmk_i2c_functionality(struct i2c_adapter *adap)
 }
 
 static const struct i2c_algorithm nmk_i2c_algo = {
-	.xfer = nmk_i2c_xfer,
+	.xfer = i2c_xfer_to_xfer_v2,
+	.xfer_v2 = nmk_i2c_xfer_v2,
 	.functionality = nmk_i2c_functionality
 };
 

-- 
2.43.0

[PATCH RFC v3 13/14] i2c: designware: use separate `i2c_algorithm` for AMD_NAVI_GPU

[Dmitry Guzman]

AMD_NAVI_GPU variant of designware I2C controller uses its own
implementation of `xfer` method. Instead of checking flags in runtime,
create a separate `i2c_algorithm` structure and put pointer to
`amd_i2c_dw_xfer_quirk` directly into it.

Signed-off-by: Dmitry Guzman <Dmitry.Guzman@mobileye.com>
---
 drivers/i2c/busses/i2c-designware-common.c | 14 +++++++++++++-
 drivers/i2c/busses/i2c-designware-core.h   |  1 +
 drivers/i2c/busses/i2c-designware-master.c |  6 ++----
 3 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c
index e934bcbe7660..4274bf809b3f 100644
--- a/drivers/i2c/busses/i2c-designware-common.c
+++ b/drivers/i2c/busses/i2c-designware-common.c
@@ -886,6 +886,15 @@ static const struct i2c_algorithm i2c_dw_algo = {
 #endif
 };
 
+static const struct i2c_algorithm i2c_dw_amd_navi_gpu_algo = {
+	.xfer = amd_i2c_dw_xfer_quirk,
+	.functionality = i2c_dw_func,
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+	.reg_slave = i2c_dw_reg_slave,
+	.unreg_slave = i2c_dw_unreg_slave,
+#endif
+};
+
 static const struct i2c_adapter_quirks i2c_dw_quirks = {
 	.flags = I2C_AQ_NO_ZERO_LEN,
 };
@@ -922,7 +931,10 @@ int i2c_dw_probe(struct dw_i2c_dev *dev)
 		strscpy(adap->name, "Synopsys DesignWare I2C adapter");
 
 	adap->retries = 3;
-	adap->algo = &i2c_dw_algo;
+	if ((dev->flags & MODEL_MASK) == MODEL_AMD_NAVI_GPU)
+		adap->algo = &i2c_dw_amd_navi_gpu_algo;
+	else
+		adap->algo = &i2c_dw_algo;
 	adap->quirks = &i2c_dw_quirks;
 	adap->dev.parent = dev->dev;
 	i2c_set_adapdata(adap, dev);
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index 025311c8662c..9f3492717e18 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -316,6 +316,7 @@ extern void i2c_dw_configure_master(struct dw_i2c_dev *dev);
 extern int i2c_dw_probe_master(struct dw_i2c_dev *dev);
 
 int i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num);
+int amd_i2c_dw_xfer_quirk(struct i2c_adapter *adap, struct i2c_msg *msgs, int num_msgs);
 
 #if IS_ENABLED(CONFIG_I2C_SLAVE)
 extern void i2c_dw_configure_slave(struct dw_i2c_dev *dev);
diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c
index 412ab0cd2a32..7670f909f88c 100644
--- a/drivers/i2c/busses/i2c-designware-master.c
+++ b/drivers/i2c/busses/i2c-designware-master.c
@@ -291,8 +291,9 @@ static int i2c_dw_status(struct dw_i2c_dev *dev)
  * Initiate and continue master read/write transaction with polling
  * based transfer routine afterward write messages into the Tx buffer.
  */
-static int amd_i2c_dw_xfer_quirk(struct dw_i2c_dev *dev, struct i2c_msg *msgs, int num_msgs)
+int amd_i2c_dw_xfer_quirk(struct i2c_adapter *adap, struct i2c_msg *msgs, int num_msgs)
 {
+	struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
 	int msg_wrt_idx, msg_itr_lmt, buf_len, data_idx;
 	int cmd = 0, status;
 	u8 *tx_buf;
@@ -947,9 +948,6 @@ int i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 {
 	struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
 
-	if ((dev->flags & MODEL_MASK) == MODEL_AMD_NAVI_GPU)
-		return amd_i2c_dw_xfer_quirk(dev, msgs, num);
-
 	return i2c_dw_xfer_common(dev, msgs, num);
 }
 

-- 
2.43.0

[PATCH RFC v3 14/14] i2c: designware: add support for I2C_XFER_V2 - detailed fault reporting

[Dmitry Guzman]

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; there was no way to
find out how many messages or bytes in a certain message have been sent
or received until the fault condition occurred, or to receive the data
read before the fault.

This patch introduces support for this feature in i2c-designware driver.
For AMD_NAVI_GPU model that has its own `xfer` callback function, this
feature is currently not implemented.

The driver uses FLUSH_CNT field of ABORT_SOURCE register to detect
how many command/data bytes written to FIFO were not actually sent.

Also, if the detailed fault report is required, the driver checks
status after each read/write from/to FIFO. If ABORT flag is set, the
driver cannot detect what happened first - FIFO read/write or
communication abort, so it assumes that abort happened first, otherwise
it either falsely reports one extra byte sent or returns the byte read
from FIFO flushed after abort as a valid read byte. So, in unfortunate
conditions, the fault report may be too pessimistic by one byte.

This check increases amount of time spent in interrupt handler, so if
client doesn't need detailed fault report, this check should be avoided.
Because of this, both `xfer` and `xfer_v2` callbacks are implemented in
struct i2c_algorithm: the former does not generate detailed fault
report, while the latter does.

Signed-off-by: Dmitry Guzman <Dmitry.Guzman@mobileye.com>
---
 drivers/i2c/busses/i2c-designware-common.c |   1 +
 drivers/i2c/busses/i2c-designware-core.h   |  12 +++
 drivers/i2c/busses/i2c-designware-master.c | 139 +++++++++++++++++++++++++++--
 3 files changed, 147 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c
index 4274bf809b3f..42fc5989c917 100644
--- a/drivers/i2c/busses/i2c-designware-common.c
+++ b/drivers/i2c/busses/i2c-designware-common.c
@@ -879,6 +879,7 @@ static irqreturn_t i2c_dw_isr(int this_irq, void *dev_id)
 
 static const struct i2c_algorithm i2c_dw_algo = {
 	.xfer = i2c_dw_xfer,
+	.xfer_v2 = i2c_dw_xfer_v2,
 	.functionality = i2c_dw_func,
 #if IS_ENABLED(CONFIG_I2C_SLAVE)
 	.reg_slave = i2c_dw_reg_slave,
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index 9f3492717e18..67684df05ba5 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -117,6 +117,8 @@
 						 DW_IC_TX_ABRT_10ADDR2_NOACK | \
 						 DW_IC_TX_ABRT_GCALL_NOACK)
 
+#define DW_IC_TX_ABRT_SOURCE_FLUSH_CNT_MASK	GENMASK(31, 23)
+
 struct clk;
 struct device;
 struct reset_control;
@@ -170,6 +172,7 @@ struct reset_control;
  * @release_lock: function to release a hardware lock on the bus
  * @semaphore_idx: Index of table with semaphore type attached to the bus. It's
  *	-1 if there is no semaphore.
+ * @tx_flushed: Number of bytes flushed from TX FIFO
  * @shared_with_punit: true if this bus is shared with the SoC's PUNIT
  * @set_sda_hold_time: callback to retrieve IP specific SDA hold timing
  * @mode: operation mode - DW_IC_MASTER or DW_IC_SLAVE
@@ -180,6 +183,9 @@ struct reset_control;
  *	to generate the high period and low period of SCL line.
  * @emptyfifo_hold_master: true if the controller acting as master holds
  *	the clock when the Tx FIFO is empty instead of emitting a stop.
+ * @need_precise_report: true if client needs precise fault report
+ * @bytes_written: number of bytes written to FIFO. Used only for precise
+	fault report.
  *
  * HCNT and LCNT parameters can be used if the platform knows more accurate
  * values than the one computed based only on the input clock frequency.
@@ -232,6 +238,7 @@ struct dw_i2c_dev {
 	int			(*acquire_lock)(void);
 	void			(*release_lock)(void);
 	int			semaphore_idx;
+	unsigned int		tx_flushed;
 	bool			shared_with_punit;
 	int			(*set_sda_hold_time)(struct dw_i2c_dev *dev);
 	int			mode;
@@ -239,6 +246,8 @@ struct dw_i2c_dev {
 	u32			bus_capacitance_pF;
 	bool			clk_freq_optimized;
 	bool			emptyfifo_hold_master;
+	bool			need_precise_report;
+	u32			bytes_written;
 };
 
 #define ACCESS_INTR_MASK			BIT(0)
@@ -318,6 +327,9 @@ extern int i2c_dw_probe_master(struct dw_i2c_dev *dev);
 int i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num);
 int amd_i2c_dw_xfer_quirk(struct i2c_adapter *adap, struct i2c_msg *msgs, int num_msgs);
 
+int i2c_dw_xfer_v2(struct i2c_adapter *adap, struct i2c_msg *msgs, int num,
+		   struct i2c_transfer_report *report);
+
 #if IS_ENABLED(CONFIG_I2C_SLAVE)
 extern void i2c_dw_configure_slave(struct dw_i2c_dev *dev);
 irqreturn_t i2c_dw_isr_slave(struct dw_i2c_dev *dev);
diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c
index 7670f909f88c..e0591d93cb57 100644
--- a/drivers/i2c/busses/i2c-designware-master.c
+++ b/drivers/i2c/busses/i2c-designware-master.c
@@ -11,6 +11,7 @@
 
 #define DEFAULT_SYMBOL_NAMESPACE	"I2C_DW"
 
+#include <linux/bitfield.h>
 #include <linux/delay.h>
 #include <linux/err.h>
 #include <linux/errno.h>
@@ -33,6 +34,14 @@
 #define AMD_TIMEOUT_MAX_US	250
 #define AMD_MASTERCFG_MASK	GENMASK(15, 0)
 
+static 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));
+}
+
 static int i2c_dw_set_timings_master(struct dw_i2c_dev *dev)
 {
 	unsigned int comp_param1;
@@ -198,6 +207,8 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)
 
 	i2c_dw_set_mode(dev, DW_IC_MASTER);
 
+	dev->rx_buf_len = 0;
+
 	/* If the slave address is ten bit address, enable 10BITADDR */
 	if (msgs[dev->msg_write_idx].flags & I2C_M_TEN) {
 		ic_con = DW_IC_CON_10BITADDR_MASTER;
@@ -459,6 +470,21 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
 					     cmd | *buf++);
 			}
 			tx_limit--; buf_len--;
+
+			/* If precise fault reporting is required, check if the transfer
+			 * is aborted after writing each byte.
+			 * Otherwise, if it is aborted during filling FIFO, there is no way
+			 * to know how many bytes was written to FIFO after transfer abort
+			 * and thus are not counted in FLUSH_CNT.
+			 * If we are checking abort flag after each byte, we can lose only 1 byte.
+			 */
+			if (dev->need_precise_report) {
+				if (i2c_dw_check_abort_flag(dev)) {
+					dev_warn_ratelimited(dev->dev, "Transfer aborted during FIFO writing. Report may be imprecise.");
+					break;
+				}
+				dev->bytes_written++;
+			}
 		}
 
 		dev->tx_buf = buf;
@@ -563,6 +589,13 @@ i2c_dw_read(struct dw_i2c_dev *dev)
 
 		for (; len > 0 && rx_valid > 0; len--, rx_valid--) {
 			regmap_read(dev->map, DW_IC_DATA_CMD, &tmp);
+			if (dev->need_precise_report) {
+				if (i2c_dw_check_abort_flag(dev)) {
+					dev_warn_ratelimited(dev->dev, "Transfer aborted during FIFO reading. Data byte may be lost");
+					/* It is unknown if the read byte is valid. Drop it. */
+					break;
+				}
+			}
 			tmp &= DW_IC_DATA_CMD_DAT;
 			/* Ensure length byte is a valid value */
 			if (flags & I2C_M_RECV_LEN) {
@@ -589,8 +622,10 @@ i2c_dw_read(struct dw_i2c_dev *dev)
 			dev->rx_buf_len = len;
 			dev->rx_buf = buf;
 			return;
-		} else
+		} else {
 			dev->status &= ~STATUS_READ_IN_PROGRESS;
+			dev->rx_buf_len = 0;
+		}
 	}
 }
 
@@ -786,6 +821,8 @@ __i2c_dw_xfer_one_part(struct dw_i2c_dev *dev, struct i2c_msg *msgs, size_t num)
 	dev->status = 0;
 	dev->abort_source = 0;
 	dev->rx_outstanding = 0;
+	dev->bytes_written = 0;
+	dev->tx_flushed = 0;
 
 	ret = i2c_dw_wait_bus_not_busy(dev);
 	if (ret < 0)
@@ -797,6 +834,9 @@ __i2c_dw_xfer_one_part(struct dw_i2c_dev *dev, struct i2c_msg *msgs, size_t num)
 	/* Wait for tx to complete */
 	ret = i2c_dw_wait_transfer(dev);
 	if (ret) {
+		if (dev->need_precise_report)
+			regmap_read(dev->map, DW_IC_TXFLR, &dev->tx_flushed);
+
 		dev_err(dev->dev, "controller timed out\n");
 		/* i2c_dw_init() implicitly disables the adapter */
 		i2c_recover_bus(&dev->adapter);
@@ -888,13 +928,72 @@ i2c_dw_msg_is_valid(struct dw_i2c_dev *dev, const struct i2c_msg *msgs, size_t i
 	return true;
 }
 
+static int i2c_dw_fault_report(struct dw_i2c_dev *dev, struct i2c_transfer_report *report)
+{
+	unsigned int idx;
+
+	/*
+	 * Get the number of bytes that were put into TX FIFO but were not sent on wire.
+	 * If abort IRQ was received, this number is stored in abort_source register.
+	 * Otherwise, if ic_dw_wait_transfer() returned timeout, use TXFLR register value
+	 * read before the controller was reset.
+	 */
+	if (dev->abort_source)
+		dev->tx_flushed = FIELD_GET(DW_IC_TX_ABRT_SOURCE_FLUSH_CNT_MASK, dev->abort_source);
+
+	if (dev->tx_flushed <= dev->bytes_written)
+		dev->bytes_written -= dev->tx_flushed;
+	else
+		dev->bytes_written = 0;
+
+	/*
+	 * The last byte that the transmission was interrupted on is not counted as flushed.
+	 * We should not report it as transferred successfully, so decrement the counter.
+	 */
+	if (dev->bytes_written)
+		dev->bytes_written--;
+
+	for (idx = 0; idx < dev->msg_write_idx; idx++) {
+		if (dev->msgs[idx].len <= dev->bytes_written)
+			dev->bytes_written -= dev->msgs[idx].len;
+		else
+			break;
+	}
+
+	report->fault_msg_idx = idx;
+	for (int i = dev->msg_read_idx; i <= idx && i < dev->msgs_num; i++) {
+		if (dev->msgs[i].flags & I2C_M_RD) {
+			report->msgs_cplt = i;
+			if (!(dev->rx_buf_len))
+				report->bytes_cplt = 0;
+			else
+				report->bytes_cplt = dev->rx_buf - dev->msgs[i].buf;
+			if ((i < idx) || (report->bytes_cplt < dev->bytes_written))
+				dev_warn_ratelimited(dev->dev, "Read data lost due to FIFO flush");
+			break;
+		}
+	}
+
+	if (report->msgs_cplt < 0) {
+		report->msgs_cplt = idx;
+		report->bytes_cplt = dev->bytes_written;
+	}
+
+	return idx;
+}
+
 static int
-i2c_dw_xfer_common(struct dw_i2c_dev *dev, struct i2c_msg msgs[], int num)
+i2c_dw_xfer_common_v2(struct dw_i2c_dev *dev, struct i2c_msg *msgs, int num,
+		      struct i2c_transfer_report *report)
 {
+	int msgs_in_prev_parts = 0;
 	struct i2c_msg *msgs_part;
-	size_t cnt;
+	size_t cnt = 0;
 	int ret;
 
+	dev->need_precise_report = !!report;
+	dev->msg_read_idx = 0;
+
 	dev_dbg(dev->dev, "msgs: %d\n", num);
 
 	PM_RUNTIME_ACQUIRE_AUTOSUSPEND(dev->dev, pm);
@@ -910,6 +1009,8 @@ i2c_dw_xfer_common(struct dw_i2c_dev *dev, struct i2c_msg msgs[], int num)
 	 * we do one transaction for each part up to the STOP.
 	 */
 	for (msgs_part = msgs; msgs_part < msgs + num; msgs_part += cnt) {
+		/* Count previously transferred messages */
+		msgs_in_prev_parts += cnt;
 		/*
 		 * Count the messages in a transaction, up to a STOP or
 		 * the end of the msgs. The last if below guarantees that
@@ -918,6 +1019,16 @@ i2c_dw_xfer_common(struct dw_i2c_dev *dev, struct i2c_msg msgs[], int num)
 		 */
 		for (cnt = 1; ; cnt++) {
 			if (!i2c_dw_msg_is_valid(dev, msgs_part, cnt - 1)) {
+				if (report) {
+					i2c_transfer_report_bad_msg(report,
+								    msgs_in_prev_parts + cnt - 1);
+					report->msgs_cplt = msgs_in_prev_parts;
+					/*
+					 * Reset report pointer to avoid
+					 * calling i2c_dw_fault_report() later.
+					 */
+					report = NULL;
+				}
 				ret = -EOPNOTSUPP;
 				break;
 			}
@@ -939,16 +1050,34 @@ 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;
+	}
+
+	if (report)
+		i2c_transfer_report_success(report, num);
+
 	return num;
 }
 
+int i2c_dw_xfer_v2(struct i2c_adapter *adap, struct i2c_msg *msgs, int num,
+		   struct i2c_transfer_report *report)
+{
+	struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
+
+	return i2c_dw_xfer_common_v2(dev, msgs, num, report);
+}
+
 int i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 {
 	struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
 
-	return i2c_dw_xfer_common(dev, msgs, num);
+	return i2c_dw_xfer_common_v2(dev, msgs, num, NULL);
 }
 
 void i2c_dw_configure_master(struct dw_i2c_dev *dev)

-- 
2.43.0

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

[sashiko-bot]

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

Re: [PATCH RFC v3 08/14] i2c: uapi: add I2C_XFER_V2 ioctl - detailed fault reporting

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] i2c: uapi: commit message refers to I2C_XFER_V2 instead of I2C_RDWR_V2
--

commit 469565334f01910f1574884cc7c0897afa2334bd
Author: Dmitry Guzman <Dmitry.Guzman@mobileye.com>

i2c: uapi: add I2C_XFER_V2 ioctl - detailed fault reporting

This commit adds the I2C_RDWR_V2 ioctl command and struct
i2c_rdwr_v2_ioctl_data to include/uapi/linux/i2c-dev.h to support
multi-message I2C transfers with detailed fault reporting.

>     i2c: uapi: add I2C_XFER_V2 ioctl - detailed fault reporting
>     
>     For user-space access to I2C multi-message transfer with detailed
>     reporting functionality, add I2C_XFER_V2 ioctl and corresponding
>     struct i2c_rdwr_v2_ioctl_data.
>     
>     The structure consists of struct i2c_rdwr_ioctl_data that used in the
>     same way as in I2C_XFER, and struct i2c_transfer_report that the
>     detailed transfer report is put in.
>     
>     Signed-off-by: Dmitry Guzman <Dmitry.Guzman@mobileye.com>

[Severity: Low]
This isn't a bug, but the commit subject and message refer to I2C_XFER_V2
and I2C_XFER, whereas include/uapi/linux/i2c-dev.h defines I2C_RDWR_V2
alongside the existing I2C_RDWR ioctl:

include/uapi/linux/i2c-dev.h:
#define I2C_RDWR	0x0707	/* Combined R/W transfer (one STOP only) */
#define I2C_RDWR_V2	0x0709  /* I2C_RDWR with detailed fault reporting */

Should the commit subject and message be updated to reference I2C_RDWR_V2
and I2C_RDWR instead?

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

Re: [PATCH RFC v3 10/14] i2c: doc: add section for I2C_RDWR_V2 ioctl

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] i2c: dev: NULL msgs with non-zero nmsgs accepted in i2cdev_ioctl()
- [Low] i2c: doc: inaccurate fault_msg_idx description in dev-interface.rst
- [Low] i2c: core: mismatched kernel-doc identifier above i2c_transfer()
--

commit 417441f5bb2c43e4da176d457da7a5caf43a4ed0
Author: Dmitry Guzman <Dmitry.Guzman@mobileye.com>

i2c: doc: add section for I2C_RDWR_V2 ioctl

This series introduces the I2C_RDWR_V2 ioctl and i2c_transfer_v2() API to
provide detailed fault reporting for I2C transfers, along with the
corresponding documentation in Documentation/i2c/dev-interface.rst.

> diff --git a/Documentation/i2c/dev-interface.rst b/Documentation/i2c/dev-interface.rst
> index c277a8e1202b5..95e60bdb2f4d0 100644
> --- a/Documentation/i2c/dev-interface.rst
> +++ b/Documentation/i2c/dev-interface.rst
> @@ -140,6 +140,54 @@ The following IOCTLs are defined:
>    The slave address and whether to use ten bit address mode has to be
>    set in each message, overriding the values set with the above ioctl's.
>  
> +``ioctl(file, I2C_RDWR_V2, struct i2c_rdwr_v2_ioctl_data *msgset)``
> +  Does the same combined read/write transaction as I2C_RDWR, but also
> +  provides detailed fault report. The argument is a pointer to a::
> +
> +    struct i2c_rdwr_v2_ioctl_data {
> +	    struct i2c_rdwr_ioctl_data rdwr_data;
> +	    struct i2c_transfer_report report;
> +    };
> +
> +  rdwr_data is the same structure as the argument for I2C_RDWR ioctl.
> +  report is the structure that the transfer report is written to::
> +
> +    struct i2c_transfer_report {
> +      __s32 fault_msg_idx;
> +      __s32 msgs_cplt;
> +      __s32 bytes_cplt;
> +    };
> +
> +  msgs_cplt is the number of messages that has been sent or received
> +  successfully. If there are read messages within this range, the returned
> +  data is guaranteed to be valid. If a message has been read from the
> +  device but the read data is lost (for example, FIFO is flushed before
> +  CPU read it), this message must not be counted.  If the controller cannot
> +  determine the number of completed messages, the value is -EOPNOTSUPP.
> +
> +  fault_msg_idx is the number of messages that caused a fault. In case of a
> +  fault, it is not necessary equal to msgs_cplt. For example, if the driver
> +  validates the whole batch before starting transmission, detects that it
> +  cannot send it, it returns -EOPNOTSUPP error immediately, so msgs_cplt is 0,
> +  while fault_msg_idx points to the message that cannot be sent. Another
> +  example when these two value may be different is I2C controller that
> +  flushes RX FIFO when an error is detected before CPU reads data from it.

[Severity: Low]
In Documentation/i2c/dev-interface.rst under the I2C_RDWR_V2 description, is
describing fault_msg_idx as "the number of messages that caused a fault"
accurate?

In include/uapi/linux/i2c.h, struct i2c_transfer_report defines fault_msg_idx
as the 0-based index of the message that caused the fault, or a negative error
code (-EOPNOTSUPP via i2c_transfer_report_init()) if the bus driver cannot
determine it:

    struct i2c_transfer_report {
        __s32 fault_msg_idx; /* In case of a fault, index of the message that caused
                              * the fault. If the bus driver cannot determine it, it
                              * puts a negative error code. If there is no fault, the
                              * value is equal to number of messages transferred.
                              */

Should this paragraph state that fault_msg_idx is the index of the message
that caused a fault, and also mention that its value is -EOPNOTSUPP if the
controller cannot determine which message caused the fault?

[ ... ]

> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 165ec14260c88..e8449f55f0452 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c

[ ... ]

> +/**
> + * 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.
> @@ -2371,35 +2453,18 @@ EXPORT_SYMBOL(__i2c_transfer);
>   */
>  int i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
>  {
> -	int ret;
> -
> -	/* REVISIT the fault reporting model here is weak:
> -	 *
> -	 *  - When we get an error after receiving N bytes from a slave,
> -	 *    there is no way to report "N".
> -	 *
> -	 *  - When we get a NAK after transmitting N bytes to a slave,
> -	 *    there is no way to report "N" ... or to let the master
> -	 *    continue executing the rest of this combined message, if
> -	 *    that's the appropriate response.
> -	 *
> -	 *  - When for example "num" is two and we successfully complete
> -	 *    the first message but get an error part way through the
> -	 *    second, it's unclear whether that should be reported as
> -	 *    one (discarding status on the second message) or errno
> -	 *    (discarding status on the first one).
> -	 */
> -	ret = __i2c_lock_bus_helper(adap);
> -	if (ret)
> -		return ret;
> -
> -	ret = __i2c_transfer(adap, msgs, num);
> -	i2c_unlock_bus(adap, I2C_LOCK_SEGMENT);
> -
> -	return ret;
> +	return i2c_transfer_v2(adap, msgs, num, NULL);
>  }

[Severity: Low]
This isn't a runtime bug, but the kernel-doc comment immediately above
i2c_transfer() in drivers/i2c/i2c-core-base.c was renamed to i2c_transfer_v2,
which causes a kernel-doc warning when building with W=1. Should the
identifier in the comment header remain i2c_transfer?

> diff --git a/drivers/i2c/i2c-dev.c b/drivers/i2c/i2c-dev.c
> index d219eb89f978b..14fe1dd7eecf4 100644
> --- a/drivers/i2c/i2c-dev.c
> +++ b/drivers/i2c/i2c-dev.c

[ ... ]

> @@ -439,18 +482,39 @@ static long i2cdev_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  		funcs = i2c_get_functionality(client->adapter);
>  		return put_user(funcs, (unsigned long __user *)arg);
>  
> -	case I2C_RDWR: {
> +	case I2C_RDWR:
> +	case I2C_RDWR_V2:
> +	{
> +		struct i2c_rdwr_ioctl_data __user *user_arg;
> +		struct i2c_transfer_report __user *user_rep;
>  		struct i2c_rdwr_ioctl_data rdwr_arg;
>  		struct i2c_msg *rdwr_pa;
>  		int res;
>  
> -		if (copy_from_user(&rdwr_arg,
> -				   (struct i2c_rdwr_ioctl_data __user *)arg,
> -				   sizeof(rdwr_arg)))
> +		if (cmd == I2C_RDWR_V2) {
> +			user_arg = &((struct i2c_rdwr_v2_ioctl_data __user *)arg)->rdwr_data;
> +			user_rep = &((struct i2c_rdwr_v2_ioctl_data __user *)arg)->report;
> +		} else {
> +			user_arg = (struct i2c_rdwr_ioctl_data __user *)arg;
> +			user_rep = NULL;
> +		}
> +
> +		if (copy_from_user(&rdwr_arg, user_arg, sizeof(rdwr_arg)))
>  			return -EFAULT;
>  
> -		if (!rdwr_arg.msgs || rdwr_arg.nmsgs == 0)
> -			return -EINVAL;
> +		if (!rdwr_arg.msgs || rdwr_arg.nmsgs == 0) {
> +			/*
> +			 * I2C_RDWR_V2 ioctl with nmsgs == 0 is used for
> +			 * discovering of the controller capability to return
> +			 * detailed fault reports.
> +			 */
> +			if (cmd == I2C_RDWR)
> +				return -EINVAL;
> +			if (client->adapter->algo->xfer_v2)
> +				return 0;
> +			else
> +				return -EOPNOTSUPP;
> +		}

[Severity: Medium]
In i2cdev_ioctl() (and the identical check in compat_i2cdev_ioctl()) in
drivers/i2c/i2c-dev.c, what happens if user space invokes I2C_RDWR_V2 with
rdwr_arg.msgs == NULL and rdwr_arg.nmsgs > 0?

Because both conditions are combined into:

    if (!rdwr_arg.msgs || rdwr_arg.nmsgs == 0)

a call with a NULL msgs pointer and non-zero nmsgs enters the capability
discovery branch and returns 0 when client->adapter->algo->xfer_v2 is set,
without transferring any messages or initializing the user-space report
structure.

Should capability discovery only trigger when rdwr_arg.nmsgs == 0, while
!rdwr_arg.msgs with non-zero nmsgs returns -EINVAL in both i2cdev_ioctl() and
compat_i2cdev_ioctl()?

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

Re: [PATCH RFC v3 09/14] i2c: core: implement I2C_RDWR_V2 ioctl - transfer with detailed report

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] i2c: dev: missing bytes_cplt bounds check in i2cdev_ioctl_rdwr_v2()
- [High] i2c: missing report reset and bounds checks in __i2c_transfer_v2()
- [Medium] i2c: dev: I2C_RDWR_V2 with NULL msgs and non-zero nmsgs returns 0 in i2cdev_ioctl()
- [Medium] i2c: dev: missing algo->xfer_v2 check in i2cdev_ioctl_rdwr_v2()
- [Low] i2c: dev: leftover fixme, sign-compare, and logging in i2cdev_ioctl_rdwr_v2()
--

commit 7d55aa9159adadd2b0feb48eabc72a0f050719a1
Author: Dmitry Guzman <Dmitry.Guzman@mobileye.com>

i2c: core: implement I2C_RDWR_V2 ioctl - transfer with detailed report

This commit adds the I2C_RDWR_V2 ioctl (and its compat counterpart) to
i2c-dev so userspace can receive an i2c_transfer_report describing completed
messages, bytes transferred, and the faulting message index when a transfer
fails.

> diff --git a/drivers/i2c/i2c-dev.c b/drivers/i2c/i2c-dev.c
> index d219eb89f978b..14fe1dd7eecf4 100644
> --- a/drivers/i2c/i2c-dev.c
> +++ b/drivers/i2c/i2c-dev.c
> @@ -240,11 +240,19 @@ static int i2cdev_check_addr(struct i2c_adapter *adapter, unsigned int addr)
>  	return result;
>  }
>  
> -static noinline int i2cdev_ioctl_rdwr(struct i2c_client *client,
> -		unsigned nmsgs, struct i2c_msg *msgs)
> +static noinline int i2cdev_ioctl_rdwr_v2(struct i2c_client *client,
> +					 unsigned int nmsgs, struct i2c_msg *msgs,
> +					 struct i2c_transfer_report __user *user_report)
>  {
> +	struct i2c_transfer_report report;
>  	u8 __user **data_ptrs;
> -	int i, res;
> +	int i, res, msgs_done;
> +
> +	i2c_transfer_report_init(&report);
> +
> +	if (user_report)
> +		if (copy_to_user(user_report, &report, sizeof(report)))
> +			return -EFAULT;
>  
>  	/* Adapter must support I2C transfers */
>  	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
>  		return -EOPNOTSUPP;

[Severity: Medium]
When user_report is non-NULL (for I2C_RDWR_V2 with nmsgs > 0), should
i2cdev_ioctl_rdwr_v2() also verify client->adapter->algo->xfer_v2 before
allocating and validating the messages?

In drivers/i2c/i2c-core-base.c, __i2c_transfer_v2() initializes the report
fields to -EOPNOTSUPP and immediately returns -EOPNOTSUPP when
adap->algo->xfer_v2 is NULL:

drivers/i2c/i2c-core-base.c:__i2c_transfer_v2() {
	if (report) {
		i2c_transfer_report_init(report);

		if (!adap->algo->xfer_v2)
			return -EOPNOTSUPP;
	}
}

Because i2cdev_ioctl_rdwr_v2() only checks algo->xfer_v2 later via
i2c_transfer_v2(), calling I2C_RDWR_V2 on an adapter without xfer_v2 with a
message that fails validation (such as msgs[i].len > 8192, a memdup_user()
failure, or an invalid I2C_M_RECV_LEN buffer) will overwrite report via
i2c_transfer_report_bad_msg(&report, i) and copy that to user_report instead
of leaving the report fields set to -EOPNOTSUPP and returning -EOPNOTSUPP.
Even when all messages are valid, it allocates data_ptrs and duplicates every
message buffer with memdup_user() before __i2c_transfer_v2() rejects the
transfer.

> @@ -259,6 +267,7 @@ static noinline int i2cdev_ioctl_rdwr(struct i2c_client *client,
>  		/* Limit the size of the message to a sane amount */
>  		if (msgs[i].len > 8192) {
>  			res = -EINVAL;
> +			i2c_transfer_report_bad_msg(&report, i);
>  			break;
>  		}
>  
> @@ -266,6 +275,7 @@ static noinline int i2cdev_ioctl_rdwr(struct i2c_client *client,
>  		msgs[i].buf = memdup_user(data_ptrs[i], msgs[i].len);
>  		if (IS_ERR(msgs[i].buf)) {
>  			res = PTR_ERR(msgs[i].buf);
> +			i2c_transfer_report_bad_msg(&report, i);
>  			break;
>  		}
>  		/* memdup_user allocates with GFP_KERNEL, so DMA is ok */
> @@ -287,6 +297,7 @@ static noinline int i2cdev_ioctl_rdwr(struct i2c_client *client,
>  			    msgs[i].len < 1 || msgs[i].buf[0] < 1 ||
>  			    msgs[i].len < msgs[i].buf[0] +
>  					     I2C_SMBUS_BLOCK_MAX) {
> +				i2c_transfer_report_bad_msg(&report, i);
>  				i++;
>  				res = -EINVAL;
>  				break;
> @@ -300,12 +311,44 @@ static noinline int i2cdev_ioctl_rdwr(struct i2c_client *client,
>  		for (j = 0; j < i; ++j)
>  			kfree(msgs[j].buf);
>  		kfree(data_ptrs);
> +		if (user_report && copy_to_user(user_report, &report, sizeof(report)))
> +			res = -EFAULT;
>  		return res;
>  	}
>  
> -	res = i2c_transfer(client->adapter, msgs, nmsgs);
> +	if (user_report) {
> +		res = i2c_transfer_v2(client->adapter, msgs, nmsgs, &report);
> +		msgs_done = report.msgs_cplt;

[Severity: High]
Passing a non-NULL &report into i2c_transfer_v2() -> __i2c_transfer_v2()
activates the report paths in drivers/i2c/i2c-core-base.c:

drivers/i2c/i2c-core-base.c:__i2c_transfer_v2() {
	...
	/* 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) {
			ret = adap->algo->master_xfer_atomic(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;
		if (time_after(jiffies, orig_jiffies + adap->timeout))
			break;
	}

	if (static_branch_unlikely(&i2c_trace_msg_key)) {
		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);
		trace_i2c_result(adap, num, ret);
	}
}

Could this cause out-of-bounds accesses on msgs[] or stale report data in
__i2c_transfer_v2()?

First, i2c_transfer_report_init(report) is called once before the retry loop
rather than before each retry attempt. If an attempt returns -EAGAIN on
arbitration loss after adap->algo->xfer_v2() has populated report, the next
retry starts with non-negative report->msgs_cplt and report->bytes_cplt from
the previous attempt (for example, in i2c_dw_fault_report() where
report->msgs_cplt < 0 guards initial assignment and report->msgs_cplt +=
msgs_in_prev_parts accumulates across parts).

Second, when i2c_trace_msg_key tracepoints are enabled, __i2c_transfer_v2()
uses n = report->msgs_cplt in the loop and indexes msgs[n] when
report->bytes_cplt > 0 without checking that 0 <= n < num or that
report->bytes_cplt <= msgs[n].len, whereas the bounds checks in
i2cdev_ioctl_rdwr_v2() only run after i2c_transfer_v2() returns.

Third, if i2c_in_atomic_xfer_mode() && adap->algo->master_xfer_atomic is
taken while report is non-NULL, master_xfer_atomic() does not update report,
leaving report->msgs_cplt set to -EOPNOTSUPP even on a successful transfer.

> +	} else {
> +		res = i2c_transfer(client->adapter, msgs, nmsgs);
> +		if (res < 0)
> +			msgs_done = 0;
> +		else
> +			msgs_done = nmsgs;
> +	}
> +
> +	if (user_report && copy_to_user(user_report, &report, sizeof(report)))
> +		res = -EFAULT;
> +
> +	if (msgs_done > (int)nmsgs) {
> +		//fixme - use dev_err?
> +		pr_err("Bad i2c_transfer_report: msgs_cplt = %i, bytes_cplt = %i, nmsgs = %i\n",
> +		       report.msgs_cplt, report.bytes_cplt, nmsgs);
> +		msgs_done = nmsgs;
> +	}
> +
> +	if (report.bytes_cplt > 0) {
> +		if (msgs_done < nmsgs && msgs_done >= 0) {
> +			msgs[msgs_done].len = report.bytes_cplt;
> +			msgs_done++;
> +		} else {
> +			pr_err("Bad i2c_transfer_report: msgs_cplt = %i, bytes_cplt = %i, nmsgs = %i\n",
> +				report.msgs_cplt, report.bytes_cplt, nmsgs);
> +		}
> +	}

[Severity: Low]
In i2cdev_ioctl_rdwr_v2(), there is a leftover "//fixme - use dev_err?"
comment above pr_err(). Should these pr_err() calls use
dev_err(&client->adapter->dev, ...) instead, and format unsigned int nmsgs
with %u?

Also, in the check:

	if (msgs_done < nmsgs && msgs_done >= 0)

msgs_done is a signed int while nmsgs is an unsigned int (unlike the earlier
check which casts nmsgs to int). Because of usual arithmetic conversions,
msgs_done is converted to unsigned int in msgs_done < nmsgs, which triggers a
-Wsign-compare warning under W=1 and makes msgs_done >= 0 redundant.

Finally, if a driver reports both report.msgs_cplt > nmsgs and
report.bytes_cplt > 0, the first block logs "Bad i2c_transfer_report" and
clamps msgs_done = nmsgs, which then causes msgs_done < nmsgs in the second
block to be false and logs the same error message a second time, all after
report was already copied to user_report.

> +
>  	while (i-- > 0) {
> -		if (res >= 0 && (msgs[i].flags & I2C_M_RD)) {
> +		if (msgs[i].flags & I2C_M_RD && i < msgs_done) {
>  			if (copy_to_user(data_ptrs[i], msgs[i].buf,
>  					 msgs[i].len))
>  				res = -EFAULT;

[Severity: High]
Can copy_to_user() read past the end of the allocated msgs[i].buf buffer in
i2cdev_ioctl_rdwr_v2()?

Earlier in i2cdev_ioctl_rdwr_v2(), each kernel buffer is allocated with the
original message length:

	msgs[i].buf = memdup_user(data_ptrs[i], msgs[i].len);

When report.bytes_cplt > 0 and 0 <= msgs_done < nmsgs, the code overwrites
msgs[msgs_done].len without checking that report.bytes_cplt does not exceed
the allocated msgs[msgs_done].len:

	if (report.bytes_cplt > 0) {
		if (msgs_done < nmsgs && msgs_done >= 0) {
			msgs[msgs_done].len = report.bytes_cplt;
			msgs_done++;
		}
	}

If a bus driver sets report.bytes_cplt to a value larger than
msgs[msgs_done].len (or if msgs[i].len was reduced to msgs[i].buf[0] for
I2C_M_RECV_LEN), copy_to_user(data_ptrs[i], msgs[i].buf, msgs[i].len) will
read out of bounds from the kernel heap buffer msgs[i].buf and copy past the
end of the userspace buffer. Should report.bytes_cplt be validated against
msgs[msgs_done].len before assigning it?

> @@ -439,18 +482,39 @@ static long i2cdev_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  		funcs = i2c_get_functionality(client->adapter);
>  		return put_user(funcs, (unsigned long __user *)arg);
>  
> -	case I2C_RDWR: {
> +	case I2C_RDWR:
> +	case I2C_RDWR_V2:
> +	{
> +		struct i2c_rdwr_ioctl_data __user *user_arg;
> +		struct i2c_transfer_report __user *user_rep;
>  		struct i2c_rdwr_ioctl_data rdwr_arg;
>  		struct i2c_msg *rdwr_pa;
>  		int res;
>  
> -		if (copy_from_user(&rdwr_arg,
> -				   (struct i2c_rdwr_ioctl_data __user *)arg,
> -				   sizeof(rdwr_arg)))
> +		if (cmd == I2C_RDWR_V2) {
> +			user_arg = &((struct i2c_rdwr_v2_ioctl_data __user *)arg)->rdwr_data;
> +			user_rep = &((struct i2c_rdwr_v2_ioctl_data __user *)arg)->report;
> +		} else {
> +			user_arg = (struct i2c_rdwr_ioctl_data __user *)arg;
> +			user_rep = NULL;
> +		}
> +
> +		if (copy_from_user(&rdwr_arg, user_arg, sizeof(rdwr_arg)))
>  			return -EFAULT;
>  
> -		if (!rdwr_arg.msgs || rdwr_arg.nmsgs == 0)
> -			return -EINVAL;
> +		if (!rdwr_arg.msgs || rdwr_arg.nmsgs == 0) {
> +			/*
> +			 * I2C_RDWR_V2 ioctl with nmsgs == 0 is used for
> +			 * discovering of the controller capability to return
> +			 * detailed fault reports.
> +			 */
> +			if (cmd == I2C_RDWR)
> +				return -EINVAL;
> +			if (client->adapter->algo->xfer_v2)
> +				return 0;
> +			else
> +				return -EOPNOTSUPP;
> +		}

[Severity: Medium]
In both i2cdev_ioctl() and compat_i2cdev_ioctl(), what happens when a caller
invokes I2C_RDWR_V2 with rdwr_arg.nmsgs > 0 and rdwr_arg.msgs == NULL?

Because !rdwr_arg.msgs is combined with rdwr_arg.nmsgs == 0 in the same
condition, passing a NULL msgs pointer with a non-zero nmsgs enters the
capability discovery branch and returns 0 (when algo->xfer_v2 is present)
without transferring any messages or updating user_rep, instead of returning
-EINVAL. Should capability discovery only trigger when rdwr_arg.nmsgs == 0,
and return -EINVAL when rdwr_arg.msgs is NULL with rdwr_arg.nmsgs > 0?

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

Re: [PATCH RFC v3 11/14] i2c: nomadik: save I2C_CR register before resetting HW in IRQ handler

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] i2c: nomadik: commit subject refers to I2C_CR register instead of I2C_SR
--

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

i2c: nomadik: save I2C_CR register before resetting HW in IRQ handler

This commit adds an i2c_sr field to struct nmk_i2c_dev to save the
I2C_SR status register value in i2c_irq_handler() before calling
init_hw(), preserving the status for fault reporting in
nmk_i2c_xfer_one().

>     i2c: nomadik: save I2C_CR register before resetting HW in IRQ handler
>     
>     On some interrupts, such as Bus Error interrupt or Arbitration Lost
>     interrupt, the interrupt handler calls init_hw() to reset the I2C
>     controller. This clears I2C_SR register value that might be used for
>     fault reporting.
>     
>     Add i2c_sr field to nmk_i2c_dev structure, store there I2C_SR register
>     value before resetting the hardware, and use this value instead of
>     reading it directly in nmk_i2c_xfer_one() if it was read in IRQ handler.
>     
>     Signed-off-by: Dmitry Guzman <Dmitry.Guzman@mobileye.com>

[Severity: Low]
This isn't a bug, but should the commit subject refer to the I2C_SR
register instead of I2C_CR? Both the commit message body and the changes
in drivers/i2c/busses/i2c-nomadik.c (in struct nmk_i2c_dev,
nmk_i2c_xfer_one(), and i2c_irq_handler()) save and inspect I2C_SR rather
than the I2C_CR control register.

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

Re: [PATCH RFC v3 12/14] i2c: nomadik: add support for I2C_XFER_V2 - detailed fault reporting

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] i2c: nomadik: unpopulated transfer report on pm_runtime_get_sync() early exit
--

commit 22d1a25cd553d39ed6c3ad6c9998ec471c05f948
Author: Dmitry Guzman <Dmitry.Guzman@mobileye.com>

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

This commit converts the i2c-nomadik driver to the xfer_v2 callback so it can
populate struct i2c_transfer_report with the number of completed messages,
completed bytes, and the faulting message index when a transfer fails.

> diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
> index 958adc51615e4..a8ccd863a6edf 100644
> --- a/drivers/i2c/busses/i2c-nomadik.c
> +++ b/drivers/i2c/busses/i2c-nomadik.c

[ ... ]

> @@ -739,8 +755,9 @@ 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)
>  {
>  	int status = 0;
>  	int i;
> @@ -750,6 +767,8 @@ static int nmk_i2c_xfer(struct i2c_adapter *i2c_adap,
>  	if (i)
>  		return i;
>  
> +	priv->bytes_cplt = 0;
> +
>  	/* setup the i2c controller */
>  	setup_i2c_controller(priv);
>  
> @@ -768,10 +787,15 @@ static int nmk_i2c_xfer(struct i2c_adapter *i2c_adap,
>  	pm_runtime_put_sync(&priv->adev->dev);
>  
>  	/* return the no. messages processed */
> -	if (status)
> +	if (status) {
> +		report->msgs_cplt = i;
> +		report->bytes_cplt = priv->bytes_cplt;
> +		report->fault_msg_idx = i;
>  		return status;
> -	else
> +	} else {
> +		i2c_transfer_report_success(report, num_msgs);
>  		return num_msgs;
> +	}
>  }

[Severity: Medium]
What happens to the transfer report in drivers/i2c/busses/i2c-nomadik.c when
nmk_i2c_xfer_v2() takes the early return on pm_runtime_get_sync()?

Before invoking adap->algo->xfer_v2(), __i2c_transfer_v2() in
drivers/i2c/i2c-core-base.c initializes all fields of struct
i2c_transfer_report to -EOPNOTSUPP via i2c_transfer_report_init(report):

drivers/i2c/i2c-core-base.c:__i2c_transfer_v2() {
	...
	if (adap->algo->xfer_v2) {
		i2c_transfer_report_init(report);
		...
		ret = adap->algo->xfer_v2(adap, msgs, num, report);
	...
}

In nmk_i2c_xfer_v2(), if pm_runtime_get_sync(&priv->adev->dev) returns a
non-zero value, the function returns immediately before reaching the block at
the end of nmk_i2c_xfer_v2() that populates report:

	i = pm_runtime_get_sync(&priv->adev->dev);
	if (i)
		return i;

If pm_runtime_get_sync() fails with a negative error code, 0 messages and 0
bytes have been transferred before failing on message 0, but report->msgs_cplt,
report->bytes_cplt, and report->fault_msg_idx remain -EOPNOTSUPP instead of 0,
telling callers of i2c_transfer_v2() and the I2C_RDWR_V2 ioctl that the
controller does not support fault reporting.

In addition, if pm_runtime_get_sync() returns 1 because the device was already
active, nmk_i2c_xfer_v2() returns 1 while leaving all fields of report set to
-EOPNOTSUPP, resulting in a positive return value paired with a negative
report->msgs_cplt.

Should nmk_i2c_xfer_v2() populate report (or use pm_runtime_resume_and_get()
and set msgs_cplt, bytes_cplt, and fault_msg_idx to 0 on error) before
returning when runtime resume fails?

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

Re: [PATCH RFC v3 14/14] i2c: designware: add support for I2C_XFER_V2 - detailed fault reporting

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] i2c: designware: stale rx_buf_len used in i2c_dw_fault_report()
- [High] i2c: designware: stale msgs_cplt across retries in i2c_dw_fault_report()
- [Medium] i2c: designware: missing outer loop break on abort in i2c_dw_xfer_msg()
- [Medium] i2c: designware: unread DW_IC_TXFLR on early stop in __i2c_dw_xfer_one_part()
- [Low] i2c: designware: unaddressed v2 review feedback on style and loop index type
--

commit 99b998b5107033a19225ae77e0693b6c68191b43
Author: Dmitry Guzman <Dmitry.Guzman@mobileye.com>

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

This commit adds i2c_dw_xfer_v2() and fault reporting support to the
DesignWare I2C master driver using the ABORT_SOURCE FLUSH_CNT field,
TXFLR register, and per-byte abort checks in the FIFO read and write
loops.

> diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c
> index 14350c53a5241..c7efb8551aa30 100644
> --- a/drivers/i2c/busses/i2c-designware-master.c
> +++ b/drivers/i2c/busses/i2c-designware-master.c
> @@ -11,6 +11,7 @@
>  
>  #define DEFAULT_SYMBOL_NAMESPACE	"I2C_DW"
>  
> +#include <linux/bitfield.h>
>  #include <linux/delay.h>
>  #include <linux/err.h>
>  #include <linux/errno.h>
> @@ -33,6 +34,14 @@
>  #define AMD_TIMEOUT_MAX_US	250
>  #define AMD_MASTERCFG_MASK	GENMASK(15, 0)
>  
> +static 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));
> +}
> +
>  static int i2c_dw_set_timings_master(struct dw_i2c_dev *dev)
>  {
>  	unsigned int comp_param1;
> @@ -198,6 +207,8 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)
>  
>  	i2c_dw_set_mode(dev, DW_IC_MASTER);
>  
> +	dev->rx_buf_len = 0;
> +
>  	/* If the slave address is ten bit address, enable 10BITADDR */
>  	if (msgs[dev->msg_write_idx].flags & I2C_M_TEN) {
>  		ic_con = DW_IC_CON_10BITADDR_MASTER;
> @@ -459,6 +470,21 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
>  					     cmd | *buf++);
>  			}
>  			tx_limit--; buf_len--;
> +
> +			/* If precise fault reporting is required, check if the transfer
> +			 * is aborted after writing each byte.
> +			 * Otherwise, if it is aborted during filling FIFO, there is no way
> +			 * to know how many bytes was written to FIFO after transfer abort
> +			 * and thus are not counted in FLUSH_CNT.
> +			 * If we are checking abort flag after each byte, we can lose only 1 byte.
> +			 */
> +			if (dev->need_precise_report) {
> +				if (i2c_dw_check_abort_flag(dev)) {
> +					dev_warn_ratelimited(dev->dev, "Transfer aborted during FIFO writing. Report may be imprecise.");
> +					break;
> +				}
> +				dev->bytes_written++;
> +			}
>  		}

[Severity: Medium]
In i2c_dw_xfer_msg(), buf_len is decremented before checking
i2c_dw_check_abort_flag(dev), and the break only exits the inner while
loop:

	tx_limit--; buf_len--;
	if (dev->need_precise_report) {
		if (i2c_dw_check_abort_flag(dev)) {
			...
			break;
		}
		dev->bytes_written++;
	}

After the inner while loop, the outer loop over dev->msg_write_idx only
breaks when flags & I2C_M_RECV_LEN or buf_len > 0:

	if (flags & I2C_M_RECV_LEN) {
		dev->status |= STATUS_WRITE_IN_PROGRESS;
		intr_mask &= ~DW_IC_INTR_TX_EMPTY;
		break;
	} else if (buf_len > 0) {
		/* more bytes to be written */
		dev->status |= STATUS_WRITE_IN_PROGRESS;
		break;
	} else
		dev->status &= ~STATUS_WRITE_IN_PROGRESS;

If the abort flag is detected while writing the final byte of a message
(where buf_len was 1 and has just been decremented to 0), won't buf_len > 0
be false, causing the outer for loop to clear STATUS_WRITE_IN_PROGRESS,
advance dev->msg_write_idx, and continue writing subsequent messages into
DW_IC_DATA_CMD after the abort?

[ ... ]

> @@ -784,6 +819,8 @@ __i2c_dw_xfer_one_part(struct dw_i2c_dev *dev, struct i2c_msg *msgs, size_t num)
>  	dev->status = 0;
>  	dev->abort_source = 0;
>  	dev->rx_outstanding = 0;
> +	dev->bytes_written = 0;
> +	dev->tx_flushed = 0;
>  
>  	ret = i2c_dw_wait_bus_not_busy(dev);
>  	if (ret < 0)
> @@ -795,6 +832,9 @@ __i2c_dw_xfer_one_part(struct dw_i2c_dev *dev, struct i2c_msg *msgs, size_t num)
>  	/* Wait for tx to complete */
>  	ret = i2c_dw_wait_transfer(dev);
>  	if (ret) {
> +		if (dev->need_precise_report)
> +			regmap_read(dev->map, DW_IC_TXFLR, &dev->tx_flushed);
> +
>  		dev_err(dev->dev, "controller timed out\n");
>  		/* i2c_dw_init() implicitly disables the adapter */
>  		i2c_recover_bus(&dev->adapter);

[Severity: Medium]
As reported by Sashiko on v2 [1], __i2c_dw_xfer_one_part() only reads
DW_IC_TXFLR into dev->tx_flushed when i2c_dw_wait_transfer() times out, and
i2c_dw_fault_report() only reads FLUSH_CNT when dev->abort_source is
non-zero.

However, __i2c_dw_xfer_one_part() can also fail with -EIO without a timeout
or TX_ABRT when i2c_dw_process_transfer() detects a spurious STOP
mid-transfer (setting dev->msg_err = -EIO) or when dev->status remains
non-zero on early termination:

	if (dev->msg_err)
		return dev->msg_err;
	...
	if (dev->status)
		dev_err(dev->dev,
			"transfer terminated early - interrupt latency too high?\n");

	return -EIO;

In addition, in i2c_dw_process_transfer(), if DW_IC_INTR_TX_EMPTY and
DW_IC_INTR_STOP_DET are both set in stat, i2c_dw_xfer_msg() refills the TX
FIFO and increments dev->bytes_written before the spurious STOP check sets
dev->msg_err = -EIO. Because __i2c_dw_xfer_one_part() then disables or
reinitializes the controller before returning, the hardware FIFO is flushed
while both dev->abort_source and dev->tx_flushed remain 0.

When i2c_dw_fault_report() runs for these -EIO errors, won't unsent bytes
still sitting in the TX FIFO be counted as transmitted on the wire?

> @@ -886,13 +926,72 @@ i2c_dw_msg_is_valid(struct dw_i2c_dev *dev, const struct i2c_msg *msgs, size_t i
>  	return true;
>  }
>  
> +static int i2c_dw_fault_report(struct dw_i2c_dev *dev, struct i2c_transfer_report *report)
> +{
> +	unsigned int idx;
> +
> +	/*
> +	 * Get the number of bytes that were put into TX FIFO but were not sent on wire.
> +	 * If abort IRQ was received, this number is stored in abort_source register.
> +	 * Otherwise, if ic_dw_wait_transfer() returned timeout, use TXFLR register value
> +	 * read before the controller was reset.
> +	 */
> +	if (dev->abort_source)
> +		dev->tx_flushed = FIELD_GET(DW_IC_TX_ABRT_SOURCE_FLUSH_CNT_MASK, dev->abort_source);
> +
> +	if (dev->tx_flushed <= dev->bytes_written)
> +		dev->bytes_written -= dev->tx_flushed;
> +	else
> +		dev->bytes_written = 0;
> +
> +	/*
> +	 * The last byte that the transmission was interrupted on is not counted as flushed.
> +	 * We should not report it as transferred successfully, so decrement the counter.
> +	 */
> +	if (dev->bytes_written)
> +		dev->bytes_written--;
> +
> +	for (idx = 0; idx < dev->msg_write_idx; idx++) {
> +		if (dev->msgs[idx].len <= dev->bytes_written)
> +			dev->bytes_written -= dev->msgs[idx].len;
> +		else
> +			break;
> +	}
> +
> +	report->fault_msg_idx = idx;
> +	for (int i = dev->msg_read_idx; i <= idx && i < dev->msgs_num; i++) {

[Severity: Low]
As reported by Andy Shevchenko on v2 [2], there are a few style and type
issues remaining from the previous revision:

1. In i2c_dw_fault_report(), idx was changed to unsigned int, but the loop
   counter i in for (int i = dev->msg_read_idx; i <= idx && i <
   dev->msgs_num; i++) is still a signed int compared against unsigned int
   idx.
2. In i2c_dw_check_abort_flag(), return (!!(stat & DW_IC_INTR_TX_ABRT));
   still uses redundant outer parentheses and double negation in a function
   returning bool.
3. In i2c_dw_xfer_msg(), the multi-line comment starting with "/* If
   precise fault reporting is required..." still places text on the opening
   /* line instead of starting with a separate /* line.

Could these be cleaned up to match the feedback from v2?

> +		if (dev->msgs[i].flags & I2C_M_RD) {
> +			report->msgs_cplt = i;
> +			if (!(dev->rx_buf_len))
> +				report->bytes_cplt = 0;
> +			else
> +				report->bytes_cplt = dev->rx_buf - dev->msgs[i].buf;

[Severity: High]
Can i2c_dw_fault_report() use a stale dev->rx_buf_len and dangling
dev->rx_buf pointer from an earlier transfer if i2c_dw_wait_bus_not_busy()
fails?

When an earlier I2C_M_RD transfer is interrupted in i2c_dw_read() while
len > 0, i2c_dw_read() saves dev->rx_buf_len = len and dev->rx_buf = buf
(pointing into that transfer's message buffer) and returns.

On the next transfer (or a subsequent part in a multi-part transfer),
__i2c_dw_xfer_one_part() calls i2c_dw_wait_bus_not_busy(dev) before calling
i2c_dw_xfer_init(dev), which is where dev->rx_buf_len = 0 was placed:

	ret = i2c_dw_wait_bus_not_busy(dev);
	if (ret < 0)
		return ret;

	/* Start the transfers */
	i2c_dw_xfer_init(dev);

If i2c_dw_wait_bus_not_busy(dev) returns a negative error,
__i2c_dw_xfer_one_part() returns early without calling i2c_dw_xfer_init().
i2c_dw_xfer_common_v2() then calls i2c_dw_fault_report(dev, report) with
idx == 0 and dev->msg_read_idx == 0. If the new transfer's msgs[0] has
I2C_M_RD set, dev->rx_buf_len is still non-zero from the previous failed
transfer, so i2c_dw_fault_report() computes:

	report->bytes_cplt = dev->rx_buf - dev->msgs[i].buf;

subtracting the current message's buffer address from the previous
transfer's freed dev->rx_buf pointer. In i2cdev_ioctl_rdwr_v2(),
msgs[msgs_done].len is then set to report.bytes_cplt and passed to
copy_to_user(), which can read out of bounds past msgs[0].buf. Should
dev->rx_buf_len = 0 be initialized at the start of __i2c_dw_xfer_one_part()
alongside dev->msg_read_idx and dev->bytes_written?

> +			if ((i < idx) || (report->bytes_cplt < dev->bytes_written))
> +				dev_warn_ratelimited(dev->dev, "Read data lost due to FIFO flush");
> +			break;
> +		}
> +	}
> +
> +	if (report->msgs_cplt < 0) {
> +		report->msgs_cplt = idx;
> +		report->bytes_cplt = dev->bytes_written;
> +	}
> +
> +	return idx;
> +}

[ ... ]

> @@ -937,16 +1048,34 @@ 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: High]
What happens to report->msgs_cplt and report->bytes_cplt when an attempt
fails with -EAGAIN (for example on DW_IC_TX_ARB_LOST) and __i2c_transfer_v2()
retries the transfer?

In __i2c_transfer_v2(), i2c_transfer_report_init(report) (which sets
report->msgs_cplt = -EOPNOTSUPP) is called only once before the retry loop:

	if (report)
		i2c_transfer_report_init(report);
	...
	for (ret = 0, try = 0; try <= adap->retries; try++) {
		...
		if (report)
			ret = adap->algo->xfer_v2(adap, msgs, num, report);
		...
		if (ret != -EAGAIN)
			break;
	}

On the first failed attempt returning -EAGAIN, i2c_dw_fault_report() sets
report->msgs_cplt to a non-negative index and sets report->bytes_cplt. When
__i2c_transfer_v2() calls i2c_dw_xfer_v2() again for the next try and that
attempt also fails, if there is no I2C_M_RD message in
[dev->msg_read_idx, idx], i2c_dw_fault_report() checks:

	if (report->msgs_cplt < 0) {
		report->msgs_cplt = idx;
		report->bytes_cplt = dev->bytes_written;
	}

Because report->msgs_cplt is already >= 0 from the earlier try, neither
report->msgs_cplt nor report->bytes_cplt is updated. Furthermore, if
msgs_in_prev_parts > 0, i2c_dw_xfer_common_v2() executes
report->msgs_cplt += msgs_in_prev_parts on each failed retry, accumulating
msgs_in_prev_parts onto the stale report->msgs_cplt until it can exceed num.
Should report be reinitialized on entry to i2c_dw_xfer_common_v2() or
i2c_dw_fault_report()?

[1] https://lore.kernel.org/r/20260903-i2c-fault-reporting-v2-12-fedeb91792e6@mobileye.com
[2] https://lore.kernel.org/r/apkl_XHaZreKoipX@ashevche-desk.local

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

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

[Andy Shevchenko]

On Thu, Nov 05, 2026 at 07:15:28PM +0200, Dmitry Guzman wrote:
> In __i2c_transfer() and i2c_transfer(), add one more parameter - pointer
> to transfer report. In __i2c_transfer(), if the pointer is not NULL,
> use algo->xfer_v2() callback instead of algo->xfer() in order to get the
> detailed transfer report. If the adapter does not have xfer_v2(), return
> -EOPNOTSUPP.
> 
> If the adapter has any quirks that make impossible to transfer requested
> message or messages, put the index of the first such message to the
> transfer report structure.
> 
> If the report is not NULL, algo->xfer_v2() returned an error and there
> are any read messages, trace the read data using `trace_i2c_reply(). If
> the last read message is incomplete (transfer was interrupted in the
> middle of a read message), trace the read portion of data also.
> 
> Rename __i2c_transfer() to __i2c_transfer_v2() and i2c_transfer() to
> i2c_transfer_v2().
> 
> For backward compatibility, add __i2c_transfer() and i2c_transfer() that
> call __i2c_transfer_v2() and i2c_transfer_v2() respectively with null
> report pointer.
> 
> Remove comment "REVISIT the fault reporting model here is weak".

...

>  	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);

Besides missing {}. the iterator is local to the loop, hence

		for (int 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);
>  		trace_i2c_result(adap, num, ret);
>  	}

...

> +/**
> + * __i2c_transfer - unlocked flavor of i2c_transfer
> + * @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.

Return section must be last in the kernel-doc (it's documented) and actually be
a such. Always validate with kernel-doc script.

> + * Adapter lock must be held when calling this function. No debug logging
> + * takes place.

This is Context section in kernel-doc for.
And more effectively is to add a lockdep annotation.

> + */
> +int __i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
> +{
> +	return __i2c_transfer_v2(adap, msgs, num, NULL);
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH RFC v3 00/14] I2C - detailed transfer reporting in case of a fault

[Jonathan Cameron]

On Thu, 05 Nov 2026 19:15:21 +0200
Dmitry Guzman <Dmitry.Guzman@mobileye.com> wrote:

> The existing API has function i2c_transfer() that transfers one or more
> messages, and it only returns a single error code if the transfer was
> failed. It doesn't allow to know how many of the messages were
> transferred successfully, neither how many bytes were transferred in the
> message that caused the fault, and also it drops all data received from
> target device before the fault. There is a comment about this in
> drivers/i2c/i2c-core-base.c: "REVISIT the fault reporting model here is
> weak".
> 
> This patch series introduces new API function i2c_transfer_v2() that does
> the same as i2c_transfer() but also returns detailed transfer report,
> including number of messages and bytes transferred before the fault. This
> also allows caller to get the bytes read from the target before the fault
> occurred.

Hi Dmitry,

Just for background, could you provide some more information on why this
information is useful?  For kernel side drivers it is remarkably rare to
find a real use case of partial transfer information and the
current i2c_transfer() return value approach complicates a lot of
drivers for very limited benefit.  Sure it is a small complication
to check for a low message count, but you do get a lot cases of it!

That leads to a mass of AI reports of bugs.  It would have been
much better for kernel side if it just returned an error code
on any short transfer. 

Given this is effectively going in the opposite direction I'd like to
know more.

Jonathan


> 
> For user space clients, new ioctl `I2C_RDWR_V2` is introduced.
> 
> Patch for `i2c-tools` user space utility that introduces support of this
> feature:
> 
> https://lore.kernel.org/linux-i2c/20260623-i2c_detailed_report-v1-1-b67259a31a9d@mobileye.com/
> 
> Below there are several usage examples with modified i2c-tools.
> 
> - Unsupported message (max length supported by I2C controller is 2047):
> 
>     # i2ctransfer -y 0 -r w1@0x19 0 r8000
>     Error: Operation not supported at message 1
>     0 out of 2 messages sent
>     #
> 
> - First two messages transferred successfully, the third one is sent to
>   non-existent target address and thus is not ACKed. The data read in the
> second message is available for the caller:
> 
>     # i2ctransfer -y 0 -r w1@0x19 0 r1 w1@0x1a 0
>     Error: No such device or address at message 2
>     2 out of 3 messages sent
>     0x32
>     #
> 
> - Target ACKs first two bytes but sends NACK on the third byte:
> 
>     # i2ctransfer -r -y 0 w4@0x09 0x80 0x80 0x55 0x55
>     Error: Input/output error at message 0
>     0 out of 1 messages sent
>     Message 0: 2 out of 4 bytes sent
>     #
> 
> - Message reading is not finished due to timeout. The data read before
>   timeout is available:
> 
>     # i2ctransfer -y 0 -r r100@0x19
>     Error: Connection timed out at message 0
>     0 out of 1 messages sent
>     Message 0: 16 out of 100 bytes sent
>     0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00
>     #
> 
> - Driver does not support detailed fault reporting:
> 
>     # i2ctransfer -y 0 -r r1@0x19
>     Error: Sending messages failed: Operation not supported
>     #