This patch series is an updated version of this one:
https://lore.kernel.org/all/[email protected]/
Changes since v2:
- Remove SMBus Quick command support
- Missed NACK from big i2c read
- Wake up device from power saving mode
- Fix a NULL pointer dereference in ft260_i2c_write
- Missed NACK from busy device
Changes since v1:
- Do not populate hidraw device
- Avoid stale read buffer pointer
Michael Zaidman (12):
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
HID: ft260: remove SMBus Quick command support
HID: ft260: missed NACK from big i2c read
HID: ft260: wake up device from power saving mode
HID: ft260: fix a NULL pointer dereference in ft260_i2c_write
HID: ft260: missed NACK from busy device
drivers/hid/hid-ft260.c | 313 ++++++++++++++++++++++++----------------
1 file changed, 185 insertions(+), 128 deletions(-)
--
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)
-------------------------------------------------------------------
71260 86 256 2 128
Signed-off-by: Michael Zaidman <[email protected]>
Tested-by: Guillaume Champagne <[email protected]>
---
drivers/hid/hid-ft260.c | 41 +++++++++++++++++++++++------------------
1 file changed, 23 insertions(+), 18 deletions(-)
diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
index 44106cadd746..cec83f69ebdc 100644
--- a/drivers/hid/hid-ft260.c
+++ b/drivers/hid/hid-ft260.c
@@ -378,41 +378,46 @@ 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;
struct hid_device *hdev = dev->hdev;
struct ft260_i2c_write_request_report *rep =
(struct ft260_i2c_write_request_report *)dev->write_buf;
+ rep->flag = FT260_FLAG_START;
+
do {
- if (data_len <= FT260_WR_DATA_MAX)
- len = data_len;
- else
- len = FT260_WR_DATA_MAX;
+ 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;
+ rep->flag = 0;
- } while (data_len > 0);
+ } while (len > 0);
return 0;
}
--
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 8d6d2a19b9ed..8b6ebc5228eb 100644
--- a/drivers/hid/hid-ft260.c
+++ b/drivers/hid/hid-ft260.c
@@ -464,7 +464,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;
@@ -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;
@@ -518,7 +522,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;
}
/*
@@ -1036,6 +1042,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;
@@ -1044,10 +1057,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
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
When writing into a slow device like an EEPROM chip, the
controller may exit the busy state before the device releases
the bus. In this case, the ft260_xfer_status returns success
before the data transfer completion.
The patch fixes it by returning from the ft260_xfer_status()
with the "-EAGAIN" on both controller and bus busy status when
appropriate.
It does not apply to the i2c combined transactions when after
the write IO, the controller keeps the bus busy until the read
IO and then between reading IOs to ensure an atomic operation.
Co-developed-by: Germain Hebert <[email protected]>
Signed-off-by: Germain Hebert <[email protected]>
Signed-off-by: Michael Zaidman <[email protected]>
---
drivers/hid/hid-ft260.c | 31 ++++++++++++++++++++++++-------
1 file changed, 24 insertions(+), 7 deletions(-)
diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
index b3f715f6ea86..da8ea0d16059 100644
--- a/drivers/hid/hid-ft260.c
+++ b/drivers/hid/hid-ft260.c
@@ -303,7 +303,7 @@ static int ft260_i2c_reset(struct hid_device *hdev)
return ret;
}
-static int ft260_xfer_status(struct ft260_device *dev)
+static int ft260_xfer_status(struct ft260_device *dev, u8 bus_busy)
{
struct hid_device *hdev = dev->hdev;
struct ft260_get_i2c_status_report report;
@@ -320,7 +320,7 @@ static int ft260_xfer_status(struct ft260_device *dev)
ft260_dbg("bus_status %#02x, clock %u\n", report.bus_status,
dev->clock);
- if (report.bus_status & FT260_I2C_STATUS_CTRL_BUSY)
+ if (report.bus_status & (FT260_I2C_STATUS_CTRL_BUSY | bus_busy))
return -EAGAIN;
/*
@@ -355,8 +355,11 @@ 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)
{
+ u8 bus_busy;
int ret, usec, try = 100;
struct hid_device *hdev = dev->hdev;
+ struct ft260_i2c_write_request_report *rep =
+ (struct ft260_i2c_write_request_report *)data;
ret = ft260_hid_output_report(hdev, data, len);
if (ret < 0) {
@@ -374,8 +377,18 @@ static int ft260_hid_output_report_check_status(struct ft260_device *dev,
ft260_dbg("wait %d usec, len %d\n", usec, len);
}
+ /*
+ * Do not check the busy bit for combined transactions
+ * since the controller keeps the bus busy between writing
+ * and reading IOs to ensure an atomic operation.
+ */
+ if (rep->flag == FT260_FLAG_START)
+ bus_busy = 0;
+ else
+ bus_busy = FT260_I2C_STATUS_BUS_BUSY;
+
do {
- ret = ft260_xfer_status(dev);
+ ret = ft260_xfer_status(dev, bus_busy);
if (ret != -EAGAIN)
break;
} while (--try);
@@ -399,7 +412,7 @@ static int ft260_i2c_write(struct ft260_device *dev, u8 addr, u8 *data,
return -EINVAL;
if (time_is_before_jiffies(dev->need_wakeup_at)) {
- (void)ft260_xfer_status(dev);
+ (void)ft260_xfer_status(dev, 0);
ft260_dbg("device wakeup");
}
@@ -453,7 +466,7 @@ static int ft260_smbus_write(struct ft260_device *dev, u8 addr, u8 cmd,
return -EINVAL;
if (time_is_before_jiffies(dev->need_wakeup_at)) {
- (void)ft260_xfer_status(dev);
+ (void)ft260_xfer_status(dev, 0);
ft260_dbg("device wakeup");
}
@@ -484,6 +497,7 @@ static int ft260_i2c_read(struct ft260_device *dev, u8 addr, u8 *data,
int timeout, ret = 0;
struct ft260_i2c_read_request_report rep;
struct hid_device *hdev = dev->hdev;
+ u8 bus_busy = 0;
if ((flag & FT260_FLAG_START_REPEATED) == FT260_FLAG_START_REPEATED)
flag = FT260_FLAG_START_REPEATED;
@@ -527,7 +541,10 @@ static int ft260_i2c_read(struct ft260_device *dev, u8 addr, u8 *data,
dev->read_buf = NULL;
- ret = ft260_xfer_status(dev);
+ if (flag & FT260_FLAG_STOP)
+ bus_busy = FT260_I2C_STATUS_BUS_BUSY;
+
+ ret = ft260_xfer_status(dev, bus_busy);
if (ret < 0) {
ret = -EIO;
ft260_i2c_reset(hdev);
@@ -1003,7 +1020,7 @@ static int ft260_probe(struct hid_device *hdev, const struct hid_device_id *id)
mutex_init(&dev->lock);
init_completion(&dev->wait);
- ret = ft260_xfer_status(dev);
+ ret = ft260_xfer_status(dev, FT260_I2C_STATUS_BUS_BUSY);
if (ret)
ft260_i2c_reset(hdev);
--
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 of a single 128 bytes read request:
[ +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
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 of a single 128 bytes read request:
[ +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
Signed-off-by: Michael Zaidman <[email protected]>
Tested-by: Guillaume Champagne <[email protected]>
---
drivers/hid/hid-ft260.c | 127 +++++++++++++++++++++-------------------
1 file changed, 66 insertions(+), 61 deletions(-)
diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
index cec83f69ebdc..a354089bb747 100644
--- a/drivers/hid/hid-ft260.c
+++ b/drivers/hid/hid-ft260.c
@@ -456,49 +456,62 @@ 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;
- if (len > FT260_RD_DATA_MAX) {
- hid_err(hdev, "%s: unsupported rd len: %d\n", __func__, len);
- return -EINVAL;
- }
+ if ((flag & FT260_FLAG_START_REPEATED) == FT260_FLAG_START_REPEATED)
+ flag = FT260_FLAG_START_REPEATED;
+ else
+ flag = FT260_FLAG_START;
+ do {
+ if (len <= FT260_RD_DATA_MAX) {
+ rd_len = len;
+ flag |= FT260_FLAG_STOP;
+ } else {
+ rd_len = FT260_RD_DATA_MAX;
+ }
- dev->read_idx = 0;
- dev->read_buf = data;
- dev->read_len = len;
+ dev->read_idx = 0;
+ dev->read_buf = data;
+ dev->read_len = rd_len;
- rep.report = FT260_I2C_READ_REQ;
- rep.length = cpu_to_le16(len);
- rep.address = addr;
- rep.flag = flag;
+ rep.report = FT260_I2C_READ_REQ;
+ rep.length = cpu_to_le16(rd_len);
+ rep.address = addr;
+ rep.flag = flag;
- ft260_dbg("rep %#02x addr %#02x len %d\n", rep.report, rep.address,
- rep.length);
+ ft260_dbg("rep %#02x addr %#02x len %d rlen %d flag %#x\n",
+ rep.report, rep.address, len, rd_len, flag);
- reinit_completion(&dev->wait);
+ reinit_completion(&dev->wait);
- 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;
- }
+ 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;
+ }
- timeout = msecs_to_jiffies(5000);
- if (!wait_for_completion_timeout(&dev->wait, timeout)) {
- ft260_i2c_reset(hdev);
- return -ETIMEDOUT;
- }
+ timeout = msecs_to_jiffies(5000);
+ if (!wait_for_completion_timeout(&dev->wait, timeout)) {
+ ft260_i2c_reset(hdev);
+ return -ETIMEDOUT;
+ }
- ret = ft260_xfer_status(dev);
- if (ret == 0)
- return 0;
+ ret = ft260_xfer_status(dev);
+ if (ret < 0) {
+ ft260_i2c_reset(hdev);
+ return -EIO;
+ }
- ft260_i2c_reset(hdev);
- return -EIO;
+ len -= rd_len;
+ data += rd_len;
+ flag = 0;
+
+ } while (len > 0);
+
+ return 0;
}
/*
@@ -509,45 +522,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;
+ read_off = *msgs[0].buf;
- 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;
-
- 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
The FT260 controller does not return NACK when performing a big
read (of multiple hid reports size) from a non-existing device
or from the device responding with NACK when it is not ready
to serve the request. However, it responds correctly with NACK
to a read of up to a single hid report size.
To overcome this issue, we split the muli-report read request
into a read of a single HID report of 60 bytes size and a
multi-report read.
Big read of 256 bytes with first read of 60 bytes:
$ sudo ./i2cperf -d 2 -o 2 -s 256 -r 0-0xff 1 0x50 -S
[ +5.633280] ft260_i2c_write_read: off 0x0 rlen 255 wlen 2
[ +0.000006] ft260_i2c_write: rep 0xd0 addr 0x50 off 0 len 2 wlen 2 flag 0x2 d[0] 0x0
[ +0.013205] ft260_xfer_status: bus_status 0x20, clock 100
[ +0.000007] ft260_i2c_read: rep 0xc2 addr 0x50 len 255 rlen 60 flag 0x3
[ +0.010932] ft260_raw_event: i2c resp: rep 0xde len 60
[ +0.004733] ft260_xfer_status: bus_status 0x40, clock 100
[ +0.000006] ft260_i2c_read: rep 0xc2 addr 0x50 len 195 rlen 128 flag 0x0
[ +0.012572] ft260_raw_event: i2c resp: rep 0xde len 60
[ +0.005789] ft260_raw_event: i2c resp: rep 0xde len 60
[ +0.003189] ft260_raw_event: i2c resp: rep 0xd1 len 8
[ +0.004092] ft260_xfer_status: bus_status 0x40, clock 100
[ +0.000010] ft260_i2c_read: rep 0xc2 addr 0x50 len 67 rlen 67 flag 0x4
[ +0.011688] ft260_raw_event: i2c resp: rep 0xde len 60
[ +0.004700] ft260_raw_event: i2c resp: rep 0xd1 len 7
[ +0.004858] ft260_xfer_status: bus_status 0x20, clock 100
Read from non-existing device at address 8. The first 60 read responded
with NACK.
$ sudo ./i2cperf -d 2 -o 2 -s 256 -r 0-0xff 1 0x8 -S
[Oct19 15:37] ft260_i2c_write_read: off 0x0 rlen 255 wlen 2
[ +0.000007] ft260_i2c_write: rep 0xd0 addr 0x8 off 0 len 2 wlen 2 flag 0x2 d[0] 0x0
[ +0.022820] ft260_xfer_status: bus_status 0x20, clock 100
[ +0.000007] ft260_i2c_read: rep 0xc2 addr 0x8 len 255 rlen 60 flag 0x3
[ +0.010658] ft260_raw_event: i2c resp: rep 0xde len 60
[ +0.005965] ft260_xfer_status: bus_status 0x46, clock 100 <-- NACK
[ +0.000009] ft260 0003:0403:6030.0004: i2c bus error: 0x46
[ +0.007784] ft260_i2c_reset: done
Co-developed-by: Enrik Berkhan <[email protected]>
Signed-off-by: Enrik Berkhan <[email protected]>
Signed-off-by: Michael Zaidman <[email protected]>
---
drivers/hid/hid-ft260.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
index d186aa5a8e73..40fae81386e3 100644
--- a/drivers/hid/hid-ft260.c
+++ b/drivers/hid/hid-ft260.c
@@ -464,6 +464,7 @@ static int ft260_i2c_read(struct ft260_device *dev, u8 addr, u8 *data,
u16 len, u8 flag)
{
u16 rd_len;
+ u16 rd_data_max = 60;
int timeout, ret = 0;
struct ft260_i2c_read_request_report rep;
struct hid_device *hdev = dev->hdev;
@@ -473,12 +474,13 @@ static int ft260_i2c_read(struct ft260_device *dev, u8 addr, u8 *data,
else
flag = FT260_FLAG_START;
do {
- if (len <= FT260_RD_DATA_MAX) {
+ if (len <= rd_data_max) {
rd_len = len;
flag |= FT260_FLAG_STOP;
} else {
- rd_len = FT260_RD_DATA_MAX;
+ rd_len = rd_data_max;
}
+ rd_data_max = FT260_RD_DATA_MAX;
rep.report = FT260_I2C_READ_REQ;
rep.length = cpu_to_le16(rd_len);
--
2.34.1
The i2cdetect uses the SMBus Quick command by default to scan devices
on the I2C bus. The FT260 implements an I2C bus controller. The SMBus
is derived from I2C, but there are several differences between the
specifications of the two buses in the areas of timing, protocols,
operation modes, and electrical characteristics.
One of the differences is that the I2C devices allow the slave not
to ACK its slave address, but SMBus requires it to always ACK it as
a mechanism to detect a detachable device’s presence on the bus.
Since FT260 is the I2C bus controller, it does not acknowledge the
SMBus Quick write command, which sends a single bit to the device at
the place of the RD/WR bit.
The ft260 driver attempted to mimic the SMBus Quick Write functionality
by writing a single byte as the SMBus Byte Write command does.
Usually, one byte in the SMBus Quick Write will be fine. However, it may
cause problems with devices with a control register at offset 0, like
i2c muxes, for example, when scanned with the i2cdetect utility.
The i2cdetect with the "-r" option uses the SMBus Read Byte command,
which is a reasonable workaround. To prevent the I2C bus from locking
at write-only devices (most notably clock chips at address 0x69), use
the "-r" option in conjunction with scanning range parameters.
This patch removes the SMBus Quick command support.
$ sudo i2cdetect -y 13
Warning: Can't use SMBus Quick Write command, will skip some addresses
0 1 2 3 4 5 6 7 8 9 a b c d e f
00:
10:
20:
30: -- -- -- -- -- -- -- --
40:
50: 50 51 -- -- -- -- -- -- -- -- -- -- -- -- -- --
60:
70:
$ sudo i2cdetect -y -r 13
0 1 2 3 4 5 6 7 8 9 a b c d e f
00: -- -- -- -- -- -- -- --
10: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
20: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
30: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
40: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
50: 50 51 -- -- -- -- -- -- -- -- -- -- -- -- -- --
60: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
70: -- -- -- -- -- -- -- --
Reported-by: Vince Asbridge <[email protected]>
Reported-by: Stephen Shirron <[email protected]>
Reported-by: Enrik Berkhan <[email protected]>
Signed-off-by: Michael Zaidman <[email protected]>
---
drivers/hid/hid-ft260.c | 10 +---------
1 file changed, 1 insertion(+), 9 deletions(-)
diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
index 8b6ebc5228eb..d186aa5a8e73 100644
--- a/drivers/hid/hid-ft260.c
+++ b/drivers/hid/hid-ft260.c
@@ -630,14 +630,6 @@ static int ft260_smbus_xfer(struct i2c_adapter *adapter, u16 addr, u16 flags,
}
switch (size) {
- case I2C_SMBUS_QUICK:
- if (read_write == I2C_SMBUS_READ)
- ret = ft260_i2c_read(dev, addr, &data->byte, 0,
- FT260_FLAG_START_STOP);
- else
- ret = ft260_smbus_write(dev, addr, cmd, NULL, 0,
- FT260_FLAG_START_STOP);
- break;
case I2C_SMBUS_BYTE:
if (read_write == I2C_SMBUS_READ)
ret = ft260_i2c_read(dev, addr, &data->byte, 1,
@@ -720,7 +712,7 @@ static int ft260_smbus_xfer(struct i2c_adapter *adapter, u16 addr, u16 flags,
static u32 ft260_functionality(struct i2c_adapter *adap)
{
- return I2C_FUNC_I2C | I2C_FUNC_SMBUS_BYTE | I2C_FUNC_SMBUS_QUICK |
+ return I2C_FUNC_I2C | I2C_FUNC_SMBUS_BYTE |
I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA |
I2C_FUNC_SMBUS_BLOCK_DATA | I2C_FUNC_SMBUS_I2C_BLOCK;
}
--
2.34.1
Hi Michael,
I love your patch! Perhaps something to improve:
[auto build test WARNING on hid/for-next]
[also build test WARNING on linus/master v6.1-rc3 next-20221102]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Michael-Zaidman/HID-ft260-fixes-and-performance-improvements/20221031-043557
base: https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git for-next
patch link: https://lore.kernel.org/r/20221030203403.4637-5-michael.zaidman%40gmail.com
patch subject: [PATCH v3 04/12] HID: ft260: support i2c reads greater than HID report size
config: m68k-randconfig-s032-20221102
compiler: m68k-linux-gcc (GCC) 12.1.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# apt-get install sparse
# sparse version: v0.6.4-39-gce1a6720-dirty
# https://github.com/intel-lab-lkp/linux/commit/57eb0752edbe6db04f5b8e1161f3ad722708b8b2
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Michael-Zaidman/HID-ft260-fixes-and-performance-improvements/20221031-043557
git checkout 57eb0752edbe6db04f5b8e1161f3ad722708b8b2
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=m68k SHELL=/bin/bash drivers/hid/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
sparse warnings: (new ones prefixed by >>)
>> drivers/hid/hid-ft260.c:539:36: sparse: sparse: cast to restricted __be16
drivers/hid/hid-ft260.c:797:50: sparse: sparse: cast to restricted __le16
drivers/hid/hid-ft260.c:797:50: sparse: sparse: cast to restricted __le16
drivers/hid/hid-ft260.c:797:50: sparse: sparse: cast to restricted __le16
drivers/hid/hid-ft260.c:797:50: sparse: sparse: cast to restricted __le16
drivers/hid/hid-ft260.c:882:1: sparse: sparse: incorrect type in initializer (different base types) @@ expected unsigned short [usertype] *field @@ got restricted __le16 * @@
drivers/hid/hid-ft260.c:882:1: sparse: expected unsigned short [usertype] *field
drivers/hid/hid-ft260.c:882:1: sparse: got restricted __le16 *
drivers/hid/hid-ft260.c:883:1: sparse: sparse: incorrect type in assignment (different base types) @@ expected restricted __le16 [usertype] clock @@ got unsigned short [addressable] [usertype] clock @@
drivers/hid/hid-ft260.c:883:1: sparse: expected restricted __le16 [usertype] clock
drivers/hid/hid-ft260.c:883:1: sparse: got unsigned short [addressable] [usertype] clock
vim +539 drivers/hid/hid-ft260.c
516
517 /*
518 * A random read operation is implemented as a dummy write operation, followed
519 * by a current address read operation. The dummy write operation is used to
520 * load the target byte address into the current byte address counter, from
521 * which the subsequent current address read operation then reads.
522 */
523 static int ft260_i2c_write_read(struct ft260_device *dev, struct i2c_msg *msgs)
524 {
525 int ret;
526 int wr_len = msgs[0].len;
527 int rd_len = msgs[1].len;
528 struct hid_device *hdev = dev->hdev;
529 u8 addr = msgs[0].addr;
530 u16 read_off = 0;
531
532 if (wr_len > 2) {
533 hid_err(hdev, "%s: invalid wr_len: %d\n", __func__, wr_len);
534 return -EOPNOTSUPP;
535 }
536
537 if (ft260_debug) {
538 if (wr_len == 2)
> 539 read_off = be16_to_cpu(*(u16 *)msgs[0].buf);
540 else
541 read_off = *msgs[0].buf;
542
543 pr_info("%s: off %#x rlen %d wlen %d\n", __func__,
544 read_off, rd_len, wr_len);
545 }
546
547 ret = ft260_i2c_write(dev, addr, msgs[0].buf, wr_len,
548 FT260_FLAG_START);
549 if (ret < 0)
550 return ret;
551
552 ret = ft260_i2c_read(dev, addr, msgs[1].buf, rd_len,
553 FT260_FLAG_START_STOP_REPEATED);
554 if (ret < 0)
555 return ret;
556
557 return 0;
558 }
559
--
0-DAY CI Kernel Test Service
https://01.org/lkp