[PATCH v2 0/7] HID: ft260: fixes and performance improvements

[PATCH v2 0/7] HID: ft260: fixes and performance improvements

[Michael Zaidman]

Hi all,

This patch series is an updated version of this one:
https://lore.kernel.org/all/20220525074757.7519-1-michael.zaidman@gmail.com/.

Changes since v1:

- Addressed two security-related issues reported by Enrik Berkhan:
  1. Do not populate hidraw device.
  2. Avoid stale read buffer pointer.

- The patch set v1 was tested by Guillaume Champagne. Added appropriate
  Tested-by notice in the related commits.

Michael Zaidman (7):
  HID: ft260: ft260_xfer_status routine cleanup
  HID: ft260: improve i2c write performance
  HID: ft260: support i2c writes larger than HID report size
  HID: ft260: support i2c reads greater than HID report size
  HID: ft260: improve i2c large reads performance
  HID: ft260: do not populate /dev/hidraw device
  HID: ft260: skip unexpected HID input reports

 drivers/hid/hid-ft260.c | 257 ++++++++++++++++++++++------------------
 1 file changed, 141 insertions(+), 116 deletions(-)

-- 
2.34.1

[PATCH v2 1/7] HID: ft260: ft260_xfer_status routine cleanup

[Michael Zaidman]

After clarifying with FTDI's support, it turned out that the error
condition (bit 1) in byte 1 of the i2c status HID report is a status
bit reflecting all error conditions. When bits 2, 3, or 4 are raised
to 1, bit 1 is set to 1 also. Since the ft260_xfer_status routine tests
the error condition bit and exits in the case of an error, the program
flow never reaches the conditional expressions for 2, 3, and 4 bits when
any of them indicates an error state. Though these expressions are never
evaluated to true, they are checked several times per IO, increasing the
ft260_xfer_status polling cycle duration.

The patch removes the conditional expressions for 2, 3, and 4 bits in
byte 1 of the i2c status HID report.

Signed-off-by: Michael Zaidman <michael.zaidman@gmail.com>
Tested-by: Guillaume Champagne <champagne.guillaume.c@gmail.com>
---
 drivers/hid/hid-ft260.c | 30 ++++++++++--------------------
 1 file changed, 10 insertions(+), 20 deletions(-)

diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
index 79505c64dbfe..a35201d68b15 100644
--- a/drivers/hid/hid-ft260.c
+++ b/drivers/hid/hid-ft260.c
@@ -313,27 +313,17 @@ static int ft260_xfer_status(struct ft260_device *dev)
 	if (report.bus_status & FT260_I2C_STATUS_CTRL_BUSY)
 		return -EAGAIN;
 
-	if (report.bus_status & FT260_I2C_STATUS_BUS_BUSY)
-		return -EBUSY;
-
-	if (report.bus_status & FT260_I2C_STATUS_ERROR)
+	/*
+	 * The error condition (bit 1) is a status bit reflecting any
+	 * error conditions. When any of the bits 2, 3, or 4 are raised
+	 * to 1, bit 1 is also set to 1.
+	 */
+	if (report.bus_status & FT260_I2C_STATUS_ERROR) {
+		hid_err(hdev, "i2c bus error: %#02x\n", report.bus_status);
 		return -EIO;
+	}
 
