usb/171810: [patch] make hid_start_parse(3) respect report ID argument
Number: 171810
Category: usb
Synopsis: [patch] make hid_start_parse(3) respect report ID argument
Confidential: no
Severity: non-critical
Priority: low
Responsible:freebsd-usb
State: open
Quarter:
Keywords:
Date-Required:
Class: change-request
Submitter-Id: current-users
Arrival-Date: Thu Sep 20 11:50:11 UTC 2012
Closed-Date:
Last-Modified:
Originator: Vitaly Magerya
Release:
Organization:
Environment:
Description:
Currently hid_start_parse(3) ignores it's 3-rd parameter -- the
report ID, so subsequent calls to hid_get_item(3) return items
of all report IDs, not just the one requested. This also affects
hid_locate(3), which effectively ignores requested report ID too.
This is different from how it works on NetBSD and OpenBSD: their
libusbhid(3) implementations only return items of the requested
report ID, unless it is -1, in which case items of all report
IDs are returned. In fact our own usbhidctl(1) always uses -1
as the report ID argument, and usbhidaction(1) has an undocumented
option '-r' to use report ID other than -1 -- which doesn't seem
to work at the moment (I did not test it, but it appears that
way from the code).
In short, I propose to follow NetBSD and OpenBSD and respect id
argument of hid_start_parse(3).
Note: we had the same API before revision 205728, which says
This merge does not change any API, so it seems this was just
overlooked.
How-To-Repeat:
Fix:
Patch attached with submission follows:
Index: usbhid.3
===
--- usbhid.3(revision 240744)
+++ usbhid.3(working copy)
@@ -144,16 +144,15 @@
.Ss Descriptor Parsing Functions
To parse the report descriptor the
.Fn hid_start_parse
-function should be called with a report descriptor and a set that
-describes which items that are interesting.
+function should be called with a report descriptor, a set that
+describes which items that are interesting, and the desired report
+ID (or -1 to obtain items of all report IDs).
The set is obtained by OR-ing together values
.Fa (1 k)
where
.Fa k
is an item of type
.Vt hid_kind_t .
-The report ID (if present) is given by
-.Fa id .
The function returns
.Dv NULL
if the initialization fails, otherwise an opaque value to be used
Index: descr.c
===
--- descr.c (revision 240744)
+++ descr.c (working copy)
@@ -68,7 +68,7 @@
if ((rep = hid_get_report_desc(fd)) == NULL)
goto use_ioctl;
kindset = 1 hid_input | 1 hid_output | 1 hid_feature;
- for (d = hid_start_parse(rep, kindset, 0); hid_get_item(d, h); ) {
+ for (d = hid_start_parse(rep, kindset, -1); hid_get_item(d, h); ) {
/* Return the first report ID we met. */
if (h.report_ID != 0) {
temp = h.report_ID;
Index: parse.c
===
--- parse.c (revision 240744)
+++ parse.c (working copy)
@@ -70,6 +70,7 @@
uint8_t iusage; /* current usages_min/max index */
uint8_t ousage; /* current usages_min/max offset */
uint8_t susage; /* usage set flags */
+ uint32_t reportid; /* requested report ID */
};
/**
@@ -149,7 +150,7 @@
* hid_start_parse
**/
hid_data_t
-hid_start_parse(report_desc_t d, int kindset, int id __unused)
+hid_start_parse(report_desc_t d, int kindset, int id)
{
struct hid_data *s;
@@ -158,6 +159,7 @@
s-start = s-p = d-data;
s-end = d-data + d-size;
s-kindset = kindset;
+ s-reportid = id;
return (s);
}
@@ -207,8 +209,8 @@
/**
* hid_get_item
**/
-int
-hid_get_item(hid_data_t s, hid_item_t *h)
+static int
+hid_get_item_raw(hid_data_t s, hid_item_t *h)
{
hid_item_t *c;
unsigned int bTag, bType, bSize;
@@ -509,6 +511,19 @@
}
int
+hid_get_item(hid_data_t s, hid_item_t *h)
+{
+ int r;
+
+ for (;;) {
+ r = hid_get_item_raw(s, h);
+ if (r = 0 || s-reportid == -1 || h-report_ID == s-reportid)
+ break;
+ }
+ return (r);
+}
+
+int
hid_report_size(report_desc_t r, enum hid_kind k, int id)
{
struct hid_data *d;
@@ -523,7 +538,7 @@
memset(h, 0, sizeof h);
for (d = hid_start_parse(r, 1 k, id); hid_get_item(d, h); ) {
- if ((h.report_ID == id || id 0) h.kind == k) {
+ if (h.kind == k) {
/* compute minimum */
if (lpos h.pos)
Re: usb/171810: commit references a PR
The following reply was made to PR usb/171810; it has been noted by GNATS.
From: dfil...@freebsd.org (dfilter service)
To: bug-follo...@freebsd.org
Cc:
Subject: Re: usb/171810: commit references a PR
Date: Thu, 20 Sep 2012 18:56:39 + (UTC)
Author: mav
Date: Thu Sep 20 18:56:27 2012
New Revision: 240762
URL: http://svn.freebsd.org/changeset/base/240762
Log:
Restore handling of the third argument (id) of hid_start_parse(), same as
it is done in NetBSD/OpenBSD, and as it was here before r205728.
I personally think this API or its implementation is incorrect, as it is not
correct to filter collections based on report ID, as they are orthogonal
in general case, but I see no harm from supporting this feature.
PR: usb/171810
Submitted by:Vitaly Magerya vmage...@gmail.com
MFC after: 1 month
Modified:
head/lib/libusbhid/descr.c
head/lib/libusbhid/parse.c
head/lib/libusbhid/usbhid.3
Modified: head/lib/libusbhid/descr.c
==
--- head/lib/libusbhid/descr.c Thu Sep 20 18:42:00 2012(r240761)
+++ head/lib/libusbhid/descr.c Thu Sep 20 18:56:27 2012(r240762)
@@ -68,7 +68,7 @@ hid_get_report_id(int fd)
if ((rep = hid_get_report_desc(fd)) == NULL)
goto use_ioctl;
kindset = 1 hid_input | 1 hid_output | 1 hid_feature;
- for (d = hid_start_parse(rep, kindset, 0); hid_get_item(d, h); ) {
+ for (d = hid_start_parse(rep, kindset, -1); hid_get_item(d, h); ) {
/* Return the first report ID we met. */
if (h.report_ID != 0) {
temp = h.report_ID;
Modified: head/lib/libusbhid/parse.c
==
--- head/lib/libusbhid/parse.c Thu Sep 20 18:42:00 2012(r240761)
+++ head/lib/libusbhid/parse.c Thu Sep 20 18:56:27 2012(r240762)
@@ -70,6 +70,7 @@ struct hid_data {
uint8_t iusage; /* current usages_min/max index */
uint8_t ousage; /* current usages_min/max offset */
uint8_t susage; /* usage set flags */
+ int32_t reportid; /* requested report ID */
};
/**
@@ -149,7 +150,7 @@ hid_switch_rid(struct hid_data *s, struc
*hid_start_parse
**/
hid_data_t
-hid_start_parse(report_desc_t d, int kindset, int id __unused)
+hid_start_parse(report_desc_t d, int kindset, int id)
{
struct hid_data *s;
@@ -158,6 +159,7 @@ hid_start_parse(report_desc_t d, int kin
s-start = s-p = d-data;
s-end = d-data + d-size;
s-kindset = kindset;
+ s-reportid = id;
return (s);
}
@@ -207,8 +209,8 @@ hid_get_byte(struct hid_data *s, const u
/**
*hid_get_item
**/
-int
-hid_get_item(hid_data_t s, hid_item_t *h)
+static int
+hid_get_item_raw(hid_data_t s, hid_item_t *h)
{
hid_item_t *c;
unsigned int bTag, bType, bSize;
@@ -509,6 +511,19 @@ hid_get_item(hid_data_t s, hid_item_t *h
}
int
+hid_get_item(hid_data_t s, hid_item_t *h)
+{
+ int r;
+
+ for (;;) {
+ r = hid_get_item_raw(s, h);
+ if (r = 0 || s-reportid == -1 || h-report_ID == s-reportid)
+ break;
+ }
+ return (r);
+}
+
+int
hid_report_size(report_desc_t r, enum hid_kind k, int id)
{
struct hid_data *d;
@@ -523,7 +538,7 @@ hid_report_size(report_desc_t r, enum hi
memset(h, 0, sizeof h);
for (d = hid_start_parse(r, 1 k, id); hid_get_item(d, h); ) {
- if ((h.report_ID == id || id 0) h.kind == k) {
+ if (h.kind == k) {
/* compute minimum */
if (lpos h.pos)
lpos = h.pos;
Modified: head/lib/libusbhid/usbhid.3
==
--- head/lib/libusbhid/usbhid.3Thu Sep 20 18:42:00 2012
(r240761)
+++ head/lib/libusbhid/usbhid.3Thu Sep 20 18:56:27 2012
(r240762)
@@ -144,16 +144,15 @@ fails it will return
.Ss Descriptor Parsing Functions
To parse the report descriptor the
.Fn hid_start_parse
-function should be called with a report descriptor and a set that
-describes which items that are interesting.
+function should be called with a report descriptor, a set that
+describes which items that are interesting, and the desired report
+ID (or -1 to obtain items of all report IDs).
The set is obtained by OR-ing together values
.Fa (1 k)
where
.Fa k
is an item of type
.Vt
