From: Benjamin Tissoires <hidden> Date: 2014-02-10 17:59:11
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
From: Benjamin Tissoires <hidden> Date: 2014-02-10 17:59:08
uhid_hid_output_report() can be implemented through a simple call
to uhid_hid_output_raw().
Signed-off-by: Benjamin Tissoires <redacted>
---
drivers/hid/uhid.c | 22 +---------------------
1 file changed, 1 insertion(+), 21 deletions(-)
From: David Herrmann <hidden> Date: 2014-02-12 10:17:24
Hi
On Mon, Feb 10, 2014 at 6:58 PM, Benjamin Tissoires
[off-list ref] wrote:
quoted hunk
uhid_hid_output_report() can be implemented through a simple call
to uhid_hid_output_raw().
Signed-off-by: Benjamin Tissoires <redacted>
---
drivers/hid/uhid.c | 22 +---------------------
1 file changed, 1 insertion(+), 21 deletions(-)
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 <redacted>
Thanks
David
From: Benjamin Tissoires <hidden> Date: 2014-02-10 17:59:15
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 <redacted>
---
drivers/hid/usbhid/hid-core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Benjamin Tissoires <hidden> Date: 2014-02-10 17:59:21
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 <redacted>
---
drivers/hid/hid-wiimote-core.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
From: David Herrmann <hidden> Date: 2014-02-12 10:39:34
Hi
On Mon, Feb 10, 2014 at 6:58 PM, Benjamin Tissoires
[off-list ref] 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 <redacted>
Thanks
David
From: Benjamin Tissoires <hidden> Date: 2014-02-10 17:59:38
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 <redacted>
---
drivers/hid/hidraw.c | 22 +++++++++++++++++-----
1 file changed, 17 insertions(+), 5 deletions(-)
From: David Herrmann <hidden> Date: 2014-02-12 10:49:53
Hi
On Mon, Feb 10, 2014 at 6:58 PM, Benjamin Tissoires
[off-list ref] wrote:
quoted hunk
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 <redacted>
---
drivers/hid/hidraw.c | 22 +++++++++++++++++-----
1 file changed, 17 insertions(+), 5 deletions(-)
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 <redacted>
Thanks
David
quoted hunk
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:--
From: Benjamin Tissoires <hidden> Date: 2014-02-13 15:16:17
On Wed, Feb 12, 2014 at 5:49 AM, David Herrmann [off-list ref] wrote:
Hi
On Mon, Feb 10, 2014 at 6:58 PM, Benjamin Tissoires
[off-list ref] wrote:
quoted
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 <redacted>
---
drivers/hid/hidraw.c | 22 +++++++++++++++++-----
1 file changed, 17 insertions(+), 5 deletions(-)
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 <redacted>
Thanks
David
quoted
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:--
From: Benjamin Tissoires <hidden> Date: 2014-02-10 17:59:43
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 <redacted>
---
drivers/hid/hid-logitech-dj.c | 21 ++++++---------------
1 file changed, 6 insertions(+), 15 deletions(-)
From: Benjamin Tissoires <hidden> Date: 2014-02-10 17:59:45
It is better to check them soon enough before triggering any kernel panic.
Signed-off-by: Benjamin Tissoires <redacted>
---
drivers/hid/i2c-hid/i2c-hid.c | 2 +-
include/linux/hid.h | 6 ++++++
2 files changed, 7 insertions(+), 1 deletion(-)
From: Benjamin Tissoires <hidden> Date: 2014-02-10 18:00:21
Having our own .request() implementation does not give anything,
so use the generic binding.
Signed-off-by: Benjamin Tissoires <redacted>
---
drivers/hid/i2c-hid/i2c-hid.c | 31 -------------------------------
1 file changed, 31 deletions(-)
From: David Herrmann <hidden> Date: 2014-02-12 10:30:51
Hi
On Mon, Feb 10, 2014 at 6:58 PM, Benjamin Tissoires
[off-list ref] wrote:
quoted hunk
Having our own .request() implementation does not give anything,
so use the generic binding.
Signed-off-by: Benjamin Tissoires <redacted>
---
drivers/hid/i2c-hid/i2c-hid.c | 31 -------------------------------
1 file changed, 31 deletions(-)
From: Benjamin Tissoires <hidden> Date: 2014-02-13 15:14:35
On Wed, Feb 12, 2014 at 5:30 AM, David Herrmann [off-list ref] wrote:
Hi
On Mon, Feb 10, 2014 at 6:58 PM, Benjamin Tissoires
[off-list ref] wrote:
quoted
Having our own .request() implementation does not give anything,
so use the generic binding.
Signed-off-by: Benjamin Tissoires <redacted>
---
drivers/hid/i2c-hid/i2c-hid.c | 31 -------------------------------
1 file changed, 31 deletions(-)
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 <redacted>
Thanks
David
quoted
- 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;
@@ -508,9 +508,6 @@ struct hid_device { /* device report descriptor */structhid_usage*,__s32);void(*hiddev_report_event)(structhid_device*,structhid_report*);-/* handler for raw output data, used by hidraw */-int(*hid_output_raw_report)(structhid_device*,__u8*,size_t,unsignedchar);-/* debugging support via debugfs */unsignedshortdebug;structdentry*debug_dir;
@@ -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);
@@ -508,9 +508,6 @@ struct hid_device { /* device report descriptor */structhid_usage*,__s32);void(*hiddev_report_event)(structhid_device*,structhid_report*);-/* handler for raw output data, used by hidraw */-int(*hid_output_raw_report)(structhid_device*,__u8*,size_t,unsignedchar);-/* debugging support via debugfs */unsignedshortdebug;structdentry*debug_dir;
@@ -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);--
From: Benjamin Tissoires <hidden> Date: 2014-02-10 18:01:28
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 <redacted>
---
drivers/hid/hid-sony.c | 75 ++++++++++++++++++++++++++++++++++----------------
1 file changed, 52 insertions(+), 23 deletions(-)
@@ -760,38 +762,52 @@ static int sony_mapping(struct hid_device *hdev, struct hid_input *hi,/**TheSonySixaxisdoesnothandleHIDOutputReportsontheInterruptEP-*likeitshouldaccordingtousbhid/hid-core.c::usbhid_output_raw_report()+*likeitshouldaccordingtousbhid/hid-core.c::usbhid_output_report()*soweneedtooverridethatforcingHIDOutputReportsontheControlEP.-*-*ThereisalsoanotherissueaboutHIDOutputReportsviaUSB,theSixaxis-*doesnotwantthereport_idaspartofthedatapacket,sowehaveto-*discardbuf[0]whensendingtheactualcontrolmessage,evenfornumbered-*reports,humpf!*/-staticintsixaxis_usb_output_raw_report(structhid_device*hid,__u8*buf,-size_tcount,unsignedcharreport_type)+staticintsixaxis_usb_output_report(structhid_device*hid,__u8*buf,+size_tcount)+{+returnhid_hw_raw_request(hid,buf[0],buf,count,HID_OUTPUT_REPORT,+HID_REQ_SET_REPORT);+}++/*+*ThereisalsoanotherissueabouttheSET_REPORTcommandviaUSB,+*theSixaxisdoesnotwantthereport_idaspartofthedatapacket,so+*wehavetodiscardbuf[0]whensendingtheactualcontrolmessage,even+*fornumberedreports,humpf!+*/+staticintsixaxis_usb_raw_request(structhid_device*hid,+unsignedcharreportnum,__u8*buf,+size_tlen,unsignedcharrtype,intreqtype){structusb_interface*intf=to_usb_interface(hid->dev.parent);structusb_device*dev=interface_to_usbdev(intf);structusb_host_interface*interface=intf->cur_altsetting;-intreport_id=buf[0];intret;+u8usb_dir;+unsignedintusb_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++;returnret;
@@ -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);-}-elseif(sc->quirks&SIXAXIS_CONTROLLER_BT)+}elseif(sc->quirks&SIXAXIS_CONTROLLER_BT){ret=sixaxis_set_operational_bt(hdev);-elseif(sc->quirks&DUALSHOCK4_CONTROLLER_USB){+}elseif(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)
From: David Herrmann <hidden> Date: 2014-02-12 10:47:13
Hi
On Mon, Feb 10, 2014 at 6:58 PM, Benjamin Tissoires
[off-list ref] 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
@@ -760,38 +762,52 @@ static int sony_mapping(struct hid_device *hdev, struct hid_input *hi,/**TheSonySixaxisdoesnothandleHIDOutputReportsontheInterruptEP-*likeitshouldaccordingtousbhid/hid-core.c::usbhid_output_raw_report()+*likeitshouldaccordingtousbhid/hid-core.c::usbhid_output_report()*soweneedtooverridethatforcingHIDOutputReportsontheControlEP.-*-*ThereisalsoanotherissueaboutHIDOutputReportsviaUSB,theSixaxis-*doesnotwantthereport_idaspartofthedatapacket,sowehaveto-*discardbuf[0]whensendingtheactualcontrolmessage,evenfornumbered-*reports,humpf!*/-staticintsixaxis_usb_output_raw_report(structhid_device*hid,__u8*buf,-size_tcount,unsignedcharreport_type)+staticintsixaxis_usb_output_report(structhid_device*hid,__u8*buf,+size_tcount)+{+returnhid_hw_raw_request(hid,buf[0],buf,count,HID_OUTPUT_REPORT,+HID_REQ_SET_REPORT);+}++/*+*ThereisalsoanotherissueabouttheSET_REPORTcommandviaUSB,+*theSixaxisdoesnotwantthereport_idaspartofthedatapacket,so+*wehavetodiscardbuf[0]whensendingtheactualcontrolmessage,even+*fornumberedreports,humpf!+*/+staticintsixaxis_usb_raw_request(structhid_device*hid,+unsignedcharreportnum,__u8*buf,+size_tlen,unsignedcharrtype,intreqtype){structusb_interface*intf=to_usb_interface(hid->dev.parent);structusb_device*dev=interface_to_usbdev(intf);structusb_host_interface*interface=intf->cur_altsetting;-intreport_id=buf[0];intret;+u8usb_dir;+unsignedintusb_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++;returnret;
@@ -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);-}-elseif(sc->quirks&SIXAXIS_CONTROLLER_BT)+}elseif(sc->quirks&SIXAXIS_CONTROLLER_BT){ret=sixaxis_set_operational_bt(hdev);-elseif(sc->quirks&DUALSHOCK4_CONTROLLER_USB){+}elseif(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)
From: Benjamin Tissoires <hidden> Date: 2014-02-13 15:47:00
On Wed, Feb 12, 2014 at 5:47 AM, David Herrmann [off-list ref] wrote:
Hi
On Mon, Feb 10, 2014 at 6:58 PM, Benjamin Tissoires
[off-list ref] wrote:
quoted
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
@@ -760,38 +762,52 @@ static int sony_mapping(struct hid_device *hdev, struct hid_input *hi,/**TheSonySixaxisdoesnothandleHIDOutputReportsontheInterruptEP-*likeitshouldaccordingtousbhid/hid-core.c::usbhid_output_raw_report()+*likeitshouldaccordingtousbhid/hid-core.c::usbhid_output_report()*soweneedtooverridethatforcingHIDOutputReportsontheControlEP.-*-*ThereisalsoanotherissueaboutHIDOutputReportsviaUSB,theSixaxis-*doesnotwantthereport_idaspartofthedatapacket,sowehaveto-*discardbuf[0]whensendingtheactualcontrolmessage,evenfornumbered-*reports,humpf!*/-staticintsixaxis_usb_output_raw_report(structhid_device*hid,__u8*buf,-size_tcount,unsignedcharreport_type)+staticintsixaxis_usb_output_report(structhid_device*hid,__u8*buf,+size_tcount)+{+returnhid_hw_raw_request(hid,buf[0],buf,count,HID_OUTPUT_REPORT,+HID_REQ_SET_REPORT);+}++/*+*ThereisalsoanotherissueabouttheSET_REPORTcommandviaUSB,+*theSixaxisdoesnotwantthereport_idaspartofthedatapacket,so+*wehavetodiscardbuf[0]whensendingtheactualcontrolmessage,even+*fornumberedreports,humpf!+*/+staticintsixaxis_usb_raw_request(structhid_device*hid,+unsignedcharreportnum,__u8*buf,+size_tlen,unsignedcharrtype,intreqtype){structusb_interface*intf=to_usb_interface(hid->dev.parent);structusb_device*dev=interface_to_usbdev(intf);structusb_host_interface*interface=intf->cur_altsetting;-intreport_id=buf[0];intret;+u8usb_dir;+unsignedintusb_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++;returnret;
@@ -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);-}-elseif(sc->quirks&SIXAXIS_CONTROLLER_BT)+}elseif(sc->quirks&SIXAXIS_CONTROLLER_BT){ret=sixaxis_set_operational_bt(hdev);-elseif(sc->quirks&DUALSHOCK4_CONTROLLER_USB){+}elseif(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)
From: Benjamin Tissoires <hidden> Date: 2014-02-10 18:01:29
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 <redacted>
---
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(-)
@@ -538,8 +538,8 @@ static int magicmouse_probe(struct hid_device *hdev,*butthereseemstobenootherwayofswitchingthemode.*Thusthesuper-uglyhackysuccesscheckbelow.*/-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);gotoerr_stop_hw;
From: Benjamin Tissoires <hidden> Date: 2014-02-10 18:02:11
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 <redacted>
---
drivers/hid/hid-input.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
From: David Herrmann <hidden> Date: 2014-02-12 10:35:12
Hi
On Mon, Feb 10, 2014 at 6:58 PM, Benjamin Tissoires
[off-list ref] wrote:
quoted hunk
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 <redacted>
---
drivers/hid/hid-input.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
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.
From: Benjamin Tissoires <hidden> Date: 2014-02-13 15:39:05
On Wed, Feb 12, 2014 at 5:35 AM, David Herrmann [off-list ref] wrote:
Hi
On Mon, Feb 10, 2014 at 6:58 PM, Benjamin Tissoires
[off-list ref] wrote:
quoted
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 <redacted>
---
drivers/hid/hid-input.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
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.
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
From: Benjamin Tissoires <hidden> Date: 2014-02-10 18:02:53
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 <redacted>
---
drivers/hid/i2c-hid/i2c-hid.c | 69 +++++++++++++++++++++++++++++++++++++------
1 file changed, 60 insertions(+), 9 deletions(-)
@@ -559,7 +572,7 @@ static int i2c_hid_get_raw_report(struct hid_device *hid,}staticinti2c_hid_output_raw_report(structhid_device*hid,__u8*buf,-size_tcount,unsignedcharreport_type)+size_tcount,unsignedcharreport_type,booluse_data){structi2c_client*client=hid->driver_data;intreport_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 */
From: David Herrmann <hidden> Date: 2014-02-12 10:29:35
Hi
On Mon, Feb 10, 2014 at 6:58 PM, Benjamin Tissoires
[off-list ref] wrote:
quoted hunk
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 <redacted>
---
drivers/hid/i2c-hid/i2c-hid.c | 69 +++++++++++++++++++++++++++++++++++++------
1 file changed, 60 insertions(+), 9 deletions(-)
@@ -559,7 +572,7 @@ static int i2c_hid_get_raw_report(struct hid_device *hid,}staticinti2c_hid_output_raw_report(structhid_device*hid,__u8*buf,-size_tcount,unsignedcharreport_type)+size_tcount,unsignedcharreport_type,booluse_data){structi2c_client*client=hid->driver_data;intreport_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 */
From: Benjamin Tissoires <hidden> Date: 2014-02-10 18:02:56
.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 <redacted>
---
drivers/hid/hid-core.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
include/linux/hid.h | 5 ++++-
2 files changed, 48 insertions(+), 2 deletions(-)
From: David Herrmann <hidden> Date: 2014-02-12 10:26:00
Hi
On Mon, Feb 10, 2014 at 6:58 PM, Benjamin Tissoires
[off-list ref] wrote:
quoted hunk
.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 <redacted>
---
drivers/hid/hid-core.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
include/linux/hid.h | 5 ++++-
2 files changed, 48 insertions(+), 2 deletions(-)
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.
quoted hunk
+}
+
/*
* 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 <redacted>
Thanks
David
quoted hunk
int hid_report_raw_event(struct hid_device *hid, int type, u8 *data, int size,
int interrupt)
{
From: Benjamin Tissoires <hidden> Date: 2014-02-13 15:21:43
On Wed, Feb 12, 2014 at 5:25 AM, David Herrmann [off-list ref] wrote:
Hi
On Mon, Feb 10, 2014 at 6:58 PM, Benjamin Tissoires
[off-list ref] wrote:
quoted
.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 <redacted>
---
drivers/hid/hid-core.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
include/linux/hid.h | 5 ++++-
2 files changed, 48 insertions(+), 2 deletions(-)
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.
quoted
+}
+
/*
* 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
From: David Herrmann <hidden> Date: 2014-02-16 16:43:05
Hi
On Thu, Feb 13, 2014 at 4:21 PM, Benjamin Tissoires
[off-list ref] wrote:
On Wed, Feb 12, 2014 at 5:25 AM, David Herrmann [off-list ref] wrote:
quoted
Hi
On Mon, Feb 10, 2014 at 6:58 PM, Benjamin Tissoires
[off-list ref] wrote:
quoted
.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 <redacted>
---
drivers/hid/hid-core.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
include/linux/hid.h | 5 ++++-
2 files changed, 48 insertions(+), 2 deletions(-)
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 :)
quoted
I always have to read that shifting code twice to get it.. Maybe add a
comment explaining the different entries here.
good idea.
quoted
quoted
+}
+
/*
* 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
From: Benjamin Tissoires <hidden> Date: 2014-02-10 18:03:47
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 <redacted>
---
drivers/hid/uhid.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
From: David Herrmann <hidden> Date: 2014-02-12 10:19:38
Hi
On Mon, Feb 10, 2014 at 6:58 PM, Benjamin Tissoires
[off-list ref] wrote:
quoted hunk
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 <redacted>
---
drivers/hid/uhid.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
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