-	ret = -EIO;
-
-	if (report.bus_status & FT260_I2C_STATUS_ADDR_NO_ACK)
-		ft260_dbg("unacknowledged address\n");
-
-	if (report.bus_status & FT260_I2C_STATUS_DATA_NO_ACK)
-		ft260_dbg("unacknowledged data\n");
-
-	if (report.bus_status & FT260_I2C_STATUS_ARBITR_LOST)
-		ft260_dbg("arbitration loss\n");
-
-	if (report.bus_status & FT260_I2C_STATUS_CTRL_IDLE)
-		ret = 0;
-
-	return ret;
+	return 0;
 }
 
 static int ft260_hid_output_report(struct hid_device *hdev, u8 *data,
@@ -376,7 +366,7 @@ static int ft260_hid_output_report_check_status(struct ft260_device *dev,
 			break;
 	} while (--try);
 
-	if (ret == 0 || ret == -EBUSY)
+	if (ret == 0)
 		return 0;
 
 	ft260_i2c_reset(hdev);
-- 
2.34.1

[PATCH v2 2/7] HID: ft260: improve i2c write performance

[Michael Zaidman]

The patch improves i2c writing performance by about 30 percent by revising
the sleep time in the ft260_hid_output_report_check_status() in the
following ways:

1. Reduce the sleep time and start to poll earlier:

  Before:
    $ sudo ./i2cperf -f 2 -o 2 -s 32 -r 0-0xff 13 0x51 -S

      Fill block with increment via i2ctransfer by chunks
      -------------------------------------------------------------------
      data rate(bps)  efficiency(%)  data size(B)  total IOs   IO size(B)
      -------------------------------------------------------------------
      40510           80             256           8           32

  After:
    $ sudo ./i2cperf -f 2 -o 2 -s 32 -r 0-0xff 13 0x51 -S

      Fill block with increment via i2ctransfer by chunks
      -------------------------------------------------------------------
      data rate(bps)  efficiency(%)  data size(B)  total IOs   IO size(B)
      -------------------------------------------------------------------
      52584           80             256           8           32

2. Do not sleep when the calculated sleep time is below 2 ms:

  Before:
    $ sudo ./i2cperf -f 2 -o 2 -s 16 -r 0-0xff 13 0x51 -S

      Fill block with increment via i2ctransfer by chunks
      -------------------------------------------------------------------
      data rate(bps)  efficiency(%)  data size(B)  total IOs   IO size(B)
      -------------------------------------------------------------------
      26707           73             256           16          16

  After:
    $ sudo ./i2cperf -f 2 -o 2 -s 16 -r 0-0xff 13 0x51 -S

      Fill block with increment via i2ctransfer by chunks
      -------------------------------------------------------------------
      data rate(bps)  efficiency(%)  data size(B)  total IOs   IO size(B)
      -------------------------------------------------------------------
      37034           73             256           16          16

Link to the i2cperf - https://github.com/MichaelZaidman/i2cperf

Signed-off-by: Michael Zaidman <michael.zaidman@gmail.com>
Tested-by: Guillaume Champagne <champagne.guillaume.c@gmail.com>
---
 drivers/hid/hid-ft260.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
index a35201d68b15..44106cadd746 100644
--- a/drivers/hid/hid-ft260.c
+++ b/drivers/hid/hid-ft260.c
@@ -345,7 +345,7 @@ static int ft260_hid_output_report(struct hid_device *hdev, u8 *data,
 static int ft260_hid_output_report_check_status(struct ft260_device *dev,
 						u8 *data, int len)
 {
-	int ret, usec, try = 3;
+	int ret, usec, try = 100;
 	struct hid_device *hdev = dev->hdev;
 
 	ret = ft260_hid_output_report(hdev, data, len);
@@ -356,10 +356,14 @@ static int ft260_hid_output_report_check_status(struct ft260_device *dev,
 		return ret;
 	}
 
-	/* transfer time = 1 / clock(KHz) * 10 bits * bytes */
-	usec = 10000 / dev->clock * len;
-	usleep_range(usec, usec + 100);
-	ft260_dbg("wait %d usec, len %d\n", usec, len);
+	/* transfer time = 1 / clock(KHz) * 9 bits * bytes */
+	usec = len * 9000 / dev->clock;
+	if (usec > 2000) {
+		usec -= 1500;
+		usleep_range(usec, usec + 100);
+		ft260_dbg("wait %d usec, len %d\n", usec, len);
+	}
+
 	do {
 		ret = ft260_xfer_status(dev);
 		if (ret != -EAGAIN)
-- 
2.34.1

[PATCH v2 3/7] HID: ft260: support i2c writes larger than HID report size

[Michael Zaidman]

To support longer than one HID report size write, the driver splits a
single i2c message data payload into multiple i2c messages of HID report
size. However, it does not replicate the offset bytes within the EEPROM
chip in every consequent HID report because it is not and should not be
aware of the EEPROM type. It breaks the i2c write message integrity and
causes the EEPROM device not to acknowledge the second HID report keeping
the i2c bus busy until the ft260 controller reports failure.

This patch preserves the i2c write message integrity by manipulating the
i2c flag bits across multiple HID reports to be seen by the EEPROM device
as a single i2c write transfer.

Before:

$ sudo ./i2cperf -f 2 -o 2 -s 64 -r 0-0xff 13 0x51 -S
Error: Sending messages failed: Input/output error

[  +3.667741] ft260_i2c_write: rep 0xde addr 0x51 off 0 len 60 d[0] 0x0
[  +0.007330] ft260_hid_output_report_check_status: wait 6400 usec, len 64
[  +0.000203] ft260_xfer_status: bus_status 0x40, clock 100
[  +0.000001] ft260_i2c_write: rep 0xd1 addr 0x51 off 60 len 6 d[0] 0x0
[  +0.002337] ft260_hid_output_report_check_status: wait 1000 usec, len 10
[  +0.000157] ft260_xfer_status: bus_status 0x2e, clock 100
[  +0.000241] ft260_i2c_reset: done
[  +0.000003] ft260_i2c_write: failed to start transfer, ret -5

After:

$ sudo ./i2cperf -f 2 -o 2 -s 128 -r 0-0xff 13 0x51 -S

  Fill block with increment via i2ctransfer by chunks
  -------------------------------------------------------------------
  data rate(bps)  efficiency(%)  data size(B)  total IOs   IO size(B)
  -------------------------------------------------------------------
  58986           86             256           2           128

Signed-off-by: Michael Zaidman <michael.zaidman@gmail.com>
Tested-by: Guillaume Champagne <champagne.guillaume.c@gmail.com>
---
 drivers/hid/hid-ft260.c | 45 ++++++++++++++++++++++++-----------------
 1 file changed, 27 insertions(+), 18 deletions(-)

diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
index 44106cadd746..bfda5b191a3a 100644
--- a/drivers/hid/hid-ft260.c
+++ b/drivers/hid/hid-ft260.c
@@ -378,41 +378,50 @@ static int ft260_hid_output_report_check_status(struct ft260_device *dev,
 }
 
 static int ft260_i2c_write(struct ft260_device *dev, u8 addr, u8 *data,
-			   int data_len, u8 flag)
+			   int len, u8 flag)
 {
-	int len, ret, idx = 0;
+	int ret, wr_len, idx = 0;
+	bool first = true;
 	struct hid_device *hdev = dev->hdev;
 	struct ft260_i2c_write_request_report *rep =
 		(struct ft260_i2c_write_request_report *)dev->write_buf;
 
 	do {
-		if (data_len <= FT260_WR_DATA_MAX)
-			len = data_len;
-		else
-			len = FT260_WR_DATA_MAX;
+		rep->flag = 0;
+		if (first) {
+			rep->flag = FT260_FLAG_START;
+			first = false;
+		}
+
+		if (len <= FT260_WR_DATA_MAX) {
+			wr_len = len;
+			if (flag == FT260_FLAG_START_STOP)
+				rep->flag |= FT260_FLAG_STOP;
+		} else {
+			wr_len = FT260_WR_DATA_MAX;
+		}
 
-		rep->report = FT260_I2C_DATA_REPORT_ID(len);
+		rep->report = FT260_I2C_DATA_REPORT_ID(wr_len);
 		rep->address = addr;
-		rep->length = len;
-		rep->flag = flag;
+		rep->length = wr_len;
 
-		memcpy(rep->data, &data[idx], len);
+		memcpy(rep->data, &data[idx], wr_len);
 
-		ft260_dbg("rep %#02x addr %#02x off %d len %d d[0] %#02x\n",
-			  rep->report, addr, idx, len, data[0]);
+		ft260_dbg("rep %#02x addr %#02x off %d len %d wlen %d flag %#x d[0] %#02x\n",
+			  rep->report, addr, idx, len, wr_len,
+			  rep->flag, data[0]);
 
 		ret = ft260_hid_output_report_check_status(dev, (u8 *)rep,
-							   len + 4);
+							   wr_len + 4);
 		if (ret < 0) {
-			hid_err(hdev, "%s: failed to start transfer, ret %d\n",
-				__func__, ret);
+			hid_err(hdev, "%s: failed with %d\n", __func__, ret);
 			return ret;
 		}
 
-		data_len -= len;
-		idx += len;
+		len -= wr_len;
+		idx += wr_len;
 
-	} while (data_len > 0);
+	} while (len > 0);
 
 	return 0;
 }
-- 
2.34.1

[PATCH v2 4/7] HID: ft260: support i2c reads greater than HID report size

[Michael Zaidman]

A random i2c read operation in EEPROM devices is implemented as a dummy
write operation, followed by a current address read operation. The dummy
write operation is used to load the target byte or word address (a.k.a
offset) into the offset counter, from which the subsequent read operation
then reads.

To support longer than one HID report size random read, the ft260 driver
issues multiple pairs of i2c write offset + read data transactions of HID
report size so that the EEPROM device sees many i2c random read requests
from different offsets.

Two issues with the current implementation:
- This approach suffers from extra overhead caused by writing offset
  requests.
- Necessity to handle offset per HID report in big-endian representation
  as EEPROM devices expect. The current implementation does not do it and
  correctly handles the reads up to 60 bytes only.

This patch addresses both issues by implementing more efficient approach.
It issues a single i2c read request of up to the EEPROM page size and then
waits for the data to arrive in multiple HID reports. For example, to read
the 256 bytes from a 24LC512 chip, which has 128 bytes page size, the old
method performs six ft260_i2c_write_read transactions while the new - two
only.

Before:

$ sudo ./i2cperf -d 2 -o 2 -s 128 -r 0-0xff 13 0x51 -S

  Read block via i2ctransfer by chunks
  -------------------------------------------------------------------
  data rate(bps)  efficiency(%)  data size(B)  total IOs   IO size(B)
  -------------------------------------------------------------------
  40803           85             256           2           128

Kernel log:

[  +2.376308] ft260_i2c_write_read: read_off 0x0 left_len 128 len 60
[  +0.000002] ft260_i2c_write: rep 0xd0 addr 0x51 off 0 len 2 wlen 2 flag 0x2 d[0] 0x0
[  +0.000707] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000173] ft260_xfer_status: bus_status 0x40, clock 100
[  +0.000001] ft260_i2c_read: rep 0xc2 addr 0x51 len 60
[  +0.008660] ft260_raw_event: i2c resp: rep 0xde len 60
[  +0.000156] ft260_xfer_status: bus_status 0x20, clock 100
[  +0.000001] ft260_i2c_write_read: read_off 0x3c left_len 68 len 60
[  +0.000001] ft260_i2c_write: rep 0xd0 addr 0x51 off 0 len 2 wlen 2 flag 0x2 d[0] 0x3c
[  +0.001034] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000191] ft260_xfer_status: bus_status 0x40, clock 100
[  +0.000001] ft260_i2c_read: rep 0xc2 addr 0x51 len 60
[  +0.008614] ft260_raw_event: i2c resp: rep 0xde len 60
[  +0.000203] ft260_xfer_status: bus_status 0x20, clock 100
[  +0.000001] ft260_i2c_write_read: read_off 0x78 left_len 8 len 8
[  +0.000001] ft260_i2c_write: rep 0xd0 addr 0x51 off 0 len 2 wlen 2 flag 0x2 d[0] 0x78
[  +0.000987] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000192] ft260_xfer_status: bus_status 0x40, clock 100
[  +0.000001] ft260_i2c_read: rep 0xc2 addr 0x51 len 8
[  +0.002614] ft260_raw_event: i2c resp: rep 0xd1 len 8
[  +0.000200] ft260_xfer_status: bus_status 0x20, clock 100
[  +0.012511] ft260_i2c_write_read: read_off 0x8000 left_len 128 len 60
[  +0.000002] ft260_i2c_write: rep 0xd0 addr 0x51 off 0 len 2 wlen 2 flag 0x2 d[0] 0x0
[  +0.001456] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000207] ft260_xfer_status: bus_status 0x40, clock 100
[  +0.000002] ft260_i2c_read: rep 0xc2 addr 0x51 len 60
[  +0.008625] ft260_raw_event: i2c resp: rep 0xde len 60
[  +0.000203] ft260_xfer_status: bus_status 0x20, clock 100
[  +0.000002] ft260_i2c_write_read: read_off 0x803c left_len 68 len 60
[  +0.000001] ft260_i2c_write: rep 0xd0 addr 0x51 off 0 len 2 wlen 2 flag 0x2 d[0] 0x3c
[  +0.000991] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000196] ft260_xfer_status: bus_status 0x40, clock 100
[  +0.000002] ft260_i2c_read: rep 0xc2 addr 0x51 len 60
[  +0.008621] ft260_raw_event: i2c resp: rep 0xde len 60
[  +0.000202] ft260_xfer_status: bus_status 0x20, clock 100
[  +0.000001] ft260_i2c_write_read: read_off 0x8078 left_len 8 len 8
[  +0.000002] ft260_i2c_write: rep 0xd0 addr 0x51 off 0 len 2 wlen 2 flag 0x2 d[0] 0x78
[  +0.000979] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000151] ft260_xfer_status: bus_status 0x40, clock 100
[  +0.000001] ft260_i2c_read: rep 0xc2 addr 0x51 len 8
[  +0.002644] ft260_raw_event: i2c resp: rep 0xd1 len 8
[  +0.000141] ft260_xfer_status: bus_status 0x20, clock 100

