| 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
| ||
|
Date: Wed, 29 Aug 2012 17:18:16 +0200
From: Benjamin Tissoires <benjamin.tissoires@...c.fr>
To: Henrik Rydberg <rydberg@...omail.se>
Cc: Dmitry Torokhov <dmitry.torokhov@...il.com>,
Jiri Kosina <jkosina@...e.cz>, linux-input@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 20/20] HID: multitouch: Remove the redundant touch state
Hi Henrik,
Thanks for the new version. I still didn't review in depth the patch
(it seemed to be fair).
However, I found out 2 bugs while testing it (related to the whole
series apparently):
- I don't know if it is my configuration or not, but I'm receiving at
each frame (between two EV_SYN) all ABS_MT_SLOT events, even when
there are not used.
- After a little bit a playing with some surfaces, I can confuse the
input_mt_assign_slot_by_id function: for the very same contact id, I
got up to four different slots.... This accumulates the clicks and is
not very good.... :-(
I'll try now to spot the bug.
Cheers,
Benjamin
On Sun, Aug 26, 2012 at 2:57 PM, Henrik Rydberg <rydberg@...omail.se> wrote:
> With the input_mt_sync_frame() function in place, there is no longer
> any need to keep the full touch state in the driver. This patch
> removes the slot state and replaces the lookup code with the input-mt
> equivalent. The initialization code is moved to mt_input_configured(),
> to make sure the full HID report has been seen.
>
> Cc: Benjamin Tissoires <benjamin.tissoires@...c.fr>
> Signed-off-by: Henrik Rydberg <rydberg@...omail.se>
> ---
> drivers/hid/hid-multitouch.c | 133 +++++++++++++++----------------------------
> 1 file changed, 46 insertions(+), 87 deletions(-)
>
> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index c400d90..dc08a4e 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -56,7 +56,6 @@ struct mt_slot {
> __s32 x, y, p, w, h;
> __s32 contactid; /* the device ContactID assigned to this slot */
> bool touch_state; /* is the touch valid? */
> - bool seen_in_this_frame;/* has this slot been updated */
> };
>
> struct mt_class {
> @@ -93,7 +92,7 @@ struct mt_device {
> * 1 means we should use a serial protocol
> * > 1 means hybrid (multitouch) protocol */
> bool curvalid; /* is the current contact valid? */
> - struct mt_slot *slots;
> + unsigned mt_flags; /* flags to pass to input-mt */
> };
>
> /* classes of device behavior */
> @@ -134,25 +133,6 @@ static int cypress_compute_slot(struct mt_device *td)
> return -1;
> }
>
> -static int find_slot_from_contactid(struct mt_device *td)
> -{
> - int i;
> - for (i = 0; i < td->maxcontacts; ++i) {
> - if (td->slots[i].contactid == td->curdata.contactid &&
> - td->slots[i].touch_state)
> - return i;
> - }
> - for (i = 0; i < td->maxcontacts; ++i) {
> - if (!td->slots[i].seen_in_this_frame &&
> - !td->slots[i].touch_state)
> - return i;
> - }
> - /* should not occurs. If this happens that means
> - * that the device sent more touches that it says
> - * in the report descriptor. It is ignored then. */
> - return -1;
> -}
> -
> static struct mt_class mt_classes[] = {
> { .name = MT_CLS_DEFAULT,
> .quirks = MT_QUIRK_NOT_SEEN_MEANS_UP },
> @@ -319,24 +299,16 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
> * We need to ignore fields that belong to other collections
> * such as Mouse that might have the same GenericDesktop usages. */
> if (field->application == HID_DG_TOUCHSCREEN)
> - set_bit(INPUT_PROP_DIRECT, hi->input->propbit);
> + td->mt_flags |= INPUT_MT_DIRECT;
> else if (field->application != HID_DG_TOUCHPAD)
> return 0;
>
> - /* In case of an indirect device (touchpad), we need to add
> - * specific BTN_TOOL_* to be handled by the synaptics xorg
> - * driver.
> - * We also consider that touchscreens providing buttons are touchpads.
> + /*
> + * Model touchscreens providing buttons as touchpads.
> */
> if (field->application == HID_DG_TOUCHPAD ||
> - (usage->hid & HID_USAGE_PAGE) == HID_UP_BUTTON ||
> - cls->is_indirect) {
> - set_bit(INPUT_PROP_POINTER, hi->input->propbit);
> - set_bit(BTN_TOOL_FINGER, hi->input->keybit);
> - set_bit(BTN_TOOL_DOUBLETAP, hi->input->keybit);
> - set_bit(BTN_TOOL_TRIPLETAP, hi->input->keybit);
> - set_bit(BTN_TOOL_QUADTAP, hi->input->keybit);
> - }
> + (usage->hid & HID_USAGE_PAGE) == HID_UP_BUTTON)
> + td->mt_flags |= INPUT_MT_POINTER;
>
> /* eGalax devices provide a Digitizer.Stylus input which overrides
> * the correct Digitizers.Finger X/Y ranges.
> @@ -353,8 +325,6 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
> EV_ABS, ABS_MT_POSITION_X);
> set_abs(hi->input, ABS_MT_POSITION_X, field,
> cls->sn_move);
> - /* touchscreen emulation */
> - set_abs(hi->input, ABS_X, field, cls->sn_move);
> mt_store_field(usage, td, hi);
> td->last_field_index = field->index;
> return 1;
> @@ -363,8 +333,6 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
> EV_ABS, ABS_MT_POSITION_Y);
> set_abs(hi->input, ABS_MT_POSITION_Y, field,
> cls->sn_move);
> - /* touchscreen emulation */
> - set_abs(hi->input, ABS_Y, field, cls->sn_move);
> mt_store_field(usage, td, hi);
> td->last_field_index = field->index;
> return 1;
> @@ -390,7 +358,6 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
> case HID_DG_CONTACTID:
> if (!td->maxcontacts)
> td->maxcontacts = MT_DEFAULT_MAXCONTACT;
> - input_mt_init_slots(hi->input, td->maxcontacts, 0);
> mt_store_field(usage, td, hi);
> td->last_field_index = field->index;
> td->touches_by_report++;
> @@ -418,9 +385,6 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
> EV_ABS, ABS_MT_PRESSURE);
> set_abs(hi->input, ABS_MT_PRESSURE, field,
> cls->sn_pressure);
> - /* touchscreen emulation */
> - set_abs(hi->input, ABS_PRESSURE, field,
> - cls->sn_pressure);
> mt_store_field(usage, td, hi);
> td->last_field_index = field->index;
> return 1;
> @@ -464,7 +428,24 @@ static int mt_input_mapped(struct hid_device *hdev, struct hid_input *hi,
> return -1;
> }
>
> -static int mt_compute_slot(struct mt_device *td)
> +static void mt_input_configured(struct hid_device *hdev, struct hid_input *hi)
> +
> +{
> + struct mt_device *td = hid_get_drvdata(hdev);
> + struct mt_class *cls = &td->mtclass;
> +
> + if (cls->is_indirect)
> + td->mt_flags |= INPUT_MT_POINTER;
> +
> + if (cls->quirks & MT_QUIRK_NOT_SEEN_MEANS_UP)
> + td->mt_flags |= INPUT_MT_DROP_UNUSED;
> +
> + input_mt_init_slots(hi->input, td->maxcontacts, td->mt_flags);
> +
> + td->mt_flags = 0;
> +}
> +
> +static int mt_compute_slot(struct mt_device *td, struct input_dev *input)
> {
> __s32 quirks = td->mtclass.quirks;
>
> @@ -480,42 +461,23 @@ static int mt_compute_slot(struct mt_device *td)
> if (quirks & MT_QUIRK_SLOT_IS_CONTACTID_MINUS_ONE)
> return td->curdata.contactid - 1;
>
> - return find_slot_from_contactid(td);
> + return input_mt_assign_slot_by_id(input, td->curdata.contactid);
> }
>
> /*
> * this function is called when a whole contact has been processed,
> * so that it can assign it to a slot and store the data there
> */
> -static void mt_complete_slot(struct mt_device *td)
> +static void mt_complete_slot(struct mt_device *td, struct input_dev *input)
> {
> - td->curdata.seen_in_this_frame = true;
> if (td->curvalid) {
> - int slotnum = mt_compute_slot(td);
> -
> - if (slotnum >= 0 && slotnum < td->maxcontacts)
> - td->slots[slotnum] = td->curdata;
> - }
> - td->num_received++;
> -}
> + int slotnum = mt_compute_slot(td, input);
> + struct mt_slot *s = &td->curdata;
>
> + if (slotnum < 0 || slotnum >= td->maxcontacts)
> + return;
>
> -/*
> - * this function is called when a whole packet has been received and processed,
> - * so that it can decide what to send to the input layer.
> - */
> -static void mt_emit_event(struct mt_device *td, struct input_dev *input)
> -{
> - int i;
> -
> - for (i = 0; i < td->maxcontacts; ++i) {
> - struct mt_slot *s = &(td->slots[i]);
> - if ((td->mtclass.quirks & MT_QUIRK_NOT_SEEN_MEANS_UP) &&
> - !s->seen_in_this_frame) {
> - s->touch_state = false;
> - }
> -
> - input_mt_slot(input, i);
> + input_mt_slot(input, slotnum);
> input_mt_report_slot_state(input, MT_TOOL_FINGER,
> s->touch_state);
> if (s->touch_state) {
> @@ -532,16 +494,22 @@ static void mt_emit_event(struct mt_device *td, struct input_dev *input)
> input_event(input, EV_ABS, ABS_MT_TOUCH_MAJOR, major);
> input_event(input, EV_ABS, ABS_MT_TOUCH_MINOR, minor);
> }
> - s->seen_in_this_frame = false;
> -
> }
>
> - input_mt_report_pointer_emulation(input, true);
> - input_sync(input);
> - td->num_received = 0;
> + td->num_received++;
> }
>
>
> +/*
> + * this function is called when a whole packet has been received and processed,
> + * so that it can decide what to send to the input layer.
> + */
> +static void mt_sync_frame(struct mt_device *td, struct input_dev *input)
> +{
> + input_mt_sync_frame(input);
> + input_sync(input);
> + td->num_received = 0;
> +}
>
> static int mt_event(struct hid_device *hid, struct hid_field *field,
> struct hid_usage *usage, __s32 value)
> @@ -549,7 +517,7 @@ static int mt_event(struct hid_device *hid, struct hid_field *field,
> struct mt_device *td = hid_get_drvdata(hid);
> __s32 quirks = td->mtclass.quirks;
>
> - if (hid->claimed & HID_CLAIMED_INPUT && td->slots) {
> + if (hid->claimed & HID_CLAIMED_INPUT) {
> switch (usage->hid) {
> case HID_DG_INRANGE:
> if (quirks & MT_QUIRK_ALWAYS_VALID)
> @@ -602,11 +570,11 @@ static int mt_event(struct hid_device *hid, struct hid_field *field,
> }
>
> if (usage->hid == td->last_slot_field)
> - mt_complete_slot(td);
> + mt_complete_slot(td, field->hidinput->input);
>
> if (field->index == td->last_field_index
> && td->num_received >= td->num_expected)
> - mt_emit_event(td, field->hidinput->input);
> + mt_sync_frame(td, field->hidinput->input);
>
> }
>
> @@ -735,15 +703,6 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
> if (id->vendor == HID_ANY_ID && id->product == HID_ANY_ID)
> mt_post_parse_default_settings(td);
>
> - td->slots = kzalloc(td->maxcontacts * sizeof(struct mt_slot),
> - GFP_KERNEL);
> - if (!td->slots) {
> - dev_err(&hdev->dev, "cannot allocate multitouch slots\n");
> - hid_hw_stop(hdev);
> - ret = -ENOMEM;
> - goto fail;
> - }
> -
> ret = sysfs_create_group(&hdev->dev.kobj, &mt_attribute_group);
>
> mt_set_maxcontacts(hdev);
> @@ -774,7 +733,6 @@ static void mt_remove(struct hid_device *hdev)
> struct mt_device *td = hid_get_drvdata(hdev);
> sysfs_remove_group(&hdev->dev.kobj, &mt_attribute_group);
> hid_hw_stop(hdev);
> - kfree(td->slots);
> kfree(td);
> hid_set_drvdata(hdev, NULL);
> }
> @@ -1087,6 +1045,7 @@ static struct hid_driver mt_driver = {
> .remove = mt_remove,
> .input_mapping = mt_input_mapping,
> .input_mapped = mt_input_mapped,
> + .input_configured = mt_input_configured,
> .feature_mapping = mt_feature_mapping,
> .usage_table = mt_grabbed_usages,
> .event = mt_event,
> --
> 1.7.12
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists