| Message ID | 20191025002527.3189-2-aduggan@synaptics.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | f6aabe1ff1d9d7bad0879253011216438bdb2530 |
| Headers | show |
| Series | Fix spurious button presses for some HID touchpads | expand |
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.
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
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) {
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(-)