After:

$ sudo ./i2cperf -d 2 -o 2 -s 128 -r 0-0xff 13 0x51 -S

  Read block via i2ctransfer by chunks
  -------------------------------------------------------------------
  data rate(bps)  efficiency(%)  data size(B)  total IOs   IO size(B)
  -------------------------------------------------------------------
  43990           85             256           2           128

Kernel log:

[  +1.464346] ft260_i2c_write_read: off 0x0 rlen 128 wlen 2
[  +0.000002] ft260_i2c_write: rep 0xd0 addr 0x51 off 0 len 2 wlen 2 flag 0x2 d[0] 0x0
[  +0.001653] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000188] ft260_xfer_status: bus_status 0x40, clock 100
[  +0.000002] ft260_i2c_read: rep 0xc2 addr 0x51 len 128 rlen 60 flag 0x3
[  +0.008609] ft260_raw_event: i2c resp: rep 0xde len 60
[  +0.000157] ft260_xfer_status: bus_status 0x40, clock 100
[  +0.000002] ft260_i2c_read: rep 0xc2 addr 0x51 len 68 rlen 60 flag 0x0
[  +0.008840] ft260_raw_event: i2c resp: rep 0xde len 60
[  +0.000203] ft260_xfer_status: bus_status 0x40, clock 100
[  +0.000002] ft260_i2c_read: rep 0xc2 addr 0x51 len 8 rlen 8 flag 0x4
[  +0.002794] ft260_raw_event: i2c resp: rep 0xd1 len 8
[  +0.000201] ft260_xfer_status: bus_status 0x20, clock 100
[  +0.015171] ft260_i2c_write_read: off 0x80 rlen 128 wlen 2
[  +0.000002] ft260_i2c_write: rep 0xd0 addr 0x51 off 0 len 2 wlen 2 flag 0x2 d[0] 0x0
[  +0.000764] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000231] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000148] ft260_xfer_status: bus_status 0x40, clock 100
[  +0.000002] ft260_i2c_read: rep 0xc2 addr 0x51 len 128 rlen 60 flag 0x3
[  +0.008488] ft260_raw_event: i2c resp: rep 0xde len 60
[  +0.000205] ft260_xfer_status: bus_status 0x40, clock 100
[  +0.000002] ft260_i2c_read: rep 0xc2 addr 0x51 len 68 rlen 60 flag 0x0
[  +0.008795] ft260_raw_event: i2c resp: rep 0xde len 60
[  +0.000176] ft260_xfer_status: bus_status 0x40, clock 100
[  +0.000001] ft260_i2c_read: rep 0xc2 addr 0x51 len 8 rlen 8 flag 0x4
[  +0.002819] ft260_raw_event: i2c resp: rep 0xd1 len 8
[  +0.000167] ft260_xfer_status: bus_status 0x20, clock 100

Signed-off-by: Michael Zaidman <michael.zaidman@gmail.com>
Tested-by: Guillaume Champagne <champagne.guillaume.c@gmail.com>
---
 drivers/hid/hid-ft260.c | 133 ++++++++++++++++++++++------------------
 1 file changed, 72 insertions(+), 61 deletions(-)

diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
index bfda5b191a3a..cb8f1782d1f0 100644
--- a/drivers/hid/hid-ft260.c
+++ b/drivers/hid/hid-ft260.c
@@ -460,49 +460,68 @@ static int ft260_smbus_write(struct ft260_device *dev, u8 addr, u8 cmd,
 static int ft260_i2c_read(struct ft260_device *dev, u8 addr, u8 *data,
 			  u16 len, u8 flag)
 {
+	u16 rd_len;
+	int timeout, ret;
 	struct ft260_i2c_read_request_report rep;
 	struct hid_device *hdev = dev->hdev;
-	int timeout;
-	int ret;
+	bool first = true;
 
-	if (len > FT260_RD_DATA_MAX) {
-		hid_err(hdev, "%s: unsupported rd len: %d\n", __func__, len);
-		return -EINVAL;
-	}
+	do {
+		if (first) {
+			if (flag & FT260_FLAG_START_REPEATED)
+				flag = FT260_FLAG_START_REPEATED;
+			else
+				flag = FT260_FLAG_START;
+			first = false;
+		} else {
+			flag = 0;
+		}
 
-	dev->read_idx = 0;
-	dev->read_buf = data;
-	dev->read_len = len;
+		if (len <= FT260_RD_DATA_MAX) {
+			rd_len = len;
+			flag |= FT260_FLAG_STOP;
+		} else {
+			rd_len = FT260_RD_DATA_MAX;
+		}
 
-	rep.report = FT260_I2C_READ_REQ;
-	rep.length = cpu_to_le16(len);
-	rep.address = addr;
-	rep.flag = flag;
+		dev->read_idx = 0;
+		dev->read_buf = data;
+		dev->read_len = rd_len;
 
-	ft260_dbg("rep %#02x addr %#02x len %d\n", rep.report, rep.address,
-		  rep.length);
+		rep.report = FT260_I2C_READ_REQ;
+		rep.length = cpu_to_le16(rd_len);
+		rep.address = addr;
+		rep.flag = flag;
 
-	reinit_completion(&dev->wait);
+		ft260_dbg("rep %#02x addr %#02x len %d rlen %d flag %#x\n",
+			  rep.report, rep.address, len, rd_len, flag);
 
-	ret = ft260_hid_output_report(hdev, (u8 *)&rep, sizeof(rep));
-	if (ret < 0) {
-		hid_err(hdev, "%s: failed to start transaction, ret %d\n",
-			__func__, ret);
-		return ret;
-	}
+		reinit_completion(&dev->wait);
 
-	timeout = msecs_to_jiffies(5000);
-	if (!wait_for_completion_timeout(&dev->wait, timeout)) {
-		ft260_i2c_reset(hdev);
-		return -ETIMEDOUT;
-	}
+		ret = ft260_hid_output_report(hdev, (u8 *)&rep, sizeof(rep));
+		if (ret < 0) {
+			hid_err(hdev, "%s: failed with %d\n", __func__, ret);
+			return ret;
+		}
 
-	ret = ft260_xfer_status(dev);
-	if (ret == 0)
-		return 0;
+		timeout = msecs_to_jiffies(5000);
+		if (!wait_for_completion_timeout(&dev->wait, timeout)) {
+			ft260_i2c_reset(hdev);
+			return -ETIMEDOUT;
+		}
 
-	ft260_i2c_reset(hdev);
-	return -EIO;
+		ret = ft260_xfer_status(dev);
+		if (ret < 0) {
+			ft260_i2c_reset(hdev);
+			return -EIO;
+		}
+
+		len -= rd_len;
+		data += rd_len;
+
+	} while (len > 0);
+
+	return 0;
 }
 
 /*
@@ -513,45 +532,37 @@ static int ft260_i2c_read(struct ft260_device *dev, u8 addr, u8 *data,
  */
 static int ft260_i2c_write_read(struct ft260_device *dev, struct i2c_msg *msgs)
 {
-	int len, ret;
-	u16 left_len = msgs[1].len;
-	u8 *read_buf = msgs[1].buf;
+	int ret;
+	int wr_len = msgs[0].len;
+	int rd_len = msgs[1].len;
+	struct hid_device *hdev = dev->hdev;
 	u8 addr = msgs[0].addr;
 	u16 read_off = 0;
-	struct hid_device *hdev = dev->hdev;
 
-	if (msgs[0].len > 2) {
-		hid_err(hdev, "%s: unsupported wr len: %d\n", __func__,
-			msgs[0].len);
+	if (wr_len > 2) {
+		hid_err(hdev, "%s: invalid wr_len: %d\n", __func__, wr_len);
 		return -EOPNOTSUPP;
 	}
 
-	memcpy(&read_off, msgs[0].buf, msgs[0].len);
-
-	do {
-		if (left_len <= FT260_RD_DATA_MAX)
-			len = left_len;
+	if (ft260_debug) {
+		if (wr_len == 2)
+			read_off = be16_to_cpu(*(u16 *)msgs[0].buf);
 		else
-			len = FT260_RD_DATA_MAX;
-
-		ft260_dbg("read_off %#x left_len %d len %d\n", read_off,
-			  left_len, len);
-
-		ret = ft260_i2c_write(dev, addr, (u8 *)&read_off, msgs[0].len,
-				      FT260_FLAG_START);
-		if (ret < 0)
-			return ret;
+			read_off = *msgs[0].buf;
 
-		ret = ft260_i2c_read(dev, addr, read_buf, len,
-				     FT260_FLAG_START_STOP);
-		if (ret < 0)
-			return ret;
+		pr_info("%s: off %#x rlen %d wlen %d\n", __func__,
+			read_off, rd_len, wr_len);
+	}
 
-		left_len -= len;
-		read_buf += len;
-		read_off += len;
+	ret = ft260_i2c_write(dev, addr, msgs[0].buf, wr_len,
+			      FT260_FLAG_START);
+	if (ret < 0)
+		return ret;
 
-	} while (left_len > 0);
+	ret = ft260_i2c_read(dev, addr, msgs[1].buf, rd_len,
+			     FT260_FLAG_START_STOP_REPEATED);
+	if (ret < 0)
+		return ret;
 
 	return 0;
 }
-- 
2.34.1

[PATCH v2 5/7] HID: ft260: improve i2c large reads performance

[Michael Zaidman]

The patch increases the read buffer size to 128 bytes. It reduces the
number of ft260_i2c_read calls by three and improves the large data
chunks' read performance by about 10%.

Before:

$ sudo ./i2cperf -d 2 -o 2 -s 128 -r 0-0xff 13 0x51 -S

  Read block via i2ctransfer by chunks
  -------------------------------------------------------------------
  data rate(bps)  efficiency(%)  data size(B)  total IOs   IO size(B)
  -------------------------------------------------------------------
  43990           85             256           2           128

Kernel log:

[  +1.464346] ft260_i2c_write_read: off 0x0 rlen 128 wlen 2
[  +0.000002] ft260_i2c_write: rep 0xd0 addr 0x51 off 0 len 2 wlen 2 flag 0x2 d[0] 0x0
[  +0.001653] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000188] ft260_xfer_status: bus_status 0x40, clock 100
[  +0.000002] ft260_i2c_read: rep 0xc2 addr 0x51 len 128 rlen 60 flag 0x3
[  +0.008609] ft260_raw_event: i2c resp: rep 0xde len 60
[  +0.000157] ft260_xfer_status: bus_status 0x40, clock 100
[  +0.000002] ft260_i2c_read: rep 0xc2 addr 0x51 len 68 rlen 60 flag 0x0
[  +0.008840] ft260_raw_event: i2c resp: rep 0xde len 60
[  +0.000203] ft260_xfer_status: bus_status 0x40, clock 100
[  +0.000002] ft260_i2c_read: rep 0xc2 addr 0x51 len 8 rlen 8 flag 0x4
[  +0.002794] ft260_raw_event: i2c resp: rep 0xd1 len 8
[  +0.000201] ft260_xfer_status: bus_status 0x20, clock 100
[  +0.015171] ft260_i2c_write_read: off 0x80 rlen 128 wlen 2
[  +0.000002] ft260_i2c_write: rep 0xd0 addr 0x51 off 0 len 2 wlen 2 flag 0x2 d[0] 0x0
[  +0.000764] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000231] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000148] ft260_xfer_status: bus_status 0x40, clock 100
[  +0.000002] ft260_i2c_read: rep 0xc2 addr 0x51 len 128 rlen 60 flag 0x3
[  +0.008488] ft260_raw_event: i2c resp: rep 0xde len 60
[  +0.000205] ft260_xfer_status: bus_status 0x40, clock 100
[  +0.000002] ft260_i2c_read: rep 0xc2 addr 0x51 len 68 rlen 60 flag 0x0
[  +0.008795] ft260_raw_event: i2c resp: rep 0xde len 60
[  +0.000176] ft260_xfer_status: bus_status 0x40, clock 100
[  +0.000001] ft260_i2c_read: rep 0xc2 addr 0x51 len 8 rlen 8 flag 0x4
[  +0.002819] ft260_raw_event: i2c resp: rep 0xd1 len 8
[  +0.000167] ft260_xfer_status: bus_status 0x20, clock 100

After:

$ sudo ./i2cperf -d 2 -o 2 -s 128 -r 0-0xff 13 0x51 -S

  Read block via i2ctransfer by chunks
  -------------------------------------------------------------------
  data rate(bps)  efficiency(%)  data size(B)  total IOs   IO size(B)
  -------------------------------------------------------------------
  49316           85             256           2           128

Kernel log:

[  +1.447360] ft260_i2c_write_read: off 0x0 rlen 128 wlen 2
[  +0.000002] ft260_i2c_write: rep 0xd0 addr 0x51 off 0 len 2 wlen 2 flag 0x2 d[0] 0x0
[  +0.001633] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000190] ft260_xfer_status: bus_status 0x40, clock 100
[  +0.000001] ft260_i2c_read: rep 0xc2 addr 0x51 len 128 rlen 128 flag 0x7
[  +0.008617] ft260_raw_event: i2c resp: rep 0xde len 60
[  +0.008033] ft260_raw_event: i2c resp: rep 0xde len 60
[  +0.000954] ft260_raw_event: i2c resp: rep 0xd1 len 8
[  +0.000208] ft260_xfer_status: bus_status 0x20, clock 100
[  +0.015853] ft260_i2c_write_read: off 0x80 rlen 128 wlen 2
[  +0.000002] ft260_i2c_write: rep 0xd0 addr 0x51 off 0 len 2 wlen 2 flag 0x2 d[0] 0x0
[  +0.001182] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000180] ft260_xfer_status: bus_status 0x40, clock 100
[  +0.000001] ft260_i2c_read: rep 0xc2 addr 0x51 len 128 rlen 128 flag 0x7
[  +0.008575] ft260_raw_event: i2c resp: rep 0xde len 60
[  +0.008014] ft260_raw_event: i2c resp: rep 0xde len 60
[  +0.001001] ft260_raw_event: i2c resp: rep 0xd1 len 8
[  +0.000223] ft260_xfer_status: bus_status 0x20, clock 100

Signed-off-by: Michael Zaidman <michael.zaidman@gmail.com>
Tested-by: Guillaume Champagne <champagne.guillaume.c@gmail.com>
---
 drivers/hid/hid-ft260.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
index cb8f1782d1f0..c7c3a9d395a2 100644
--- a/drivers/hid/hid-ft260.c
+++ b/drivers/hid/hid-ft260.c
@@ -30,12 +30,8 @@ MODULE_PARM_DESC(debug, "Toggle FT260 debugging messages");
 
 #define FT260_REPORT_MAX_LENGTH (64)
 #define FT260_I2C_DATA_REPORT_ID(len) (FT260_I2C_REPORT_MIN + (len - 1) / 4)
-/*
- * The input report format assigns 62 bytes for the data payload, but ft260
- * returns 60 and 2 in two separate transactions. To minimize transfer time
- * in reading chunks mode, set the maximum read payload length to 60 bytes.
- */
-#define FT260_RD_DATA_MAX (60)
+
+#define FT260_RD_DATA_MAX (128)
 #define FT260_WR_DATA_MAX (60)
 
 /*
-- 
2.34.1

[PATCH v2 6/7] HID: ft260: do not populate /dev/hidraw device

[Michael Zaidman]

Do not populate the /dev/hidraw on ft260 interfaces when the hid-ft260
driver is loaded.

$ sudo insmod hid-ft260.ko
$ ls /dev/hidraw*
/dev/hidraw0

$ sudo rmmod hid-ft260.ko
$ ls /dev/hidraw*
/dev/hidraw0  /dev/hidraw1  /dev/hidraw2

Reported-by: Enrik Berkhan <Enrik.Berkhan@inka.de>
Signed-off-by: Michael Zaidman <michael.zaidman@gmail.com>
---
 drivers/hid/hid-ft260.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
index c7c3a9d395a2..bb9f4e07c21d 100644
--- a/drivers/hid/hid-ft260.c
+++ b/drivers/hid/hid-ft260.c
@@ -938,7 +938,7 @@ static int ft260_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		return ret;
 	}
 
-	ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
+	ret = hid_hw_start(hdev, 0);
 	if (ret) {
 		hid_err(hdev, "failed to start HID HW\n");
 		return ret;
@@ -965,6 +965,10 @@ static int ft260_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	if (ret <= 0)
 		goto err_hid_close;
 
+	hid_info(hdev, "USB HID v%x.%02x Device [%s] on %s\n",
+		hdev->version >> 8, hdev->version & 0xff, hdev->name,
+		hdev->phys);
+
 	hid_set_drvdata(hdev, dev);
 	dev->hdev = hdev;
 	dev->adap.owner = THIS_MODULE;
@@ -973,8 +977,7 @@ static int ft260_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	dev->adap.quirks = &ft260_i2c_quirks;
 	dev->adap.dev.parent = &hdev->dev;
 	snprintf(dev->adap.name, sizeof(dev->adap.name),
-		 "FT260 usb-i2c bridge on hidraw%d",
-		 ((struct hidraw *)hdev->hidraw)->minor);
+		 "FT260 usb-i2c bridge");
 
 	mutex_init(&dev->lock);
 	init_completion(&dev->wait);
-- 
2.34.1

[PATCH v2 7/7] HID: ft260: skip unexpected HID input reports

[Michael Zaidman]

The FT260 is not supposed to generate unexpected HID reports. However,
in theory, the unsolicited HID Input reports can be issued by a specially
crafted malicious USB device masquerading as FT260 when the attacker has
physical access to the USB port. In this case, the read_buf pointer points
to the final data portion of the previous I2C Read transfer, and the memcpy
invoked in the ft260_raw_event() will try copying the content of the
unexpected report into the wrong location.

This commit sets the Read buffer pointer to NULL on the I2C Read
transaction completion and checks it in the ft260_raw_event() to detect
and skip the unsolicited Input report.

Reported-by: Enrik Berkhan <Enrik.Berkhan@inka.de>
Signed-off-by: Michael Zaidman <michael.zaidman@gmail.com>
---
 drivers/hid/hid-ft260.c | 36 ++++++++++++++++++++++++------------
 1 file changed, 24 insertions(+), 12 deletions(-)

diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
index bb9f4e07c21d..a162e9d14a08 100644
--- a/drivers/hid/hid-ft260.c
+++ b/drivers/hid/hid-ft260.c
@@ -457,7 +457,7 @@ static int ft260_i2c_read(struct ft260_device *dev, u8 addr, u8 *data,
 			  u16 len, u8 flag)
 {
 	u16 rd_len;
-	int timeout, ret;
+	int timeout, ret = 0;
 	struct ft260_i2c_read_request_report rep;
 	struct hid_device *hdev = dev->hdev;
 	bool first = true;
@@ -480,10 +480,6 @@ static int ft260_i2c_read(struct ft260_device *dev, u8 addr, u8 *data,
 			rd_len = FT260_RD_DATA_MAX;
 		}
 
-		dev->read_idx = 0;
-		dev->read_buf = data;
-		dev->read_len = rd_len;
-
 		rep.report = FT260_I2C_READ_REQ;
 		rep.length = cpu_to_le16(rd_len);
 		rep.address = addr;
@@ -494,22 +490,30 @@ static int ft260_i2c_read(struct ft260_device *dev, u8 addr, u8 *data,
 
 		reinit_completion(&dev->wait);
 
+		dev->read_idx = 0;
+		dev->read_buf = data;
+		dev->read_len = rd_len;
+
 		ret = ft260_hid_output_report(hdev, (u8 *)&rep, sizeof(rep));
 		if (ret < 0) {
 			hid_err(hdev, "%s: failed with %d\n", __func__, ret);
-			return ret;
+			goto ft260_i2c_read_exit;
 		}
 
 		timeout = msecs_to_jiffies(5000);
 		if (!wait_for_completion_timeout(&dev->wait, timeout)) {
+			ret = -ETIMEDOUT;
 			ft260_i2c_reset(hdev);
-			return -ETIMEDOUT;
+			goto ft260_i2c_read_exit;
 		}
 
+		dev->read_buf = NULL;
+
 		ret = ft260_xfer_status(dev);
 		if (ret < 0) {
+			ret = -EIO;
 			ft260_i2c_reset(hdev);
-			return -EIO;
+			goto ft260_i2c_read_exit;
 		}
 
 		len -= rd_len;
@@ -517,7 +521,9 @@ static int ft260_i2c_read(struct ft260_device *dev, u8 addr, u8 *data,
 
 	} while (len > 0);
 
-	return 0;
+ft260_i2c_read_exit:
+	dev->read_buf = NULL;
+	return ret;
 }
 
 /*
@@ -1035,6 +1041,13 @@ static int ft260_raw_event(struct hid_device *hdev, struct hid_report *report,
 		ft260_dbg("i2c resp: rep %#02x len %d\n", xfer->report,
 			  xfer->length);
 
+		if ((dev->read_buf == NULL) ||
+		    (xfer->length > dev->read_len - dev->read_idx)) {
+			hid_err(hdev, "unexpected report %#02x, length %d\n",
+				xfer->report, xfer->length);
+			return -1;
+		}
+
 		memcpy(&dev->read_buf[dev->read_idx], &xfer->data,
 		       xfer->length);
 		dev->read_idx += xfer->length;
@@ -1043,10 +1056,9 @@ static int ft260_raw_event(struct hid_device *hdev, struct hid_report *report,
 			complete(&dev->wait);
 
 	} else {
-		hid_err(hdev, "unknown report: %#02x\n", xfer->report);
-		return 0;
+		hid_err(hdev, "unhandled report %#02x\n", xfer->report);
 	}
-	return 1;
+	return 0;
 }
 
 static struct hid_driver ft260_driver = {
-- 
2.34.1

RE: [PATCH v2 2/7] HID: ft260: improve i2c write performance

[David Laight]

From: Michael Zaidman
> Sent: 28 September 2022 15:49
> 
> The patch improves i2c writing performance by about 30 percent by revising
> the sleep time in the ft260_hid_output_report_check_status() in the
> following ways:

Spinning in kernel for several milliseconds isn't friendly at all.

	David

> 
> 1. Reduce the sleep time and start to poll earlier:
> 
>   Before:
>     $ sudo ./i2cperf -f 2 -o 2 -s 32 -r 0-0xff 13 0x51 -S
> 
>       Fill block with increment via i2ctransfer by chunks
>       -------------------------------------------------------------------
>       data rate(bps)  efficiency(%)  data size(B)  total IOs   IO size(B)
>       -------------------------------------------------------------------
>       40510           80             256           8           32
> 
>   After:
>     $ sudo ./i2cperf -f 2 -o 2 -s 32 -r 0-0xff 13 0x51 -S
> 
>       Fill block with increment via i2ctransfer by chunks
>       -------------------------------------------------------------------
>       data rate(bps)  efficiency(%)  data size(B)  total IOs   IO size(B)
>       -------------------------------------------------------------------
>       52584           80             256           8           32
> 
> 2. Do not sleep when the calculated sleep time is below 2 ms:
> 
>   Before:
>     $ sudo ./i2cperf -f 2 -o 2 -s 16 -r 0-0xff 13 0x51 -S
> 
>       Fill block with increment via i2ctransfer by chunks
>       -------------------------------------------------------------------
>       data rate(bps)  efficiency(%)  data size(B)  total IOs   IO size(B)
>       -------------------------------------------------------------------
>       26707           73             256           16          16
> 
>   After:
>     $ sudo ./i2cperf -f 2 -o 2 -s 16 -r 0-0xff 13 0x51 -S
> 
>       Fill block with increment via i2ctransfer by chunks
>       -------------------------------------------------------------------
>       data rate(bps)  efficiency(%)  data size(B)  total IOs   IO size(B)
>       -------------------------------------------------------------------
>       37034           73             256           16          16
> 
> Link to the i2cperf - https://github.com/MichaelZaidman/i2cperf
> 
> Signed-off-by: Michael Zaidman <michael.zaidman@gmail.com>
> Tested-by: Guillaume Champagne <champagne.guillaume.c@gmail.com>
> ---
>  drivers/hid/hid-ft260.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
> index a35201d68b15..44106cadd746 100644
> --- a/drivers/hid/hid-ft260.c
> +++ b/drivers/hid/hid-ft260.c
> @@ -345,7 +345,7 @@ static int ft260_hid_output_report(struct hid_device *hdev, u8 *data,
>  static int ft260_hid_output_report_check_status(struct ft260_device *dev,
>  						u8 *data, int len)
>  {
> -	int ret, usec, try = 3;
> +	int ret, usec, try = 100;
>  	struct hid_device *hdev = dev->hdev;
> 
>  	ret = ft260_hid_output_report(hdev, data, len);
> @@ -356,10 +356,14 @@ static int ft260_hid_output_report_check_status(struct ft260_device *dev,
>  		return ret;
>  	}
> 
> -	/* transfer time = 1 / clock(KHz) * 10 bits * bytes */
> -	usec = 10000 / dev->clock * len;
> -	usleep_range(usec, usec + 100);
> -	ft260_dbg("wait %d usec, len %d\n", usec, len);
> +	/* transfer time = 1 / clock(KHz) * 9 bits * bytes */
> +	usec = len * 9000 / dev->clock;
> +	if (usec > 2000) {
> +		usec -= 1500;
> +		usleep_range(usec, usec + 100);
> +		ft260_dbg("wait %d usec, len %d\n", usec, len);
> +	}
> +
>  	do {
>  		ret = ft260_xfer_status(dev);
>  		if (ret != -EAGAIN)
> --
> 2.34.1

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH v2 2/7] HID: ft260: improve i2c write performance

