linux-kernel - [PATCH v3 12/12] HID: ft260: missed NACK from busy device

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20221030203403.4637-13-michael.zaidman@gmail.com>
Date:   Sun, 30 Oct 2022 22:34:03 +0200
From:   Michael Zaidman <michael.zaidman@...il.com>
To:     jikos@...nel.org
Cc:     linux-kernel@...r.kernel.org, linux-input@...r.kernel.org,
        linux-i2c@...r.kernel.org, germain.hebert@...abb.com,
        Enrik.Berkhan@...a.de, Michael Zaidman <michael.zaidman@...il.com>
Subject: [PATCH v3 12/12] HID: ft260: missed NACK from busy device

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 <germain.hebert@...abb.com>
Signed-off-by: Germain Hebert <germain.hebert@...abb.com>
Signed-off-by: Michael Zaidman <michael.zaidman@...il.com>
---
 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