linux-kernel - Re: [RFC v3 3/5] hid-multitouch: support for PixCir-based panels

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <AANLkTikqApgrHEh90dvw+8zUne1HA3FmDQYGeEnqZ2W6@mail.gmail.com>
Date:	Tue, 11 Jan 2011 14:53:45 +0100
From:	Benjamin Tissoires <benjamin.tissoires@...c.fr>
To:	Henrik Rydberg <rydberg@...math.org>
Cc:	Stephane Chatty <chatty@...c.fr>,
	Dmitry Torokhov <dmitry.torokhov@...il.com>,
	Jiri Kosina <jkosina@...e.cz>, linux-input@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [RFC v3 3/5] hid-multitouch: support for PixCir-based panels

On Tue, Jan 11, 2011 at 2:00 PM, Henrik Rydberg <rydberg@...math.org> wrote:
>> >> +static int mt_compute_slot(struct mt_device *td)
>> >> +{
>> >> +     struct mt_class *cls = td->mtclass;
>> >> +
>> >> +     if (cls->quirks & MT_QUIRK_SLOT_IS_CONTACTID)
>> >> +             return slot_is_contactid(td);
>> >
>> > No real need for a function call here...
>>
>> just to prepare for the other patches
>
> I mean the line could simply read
>
>        return td->curdata.contactid;

If you want, but we will loose consistency in between the different
slot computation functions.

>
> [...]
>> >> +static int mt_event(struct hid_device *hid, struct hid_field *field,
>> >> +                             struct hid_usage *usage, __s32 value)
>> >> +{
>> >> +     struct mt_device *td = hid_get_drvdata(hid);
>> >> +
>> >> +     if (hid->claimed & HID_CLAIMED_INPUT) {
>> >> +             switch (usage->hid) {
>> >> +             case HID_DG_INRANGE:
>> >> +                     break;
>> >> +             case HID_DG_TIPSWITCH:
>> >> +                     td->curvalid = value;
>> >
>> > I do not think curvalid should depend on the touch state (which is
>> > what tipswitch is). Either move to INRANGE, or simply set to true.
>>
>> INRANGE is not all the time used as curvalid as you are saying.
>> At least for Stantum, curvalid is given by CONFIDENCE.
>
> Yes.
>
>> The solution of giving the value true to curvalid is not working too:
>> Cypress devices can send their touches over 2 reports of 7 touches,
>> even if it handles 10 touches.
>
> Yes.
>
>> Solution:
>> 1) td->curvalid = td->num_received < td->mtclass->maxcontacts;
>> (not sure it will work for all devices)
>
> For the ones where NOT_SEEN_MEANS_UP, it ought to be true. For egalax,
> it is not true.

After thinking about it, this case doesn't work at all. It's working
for cypress devices only because we are not using the default slot
retrieval function.
This device sends contactid=0 when the touch is not in range, and
starts for the in range touches at 0 too. The MT_CLS_CYPRESS saved us
here, but it won't save other devices.

>
>>
>> or 2) introduce MT_QUIRK_VALID_IS_INRANGE and MT_QUIRK_VALID_IS_CONFIDENCE
>
> Yes, I believe this is a good way to go.

Will do this way, but it might implies another patch for generaltouch
devices as I don't know if inrange or confidence is used.

>
>> or 3) let curvalid=value and disable the quirk
>> MT_QUIRK_NOT_SEEN_MEANS_UP (adding a FixMe comment above)
>
> This would mean setting curvalid = touch_state again, which according
> to the discussion is precisely what we should not do. We know we need
> to check whether the incoming packet represents data is whether it is
> just filling up the transfer protocol size. We just need to identify,
> per device, how that is done.
>
>> >> +                     if (value)
>> >> +                             td->num_expected = value - 1;
>> >
>> > The - 1 should be dropped here.
>>
>> Here is the really tricky one:
>>
>> if I drop -1,
>>
>> the test below will be:
>>  if (field->index == td->last_field_index
>>       && td->num_received >= td->num_expected)
>>
>> but during the init of the device, the kernel receive 1 event though
>> the driver is not ready.
>
> So the current code really needs to also check whether the driver is
> ready to events. Adding a state for it would be appropriate. As it
> stands, an unrelated syntatic oddity is covering up this fact.
>
>>
>> During this time, num_expected is at 0, and when we receive the
>> last_field_index: segfault!
>>
>> To sum up, no, we can't.
>
> Yes we can. :-)

I'll look into it but I promise nothing.

Thanks,
Benjamin

>
>> >> +     if (field->index == td->last_field_index
>> >> +             && td->num_received > td->num_expected)
>> >
>> > A ">=" here.
>
> still applies then.
>
> Thanks,
> Henrik
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
--
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/