[Michael Zaidman]

On Wed, Sep 28, 2022 at 03:50:36PM +0000, David Laight wrote:
> From: Michael Zaidman
> > Sent: 28 September 2022 15:49
> > 
> > The patch improves i2c writing performance by about 30 percent by revising
> > the sleep time in the ft260_hid_output_report_check_status() in the
> > following ways:
> 
> Spinning in kernel for several milliseconds isn't friendly at all.
> 
> 	David
> 
We do not sleep if the estimated I2C transfer time is below 2 ms since the
first xfer check frequently takes about 1.5 ms on the real HW (i7-4790K @ 4.0GHz).
That means the condition is usually satisfied already on the first 1-3 checks,
as can be seen in example #1.

On the other hand, sending a large amount of data at a low I2C clock rate
saturates the internal FT260 buffer and causes hiccups in status readiness,
as shown in exemple #2. Aligning the status checking wait time to the worst
case significantly reduces the performance.  

Exemple #1

$ sudo ./i2cperf -f 2 -o 2 -s 16 -r 0-0xff 13 0x51 -S

  Fill block with increment via i2ctransfer by chunks
  -------------------------------------------------------------------
  data rate(bps)  efficiency(%)  data size(B)  total IOs   IO size(B)
  -------------------------------------------------------------------
  49823           73             256           16          16

[Sep28 21:40] ft260_i2c_write: rep 0xd4 addr 0x51 off 0 len 18 wlen 18 flag 0x6 d[0] 0x0
[  +0.005031] ft260_xfer_status: bus_status 0x20, clock 100
[  +0.016531] ft260_i2c_write: rep 0xd4 addr 0x51 off 0 len 18 wlen 18 flag 0x6 d[0] 0x0
[  +0.001710] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000227] ft260_xfer_status: bus_status 0x40, clock 100
[  +0.011805] ft260_i2c_write: rep 0xd4 addr 0x51 off 0 len 18 wlen 18 flag 0x6 d[0] 0x0
[  +0.000747] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000189] ft260_xfer_status: bus_status 0x40, clock 100
[  +0.014932] ft260_i2c_write: rep 0xd4 addr 0x51 off 0 len 18 wlen 18 flag 0x6 d[0] 0x0
[  +0.000806] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000223] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000178] ft260_xfer_status: bus_status 0x40, clock 100
[  +0.010921] ft260_i2c_write: rep 0xd4 addr 0x51 off 0 len 18 wlen 18 flag 0x6 d[0] 0x0
[  +0.000704] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000181] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000228] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000182] ft260_xfer_status: bus_status 0x40, clock 100
[  +0.014243] ft260_i2c_write: rep 0xd4 addr 0x51 off 0 len 18 wlen 18 flag 0x6 d[0] 0x0
[  +0.001220] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000143] ft260_xfer_status: bus_status 0x40, clock 100
[  +0.011332] ft260_i2c_write: rep 0xd4 addr 0x51 off 0 len 18 wlen 18 flag 0x6 d[0] 0x0
[  +0.001533] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000183] ft260_xfer_status: bus_status 0x40, clock 100
[  +0.011865] ft260_i2c_write: rep 0xd4 addr 0x51 off 0 len 18 wlen 18 flag 0x6 d[0] 0x0
[  +0.000934] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000197] ft260_xfer_status: bus_status 0x40, clock 100
[  +0.014102] ft260_i2c_write: rep 0xd4 addr 0x51 off 0 len 18 wlen 18 flag 0x6 d[0] 0x0
[  +0.001722] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000181] ft260_xfer_status: bus_status 0x40, clock 100
[  +0.014000] ft260_i2c_write: rep 0xd4 addr 0x51 off 0 len 18 wlen 18 flag 0x6 d[0] 0x0
[  +0.000845] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000172] ft260_xfer_status: bus_status 0x40, clock 100
[  +0.013294] ft260_i2c_write: rep 0xd4 addr 0x51 off 0 len 18 wlen 18 flag 0x6 d[0] 0x0
[  +0.001499] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000190] ft260_xfer_status: bus_status 0x40, clock 100
[  +0.011524] ft260_i2c_write: rep 0xd4 addr 0x51 off 0 len 18 wlen 18 flag 0x6 d[0] 0x0
[  +0.001238] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000291] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000182] ft260_xfer_status: bus_status 0x40, clock 100
[  +0.013632] ft260_i2c_write: rep 0xd4 addr 0x51 off 0 len 18 wlen 18 flag 0x6 d[0] 0x0
[  +0.000967] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000184] ft260_xfer_status: bus_status 0x40, clock 100
[  +0.013505] ft260_i2c_write: rep 0xd4 addr 0x51 off 0 len 18 wlen 18 flag 0x6 d[0] 0x0
[  +0.001302] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000140] ft260_xfer_status: bus_status 0x40, clock 100
[  +0.012673] ft260_i2c_write: rep 0xd4 addr 0x51 off 0 len 18 wlen 18 flag 0x6 d[0] 0x0
[  +0.001147] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000140] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000217] ft260_xfer_status: bus_status 0x40, clock 100
[  +0.012426] ft260_i2c_write: rep 0xd4 addr 0x51 off 0 len 18 wlen 18 flag 0x6 d[0] 0x0
[  +0.001172] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000192] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000192] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000184] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000211] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000193] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000194] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000197] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000175] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000226] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000132] ft260_xfer_status: bus_status 0x20, clock 100


Exemple #2

$ sudo ./i2cperf -f 2 -o 2 -s 128 -r 0-0x1ff 13 0x51 -S

  Fill block with increment via i2ctransfer by chunks
  -------------------------------------------------------------------
  data rate(bps)  efficiency(%)  data size(B)  total IOs   IO size(B)
  -------------------------------------------------------------------
  65553           86             512           4           128

