linux-kernel - [PATCH] input: goodix: Poll the 'buffer status' bit before reading data

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-next>] [day] [month] [year] [list]

Message-ID: <20170330133349.19931-1-paul@crapouillou.net>
Date:   Thu, 30 Mar 2017 15:33:49 +0200
From:   Paul Cercueil <paul@...pouillou.net>
To:     Bastien Nocera <hadess@...ess.net>,
        Dmitry Torokhov <dmitry.torokhov@...il.com>
CC:     <linux-input@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        "Paul Cercueil" <paul@...pouillou.net>
Subject: [PATCH] input: goodix: Poll the 'buffer status' bit before reading data

The Goodix panel triggers an interrupt on touch events. However, its
registers will contain the valid values a short time after the
interrupt, and not when it's raised. At that moment, the 'buffer status'
bit is set.

Previously, if the 'buffer status' bit was not set when the registers
were read, the data was discarded and no input event was emitted,
causing "finger down" or "finger up" events to be missed sometimes.

This went unnoticed until v4.9, as the DesignWare I2C driver commonly
used with this driver had enough latency for that bug to never trigger.

Now, in the IRQ handler we will poll (with a timeout) the 'buffer status'
bit and process the data of the panel as soon as this bit gets set.

Note that the Goodix panel will send a few spurious interrupts after the
'finger up' event, in which the 'buffer status' bit will never be set.

Signed-off-by: Paul Cercueil <paul@...pouillou.net>
---
 drivers/input/touchscreen/goodix.c | 30 +++++++++++++++++++++++-------
 1 file changed, 23 insertions(+), 7 deletions(-)

diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index 240b16f3ee97..7a8b89b87c2f 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -195,18 +195,34 @@ static int goodix_get_cfg_len(u16 id)

 static int goodix_ts_read_input_report(struct goodix_ts_data *ts, u8 *data)
 {
+	unsigned int timeout = 100;
 	int touch_num;
 	int error;

-	error = goodix_i2c_read(ts->client, GOODIX_READ_COOR_ADDR, data,
+	/* The 'buffer status' bit, which indicates that the data is valid, is
+	 * not set as soon as the interrupt is raised, but slightly after.
+	 */
+	do {
+		error = goodix_i2c_read(ts->client, GOODIX_READ_COOR_ADDR, data,
 				GOODIX_CONTACT_SIZE + 1);
-	if (error) {
-		dev_err(&ts->client->dev, "I2C transfer error: %d\n", error);
-		return error;
-	}
+		if (error) {
+			dev_err(&ts->client->dev, "I2C transfer error: %d\n",
+					error);
+			return error;
+		}

-	if (!(data[0] & 0x80))
-		return -EAGAIN;
+		if (data[0] & 0x80)
+			break;
+
+		usleep_range(100, 200);
+	} while (--timeout);
+
+	if (!timeout) {
+		/* The Goodix panel will send spurious interrupts after a
+		 * 'finger up' event, which will always cause a timeout.
+		 */
+		return 0;
+	}

 	touch_num = data[0] & 0x0f;
 	if (touch_num > ts->max_touch_num)
-- 
2.11.0