| 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: Thu, 19 Feb 2015 13:37:24 +0200
From: Nikolai Kondrashov <spbnick@...il.com>
To: Benjamin Tissoires <benjamin.tissoires@...hat.com>
CC: Jiri Kosina <jkosina@...e.cz>,
Peter Hutterer <peter.hutterer@...-t.net>,
linux-input@...r.kernel.org, linux-kernel@...r.kernel.org,
DIGImend-devel <DIGImend-devel@...ts.sourceforge.net>
Subject: Re: [PATCH 1/2] HID: huion: enable button mode reporting
On 02/18/2015 10:24 PM, Benjamin Tissoires wrote:
> On Feb 18 2015 or thereabouts, Nikolai Kondrashov wrote:
>> On 02/18/2015 12:54 AM, Benjamin Tissoires wrote:
>>> diff --git a/drivers/hid/hid-huion.c b/drivers/hid/hid-huion.c
>>> index 61b68ca..50fbda4 100644
>>> --- a/drivers/hid/hid-huion.c
>>> +++ b/drivers/hid/hid-huion.c
>>> @@ -34,6 +34,9 @@ enum huion_ph_id {
>>> HUION_PH_ID_NUM
>>> };
>>>
>>> +/* header of a button report sent through the Pen report */
>>> +static const u8 button_report[] = {0x07, 0xa0, 0x01, 0x01};
>>
>> Hmm, I see the second byte being 0xe0 on Huion H610, the rest is the same.
>> Considering this, the fact that bit 7 is always 1 and bit 6 is pen proximity,
>> I think we can assume that bit 5 in byte 2 indicates button reports and get
>> away with just a "data[1] & 0x20" test.
>
> that would be a nicer approach. Thanks for the analysis.
> Actually, I understood the difference. I tested the bits _after_ the
> driver reverts the in-range bit :)
Ah, I missed that.
> The raw data is {0x07, 0xe0, 0x01, 0x01} on the H610 Pro too.
That's good, less weirdness to handle :)
>>> /* Report descriptor template placeholder */
>>> #define HUION_PH(_ID) HUION_PH_HEAD, HUION_PH_ID_##_ID
>>>
>>> @@ -81,6 +84,31 @@ static const __u8 huion_tablet_rdesc_template[] = {
>>> HUION_PH(PRESSURE_LM), /* Logical Maximum (PLACEHOLDER), */
>>> 0x81, 0x02, /* Input (Variable), */
>>> 0xC0, /* End Collection, */
>>> + 0x05, 0x01, /* Usage Page (Desktop) */
>>> + 0x09, 0x07, /* Usage (Keypad) */
>>> + 0xa1, 0x01, /* Collection (Application) */
>>> + 0x85, 0x08, /* Report ID (8) */
>>> + 0x05, 0x0d, /* Usage Page (Digitizers) */
>>> + 0x09, 0x22, /* Usage (Finger) */
>>
>> I'd say "Finger" usage is wrong here. The spec says:
>>
>> Finger
>>
>> CL – Any human appendage used as a transducer, such as a finger
>> touching a touch screen to set the location of the screen cursor. A
>> digitizer typically reports the coordinates of center of the finger.
>> In the Finger collection a Pointer physical collection will contain
>> the axes reported by the finger.
>>
>> I.e. the buttons are not a pointing device. The specification contains another
>> collection usage which seems more suitable:
>>
>> Tablet Function Keys
>>
>> CL – These controls are located on the surface of a digitizing tablet,
>> and may be implemented as actual switches, or as soft keys actuated by
>> the digitizing transducer. These are often used to trigger
>> location-independent macros or other events.
>
> Actually, the kernel knows about it: HID_DG_TABLETFUNCTIONKEY.
> I don't think it should influence to have it set. The hid processing
> would work on the BTN usages, not on the physical.
>
> [5 min later]
>
> yep, just works :)
Cool :)!
>> However the kernel doesn't seem to know anything about it (but we can fix
>> that). In my version of this I simply used a keyboard with buttons:
>>
>> 0x05, 0x01, /* Usage Page (Desktop), */
>> 0x09, 0x06, /* Usage (Keyboard), */
>> 0xA1, 0x01, /* Collection (Application), */
>> 0x85, 0xF7, /* Report ID (247), */
>> 0x05, 0x09, /* Usage Page (Button), */
>> 0x75, 0x01, /* Report Size (1), */
>> 0x95, 0x18, /* Report Count (24), */
>> 0x81, 0x03, /* Input (Constant, Variable), */
>> 0x19, 0x01, /* Usage Minimum (01h), */
>> 0x29, 0x08, /* Usage Maximum (08h), */
>> 0x95, 0x08, /* Report Count (8), */
>> 0x81, 0x02, /* Input (Variable), */
>> 0xC0 /* End Collection */
>>
>> Although it might not be entirely correct either.
>
> Even if no-one but hid-core uses the report descriptor, I would rather
> not declare ourself as a keyboard. It's shooting on our own foot if
> someone decides to actually merge a keyboard and a tablet.
Yes, I think you're right.
>>> + 0xa0, /* Collection (Physical) */
>>> + 0x14, /* Logical Minimum (0) */
>>> + 0x25, 0x01, /* Logical Maximum (1) */
>>> + 0x75, 0x08, /* Report Size (8) */
>>> + 0x95, 0x03, /* Report Count (3) */
>>> + 0x81, 0x03, /* Input (Cnst,Var,Abs) */
>>> + 0x05, 0x09, /* Usage Page (Button) */
>>> + 0x19, 0x01, /* Usage Minimum (1) */
>>> + 0x29, 0x08, /* Usage Maximum (8) */
>>> + 0x14, /* Logical Minimum (0) */
>>> + 0x25, 0x01, /* Logical Maximum (1) */
>>> + 0x75, 0x01, /* Report Size (1) */
>>> + 0x95, 0x08, /* Report Count (8) */
>>> + 0x81, 0x02, /* Input (Data,Var,Abs) */
>>> + 0x75, 0x08, /* Report Size (8) */
>>> + 0x95, 0x03, /* Report Count (3) */
>>> + 0x81, 0x03, /* Input (Cnst,Var,Abs) */
>>> + 0xc0, /* End Collection */
>>> + 0xc0, /* End Collection */
>>
>> Which tool did you use to generate this?
>
> My own custom-made:
> https://github.com/bentiss/hid-replay/blob/master/tools/editor.py
>
> not 100% implemented, but it works for me :)
Ah, nice :) Here is mine: https://github.com/DIGImend/hidrd
>>> 0xC0 /* End Collection */
>>> };
>>>
>>> @@ -205,6 +233,25 @@ static int huion_tablet_enable(struct hid_device *hdev)
>>> }
>>> }
>>>
>>> + /* switch to the button mode reporting */
>>> + rc = usb_control_msg(usb_dev, usb_rcvctrlpipe(usb_dev, 0),
>>> + USB_REQ_GET_DESCRIPTOR, USB_DIR_IN,
>>> + (USB_DT_STRING << 8) + 0x7b,
>>> + 0x0409, buf, len,
>>> + USB_CTRL_GET_TIMEOUT);
>>
>> I'm a bit uncomfortable about reusing a buffer which was sized specifically
>> for another task, as it's confusing. But it will work as is, so it's OK.
>
> Yes, and no :)
>
> Actually, I would prefer that we stick to what the Windows driver do.
> But it requests 32 bytes in each requests, and we receive 14 and 22
> IIRC. The trick I exploited here is that the ctrl message answers back
> at most len data, so we are find in both cases because 12 is less than
> 14 and 22. I am not sure we should check at all the length of the
> returning buffer (though for the first command, we have to be sure that
> we received enough to get the values in the buffer).
In that case, if we want to mimic the Windows driver we can request 32 bytes
always and do a compile-time check that our parameters fit into that.
> Side note: the huion-abstract-keyboard branch uses usb_string() instead
> of a plain usb_control_msg(). I like this much better and I think we
> should change the first call with that. This way, it will be clear that
> the tablet is not fully HID compatible and that we need to keep the usb
> access.
No, we can't do that to the parameters string, because usb_string() does
utf16s_to_utf8s on the received data.
>>> + /* check for buttons events and change the report ID */
>>> + if (size >= sizeof(button_report) &&
>>> + !memcmp(data, button_report, sizeof(button_report)))
>>
>> So, yes, I think it's better to have a "data[1] & 0x20" test here instead.
>
> Yep, works just fine.
Nice :)
Nick
--
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