[Sep28 22:04] ft260_i2c_write: rep 0xde addr 0x51 off 0 len 130 wlen 60 flag 0x2 d[0] 0x0
[  +0.005707] ft260_hid_output_report_check_status: wait 4260 usec, len 64
[  +0.000846] ft260_xfer_status: bus_status 0x20, clock 100
[  +0.000002] ft260_i2c_write: rep 0xde addr 0x51 off 60 len 70 wlen 60 flag 0x0 d[0] 0x0
[  +0.005155] ft260_hid_output_report_check_status: wait 4260 usec, len 64
[  +0.000245] ft260_xfer_status: bus_status 0x40, clock 100
[  +0.000002] ft260_i2c_write: rep 0xd2 addr 0x51 off 120 len 10 wlen 10 flag 0x4 d[0] 0x0
[  +0.001593] ft260_xfer_status: bus_status 0x40, clock 100
[  +0.016725] ft260_i2c_write: rep 0xde addr 0x51 off 0 len 130 wlen 60 flag 0x2 d[0] 0x0
[  +0.005433] ft260_hid_output_report_check_status: wait 4260 usec, len 64
[  +0.000240] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000193] ft260_xfer_status: bus_status 0x40, clock 100
[  +0.000001] ft260_i2c_write: rep 0xde addr 0x51 off 60 len 70 wlen 60 flag 0x0 d[0] 0x0
[  +0.005547] ft260_hid_output_report_check_status: wait 4260 usec, len 64
[  +0.000185] ft260_xfer_status: bus_status 0x40, clock 100
[  +0.000002] ft260_i2c_write: rep 0xd2 addr 0x51 off 120 len 10 wlen 10 flag 0x4 d[0] 0x0
[  +0.001575] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000235] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000186] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000195] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000160] ft260_xfer_status: bus_status 0x40, clock 100
[  +0.014875] ft260_i2c_write: rep 0xde addr 0x51 off 0 len 130 wlen 60 flag 0x2 d[0] 0x1
[  +0.005596] ft260_hid_output_report_check_status: wait 4260 usec, len 64
[  +0.000230] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000188] ft260_xfer_status: bus_status 0x40, clock 100
[  +0.000002] ft260_i2c_write: rep 0xde addr 0x51 off 60 len 70 wlen 60 flag 0x0 d[0] 0x1
[  +0.005568] ft260_hid_output_report_check_status: wait 4260 usec, len 64
[  +0.000190] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000175] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000172] ft260_xfer_status: bus_status 0x40, clock 100
[  +0.000002] ft260_i2c_write: rep 0xd2 addr 0x51 off 120 len 10 wlen 10 flag 0x4 d[0] 0x1
[  +0.001287] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000226] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000176] ft260_xfer_status: bus_status 0x41, clock 100]
[  +0.000234] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000201] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000173] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000223] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000180] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000218] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000195] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000178] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000129] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000128] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000130] ft260_xfer_status: bus_status 0x40, clock 100
[  +0.014159] ft260_i2c_write: rep 0xde addr 0x51 off 0 len 130 wlen 60 flag 0x2 d[0] 0x1
[  +0.005546] ft260_hid_output_report_check_status: wait 4260 usec, len 64
[  +0.000196] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000183] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000251] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000141] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000191] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000181] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000225] ft260_xfer_status: bus_status 0x40, clock 100
[  +0.000001] ft260_i2c_write: rep 0xde addr 0x51 off 60 len 70 wlen 60 flag 0x0 d[0] 0x1
[  +0.005693] ft260_hid_output_report_check_status: wait 4260 usec, len 64
[  +0.000180] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000227] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000151] ft260_xfer_status: bus_status 0x40, clock 100
[  +0.000002] ft260_i2c_write: rep 0xd2 addr 0x51 off 120 len 10 wlen 10 flag 0x4 d[0] 0x1
[  +0.001321] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000185] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000179] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000234] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000162] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000237] ft260_xfer_status: bus_status 0x40, clock 100



