| 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
| ||
|
Message-ID: <CAN+gG=GNucfg4-wCBZQtz0yJDjpTD_56Js3d68nm8VFi1UhZAA@mail.gmail.com>
Date: Tue, 30 Oct 2012 12:04:40 +0100
From: Benjamin Tissoires <benjamin.tissoires@...il.com>
To: Henrik Rydberg <rydberg@...omail.se>
Cc: Dmitry Torokhov <dmitry.torokhov@...il.com>,
Jiri Kosina <jkosina@...e.cz>,
Stephane Chatty <chatty@...c.fr>, linux-input@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 11/11] HID: hid-multitouch: get rid of usbhid depedency
for general path
Hi Henrik,
On Mon, Oct 29, 2012 at 11:57 PM, Henrik Rydberg <rydberg@...omail.se> wrote:
> Hi Benjamin,
>
>> This patch factorizes the hid set_feature command by using
>> hid_device->hid_output_raw_report instead of direclty relying on
>> usbhid. This makes the driver usb independant.
>>
>> However I still can't remove the 2 usb related headers because the
>> function mt_resume has a specific patch for usb devices.
>>
>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@...il.com>
>> ---
>> drivers/hid/hid-multitouch.c | 63 ++++++++++++++++++++++++++------------------
>> 1 file changed, 37 insertions(+), 26 deletions(-)
>
> In my drawer, I have a patchset that aims to remove all usbhid
> dependence, from all the drivers. Perhaps the attached patch is
> something to consider here?
yep, removing usbhid dependencies is a good thing.
See my review below :)
>
>>
>> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
>> index 21a120b..33038c5 100644
>> --- a/drivers/hid/hid-multitouch.c
>> +++ b/drivers/hid/hid-multitouch.c
>> @@ -670,47 +670,58 @@ static int mt_event(struct hid_device *hid, struct hid_field *field,
>> return 1;
>> }
>>
>> -static void mt_set_input_mode(struct hid_device *hdev)
>> +static void mt_set_feature(struct hid_device *hdev, __s8 feature_id,
>> + u8 value, size_t index)
>> {
>> - struct mt_device *td = hid_get_drvdata(hdev);
>> struct hid_report *r;
>> struct hid_report_enum *re;
>> + u8 *data;
>> + u8 max_value;
>> + int len;
>> +
>> + if (feature_id < 0 || !hdev->hid_output_raw_report)
>> + return;
>> +
>> + re = &hdev->report_enum[HID_FEATURE_REPORT];
>> + r = re->report_id_hash[feature_id];
>> + if (!r)
>> + return;
>> +
>> + len = ((r->size - 1) >> 3) + 1 + (r->id > 0);
>> + max_value = r->field[0]->logical_maximum;
>> + value = min(value, max_value);
>>
>> - if (td->inputmode < 0)
>> + if (r->field[0]->value[index] == value || len < 2 || index + 1 >= len)
>> return;
>>
>> - re = &(hdev->report_enum[HID_FEATURE_REPORT]);
>> - r = re->report_id_hash[td->inputmode];
>> - if (r) {
>> - r->field[0]->value[td->inputmode_index] = 0x02;
>> - usbhid_submit_report(hdev, r, USB_DIR_OUT);
>> + data = kzalloc(len, GFP_ATOMIC);
>> + if (!data) {
>> + hid_warn(hdev, "output queueing failed\n");
>> + return;
>> }
>> +
>> + data[0] = r->id;
>> + data[index + 1] = value;
>> + hdev->hid_output_raw_report(hdev, data, len, HID_FEATURE_REPORT);
>> + kfree(data);
>> }
>>
>> -static void mt_set_maxcontacts(struct hid_device *hdev)
>> +static void mt_set_input_mode(struct hid_device *hdev)
>> {
>> struct mt_device *td = hid_get_drvdata(hdev);
>> - struct hid_report *r;
>> - struct hid_report_enum *re;
>> - int fieldmax, max;
>>
>> - if (td->maxcontact_report_id < 0)
>> - return;
>> + mt_set_feature(hdev, td->inputmode, 0x02, td->inputmode_index);
>> +}
>>
>> - if (!td->mtclass.maxcontacts)
>> +static void mt_set_maxcontacts(struct hid_device *hdev)
>> +{
>> + struct mt_device *td = hid_get_drvdata(hdev);
>> + int max = td->mtclass.maxcontacts;
>> +
>> + if (!max)
>> return;
>>
>> - re = &hdev->report_enum[HID_FEATURE_REPORT];
>> - r = re->report_id_hash[td->maxcontact_report_id];
>> - if (r) {
>> - max = td->mtclass.maxcontacts;
>> - fieldmax = r->field[0]->logical_maximum;
>> - max = min(fieldmax, max);
>> - if (r->field[0]->value[0] != max) {
>> - r->field[0]->value[0] = max;
>> - usbhid_submit_report(hdev, r, USB_DIR_OUT);
>> - }
>> - }
>> + mt_set_feature(hdev, td->maxcontact_report_id, max, 0);
>> }
>>
>> static void mt_post_parse_default_settings(struct mt_device *td)
>> --
>> 1.7.11.7
>>
>
> Thanks,
> Henrik
>
> ---
>
> From 5d9a791e0a9e41bcea0fcb286e2849b403675f37 Mon Sep 17 00:00:00 2001
> From: Henrik Rydberg <rydberg@...omail.se>
> Date: Mon, 2 Jul 2012 20:38:21 +0200
> Subject: [PATCH 3/7] hid: extend interface with report requests
>
> ---
> drivers/hid/usbhid/hid-core.c | 14 ++++++++++++++
> include/linux/hid.h | 33 +++++++++++++++++++++++++++++++++
> 2 files changed, 47 insertions(+)
>
> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> index de1f9ac..3c618da 100644
> --- a/drivers/hid/usbhid/hid-core.c
> +++ b/drivers/hid/usbhid/hid-core.c
> @@ -1251,6 +1251,18 @@ static int usbhid_power(struct hid_device *hid, int lvl)
> return r;
> }
>
> +static void usbhid_send(struct hid_device *hid, struct hid_report *rep, int req)
> +{
> + switch (req) {
> + case HID_REQ_GET_REPORT:
> + usbhid_submit_report(hid, rep, USB_DIR_IN);
> + break;
> + case HID_REQ_SET_REPORT:
> + usbhid_submit_report(hid, rep, USB_DIR_OUT);
> + break;
> + }
> +}
> +
> static struct hid_ll_driver usb_hid_driver = {
> .parse = usbhid_parse,
> .start = usbhid_start,
> @@ -1259,6 +1271,8 @@ static struct hid_ll_driver usb_hid_driver = {
> .close = usbhid_close,
> .power = usbhid_power,
> .hidinput_input_event = usb_hidinput_input_event,
> + .send = usbhid_send,
the name here is a little bit misleading. You are using "send" to also
"get" reports...
Maybe "hid_request" is a better name. This will allows us to add the
missing function:
Get_Descriptor, Set_Descriptor, Get_Report Request, Set_Report
Request, Get_Idle, Set_Idle, Get_Protocol, Set_Protocol and maybe
others - even WAIT for instance :)
> + .wait = usbhid_wait_io,
is it really necessary to wait for IO? (I think it will not be one
line for hid over i2c...).
Cheers,
Benjamin
> };
>
> static int usbhid_probe(struct usb_interface *intf, const struct usb_device_id *id)
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index 241eb40..5e169c1 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -686,6 +686,8 @@ struct hid_driver {
> * @hidinput_input_event: event input event (e.g. ff or leds)
> * @parse: this method is called only once to parse the device data,
> * shouldn't allocate anything to not leak memory
> + * @send: send report request to device (e.g. feature report)
> + * @wait: wait for buffered io to complete (send/recv reports)
> */
> struct hid_ll_driver {
> int (*start)(struct hid_device *hdev);
> @@ -700,6 +702,11 @@ struct hid_ll_driver {
> unsigned int code, int value);
>
> int (*parse)(struct hid_device *hdev);
> +
> + void (*send)(struct hid_device *hdev,
> + struct hid_report *report, int req);
> + int (*wait)(struct hid_device *hdev);
> +
> };
>
> #define PM_HINT_FULLON 1<<5
> @@ -892,6 +899,32 @@ static inline int hid_hw_power(struct hid_device *hdev, int level)
> return hdev->ll_driver->power ? hdev->ll_driver->power(hdev, level) : 0;
> }
>
> +
> +/**
> + * hid_hw_send - send report request to device
> + *
> + * @hdev: hid device
> + * @report: report to send
> + * @req: hid request type
> + */
> +static inline void hid_hw_send(struct hid_device *hdev,
> + struct hid_report *report, int req)
> +{
> + if (hdev->ll_driver->send)
> + hdev->ll_driver->send(hdev, report, req);
> +}
> +
> +/**
> + * hid_hw_wait - wait for buffered io to complete
> + *
> + * @hdev: hid device
> + */
> +static inline void hid_hw_wait(struct hid_device *hdev)
> +{
> + if (hdev->ll_driver->wait)
> + hdev->ll_driver->wait(hdev);
> +}
> +
> int hid_report_raw_event(struct hid_device *hid, int type, u8 *data, int size,
> int interrupt);
>
> --
> 1.7.11.1
>
--
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