[PATCH 00/14] HID: low-level transport cleanup, round 2

[PATCH 00/14] HID: low-level transport cleanup, round 2

[Benjamin Tissoires]

Hi guys,

this is the second part of the low-level HID transport cleanup.
The series goes on top of the previous one I sent last week.

Some highlights:
- remove hid_output_raw_report from struct hid_device
- uniformization of all transport driver by having only 2 mandatory
communication functions to implement: .raw_request and .output_report

Cheers,
Benjamin

Benjamin Tissoires (14):
  HID: uHID: remove duplicated code
  HID: uHID: implement .raw_request
  HID: core: implement generic .request()
  HID: i2c-hid: implement ll_driver transport-layer callbacks
  HID: i2c-hid: use generic .request() implementation
  HID: usbhid: change return error of usbhid_output_report
  HID: input: hid-input remove hid_output_raw_report call
  HID: logitech-dj: remove hid_output_raw_report call
  HID: replace hid_output_raw_report with hid_hw_raw_request for feature
    requests
  HID: wiimote: replace hid_output_raw_report with hid_hw_output_report
    for output requests
  HID: sony: remove hid_output_raw_report calls
  HID: hidraw: replace hid_output_raw_report() calls by appropriates
    ones
  HID: remove hid_output_raw_report
  HID: core: check parameters when sending/receiving data from the
    device

 drivers/hid/hid-core.c         | 45 +++++++++++++++++++++++-
 drivers/hid/hid-input.c        | 10 ++++--
 drivers/hid/hid-lg.c           |  8 ++---
 drivers/hid/hid-logitech-dj.c  | 21 ++++-------
 drivers/hid/hid-magicmouse.c   |  4 +--
 drivers/hid/hid-sony.c         | 79 +++++++++++++++++++++++++++++-------------
 drivers/hid/hid-thingm.c       |  4 +--
 drivers/hid/hid-wacom.c        | 26 ++++++++------
 drivers/hid/hid-wiimote-core.c |  4 +--
 drivers/hid/hidraw.c           | 22 +++++++++---
 drivers/hid/i2c-hid/i2c-hid.c  | 70 ++++++++++++++++++++-----------------
 drivers/hid/uhid.c             | 37 +++++++++-----------
 drivers/hid/usbhid/hid-core.c  | 14 +-------
 include/linux/hid.h            | 30 ++++++----------
 net/bluetooth/hidp/core.c      | 14 --------
 15 files changed, 218 insertions(+), 170 deletions(-)

-- 
1.8.3.1

[PATCH 01/14] HID: uHID: remove duplicated code

[Benjamin Tissoires]

uhid_hid_output_report() can be implemented through a simple call
to uhid_hid_output_raw().

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/uhid.c | 22 +---------------------
 1 file changed, 1 insertion(+), 21 deletions(-)

diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
index 12439e1..b6de903 100644
--- a/drivers/hid/uhid.c
+++ b/drivers/hid/uhid.c
@@ -247,27 +247,7 @@ static int uhid_hid_output_raw(struct hid_device *hid, __u8 *buf, size_t count,
 static int uhid_hid_output_report(struct hid_device *hid, __u8 *buf,
 				  size_t count)
 {
-	struct uhid_device *uhid = hid->driver_data;
-	unsigned long flags;
-	struct uhid_event *ev;
-
-	if (count < 1 || count > UHID_DATA_MAX)
-		return -EINVAL;
-
-	ev = kzalloc(sizeof(*ev), GFP_KERNEL);
-	if (!ev)
-		return -ENOMEM;
-
-	ev->type = UHID_OUTPUT;
-	ev->u.output.size = count;
-	ev->u.output.rtype = UHID_OUTPUT_REPORT;
-	memcpy(ev->u.output.data, buf, count);
-
-	spin_lock_irqsave(&uhid->qlock, flags);
-	uhid_queue(uhid, ev);
-	spin_unlock_irqrestore(&uhid->qlock, flags);
-
-	return count;
+	return uhid_hid_output_raw(hid, buf, count, HID_OUTPUT_REPORT);
 }
 
 static struct hid_ll_driver uhid_hid_driver = {
-- 
1.8.3.1

Re: [PATCH 01/14] HID: uHID: remove duplicated code

[David Herrmann]

Hi

On Mon, Feb 10, 2014 at 6:58 PM, Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
> uhid_hid_output_report() can be implemented through a simple call
> to uhid_hid_output_raw().
>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
>  drivers/hid/uhid.c | 22 +---------------------
>  1 file changed, 1 insertion(+), 21 deletions(-)
>
> diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
> index 12439e1..b6de903 100644
> --- a/drivers/hid/uhid.c
> +++ b/drivers/hid/uhid.c
> @@ -247,27 +247,7 @@ static int uhid_hid_output_raw(struct hid_device *hid, __u8 *buf, size_t count,
>  static int uhid_hid_output_report(struct hid_device *hid, __u8 *buf,
>                                   size_t count)
>  {
> -       struct uhid_device *uhid = hid->driver_data;
> -       unsigned long flags;
> -       struct uhid_event *ev;
> -
> -       if (count < 1 || count > UHID_DATA_MAX)
> -               return -EINVAL;
> -
> -       ev = kzalloc(sizeof(*ev), GFP_KERNEL);
> -       if (!ev)
> -               return -ENOMEM;
> -
> -       ev->type = UHID_OUTPUT;
> -       ev->u.output.size = count;
> -       ev->u.output.rtype = UHID_OUTPUT_REPORT;
> -       memcpy(ev->u.output.data, buf, count);
> -
> -       spin_lock_irqsave(&uhid->qlock, flags);
> -       uhid_queue(uhid, ev);
> -       spin_unlock_irqrestore(&uhid->qlock, flags);
> -
> -       return count;
> +       return uhid_hid_output_raw(hid, buf, count, HID_OUTPUT_REPORT);

Took me a while to see that we have:
  HID_OUTPUT_REPORT
  UHID_OUTPUT_REPORT
.. gnah! I should have named the uhid stuff better.. anyhow, patch looks good:

Reviewed-by: David Herrmann <dh.herrmann@gmail.com>

Thanks
David

>  }
>
>  static struct hid_ll_driver uhid_hid_driver = {
> --
> 1.8.3.1
>

[PATCH 02/14] HID: uHID: implement .raw_request

[Benjamin Tissoires]

uHID is missing a SET_REPORT protocol implementation, but as
.hid_get_raw_report() as been removed from struct hid_device,
there were no means to access GET_REPORT in uhid.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/uhid.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
index b6de903..60acee4 100644
--- a/drivers/hid/uhid.c
+++ b/drivers/hid/uhid.c
@@ -250,6 +250,21 @@ static int uhid_hid_output_report(struct hid_device *hid, __u8 *buf,
 	return uhid_hid_output_raw(hid, buf, count, HID_OUTPUT_REPORT);
 }
 
+static int uhid_raw_request(struct hid_device *hid, unsigned char reportnum,
+			    __u8 *buf, size_t len, unsigned char rtype,
+			    int reqtype)
+{
+	switch (reqtype) {
+	case HID_REQ_GET_REPORT:
+		return uhid_hid_get_raw(hid, reportnum, buf, len, rtype);
+	case HID_REQ_SET_REPORT:
+		/* TODO: implement proper SET_REPORT functionality */
+		return -ENOSYS;
+	default:
+		return -EIO;
+	}
+}
+
 static struct hid_ll_driver uhid_hid_driver = {
 	.start = uhid_hid_start,
 	.stop = uhid_hid_stop,
@@ -257,6 +272,7 @@ static struct hid_ll_driver uhid_hid_driver = {
 	.close = uhid_hid_close,
 	.parse = uhid_hid_parse,
 	.output_report = uhid_hid_output_report,
+	.raw_request = uhid_raw_request,
 };
 
 #ifdef CONFIG_COMPAT
-- 
1.8.3.1

Re: [PATCH 02/14] HID: uHID: implement .raw_request

[David Herrmann]

Hi

On Mon, Feb 10, 2014 at 6:58 PM, Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
> uHID is missing a SET_REPORT protocol implementation, but as
> .hid_get_raw_report() as been removed from struct hid_device,
> there were no means to access GET_REPORT in uhid.
>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
>  drivers/hid/uhid.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
> index b6de903..60acee4 100644
> --- a/drivers/hid/uhid.c
> +++ b/drivers/hid/uhid.c
> @@ -250,6 +250,21 @@ static int uhid_hid_output_report(struct hid_device *hid, __u8 *buf,
>         return uhid_hid_output_raw(hid, buf, count, HID_OUTPUT_REPORT);
>  }
>
> +static int uhid_raw_request(struct hid_device *hid, unsigned char reportnum,
> +                           __u8 *buf, size_t len, unsigned char rtype,
> +                           int reqtype)
> +{
> +       switch (reqtype) {
> +       case HID_REQ_GET_REPORT:
> +               return uhid_hid_get_raw(hid, reportnum, buf, len, rtype);
> +       case HID_REQ_SET_REPORT:
> +               /* TODO: implement proper SET_REPORT functionality */
> +               return -ENOSYS;

Looks good. I will try to implement it today and send a patch.

Reviewed-by: David Herrmann <dh.herrmann@gmail.com>

Thanks
David

> +       default:
> +               return -EIO;
> +       }
> +}
> +
>  static struct hid_ll_driver uhid_hid_driver = {
>         .start = uhid_hid_start,
>         .stop = uhid_hid_stop,
> @@ -257,6 +272,7 @@ static struct hid_ll_driver uhid_hid_driver = {
>         .close = uhid_hid_close,
>         .parse = uhid_hid_parse,
>         .output_report = uhid_hid_output_report,
> +       .raw_request = uhid_raw_request,
>  };
>
>  #ifdef CONFIG_COMPAT
> --
> 1.8.3.1
>

[PATCH 03/14] HID: core: implement generic .request()

[Benjamin Tissoires]

.request() can be emulated through .raw_request()
we can implement this emulation in hid-core, and make .request
not mandatory for transport layer drivers.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/hid-core.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
 include/linux/hid.h    |  5 ++++-
 2 files changed, 48 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 18fe49b..f36778a 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1248,6 +1248,11 @@ void hid_output_report(struct hid_report *report, __u8 *data)
 }
 EXPORT_SYMBOL_GPL(hid_output_report);
 
+static int hid_report_len(struct hid_report *report)
+{
+	return ((report->size - 1) >> 3) + 1 + (report->id > 0) + 7;
+}
+
 /*
  * Allocator for buffer that is going to be passed to hid_output_report()
  */
@@ -1258,7 +1263,7 @@ u8 *hid_alloc_report_buf(struct hid_report *report, gfp_t flags)
 	 * of implement() working on 8 byte chunks
 	 */
 
-	int len = ((report->size - 1) >> 3) + 1 + (report->id > 0) + 7;
+	int len = hid_report_len(report);
 
 	return kmalloc(len, flags);
 }
@@ -1314,6 +1319,44 @@ static struct hid_report *hid_get_report(struct hid_report_enum *report_enum,
 	return report;
 }
 
+/*
+ * Implement a generic .request() callback, using .raw_request()
+ * DO NOT USE in hid drivers directly, but through hid_hw_request instead.
+ */
+void __hid_request(struct hid_device *hid, struct hid_report *report,
+		int reqtype)
+{
+	char *buf;
+	int ret;
+	int len;
+
+	if (!hid->ll_driver->raw_request)
+		return;
+
+	buf = hid_alloc_report_buf(report, GFP_KERNEL);
+	if (!buf)
+		return;
+
+	len = hid_report_len(report);
+
+	if (reqtype == HID_REQ_SET_REPORT)
+		hid_output_report(report, buf);
+
+	ret = hid->ll_driver->raw_request(hid, report->id, buf, len,
+					  report->type, reqtype);
+	if (ret < 0) {
+		dbg_hid("unable to complete request: %d\n", ret);
+		goto out;
+	}
+
+	if (reqtype == HID_REQ_GET_REPORT)
+		hid_input_report(hid, report->type, buf, ret, 0);
+
+out:
+	kfree(buf);
+}
+EXPORT_SYMBOL_GPL(__hid_request);
+
 int hid_report_raw_event(struct hid_device *hid, int type, u8 *data, int size,
 		int interrupt)
 {
diff --git a/include/linux/hid.h b/include/linux/hid.h
index a837ede..09fbbd7 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -753,6 +753,7 @@ struct hid_field *hidinput_get_led_field(struct hid_device *hid);
 unsigned int hidinput_count_leds(struct hid_device *hid);
 __s32 hidinput_calc_abs_res(const struct hid_field *field, __u16 code);
 void hid_output_report(struct hid_report *report, __u8 *data);
+void __hid_request(struct hid_device *hid, struct hid_report *rep, int reqtype);
 u8 *hid_alloc_report_buf(struct hid_report *report, gfp_t flags);
 struct hid_device *hid_allocate_device(void);
 struct hid_report *hid_register_report(struct hid_device *device, unsigned type, unsigned id);
@@ -965,7 +966,9 @@ static inline void hid_hw_request(struct hid_device *hdev,
 				  struct hid_report *report, int reqtype)
 {
 	if (hdev->ll_driver->request)
-		hdev->ll_driver->request(hdev, report, reqtype);
+		return hdev->ll_driver->request(hdev, report, reqtype);
+
+	__hid_request(hdev, report, reqtype);
 }
 
 /**
-- 
1.8.3.1

Re: [PATCH 03/14] HID: core: implement generic .request()

[David Herrmann]

Hi

On Mon, Feb 10, 2014 at 6:58 PM, Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
> .request() can be emulated through .raw_request()
> we can implement this emulation in hid-core, and make .request
> not mandatory for transport layer drivers.
>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
>  drivers/hid/hid-core.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
>  include/linux/hid.h    |  5 ++++-
>  2 files changed, 48 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index 18fe49b..f36778a 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -1248,6 +1248,11 @@ void hid_output_report(struct hid_report *report, __u8 *data)
>  }
>  EXPORT_SYMBOL_GPL(hid_output_report);
>
> +static int hid_report_len(struct hid_report *report)
> +{
> +       return ((report->size - 1) >> 3) + 1 + (report->id > 0) + 7;

Just for clarity, this is equivalent to the following, right?

return DIV_ROUND_UP(report->size, 8) + !!(report->id > 0) + 7;

I always have to read that shifting code twice to get it.. Maybe add a
comment explaining the different entries here.

> +}
> +
>  /*
>   * Allocator for buffer that is going to be passed to hid_output_report()
>   */
> @@ -1258,7 +1263,7 @@ u8 *hid_alloc_report_buf(struct hid_report *report, gfp_t flags)
>          * of implement() working on 8 byte chunks
>          */
>
> -       int len = ((report->size - 1) >> 3) + 1 + (report->id > 0) + 7;
> +       int len = hid_report_len(report);
>
>         return kmalloc(len, flags);
>  }
> @@ -1314,6 +1319,44 @@ static struct hid_report *hid_get_report(struct hid_report_enum *report_enum,
>         return report;
>  }
>
> +/*
> + * Implement a generic .request() callback, using .raw_request()
> + * DO NOT USE in hid drivers directly, but through hid_hw_request instead.
> + */
> +void __hid_request(struct hid_device *hid, struct hid_report *report,
> +               int reqtype)
> +{
> +       char *buf;
> +       int ret;
> +       int len;
> +
> +       if (!hid->ll_driver->raw_request)
> +               return;
> +
> +       buf = hid_alloc_report_buf(report, GFP_KERNEL);
> +       if (!buf)
> +               return;
> +
> +       len = hid_report_len(report);
> +
> +       if (reqtype == HID_REQ_SET_REPORT)
> +               hid_output_report(report, buf);
> +
> +       ret = hid->ll_driver->raw_request(hid, report->id, buf, len,
> +                                         report->type, reqtype);
> +       if (ret < 0) {
> +               dbg_hid("unable to complete request: %d\n", ret);
> +               goto out;
> +       }
> +
> +       if (reqtype == HID_REQ_GET_REPORT)
> +               hid_input_report(hid, report->type, buf, ret, 0);
> +
> +out:
> +       kfree(buf);
> +}
> +EXPORT_SYMBOL_GPL(__hid_request);
> +

Looks good to me.

Reviewed-by: David Herrmann <dh.herrmann@gmail.com>

Thanks
David

>  int hid_report_raw_event(struct hid_device *hid, int type, u8 *data, int size,
>                 int interrupt)
>  {
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index a837ede..09fbbd7 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -753,6 +753,7 @@ struct hid_field *hidinput_get_led_field(struct hid_device *hid);
>  unsigned int hidinput_count_leds(struct hid_device *hid);
>  __s32 hidinput_calc_abs_res(const struct hid_field *field, __u16 code);
>  void hid_output_report(struct hid_report *report, __u8 *data);
> +void __hid_request(struct hid_device *hid, struct hid_report *rep, int reqtype);
>  u8 *hid_alloc_report_buf(struct hid_report *report, gfp_t flags);
>  struct hid_device *hid_allocate_device(void);
>  struct hid_report *hid_register_report(struct hid_device *device, unsigned type, unsigned id);
> @@ -965,7 +966,9 @@ static inline void hid_hw_request(struct hid_device *hdev,
>                                   struct hid_report *report, int reqtype)
>  {
>         if (hdev->ll_driver->request)
> -               hdev->ll_driver->request(hdev, report, reqtype);
> +               return hdev->ll_driver->request(hdev, report, reqtype);
> +
> +       __hid_request(hdev, report, reqtype);
>  }
>
>  /**
> --
> 1.8.3.1
>

Re: [PATCH 03/14] HID: core: implement generic .request()

[Benjamin Tissoires]

On Wed, Feb 12, 2014 at 5:25 AM, David Herrmann <dh.herrmann@gmail.com> wrote:
> Hi
>
> On Mon, Feb 10, 2014 at 6:58 PM, Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
>> .request() can be emulated through .raw_request()
>> we can implement this emulation in hid-core, and make .request
>> not mandatory for transport layer drivers.
>>
>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>> ---
>>  drivers/hid/hid-core.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
>>  include/linux/hid.h    |  5 ++++-
>>  2 files changed, 48 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
>> index 18fe49b..f36778a 100644
>> --- a/drivers/hid/hid-core.c
>> +++ b/drivers/hid/hid-core.c
>> @@ -1248,6 +1248,11 @@ void hid_output_report(struct hid_report *report, __u8 *data)
>>  }
>>  EXPORT_SYMBOL_GPL(hid_output_report);
>>
>> +static int hid_report_len(struct hid_report *report)
>> +{
>> +       return ((report->size - 1) >> 3) + 1 + (report->id > 0) + 7;
>
> Just for clarity, this is equivalent to the following, right?
>
> return DIV_ROUND_UP(report->size, 8) + !!(report->id > 0) + 7;

yes, it should (at least that's what I understand too :)

>
> I always have to read that shifting code twice to get it.. Maybe add a
> comment explaining the different entries here.

good idea.

>
>> +}
>> +
>>  /*
>>   * Allocator for buffer that is going to be passed to hid_output_report()
>>   */
>> @@ -1258,7 +1263,7 @@ u8 *hid_alloc_report_buf(struct hid_report *report, gfp_t flags)
>>          * of implement() working on 8 byte chunks
>>          */
>>
>> -       int len = ((report->size - 1) >> 3) + 1 + (report->id > 0) + 7;
>> +       int len = hid_report_len(report);
>>
>>         return kmalloc(len, flags);
>>  }
>> @@ -1314,6 +1319,44 @@ static struct hid_report *hid_get_report(struct hid_report_enum *report_enum,
>>         return report;
>>  }
>>
>> +/*
>> + * Implement a generic .request() callback, using .raw_request()
>> + * DO NOT USE in hid drivers directly, but through hid_hw_request instead.
>> + */
>> +void __hid_request(struct hid_device *hid, struct hid_report *report,
>> +               int reqtype)
>> +{
>> +       char *buf;
>> +       int ret;
>> +       int len;
>> +
>> +       if (!hid->ll_driver->raw_request)
>> +               return;
>> +
>> +       buf = hid_alloc_report_buf(report, GFP_KERNEL);
>> +       if (!buf)
>> +               return;
>> +
>> +       len = hid_report_len(report);

actually, after sending the patches, I was wondering if we should use
the +7 in hid_report_len.
"len" is used in .raw_request(), and the +7 was only for the implement(), right?

So maybe a device can reject this because the size of the report is too big...

Jiri, David, any ideas?

Cheers,
Benjamin

>> +
>> +       if (reqtype == HID_REQ_SET_REPORT)
>> +               hid_output_report(report, buf);
>> +
>> +       ret = hid->ll_driver->raw_request(hid, report->id, buf, len,
>> +                                         report->type, reqtype);
>> +       if (ret < 0) {
>> +               dbg_hid("unable to complete request: %d\n", ret);
>> +               goto out;
>> +       }
>> +
>> +       if (reqtype == HID_REQ_GET_REPORT)
>> +               hid_input_report(hid, report->type, buf, ret, 0);
>> +
>> +out:
>> +       kfree(buf);
>> +}
>> +EXPORT_SYMBOL_GPL(__hid_request);
>> +
>
> Looks good to me.
>
> Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
>
> Thanks
> David
>
>>  int hid_report_raw_event(struct hid_device *hid, int type, u8 *data, int size,
>>                 int interrupt)
>>  {
>> diff --git a/include/linux/hid.h b/include/linux/hid.h
>> index a837ede..09fbbd7 100644
>> --- a/include/linux/hid.h
>> +++ b/include/linux/hid.h
>> @@ -753,6 +753,7 @@ struct hid_field *hidinput_get_led_field(struct hid_device *hid);
>>  unsigned int hidinput_count_leds(struct hid_device *hid);
>>  __s32 hidinput_calc_abs_res(const struct hid_field *field, __u16 code);
>>  void hid_output_report(struct hid_report *report, __u8 *data);
>> +void __hid_request(struct hid_device *hid, struct hid_report *rep, int reqtype);
>>  u8 *hid_alloc_report_buf(struct hid_report *report, gfp_t flags);
>>  struct hid_device *hid_allocate_device(void);
>>  struct hid_report *hid_register_report(struct hid_device *device, unsigned type, unsigned id);
>> @@ -965,7 +966,9 @@ static inline void hid_hw_request(struct hid_device *hdev,
>>                                   struct hid_report *report, int reqtype)
>>  {
>>         if (hdev->ll_driver->request)
>> -               hdev->ll_driver->request(hdev, report, reqtype);
>> +               return hdev->ll_driver->request(hdev, report, reqtype);
>> +
>> +       __hid_request(hdev, report, reqtype);
>>  }
>>
>>  /**
>> --
>> 1.8.3.1
>>

Re: [PATCH 03/14] HID: core: implement generic .request()

[David Herrmann]

Hi

On Thu, Feb 13, 2014 at 4:21 PM, Benjamin Tissoires
<benjamin.tissoires@gmail.com> wrote:
> On Wed, Feb 12, 2014 at 5:25 AM, David Herrmann <dh.herrmann@gmail.com> wrote:
>> Hi
>>
>> On Mon, Feb 10, 2014 at 6:58 PM, Benjamin Tissoires
>> <benjamin.tissoires@redhat.com> wrote:
>>> .request() can be emulated through .raw_request()
>>> we can implement this emulation in hid-core, and make .request
>>> not mandatory for transport layer drivers.
>>>
>>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>>> ---
>>>  drivers/hid/hid-core.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
>>>  include/linux/hid.h    |  5 ++++-
>>>  2 files changed, 48 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
>>> index 18fe49b..f36778a 100644
>>> --- a/drivers/hid/hid-core.c
>>> +++ b/drivers/hid/hid-core.c
>>> @@ -1248,6 +1248,11 @@ void hid_output_report(struct hid_report *report, __u8 *data)
>>>  }
>>>  EXPORT_SYMBOL_GPL(hid_output_report);
>>>
>>> +static int hid_report_len(struct hid_report *report)
>>> +{
>>> +       return ((report->size - 1) >> 3) + 1 + (report->id > 0) + 7;
>>
>> Just for clarity, this is equivalent to the following, right?
>>
>> return DIV_ROUND_UP(report->size, 8) + !!(report->id > 0) + 7;
>
> yes, it should (at least that's what I understand too :)
>
>>
>> I always have to read that shifting code twice to get it.. Maybe add a
>> comment explaining the different entries here.
>
> good idea.
>
>>
>>> +}
>>> +
>>>  /*
>>>   * Allocator for buffer that is going to be passed to hid_output_report()
>>>   */
>>> @@ -1258,7 +1263,7 @@ u8 *hid_alloc_report_buf(struct hid_report *report, gfp_t flags)
>>>          * of implement() working on 8 byte chunks
>>>          */
>>>
>>> -       int len = ((report->size - 1) >> 3) + 1 + (report->id > 0) + 7;
>>> +       int len = hid_report_len(report);
>>>
>>>         return kmalloc(len, flags);
>>>  }
>>> @@ -1314,6 +1319,44 @@ static struct hid_report *hid_get_report(struct hid_report_enum *report_enum,
>>>         return report;
>>>  }
>>>
>>> +/*
>>> + * Implement a generic .request() callback, using .raw_request()
>>> + * DO NOT USE in hid drivers directly, but through hid_hw_request instead.
>>> + */
>>> +void __hid_request(struct hid_device *hid, struct hid_report *report,
>>> +               int reqtype)
>>> +{
>>> +       char *buf;
>>> +       int ret;
>>> +       int len;
>>> +
>>> +       if (!hid->ll_driver->raw_request)
>>> +               return;
>>> +
>>> +       buf = hid_alloc_report_buf(report, GFP_KERNEL);
>>> +       if (!buf)
>>> +               return;
>>> +
>>> +       len = hid_report_len(report);
>
> actually, after sending the patches, I was wondering if we should use
> the +7 in hid_report_len.
> "len" is used in .raw_request(), and the +7 was only for the implement(), right?
>
> So maybe a device can reject this because the size of the report is too big...
>
> Jiri, David, any ideas?

Yeah, we should allocate the +7 size, but we shouldn't use it as
"length" argument. We should just silently guarantee the buffer is big
enough.

Thanks
David

> Cheers,
> Benjamin
>
>>> +
>>> +       if (reqtype == HID_REQ_SET_REPORT)
>>> +               hid_output_report(report, buf);
>>> +
>>> +       ret = hid->ll_driver->raw_request(hid, report->id, buf, len,
>>> +                                         report->type, reqtype);
>>> +       if (ret < 0) {
>>> +               dbg_hid("unable to complete request: %d\n", ret);
>>> +               goto out;
>>> +       }
>>> +
>>> +       if (reqtype == HID_REQ_GET_REPORT)
>>> +               hid_input_report(hid, report->type, buf, ret, 0);
>>> +
>>> +out:
>>> +       kfree(buf);
>>> +}
>>> +EXPORT_SYMBOL_GPL(__hid_request);
>>> +
>>
>> Looks good to me.
>>
>> Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
>>
>> Thanks
>> David
>>
>>>  int hid_report_raw_event(struct hid_device *hid, int type, u8 *data, int size,
>>>                 int interrupt)
>>>  {
>>> diff --git a/include/linux/hid.h b/include/linux/hid.h
>>> index a837ede..09fbbd7 100644
>>> --- a/include/linux/hid.h
>>> +++ b/include/linux/hid.h
>>> @@ -753,6 +753,7 @@ struct hid_field *hidinput_get_led_field(struct hid_device *hid);
>>>  unsigned int hidinput_count_leds(struct hid_device *hid);
>>>  __s32 hidinput_calc_abs_res(const struct hid_field *field, __u16 code);
>>>  void hid_output_report(struct hid_report *report, __u8 *data);
>>> +void __hid_request(struct hid_device *hid, struct hid_report *rep, int reqtype);
>>>  u8 *hid_alloc_report_buf(struct hid_report *report, gfp_t flags);
>>>  struct hid_device *hid_allocate_device(void);
>>>  struct hid_report *hid_register_report(struct hid_device *device, unsigned type, unsigned id);
>>> @@ -965,7 +966,9 @@ static inline void hid_hw_request(struct hid_device *hdev,
>>>                                   struct hid_report *report, int reqtype)
>>>  {
>>>         if (hdev->ll_driver->request)
>>> -               hdev->ll_driver->request(hdev, report, reqtype);
>>> +               return hdev->ll_driver->request(hdev, report, reqtype);
>>> +
>>> +       __hid_request(hdev, report, reqtype);
>>>  }
>>>
>>>  /**
>>> --
>>> 1.8.3.1
>>>

[PATCH 04/14] HID: i2c-hid: implement ll_driver transport-layer callbacks

[Benjamin Tissoires]

Add output_report and raw_request to i2c-hid.
The current implementation of i2c_hid_output_raw_report decides
by itself if it should use a direct send of the output report
or use the data register (SET_REPORT). Split that by reimplement
the logic in __i2c_hid_output_raw_report() which will be dropped
soon.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/i2c-hid/i2c-hid.c | 69 +++++++++++++++++++++++++++++++++++++------
 1 file changed, 60 insertions(+), 9 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index f57de7f..07a054a 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c
@@ -257,12 +257,21 @@ static int i2c_hid_get_report(struct i2c_client *client, u8 reportType,
 	return 0;
 }
 
-static int i2c_hid_set_report(struct i2c_client *client, u8 reportType,
-		u8 reportID, unsigned char *buf, size_t data_len)
+/**
+ * i2c_hid_set_or_send_report: forward an incoming report to the device
+ * @client: the i2c_client of the device
+ * @reportType: 0x03 for HID_FEATURE_REPORT ; 0x02 for HID_OUTPUT_REPORT
+ * @reportID: the report ID
+ * @buf: the actual data to transfer, without the report ID
+ * @len: size of buf
+ * @use_data: true: use SET_REPORT HID command, false: send plain OUTPUT report
+ */
+static int i2c_hid_set_or_send_report(struct i2c_client *client, u8 reportType,
+		u8 reportID, unsigned char *buf, size_t data_len, bool use_data)
 {
 	struct i2c_hid *ihid = i2c_get_clientdata(client);
 	u8 *args = ihid->argsbuf;
-	const struct i2c_hid_cmd * hidcmd = &hid_set_report_cmd;
+	const struct i2c_hid_cmd *hidcmd;
 	int ret;
 	u16 dataRegister = le16_to_cpu(ihid->hdesc.wDataRegister);
 	u16 outputRegister = le16_to_cpu(ihid->hdesc.wOutputRegister);
@@ -279,6 +288,9 @@ static int i2c_hid_set_report(struct i2c_client *client, u8 reportType,
 
 	i2c_hid_dbg(ihid, "%s\n", __func__);
 
+	if (!use_data && maxOutputLength == 0)
+		return -ENOSYS;
+
 	if (reportID >= 0x0F) {
 		args[index++] = reportID;
 		reportID = 0x0F;
@@ -288,9 +300,10 @@ static int i2c_hid_set_report(struct i2c_client *client, u8 reportType,
 	 * use the data register for feature reports or if the device does not
 	 * support the output register
 	 */
-	if (reportType == 0x03 || maxOutputLength == 0) {
+	if (use_data) {
 		args[index++] = dataRegister & 0xFF;
 		args[index++] = dataRegister >> 8;
+		hidcmd = &hid_set_report_cmd;
 	} else {
 		args[index++] = outputRegister & 0xFF;
 		args[index++] = outputRegister >> 8;
@@ -559,7 +572,7 @@ static int i2c_hid_get_raw_report(struct hid_device *hid,
 }
 
 static int i2c_hid_output_raw_report(struct hid_device *hid, __u8 *buf,
-		size_t count, unsigned char report_type)
+		size_t count, unsigned char report_type, bool use_data)
 {
 	struct i2c_client *client = hid->driver_data;
 	int report_id = buf[0];
@@ -573,9 +586,9 @@ static int i2c_hid_output_raw_report(struct hid_device *hid, __u8 *buf,
 		count--;
 	}
 
-	ret = i2c_hid_set_report(client,
+	ret = i2c_hid_set_or_send_report(client,
 				report_type == HID_FEATURE_REPORT ? 0x03 : 0x02,
-				report_id, buf, count);
+				report_id, buf, count, use_data);
 
 	if (report_id && ret >= 0)
 		ret++; /* add report_id to the number of transfered bytes */
@@ -583,6 +596,42 @@ static int i2c_hid_output_raw_report(struct hid_device *hid, __u8 *buf,
 	return ret;
 }
 
+static int __i2c_hid_output_raw_report(struct hid_device *hid, __u8 *buf,
+		size_t count, unsigned char report_type)
+{
+	struct i2c_client *client = hid->driver_data;
+	struct i2c_hid *ihid = i2c_get_clientdata(client);
+	bool data = true; /* SET_REPORT */
+
+	if (report_type == HID_OUTPUT_REPORT)
+		data = le16_to_cpu(ihid->hdesc.wMaxOutputLength) == 0;
+
+	return i2c_hid_output_raw_report(hid, buf, count, report_type, data);
+}
+
+static int i2c_hid_output_report(struct hid_device *hid, __u8 *buf,
+		size_t count)
+{
+	return i2c_hid_output_raw_report(hid, buf, count, HID_OUTPUT_REPORT,
+			false);
+}
+
+static int i2c_hid_raw_request(struct hid_device *hid, unsigned char reportnum,
+			       __u8 *buf, size_t len, unsigned char rtype,
+			       int reqtype)
+{
+	switch (reqtype) {
+	case HID_REQ_GET_REPORT:
+		return i2c_hid_get_raw_report(hid, reportnum, buf, len, rtype);
+	case HID_REQ_SET_REPORT:
+		if (buf[0] != reportnum)
+			return -EINVAL;
+		return i2c_hid_output_raw_report(hid, buf, len, rtype, true);
+	default:
+		return -EIO;
+	}
+}
+
 static void i2c_hid_request(struct hid_device *hid, struct hid_report *rep,
 		int reqtype)
 {
@@ -606,7 +655,7 @@ static void i2c_hid_request(struct hid_device *hid, struct hid_report *rep,
 		break;
 	case HID_REQ_SET_REPORT:
 		hid_output_report(rep, buf);
-		i2c_hid_output_raw_report(hid, buf, len, rep->type);
+		i2c_hid_output_raw_report(hid, buf, len, rep->type, true);
 		break;
 	}
 
@@ -769,6 +818,8 @@ static struct hid_ll_driver i2c_hid_ll_driver = {
 	.close = i2c_hid_close,
 	.power = i2c_hid_power,
 	.request = i2c_hid_request,
+	.output_report = i2c_hid_output_report,
+	.raw_request = i2c_hid_raw_request,
 };
 
 static int i2c_hid_init_irq(struct i2c_client *client)
@@ -1003,7 +1054,7 @@ static int i2c_hid_probe(struct i2c_client *client,
 
 	hid->driver_data = client;
 	hid->ll_driver = &i2c_hid_ll_driver;
-	hid->hid_output_raw_report = i2c_hid_output_raw_report;
+	hid->hid_output_raw_report = __i2c_hid_output_raw_report;
 	hid->dev.parent = &client->dev;
 	ACPI_COMPANION_SET(&hid->dev, ACPI_COMPANION(&client->dev));
 	hid->bus = BUS_I2C;
-- 
1.8.3.1

Re: [PATCH 04/14] HID: i2c-hid: implement ll_driver transport-layer callbacks

[David Herrmann]

Hi

On Mon, Feb 10, 2014 at 6:58 PM, Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
> Add output_report and raw_request to i2c-hid.
> The current implementation of i2c_hid_output_raw_report decides
> by itself if it should use a direct send of the output report
> or use the data register (SET_REPORT). Split that by reimplement
> the logic in __i2c_hid_output_raw_report() which will be dropped
> soon.
>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
>  drivers/hid/i2c-hid/i2c-hid.c | 69 +++++++++++++++++++++++++++++++++++++------
>  1 file changed, 60 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> index f57de7f..07a054a 100644
> --- a/drivers/hid/i2c-hid/i2c-hid.c
> +++ b/drivers/hid/i2c-hid/i2c-hid.c
> @@ -257,12 +257,21 @@ static int i2c_hid_get_report(struct i2c_client *client, u8 reportType,
>         return 0;
>  }
>
> -static int i2c_hid_set_report(struct i2c_client *client, u8 reportType,
> -               u8 reportID, unsigned char *buf, size_t data_len)
> +/**
> + * i2c_hid_set_or_send_report: forward an incoming report to the device
> + * @client: the i2c_client of the device
> + * @reportType: 0x03 for HID_FEATURE_REPORT ; 0x02 for HID_OUTPUT_REPORT
> + * @reportID: the report ID
> + * @buf: the actual data to transfer, without the report ID
> + * @len: size of buf
> + * @use_data: true: use SET_REPORT HID command, false: send plain OUTPUT report
> + */
> +static int i2c_hid_set_or_send_report(struct i2c_client *client, u8 reportType,
> +               u8 reportID, unsigned char *buf, size_t data_len, bool use_data)
>  {
>         struct i2c_hid *ihid = i2c_get_clientdata(client);
>         u8 *args = ihid->argsbuf;
> -       const struct i2c_hid_cmd * hidcmd = &hid_set_report_cmd;
> +       const struct i2c_hid_cmd *hidcmd;
>         int ret;
>         u16 dataRegister = le16_to_cpu(ihid->hdesc.wDataRegister);
>         u16 outputRegister = le16_to_cpu(ihid->hdesc.wOutputRegister);
> @@ -279,6 +288,9 @@ static int i2c_hid_set_report(struct i2c_client *client, u8 reportType,
>
>         i2c_hid_dbg(ihid, "%s\n", __func__);
>
> +       if (!use_data && maxOutputLength == 0)
> +               return -ENOSYS;
> +
>         if (reportID >= 0x0F) {
>                 args[index++] = reportID;
>                 reportID = 0x0F;
> @@ -288,9 +300,10 @@ static int i2c_hid_set_report(struct i2c_client *client, u8 reportType,
>          * use the data register for feature reports or if the device does not
>          * support the output register
>          */
> -       if (reportType == 0x03 || maxOutputLength == 0) {
> +       if (use_data) {
>                 args[index++] = dataRegister & 0xFF;
>                 args[index++] = dataRegister >> 8;
> +               hidcmd = &hid_set_report_cmd;
>         } else {
>                 args[index++] = outputRegister & 0xFF;
>                 args[index++] = outputRegister >> 8;
> @@ -559,7 +572,7 @@ static int i2c_hid_get_raw_report(struct hid_device *hid,
>  }
>
>  static int i2c_hid_output_raw_report(struct hid_device *hid, __u8 *buf,
> -               size_t count, unsigned char report_type)
> +               size_t count, unsigned char report_type, bool use_data)
>  {
>         struct i2c_client *client = hid->driver_data;
>         int report_id = buf[0];
> @@ -573,9 +586,9 @@ static int i2c_hid_output_raw_report(struct hid_device *hid, __u8 *buf,
>                 count--;
>         }
>
> -       ret = i2c_hid_set_report(client,
> +       ret = i2c_hid_set_or_send_report(client,
>                                 report_type == HID_FEATURE_REPORT ? 0x03 : 0x02,
> -                               report_id, buf, count);
> +                               report_id, buf, count, use_data);
>
>         if (report_id && ret >= 0)
>                 ret++; /* add report_id to the number of transfered bytes */
> @@ -583,6 +596,42 @@ static int i2c_hid_output_raw_report(struct hid_device *hid, __u8 *buf,
>         return ret;
>  }
>
> +static int __i2c_hid_output_raw_report(struct hid_device *hid, __u8 *buf,
> +               size_t count, unsigned char report_type)
> +{
> +       struct i2c_client *client = hid->driver_data;
> +       struct i2c_hid *ihid = i2c_get_clientdata(client);
> +       bool data = true; /* SET_REPORT */
> +
> +       if (report_type == HID_OUTPUT_REPORT)
> +               data = le16_to_cpu(ihid->hdesc.wMaxOutputLength) == 0;

Yepp, thanks for splitting that logic out of the report-handling,
makes it much easier to follow.

Reviewed-by: David Herrmann <dh.herrmann@gmail.com>

Thanks
David

> +
> +       return i2c_hid_output_raw_report(hid, buf, count, report_type, data);
> +}
> +
> +static int i2c_hid_output_report(struct hid_device *hid, __u8 *buf,
> +               size_t count)
> +{
> +       return i2c_hid_output_raw_report(hid, buf, count, HID_OUTPUT_REPORT,
> +                       false);
> +}
> +
> +static int i2c_hid_raw_request(struct hid_device *hid, unsigned char reportnum,
> +                              __u8 *buf, size_t len, unsigned char rtype,
> +                              int reqtype)
> +{
> +       switch (reqtype) {
> +       case HID_REQ_GET_REPORT:
> +               return i2c_hid_get_raw_report(hid, reportnum, buf, len, rtype);
> +       case HID_REQ_SET_REPORT:
> +               if (buf[0] != reportnum)
> +                       return -EINVAL;
> +               return i2c_hid_output_raw_report(hid, buf, len, rtype, true);
> +       default:
> +               return -EIO;
> +       }
> +}
> +
>  static void i2c_hid_request(struct hid_device *hid, struct hid_report *rep,
>                 int reqtype)
>  {
> @@ -606,7 +655,7 @@ static void i2c_hid_request(struct hid_device *hid, struct hid_report *rep,
>                 break;
>         case HID_REQ_SET_REPORT:
>                 hid_output_report(rep, buf);
> -               i2c_hid_output_raw_report(hid, buf, len, rep->type);
> +               i2c_hid_output_raw_report(hid, buf, len, rep->type, true);
>                 break;
>         }
>
> @@ -769,6 +818,8 @@ static struct hid_ll_driver i2c_hid_ll_driver = {
>         .close = i2c_hid_close,
>         .power = i2c_hid_power,
>         .request = i2c_hid_request,
> +       .output_report = i2c_hid_output_report,
> +       .raw_request = i2c_hid_raw_request,
>  };
>
>  static int i2c_hid_init_irq(struct i2c_client *client)
> @@ -1003,7 +1054,7 @@ static int i2c_hid_probe(struct i2c_client *client,
>
>         hid->driver_data = client;
>         hid->ll_driver = &i2c_hid_ll_driver;
> -       hid->hid_output_raw_report = i2c_hid_output_raw_report;
> +       hid->hid_output_raw_report = __i2c_hid_output_raw_report;
>         hid->dev.parent = &client->dev;
>         ACPI_COMPANION_SET(&hid->dev, ACPI_COMPANION(&client->dev));
>         hid->bus = BUS_I2C;
> --
> 1.8.3.1
>

[PATCH 05/14] HID: i2c-hid: use generic .request() implementation

[Benjamin Tissoires]

Having our own .request() implementation does not give anything,
so use the generic binding.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/i2c-hid/i2c-hid.c | 31 -------------------------------
 1 file changed, 31 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index 07a054a..925fb8d 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c
@@ -632,36 +632,6 @@ static int i2c_hid_raw_request(struct hid_device *hid, unsigned char reportnum,
 	}
 }
 
-static void i2c_hid_request(struct hid_device *hid, struct hid_report *rep,
-		int reqtype)
-{
-	struct i2c_client *client = hid->driver_data;
-	char *buf;
-	int ret;
-	int len = i2c_hid_get_report_length(rep) - 2;
-
-	buf = kzalloc(len, GFP_KERNEL);
-	if (!buf)
-		return;
-
-	switch (reqtype) {
-	case HID_REQ_GET_REPORT:
-		ret = i2c_hid_get_raw_report(hid, rep->id, buf, len, rep->type);
-		if (ret < 0)
-			dev_err(&client->dev, "%s: unable to get report: %d\n",
-				__func__, ret);
-		else
-			hid_input_report(hid, rep->type, buf, ret, 0);
-		break;
-	case HID_REQ_SET_REPORT:
-		hid_output_report(rep, buf);
-		i2c_hid_output_raw_report(hid, buf, len, rep->type, true);
-		break;
-	}
-
-	kfree(buf);
-}
-
 static int i2c_hid_parse(struct hid_device *hid)
 {
 	struct i2c_client *client = hid->driver_data;
@@ -817,7 +787,6 @@ static struct hid_ll_driver i2c_hid_ll_driver = {
 	.open = i2c_hid_open,
 	.close = i2c_hid_close,
 	.power = i2c_hid_power,
-	.request = i2c_hid_request,
 	.output_report = i2c_hid_output_report,
 	.raw_request = i2c_hid_raw_request,
 };
-- 
1.8.3.1

Re: [PATCH 05/14] HID: i2c-hid: use generic .request() implementation

[David Herrmann]

Hi

On Mon, Feb 10, 2014 at 6:58 PM, Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
> Having our own .request() implementation does not give anything,
> so use the generic binding.
>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
>  drivers/hid/i2c-hid/i2c-hid.c | 31 -------------------------------
>  1 file changed, 31 deletions(-)
>
> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> index 07a054a..925fb8d 100644
> --- a/drivers/hid/i2c-hid/i2c-hid.c
> +++ b/drivers/hid/i2c-hid/i2c-hid.c
> @@ -632,36 +632,6 @@ static int i2c_hid_raw_request(struct hid_device *hid, unsigned char reportnum,
>         }
>  }
>
> -static void i2c_hid_request(struct hid_device *hid, struct hid_report *rep,
> -               int reqtype)
> -{
> -       struct i2c_client *client = hid->driver_data;
> -       char *buf;
> -       int ret;
> -       int len = i2c_hid_get_report_length(rep) - 2;
> -
> -       buf = kzalloc(len, GFP_KERNEL);

Haven't you recently fixed this to use hid_alloc_buffer()? Anyhow,
patch obviously looks good:

Reviewed-by: David Herrmann <dh.herrmann@gmail.com>

Thanks
David

> -       if (!buf)
> -               return;
> -
> -       switch (reqtype) {
> -       case HID_REQ_GET_REPORT:
> -               ret = i2c_hid_get_raw_report(hid, rep->id, buf, len, rep->type);
> -               if (ret < 0)
> -                       dev_err(&client->dev, "%s: unable to get report: %d\n",
> -                               __func__, ret);
> -               else
> -                       hid_input_report(hid, rep->type, buf, ret, 0);
> -               break;
> -       case HID_REQ_SET_REPORT:
> -               hid_output_report(rep, buf);
> -               i2c_hid_output_raw_report(hid, buf, len, rep->type, true);
> -               break;
> -       }
> -
> -       kfree(buf);
> -}
> -
>  static int i2c_hid_parse(struct hid_device *hid)
>  {
>         struct i2c_client *client = hid->driver_data;
> @@ -817,7 +787,6 @@ static struct hid_ll_driver i2c_hid_ll_driver = {
>         .open = i2c_hid_open,
>         .close = i2c_hid_close,
>         .power = i2c_hid_power,
> -       .request = i2c_hid_request,
>         .output_report = i2c_hid_output_report,
>         .raw_request = i2c_hid_raw_request,
>  };
> --
> 1.8.3.1
>

Re: [PATCH 05/14] HID: i2c-hid: use generic .request() implementation

[Benjamin Tissoires]

On Wed, Feb 12, 2014 at 5:30 AM, David Herrmann <dh.herrmann@gmail.com> wrote:
> Hi
>
> On Mon, Feb 10, 2014 at 6:58 PM, Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
>> Having our own .request() implementation does not give anything,
>> so use the generic binding.
>>
>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>> ---
>>  drivers/hid/i2c-hid/i2c-hid.c | 31 -------------------------------
>>  1 file changed, 31 deletions(-)
>>
>> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
>> index 07a054a..925fb8d 100644
>> --- a/drivers/hid/i2c-hid/i2c-hid.c
>> +++ b/drivers/hid/i2c-hid/i2c-hid.c
>> @@ -632,36 +632,6 @@ static int i2c_hid_raw_request(struct hid_device *hid, unsigned char reportnum,
>>         }
>>  }
>>
>> -static void i2c_hid_request(struct hid_device *hid, struct hid_report *rep,
>> -               int reqtype)
>> -{
>> -       struct i2c_client *client = hid->driver_data;
>> -       char *buf;
>> -       int ret;
>> -       int len = i2c_hid_get_report_length(rep) - 2;
>> -
>> -       buf = kzalloc(len, GFP_KERNEL);
>
> Haven't you recently fixed this to use hid_alloc_buffer()? Anyhow,

yes, it has been fixed. But Jiri carried it through a *-upstream
branch, which is not merged into his for-next currently (or at the
time I sent the patch). hopefully the merge will be easy, or I can
wait for it to be in for-next before resending a smoother v2.

Cheers,
Benjamin

> patch obviously looks good:
>
> Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
>
> Thanks
> David
>
>> -       if (!buf)
>> -               return;
>> -
>> -       switch (reqtype) {
>> -       case HID_REQ_GET_REPORT:
>> -               ret = i2c_hid_get_raw_report(hid, rep->id, buf, len, rep->type);
>> -               if (ret < 0)
>> -                       dev_err(&client->dev, "%s: unable to get report: %d\n",
>> -                               __func__, ret);
>> -               else
>> -                       hid_input_report(hid, rep->type, buf, ret, 0);
>> -               break;
>> -       case HID_REQ_SET_REPORT:
>> -               hid_output_report(rep, buf);
>> -               i2c_hid_output_raw_report(hid, buf, len, rep->type, true);
>> -               break;
>> -       }
>> -
>> -       kfree(buf);
>> -}
>> -
>>  static int i2c_hid_parse(struct hid_device *hid)
>>  {
>>         struct i2c_client *client = hid->driver_data;
>> @@ -817,7 +787,6 @@ static struct hid_ll_driver i2c_hid_ll_driver = {
>>         .open = i2c_hid_open,
>>         .close = i2c_hid_close,
>>         .power = i2c_hid_power,
>> -       .request = i2c_hid_request,
>>         .output_report = i2c_hid_output_report,
>>         .raw_request = i2c_hid_raw_request,
>>  };
>> --
>> 1.8.3.1
>>

[PATCH 06/14] HID: usbhid: change return error of usbhid_output_report

[Benjamin Tissoires]

If there is no urbout when sending a output report, ENOSYS (Function
not implemented) is a better error than EIO (I/O error).

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/usbhid/hid-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index b9a770f..0d1d875 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -922,7 +922,7 @@ static int usbhid_output_report(struct hid_device *hid, __u8 *buf, size_t count)
 	int actual_length, skipped_report_id = 0, ret;
 
 	if (!usbhid->urbout)
-		return -EIO;
+		return -ENOSYS;
 
 	if (buf[0] == 0x0) {
 		/* Don't send the Report ID */
-- 
1.8.3.1

Re: [PATCH 06/14] HID: usbhid: change return error of usbhid_output_report

[David Herrmann]

Hi

On Mon, Feb 10, 2014 at 6:58 PM, Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
> If there is no urbout when sending a output report, ENOSYS (Function
> not implemented) is a better error than EIO (I/O error).

Reviewed-by: David Herrmann <dh.herrmann@gmail.com>

Thanks
David

> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
>  drivers/hid/usbhid/hid-core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> index b9a770f..0d1d875 100644
> --- a/drivers/hid/usbhid/hid-core.c
> +++ b/drivers/hid/usbhid/hid-core.c
> @@ -922,7 +922,7 @@ static int usbhid_output_report(struct hid_device *hid, __u8 *buf, size_t count)
>         int actual_length, skipped_report_id = 0, ret;
>
>         if (!usbhid->urbout)
> -               return -EIO;
> +               return -ENOSYS;
>
>         if (buf[0] == 0x0) {
>                 /* Don't send the Report ID */
> --
> 1.8.3.1
>

[PATCH 07/14] HID: input: hid-input remove hid_output_raw_report call

[Benjamin Tissoires]

hid_output_raw_report() is not a ll_driver callback and should not be used.
To keep the same code path than before, we are forced to play with the
different hid_hw_* calls: if the usb or i2c device does not support
direct output reports, then we will rely on the SET_REPORT HID call.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/hid-input.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index eb00a5b..6b7bdca 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -1153,7 +1153,7 @@ static void hidinput_led_worker(struct work_struct *work)
 					      led_work);
 	struct hid_field *field;
 	struct hid_report *report;
-	int len;
+	int len, ret;
 	__u8 *buf;
 
 	field = hidinput_get_led_field(hid);
@@ -1187,7 +1187,10 @@ static void hidinput_led_worker(struct work_struct *work)
 
 	hid_output_report(report, buf);
 	/* synchronous output report */
-	hid_output_raw_report(hid, buf, len, HID_OUTPUT_REPORT);
+	ret = hid_hw_output_report(hid, buf, len);
+	if (ret == -ENOSYS)
+		hid_hw_raw_request(hid, buf[0], buf, len, HID_OUTPUT_REPORT,
+				HID_REQ_SET_REPORT);
 	kfree(buf);
 }
 
@@ -1266,7 +1269,8 @@ static struct hid_input *hidinput_allocate(struct hid_device *hid)
 	}
 
 	input_set_drvdata(input_dev, hid);
-	if (hid->ll_driver->request || hid->hid_output_raw_report)
+	if (hid->ll_driver->request || hid->ll_driver->output_report ||
+	    hid->ll_driver->raw_request)
 		input_dev->event = hidinput_input_event;
 	input_dev->open = hidinput_open;
 	input_dev->close = hidinput_close;
-- 
1.8.3.1

Re: [PATCH 07/14] HID: input: hid-input remove hid_output_raw_report call

[David Herrmann]

Hi

On Mon, Feb 10, 2014 at 6:58 PM, Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
> hid_output_raw_report() is not a ll_driver callback and should not be used.
> To keep the same code path than before, we are forced to play with the
> different hid_hw_* calls: if the usb or i2c device does not support
> direct output reports, then we will rely on the SET_REPORT HID call.
>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
>  drivers/hid/hid-input.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> index eb00a5b..6b7bdca 100644
> --- a/drivers/hid/hid-input.c
> +++ b/drivers/hid/hid-input.c
> @@ -1153,7 +1153,7 @@ static void hidinput_led_worker(struct work_struct *work)
>                                               led_work);
>         struct hid_field *field;
>         struct hid_report *report;
> -       int len;
> +       int len, ret;
>         __u8 *buf;
>
>         field = hidinput_get_led_field(hid);
> @@ -1187,7 +1187,10 @@ static void hidinput_led_worker(struct work_struct *work)
>
>         hid_output_report(report, buf);
>         /* synchronous output report */
> -       hid_output_raw_report(hid, buf, len, HID_OUTPUT_REPORT);
> +       ret = hid_hw_output_report(hid, buf, len);
> +       if (ret == -ENOSYS)
> +               hid_hw_raw_request(hid, buf[0], buf, len, HID_OUTPUT_REPORT,
> +                               HID_REQ_SET_REPORT);

Does HID core always set the report-id in buf[0]? Even if none are
used? I know the incoming data may lack the report-id, but I always
thought we do the same for outgoing if it's implicit?

I also already see devices with broken OUTPUT_REPORTs.. I guess at
some point we have to introduce a quirk-flag to choose between both
calls. But lets wait for that to happen, maybe we're lucky.

>         kfree(buf);
>  }
>
> @@ -1266,7 +1269,8 @@ static struct hid_input *hidinput_allocate(struct hid_device *hid)
>         }
>
>         input_set_drvdata(input_dev, hid);
> -       if (hid->ll_driver->request || hid->hid_output_raw_report)
> +       if (hid->ll_driver->request || hid->ll_driver->output_report ||
> +           hid->ll_driver->raw_request)

Isn't raw_request mandatory? So we could remove that whole if() thing here.

Thanks
David

>                 input_dev->event = hidinput_input_event;
>         input_dev->open = hidinput_open;
>         input_dev->close = hidinput_close;
> --
> 1.8.3.1
>

Re: [PATCH 07/14] HID: input: hid-input remove hid_output_raw_report call

[Benjamin Tissoires]

On Wed, Feb 12, 2014 at 5:35 AM, David Herrmann <dh.herrmann@gmail.com> wrote:
> Hi
>
> On Mon, Feb 10, 2014 at 6:58 PM, Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
>> hid_output_raw_report() is not a ll_driver callback and should not be used.
>> To keep the same code path than before, we are forced to play with the
>> different hid_hw_* calls: if the usb or i2c device does not support
>> direct output reports, then we will rely on the SET_REPORT HID call.
>>
>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>> ---
>>  drivers/hid/hid-input.c | 10 +++++++---
>>  1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
>> index eb00a5b..6b7bdca 100644
>> --- a/drivers/hid/hid-input.c
>> +++ b/drivers/hid/hid-input.c
>> @@ -1153,7 +1153,7 @@ static void hidinput_led_worker(struct work_struct *work)
>>                                               led_work);
>>         struct hid_field *field;
>>         struct hid_report *report;
>> -       int len;
>> +       int len, ret;
>>         __u8 *buf;
>>
>>         field = hidinput_get_led_field(hid);
>> @@ -1187,7 +1187,10 @@ static void hidinput_led_worker(struct work_struct *work)
>>
>>         hid_output_report(report, buf);
>>         /* synchronous output report */
>> -       hid_output_raw_report(hid, buf, len, HID_OUTPUT_REPORT);
>> +       ret = hid_hw_output_report(hid, buf, len);
>> +       if (ret == -ENOSYS)
>> +               hid_hw_raw_request(hid, buf[0], buf, len, HID_OUTPUT_REPORT,
>> +                               HID_REQ_SET_REPORT);
>
> Does HID core always set the report-id in buf[0]? Even if none are
> used? I know the incoming data may lack the report-id, but I always
> thought we do the same for outgoing if it's implicit?

oh, yes, you are right. The HID spec says: "The meaning of the request
fields for the Set_Report request is the same as for the Get_Report
request, however the data direction is reversed and the Report Data is
sent from host to device. "

So this is wrong... We should use (report->id) instead of buf[0].

Will fix in v2.

>
> I also already see devices with broken OUTPUT_REPORTs.. I guess at
> some point we have to introduce a quirk-flag to choose between both
> calls. But lets wait for that to happen, maybe we're lucky.
>
>>         kfree(buf);
>>  }
>>
>> @@ -1266,7 +1269,8 @@ static struct hid_input *hidinput_allocate(struct hid_device *hid)
>>         }
>>
>>         input_set_drvdata(input_dev, hid);
>> -       if (hid->ll_driver->request || hid->hid_output_raw_report)
>> +       if (hid->ll_driver->request || hid->ll_driver->output_report ||
>> +           hid->ll_driver->raw_request)
>
> Isn't raw_request mandatory? So we could remove that whole if() thing here.

Currently, it's not (see hid_hw_raw_request). However, all the
upstream hid transport drivers implement it.

We can make it mandatory, but we should check it while adding the
device in hid_add_device.

will do for v2 too.

Cheers,
Benjamin

>
> Thanks
> David
>
>>                 input_dev->event = hidinput_input_event;
>>         input_dev->open = hidinput_open;
>>         input_dev->close = hidinput_close;
>> --
>> 1.8.3.1
>>

[PATCH 08/14] HID: logitech-dj: remove hid_output_raw_report call

[Benjamin Tissoires]

hid-input do not use anymore hid_output_raw_report() to set the LEDs.
Use the correct implementation now and make them working again.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/hid-logitech-dj.c | 21 ++++++---------------
 1 file changed, 6 insertions(+), 15 deletions(-)

diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
index 980ede5..486dbde 100644
--- a/drivers/hid/hid-logitech-dj.c
+++ b/drivers/hid/hid-logitech-dj.c
@@ -193,9 +193,6 @@ static const u8 hid_reportid_size_map[NUMBER_OF_HID_REPORTS] = {
 
 static struct hid_ll_driver logi_dj_ll_driver;
 
-static int logi_dj_output_hidraw_report(struct hid_device *hid, u8 * buf,
-					size_t count,
-					unsigned char report_type);
 static int logi_dj_recv_query_paired_devices(struct dj_receiver_dev *djrcv_dev);
 
 static void logi_dj_recv_destroy_djhid_device(struct dj_receiver_dev *djrcv_dev,
@@ -262,7 +259,6 @@ static void logi_dj_recv_add_djhid_device(struct dj_receiver_dev *djrcv_dev,
 	}
 
 	dj_hiddev->ll_driver = &logi_dj_ll_driver;
-	dj_hiddev->hid_output_raw_report = logi_dj_output_hidraw_report;
 
 	dj_hiddev->dev.parent = &djrcv_hdev->dev;
 	dj_hiddev->bus = BUS_USB;
@@ -544,9 +540,10 @@ static void logi_dj_ll_close(struct hid_device *hid)
 	dbg_hid("%s:%s\n", __func__, hid->phys);
 }
 
-static int logi_dj_output_hidraw_report(struct hid_device *hid, u8 * buf,
-					size_t count,
-					unsigned char report_type)
+static int logi_dj_ll_raw_request(struct hid_device *hid,
+				  unsigned char reportnum, __u8 *buf,
+				  size_t count, unsigned char report_type,
+				  int reqtype)
 {
 	struct dj_device *djdev = hid->driver_data;
 	struct dj_receiver_dev *djrcv_dev = djdev->dj_receiver_dev;
@@ -567,15 +564,8 @@ static int logi_dj_output_hidraw_report(struct hid_device *hid, u8 * buf,
 	out_buf[1] = djdev->device_index;
 	memcpy(out_buf + 2, buf, count);
 
-	/*
-	 * hid-generic calls us with hid_output_raw_report(), but the LEDs
-	 * are set through a SET_REPORT command. It works for USB-HID devices
-	 * because usbhid either calls a SET_REPORT or directly send the output
-	 * report depending if the device presents an urbout.
-	 * Let be simple, send a SET_REPORT request.
-	 */
 	ret = hid_hw_raw_request(djrcv_dev->hdev, out_buf[0], out_buf,
-		DJREPORT_SHORT_LENGTH, report_type, HID_REQ_SET_REPORT);
+		DJREPORT_SHORT_LENGTH, report_type, reqtype);
 
 	kfree(out_buf);
 	return ret;
@@ -662,6 +652,7 @@ static struct hid_ll_driver logi_dj_ll_driver = {
 	.stop = logi_dj_ll_stop,
 	.open = logi_dj_ll_open,
 	.close = logi_dj_ll_close,
+	.raw_request = logi_dj_ll_raw_request,
 };
 
 
-- 
1.8.3.1

Re: [PATCH 08/14] HID: logitech-dj: remove hid_output_raw_report call

[David Herrmann]

Hi

On Mon, Feb 10, 2014 at 6:58 PM, Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
> hid-input do not use anymore hid_output_raw_report() to set the LEDs.
> Use the correct implementation now and make them working again.

Looks good.

Reviewed-by: David Herrmann <dh.herrmann@gmail.com>

Thanks
David

> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
>  drivers/hid/hid-logitech-dj.c | 21 ++++++---------------
>  1 file changed, 6 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
> index 980ede5..486dbde 100644
> --- a/drivers/hid/hid-logitech-dj.c
> +++ b/drivers/hid/hid-logitech-dj.c
> @@ -193,9 +193,6 @@ static const u8 hid_reportid_size_map[NUMBER_OF_HID_REPORTS] = {
>
>  static struct hid_ll_driver logi_dj_ll_driver;
>
> -static int logi_dj_output_hidraw_report(struct hid_device *hid, u8 * buf,
> -                                       size_t count,
> -                                       unsigned char report_type);
>  static int logi_dj_recv_query_paired_devices(struct dj_receiver_dev *djrcv_dev);
>
>  static void logi_dj_recv_destroy_djhid_device(struct dj_receiver_dev *djrcv_dev,
> @@ -262,7 +259,6 @@ static void logi_dj_recv_add_djhid_device(struct dj_receiver_dev *djrcv_dev,
>         }
>
>         dj_hiddev->ll_driver = &logi_dj_ll_driver;
> -       dj_hiddev->hid_output_raw_report = logi_dj_output_hidraw_report;
>
>         dj_hiddev->dev.parent = &djrcv_hdev->dev;
>         dj_hiddev->bus = BUS_USB;
> @@ -544,9 +540,10 @@ static void logi_dj_ll_close(struct hid_device *hid)
>         dbg_hid("%s:%s\n", __func__, hid->phys);
>  }
>
> -static int logi_dj_output_hidraw_report(struct hid_device *hid, u8 * buf,
> -                                       size_t count,
> -                                       unsigned char report_type)
> +static int logi_dj_ll_raw_request(struct hid_device *hid,
> +                                 unsigned char reportnum, __u8 *buf,
> +                                 size_t count, unsigned char report_type,
> +                                 int reqtype)
>  {
>         struct dj_device *djdev = hid->driver_data;
>         struct dj_receiver_dev *djrcv_dev = djdev->dj_receiver_dev;
> @@ -567,15 +564,8 @@ static int logi_dj_output_hidraw_report(struct hid_device *hid, u8 * buf,
>         out_buf[1] = djdev->device_index;
>         memcpy(out_buf + 2, buf, count);
>
> -       /*
> -        * hid-generic calls us with hid_output_raw_report(), but the LEDs
> -        * are set through a SET_REPORT command. It works for USB-HID devices
> -        * because usbhid either calls a SET_REPORT or directly send the output
> -        * report depending if the device presents an urbout.
> -        * Let be simple, send a SET_REPORT request.
> -        */
>         ret = hid_hw_raw_request(djrcv_dev->hdev, out_buf[0], out_buf,
> -               DJREPORT_SHORT_LENGTH, report_type, HID_REQ_SET_REPORT);
> +               DJREPORT_SHORT_LENGTH, report_type, reqtype);
>
>         kfree(out_buf);
>         return ret;
> @@ -662,6 +652,7 @@ static struct hid_ll_driver logi_dj_ll_driver = {
>         .stop = logi_dj_ll_stop,
>         .open = logi_dj_ll_open,
>         .close = logi_dj_ll_close,
> +       .raw_request = logi_dj_ll_raw_request,
>  };
>
>
> --
> 1.8.3.1
>

[PATCH 09/14] HID: replace hid_output_raw_report with hid_hw_raw_request for feature requests

[Benjamin Tissoires]

  ret = hid_output_raw_report(A, B, C, HID_FEATURE_REPORT);
is equivalent to
  ret = hid_hw_raw_request(A, B[0], B, C, HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
whatever the transport layer is.

So use the new API where available

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/hid-lg.c         |  8 ++++----
 drivers/hid/hid-magicmouse.c |  4 ++--
 drivers/hid/hid-sony.c       |  4 ++--
 drivers/hid/hid-thingm.c     |  4 ++--
 drivers/hid/hid-wacom.c      | 26 +++++++++++++++-----------
 5 files changed, 25 insertions(+), 21 deletions(-)

diff --git a/drivers/hid/hid-lg.c b/drivers/hid/hid-lg.c
index 76ed7e5..a976f48 100644
--- a/drivers/hid/hid-lg.c
+++ b/drivers/hid/hid-lg.c
@@ -692,8 +692,8 @@ static int lg_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	if (hdev->product == USB_DEVICE_ID_LOGITECH_WII_WHEEL) {
 		unsigned char buf[] = { 0x00, 0xAF,  0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 };
 
-		ret = hid_output_raw_report(hdev, buf, sizeof(buf),
-					    HID_FEATURE_REPORT);
+		ret = hid_hw_raw_request(hdev, buf[0], buf, sizeof(buf),
+					HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
 
 		if (ret >= 0) {
 			/* insert a little delay of 10 jiffies ~ 40ms */
@@ -705,8 +705,8 @@ static int lg_probe(struct hid_device *hdev, const struct hid_device_id *id)
 			buf[1] = 0xB2;
 			get_random_bytes(&buf[2], 2);
 
-			ret = hid_output_raw_report(hdev, buf, sizeof(buf),
-						    HID_FEATURE_REPORT);
+			ret = hid_hw_raw_request(hdev, buf[0], buf, sizeof(buf),
+					HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
 		}
 	}
 
diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
index cb5db3a..ecc2cbf 100644
--- a/drivers/hid/hid-magicmouse.c
+++ b/drivers/hid/hid-magicmouse.c
@@ -538,8 +538,8 @@ static int magicmouse_probe(struct hid_device *hdev,
 	 * but there seems to be no other way of switching the mode.
 	 * Thus the super-ugly hacky success check below.
 	 */
-	ret = hid_output_raw_report(hdev, feature, sizeof(feature),
-			HID_FEATURE_REPORT);
+	ret = hid_hw_raw_request(hdev, feature[0], feature, sizeof(feature),
+				HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
 	if (ret != -EIO && ret != sizeof(feature)) {
 		hid_err(hdev, "unable to request touch data (%d)\n", ret);
 		goto err_stop_hw;
diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index d275c0d..d980943 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -824,8 +824,8 @@ static int sixaxis_set_operational_usb(struct hid_device *hdev)
 static int sixaxis_set_operational_bt(struct hid_device *hdev)
 {
 	unsigned char buf[] = { 0xf4,  0x42, 0x03, 0x00, 0x00 };
-	return hid_output_raw_report(hdev, buf, sizeof(buf),
-				     HID_FEATURE_REPORT);
+	return hid_hw_raw_request(hdev, buf[0], buf, sizeof(buf),
+				  HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
 }
 
 static void buzz_set_leds(struct hid_device *hdev, const __u8 *leds)
diff --git a/drivers/hid/hid-thingm.c b/drivers/hid/hid-thingm.c
index 7dd3197..a97c788 100644
--- a/drivers/hid/hid-thingm.c
+++ b/drivers/hid/hid-thingm.c
@@ -48,8 +48,8 @@ static int blink1_send_command(struct blink1_data *data,
 			buf[0], buf[1], buf[2], buf[3], buf[4],
 			buf[5], buf[6], buf[7], buf[8]);
 
-	ret = hid_output_raw_report(data->hdev, buf, BLINK1_CMD_SIZE,
-				    HID_FEATURE_REPORT);
+	ret = hid_hw_raw_request(data->hdev, buf[0], buf, BLINK1_CMD_SIZE,
+				 HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
 
 	return ret < 0 ? ret : 0;
 }
diff --git a/drivers/hid/hid-wacom.c b/drivers/hid/hid-wacom.c
index c720db9..902013e 100644
--- a/drivers/hid/hid-wacom.c
+++ b/drivers/hid/hid-wacom.c
@@ -128,7 +128,8 @@ static void wacom_set_image(struct hid_device *hdev, const char *image,
 
 	rep_data[0] = WAC_CMD_ICON_START_STOP;
 	rep_data[1] = 0;
-	ret = hid_output_raw_report(hdev, rep_data, 2, HID_FEATURE_REPORT);
+	ret = hid_hw_raw_request(hdev, rep_data[0], rep_data, 2,
+				 HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
 	if (ret < 0)
 		goto err;
 
@@ -142,14 +143,15 @@ static void wacom_set_image(struct hid_device *hdev, const char *image,
 			rep_data[j + 3] = p[(i << 6) + j];
 
 		rep_data[2] = i;
-		ret = hid_output_raw_report(hdev, rep_data, 67,
-					HID_FEATURE_REPORT);
+		ret = hid_hw_raw_request(hdev, rep_data[0], rep_data, 67,
+					HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
 	}
 
 	rep_data[0] = WAC_CMD_ICON_START_STOP;
 	rep_data[1] = 0;
 
-	ret = hid_output_raw_report(hdev, rep_data, 2, HID_FEATURE_REPORT);
+	ret = hid_hw_raw_request(hdev, rep_data[0], rep_data, 2,
+				 HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
 
 err:
 	return;
@@ -181,7 +183,8 @@ static void wacom_leds_set_brightness(struct led_classdev *led_dev,
 		buf[3] = value;
 		/* use fixed brightness for OLEDs */
 		buf[4] = 0x08;
-		hid_output_raw_report(hdev, buf, 9, HID_FEATURE_REPORT);
+		hid_hw_raw_request(hdev, buf[0], buf, 9, HID_FEATURE_REPORT,
+				   HID_REQ_SET_REPORT);
 		kfree(buf);
 	}
 
@@ -337,8 +340,8 @@ static void wacom_set_features(struct hid_device *hdev, u8 speed)
 		rep_data[0] = 0x03 ; rep_data[1] = 0x00;
 		limit = 3;
 		do {
-			ret = hid_output_raw_report(hdev, rep_data, 2,
-					HID_FEATURE_REPORT);
+			ret = hid_hw_raw_request(hdev, rep_data[0], rep_data, 2,
+					HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
 		} while (ret < 0 && limit-- > 0);
 
 		if (ret >= 0) {
@@ -350,8 +353,9 @@ static void wacom_set_features(struct hid_device *hdev, u8 speed)
 			rep_data[1] = 0x00;
 			limit = 3;
 			do {
-				ret = hid_output_raw_report(hdev,
-					rep_data, 2, HID_FEATURE_REPORT);
+				ret = hid_hw_raw_request(hdev, rep_data[0],
+					rep_data, 2, HID_FEATURE_REPORT,
+					HID_REQ_SET_REPORT);
 			} while (ret < 0 && limit-- > 0);
 
 			if (ret >= 0) {
@@ -376,8 +380,8 @@ static void wacom_set_features(struct hid_device *hdev, u8 speed)
 		rep_data[0] = 0x03;
 		rep_data[1] = wdata->features;
 
-		ret = hid_output_raw_report(hdev, rep_data, 2,
-					HID_FEATURE_REPORT);
+		ret = hid_hw_raw_request(hdev, rep_data[0], rep_data, 2,
+				HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
 		if (ret >= 0)
 			wdata->high_speed = speed;
 		break;
-- 
1.8.3.1

[PATCH 10/14] HID: wiimote: replace hid_output_raw_report with hid_hw_output_report for output requests

[Benjamin Tissoires]

For BT transport layer,
  ret = hid_output_raw_report(A, B, C, HID_OUTPUT_REPORT);
is equivalent to
  ret = hid_hw_output_report(A, B, C);

So use the new API where available

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/hid-wiimote-core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-wiimote-core.c b/drivers/hid/hid-wiimote-core.c
index d7dc6c5b..d003914 100644
--- a/drivers/hid/hid-wiimote-core.c
+++ b/drivers/hid/hid-wiimote-core.c
@@ -28,14 +28,14 @@ static int wiimote_hid_send(struct hid_device *hdev, __u8 *buffer,
 	__u8 *buf;
 	int ret;
 
-	if (!hdev->hid_output_raw_report)
+	if (!hdev->ll_driver->output_report)
 		return -ENODEV;
 
 	buf = kmemdup(buffer, count, GFP_KERNEL);
 	if (!buf)
 		return -ENOMEM;
 
-	ret = hid_output_raw_report(hdev, buf, count, HID_OUTPUT_REPORT);
+	ret = hid_hw_output_report(hdev, buf, count);
 
 	kfree(buf);
 	return ret;
-- 
1.8.3.1

Re: [PATCH 10/14] HID: wiimote: replace hid_output_raw_report with hid_hw_output_report for output requests

[David Herrmann]

Hi

On Mon, Feb 10, 2014 at 6:58 PM, Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
> For BT transport layer,
>   ret = hid_output_raw_report(A, B, C, HID_OUTPUT_REPORT);
> is equivalent to
>   ret = hid_hw_output_report(A, B, C);
>
> So use the new API where available

Reviewed-by: David Herrmann <dh.herrmann@gmail.com>

Thanks
David

>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
>  drivers/hid/hid-wiimote-core.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hid/hid-wiimote-core.c b/drivers/hid/hid-wiimote-core.c
> index d7dc6c5b..d003914 100644
> --- a/drivers/hid/hid-wiimote-core.c
> +++ b/drivers/hid/hid-wiimote-core.c
> @@ -28,14 +28,14 @@ static int wiimote_hid_send(struct hid_device *hdev, __u8 *buffer,
>         __u8 *buf;
>         int ret;
>
> -       if (!hdev->hid_output_raw_report)
> +       if (!hdev->ll_driver->output_report)
>                 return -ENODEV;
>
>         buf = kmemdup(buffer, count, GFP_KERNEL);
>         if (!buf)
>                 return -ENOMEM;
>
> -       ret = hid_output_raw_report(hdev, buf, count, HID_OUTPUT_REPORT);
> +       ret = hid_hw_output_report(hdev, buf, count);
>
>         kfree(buf);
>         return ret;
> --
> 1.8.3.1
>

[PATCH 11/14] HID: sony: remove hid_output_raw_report calls

[Benjamin Tissoires]

We can not directly change the underlying struct hid_ll_driver here
as we did for hdev->hid_output_raw_report.
So allocate a struct ll_driver, and replace the old one when removing
the device.
To get a fully functional driver, we must also split the function
sixaxis_usb_output_raw_report() in 2, one regarding output_report, and
the other for raw_request.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/hid-sony.c | 75 ++++++++++++++++++++++++++++++++++----------------
 1 file changed, 52 insertions(+), 23 deletions(-)

diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index d980943..dbbcd0c8 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -507,6 +507,8 @@ struct sony_sc {
 	struct work_struct state_worker;
 	struct power_supply battery;
 
+	struct hid_ll_driver *prev_ll_driver;
+
 #ifdef CONFIG_SONY_FF
 	__u8 left;
 	__u8 right;
@@ -760,38 +762,52 @@ static int sony_mapping(struct hid_device *hdev, struct hid_input *hi,
 
 /*
  * The Sony Sixaxis does not handle HID Output Reports on the Interrupt EP
- * like it should according to usbhid/hid-core.c::usbhid_output_raw_report()
+ * like it should according to usbhid/hid-core.c::usbhid_output_report()
  * so we need to override that forcing HID Output Reports on the Control EP.
- *
- * There is also another issue about HID Output Reports via USB, the Sixaxis
- * does not want the report_id as part of the data packet, so we have to
- * discard buf[0] when sending the actual control message, even for numbered
- * reports, humpf!
  */
-static int sixaxis_usb_output_raw_report(struct hid_device *hid, __u8 *buf,
-		size_t count, unsigned char report_type)
+static int sixaxis_usb_output_report(struct hid_device *hid, __u8 *buf,
+		size_t count)
+{
+	return hid_hw_raw_request(hid, buf[0], buf, count, HID_OUTPUT_REPORT,
+		HID_REQ_SET_REPORT);
+}
+
+/*
+ * There is also another issue about the SET_REPORT command via USB,
+ * the Sixaxis does not want the report_id as part of the data packet, so
+ * we have to discard buf[0] when sending the actual control message, even
+ * for numbered reports, humpf!
+ */
+static int sixaxis_usb_raw_request(struct hid_device *hid,
+				   unsigned char reportnum, __u8 *buf,
+				   size_t len, unsigned char rtype, int reqtype)
 {
 	struct usb_interface *intf = to_usb_interface(hid->dev.parent);
 	struct usb_device *dev = interface_to_usbdev(intf);
 	struct usb_host_interface *interface = intf->cur_altsetting;
-	int report_id = buf[0];
 	int ret;
+	u8 usb_dir;
+	unsigned int usb_pipe;
 
-	if (report_type == HID_OUTPUT_REPORT) {
+	if (reqtype == HID_REQ_SET_REPORT) {
 		/* Don't send the Report ID */
 		buf++;
-		count--;
+		len--;
+		usb_dir = USB_DIR_OUT;
+		usb_pipe = usb_sndctrlpipe(dev, 0);
+	} else {
+		usb_dir = USB_DIR_IN;
+		usb_pipe = usb_rcvctrlpipe(dev, 0);
 	}
 
-	ret = usb_control_msg(dev, usb_sndctrlpipe(dev, 0),
-		HID_REQ_SET_REPORT,
-		USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
-		((report_type + 1) << 8) | report_id,
-		interface->desc.bInterfaceNumber, buf, count,
+	ret = usb_control_msg(dev, usb_pipe, reqtype,
+		usb_dir | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
+		((rtype + 1) << 8) | reportnum,
+		interface->desc.bInterfaceNumber, buf, len,
 		USB_CTRL_SET_TIMEOUT);
 
-	/* Count also the Report ID, in case of an Output report. */
-	if (ret > 0 && report_type == HID_OUTPUT_REPORT)
+	/* Count also the Report ID. */
+	if (ret > 0 && reqtype == HID_REQ_SET_REPORT)
 		ret++;
 
 	return ret;
@@ -1047,7 +1063,7 @@ static void sixaxis_state_worker(struct work_struct *work)
 	buf[10] |= sc->led_state[2] << 3;
 	buf[10] |= sc->led_state[3] << 4;
 
-	hid_output_raw_report(sc->hdev, buf, sizeof(buf), HID_OUTPUT_REPORT);
+	hid_hw_output_report(sc->hdev, buf, sizeof(buf));
 }
 
 static void dualshock4_state_worker(struct work_struct *work)
@@ -1254,6 +1270,7 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	unsigned long quirks = id->driver_data;
 	struct sony_sc *sc;
 	unsigned int connect_mask = HID_CONNECT_DEFAULT;
+	struct hid_ll_driver *ll_driver;
 
 	sc = devm_kzalloc(&hdev->dev, sizeof(*sc), GFP_KERNEL);
 	if (sc == NULL) {
@@ -1285,13 +1302,20 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	}
 
 	if (sc->quirks & SIXAXIS_CONTROLLER_USB) {
-		hdev->hid_output_raw_report = sixaxis_usb_output_raw_report;
+		ll_driver = devm_kzalloc(&hdev->dev, sizeof(*ll_driver),
+					     GFP_KERNEL);
+		if (ll_driver == NULL)
+			return -ENOMEM;
+		sc->prev_ll_driver = hdev->ll_driver;
+		*ll_driver = *hdev->ll_driver;
+		hdev->ll_driver = ll_driver;
+		ll_driver->output_report = sixaxis_usb_output_report;
+		ll_driver->raw_request = sixaxis_usb_raw_request;
 		ret = sixaxis_set_operational_usb(hdev);
 		INIT_WORK(&sc->state_worker, sixaxis_state_worker);
-	}
-	else if (sc->quirks & SIXAXIS_CONTROLLER_BT)
+	} else if (sc->quirks & SIXAXIS_CONTROLLER_BT) {
 		ret = sixaxis_set_operational_bt(hdev);
-	else if (sc->quirks & DUALSHOCK4_CONTROLLER_USB) {
+	} else if (sc->quirks & DUALSHOCK4_CONTROLLER_USB) {
 		/* Report 5 (31 bytes) is used to send data to the controller via USB */
 		ret = sony_set_output_report(sc, 0x05, 248);
 		if (ret < 0)
@@ -1339,6 +1363,8 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
 err_close:
 	hid_hw_close(hdev);
 err_stop:
+	if (sc->prev_ll_driver)
+		hdev->ll_driver = sc->prev_ll_driver;
 	if (sc->quirks & SONY_LED_SUPPORT)
 		sony_leds_remove(hdev);
 	if (sc->quirks & SONY_BATTERY_SUPPORT)
@@ -1351,6 +1377,9 @@ static void sony_remove(struct hid_device *hdev)
 {
 	struct sony_sc *sc = hid_get_drvdata(hdev);
 
+	if (sc->prev_ll_driver)
+		hdev->ll_driver = sc->prev_ll_driver;
+
 	if (sc->quirks & SONY_LED_SUPPORT)
 		sony_leds_remove(hdev);
 
-- 
1.8.3.1

Re: [PATCH 11/14] HID: sony: remove hid_output_raw_report calls

[David Herrmann]

Hi

On Mon, Feb 10, 2014 at 6:58 PM, Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
> We can not directly change the underlying struct hid_ll_driver here
> as we did for hdev->hid_output_raw_report.
> So allocate a struct ll_driver, and replace the old one when removing
> the device.
> To get a fully functional driver, we must also split the function
> sixaxis_usb_output_raw_report() in 2, one regarding output_report, and
> the other for raw_request.

Sorry, i cannot follow here. Could you explain why this is needed? And
I also don't think you're supposed to change the ll_driver unlocked.
The real ll_driver might access it in any way. I don't think we do it
right now, but it might happen.

Why can't we introduce QUIRK flags that make HID core drop the
report_id for outgoing messages?

Thanks
David

> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
>  drivers/hid/hid-sony.c | 75 ++++++++++++++++++++++++++++++++++----------------
>  1 file changed, 52 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
> index d980943..dbbcd0c8 100644
> --- a/drivers/hid/hid-sony.c
> +++ b/drivers/hid/hid-sony.c
> @@ -507,6 +507,8 @@ struct sony_sc {
>         struct work_struct state_worker;
>         struct power_supply battery;
>
> +       struct hid_ll_driver *prev_ll_driver;
> +
>  #ifdef CONFIG_SONY_FF
>         __u8 left;
>         __u8 right;
> @@ -760,38 +762,52 @@ static int sony_mapping(struct hid_device *hdev, struct hid_input *hi,
>
>  /*
>   * The Sony Sixaxis does not handle HID Output Reports on the Interrupt EP
> - * like it should according to usbhid/hid-core.c::usbhid_output_raw_report()
> + * like it should according to usbhid/hid-core.c::usbhid_output_report()
>   * so we need to override that forcing HID Output Reports on the Control EP.
> - *
> - * There is also another issue about HID Output Reports via USB, the Sixaxis
> - * does not want the report_id as part of the data packet, so we have to
> - * discard buf[0] when sending the actual control message, even for numbered
> - * reports, humpf!
>   */
> -static int sixaxis_usb_output_raw_report(struct hid_device *hid, __u8 *buf,
> -               size_t count, unsigned char report_type)
> +static int sixaxis_usb_output_report(struct hid_device *hid, __u8 *buf,
> +               size_t count)
> +{
> +       return hid_hw_raw_request(hid, buf[0], buf, count, HID_OUTPUT_REPORT,
> +               HID_REQ_SET_REPORT);
> +}
> +
> +/*
> + * There is also another issue about the SET_REPORT command via USB,
> + * the Sixaxis does not want the report_id as part of the data packet, so
> + * we have to discard buf[0] when sending the actual control message, even
> + * for numbered reports, humpf!
> + */
> +static int sixaxis_usb_raw_request(struct hid_device *hid,
> +                                  unsigned char reportnum, __u8 *buf,
> +                                  size_t len, unsigned char rtype, int reqtype)
>  {
>         struct usb_interface *intf = to_usb_interface(hid->dev.parent);
>         struct usb_device *dev = interface_to_usbdev(intf);
>         struct usb_host_interface *interface = intf->cur_altsetting;
> -       int report_id = buf[0];
>         int ret;
> +       u8 usb_dir;
> +       unsigned int usb_pipe;
>
> -       if (report_type == HID_OUTPUT_REPORT) {
> +       if (reqtype == HID_REQ_SET_REPORT) {
>                 /* Don't send the Report ID */
>                 buf++;
> -               count--;
> +               len--;
> +               usb_dir = USB_DIR_OUT;
> +               usb_pipe = usb_sndctrlpipe(dev, 0);
> +       } else {
> +               usb_dir = USB_DIR_IN;
> +               usb_pipe = usb_rcvctrlpipe(dev, 0);
>         }
>
> -       ret = usb_control_msg(dev, usb_sndctrlpipe(dev, 0),
> -               HID_REQ_SET_REPORT,
> -               USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
> -               ((report_type + 1) << 8) | report_id,
> -               interface->desc.bInterfaceNumber, buf, count,
> +       ret = usb_control_msg(dev, usb_pipe, reqtype,
> +               usb_dir | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
> +               ((rtype + 1) << 8) | reportnum,
> +               interface->desc.bInterfaceNumber, buf, len,
>                 USB_CTRL_SET_TIMEOUT);
>
> -       /* Count also the Report ID, in case of an Output report. */
> -       if (ret > 0 && report_type == HID_OUTPUT_REPORT)
> +       /* Count also the Report ID. */
> +       if (ret > 0 && reqtype == HID_REQ_SET_REPORT)
>                 ret++;
>
>         return ret;
> @@ -1047,7 +1063,7 @@ static void sixaxis_state_worker(struct work_struct *work)
>         buf[10] |= sc->led_state[2] << 3;
>         buf[10] |= sc->led_state[3] << 4;
>
> -       hid_output_raw_report(sc->hdev, buf, sizeof(buf), HID_OUTPUT_REPORT);
> +       hid_hw_output_report(sc->hdev, buf, sizeof(buf));
>  }
>
>  static void dualshock4_state_worker(struct work_struct *work)
> @@ -1254,6 +1270,7 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
>         unsigned long quirks = id->driver_data;
>         struct sony_sc *sc;
>         unsigned int connect_mask = HID_CONNECT_DEFAULT;
> +       struct hid_ll_driver *ll_driver;
>
>         sc = devm_kzalloc(&hdev->dev, sizeof(*sc), GFP_KERNEL);
>         if (sc == NULL) {
> @@ -1285,13 +1302,20 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
>         }
>
>         if (sc->quirks & SIXAXIS_CONTROLLER_USB) {
> -               hdev->hid_output_raw_report = sixaxis_usb_output_raw_report;
> +               ll_driver = devm_kzalloc(&hdev->dev, sizeof(*ll_driver),
> +                                            GFP_KERNEL);
> +               if (ll_driver == NULL)
> +                       return -ENOMEM;
> +               sc->prev_ll_driver = hdev->ll_driver;
> +               *ll_driver = *hdev->ll_driver;
> +               hdev->ll_driver = ll_driver;
> +               ll_driver->output_report = sixaxis_usb_output_report;
> +               ll_driver->raw_request = sixaxis_usb_raw_request;
>                 ret = sixaxis_set_operational_usb(hdev);
>                 INIT_WORK(&sc->state_worker, sixaxis_state_worker);
> -       }
> -       else if (sc->quirks & SIXAXIS_CONTROLLER_BT)
> +       } else if (sc->quirks & SIXAXIS_CONTROLLER_BT) {
>                 ret = sixaxis_set_operational_bt(hdev);
> -       else if (sc->quirks & DUALSHOCK4_CONTROLLER_USB) {
> +       } else if (sc->quirks & DUALSHOCK4_CONTROLLER_USB) {
>                 /* Report 5 (31 bytes) is used to send data to the controller via USB */
>                 ret = sony_set_output_report(sc, 0x05, 248);
>                 if (ret < 0)
> @@ -1339,6 +1363,8 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  err_close:
>         hid_hw_close(hdev);
>  err_stop:
> +       if (sc->prev_ll_driver)
> +               hdev->ll_driver = sc->prev_ll_driver;
>         if (sc->quirks & SONY_LED_SUPPORT)
>                 sony_leds_remove(hdev);
>         if (sc->quirks & SONY_BATTERY_SUPPORT)
> @@ -1351,6 +1377,9 @@ static void sony_remove(struct hid_device *hdev)
>  {
>         struct sony_sc *sc = hid_get_drvdata(hdev);
>
> +       if (sc->prev_ll_driver)
> +               hdev->ll_driver = sc->prev_ll_driver;
> +
>         if (sc->quirks & SONY_LED_SUPPORT)
>                 sony_leds_remove(hdev);
>
> --
> 1.8.3.1
>

Re: [PATCH 11/14] HID: sony: remove hid_output_raw_report calls

[Benjamin Tissoires]

On Wed, Feb 12, 2014 at 5:47 AM, David Herrmann <dh.herrmann@gmail.com> wrote:
> Hi
>
> On Mon, Feb 10, 2014 at 6:58 PM, Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
>> We can not directly change the underlying struct hid_ll_driver here
>> as we did for hdev->hid_output_raw_report.
>> So allocate a struct ll_driver, and replace the old one when removing
>> the device.
>> To get a fully functional driver, we must also split the function
>> sixaxis_usb_output_raw_report() in 2, one regarding output_report, and
>> the other for raw_request.
>
> Sorry, i cannot follow here. Could you explain why this is needed? And

Well, as mentioned in the original comments, the sixaxis has two problem:
- when you send an output report, you should call hid_hw_raw_request
instead (though there is an interrupt output endpoint)
- when you send a raw_request, you should drop the reportID in the
buffer you are sending, but keep it in the SET_REPORT call... (quite
annoying).

Splitting this in two (it was all implemented in hid_output_report)
allows to transparently use the hid_hw_call, not matter who calls
what.

> I also don't think you're supposed to change the ll_driver unlocked.
> The real ll_driver might access it in any way. I don't think we do it
> right now, but it might happen.

Yeah, I am not a big fan of this either. My other concern regarding
that is the dependency against usb, while it could be a uHID device.

>
> Why can't we introduce QUIRK flags that make HID core drop the
> report_id for outgoing messages?

I did not want to come with because it would make the bright new
ll_transport interface ugly, but this may be the only way to have
something generic and drop the usb dependency and the races that may
appears (plus the ugly alloc/free).

Cheers,
Benjamin

>
> Thanks
> David
>
>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>> ---
>>  drivers/hid/hid-sony.c | 75 ++++++++++++++++++++++++++++++++++----------------
>>  1 file changed, 52 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
>> index d980943..dbbcd0c8 100644
>> --- a/drivers/hid/hid-sony.c
>> +++ b/drivers/hid/hid-sony.c
>> @@ -507,6 +507,8 @@ struct sony_sc {
>>         struct work_struct state_worker;
>>         struct power_supply battery;
>>
>> +       struct hid_ll_driver *prev_ll_driver;
>> +
>>  #ifdef CONFIG_SONY_FF
>>         __u8 left;
>>         __u8 right;
>> @@ -760,38 +762,52 @@ static int sony_mapping(struct hid_device *hdev, struct hid_input *hi,
>>
>>  /*
>>   * The Sony Sixaxis does not handle HID Output Reports on the Interrupt EP
>> - * like it should according to usbhid/hid-core.c::usbhid_output_raw_report()
>> + * like it should according to usbhid/hid-core.c::usbhid_output_report()
>>   * so we need to override that forcing HID Output Reports on the Control EP.
>> - *
>> - * There is also another issue about HID Output Reports via USB, the Sixaxis
>> - * does not want the report_id as part of the data packet, so we have to
>> - * discard buf[0] when sending the actual control message, even for numbered
>> - * reports, humpf!
>>   */
>> -static int sixaxis_usb_output_raw_report(struct hid_device *hid, __u8 *buf,
>> -               size_t count, unsigned char report_type)
>> +static int sixaxis_usb_output_report(struct hid_device *hid, __u8 *buf,
>> +               size_t count)
>> +{
>> +       return hid_hw_raw_request(hid, buf[0], buf, count, HID_OUTPUT_REPORT,
>> +               HID_REQ_SET_REPORT);
>> +}
>> +
>> +/*
>> + * There is also another issue about the SET_REPORT command via USB,
>> + * the Sixaxis does not want the report_id as part of the data packet, so
>> + * we have to discard buf[0] when sending the actual control message, even
>> + * for numbered reports, humpf!
>> + */
>> +static int sixaxis_usb_raw_request(struct hid_device *hid,
>> +                                  unsigned char reportnum, __u8 *buf,
>> +                                  size_t len, unsigned char rtype, int reqtype)
>>  {
>>         struct usb_interface *intf = to_usb_interface(hid->dev.parent);
>>         struct usb_device *dev = interface_to_usbdev(intf);
>>         struct usb_host_interface *interface = intf->cur_altsetting;
>> -       int report_id = buf[0];
>>         int ret;
>> +       u8 usb_dir;
>> +       unsigned int usb_pipe;
>>
>> -       if (report_type == HID_OUTPUT_REPORT) {
>> +       if (reqtype == HID_REQ_SET_REPORT) {
>>                 /* Don't send the Report ID */
>>                 buf++;
>> -               count--;
>> +               len--;
>> +               usb_dir = USB_DIR_OUT;
>> +               usb_pipe = usb_sndctrlpipe(dev, 0);
>> +       } else {
>> +               usb_dir = USB_DIR_IN;
>> +               usb_pipe = usb_rcvctrlpipe(dev, 0);
>>         }
>>
>> -       ret = usb_control_msg(dev, usb_sndctrlpipe(dev, 0),
>> -               HID_REQ_SET_REPORT,
>> -               USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
>> -               ((report_type + 1) << 8) | report_id,
>> -               interface->desc.bInterfaceNumber, buf, count,
>> +       ret = usb_control_msg(dev, usb_pipe, reqtype,
>> +               usb_dir | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
>> +               ((rtype + 1) << 8) | reportnum,
>> +               interface->desc.bInterfaceNumber, buf, len,
>>                 USB_CTRL_SET_TIMEOUT);
>>
>> -       /* Count also the Report ID, in case of an Output report. */
>> -       if (ret > 0 && report_type == HID_OUTPUT_REPORT)
>> +       /* Count also the Report ID. */
>> +       if (ret > 0 && reqtype == HID_REQ_SET_REPORT)
>>                 ret++;
>>
>>         return ret;
>> @@ -1047,7 +1063,7 @@ static void sixaxis_state_worker(struct work_struct *work)
>>         buf[10] |= sc->led_state[2] << 3;
>>         buf[10] |= sc->led_state[3] << 4;
>>
>> -       hid_output_raw_report(sc->hdev, buf, sizeof(buf), HID_OUTPUT_REPORT);
>> +       hid_hw_output_report(sc->hdev, buf, sizeof(buf));
>>  }
>>
>>  static void dualshock4_state_worker(struct work_struct *work)
>> @@ -1254,6 +1270,7 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
>>         unsigned long quirks = id->driver_data;
>>         struct sony_sc *sc;
>>         unsigned int connect_mask = HID_CONNECT_DEFAULT;
>> +       struct hid_ll_driver *ll_driver;
>>
>>         sc = devm_kzalloc(&hdev->dev, sizeof(*sc), GFP_KERNEL);
>>         if (sc == NULL) {
>> @@ -1285,13 +1302,20 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
>>         }
>>
>>         if (sc->quirks & SIXAXIS_CONTROLLER_USB) {
>> -               hdev->hid_output_raw_report = sixaxis_usb_output_raw_report;
>> +               ll_driver = devm_kzalloc(&hdev->dev, sizeof(*ll_driver),
>> +                                            GFP_KERNEL);
>> +               if (ll_driver == NULL)
>> +                       return -ENOMEM;
>> +               sc->prev_ll_driver = hdev->ll_driver;
>> +               *ll_driver = *hdev->ll_driver;
>> +               hdev->ll_driver = ll_driver;
>> +               ll_driver->output_report = sixaxis_usb_output_report;
>> +               ll_driver->raw_request = sixaxis_usb_raw_request;
>>                 ret = sixaxis_set_operational_usb(hdev);
>>                 INIT_WORK(&sc->state_worker, sixaxis_state_worker);
>> -       }
>> -       else if (sc->quirks & SIXAXIS_CONTROLLER_BT)
>> +       } else if (sc->quirks & SIXAXIS_CONTROLLER_BT) {
>>                 ret = sixaxis_set_operational_bt(hdev);
>> -       else if (sc->quirks & DUALSHOCK4_CONTROLLER_USB) {
>> +       } else if (sc->quirks & DUALSHOCK4_CONTROLLER_USB) {
>>                 /* Report 5 (31 bytes) is used to send data to the controller via USB */
>>                 ret = sony_set_output_report(sc, 0x05, 248);
>>                 if (ret < 0)
>> @@ -1339,6 +1363,8 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
>>  err_close:
>>         hid_hw_close(hdev);
>>  err_stop:
>> +       if (sc->prev_ll_driver)
>> +               hdev->ll_driver = sc->prev_ll_driver;
>>         if (sc->quirks & SONY_LED_SUPPORT)
>>                 sony_leds_remove(hdev);
>>         if (sc->quirks & SONY_BATTERY_SUPPORT)
>> @@ -1351,6 +1377,9 @@ static void sony_remove(struct hid_device *hdev)
>>  {
>>         struct sony_sc *sc = hid_get_drvdata(hdev);
>>
>> +       if (sc->prev_ll_driver)
>> +               hdev->ll_driver = sc->prev_ll_driver;
>> +
>>         if (sc->quirks & SONY_LED_SUPPORT)
>>                 sony_leds_remove(hdev);
>>
>> --
>> 1.8.3.1
>>

[PATCH 12/14] HID: hidraw: replace hid_output_raw_report() calls by appropriates ones

[Benjamin Tissoires]

Remove hid_output_raw_report() call as it is not a ll_driver callbacj,
and switch to the hid_hw_* implementation. USB-HID used to fallback
into SET_REPORT when there were no output interrupt endpoint,
so emulating this if hid_hw_output_report() returns -ENOSYS.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/hidraw.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/hid/hidraw.c b/drivers/hid/hidraw.c
index f8708c9..704718b 100644
--- a/drivers/hid/hidraw.c
+++ b/drivers/hid/hidraw.c
@@ -123,10 +123,8 @@ static ssize_t hidraw_send_report(struct file *file, const char __user *buffer,
 
 	dev = hidraw_table[minor]->hid;
 
-	if (!dev->hid_output_raw_report) {
-		ret = -ENODEV;
-		goto out;
-	}
+	if (!dev->ll_driver->raw_request || !dev->ll_driver->output_report)
+		return -ENODEV;
 
 	if (count > HID_MAX_BUFFER_SIZE) {
 		hid_warn(dev, "pid %d passed too large report\n",
@@ -153,7 +151,21 @@ static ssize_t hidraw_send_report(struct file *file, const char __user *buffer,
 		goto out_free;
 	}
 
-	ret = hid_output_raw_report(dev, buf, count, report_type);
+	if (report_type == HID_OUTPUT_REPORT) {
+		ret = hid_hw_output_report(dev, buf, count);
+		/*
+		 * compatibility with old implementation of USB-HID and I2C-HID:
+		 * if the device does not support receiving output reports,
+		 * on an interrupt endpoint, then fallback to the SET_REPORT
+		 * HID command.
+		 */
+		if (ret != -ENOSYS)
+			goto out_free;
+	}
+
+	ret = hid_hw_raw_request(dev, buf[0], buf, count, report_type,
+				HID_REQ_SET_REPORT);
+
 out_free:
 	kfree(buf);
 out:
-- 
1.8.3.1

Re: [PATCH 12/14] HID: hidraw: replace hid_output_raw_report() calls by appropriates ones

[David Herrmann]

Hi

On Mon, Feb 10, 2014 at 6:58 PM, Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
> Remove hid_output_raw_report() call as it is not a ll_driver callbacj,
> and switch to the hid_hw_* implementation. USB-HID used to fallback
> into SET_REPORT when there were no output interrupt endpoint,
> so emulating this if hid_hw_output_report() returns -ENOSYS.
>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
>  drivers/hid/hidraw.c | 22 +++++++++++++++++-----
>  1 file changed, 17 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/hid/hidraw.c b/drivers/hid/hidraw.c
> index f8708c9..704718b 100644
> --- a/drivers/hid/hidraw.c
> +++ b/drivers/hid/hidraw.c
> @@ -123,10 +123,8 @@ static ssize_t hidraw_send_report(struct file *file, const char __user *buffer,
>
>         dev = hidraw_table[minor]->hid;
>
> -       if (!dev->hid_output_raw_report) {
> -               ret = -ENODEV;
> -               goto out;
> -       }
> +       if (!dev->ll_driver->raw_request || !dev->ll_driver->output_report)
> +               return -ENODEV;

You still have a "goto out;" above (not visible in the patch). The
error paths look quite ugly now. I'd prefer consistency here, so
either keep the jump or fix the error-path above, too. Otherwise looks
good.

Reviewed-by: David Herrmann <dh.herrmann@gmail.com>

Thanks
David

>
>         if (count > HID_MAX_BUFFER_SIZE) {
>                 hid_warn(dev, "pid %d passed too large report\n",
> @@ -153,7 +151,21 @@ static ssize_t hidraw_send_report(struct file *file, const char __user *buffer,
>                 goto out_free;
>         }
>
> -       ret = hid_output_raw_report(dev, buf, count, report_type);
> +       if (report_type == HID_OUTPUT_REPORT) {
> +               ret = hid_hw_output_report(dev, buf, count);
> +               /*
> +                * compatibility with old implementation of USB-HID and I2C-HID:
> +                * if the device does not support receiving output reports,
> +                * on an interrupt endpoint, then fallback to the SET_REPORT
> +                * HID command.
> +                */
> +               if (ret != -ENOSYS)
> +                       goto out_free;
> +       }
> +
> +       ret = hid_hw_raw_request(dev, buf[0], buf, count, report_type,
> +                               HID_REQ_SET_REPORT);
> +
>  out_free:
>         kfree(buf);
>  out:
> --
> 1.8.3.1
>

Re: [PATCH 12/14] HID: hidraw: replace hid_output_raw_report() calls by appropriates ones

[Benjamin Tissoires]

On Wed, Feb 12, 2014 at 5:49 AM, David Herrmann <dh.herrmann@gmail.com> wrote:
> Hi
>
> On Mon, Feb 10, 2014 at 6:58 PM, Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
>> Remove hid_output_raw_report() call as it is not a ll_driver callbacj,
>> and switch to the hid_hw_* implementation. USB-HID used to fallback
>> into SET_REPORT when there were no output interrupt endpoint,
>> so emulating this if hid_hw_output_report() returns -ENOSYS.
>>
>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>> ---
>>  drivers/hid/hidraw.c | 22 +++++++++++++++++-----
>>  1 file changed, 17 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/hid/hidraw.c b/drivers/hid/hidraw.c
>> index f8708c9..704718b 100644
>> --- a/drivers/hid/hidraw.c
>> +++ b/drivers/hid/hidraw.c
>> @@ -123,10 +123,8 @@ static ssize_t hidraw_send_report(struct file *file, const char __user *buffer,
>>
>>         dev = hidraw_table[minor]->hid;
>>
>> -       if (!dev->hid_output_raw_report) {
>> -               ret = -ENODEV;
>> -               goto out;
>> -       }
>> +       if (!dev->ll_driver->raw_request || !dev->ll_driver->output_report)
>> +               return -ENODEV;
>
> You still have a "goto out;" above (not visible in the patch). The
> error paths look quite ugly now. I'd prefer consistency here, so
> either keep the jump or fix the error-path above, too. Otherwise looks
> good.

ok, will use "goto out" in the v2 then.

Cheers,
Benjamin

>
> Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
>
> Thanks
> David
>
>>
>>         if (count > HID_MAX_BUFFER_SIZE) {
>>                 hid_warn(dev, "pid %d passed too large report\n",
>> @@ -153,7 +151,21 @@ static ssize_t hidraw_send_report(struct file *file, const char __user *buffer,
>>                 goto out_free;
>>         }
>>
>> -       ret = hid_output_raw_report(dev, buf, count, report_type);
>> +       if (report_type == HID_OUTPUT_REPORT) {
>> +               ret = hid_hw_output_report(dev, buf, count);
>> +               /*
>> +                * compatibility with old implementation of USB-HID and I2C-HID:
>> +                * if the device does not support receiving output reports,
>> +                * on an interrupt endpoint, then fallback to the SET_REPORT
>> +                * HID command.
>> +                */
>> +               if (ret != -ENOSYS)
>> +                       goto out_free;
>> +       }
>> +
>> +       ret = hid_hw_raw_request(dev, buf[0], buf, count, report_type,
>> +                               HID_REQ_SET_REPORT);
>> +
>>  out_free:
>>         kfree(buf);
>>  out:
>> --
>> 1.8.3.1
>>

[PATCH 13/14] HID: remove hid_output_raw_report

[Benjamin Tissoires]

hid_output_raw_report() is not used anymore, delete it.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/i2c-hid/i2c-hid.c | 14 --------------
 drivers/hid/uhid.c            |  1 -
 drivers/hid/usbhid/hid-core.c | 12 ------------
 include/linux/hid.h           | 19 -------------------
 net/bluetooth/hidp/core.c     | 14 --------------
 5 files changed, 60 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index 925fb8d..d3b8d7a 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c
@@ -596,19 +596,6 @@ static int i2c_hid_output_raw_report(struct hid_device *hid, __u8 *buf,
 	return ret;
 }
 
-static int __i2c_hid_output_raw_report(struct hid_device *hid, __u8 *buf,
-		size_t count, unsigned char report_type)
-{
-	struct i2c_client *client = hid->driver_data;
-	struct i2c_hid *ihid = i2c_get_clientdata(client);
-	bool data = true; /* SET_REPORT */
-
-	if (report_type == HID_OUTPUT_REPORT)
-		data = le16_to_cpu(ihid->hdesc.wMaxOutputLength) == 0;
-
-	return i2c_hid_output_raw_report(hid, buf, count, report_type, data);
-}
-
 static int i2c_hid_output_report(struct hid_device *hid, __u8 *buf,
 		size_t count)
 {
@@ -1023,7 +1010,6 @@ static int i2c_hid_probe(struct i2c_client *client,
 
 	hid->driver_data = client;
 	hid->ll_driver = &i2c_hid_ll_driver;
-	hid->hid_output_raw_report = __i2c_hid_output_raw_report;
 	hid->dev.parent = &client->dev;
 	ACPI_COMPANION_SET(&hid->dev, ACPI_COMPANION(&client->dev));
 	hid->bus = BUS_I2C;
diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
index 60acee4..7ed79be 100644
--- a/drivers/hid/uhid.c
+++ b/drivers/hid/uhid.c
@@ -400,7 +400,6 @@ static int uhid_dev_create(struct uhid_device *uhid,
 	hid->uniq[63] = 0;
 
 	hid->ll_driver = &uhid_hid_driver;
-	hid->hid_output_raw_report = uhid_hid_output_raw;
 	hid->bus = ev->u.create.bus;
 	hid->vendor = ev->u.create.vendor;
 	hid->product = ev->u.create.product;
diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index 0d1d875..02b3256 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -945,17 +945,6 @@ static int usbhid_output_report(struct hid_device *hid, __u8 *buf, size_t count)
 	return ret;
 }
 
-static int usbhid_output_raw_report(struct hid_device *hid, __u8 *buf,
-		size_t count, unsigned char report_type)
-{
-	struct usbhid_device *usbhid = hid->driver_data;
-
-	if (usbhid->urbout && report_type != HID_FEATURE_REPORT)
-		return usbhid_output_report(hid, buf, count);
-
-	return usbhid_set_raw_report(hid, buf[0], buf, count, report_type);
-}
-
 static void usbhid_restart_queues(struct usbhid_device *usbhid)
 {
 	if (usbhid->urbout && !test_bit(HID_OUT_RUNNING, &usbhid->iofl))
@@ -1289,7 +1278,6 @@ static int usbhid_probe(struct usb_interface *intf, const struct usb_device_id *
 
 	usb_set_intfdata(intf, hid);
 	hid->ll_driver = &usb_hid_driver;
-	hid->hid_output_raw_report = usbhid_output_raw_report;
 	hid->ff_init = hid_pidff_init;
 #ifdef CONFIG_USB_HIDDEV
 	hid->hiddev_connect = hiddev_connect;
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 09fbbd7..fa07639 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -508,9 +508,6 @@ struct hid_device {							/* device report descriptor */
 				  struct hid_usage *, __s32);
 	void (*hiddev_report_event) (struct hid_device *, struct hid_report *);
 
-	/* handler for raw output data, used by hidraw */
-	int (*hid_output_raw_report) (struct hid_device *, __u8 *, size_t, unsigned char);
-
 	/* debugging support via debugfs */
 	unsigned short debug;
 	struct dentry *debug_dir;
@@ -1015,22 +1012,6 @@ static inline int hid_hw_output_report(struct hid_device *hdev, __u8 *buf,
 }
 
 /**
- * hid_output_raw_report - send an output or a feature report to the device
- *
- * @hdev: hid device
- * @buf: raw data to transfer
- * @len: length of buf
- * @report_type: HID_FEATURE_REPORT or HID_OUTPUT_REPORT
- *
- * @return: count of data transfered, negative if error
- */
-static inline int hid_output_raw_report(struct hid_device *hdev, __u8 *buf,
-					size_t len, unsigned char report_type)
-{
-	return hdev->hid_output_raw_report(hdev, buf, len, report_type);
-}
-
-/**
  * hid_hw_idle - send idle request to device
  *
  * @hdev: hid device
diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index 77c4bad..89da18d 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -382,18 +382,6 @@ static int hidp_output_report(struct hid_device *hid, __u8 *data, size_t count)
 				      data, count);
 }
 
-static int hidp_output_raw_report(struct hid_device *hid, unsigned char *data,
-		size_t count, unsigned char report_type)
-{
-	if (report_type == HID_OUTPUT_REPORT) {
-		return hidp_output_report(hid, data, count);
-	} else if (report_type != HID_FEATURE_REPORT) {
-		return -EINVAL;
-	}
-
-	return hidp_set_raw_report(hid, data[0], data, count, report_type);
-}
-
 static int hidp_raw_request(struct hid_device *hid, unsigned char reportnum,
 			    __u8 *buf, size_t len, unsigned char rtype,
 			    int reqtype)
@@ -773,8 +761,6 @@ static int hidp_setup_hid(struct hidp_session *session,
 	hid->dev.parent = &session->conn->hcon->dev;
 	hid->ll_driver = &hidp_hid_driver;
 
-	hid->hid_output_raw_report = hidp_output_raw_report;
-
 	/* True if device is blacklisted in drivers/hid/hid-core.c */
 	if (hid_ignore(hid)) {
 		hid_destroy_device(session->hid);
-- 
1.8.3.1

Re: [PATCH 13/14] HID: remove hid_output_raw_report

[David Herrmann]

Hi

On Mon, Feb 10, 2014 at 6:58 PM, Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
> hid_output_raw_report() is not used anymore, delete it.

yippieh!

Reviewed-by: David Herrmann <dh.herrmann@gmail.com>

Thanks
David

> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
>  drivers/hid/i2c-hid/i2c-hid.c | 14 --------------
>  drivers/hid/uhid.c            |  1 -
>  drivers/hid/usbhid/hid-core.c | 12 ------------
>  include/linux/hid.h           | 19 -------------------
>  net/bluetooth/hidp/core.c     | 14 --------------
>  5 files changed, 60 deletions(-)
>
> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> index 925fb8d..d3b8d7a 100644
> --- a/drivers/hid/i2c-hid/i2c-hid.c
> +++ b/drivers/hid/i2c-hid/i2c-hid.c
> @@ -596,19 +596,6 @@ static int i2c_hid_output_raw_report(struct hid_device *hid, __u8 *buf,
>         return ret;
>  }
>
> -static int __i2c_hid_output_raw_report(struct hid_device *hid, __u8 *buf,
> -               size_t count, unsigned char report_type)
> -{
> -       struct i2c_client *client = hid->driver_data;
> -       struct i2c_hid *ihid = i2c_get_clientdata(client);
> -       bool data = true; /* SET_REPORT */
> -
> -       if (report_type == HID_OUTPUT_REPORT)
> -               data = le16_to_cpu(ihid->hdesc.wMaxOutputLength) == 0;
> -
> -       return i2c_hid_output_raw_report(hid, buf, count, report_type, data);
> -}
> -
>  static int i2c_hid_output_report(struct hid_device *hid, __u8 *buf,
>                 size_t count)
>  {
> @@ -1023,7 +1010,6 @@ static int i2c_hid_probe(struct i2c_client *client,
>
>         hid->driver_data = client;
>         hid->ll_driver = &i2c_hid_ll_driver;
> -       hid->hid_output_raw_report = __i2c_hid_output_raw_report;
>         hid->dev.parent = &client->dev;
>         ACPI_COMPANION_SET(&hid->dev, ACPI_COMPANION(&client->dev));
>         hid->bus = BUS_I2C;
> diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
> index 60acee4..7ed79be 100644
> --- a/drivers/hid/uhid.c
> +++ b/drivers/hid/uhid.c
> @@ -400,7 +400,6 @@ static int uhid_dev_create(struct uhid_device *uhid,
>         hid->uniq[63] = 0;
>
>         hid->ll_driver = &uhid_hid_driver;
> -       hid->hid_output_raw_report = uhid_hid_output_raw;
>         hid->bus = ev->u.create.bus;
>         hid->vendor = ev->u.create.vendor;
>         hid->product = ev->u.create.product;
> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> index 0d1d875..02b3256 100644
> --- a/drivers/hid/usbhid/hid-core.c
> +++ b/drivers/hid/usbhid/hid-core.c
> @@ -945,17 +945,6 @@ static int usbhid_output_report(struct hid_device *hid, __u8 *buf, size_t count)
>         return ret;
>  }
>
> -static int usbhid_output_raw_report(struct hid_device *hid, __u8 *buf,
> -               size_t count, unsigned char report_type)
> -{
> -       struct usbhid_device *usbhid = hid->driver_data;
> -
> -       if (usbhid->urbout && report_type != HID_FEATURE_REPORT)
> -               return usbhid_output_report(hid, buf, count);
> -
> -       return usbhid_set_raw_report(hid, buf[0], buf, count, report_type);
> -}
> -
>  static void usbhid_restart_queues(struct usbhid_device *usbhid)
>  {
>         if (usbhid->urbout && !test_bit(HID_OUT_RUNNING, &usbhid->iofl))
> @@ -1289,7 +1278,6 @@ static int usbhid_probe(struct usb_interface *intf, const struct usb_device_id *
>
>         usb_set_intfdata(intf, hid);
>         hid->ll_driver = &usb_hid_driver;
> -       hid->hid_output_raw_report = usbhid_output_raw_report;
>         hid->ff_init = hid_pidff_init;
>  #ifdef CONFIG_USB_HIDDEV
>         hid->hiddev_connect = hiddev_connect;
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index 09fbbd7..fa07639 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -508,9 +508,6 @@ struct hid_device {                                                 /* device report descriptor */
>                                   struct hid_usage *, __s32);
>         void (*hiddev_report_event) (struct hid_device *, struct hid_report *);
>
> -       /* handler for raw output data, used by hidraw */
> -       int (*hid_output_raw_report) (struct hid_device *, __u8 *, size_t, unsigned char);
> -
>         /* debugging support via debugfs */
>         unsigned short debug;
>         struct dentry *debug_dir;
> @@ -1015,22 +1012,6 @@ static inline int hid_hw_output_report(struct hid_device *hdev, __u8 *buf,
>  }
>
>  /**
> - * hid_output_raw_report - send an output or a feature report to the device
> - *
> - * @hdev: hid device
> - * @buf: raw data to transfer
> - * @len: length of buf
> - * @report_type: HID_FEATURE_REPORT or HID_OUTPUT_REPORT
> - *
> - * @return: count of data transfered, negative if error
> - */
> -static inline int hid_output_raw_report(struct hid_device *hdev, __u8 *buf,
> -                                       size_t len, unsigned char report_type)
> -{
> -       return hdev->hid_output_raw_report(hdev, buf, len, report_type);
> -}
> -
> -/**
>   * hid_hw_idle - send idle request to device
>   *
>   * @hdev: hid device
> diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
> index 77c4bad..89da18d 100644
> --- a/net/bluetooth/hidp/core.c
> +++ b/net/bluetooth/hidp/core.c
> @@ -382,18 +382,6 @@ static int hidp_output_report(struct hid_device *hid, __u8 *data, size_t count)
>                                       data, count);
>  }
>
> -static int hidp_output_raw_report(struct hid_device *hid, unsigned char *data,
> -               size_t count, unsigned char report_type)
> -{
> -       if (report_type == HID_OUTPUT_REPORT) {
> -               return hidp_output_report(hid, data, count);
> -       } else if (report_type != HID_FEATURE_REPORT) {
> -               return -EINVAL;
> -       }
> -
> -       return hidp_set_raw_report(hid, data[0], data, count, report_type);
> -}
> -
>  static int hidp_raw_request(struct hid_device *hid, unsigned char reportnum,
>                             __u8 *buf, size_t len, unsigned char rtype,
>                             int reqtype)
> @@ -773,8 +761,6 @@ static int hidp_setup_hid(struct hidp_session *session,
>         hid->dev.parent = &session->conn->hcon->dev;
>         hid->ll_driver = &hidp_hid_driver;
>
> -       hid->hid_output_raw_report = hidp_output_raw_report;
> -
>         /* True if device is blacklisted in drivers/hid/hid-core.c */
>         if (hid_ignore(hid)) {
>                 hid_destroy_device(session->hid);
> --
> 1.8.3.1
>

[PATCH 14/14] HID: core: check parameters when sending/receiving data from the device

[Benjamin Tissoires]

It is better to check them soon enough before triggering any kernel panic.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/i2c-hid/i2c-hid.c | 2 +-
 include/linux/hid.h           | 6 ++++++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index d3b8d7a..b50860d 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c
@@ -277,7 +277,7 @@ static int i2c_hid_set_or_send_report(struct i2c_client *client, u8 reportType,
 	u16 outputRegister = le16_to_cpu(ihid->hdesc.wOutputRegister);
 	u16 maxOutputLength = le16_to_cpu(ihid->hdesc.wMaxOutputLength);
 
-	/* hidraw already checked that data_len < HID_MAX_BUFFER_SIZE */
+	/* hid_hw_* already checked that data_len < HID_MAX_BUFFER_SIZE */
 	u16 size =	2			/* size */ +
 			(reportID ? 1 : 0)	/* reportID */ +
 			data_len		/* buf */;
diff --git a/include/linux/hid.h b/include/linux/hid.h
index fa07639..f801506 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -986,6 +986,9 @@ static inline int hid_hw_raw_request(struct hid_device *hdev,
 				  unsigned char reportnum, __u8 *buf,
 				  size_t len, unsigned char rtype, int reqtype)
 {
+	if (len < 1 || len > HID_MAX_BUFFER_SIZE || !buf)
+		return -EINVAL;
+
 	if (hdev->ll_driver->raw_request)
 		return hdev->ll_driver->raw_request(hdev, reportnum, buf, len,
 						    rtype, reqtype);
@@ -1005,6 +1008,9 @@ static inline int hid_hw_raw_request(struct hid_device *hdev,
 static inline int hid_hw_output_report(struct hid_device *hdev, __u8 *buf,
 					size_t len)
 {
+	if (len < 1 || len > HID_MAX_BUFFER_SIZE || !buf)
+		return -EINVAL;
+
 	if (hdev->ll_driver->output_report)
 		return hdev->ll_driver->output_report(hdev, buf, len);
 
-- 
1.8.3.1

Re: [PATCH 14/14] HID: core: check parameters when sending/receiving data from the device

[David Herrmann]

Hi

On Mon, Feb 10, 2014 at 6:58 PM, Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
> It is better to check them soon enough before triggering any kernel panic.

Reviewed-by: David Herrmann <dh.herrmann@gmail.com>

Thanks
David

> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
>  drivers/hid/i2c-hid/i2c-hid.c | 2 +-
>  include/linux/hid.h           | 6 ++++++
>  2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> index d3b8d7a..b50860d 100644
> --- a/drivers/hid/i2c-hid/i2c-hid.c
> +++ b/drivers/hid/i2c-hid/i2c-hid.c
> @@ -277,7 +277,7 @@ static int i2c_hid_set_or_send_report(struct i2c_client *client, u8 reportType,
>         u16 outputRegister = le16_to_cpu(ihid->hdesc.wOutputRegister);
>         u16 maxOutputLength = le16_to_cpu(ihid->hdesc.wMaxOutputLength);
>
> -       /* hidraw already checked that data_len < HID_MAX_BUFFER_SIZE */
> +       /* hid_hw_* already checked that data_len < HID_MAX_BUFFER_SIZE */
>         u16 size =      2                       /* size */ +
>                         (reportID ? 1 : 0)      /* reportID */ +
>                         data_len                /* buf */;
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index fa07639..f801506 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -986,6 +986,9 @@ static inline int hid_hw_raw_request(struct hid_device *hdev,
>                                   unsigned char reportnum, __u8 *buf,
>                                   size_t len, unsigned char rtype, int reqtype)
>  {
> +       if (len < 1 || len > HID_MAX_BUFFER_SIZE || !buf)
> +               return -EINVAL;
> +
>         if (hdev->ll_driver->raw_request)
>                 return hdev->ll_driver->raw_request(hdev, reportnum, buf, len,
>                                                     rtype, reqtype);
> @@ -1005,6 +1008,9 @@ static inline int hid_hw_raw_request(struct hid_device *hdev,
>  static inline int hid_hw_output_report(struct hid_device *hdev, __u8 *buf,
>                                         size_t len)
>  {
> +       if (len < 1 || len > HID_MAX_BUFFER_SIZE || !buf)
> +               return -EINVAL;
> +
>         if (hdev->ll_driver->output_report)
>                 return hdev->ll_driver->output_report(hdev, buf, len);
>
> --
> 1.8.3.1
>

Re: [PATCH 00/14] HID: low-level transport cleanup, round 2

[Jiri Kosina]

On Mon, 10 Feb 2014, Benjamin Tissoires wrote:

> this is the second part of the low-level HID transport cleanup.
> The series goes on top of the previous one I sent last week.

Thanks!

I have applied all but patches 7,11,12 (and 13 as a natural followup). I 
am waiting for v2 of 7 and 12, and I want to think a little bit more about 
11.

-- 
Jiri Kosina
SUSE Labs