| 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, 10 Dec 2014 17:00:56 -0800
From: Andrew Duggan <aduggan@...aptics.com>
To: Benjamin Tissoires <benjamin.tissoires@...il.com>
CC: linux-input <linux-input@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Jiri Kosina <jkosina@...e.cz>
Subject: Re: [PATCH] HID: rmi: Add support for the touchpad in the Razer Blade
14 laptop
Hi Benjamin,
Thanks for reviewing my patch. Comments and questions below.
On 12/10/2014 12:51 PM, Benjamin Tissoires wrote:
> On Mon, Dec 8, 2014 at 7:16 PM, Andrew Duggan <aduggan@...aptics.com> wrote:
>> On 11/24/2014 04:20 PM, Andrew Duggan wrote:
>>> The Razer Blade 14 has a Synaptic's TouchPad on one of the interfaces of
>>> a composite USB device. This patch allows the hid-rmi driver to bind
>>> to that interface. It also adds support for the external click buttons
>>> on the Razer's touchpad. External buttons are reported using generic
>>> mouse reports instead of through the F30 like it is on ClickPads.
>>>
>>> Signed-off-by: Andrew Duggan <aduggan@...aptics.com>
>>> ---
>>> This patch depends on the "HID: rmi: Scan the report descriptor to
>>> determine if the device is
>>> suitable for the hid-rmi driver" I submitted earlier today to correctly
>>> bind to the touchpad HID
>>> device in the composite USB device.
>> Any comments on this patch?
> Again, sorry for the lag on this series. I think now that I re-read
> this one I understood why I did not put too many efforts to properly
> review the series. This one is a little bit worrisome IMO.
>
I wasn't sure about this patch either. But, after waiting a while and
not coming with with anything better I figured I would post it to at
least get some feedback.
>> Thanks,
>> Andrew
>>
>>> drivers/hid/hid-core.c | 4 +++-
>>> drivers/hid/hid-ids.h | 3 +++
>>> drivers/hid/hid-rmi.c | 15 ++++++++++++++-
>>> 3 files changed, 20 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
>>> index ba9dc59..d69ea16 100644
>>> --- a/drivers/hid/hid-core.c
>>> +++ b/drivers/hid/hid-core.c
>>> @@ -792,7 +792,9 @@ static int hid_scan_report(struct hid_device *hid)
>>> /*
>>> * Vendor specific handlings
>>> */
>>> - if ((hid->vendor == USB_VENDOR_ID_SYNAPTICS) &&
>>> + if ((hid->vendor == USB_VENDOR_ID_SYNAPTICS
>>> + || (hid->vendor == USB_VENDOR_ID_RAZER
>>> + && hid->product == USB_DEVICE_ID_RAZER_BLADE_14)) &&
> I don't like this. We already have a blacklist, an ignore list, and
> there, we will have a new blacklist...
>
> I understood why you put this here, the current have_special_driver
> list will not fit 100%. Still, I find it not good.
>
> It works, but I think we should still head for an entry in
> have_special_driver, and a specific behavior in hid-rmi which would
> rely on hid-input to handle the keyboard/mouse buttons and the rest.
Are you suggesting that we have hid-rmi bind to all of the hid devices
on this USB device and then have hid-rmi decide what reports it should
process and let the remaining events continue to hid-input? I guess once
hid-rmi loads it could look at the hid report ids and determine if it is
one of our devices and if it should put the device into rmi mode. If not
simply act as a pass through.
>>> (hid->group == HID_GROUP_GENERIC)) {
>>> if ((parser->scan_flags & HID_SCAN_FLAG_VENDOR_SPECIFIC)
>>> && (parser->scan_flags &
>>> HID_SCAN_FLAG_GENDESK_POINTER))
>>> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
>>> index 25cd674..c677aad 100644
>>> --- a/drivers/hid/hid-ids.h
>>> +++ b/drivers/hid/hid-ids.h
>>> @@ -751,6 +751,9 @@
>>> #define USB_DEVICE_ID_QUANTA_OPTICAL_TOUCH_3001 0x3001
>>> #define USB_DEVICE_ID_QUANTA_OPTICAL_TOUCH_3008 0x3008
>>> +#define USB_VENDOR_ID_RAZER 0x1532
>>> +#define USB_DEVICE_ID_RAZER_BLADE_14 0x011D
>>> +
>>> #define USB_VENDOR_ID_REALTEK 0x0bda
>>> #define USB_DEVICE_ID_REALTEK_READER 0x0152
>>> diff --git a/drivers/hid/hid-rmi.c b/drivers/hid/hid-rmi.c
>>> index 3cccff7..1f131df 100644
>>> --- a/drivers/hid/hid-rmi.c
>>> +++ b/drivers/hid/hid-rmi.c
>>> @@ -453,7 +453,15 @@ static int rmi_raw_event(struct hid_device *hdev,
>>> case RMI_ATTN_REPORT_ID:
>>> return rmi_input_event(hdev, data, size);
>>> case RMI_MOUSE_REPORT_ID:
>>> - rmi_schedule_reset(hdev);
>>> + /*
>>> + * touchpads with physical mouse buttons will report those
>>> + * buttons in mouse reports even in RMI mode. Only reset
>>> + * the device if we see reports which contain X or Y data.
>>> + */
>>> + if (data[2] != 0 || data[3] != 0)
> this seems a little bit magical. There may not be an other solution,
> but I would prefer that we look at alternatives before using this
> magical numbers (will the touchpad always use the same report
> descriptor on the mouse interface?)
I did just look at the reports and see where X and Y where reported.
Since mouse mode is really only there for compatibility when no custom
driver is present I think it is unlikely that it will change in the
future. However, there is no guarantee that it won't change. I will see
if I can come up with is less of a hack. Maybe, I can use the event
callback instead of raw_event.
>>> + rmi_schedule_reset(hdev);
>>> + else
>>> + return 1;
>>> break;
>>> }
>>> @@ -871,6 +879,11 @@ static int rmi_input_mapping(struct hid_device
>>> *hdev,
>>> struct hid_input *hi, struct hid_field *field,
>>> struct hid_usage *usage, unsigned long **bit, int *max)
>>> {
>>> + if (field->application == HID_GD_POINTER
>>> + && (usage->hid & HID_USAGE_PAGE) == HID_UP_BUTTON)
>>> + /* Pass mouse button reports to generic code for
>>> processing */
>>> + return 0;
>>> +
>>> /* we want to make HID ignore the advertised HID collection */
>>> return -1;
>>> }
>>
> Cheers,
> Benjamin
Andrew
--
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