Hi all,
This patch series is an updated version of this one:
https://lore.kernel.org/all/[email protected]/.
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
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 <[email protected]>
Signed-off-by: Michael Zaidman <[email protected]>
---
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
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 <[email protected]>
Tested-by: Guillaume Champagne <[email protected]>
---
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
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 <[email protected]>
Tested-by: Guillaume Champagne <[email protected]>
---
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
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 <[email protected]>
Tested-by: Guillaume Champagne <[email protected]>
---
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
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 <[email protected]>
Tested-by: Guillaume Champagne <[email protected]>
---
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
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 <[email protected]>
Tested-by: Guillaume Champagne <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Michael Zaidman <[email protected]>
---
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
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 <[email protected]>
> Tested-by: Guillaume Champagne <[email protected]>
> ---
> 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)
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 <[email protected]>
> > Tested-by: Guillaume Champagne <[email protected]>
> > ---
> > 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)
>
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
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
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
>
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
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