[PATCH 0/3] Fix spurious button presses for some HID touchpads

[PATCH 0/3] Fix spurious button presses for some HID touchpads

[Andrew Duggan]

This series fixes an issue introduced by the switch to using an irq_domain
which caused some touchpads to report spurious button presses.

Andrew Duggan (3):
  Input: synaptics-rmi4 - disable the relative position IRQ in the F12
    driver
  Input: synaptics-rmi4 - use the number of valid bytes read when
    updating the attn_data fields in F11 and F12
  Input: synaptics-rmi4 - remove unused result_bits mask

 drivers/input/rmi4/rmi_f11.c |  9 +++------
 drivers/input/rmi4/rmi_f12.c | 32 ++++++++++++++++++++++++++++----
 2 files changed, 31 insertions(+), 10 deletions(-)

-- 
2.20.1

[PATCH 1/3] Input: synaptics-rmi4 - disable the relative position IRQ in the F12 driver

[Andrew Duggan]

This patch fixes an issue seen on HID touchpads which report finger
positions using RMI4 Function 12. The issue manifests itself as
spurious button presses as described in:
https://www.spinics.net/lists/linux-input/msg58618.html

Commit 24d28e4f1271 ("Input: synaptics-rmi4 - convert irq distribution
to irq_domain") switched the RMI4 driver to using an irq_domain to handle
RMI4 function interrupts. Functions with more then one interrupt now have
each interrupt mapped to their own IRQ and IRQ handler. The result of
this change is that the F12 IRQ handler was now getting called twice. Once
for the absolute data interrupt and once for the relative data interrupt.
For HID devices, calling rmi_f12_attention() a second time causes the
attn_data data pointer and size to be set incorrectly. When the touchpad
button is pressed, F30 will generate an interrupt and attempt to read the
F30 data from the invalid attn_data data pointer and report incorrect
button events.

This patch disables the F12 relative interrupt which prevents
rmi_f12_attention() from being called twice.

Signed-off-by: Andrew Duggan <aduggan@synaptics.com>
Reported-by: Simon Wood <simon@mungewell.org>
---
 drivers/input/rmi4/rmi_f12.c | 28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

diff --git a/drivers/input/rmi4/rmi_f12.c b/drivers/input/rmi4/rmi_f12.c
index d20a5d6780d1..734077f2c40b 100644
--- a/drivers/input/rmi4/rmi_f12.c
+++ b/drivers/input/rmi4/rmi_f12.c
@@ -55,6 +55,9 @@ struct f12_data {
 
 	const struct rmi_register_desc_item *data15;
 	u16 data15_offset;
+
+	unsigned long *abs_mask;
+	unsigned long *rel_mask;
 };
 
 static int rmi_f12_read_sensor_tuning(struct f12_data *f12)
@@ -291,9 +294,18 @@ static int rmi_f12_write_control_regs(struct rmi_function *fn)
 static int rmi_f12_config(struct rmi_function *fn)
 {
 	struct rmi_driver *drv = fn->rmi_dev->driver;
+	struct f12_data *f12 = dev_get_drvdata(&fn->dev);
+	struct rmi_2d_sensor *sensor;
 	int ret;
 
-	drv->set_irq_bits(fn->rmi_dev, fn->irq_mask);
+	sensor = &f12->sensor;
+
+	if (!sensor->report_abs)
+		drv->clear_irq_bits(fn->rmi_dev, f12->abs_mask);
+	else
+		drv->set_irq_bits(fn->rmi_dev, f12->abs_mask);
+
+	drv->clear_irq_bits(fn->rmi_dev, f12->rel_mask);
 
 	ret = rmi_f12_write_control_regs(fn);
 	if (ret)
@@ -315,9 +327,12 @@ static int rmi_f12_probe(struct rmi_function *fn)
 	struct rmi_device_platform_data *pdata = rmi_get_platform_data(rmi_dev);
 	struct rmi_driver_data *drvdata = dev_get_drvdata(&rmi_dev->dev);
 	u16 data_offset = 0;
+	int mask_size;
 
 	rmi_dbg(RMI_DEBUG_FN, &fn->dev, "%s\n", __func__);
 
+	mask_size = BITS_TO_LONGS(drvdata->irq_count) * sizeof(unsigned long);
+
 	ret = rmi_read(fn->rmi_dev, query_addr, &buf);
 	if (ret < 0) {
 		dev_err(&fn->dev, "Failed to read general info register: %d\n",
@@ -332,10 +347,19 @@ static int rmi_f12_probe(struct rmi_function *fn)
 		return -ENODEV;
 	}
 
-	f12 = devm_kzalloc(&fn->dev, sizeof(struct f12_data), GFP_KERNEL);
+	f12 = devm_kzalloc(&fn->dev, sizeof(struct f12_data) + mask_size * 2,
+			GFP_KERNEL);
 	if (!f12)
 		return -ENOMEM;
 
+	f12->abs_mask = (unsigned long *)((char *)f12
+			+ sizeof(struct f12_data));
+	f12->rel_mask = (unsigned long *)((char *)f12
+			+ sizeof(struct f12_data) + mask_size);
+
+	set_bit(fn->irq_pos, f12->abs_mask);
+	set_bit(fn->irq_pos + 1, f12->rel_mask);
+
 	f12->has_dribble = !!(buf & BIT(3));
 
 	if (fn->dev.of_node) {
-- 
2.20.1

Re: [PATCH 1/3] Input: synaptics-rmi4 - disable the relative position IRQ in the F12 driver

[Dmitry Torokhov]

Hi Andrew,

On Fri, Oct 25, 2019 at 12:25:56AM +0000, Andrew Duggan wrote:
> This patch fixes an issue seen on HID touchpads which report finger
> positions using RMI4 Function 12. The issue manifests itself as
> spurious button presses as described in:
> https://www.spinics.net/lists/linux-input/msg58618.html
> 
> Commit 24d28e4f1271 ("Input: synaptics-rmi4 - convert irq distribution
> to irq_domain") switched the RMI4 driver to using an irq_domain to handle
> RMI4 function interrupts. Functions with more then one interrupt now have
> each interrupt mapped to their own IRQ and IRQ handler. The result of
> this change is that the F12 IRQ handler was now getting called twice. Once
> for the absolute data interrupt and once for the relative data interrupt.
> For HID devices, calling rmi_f12_attention() a second time causes the
> attn_data data pointer and size to be set incorrectly. When the touchpad
> button is pressed, F30 will generate an interrupt and attempt to read the
> F30 data from the invalid attn_data data pointer and report incorrect
> button events.

Maybe we should create only 1 interrupt per function instead of
multiple? It looks like the functions read their entire block of data on
any interrupt received.

> 
> This patch disables the F12 relative interrupt which prevents
> rmi_f12_attention() from being called twice.

Don't we have similar issue with F11, and maybe others?

Also, as far as F12 goes, I see that it may mark sensor as reporting
relative coordinates, but I do not see where it would actually emit
relative events. I must be missing something here...

Thanks.

-- 
Dmitry

Re: [PATCH 1/3] Input: synaptics-rmi4 - disable the relative position IRQ in the F12 driver

[Andrew Duggan]

Hi Dmitry,

On 10/27/19 10:39 PM, Dmitry Torokhov wrote:
>
> Hi Andrew,
>
> On Fri, Oct 25, 2019 at 12:25:56AM +0000, Andrew Duggan wrote:
>> This patch fixes an issue seen on HID touchpads which report finger
>> positions using RMI4 Function 12. The issue manifests itself as
>> spurious button presses as described in:
>> https://www.spinics.net/lists/linux-input/msg58618.html
>>
>> Commit 24d28e4f1271 ("Input: synaptics-rmi4 - convert irq distribution
>> to irq_domain") switched the RMI4 driver to using an irq_domain to handle
>> RMI4 function interrupts. Functions with more then one interrupt now have
>> each interrupt mapped to their own IRQ and IRQ handler. The result of
>> this change is that the F12 IRQ handler was now getting called twice. Once
>> for the absolute data interrupt and once for the relative data interrupt.
>> For HID devices, calling rmi_f12_attention() a second time causes the
>> attn_data data pointer and size to be set incorrectly. When the touchpad
>> button is pressed, F30 will generate an interrupt and attempt to read the
>> F30 data from the invalid attn_data data pointer and report incorrect
>> button events.
> Maybe we should create only 1 interrupt per function instead of
> multiple? It looks like the functions read their entire block of data on
> any interrupt received.

Yes, we could only create 1 interrupt per function. We could modify the 
rmi_create_function_irq() function to only map a specific IRQ and call 
it from the function driver's probe function instead of 
rmi_function_probe(). I considered that, but it was a bit more of a 
modification then I wanted to make at this point.

>> This patch disables the F12 relative interrupt which prevents
>> rmi_f12_attention() from being called twice.
> Don't we have similar issue with F11, and maybe others?

Currently, F11 prevents this from happening by disabling one of the two 
IRQs on the touch controller (usually the relative IRQ). F11 will decide 
to report abs or rel events based on the F11 query registers. Then it 
will set the IRQ it wants and clear the IRQ it does not in 
rmi_f11_config(). Both IRQs are still mapped in the irq_domain, but the 
touch controller will only generate an IRQ for one of them and 
rmi_f11_attention() will only be called once. I believe F11 and F12 are 
the only RMI functions which report more then one IRQ. Even if 
technically the spec allows up to 6 to be declared. This patch just 
implements the same behavior as F11 for F12.

>
> Also, as far as F12 goes, I see that it may mark sensor as reporting
> relative coordinates, but I do not see where it would actually emit
> relative events. I must be missing something here...

Nope, you are not missing anything. The code to parse the relative data 
from the data9 register was never implemented. I noticed that too when 
creating this patch. This must have been an oversight during the 
original implementation of the F12 driver. I considered adding the code 
to report relative events, but I did not have any hardware readily 
available to test with. It seems unlikely that there would be a device 
using one of our touch controllers supporting F12 which only reported 
relative events. Also, no one seems to have complained about the last of 
reporting relative events in the last few years. Instead, in this patch 
I just clear the relative IRQ to prevent rmi_f12_attention() from being 
called twice. I could add a comment or a warning stating that reporting 
relative events in F12 is unsupported.

Thanks,

Andrew

> Thanks.
>
> --
> Dmitry

[PATCH 2/3] Input: synaptics-rmi4 - use the number of valid bytes read when updating the attn_data fields in F11 and F12

[Andrew Duggan]

Currently, rmi_f11_attention() and rmi_f12_attention() functions update
the attn_data data pointer and size based on the size of the expected
size of the attention data. However, if the actual valid data in the
attn buffer is less then the expected value then the updated data
pointer will point to memory beyond the end of the attn buffer. Using
the calculated valid_bytes instead will prevent this from happening.

Signed-off-by: Andrew Duggan <aduggan@synaptics.com>
---
 drivers/input/rmi4/rmi_f11.c | 4 ++--
 drivers/input/rmi4/rmi_f12.c | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/input/rmi4/rmi_f11.c b/drivers/input/rmi4/rmi_f11.c
index f28a7158b2ef..26c239325f95 100644
--- a/drivers/input/rmi4/rmi_f11.c
+++ b/drivers/input/rmi4/rmi_f11.c
@@ -1284,8 +1284,8 @@ static irqreturn_t rmi_f11_attention(int irq, void *ctx)
 			valid_bytes = f11->sensor.attn_size;
 		memcpy(f11->sensor.data_pkt, drvdata->attn_data.data,
 			valid_bytes);
-		drvdata->attn_data.data += f11->sensor.attn_size;
-		drvdata->attn_data.size -= f11->sensor.attn_size;
+		drvdata->attn_data.data += valid_bytes;
+		drvdata->attn_data.size -= valid_bytes;
 	} else {
 		error = rmi_read_block(rmi_dev,
 				data_base_addr, f11->sensor.data_pkt,
diff --git a/drivers/input/rmi4/rmi_f12.c b/drivers/input/rmi4/rmi_f12.c
index 734077f2c40b..7e97944f7616 100644
--- a/drivers/input/rmi4/rmi_f12.c
+++ b/drivers/input/rmi4/rmi_f12.c
@@ -212,8 +212,8 @@ static irqreturn_t rmi_f12_attention(int irq, void *ctx)
 			valid_bytes = sensor->attn_size;
 		memcpy(sensor->data_pkt, drvdata->attn_data.data,
 			valid_bytes);
-		drvdata->attn_data.data += sensor->attn_size;
-		drvdata->attn_data.size -= sensor->attn_size;
+		drvdata->attn_data.data += valid_bytes;
+		drvdata->attn_data.size -= valid_bytes;
 	} else {
 		retval = rmi_read_block(rmi_dev, f12->data_addr,
 					sensor->data_pkt, sensor->pkt_size);
-- 
2.20.1

[PATCH 3/3] Input: synaptics-rmi4 - remove unused result_bits mask

[Andrew Duggan]

The result_bits mask is no longer used by the driver and should be
removed.

Signed-off-by: Andrew Duggan <aduggan@synaptics.com>
---
 drivers/input/rmi4/rmi_f11.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/input/rmi4/rmi_f11.c b/drivers/input/rmi4/rmi_f11.c
index 26c239325f95..bbf9ae9f3f0c 100644
--- a/drivers/input/rmi4/rmi_f11.c
+++ b/drivers/input/rmi4/rmi_f11.c
@@ -510,7 +510,6 @@ struct f11_data {
 	struct rmi_2d_sensor_platform_data sensor_pdata;
 	unsigned long *abs_mask;
 	unsigned long *rel_mask;
-	unsigned long *result_bits;
 };
 
 enum f11_finger_state {
@@ -1057,7 +1056,7 @@ static int rmi_f11_initialize(struct rmi_function *fn)
 	/*
 	** init instance data, fill in values and create any sysfs files
 	*/
-	f11 = devm_kzalloc(&fn->dev, sizeof(struct f11_data) + mask_size * 3,
+	f11 = devm_kzalloc(&fn->dev, sizeof(struct f11_data) + mask_size * 2,
 			GFP_KERNEL);
 	if (!f11)
 		return -ENOMEM;
@@ -1076,8 +1075,6 @@ static int rmi_f11_initialize(struct rmi_function *fn)
 			+ sizeof(struct f11_data));
 	f11->rel_mask = (unsigned long *)((char *)f11
 			+ sizeof(struct f11_data) + mask_size);
-	f11->result_bits = (unsigned long *)((char *)f11
-			+ sizeof(struct f11_data) + mask_size * 2);
 
 	set_bit(fn->irq_pos, f11->abs_mask);
 	set_bit(fn->irq_pos + 1, f11->rel_mask);
-- 
2.20.1