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

From: Andrew Duggan
Date: Tue Oct 29 2019 - 15:54:33 EST


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