[PATCH 0/3] HID: low level transport cleanup, series 3

[PATCH 0/3] HID: low level transport cleanup, series 3

[Benjamin Tissoires]

Hi guys,

this is a new round in the low level HID transport cleanup.
These are the formely 7/14 an 12/14 only.
I add in between a patch which makes .raw_request mandatory, to cleanup
the old 12/14 (so 3/3 here).

There is only two patches 2 merge after these 3... We are approaching the end!

Cheers,
Benjamin

Benjamin Tissoires (3):
  HID: input: hid-input remove hid_output_raw_report call
  HID: make .raw_request mandatory
  HID: hidraw: replace hid_output_raw_report() calls by appropriates
    ones

 Documentation/hid/hid-transport.txt |  3 ++-
 drivers/hid/hid-core.c              | 11 ++++++++---
 drivers/hid/hid-input.c             | 10 ++++++----
 drivers/hid/hidraw.c                | 19 ++++++++++++++-----
 include/linux/hid.h                 |  5 +----
 5 files changed, 31 insertions(+), 17 deletions(-)

-- 
1.8.5.3

[PATCH v2 1/3] 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>
---

ex 7/14

changes since last version:
- replace buf[0] by report->id to prevent a bug with devices not having a report ID.

 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..e3d625a 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, report->id, 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.5.3

[PATCH v2 2/3] HID: make .raw_request mandatory

[Benjamin Tissoires]

SET_REPORT and GET_REPORT are mandatory in the HID specification.
Make the corresponding API in hid-core mandatory too, which removes the
need to test against it in some various places.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---

new in v2

 Documentation/hid/hid-transport.txt |  3 ++-
 drivers/hid/hid-core.c              | 11 ++++++++---
 drivers/hid/hid-input.c             |  4 +---
 include/linux/hid.h                 |  5 +----
 4 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/Documentation/hid/hid-transport.txt b/Documentation/hid/hid-transport.txt
index 9dbbcea..3dcba9f 100644
--- a/Documentation/hid/hid-transport.txt
+++ b/Documentation/hid/hid-transport.txt
@@ -283,7 +283,8 @@ The available HID callbacks are:
                        int reqtype)
    Same as ->request() but provides the report as raw buffer. This request shall
    be synchronous. A transport driver must not use ->wait() to complete such
-   requests.
+   requests. This request is mandatory and hid core will reject the device if
+   it is missing.
 
  - int (*output_report) (struct hid_device *hdev, __u8 *buf, size_t len)
    Send raw output report via intr channel. Used by some HID device drivers
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index fdf3247..9190090 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1330,9 +1330,6 @@ void __hid_request(struct hid_device *hid, struct hid_report *report,
 	int ret;
 	int len;
 
-	if (!hid->ll_driver->raw_request)
-		return;
-
 	buf = hid_alloc_report_buf(report, GFP_KERNEL);
 	if (!buf)
 		return;
@@ -2475,6 +2472,14 @@ int hid_add_device(struct hid_device *hdev)
 		return -ENODEV;
 
 	/*
+	 * Check for the mandatory transport channel.
+	 */
+	 if (!hdev->ll_driver->raw_request) {
+		hid_err(hdev, "transport driver missing .raw_request()\n");
+		return -EINVAL;
+	 }
+
+	/*
 	 * Read the device report descriptor once and use as template
 	 * for the driver-specific modifications.
 	 */
diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index e3d625a..930382e 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -1269,9 +1269,7 @@ static struct hid_input *hidinput_allocate(struct hid_device *hid)
 	}
 
 	input_set_drvdata(input_dev, hid);
-	if (hid->ll_driver->request || hid->ll_driver->output_report ||
-	    hid->ll_driver->raw_request)
-		input_dev->event = hidinput_input_event;
+	input_dev->event = hidinput_input_event;
 	input_dev->open = hidinput_open;
 	input_dev->close = hidinput_close;
 	input_dev->setkeycode = hidinput_setkeycode;
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 60f3ff7..5eb282e 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -992,11 +992,8 @@ static inline int hid_hw_raw_request(struct hid_device *hdev,
 	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,
+	return hdev->ll_driver->raw_request(hdev, reportnum, buf, len,
 						    rtype, reqtype);
-
-	return -ENOSYS;
 }
 
 /**
-- 
1.8.5.3

[PATCH v2 3/3] 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.

Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---

ex 12/14

v2:
- get rid of the test regarding .raw_request now that it is mandatory.

 drivers/hid/hidraw.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/hid/hidraw.c b/drivers/hid/hidraw.c
index f8708c9..2cc484c 100644
--- a/drivers/hid/hidraw.c
+++ b/drivers/hid/hidraw.c
@@ -123,10 +123,6 @@ 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 (count > HID_MAX_BUFFER_SIZE) {
 		hid_warn(dev, "pid %d passed too large report\n",
@@ -153,7 +149,20 @@ 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, fallback to 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.5.3

Re: [PATCH 0/3] HID: low level transport cleanup, series 3

[David Herrmann]

Hi

On Thu, Feb 20, 2014 at 9:24 PM, Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
> Hi guys,
>
> this is a new round in the low level HID transport cleanup.
> These are the formely 7/14 an 12/14 only.
> I add in between a patch which makes .raw_request mandatory, to cleanup
> the old 12/14 (so 3/3 here).
>
> There is only two patches 2 merge after these 3... We are approaching the end!

All 3 look good:

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

Thanks again!
David

> Cheers,
> Benjamin
>
> Benjamin Tissoires (3):
>   HID: input: hid-input remove hid_output_raw_report call
>   HID: make .raw_request mandatory
>   HID: hidraw: replace hid_output_raw_report() calls by appropriates
>     ones
>
>  Documentation/hid/hid-transport.txt |  3 ++-
>  drivers/hid/hid-core.c              | 11 ++++++++---
>  drivers/hid/hid-input.c             | 10 ++++++----
>  drivers/hid/hidraw.c                | 19 ++++++++++++++-----
>  include/linux/hid.h                 |  5 +----
>  5 files changed, 31 insertions(+), 17 deletions(-)
>
> --
> 1.8.5.3
>

Re: [PATCH 0/3] HID: low level transport cleanup, series 3

[Jiri Kosina]

On Thu, 20 Feb 2014, Benjamin Tissoires wrote:

> Hi guys,
> 
> this is a new round in the low level HID transport cleanup.
> These are the formely 7/14 an 12/14 only.
> I add in between a patch which makes .raw_request mandatory, to cleanup
> the old 12/14 (so 3/3 here).
> 
> There is only two patches 2 merge after these 3... We are approaching the end!

Good stuff, thanks Benjamin. Queued for 3.15.

-- 
Jiri Kosina
SUSE Labs