| 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, 18 Feb 2015 12:11:59 +0200
From: Nikolai Kondrashov <spbnick@...il.com>
To: Benjamin Tissoires <benjamin.tissoires@...hat.com>,
Jiri Kosina <jkosina@...e.cz>
CC: Peter Hutterer <peter.hutterer@...-t.net>,
linux-input@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] HID: huion: enable button mode reporting
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.
> /* 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.
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.
> + 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?
> 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.
> + if (rc == -EPIPE) {
> + hid_err(hdev, "button mode switch not found\n");
> + rc = -ENODEV;
> + goto cleanup;
> + } else if (rc < 0) {
> + hid_err(hdev, "failed to switch to button mode: %d\n", rc);
> + rc = -ENODEV;
> + goto cleanup;
> + } else if (rc != len) {
> + hid_err(hdev, "invalid button mode switch\n");
> + rc = -ENODEV;
> + goto cleanup;
> + }
> rc = 0;
>
> cleanup:
> @@ -262,10 +309,16 @@ static int huion_raw_event(struct hid_device *hdev, struct hid_report *report,
> /* If this is a pen input report */
> if (intf->cur_altsetting->desc.bInterfaceNumber == 0 &&
> report->type == HID_INPUT_REPORT &&
> - report->id == 0x07 && size >= 2)
> + report->id == 0x07 && size >= 2) {
> /* Invert the in-range bit */
> data[1] ^= 0x40;
>
> + /* 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.
> + data[0] = 0x08;
> + }
> +
> return 0;
> }
>
>
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