[PATCH 1/1] Don't Send Feature Reports on Interrupt Endpoint

[PATCH 1/1] Don't Send Feature Reports on Interrupt Endpoint

[Alan Ott]

Feature reports should only be sent on the control endpoint.

Reported-by: simon@mungewell.org
Signed-off-by: Alan Ott <alan@signal11.us>
---
 drivers/hid/usbhid/hid-core.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index b729c02..b0ccc42 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -807,7 +807,7 @@ static int usbhid_output_raw_report(struct hid_device *hid, __u8 *buf, size_t co
 	struct usb_host_interface *interface = intf->cur_altsetting;
 	int ret;
 
-	if (usbhid->urbout) {
+	if (usbhid->urbout && report_type != HID_FEATURE_REPORT) {
 		int actual_length;
 		int skipped_report_id = 0;
 		if (buf[0] == 0x0) {
-- 
1.7.0.4

Re: [PATCH 1/1] Don't Send Feature Reports on Interrupt Endpoint

[Alan Stern]

On Wed, 15 Sep 2010, Alan Ott wrote:

> Feature reports should only be sent on the control endpoint.

Where is this requirement?  Section 5.6 of the HID spec says: 

	Note  Only Input reports are sent via the Interrupt In pipe. 
	Feature and Output reports must be initiated by the host via
	the Control pipe or an optional Interrupt Out pipe.

So if there is an Interrupt-OUT endpoint, it should be valid to use it 
for a Feature report.

Alan Stern

Re: [PATCH 1/1] Don't Send Feature Reports on Interrupt Endpoint

[Alan Ott]

On 09/15/2010 12:10 PM, Alan Stern wrote:
> On Wed, 15 Sep 2010, Alan Ott wrote:
>
>    
>> Feature reports should only be sent on the control endpoint.
>>      
> Where is this requirement?  Section 5.6 of the HID spec says:
>
> 	Note  Only Input reports are sent via the Interrupt In pipe.
> 	Feature and Output reports must be initiated by the host via
> 	the Control pipe or an optional Interrupt Out pipe.
>
> So if there is an Interrupt-OUT endpoint, it should be valid to use it
> for a Feature report.
>
> Alan Stern
>    

That is true, the standard does say that. Thanks for checking my work.

I expected to go into the HID standard and find something that would 
back me up. All I ended up finding was contradictions. I know I had seen 
it somewhere (that Feature reports must use the Control endpoint), and 
eventually I remembered that I had read it in Jan Axelson's USB 
Complete, 3rd Edition[1]. I sent her an email asking what she based it 
on, and I cited parts of the HID standard which I thought to be either 
unspecific, or even seemed to indicate the opposite of what she asserts 
in her book (one good example of which is the one you cited). I have not 
heard back from her yet (but she does indeed answer her email, so I 
expect something in a day or two).

In addition to the section you cited, there's also section 6.2.2.5 which 
says, at the very end of the section, on page 32:

    Output type reports can optionally be sent via an Interrupt Out pipe.
    While similar in function, Output and Feature items differ in the
    following
    ways:
         [snip]
       Like Output items, Feature items make up Feature Reports
    accessible via the Control
       pipe with the Get_Report (Feature) and Set_Report (Feature) requests.

That section seems to say both ways, depending on how you read "Output 
type" (ie: does it mean OUTPUT reports or does it mean "reports which go 
out from the host"). Note that particular section is the only place 
where the wording "input type" or "output type" is used, indicating it 
may mean "reports which go out from the host."

The second part of that quote says feature reports are accessible 
through the control pipe. (It doesn't say that they _aren't_ accessible 
any other way). It's curious to me why it would say it in that way, 
without saying "only accessible" or "also accessible."

In all, although somewhat unclear, the HID document does seem to suggest 
that the Interrupt OUT pipe can handle Feature Reports. However, there 
are several things which make me think otherwise.

1. The Windows implementation will refuse to send feature reports 
through WriteFile() (which is the function used to send reports out the 
interrupt OUT pipe if it exists). The HidD_SetFeature() function will 
ONLY send feature reports out the Control pipe, regardless of the 
presence of an Interrupt OUT endpoint.

2. The Macintosh HID implementation does the same thing as the Windows 
version. On the Mac, there's IOHIDDeviceSetReport() which allows you to 
specify a report type of Feature or Output. Output reports go to the 
Interrupt OUT pipe if it exists; feature reports do not. Feature reports 
only go out the Control endpoint.

3. Jan Axelson's book. While it's not an official standard, it's widely 
accepted as a good general reference on USB. Like I said, I have an 
email in to the author asking her to cite her source on Feature Reports. 
That said, the book is very Windows-centric on the host side, and she 
may be basing her assertion on #1 above.

4. Simon (Mungewell)'s email which started this whole thing. First, he 
indicates that his device doesn't handle Feature reports in the 
Interrupt OUT pipe. Second, he asserts that libhid works the same way 
Windows and Mac do, sending Feature reports out the Control pipe[2]. I 
have a PS3 controller that I borrowed which also seems to work the same 
way (ie: Feature reports don't work if they go out the Interrupt OUT 
endpoint).

5. The Bluetooth HID specification says in multiple places that "Feature 
Reports must be carried on the Control channel." Yes yes, you don't have 
to say it. Bluetooth is not USB, so the bluetooth standard shouldn't 
apply. Maybe true.

So there it is. That's everything I know about this particular problem. 
The standard to me is unclear at best; Every single other 
implementation[3] (Windows, Mac, libhid) uses the control endpoint only; 
Jan's book says that only the control endpoint can be used; Simon's 
hardware only works using the control endpoint for feature reports; my 
hardware only works using the control endpoint for Feature reports; the 
Bluetooth spec says Feature reports must use the control endpoint.

PS: I'll throw out one more thing. HID transfers which use the control 
endpoint have their report type identified in the wValue field of their 
header. Transfers which use the Interrupt OUT endpoint do not have their 
type identified in any way. (they only have the report ID, and even 
then, only when numbered reports are used). If a device which did not 
use report IDs had both a single OUTPUT and a single FEATURE report, how 
could the reports be differentiated by the device if it were possible 
for the FEATURE report to go out on the OUT endpoint? I think that 
actually closes the argument in my mind.

I'd be happy to hear alternate theories.

Alan.


[1] Fourth edition is out. I don't have it. Can anyone here confirm or 
deny that the 4th edition has similar language?
[2] I've confirmed this at:
      
http://libhid.alioth.debian.org/doc/hid__exchange_8c-source.html#l00191
[3] I'm not counting my own hidapi library 
(http://www.signal11.us/oss/hidapi), since it was written by me and is 
of course implemented the way I'm arguing for.

Re: [PATCH 1/1] Don't Send Feature Reports on Interrupt Endpoint

[Alan Stern]

On Wed, 15 Sep 2010, Alan Ott wrote:

> On 09/15/2010 12:10 PM, Alan Stern wrote:
> > On Wed, 15 Sep 2010, Alan Ott wrote:
> >
> >    
> >> Feature reports should only be sent on the control endpoint.
> >>      
> > Where is this requirement?  Section 5.6 of the HID spec says:
> >
> > 	Note  Only Input reports are sent via the Interrupt In pipe.
> > 	Feature and Output reports must be initiated by the host via
> > 	the Control pipe or an optional Interrupt Out pipe.
> >
> > So if there is an Interrupt-OUT endpoint, it should be valid to use it
> > for a Feature report.
> >
> > Alan Stern
> >    
> 
> That is true, the standard does say that. Thanks for checking my work.
> 
> I expected to go into the HID standard and find something that would 
> back me up. All I ended up finding was contradictions. I know I had seen 
> it somewhere (that Feature reports must use the Control endpoint), and 
> eventually I remembered that I had read it in Jan Axelson's USB 
> Complete, 3rd Edition[1]. I sent her an email asking what she based it 
> on, and I cited parts of the HID standard which I thought to be either 
> unspecific, or even seemed to indicate the opposite of what she asserts 
> in her book (one good example of which is the one you cited). I have not 
> heard back from her yet (but she does indeed answer her email, so I 
> expect something in a day or two).
> 
> In addition to the section you cited, there's also section 6.2.2.5 which 
> says, at the very end of the section, on page 32:
> 
>     Output type reports can optionally be sent via an Interrupt Out pipe.
>     While similar in function, Output and Feature items differ in the
>     following
>     ways:
>          [snip]
>        Like Output items, Feature items make up Feature Reports
>     accessible via the Control
>        pipe with the Get_Report (Feature) and Set_Report (Feature) requests.
> 
> That section seems to say both ways, depending on how you read "Output 
> type" (ie: does it mean OUTPUT reports or does it mean "reports which go 
> out from the host"). Note that particular section is the only place 
> where the wording "input type" or "output type" is used, indicating it 
> may mean "reports which go out from the host."
> 
> The second part of that quote says feature reports are accessible 
> through the control pipe. (It doesn't say that they _aren't_ accessible 
> any other way). It's curious to me why it would say it in that way, 
> without saying "only accessible" or "also accessible."
> 
> In all, although somewhat unclear, the HID document does seem to suggest 
> that the Interrupt OUT pipe can handle Feature Reports. However, there 
> are several things which make me think otherwise.
> 
> 1. The Windows implementation will refuse to send feature reports 
> through WriteFile() (which is the function used to send reports out the 
> interrupt OUT pipe if it exists). The HidD_SetFeature() function will 
> ONLY send feature reports out the Control pipe, regardless of the 
> presence of an Interrupt OUT endpoint.
> 
> 2. The Macintosh HID implementation does the same thing as the Windows 
> version. On the Mac, there's IOHIDDeviceSetReport() which allows you to 
> specify a report type of Feature or Output. Output reports go to the 
> Interrupt OUT pipe if it exists; feature reports do not. Feature reports 
> only go out the Control endpoint.
> 
> 3. Jan Axelson's book. While it's not an official standard, it's widely 
> accepted as a good general reference on USB. Like I said, I have an 
> email in to the author asking her to cite her source on Feature Reports. 
> That said, the book is very Windows-centric on the host side, and she 
> may be basing her assertion on #1 above.
> 
> 4. Simon (Mungewell)'s email which started this whole thing. First, he 
> indicates that his device doesn't handle Feature reports in the 
> Interrupt OUT pipe. Second, he asserts that libhid works the same way 
> Windows and Mac do, sending Feature reports out the Control pipe[2]. I 
> have a PS3 controller that I borrowed which also seems to work the same 
> way (ie: Feature reports don't work if they go out the Interrupt OUT 
> endpoint).
> 
> 5. The Bluetooth HID specification says in multiple places that "Feature 
> Reports must be carried on the Control channel." Yes yes, you don't have 
> to say it. Bluetooth is not USB, so the bluetooth standard shouldn't 
> apply. Maybe true.
> 
> So there it is. That's everything I know about this particular problem. 
> The standard to me is unclear at best; Every single other 
> implementation[3] (Windows, Mac, libhid) uses the control endpoint only; 
> Jan's book says that only the control endpoint can be used; Simon's 
> hardware only works using the control endpoint for feature reports; my 
> hardware only works using the control endpoint for Feature reports; the 
> Bluetooth spec says Feature reports must use the control endpoint.
> 
> PS: I'll throw out one more thing. HID transfers which use the control 
> endpoint have their report type identified in the wValue field of their 
> header. Transfers which use the Interrupt OUT endpoint do not have their 
> type identified in any way. (they only have the report ID, and even 
> then, only when numbered reports are used). If a device which did not 
> use report IDs had both a single OUTPUT and a single FEATURE report, how 
> could the reports be differentiated by the device if it were possible 
> for the FEATURE report to go out on the OUT endpoint? I think that 
> actually closes the argument in my mind.
> 
> I'd be happy to hear alternate theories.

That's all fine, and I have no objection to the patch itself.  You 
should include (in brief form) some of this explanation in the patch 
description so that people will know _why_ this has to be done.

Alan Stern

Re: [PATCH 1/1] Don't Send Feature Reports on Interrupt Endpoint

[Jiri Kosina]

On Thu, 16 Sep 2010, Alan Stern wrote:

> That's all fine, and I have no objection to the patch itself.  You 
> should include (in brief form) some of this explanation in the patch 
> description so that people will know _why_ this has to be done.

Fully, agreed. Alan, could you please write a short description into the 
patch changelog and resubmit it to me?

Thanks a lot,

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

[PATCH v2 1/1] Don't Send Feature Reports on Interrupt Endpoint

[Alan Ott]

Feature reports should only be sent on the control endpoint.

The USB HID standard is unclear and confusing on this issue. It seems to
suggest that Feature reports can be sent on a HID device's Interrupt OUT
endpoint. This cannot be the case because the report type is not
encoded in transfers sent out the Interrput OUT endpoint. If Feature 
reports were sent on the Interrupt OUT endpint, they would be 
indistinguishable from Output reports in the case where Report IDs were 
not used.

Further, Windows and Mac OS X do not send Feature reports out the 
interrupt OUT Endpoint. They will only go out the Control Endpoint.

In addition, many devices simply do not hande Feature reports sent out 
the Interrupt OUT endpoint.

Reported-by: simon@mungewell.org
Signed-off-by: Alan Ott <alan@signal11.us>
---
  drivers/hid/usbhid/hid-core.c |    2 +-
  1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index b729c02..b0ccc42 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -807,7 +807,7 @@ static int usbhid_output_raw_report(struct 
hid_device *hid, __u8 *buf, size_t co
  	struct usb_host_interface *interface = intf->cur_altsetting;
  	int ret;
  -	if (usbhid->urbout) {
+	if (usbhid->urbout && report_type != HID_FEATURE_REPORT) {
  		int actual_length;
  		int skipped_report_id = 0;
  		if (buf[0] == 0x0) {
-- 
1.7.0.4

Re: [PATCH v2 1/1] Don't Send Feature Reports on Interrupt Endpoint

[Sergei Shtylyov]

Hello.

Alan Ott wrote:

> Feature reports should only be sent on the control endpoint.

> The USB HID standard is unclear and confusing on this issue. It seems to
> suggest that Feature reports can be sent on a HID device's Interrupt OUT
> endpoint. This cannot be the case because the report type is not
> encoded in transfers sent out the Interrput OUT endpoint. If Feature 
> reports were sent on the Interrupt OUT endpint, they would be 
> indistinguishable from Output reports in the case where Report IDs were 
> not used.

> Further, Windows and Mac OS X do not send Feature reports out the 
> interrupt OUT Endpoint. They will only go out the Control Endpoint.

> In addition, many devices simply do not hande Feature reports sent out 
> the Interrupt OUT endpoint.

> Reported-by: simon@mungewell.org
> Signed-off-by: Alan Ott <alan@signal11.us>
> ---
>  drivers/hid/usbhid/hid-core.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> index b729c02..b0ccc42 100644
> --- a/drivers/hid/usbhid/hid-core.c
> +++ b/drivers/hid/usbhid/hid-core.c
> @@ -807,7 +807,7 @@ static int usbhid_output_raw_report(struct 
> hid_device *hid, __u8 *buf, size_t co
>      struct usb_host_interface *interface = intf->cur_altsetting;
>      int ret;

    It's high time to add an empty line here...

>  -    if (usbhid->urbout) {

    The patch is damaged -- something inserted spaces at the start of lines...

WBR, Sergei

[PATCH v3 1/1] Don't Send Feature Reports on Interrupt Endpoint

[Alan Ott]

Feature reports should only be sent on the control endpoint.

The USB HID standard is unclear and confusing on this issue. It seems to
suggest that Feature reports can be sent on a HID device's Interrupt OUT
endpoint.  This cannot be the case because the report type is not encoded in
transfers sent out the Interrput OUT endpoint.  If Feature reports were sent
on the Interrupt OUT endpint, they would be indistinguishable from Output
reports in the case where Report IDs were not used.

Further, Windows and Mac OS X do not send Feature reports out the interrupt
OUT Endpoint.  They will only go out the Control Endpoint.

In addition, many devices simply do not hande Feature reports sent out the Interrupt OUT endpoint.

Reported-by: simon@mungewell.org
Signed-off-by: Alan Ott <alan@signal11.us>
---
 drivers/hid/usbhid/hid-core.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index b729c02..b0ccc42 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -807,7 +807,7 @@ static int usbhid_output_raw_report(struct hid_device *hid, __u8 *buf, size_t co
 	struct usb_host_interface *interface = intf->cur_altsetting;
 	int ret;
 
-	if (usbhid->urbout) {
+	if (usbhid->urbout && report_type != HID_FEATURE_REPORT) {
 		int actual_length;
 		int skipped_report_id = 0;
 		if (buf[0] == 0x0) {
-- 
1.7.0.4

[PATCH v2 1/1] trivial: usbhid: Formatting fix

[Alan Ott]

Added blank line after declarations.

Signed-off-by: Alan Ott <alan@signal11.us>
---
 drivers/hid/usbhid/hid-core.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index b729c02..ff90069 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -810,6 +810,7 @@ static int usbhid_output_raw_report(struct hid_device *hid, __u8 *buf, size_t co
 	if (usbhid->urbout) {
 		int actual_length;
 		int skipped_report_id = 0;
+
 		if (buf[0] == 0x0) {
 			/* Don't send the Report ID */
 			buf++;
-- 
1.7.0.4

Re: [PATCH v3 1/1] Don't Send Feature Reports on Interrupt Endpoint

[Jiri Kosina]

On Tue, 21 Sep 2010, Alan Ott wrote:

> Feature reports should only be sent on the control endpoint.
> 
> The USB HID standard is unclear and confusing on this issue. It seems to
> suggest that Feature reports can be sent on a HID device's Interrupt OUT
> endpoint.  This cannot be the case because the report type is not encoded in
> transfers sent out the Interrput OUT endpoint.  If Feature reports were sent
> on the Interrupt OUT endpint, they would be indistinguishable from Output
> reports in the case where Report IDs were not used.
> 
> Further, Windows and Mac OS X do not send Feature reports out the interrupt
> OUT Endpoint.  They will only go out the Control Endpoint.
> 
> In addition, many devices simply do not hande Feature reports sent out the Interrupt OUT endpoint.
> 
> Reported-by: simon@mungewell.org
> Signed-off-by: Alan Ott <alan@signal11.us>
> ---
>  drivers/hid/usbhid/hid-core.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> index b729c02..b0ccc42 100644
> --- a/drivers/hid/usbhid/hid-core.c
> +++ b/drivers/hid/usbhid/hid-core.c
> @@ -807,7 +807,7 @@ static int usbhid_output_raw_report(struct hid_device *hid, __u8 *buf, size_t co
>  	struct usb_host_interface *interface = intf->cur_altsetting;
>  	int ret;
>  
> -	if (usbhid->urbout) {
> +	if (usbhid->urbout && report_type != HID_FEATURE_REPORT) {
>  		int actual_length;
>  		int skipped_report_id = 0;
>  		if (buf[0] == 0x0) {

Applied to the 'logitech' branch (as Simon's new Logitech driver is 
depending on this).

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

Re: [PATCH v2 1/1] trivial: usbhid: Formatting fix

[Jiri Kosina]

On Tue, 21 Sep 2010, Alan Ott wrote:

> Added blank line after declarations.
> 
> Signed-off-by: Alan Ott <alan@signal11.us>
> ---
>  drivers/hid/usbhid/hid-core.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> index b729c02..ff90069 100644
> --- a/drivers/hid/usbhid/hid-core.c
> +++ b/drivers/hid/usbhid/hid-core.c
> @@ -810,6 +810,7 @@ static int usbhid_output_raw_report(struct hid_device *hid, __u8 *buf, size_t co
>  	if (usbhid->urbout) {
>  		int actual_length;
>  		int skipped_report_id = 0;
> +
>  		if (buf[0] == 0x0) {
>  			/* Don't send the Report ID */
>  			buf++;

Applied, thank you.

-- 
Jiri Kosina
SUSE Labs, Novell Inc.