> > 
> > 1. Reduce the sleep time and start to poll earlier:
> > 
> >   Before:
> >     $ sudo ./i2cperf -f 2 -o 2 -s 32 -r 0-0xff 13 0x51 -S
> > 
> >       Fill block with increment via i2ctransfer by chunks
> >       -------------------------------------------------------------------
> >       data rate(bps)  efficiency(%)  data size(B)  total IOs   IO size(B)
> >       -------------------------------------------------------------------
> >       40510           80             256           8           32
> > 
> >   After:
> >     $ sudo ./i2cperf -f 2 -o 2 -s 32 -r 0-0xff 13 0x51 -S
> > 
> >       Fill block with increment via i2ctransfer by chunks
> >       -------------------------------------------------------------------
> >       data rate(bps)  efficiency(%)  data size(B)  total IOs   IO size(B)
> >       -------------------------------------------------------------------
> >       52584           80             256           8           32
> > 
> > 2. Do not sleep when the calculated sleep time is below 2 ms:
> > 
> >   Before:
> >     $ sudo ./i2cperf -f 2 -o 2 -s 16 -r 0-0xff 13 0x51 -S
> > 
> >       Fill block with increment via i2ctransfer by chunks
> >       -------------------------------------------------------------------
> >       data rate(bps)  efficiency(%)  data size(B)  total IOs   IO size(B)
> >       -------------------------------------------------------------------
> >       26707           73             256           16          16
> > 
> >   After:
> >     $ sudo ./i2cperf -f 2 -o 2 -s 16 -r 0-0xff 13 0x51 -S
> > 
> >       Fill block with increment via i2ctransfer by chunks
> >       -------------------------------------------------------------------
> >       data rate(bps)  efficiency(%)  data size(B)  total IOs   IO size(B)
> >       -------------------------------------------------------------------
> >       37034           73             256           16          16
> > 
> > Link to the i2cperf - https://github.com/MichaelZaidman/i2cperf
> > 
> > Signed-off-by: Michael Zaidman <michael.zaidman@gmail.com>
> > Tested-by: Guillaume Champagne <champagne.guillaume.c@gmail.com>
> > ---
> >  drivers/hid/hid-ft260.c | 14 +++++++++-----
> >  1 file changed, 9 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
> > index a35201d68b15..44106cadd746 100644
> > --- a/drivers/hid/hid-ft260.c
> > +++ b/drivers/hid/hid-ft260.c
> > @@ -345,7 +345,7 @@ static int ft260_hid_output_report(struct hid_device *hdev, u8 *data,
> >  static int ft260_hid_output_report_check_status(struct ft260_device *dev,
> >  						u8 *data, int len)
> >  {
> > -	int ret, usec, try = 3;
> > +	int ret, usec, try = 100;
> >  	struct hid_device *hdev = dev->hdev;
> > 
> >  	ret = ft260_hid_output_report(hdev, data, len);
> > @@ -356,10 +356,14 @@ static int ft260_hid_output_report_check_status(struct ft260_device *dev,
> >  		return ret;
> >  	}
> > 
> > -	/* transfer time = 1 / clock(KHz) * 10 bits * bytes */
> > -	usec = 10000 / dev->clock * len;
> > -	usleep_range(usec, usec + 100);
> > -	ft260_dbg("wait %d usec, len %d\n", usec, len);
> > +	/* transfer time = 1 / clock(KHz) * 9 bits * bytes */
> > +	usec = len * 9000 / dev->clock;
> > +	if (usec > 2000) {
> > +		usec -= 1500;
> > +		usleep_range(usec, usec + 100);
> > +		ft260_dbg("wait %d usec, len %d\n", usec, len);
> > +	}
> > +
> >  	do {
> >  		ret = ft260_xfer_status(dev);
> >  		if (ret != -EAGAIN)
> > --
> > 2.34.1
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
> 

Re: [PATCH v2 4/7] HID: ft260: support i2c reads greater than HID report size

[Enrik Berkhan]

Hi Michael,

On Wed, 2022-09-28 at 17:48 +0300, Michael Zaidman wrote:
> diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
> index bfda5b191a3a..cb8f1782d1f0 100644
> --- a/drivers/hid/hid-ft260.c
> +++ b/drivers/hid/hid-ft260.c
> @@ -460,49 +460,68 @@ static int ft260_smbus_write(struct ft260_device *dev, u8 addr, u8 cmd,
>  static int ft260_i2c_read(struct ft260_device *dev, u8 addr, u8 *data,
>  			  u16 len, u8 flag)
>  {
> +	u16 rd_len;
> +	int timeout, ret;
>  	struct ft260_i2c_read_request_report rep;
>  	struct hid_device *hdev = dev->hdev;
> -	int timeout;
> -	int ret;
> +	bool first = true;
>  
> -	if (len > FT260_RD_DATA_MAX) {
> -		hid_err(hdev, "%s: unsupported rd len: %d\n", __func__, len);
> -		return -EINVAL;
> -	}
> +	do {
> +		if (first) {
> +			if (flag & FT260_FLAG_START_REPEATED)

I think the test should be

    if ((flag & FT260_FLAG_START_REPEATED) == FT260_FLAG_START_REPEATED)

Otherwise, flag will never be FT260_FLAG_START ( = 2), as
FT260_FLAG_START_REPEATED (= 3) has tow bits set. It looks as if bit 0
actually means "repeated", bit 1 is "start" and bit 2 is "stop".

Cheers,
Enrik

Re: [PATCH v2 5/7] HID: ft260: improve i2c large reads performance

[Enrik Berkhan]

Hi Michael,

On Wed, 2022-09-28 at 17:48 +0300, Michael Zaidman wrote:
> After:
> 
> $ sudo ./i2cperf -d 2 -o 2 -s 128 -r 0-0xff 13 0x51 -S
> 
>   Read block via i2ctransfer by chunks
>   -------------------------------------------------------------------
>   data rate(bps)  efficiency(%)  data size(B)  total IOs   IO size(B)
>   -------------------------------------------------------------------
>   49316           85             256           2           128
> 
> Kernel log:
> 
> [  +1.447360] ft260_i2c_write_read: off 0x0 rlen 128 wlen 2
> [  +0.000002] ft260_i2c_write: rep 0xd0 addr 0x51 off 0 len 2 wlen 2 flag 0x2 d[0] 0x0
> [  +0.001633] ft260_xfer_status: bus_status 0x41, clock 100
> [  +0.000190] ft260_xfer_status: bus_status 0x40, clock 100
> [  +0.000001] ft260_i2c_read: rep 0xc2 addr 0x51 len 128 rlen 128 flag 0x7
> [  +0.008617] ft260_raw_event: i2c resp: rep 0xde len 60
> [  +0.008033] ft260_raw_event: i2c resp: rep 0xde len 60
> [  +0.000954] ft260_raw_event: i2c resp: rep 0xd1 len 8

As the ft260 can pack up to 60 bytes into one report, would it make
sense to use a multiple-of-60 size (120 or 180)? Might reduce overhead
by another tiny bit ...

Cheers,
Enrik

Re: [PATCH v2 5/7] HID: ft260: improve i2c large reads performance

[Michael Zaidman]

On Tue, Oct 04, 2022 at 08:15:56PM +0200, Enrik Berkhan wrote:
> Hi Michael,
> 
> On Wed, 2022-09-28 at 17:48 +0300, Michael Zaidman wrote:
> > After:
> > 
> > $ sudo ./i2cperf -d 2 -o 2 -s 128 -r 0-0xff 13 0x51 -S
> > 
> >   Read block via i2ctransfer by chunks
> >   -------------------------------------------------------------------
> >   data rate(bps)  efficiency(%)  data size(B)  total IOs   IO size(B)
> >   -------------------------------------------------------------------
> >   49316           85             256           2           128
> > 
> > Kernel log:
> > 
> > [  +1.447360] ft260_i2c_write_read: off 0x0 rlen 128 wlen 2
> > [  +0.000002] ft260_i2c_write: rep 0xd0 addr 0x51 off 0 len 2 wlen 2 flag 0x2 d[0] 0x0
> > [  +0.001633] ft260_xfer_status: bus_status 0x41, clock 100
> > [  +0.000190] ft260_xfer_status: bus_status 0x40, clock 100
> > [  +0.000001] ft260_i2c_read: rep 0xc2 addr 0x51 len 128 rlen 128 flag 0x7
> > [  +0.008617] ft260_raw_event: i2c resp: rep 0xde len 60
> > [  +0.008033] ft260_raw_event: i2c resp: rep 0xde len 60
> > [  +0.000954] ft260_raw_event: i2c resp: rep 0xd1 len 8
> 
> As the ft260 can pack up to 60 bytes into one report, would it make
> sense to use a multiple-of-60 size (120 or 180)? Might reduce overhead
> by another tiny bit ...
> 
> Cheers,
> Enrik
> 
The size of the Read IO to perform is given to the driver by the upper
layer. So it's up to him how to align the IO request size.

When we read from the EEPROM, we want to issue the read requests with
EEPROM page size granularity. The I2C EEPROMs page sizes are usually a
power of 2 aligned.

Please see the examples of reading 4K bytes from the 24C512 EEPROM
first by the read requests of EEPROM page size granularity of 128 bytes
and the second time of the 120 bytes (a multiple of 60 bytes granularity).
In the power of 2 aligned cases, we issued lesser Read IOs (I2C combined
transactions - write address read data) than when we did it with the 60
bytes alignment. Hence the performance gain.

$ sudo ./i2cperf -d 2 -o 2 -s 128 -r 0-0xfff 13 0x51 -S
  Read block via i2ctransfer by chunks
  -------------------------------------------------------------------
  data rate(bps)  efficiency(%)  data size(B)  total IOs   IO size(B)
  -------------------------------------------------------------------
  49581           85             4096          32          128

$ sudo ./i2cperf -d 2 -o 2 -s 120 -r 0-0xfff 13 0x51 -S
  Read block via i2ctransfer by chunks
  -------------------------------------------------------------------
  data rate(bps)  efficiency(%)  data size(B)  total IOs   IO size(B)
  -------------------------------------------------------------------
  48816           85             4096          35          120

Thanks,
Michael

> 

Re: [PATCH v2 4/7] HID: ft260: support i2c reads greater than HID report size

[Michael Zaidman]

On Tue, Oct 04, 2022 at 08:11:37PM +0200, Enrik Berkhan wrote:
> Hi Michael,
> 
> On Wed, 2022-09-28 at 17:48 +0300, Michael Zaidman wrote:
> > diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
> > index bfda5b191a3a..cb8f1782d1f0 100644
> > --- a/drivers/hid/hid-ft260.c
> > +++ b/drivers/hid/hid-ft260.c
> > @@ -460,49 +460,68 @@ static int ft260_smbus_write(struct ft260_device *dev, u8 addr, u8 cmd,
> >  static int ft260_i2c_read(struct ft260_device *dev, u8 addr, u8 *data,
> >  			  u16 len, u8 flag)
> >  {
> > +	u16 rd_len;
> > +	int timeout, ret;
> >  	struct ft260_i2c_read_request_report rep;
> >  	struct hid_device *hdev = dev->hdev;
> > -	int timeout;
> > -	int ret;
> > +	bool first = true;
> >  
> > -	if (len > FT260_RD_DATA_MAX) {
> > -		hid_err(hdev, "%s: unsupported rd len: %d\n", __func__, len);
> > -		return -EINVAL;
> > -	}
> > +	do {
> > +		if (first) {
> > +			if (flag & FT260_FLAG_START_REPEATED)
> 
> I think the test should be
> 
>     if ((flag & FT260_FLAG_START_REPEATED) == FT260_FLAG_START_REPEATED)
> 
> Otherwise, flag will never be FT260_FLAG_START ( = 2), as
> FT260_FLAG_START_REPEATED (= 3) has tow bits set. It looks as if bit 0
> actually means "repeated", bit 1 is "start" and bit 2 is "stop".
> 
> Cheers,
> Enrik
>

Thanks for reviewing the code!

Though the FT260 works fine with the FT260_FLAG_START_REPEATED flag at the
beginning of the I2C IO, I will fix it as you suggested.

Thanks,
Michael

Re: [PATCH v2 5/7] HID: ft260: improve i2c large reads performance

[Enrik Berkhan]

Hi Michael,

On Wed, 2022-10-05 at 17:34 +0300, Michael Zaidman wrote:
> On Tue, Oct 04, 2022 at 08:15:56PM +0200, Enrik Berkhan wrote:
> > As the ft260 can pack up to 60 bytes into one report, would it make
> > sense to use a multiple-of-60 size (120 or 180)? Might reduce overhead
> > by another tiny bit ...
> > 
> > Cheers,
> > Enrik
> > 
> The size of the Read IO to perform is given to the driver by the upper
> layer. So it's up to him how to align the IO request size.
> 
> When we read from the EEPROM, we want to issue the read requests with
> EEPROM page size granularity. The I2C EEPROMs page sizes are usually a
> power of 2 aligned.

Understood! I only thought about the HID report sizes. With EEPROMs
etc. in mind, it makes perfect sense to prefer power of 2 sizes.

Thanks for also providing the test results.

Cheers,
Enrik