| Message ID | 1422982530-4906-1-git-send-email-benjamin.tissoires@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Benjamin, > Slot 0 is released correclty, but when we look for Contact ID 2, the slot > 0 is then picked up again because it is marked as inactive (trackingID < 0). > > This is wrong, and we should not reuse a slot in the same frame. > The test should also check for input_mt_is_used(). Good catch! However... > @@ -453,7 +456,7 @@ int input_mt_get_slot_by_key(struct input_dev *dev, int key) > return s - mt->slots; > > for (s = mt->slots; s != mt->slots + mt->num_slots; s++) > - if (!input_mt_is_active(s)) { > + if (!input_mt_is_active(s) && !input_mt_is_used(mt, s)) { > s->key = key; > return s - mt->slots; > } > Here, you are changing the preconditions of the function without explicit reference to all its users. For one, it is now assumed that input_mt_is_used() is up-to-date, which requires either input_mt_drop_unused() or input_mt_sync_frame(), which does not seem to be true for all users of input_mt_get_slot_by_key(). After a couple of iterations with input_mt_report_slot_state() in those drivers, input_mt_is_used() will be true for all slots, and the driver will stop working. How about defering the deassignments until the end of the loop instead? That would remove possible reuse. Thanks, Henrik -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Feb 03 2015 or thereabouts, Henrik Rydberg wrote: > Hi Benjamin, > > > Slot 0 is released correclty, but when we look for Contact ID 2, the slot > > 0 is then picked up again because it is marked as inactive (trackingID < 0). > > > > This is wrong, and we should not reuse a slot in the same frame. > > The test should also check for input_mt_is_used(). > > Good catch! However... > > > @@ -453,7 +456,7 @@ int input_mt_get_slot_by_key(struct input_dev *dev, int key) > > return s - mt->slots; > > > > for (s = mt->slots; s != mt->slots + mt->num_slots; s++) > > - if (!input_mt_is_active(s)) { > > + if (!input_mt_is_active(s) && !input_mt_is_used(mt, s)) { > > s->key = key; > > return s - mt->slots; > > } > > > > Here, you are changing the preconditions of the function without explicit > reference to all its users. For one, it is now assumed that input_mt_is_used() > is up-to-date, which requires either input_mt_drop_unused() or > input_mt_sync_frame(), which does not seem to be true for all users of > input_mt_get_slot_by_key(). After a couple of iterations with > input_mt_report_slot_state() in those drivers, input_mt_is_used() will be true > for all slots, and the driver will stop working. Hmm... You are right. Thanks for pointing this out. This is something I should have definitively thought. So, on the kernel tree, I have only 5 drivers matching input_mt_get_slot_by_key: $ grep -lIr input_mt_get_slot_by_key | grep -E "\.[ch]$" drivers/hid/hid-logitech-hidpp.c drivers/hid/hid-multitouch.c drivers/hid/wacom_wac.c drivers/input/input-mt.c drivers/input/touchscreen/sur40.c drivers/input/touchscreen/pixcir_i2c_ts.c include/linux/input/mt.h On these five drivers, 4 are using properly input_mt_sync_frame, and only one is not. This one is the Wacom one :) For most of the touch devices (except those that are auto-detected and handled), the code just calls input_mt_report_pointer_emulation() instead of input_mt_sync_frame(). Changing the 5 calls in this driver and adding a requirement to call input_mt_sync_frame() when using input_mt_get_slot_by_key() does not seem terrible. > > How about defering the deassignments until the end of the loop instead? That > would remove possible reuse. > I don't think this is feasible. There is no loop in the slot assignment case. The state is stored by the input subsystem, and when we process a touch, all the previous touches have already been processed and forwarded to the user space. We *could* defer the slot release before sending the EV_SYN event, but that would force users to call input_mt_sync_frame() anyway to release the slots. So we would get an even greater requirement, because now, all users of the slotted protocol would have to call input_mt_sync_frame(). Cheers, Benjamin -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/input/input-mt.c b/drivers/input/input-mt.c index cb150a1..e02fd92 100644 --- a/drivers/input/input-mt.c +++ b/drivers/input/input-mt.c @@ -88,10 +88,13 @@ int input_mt_init_slots(struct input_dev *dev, unsigned int num_slots, goto err_mem; } - /* Mark slots as 'unused' */ + /* Mark slots as 'inactive' */ for (i = 0; i < num_slots; i++) input_mt_set_value(&mt->slots[i], ABS_MT_TRACKING_ID, -1); + /* Mark slots as 'unused' */ + mt->frame = 1; + dev->mt = mt; return 0; err_mem: @@ -453,7 +456,7 @@ int input_mt_get_slot_by_key(struct input_dev *dev, int key) return s - mt->slots; for (s = mt->slots; s != mt->slots + mt->num_slots; s++) - if (!input_mt_is_active(s)) { + if (!input_mt_is_active(s) && !input_mt_is_used(mt, s)) { s->key = key; return s - mt->slots; }
The case occured recently with a touchscreen using twice a slot during a single EV_SYN event: E: 0.288415 0000 0000 0000 # ------------ SYN_REPORT (0) ---------- E: 0.296207 0003 002f 0000 # EV_ABS / ABS_MT_SLOT 0 E: 0.296207 0003 0039 -001 # EV_ABS / ABS_MT_TRACKING_ID -1 E: 0.296207 0003 002f 0001 # EV_ABS / ABS_MT_SLOT 1 E: 0.296207 0003 0035 0908 # EV_ABS / ABS_MT_POSITION_X 908 E: 0.296207 0003 0036 1062 # EV_ABS / ABS_MT_POSITION_Y 1062 E: 0.296207 0003 002f 0000 # EV_ABS / ABS_MT_SLOT 0 E: 0.296207 0003 0039 8787 # EV_ABS / ABS_MT_TRACKING_ID 8787 E: 0.296207 0003 0035 1566 # EV_ABS / ABS_MT_POSITION_X 1566 E: 0.296207 0003 0036 0861 # EV_ABS / ABS_MT_POSITION_Y 861 E: 0.296207 0003 0000 0908 # EV_ABS / ABS_X 908 E: 0.296207 0003 0001 1062 # EV_ABS / ABS_Y 1062 E: 0.296207 0000 0000 0000 # ------------ SYN_REPORT (0) ---------- This occured because while having already slots 0 and 1 assigned, the touchscreen sent: 0.293377 Tip Switch: 0 | Contact Id: 0 | X: 539 | Y: 1960 | Contact Count: 3 0.294783 Tip Switch: 1 | Contact Id: 1 | X: 908 | Y: 1062 | Contact Count: 0 0.296187 Tip Switch: 1 | Contact Id: 2 | X: 1566 | Y: 861 | Contact Count: 0 Slot 0 is released correclty, but when we look for Contact ID 2, the slot 0 is then picked up again because it is marked as inactive (trackingID < 0). This is wrong, and we should not reuse a slot in the same frame. The test should also check for input_mt_is_used(). In addition, we need to initialize mt->frame to an other value than 0. With mt->frame being 0, all slots are tagged as being used, and so input_mt_get_slot_by_key() would return -1 for all requests. Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=88903 Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> --- Hi Dmitry, I wonder if this one should go to stable as well. Cheers, Benjamin drivers/input/input-mt.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)