Bug 137188 - [usb8][patch] correctly handle USB report descriptors with interleaved report IDs
Summary: [usb8][patch] correctly handle USB report descriptors with interleaved report...
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: usb (show other bugs)
Version: 8.0-BETA2
Hardware: Any Any
: Normal Affects Only Me
Assignee: freebsd-usb (Nobody)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-07-27 20:40 UTC by Eygene Ryabinkin
Modified: 2014-06-27 10:31 UTC (History)
1 user (show)

See Also:


Attachments
correctly-handle-interleaved-report-IDs.patch (22.39 KB, patch)
2009-07-27 20:40 UTC, Eygene Ryabinkin
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eygene Ryabinkin 2009-07-27 20:40:01 UTC
Some devices can have interleaved report IDs.  For example, when vendor
groups some items inside logical collections and these items come from
various report IDs, they will be interleaved.  Current code sets item
position to zero everytime when new report ID is encountered.  Such
behaviour isn't enforced by the HID specification (at least I hadn't
found such place in the HID spec v 1.11), so I take that interleaved IDs
are OK and the code should be changed to handle these devices.

Fix: The following patch adds the ability to save current item positions
when new report ID is encountered.  It fixes both kernel parser and
libusbhid one.  Patches are generally the same, modulo differences
between kernel- and user-side needs.
How-To-Repeat: 
One can reproduce this with, for example, Microsoft Wireless Laser Mouse
5000: it has Z-axis and tilt axis output items grouped with their feature
items that control sensitivity.  HID descriptor for this mouse can be
found in the supplied patch.

For me, without the below patch Z-axis is reported to be at offset 0, so
button clicks produce extra wheel movements that are translated to
scrolls under Xorg.  And that's very funny when click produces scrolls --
it reminds me old game with the hiding mouse pointer under WFWG 3.11.
Comment 1 Hans Petter Selasky freebsd_committer freebsd_triage 2009-07-27 21:37:21 UTC
On Monday 27 July 2009 21:36:29 Eygene Ryabinkin wrote:
> usb/137188

Hi,

Kernel part committed. I removed the malloc() part from the initial patch, 
because it is simpler to not use malloc in this case. Is the following working 
for you:

http://perforce.freebsd.org/chv.cgi?CH=166650

Second, can you port the kernel HID parser into libusbhid, because the kernel 
one is more advanced and handles more HID features than the userland one.

When you are done, send a new patch.

Thanks,

--HPS
Comment 2 dfilter service freebsd_committer freebsd_triage 2009-07-30 01:17:20 UTC
Author: alfred
Date: Thu Jul 30 00:17:08 2009
New Revision: 195967
URL: http://svn.freebsd.org/changeset/base/195967

Log:
  USB CORE - Improve HID parsing
  
  See PR description for more info. Patch is
  implemented differently than suggested, but
  having the same result.
  
  PR:     usb/137188
  
  Submitted by:	hps
  Approved by:	re

Modified:
  head/sys/dev/usb/usb_hid.c

Modified: head/sys/dev/usb/usb_hid.c
==============================================================================
--- head/sys/dev/usb/usb_hid.c	Thu Jul 30 00:16:50 2009	(r195966)
+++ head/sys/dev/usb/usb_hid.c	Thu Jul 30 00:17:08 2009	(r195967)
@@ -78,11 +78,19 @@ static uint8_t hid_get_byte(struct hid_d
 
 #define	MAXUSAGE 64
 #define	MAXPUSH 4
+#define	MAXID 16
+
+struct hid_pos_data {
+	int32_t rid;
+	uint32_t pos;
+};
+
 struct hid_data {
 	const uint8_t *start;
 	const uint8_t *end;
 	const uint8_t *p;
 	struct hid_item cur[MAXPUSH];
+	struct hid_pos_data last_pos[MAXID];
 	int32_t	usages_min[MAXUSAGE];
 	int32_t	usages_max[MAXUSAGE];
 	int32_t usage_last;	/* last seen usage */
@@ -119,6 +127,58 @@ hid_clear_local(struct hid_item *c)
 	c->set_delimiter = 0;
 }
 
+static void
+hid_switch_rid(struct hid_data *s, struct hid_item *c, int32_t next_rID)
+{
+	uint8_t i;
+
+	/* check for same report ID - optimise */
+
+	if (c->report_ID == next_rID)
+		return;
+
+	/* save current position for current rID */
+
+	if (c->report_ID == 0) {
+		i = 0;
+	} else {
+		for (i = 1; i != MAXID; i++) {
+			if (s->last_pos[i].rid == c->report_ID)
+				break;
+			if (s->last_pos[i].rid == 0)
+				break;
+		}
+	}
+	if (i != MAXID) {
+		s->last_pos[i].rid = c->report_ID;
+		s->last_pos[i].pos = c->loc.pos;
+	}
+
+	/* store next report ID */
+
+	c->report_ID = next_rID;
+
+	/* lookup last position for next rID */
+
+	if (next_rID == 0) {
+		i = 0;
+	} else {
+		for (i = 1; i != MAXID; i++) {
+			if (s->last_pos[i].rid == next_rID)
+				break;
+			if (s->last_pos[i].rid == 0)
+				break;
+		}
+	}
+	if (i != MAXID) {
+		s->last_pos[i].rid = next_rID;
+		c->loc.pos = s->last_pos[i].pos;
+	} else {
+		DPRINTF("Out of RID entries, position is set to zero!\n");
+		c->loc.pos = 0;
+	}
+}
+
 /*------------------------------------------------------------------------*
  *	hid_start_parse
  *------------------------------------------------------------------------*/
@@ -373,9 +433,7 @@ hid_get_item(struct hid_data *s, struct 
 				s->loc_size = dval & mask;
 				break;
 			case 8:
-				c->report_ID = dval;
-				/* new report - reset position */
-				c->loc.pos = 0;
+				hid_switch_rid(s, c, dval);
 				break;
 			case 9:
 				/* mask because value is unsigned */
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org"
Comment 3 Gavin Atkinson freebsd_committer freebsd_triage 2009-08-24 14:08:16 UTC
State Changed
From-To: open->patched

Mark as patched.  It's fixed in HEAD but I have no idea if it is applicable 
or even a problem in 7.x.  Hopefully the submitter or somebody more involved 
with USB knows.  Either way, "patched" is more appropriate.
Comment 4 Hans Petter Selasky freebsd_committer freebsd_triage 2014-06-27 10:31:22 UTC
This patch is MFC'ed to FreeBSD 8/9 and 10.

--HPS