* [PATCH 01/12] HID: uhid: simplify report-cb shutdown
2014-07-29 15:14 [PATCH 00/12] HID: Convert UHID to new HID transport-layer David Herrmann
@ 2014-07-29 15:14 ` David Herrmann
2014-07-29 15:14 ` [PATCH 02/12] HID: uhid: forward create_req to create2_req David Herrmann
` (12 subsequent siblings)
13 siblings, 0 replies; 16+ messages in thread
From: David Herrmann @ 2014-07-29 15:14 UTC (permalink / raw)
To: linux-input; +Cc: Jiri Kosina, Benjamin Tissoires, David Herrmann
The report-query is blocking, so when user-space destroys a device we have
to wake up any blocking kernel context that is currently in the report-cb.
We used some broken correlation between @report_done and @running so far.
Replace it by a much more obvious use.
We now wake up the report-cb if either @report_done or @running is set.
wake_up() and wait_event() serve as implicit barriers (as they always do)
so no need to use smp_rmb/wmb directly.
Note that @report_done is never reset by anyone but the report-cb, thus
it cannot flip twice while we wait for it. And whenever we set @running,
we afterwards synchronously remove the HID device. Therefore, we wait for
all report-cbs to finish before we return. This way, @running can never
flip to true while we wait for it.
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
drivers/hid/uhid.c | 11 ++---------
1 file changed, 2 insertions(+), 9 deletions(-)
diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
index 0cb92e3..16af4d3 100644
--- a/drivers/hid/uhid.c
+++ b/drivers/hid/uhid.c
@@ -172,13 +172,9 @@ static int uhid_hid_get_raw(struct hid_device *hid, unsigned char rnum,
spin_unlock_irqrestore(&uhid->qlock, flags);
ret = wait_event_interruptible_timeout(uhid->report_wait,
- atomic_read(&uhid->report_done), 5 * HZ);
+ atomic_read(&uhid->report_done) || !uhid->running,
+ 5 * HZ);
- /*
- * Make sure "uhid->running" is cleared on shutdown before
- * "uhid->report_done" is set.
- */
- smp_rmb();
if (!ret || !uhid->running) {
ret = -EIO;
} else if (ret < 0) {
@@ -493,10 +489,7 @@ static int uhid_dev_destroy(struct uhid_device *uhid)
if (!uhid->running)
return -EINVAL;
- /* clear "running" before setting "report_done" */
uhid->running = false;
- smp_wmb();
- atomic_set(&uhid->report_done, 1);
wake_up_interruptible(&uhid->report_wait);
hid_destroy_device(uhid->hid);
--
2.0.3
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH 02/12] HID: uhid: forward create_req to create2_req
2014-07-29 15:14 [PATCH 00/12] HID: Convert UHID to new HID transport-layer David Herrmann
2014-07-29 15:14 ` [PATCH 01/12] HID: uhid: simplify report-cb shutdown David Herrmann
@ 2014-07-29 15:14 ` David Herrmann
2014-07-29 15:14 ` [PATCH 03/12] HID: uhid: avoid dangling pointers in uhid context David Herrmann
` (11 subsequent siblings)
13 siblings, 0 replies; 16+ messages in thread
From: David Herrmann @ 2014-07-29 15:14 UTC (permalink / raw)
To: linux-input; +Cc: Jiri Kosina, Benjamin Tissoires, David Herrmann
Instead of hard-coding the uhid_dev_create() function twice, copy any
create_req into a create2_req structure and forward it.
We allocate uhid_create_req on the stack here, but that should be fine.
Unlike uhid_create2_req it is fairly small (<1KB) and it's only used
temporarily to swap entries. uhid_dev_create2() doesn't access it.
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
drivers/hid/uhid.c | 90 +++++++++++++++---------------------------------------
1 file changed, 25 insertions(+), 65 deletions(-)
diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
index 16af4d3..c05b544 100644
--- a/drivers/hid/uhid.c
+++ b/drivers/hid/uhid.c
@@ -359,71 +359,6 @@ static int uhid_event_from_user(const char __user *buffer, size_t len,
}
#endif
-static int uhid_dev_create(struct uhid_device *uhid,
- const struct uhid_event *ev)
-{
- struct hid_device *hid;
- int ret;
-
- if (uhid->running)
- return -EALREADY;
-
- uhid->rd_size = ev->u.create.rd_size;
- if (uhid->rd_size <= 0 || uhid->rd_size > HID_MAX_DESCRIPTOR_SIZE)
- return -EINVAL;
-
- uhid->rd_data = kmalloc(uhid->rd_size, GFP_KERNEL);
- if (!uhid->rd_data)
- return -ENOMEM;
-
- if (copy_from_user(uhid->rd_data, ev->u.create.rd_data,
- uhid->rd_size)) {
- ret = -EFAULT;
- goto err_free;
- }
-
- hid = hid_allocate_device();
- if (IS_ERR(hid)) {
- ret = PTR_ERR(hid);
- goto err_free;
- }
-
- strncpy(hid->name, ev->u.create.name, 127);
- hid->name[127] = 0;
- strncpy(hid->phys, ev->u.create.phys, 63);
- hid->phys[63] = 0;
- strncpy(hid->uniq, ev->u.create.uniq, 63);
- hid->uniq[63] = 0;
-
- hid->ll_driver = &uhid_hid_driver;
- hid->bus = ev->u.create.bus;
- hid->vendor = ev->u.create.vendor;
- hid->product = ev->u.create.product;
- hid->version = ev->u.create.version;
- hid->country = ev->u.create.country;
- hid->driver_data = uhid;
- hid->dev.parent = uhid_misc.this_device;
-
- uhid->hid = hid;
- uhid->running = true;
-
- ret = hid_add_device(hid);
- if (ret) {
- hid_err(hid, "Cannot register HID device\n");
- goto err_hid;
- }
-
- return 0;
-
-err_hid:
- hid_destroy_device(hid);
- uhid->hid = NULL;
- uhid->running = false;
-err_free:
- kfree(uhid->rd_data);
- return ret;
-}
-
static int uhid_dev_create2(struct uhid_device *uhid,
const struct uhid_event *ev)
{
@@ -484,6 +419,31 @@ err_free:
return ret;
}
+static int uhid_dev_create(struct uhid_device *uhid,
+ struct uhid_event *ev)
+{
+ struct uhid_create_req orig;
+
+ orig = ev->u.create;
+
+ if (orig.rd_size <= 0 || orig.rd_size > HID_MAX_DESCRIPTOR_SIZE)
+ return -EINVAL;
+ if (copy_from_user(&ev->u.create2.rd_data, orig.rd_data, orig.rd_size))
+ return -EFAULT;
+
+ memcpy(ev->u.create2.name, orig.name, sizeof(orig.name));
+ memcpy(ev->u.create2.phys, orig.phys, sizeof(orig.phys));
+ memcpy(ev->u.create2.uniq, orig.uniq, sizeof(orig.uniq));
+ ev->u.create2.rd_size = orig.rd_size;
+ ev->u.create2.bus = orig.bus;
+ ev->u.create2.vendor = orig.vendor;
+ ev->u.create2.product = orig.product;
+ ev->u.create2.version = orig.version;
+ ev->u.create2.country = orig.country;
+
+ return uhid_dev_create2(uhid, ev);
+}
+
static int uhid_dev_destroy(struct uhid_device *uhid)
{
if (!uhid->running)
--
2.0.3
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH 03/12] HID: uhid: avoid dangling pointers in uhid context
2014-07-29 15:14 [PATCH 00/12] HID: Convert UHID to new HID transport-layer David Herrmann
2014-07-29 15:14 ` [PATCH 01/12] HID: uhid: simplify report-cb shutdown David Herrmann
2014-07-29 15:14 ` [PATCH 02/12] HID: uhid: forward create_req to create2_req David Herrmann
@ 2014-07-29 15:14 ` David Herrmann
2014-07-29 15:14 ` [PATCH 04/12] HID: uhid: avoid magic-numbers when setting strings David Herrmann
` (10 subsequent siblings)
13 siblings, 0 replies; 16+ messages in thread
From: David Herrmann @ 2014-07-29 15:14 UTC (permalink / raw)
To: linux-input; +Cc: Jiri Kosina, Benjamin Tissoires, David Herrmann
Avoid keeping uhid->rd_data and uhid->rd_size set in case
uhid_dev_create2() fails. This is non-critical as we never flip
uhid->running and thus never enter uhid_dev_destroy(). However, it's much
nicer for debugging if pointers are only set if they point to valid data.
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
drivers/hid/uhid.c | 16 +++++++++++-----
1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
index c05b544..bf13746 100644
--- a/drivers/hid/uhid.c
+++ b/drivers/hid/uhid.c
@@ -363,20 +363,24 @@ static int uhid_dev_create2(struct uhid_device *uhid,
const struct uhid_event *ev)
{
struct hid_device *hid;
+ size_t rd_size;
+ void *rd_data;
int ret;
if (uhid->running)
return -EALREADY;
- uhid->rd_size = ev->u.create2.rd_size;
- if (uhid->rd_size <= 0 || uhid->rd_size > HID_MAX_DESCRIPTOR_SIZE)
+ rd_size = ev->u.create2.rd_size;
+ if (rd_size <= 0 || rd_size > HID_MAX_DESCRIPTOR_SIZE)
return -EINVAL;
- uhid->rd_data = kmemdup(ev->u.create2.rd_data, uhid->rd_size,
- GFP_KERNEL);
- if (!uhid->rd_data)
+ rd_data = kmemdup(ev->u.create2.rd_data, rd_size, GFP_KERNEL);
+ if (!rd_data)
return -ENOMEM;
+ uhid->rd_size = rd_size;
+ uhid->rd_data = rd_data;
+
hid = hid_allocate_device();
if (IS_ERR(hid)) {
ret = PTR_ERR(hid);
@@ -416,6 +420,8 @@ err_hid:
uhid->running = false;
err_free:
kfree(uhid->rd_data);
+ uhid->rd_data = NULL;
+ uhid->rd_size = 0;
return ret;
}
--
2.0.3
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH 04/12] HID: uhid: avoid magic-numbers when setting strings
2014-07-29 15:14 [PATCH 00/12] HID: Convert UHID to new HID transport-layer David Herrmann
` (2 preceding siblings ...)
2014-07-29 15:14 ` [PATCH 03/12] HID: uhid: avoid dangling pointers in uhid context David Herrmann
@ 2014-07-29 15:14 ` David Herrmann
2014-07-29 15:14 ` [PATCH 05/12] HID: uhid: turn report_id into u32 David Herrmann
` (9 subsequent siblings)
13 siblings, 0 replies; 16+ messages in thread
From: David Herrmann @ 2014-07-29 15:14 UTC (permalink / raw)
To: linux-input; +Cc: Jiri Kosina, Benjamin Tissoires, David Herrmann
Avoid hard-coding the target buffer sizes and use sizeof() instead. This
also makes us future-proof to buffer-extensions later on.
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
drivers/hid/uhid.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
index bf13746..5dee8bd 100644
--- a/drivers/hid/uhid.c
+++ b/drivers/hid/uhid.c
@@ -363,7 +363,7 @@ static int uhid_dev_create2(struct uhid_device *uhid,
const struct uhid_event *ev)
{
struct hid_device *hid;
- size_t rd_size;
+ size_t rd_size, len;
void *rd_data;
int ret;
@@ -387,12 +387,12 @@ static int uhid_dev_create2(struct uhid_device *uhid,
goto err_free;
}
- strncpy(hid->name, ev->u.create2.name, 127);
- hid->name[127] = 0;
- strncpy(hid->phys, ev->u.create2.phys, 63);
- hid->phys[63] = 0;
- strncpy(hid->uniq, ev->u.create2.uniq, 63);
- hid->uniq[63] = 0;
+ len = min(sizeof(hid->name), sizeof(ev->u.create2.name)) - 1;
+ strncpy(hid->name, ev->u.create2.name, len);
+ len = min(sizeof(hid->phys), sizeof(ev->u.create2.phys)) - 1;
+ strncpy(hid->phys, ev->u.create2.phys, len);
+ len = min(sizeof(hid->uniq), sizeof(ev->u.create2.uniq)) - 1;
+ strncpy(hid->uniq, ev->u.create2.uniq, len);
hid->ll_driver = &uhid_hid_driver;
hid->bus = ev->u.create2.bus;
--
2.0.3
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH 05/12] HID: uhid: turn report_id into u32
2014-07-29 15:14 [PATCH 00/12] HID: Convert UHID to new HID transport-layer David Herrmann
` (3 preceding siblings ...)
2014-07-29 15:14 ` [PATCH 04/12] HID: uhid: avoid magic-numbers when setting strings David Herrmann
@ 2014-07-29 15:14 ` David Herrmann
2014-07-29 15:14 ` [PATCH 06/12] HID: uhid: invert report_done and make non-atomic David Herrmann
` (8 subsequent siblings)
13 siblings, 0 replies; 16+ messages in thread
From: David Herrmann @ 2014-07-29 15:14 UTC (permalink / raw)
To: linux-input; +Cc: Jiri Kosina, Benjamin Tissoires, David Herrmann
All accesses to @report_id are protected by @qlock. No need to use an
atomic.
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
drivers/hid/uhid.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
index 5dee8bd..db4e119 100644
--- a/drivers/hid/uhid.c
+++ b/drivers/hid/uhid.c
@@ -44,10 +44,11 @@ struct uhid_device {
__u8 tail;
struct uhid_event *outq[UHID_BUFSIZE];
+ /* blocking GET_REPORT support; state changes protected by qlock */
struct mutex report_lock;
wait_queue_head_t report_wait;
atomic_t report_done;
- atomic_t report_id;
+ u32 report_id;
struct uhid_event report_buf;
};
@@ -163,7 +164,7 @@ static int uhid_hid_get_raw(struct hid_device *hid, unsigned char rnum,
spin_lock_irqsave(&uhid->qlock, flags);
ev->type = UHID_FEATURE;
- ev->u.feature.id = atomic_inc_return(&uhid->report_id);
+ ev->u.feature.id = ++uhid->report_id;
ev->u.feature.rnum = rnum;
ev->u.feature.rtype = report_type;
@@ -497,7 +498,7 @@ static int uhid_dev_feature_answer(struct uhid_device *uhid,
spin_lock_irqsave(&uhid->qlock, flags);
/* id for old report; drop it silently */
- if (atomic_read(&uhid->report_id) != ev->u.feature_answer.id)
+ if (uhid->report_id != ev->u.feature_answer.id)
goto unlock;
if (atomic_read(&uhid->report_done))
goto unlock;
--
2.0.3
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH 06/12] HID: uhid: invert report_done and make non-atomic
2014-07-29 15:14 [PATCH 00/12] HID: Convert UHID to new HID transport-layer David Herrmann
` (4 preceding siblings ...)
2014-07-29 15:14 ` [PATCH 05/12] HID: uhid: turn report_id into u32 David Herrmann
@ 2014-07-29 15:14 ` David Herrmann
2014-07-29 15:14 ` [PATCH 07/12] HID: uhid: add ABI compatible UHID_GET_REPORT replacing UHID_FEATURE David Herrmann
` (7 subsequent siblings)
13 siblings, 0 replies; 16+ messages in thread
From: David Herrmann @ 2014-07-29 15:14 UTC (permalink / raw)
To: linux-input; +Cc: Jiri Kosina, Benjamin Tissoires, David Herrmann
All accesses to @report_done are protected by qlock (or report-contexts).
No need to use an atomic.
While at it, invert the logic and call it "report_running". This is
similar to the uhid->running field and easier to read.
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
drivers/hid/uhid.c | 15 +++++++--------
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
index db4e119..2d2025a 100644
--- a/drivers/hid/uhid.c
+++ b/drivers/hid/uhid.c
@@ -47,7 +47,7 @@ struct uhid_device {
/* blocking GET_REPORT support; state changes protected by qlock */
struct mutex report_lock;
wait_queue_head_t report_wait;
- atomic_t report_done;
+ bool report_running;
u32 report_id;
struct uhid_event report_buf;
};
@@ -168,13 +168,13 @@ static int uhid_hid_get_raw(struct hid_device *hid, unsigned char rnum,
ev->u.feature.rnum = rnum;
ev->u.feature.rtype = report_type;
- atomic_set(&uhid->report_done, 0);
+ uhid->report_running = true;
uhid_queue(uhid, ev);
spin_unlock_irqrestore(&uhid->qlock, flags);
ret = wait_event_interruptible_timeout(uhid->report_wait,
- atomic_read(&uhid->report_done) || !uhid->running,
- 5 * HZ);
+ !uhid->report_running || !uhid->running,
+ 5 * HZ);
if (!ret || !uhid->running) {
ret = -EIO;
@@ -196,7 +196,7 @@ static int uhid_hid_get_raw(struct hid_device *hid, unsigned char rnum,
spin_unlock_irqrestore(&uhid->qlock, flags);
}
- atomic_set(&uhid->report_done, 1);
+ uhid->report_running = false;
unlock:
mutex_unlock(&uhid->report_lock);
@@ -500,11 +500,11 @@ static int uhid_dev_feature_answer(struct uhid_device *uhid,
/* id for old report; drop it silently */
if (uhid->report_id != ev->u.feature_answer.id)
goto unlock;
- if (atomic_read(&uhid->report_done))
+ if (!uhid->report_running)
goto unlock;
memcpy(&uhid->report_buf, ev, sizeof(*ev));
- atomic_set(&uhid->report_done, 1);
+ uhid->report_running = false;
wake_up_interruptible(&uhid->report_wait);
unlock:
@@ -526,7 +526,6 @@ static int uhid_char_open(struct inode *inode, struct file *file)
init_waitqueue_head(&uhid->waitq);
init_waitqueue_head(&uhid->report_wait);
uhid->running = false;
- atomic_set(&uhid->report_done, 1);
file->private_data = uhid;
nonseekable_open(inode, file);
--
2.0.3
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH 07/12] HID: uhid: add ABI compatible UHID_GET_REPORT replacing UHID_FEATURE
2014-07-29 15:14 [PATCH 00/12] HID: Convert UHID to new HID transport-layer David Herrmann
` (5 preceding siblings ...)
2014-07-29 15:14 ` [PATCH 06/12] HID: uhid: invert report_done and make non-atomic David Herrmann
@ 2014-07-29 15:14 ` David Herrmann
2014-07-29 15:14 ` [PATCH 08/12] HID: uhid: keep legacy definitions at the bottom of uhid.h David Herrmann
` (6 subsequent siblings)
13 siblings, 0 replies; 16+ messages in thread
From: David Herrmann @ 2014-07-29 15:14 UTC (permalink / raw)
To: linux-input; +Cc: Jiri Kosina, Benjamin Tissoires, David Herrmann
The old hdev->hid_get_raw_report() was broken by design. It was never
clear what kind of HW request it should trigger. Benjamin fixed that with
the core HID cleanup, though we never really adjusted uhid.
Unfortunately, our old UHID_FEATURE command was modelled around the broken
hid_get_raw_report(). We converted it silently to the new GET_REPORT and
nothing broke. Make this explicit by renaming UHID_FEATURE to
UHID_GET_REPORT and UHID_FEATURE_ANSWER to UHID_GET_REPORT_REPLY.
Note that this is 100% ABI compatible to UHID_FEATURE. This is just a
rename. But we have to keep the old definitions around to not break API.
>From now on, UHID_GET_REPORT must trigger a GET_REPORT request on the
user-space hardware layer. All the ambiguity due to the weird "feature"
name should be gone now.
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
drivers/hid/uhid.c | 28 ++++++++++++++--------------
include/uapi/linux/uhid.h | 23 +++++++++++++++++++++--
2 files changed, 35 insertions(+), 16 deletions(-)
diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
index 2d2025a..8f5e46b 100644
--- a/drivers/hid/uhid.c
+++ b/drivers/hid/uhid.c
@@ -124,8 +124,8 @@ static int uhid_hid_parse(struct hid_device *hid)
return hid_parse_report(hid, uhid->rd_data, uhid->rd_size);
}
-static int uhid_hid_get_raw(struct hid_device *hid, unsigned char rnum,
- __u8 *buf, size_t count, unsigned char rtype)
+static int uhid_hid_get_report(struct hid_device *hid, unsigned char rnum,
+ __u8 *buf, size_t count, unsigned char rtype)
{
struct uhid_device *uhid = hid->driver_data;
__u8 report_type;
@@ -133,7 +133,7 @@ static int uhid_hid_get_raw(struct hid_device *hid, unsigned char rnum,
unsigned long flags;
int ret;
size_t uninitialized_var(len);
- struct uhid_feature_answer_req *req;
+ struct uhid_get_report_reply_req *req;
if (!uhid->running)
return -EIO;
@@ -163,10 +163,10 @@ static int uhid_hid_get_raw(struct hid_device *hid, unsigned char rnum,
}
spin_lock_irqsave(&uhid->qlock, flags);
- ev->type = UHID_FEATURE;
- ev->u.feature.id = ++uhid->report_id;
- ev->u.feature.rnum = rnum;
- ev->u.feature.rtype = report_type;
+ ev->type = UHID_GET_REPORT;
+ ev->u.get_report.id = ++uhid->report_id;
+ ev->u.get_report.rnum = rnum;
+ ev->u.get_report.rtype = report_type;
uhid->report_running = true;
uhid_queue(uhid, ev);
@@ -182,7 +182,7 @@ static int uhid_hid_get_raw(struct hid_device *hid, unsigned char rnum,
ret = -ERESTARTSYS;
} else {
spin_lock_irqsave(&uhid->qlock, flags);
- req = &uhid->report_buf.u.feature_answer;
+ req = &uhid->report_buf.u.get_report_reply;
if (req->err) {
ret = -EIO;
@@ -253,7 +253,7 @@ static int uhid_raw_request(struct hid_device *hid, unsigned char reportnum,
{
switch (reqtype) {
case HID_REQ_GET_REPORT:
- return uhid_hid_get_raw(hid, reportnum, buf, len, rtype);
+ return uhid_hid_get_report(hid, reportnum, buf, len, rtype);
case HID_REQ_SET_REPORT:
/* TODO: implement proper SET_REPORT functionality */
return -ENOSYS;
@@ -487,8 +487,8 @@ static int uhid_dev_input2(struct uhid_device *uhid, struct uhid_event *ev)
return 0;
}
-static int uhid_dev_feature_answer(struct uhid_device *uhid,
- struct uhid_event *ev)
+static int uhid_dev_get_report_reply(struct uhid_device *uhid,
+ struct uhid_event *ev)
{
unsigned long flags;
@@ -498,7 +498,7 @@ static int uhid_dev_feature_answer(struct uhid_device *uhid,
spin_lock_irqsave(&uhid->qlock, flags);
/* id for old report; drop it silently */
- if (uhid->report_id != ev->u.feature_answer.id)
+ if (uhid->report_id != ev->u.get_report_reply.id)
goto unlock;
if (!uhid->report_running)
goto unlock;
@@ -634,8 +634,8 @@ static ssize_t uhid_char_write(struct file *file, const char __user *buffer,
case UHID_INPUT2:
ret = uhid_dev_input2(uhid, &uhid->input_buf);
break;
- case UHID_FEATURE_ANSWER:
- ret = uhid_dev_feature_answer(uhid, &uhid->input_buf);
+ case UHID_GET_REPORT_REPLY:
+ ret = uhid_dev_get_report_reply(uhid, &uhid->input_buf);
break;
default:
ret = -EOPNOTSUPP;
diff --git a/include/uapi/linux/uhid.h b/include/uapi/linux/uhid.h
index 1e3b09c..0a08f2b 100644
--- a/include/uapi/linux/uhid.h
+++ b/include/uapi/linux/uhid.h
@@ -33,8 +33,10 @@ enum uhid_event_type {
UHID_OUTPUT,
UHID_OUTPUT_EV, /* obsolete! */
UHID_INPUT,
- UHID_FEATURE,
- UHID_FEATURE_ANSWER,
+ UHID_FEATURE, /* obsolete! use UHID_GET_REPORT */
+ UHID_GET_REPORT = UHID_FEATURE,
+ UHID_FEATURE_ANSWER, /* obsolete! use UHID_GET_REPORT_REPLY */
+ UHID_GET_REPORT_REPLY = UHID_FEATURE_ANSWER,
UHID_CREATE2,
UHID_INPUT2,
};
@@ -98,12 +100,20 @@ struct uhid_output_ev_req {
__s32 value;
} __attribute__((__packed__));
+/* Obsolete! Kernel uses ABI compatible UHID_GET_REPORT. */
struct uhid_feature_req {
__u32 id;
__u8 rnum;
__u8 rtype;
} __attribute__((__packed__));
+struct uhid_get_report_req {
+ __u32 id;
+ __u8 rnum;
+ __u8 rtype;
+} __attribute__((__packed__));
+
+/* Obsolete! Use ABI compatible UHID_GET_REPORT_REPLY. */
struct uhid_feature_answer_req {
__u32 id;
__u16 err;
@@ -111,6 +121,13 @@ struct uhid_feature_answer_req {
__u8 data[UHID_DATA_MAX];
} __attribute__((__packed__));
+struct uhid_get_report_reply_req {
+ __u32 id;
+ __u16 err;
+ __u16 size;
+ __u8 data[UHID_DATA_MAX];
+} __attribute__((__packed__));
+
struct uhid_event {
__u32 type;
@@ -120,7 +137,9 @@ struct uhid_event {
struct uhid_output_req output;
struct uhid_output_ev_req output_ev;
struct uhid_feature_req feature;
+ struct uhid_get_report_req get_report;
struct uhid_feature_answer_req feature_answer;
+ struct uhid_get_report_reply_req get_report_reply;
struct uhid_create2_req create2;
struct uhid_input2_req input2;
} u;
--
2.0.3
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH 08/12] HID: uhid: keep legacy definitions at the bottom of uhid.h
2014-07-29 15:14 [PATCH 00/12] HID: Convert UHID to new HID transport-layer David Herrmann
` (6 preceding siblings ...)
2014-07-29 15:14 ` [PATCH 07/12] HID: uhid: add ABI compatible UHID_GET_REPORT replacing UHID_FEATURE David Herrmann
@ 2014-07-29 15:14 ` David Herrmann
2014-07-29 15:14 ` [PATCH 09/12] HID: uhid: rename uhid_raw_request to uhid_hid_raw_request David Herrmann
` (5 subsequent siblings)
13 siblings, 0 replies; 16+ messages in thread
From: David Herrmann @ 2014-07-29 15:14 UTC (permalink / raw)
To: linux-input; +Cc: Jiri Kosina, Benjamin Tissoires, David Herrmann
Instead of inlining the legacy definitions into the main part of uhid.h,
keep them at the bottom now. This way, the API is much easier to read and
legacy requests can be looked up at a separate place.
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
include/uapi/linux/uhid.h | 101 ++++++++++++++++++++++++++++------------------
1 file changed, 62 insertions(+), 39 deletions(-)
diff --git a/include/uapi/linux/uhid.h b/include/uapi/linux/uhid.h
index 0a08f2b..116536e 100644
--- a/include/uapi/linux/uhid.h
+++ b/include/uapi/linux/uhid.h
@@ -24,37 +24,21 @@
#include <linux/hid.h>
enum uhid_event_type {
- UHID_CREATE,
+ __UHID_LEGACY_CREATE,
UHID_DESTROY,
UHID_START,
UHID_STOP,
UHID_OPEN,
UHID_CLOSE,
UHID_OUTPUT,
- UHID_OUTPUT_EV, /* obsolete! */
- UHID_INPUT,
- UHID_FEATURE, /* obsolete! use UHID_GET_REPORT */
- UHID_GET_REPORT = UHID_FEATURE,
- UHID_FEATURE_ANSWER, /* obsolete! use UHID_GET_REPORT_REPLY */
- UHID_GET_REPORT_REPLY = UHID_FEATURE_ANSWER,
+ __UHID_LEGACY_OUTPUT_EV,
+ __UHID_LEGACY_INPUT,
+ UHID_GET_REPORT,
+ UHID_GET_REPORT_REPLY,
UHID_CREATE2,
UHID_INPUT2,
};
-struct uhid_create_req {
- __u8 name[128];
- __u8 phys[64];
- __u8 uniq[64];
- __u8 __user *rd_data;
- __u16 rd_size;
-
- __u16 bus;
- __u32 vendor;
- __u32 product;
- __u32 version;
- __u32 country;
-} __attribute__((__packed__));
-
struct uhid_create2_req {
__u8 name[128];
__u8 phys[64];
@@ -76,24 +60,67 @@ enum uhid_report_type {
UHID_INPUT_REPORT,
};
-struct uhid_input_req {
+struct uhid_input2_req {
+ __u16 size;
+ __u8 data[UHID_DATA_MAX];
+} __attribute__((__packed__));
+
+struct uhid_output_req {
__u8 data[UHID_DATA_MAX];
__u16 size;
+ __u8 rtype;
} __attribute__((__packed__));
-struct uhid_input2_req {
+struct uhid_get_report_req {
+ __u32 id;
+ __u8 rnum;
+ __u8 rtype;
+} __attribute__((__packed__));
+
+struct uhid_get_report_reply_req {
+ __u32 id;
+ __u16 err;
__u16 size;
__u8 data[UHID_DATA_MAX];
} __attribute__((__packed__));
-struct uhid_output_req {
+/*
+ * Compat Layer
+ * All these commands and requests are obsolete. You should avoid using them in
+ * new code. We support them for backwards-compatibility, but you might not get
+ * access to new feature in case you use them.
+ */
+
+enum uhid_legacy_event_type {
+ UHID_CREATE = __UHID_LEGACY_CREATE,
+ UHID_OUTPUT_EV = __UHID_LEGACY_OUTPUT_EV,
+ UHID_INPUT = __UHID_LEGACY_INPUT,
+ UHID_FEATURE = UHID_GET_REPORT,
+ UHID_FEATURE_ANSWER = UHID_GET_REPORT_REPLY,
+};
+
+/* Obsolete! Use UHID_CREATE2. */
+struct uhid_create_req {
+ __u8 name[128];
+ __u8 phys[64];
+ __u8 uniq[64];
+ __u8 __user *rd_data;
+ __u16 rd_size;
+
+ __u16 bus;
+ __u32 vendor;
+ __u32 product;
+ __u32 version;
+ __u32 country;
+} __attribute__((__packed__));
+
+/* Obsolete! Use UHID_INPUT2. */
+struct uhid_input_req {
__u8 data[UHID_DATA_MAX];
__u16 size;
- __u8 rtype;
} __attribute__((__packed__));
-/* Obsolete! Newer kernels will no longer send these events but instead convert
- * it into raw output reports via UHID_OUTPUT. */
+/* Obsolete! Kernel uses UHID_OUTPUT exclusively now. */
struct uhid_output_ev_req {
__u16 type;
__u16 code;
@@ -107,12 +134,6 @@ struct uhid_feature_req {
__u8 rtype;
} __attribute__((__packed__));
-struct uhid_get_report_req {
- __u32 id;
- __u8 rnum;
- __u8 rtype;
-} __attribute__((__packed__));
-
/* Obsolete! Use ABI compatible UHID_GET_REPORT_REPLY. */
struct uhid_feature_answer_req {
__u32 id;
@@ -121,12 +142,14 @@ struct uhid_feature_answer_req {
__u8 data[UHID_DATA_MAX];
} __attribute__((__packed__));
-struct uhid_get_report_reply_req {
- __u32 id;
- __u16 err;
- __u16 size;
- __u8 data[UHID_DATA_MAX];
-} __attribute__((__packed__));
+/*
+ * UHID Events
+ * All UHID events from and to the kernel are encoded as "struct uhid_event".
+ * The "type" field contains a UHID_* type identifier. All payload depends on
+ * that type and can be accessed via ev->u.XYZ accordingly.
+ * If user-space writes short events, they're extended with 0s by the kernel. If
+ * the kernel writes short events, user-space shall extend them with 0s.
+ */
struct uhid_event {
__u32 type;
--
2.0.3
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH 09/12] HID: uhid: rename uhid_raw_request to uhid_hid_raw_request
2014-07-29 15:14 [PATCH 00/12] HID: Convert UHID to new HID transport-layer David Herrmann
` (7 preceding siblings ...)
2014-07-29 15:14 ` [PATCH 08/12] HID: uhid: keep legacy definitions at the bottom of uhid.h David Herrmann
@ 2014-07-29 15:14 ` David Herrmann
2014-07-29 15:14 ` [PATCH 10/12] HID: uhid: implement SET_REPORT David Herrmann
` (4 subsequent siblings)
13 siblings, 0 replies; 16+ messages in thread
From: David Herrmann @ 2014-07-29 15:14 UTC (permalink / raw)
To: linux-input; +Cc: Jiri Kosina, Benjamin Tissoires, David Herrmann
We use strict prefixed in uhid.c:
uhid_char_*: implement char-dev callbacks
uhid_dev_*: implement uhid device management and runtime
uhid_hid_*: implement hid-dev callbacks
uhid_raw_request is an hid callback, so rename it to uhid_hid_raw_request.
While at it, move it closer to it's extracted helpers and keep the same
order as in "struct hid_driver".
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
drivers/hid/uhid.c | 32 ++++++++++++++++----------------
1 file changed, 16 insertions(+), 16 deletions(-)
diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
index 8f5e46b..8bf613e 100644
--- a/drivers/hid/uhid.c
+++ b/drivers/hid/uhid.c
@@ -203,6 +203,21 @@ unlock:
return ret ? ret : len;
}
+static int uhid_hid_raw_request(struct hid_device *hid, unsigned char reportnum,
+ __u8 *buf, size_t len, unsigned char rtype,
+ int reqtype)
+{
+ switch (reqtype) {
+ case HID_REQ_GET_REPORT:
+ return uhid_hid_get_report(hid, reportnum, buf, len, rtype);
+ case HID_REQ_SET_REPORT:
+ /* TODO: implement proper SET_REPORT functionality */
+ return -ENOSYS;
+ default:
+ return -EIO;
+ }
+}
+
static int uhid_hid_output_raw(struct hid_device *hid, __u8 *buf, size_t count,
unsigned char report_type)
{
@@ -247,29 +262,14 @@ static int uhid_hid_output_report(struct hid_device *hid, __u8 *buf,
return uhid_hid_output_raw(hid, buf, count, HID_OUTPUT_REPORT);
}
-static int uhid_raw_request(struct hid_device *hid, unsigned char reportnum,
- __u8 *buf, size_t len, unsigned char rtype,
- int reqtype)
-{
- switch (reqtype) {
- case HID_REQ_GET_REPORT:
- return uhid_hid_get_report(hid, reportnum, buf, len, rtype);
- case HID_REQ_SET_REPORT:
- /* TODO: implement proper SET_REPORT functionality */
- return -ENOSYS;
- default:
- return -EIO;
- }
-}
-
static struct hid_ll_driver uhid_hid_driver = {
.start = uhid_hid_start,
.stop = uhid_hid_stop,
.open = uhid_hid_open,
.close = uhid_hid_close,
.parse = uhid_hid_parse,
+ .raw_request = uhid_hid_raw_request,
.output_report = uhid_hid_output_report,
- .raw_request = uhid_raw_request,
};
#ifdef CONFIG_COMPAT
--
2.0.3
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH 10/12] HID: uhid: implement SET_REPORT
2014-07-29 15:14 [PATCH 00/12] HID: Convert UHID to new HID transport-layer David Herrmann
` (8 preceding siblings ...)
2014-07-29 15:14 ` [PATCH 09/12] HID: uhid: rename uhid_raw_request to uhid_hid_raw_request David Herrmann
@ 2014-07-29 15:14 ` David Herrmann
2014-07-29 15:14 ` [PATCH 11/12] HID: uhid: report to user-space whether reports are numbered David Herrmann
` (3 subsequent siblings)
13 siblings, 0 replies; 16+ messages in thread
From: David Herrmann @ 2014-07-29 15:14 UTC (permalink / raw)
To: linux-input; +Cc: Jiri Kosina, Benjamin Tissoires, David Herrmann
We so far lacked support for hid_hw_raw_request(..., HID_REQ_SET_REPORT);
Add support for it and simply forward the request to user-space. Note that
SET_REPORT is synchronous, just like GET_REPORT, even though it does not
provide any data back besides an error code.
If a transport layer does SET_REPORT asynchronously, they can just ACK it
immediately by writing an uhid_set_report_reply to uhid.
This patch re-uses the synchronous uhid-report infrastructure to query
user-space. Note that this means you cannot run SET_REPORT and GET_REPORT
in parallel. However, that has always been a restriction of HID and due to
its blocking nature, this is just fine. Maybe some future transport layer
supports parallel requests (very unlikely), however, until then lets not
over-complicate things and avoid request-lookup-tables.
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
drivers/hid/uhid.c | 206 +++++++++++++++++++++++++++++++---------------
include/uapi/linux/uhid.h | 17 ++++
2 files changed, 155 insertions(+), 68 deletions(-)
diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
index 8bf613e..1951148 100644
--- a/drivers/hid/uhid.c
+++ b/drivers/hid/uhid.c
@@ -49,6 +49,7 @@ struct uhid_device {
wait_queue_head_t report_wait;
bool report_running;
u32 report_id;
+ u32 report_type;
struct uhid_event report_buf;
};
@@ -124,95 +125,166 @@ static int uhid_hid_parse(struct hid_device *hid)
return hid_parse_report(hid, uhid->rd_data, uhid->rd_size);
}
+/* must be called with report_lock held */
+static int __uhid_report_queue_and_wait(struct uhid_device *uhid,
+ struct uhid_event *ev,
+ __u32 *report_id)
+{
+ unsigned long flags;
+ int ret;
+
+ spin_lock_irqsave(&uhid->qlock, flags);
+ *report_id = ++uhid->report_id;
+ uhid->report_type = ev->type;
+ uhid->report_running = true;
+ uhid_queue(uhid, ev);
+ spin_unlock_irqrestore(&uhid->qlock, flags);
+
+ ret = wait_event_interruptible_timeout(uhid->report_wait,
+ !uhid->report_running || !uhid->running,
+ 5 * HZ);
+ if (!ret || !uhid->running || uhid->report_running)
+ ret = -EIO;
+ else if (ret < 0)
+ ret = -ERESTARTSYS;
+ else
+ ret = 0;
+
+ uhid->report_running = false;
+
+ return ret;
+}
+
+static void uhid_report_wake_up(struct uhid_device *uhid, u32 id,
+ const struct uhid_event *ev)
+{
+ unsigned long flags;
+
+ spin_lock_irqsave(&uhid->qlock, flags);
+
+ /* id for old report; drop it silently */
+ if (uhid->report_type != ev->type || uhid->report_id != id)
+ goto unlock;
+ if (!uhid->report_running)
+ goto unlock;
+
+ memcpy(&uhid->report_buf, ev, sizeof(*ev));
+ uhid->report_running = false;
+ wake_up_interruptible(&uhid->report_wait);
+
+unlock:
+ spin_unlock_irqrestore(&uhid->qlock, flags);
+}
+
static int uhid_hid_get_report(struct hid_device *hid, unsigned char rnum,
- __u8 *buf, size_t count, unsigned char rtype)
+ u8 *buf, size_t count, u8 rtype)
{
struct uhid_device *uhid = hid->driver_data;
- __u8 report_type;
+ struct uhid_get_report_reply_req *req;
struct uhid_event *ev;
- unsigned long flags;
int ret;
- size_t uninitialized_var(len);
- struct uhid_get_report_reply_req *req;
if (!uhid->running)
return -EIO;
- switch (rtype) {
- case HID_FEATURE_REPORT:
- report_type = UHID_FEATURE_REPORT;
- break;
- case HID_OUTPUT_REPORT:
- report_type = UHID_OUTPUT_REPORT;
- break;
- case HID_INPUT_REPORT:
- report_type = UHID_INPUT_REPORT;
- break;
- default:
- return -EINVAL;
- }
+ ev = kzalloc(sizeof(*ev), GFP_KERNEL);
+ if (!ev)
+ return -ENOMEM;
+
+ ev->type = UHID_GET_REPORT;
+ ev->u.get_report.rnum = rnum;
+ ev->u.get_report.rtype = rtype;
ret = mutex_lock_interruptible(&uhid->report_lock);
- if (ret)
+ if (ret) {
+ kfree(ev);
return ret;
+ }
- ev = kzalloc(sizeof(*ev), GFP_KERNEL);
- if (!ev) {
- ret = -ENOMEM;
+ /* this _always_ takes ownership of @ev */
+ ret = __uhid_report_queue_and_wait(uhid, ev, &ev->u.get_report.id);
+ if (ret)
goto unlock;
+
+ req = &uhid->report_buf.u.get_report_reply;
+ if (req->err) {
+ ret = -EIO;
+ } else {
+ ret = min3(count, (size_t)req->size, (size_t)UHID_DATA_MAX);
+ memcpy(buf, req->data, ret);
}
- spin_lock_irqsave(&uhid->qlock, flags);
- ev->type = UHID_GET_REPORT;
- ev->u.get_report.id = ++uhid->report_id;
- ev->u.get_report.rnum = rnum;
- ev->u.get_report.rtype = report_type;
+unlock:
+ mutex_unlock(&uhid->report_lock);
+ return ret;
+}
- uhid->report_running = true;
- uhid_queue(uhid, ev);
- spin_unlock_irqrestore(&uhid->qlock, flags);
+static int uhid_hid_set_report(struct hid_device *hid, unsigned char rnum,
+ const u8 *buf, size_t count, u8 rtype)
+{
+ struct uhid_device *uhid = hid->driver_data;
+ struct uhid_event *ev;
+ int ret;
- ret = wait_event_interruptible_timeout(uhid->report_wait,
- !uhid->report_running || !uhid->running,
- 5 * HZ);
+ if (!uhid->running || count > UHID_DATA_MAX)
+ return -EIO;
- if (!ret || !uhid->running) {
- ret = -EIO;
- } else if (ret < 0) {
- ret = -ERESTARTSYS;
- } else {
- spin_lock_irqsave(&uhid->qlock, flags);
- req = &uhid->report_buf.u.get_report_reply;
+ ev = kzalloc(sizeof(*ev), GFP_KERNEL);
+ if (!ev)
+ return -ENOMEM;
- if (req->err) {
- ret = -EIO;
- } else {
- ret = 0;
- len = min(count,
- min_t(size_t, req->size, UHID_DATA_MAX));
- memcpy(buf, req->data, len);
- }
+ ev->type = UHID_SET_REPORT;
+ ev->u.set_report.rnum = rnum;
+ ev->u.set_report.rtype = rtype;
+ ev->u.set_report.size = count;
+ memcpy(ev->u.set_report.data, buf, count);
- spin_unlock_irqrestore(&uhid->qlock, flags);
+ ret = mutex_lock_interruptible(&uhid->report_lock);
+ if (ret) {
+ kfree(ev);
+ return ret;
}
- uhid->report_running = false;
+ /* this _always_ takes ownership of @ev */
+ ret = __uhid_report_queue_and_wait(uhid, ev, &ev->u.set_report.id);
+ if (ret)
+ goto unlock;
+
+ if (uhid->report_buf.u.set_report_reply.err)
+ ret = -EIO;
+ else
+ ret = count;
unlock:
mutex_unlock(&uhid->report_lock);
- return ret ? ret : len;
+ return ret;
}
static int uhid_hid_raw_request(struct hid_device *hid, unsigned char reportnum,
__u8 *buf, size_t len, unsigned char rtype,
int reqtype)
{
+ u8 u_rtype;
+
+ switch (rtype) {
+ case HID_FEATURE_REPORT:
+ u_rtype = UHID_FEATURE_REPORT;
+ break;
+ case HID_OUTPUT_REPORT:
+ u_rtype = UHID_OUTPUT_REPORT;
+ break;
+ case HID_INPUT_REPORT:
+ u_rtype = UHID_INPUT_REPORT;
+ break;
+ default:
+ return -EINVAL;
+ }
+
switch (reqtype) {
case HID_REQ_GET_REPORT:
- return uhid_hid_get_report(hid, reportnum, buf, len, rtype);
+ return uhid_hid_get_report(hid, reportnum, buf, len, u_rtype);
case HID_REQ_SET_REPORT:
- /* TODO: implement proper SET_REPORT functionality */
- return -ENOSYS;
+ return uhid_hid_set_report(hid, reportnum, buf, len, u_rtype);
default:
return -EIO;
}
@@ -490,25 +562,20 @@ static int uhid_dev_input2(struct uhid_device *uhid, struct uhid_event *ev)
static int uhid_dev_get_report_reply(struct uhid_device *uhid,
struct uhid_event *ev)
{
- unsigned long flags;
-
if (!uhid->running)
return -EINVAL;
- spin_lock_irqsave(&uhid->qlock, flags);
-
- /* id for old report; drop it silently */
- if (uhid->report_id != ev->u.get_report_reply.id)
- goto unlock;
- if (!uhid->report_running)
- goto unlock;
+ uhid_report_wake_up(uhid, ev->u.get_report_reply.id, ev);
+ return 0;
+}
- memcpy(&uhid->report_buf, ev, sizeof(*ev));
- uhid->report_running = false;
- wake_up_interruptible(&uhid->report_wait);
+static int uhid_dev_set_report_reply(struct uhid_device *uhid,
+ struct uhid_event *ev)
+{
+ if (!uhid->running)
+ return -EINVAL;
-unlock:
- spin_unlock_irqrestore(&uhid->qlock, flags);
+ uhid_report_wake_up(uhid, ev->u.set_report_reply.id, ev);
return 0;
}
@@ -637,6 +704,9 @@ static ssize_t uhid_char_write(struct file *file, const char __user *buffer,
case UHID_GET_REPORT_REPLY:
ret = uhid_dev_get_report_reply(uhid, &uhid->input_buf);
break;
+ case UHID_SET_REPORT_REPLY:
+ ret = uhid_dev_set_report_reply(uhid, &uhid->input_buf);
+ break;
default:
ret = -EOPNOTSUPP;
}
diff --git a/include/uapi/linux/uhid.h b/include/uapi/linux/uhid.h
index 116536e..62aac0e 100644
--- a/include/uapi/linux/uhid.h
+++ b/include/uapi/linux/uhid.h
@@ -37,6 +37,8 @@ enum uhid_event_type {
UHID_GET_REPORT_REPLY,
UHID_CREATE2,
UHID_INPUT2,
+ UHID_SET_REPORT,
+ UHID_SET_REPORT_REPLY,
};
struct uhid_create2_req {
@@ -84,6 +86,19 @@ struct uhid_get_report_reply_req {
__u8 data[UHID_DATA_MAX];
} __attribute__((__packed__));
+struct uhid_set_report_req {
+ __u32 id;
+ __u8 rnum;
+ __u8 rtype;
+ __u16 size;
+ __u8 data[UHID_DATA_MAX];
+} __attribute__((__packed__));
+
+struct uhid_set_report_reply_req {
+ __u32 id;
+ __u16 err;
+} __attribute__((__packed__));
+
/*
* Compat Layer
* All these commands and requests are obsolete. You should avoid using them in
@@ -165,6 +180,8 @@ struct uhid_event {
struct uhid_get_report_reply_req get_report_reply;
struct uhid_create2_req create2;
struct uhid_input2_req input2;
+ struct uhid_set_report_req set_report;
+ struct uhid_set_report_reply_req set_report_reply;
} u;
} __attribute__((__packed__));
--
2.0.3
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH 11/12] HID: uhid: report to user-space whether reports are numbered
2014-07-29 15:14 [PATCH 00/12] HID: Convert UHID to new HID transport-layer David Herrmann
` (9 preceding siblings ...)
2014-07-29 15:14 ` [PATCH 10/12] HID: uhid: implement SET_REPORT David Herrmann
@ 2014-07-29 15:14 ` David Herrmann
2014-07-29 15:14 ` [PATCH 12/12] HID: uhid: update documentation David Herrmann
` (2 subsequent siblings)
13 siblings, 0 replies; 16+ messages in thread
From: David Herrmann @ 2014-07-29 15:14 UTC (permalink / raw)
To: linux-input; +Cc: Jiri Kosina, Benjamin Tissoires, David Herrmann
This makes UHID_START include a "dev_flags" field that describes details
of the hid-device in the kernel. The first flags we introduce describe
whether a given report-type uses numbered reports. This is useful for
transport layers that force report-numbers and therefore might have to
prefix kernel-provided HID-messages with the report-number.
Currently, only HoG needs this and the spec only talks about "global
report numbers". That is, it's a global boolean not a per-type boolean.
However, given the quirks we already have in kernel-space, a per-type
value seems much more appropriate.
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
drivers/hid/uhid.c | 21 ++++++++++++++++++++-
include/uapi/linux/uhid.h | 11 +++++++++++
2 files changed, 31 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
index 1951148..f6ec5ea 100644
--- a/drivers/hid/uhid.c
+++ b/drivers/hid/uhid.c
@@ -92,8 +92,27 @@ static int uhid_queue_event(struct uhid_device *uhid, __u32 event)
static int uhid_hid_start(struct hid_device *hid)
{
struct uhid_device *uhid = hid->driver_data;
+ struct uhid_event *ev;
+ unsigned long flags;
+
+ ev = kzalloc(sizeof(*ev), GFP_KERNEL);
+ if (!ev)
+ return -ENOMEM;
+
+ ev->type = UHID_START;
- return uhid_queue_event(uhid, UHID_START);
+ if (hid->report_enum[HID_FEATURE_REPORT].numbered)
+ ev->u.start.dev_flags |= UHID_DEV_NUMBERED_FEATURE_REPORTS;
+ if (hid->report_enum[HID_OUTPUT_REPORT].numbered)
+ ev->u.start.dev_flags |= UHID_DEV_NUMBERED_OUTPUT_REPORTS;
+ if (hid->report_enum[HID_INPUT_REPORT].numbered)
+ ev->u.start.dev_flags |= UHID_DEV_NUMBERED_INPUT_REPORTS;
+
+ spin_lock_irqsave(&uhid->qlock, flags);
+ uhid_queue(uhid, ev);
+ spin_unlock_irqrestore(&uhid->qlock, flags);
+
+ return 0;
}
static void uhid_hid_stop(struct hid_device *hid)
diff --git a/include/uapi/linux/uhid.h b/include/uapi/linux/uhid.h
index 62aac0e..aaa86d6 100644
--- a/include/uapi/linux/uhid.h
+++ b/include/uapi/linux/uhid.h
@@ -54,6 +54,16 @@ struct uhid_create2_req {
__u8 rd_data[HID_MAX_DESCRIPTOR_SIZE];
} __attribute__((__packed__));
+enum uhid_dev_flag {
+ UHID_DEV_NUMBERED_FEATURE_REPORTS = (1ULL << 0),
+ UHID_DEV_NUMBERED_OUTPUT_REPORTS = (1ULL << 1),
+ UHID_DEV_NUMBERED_INPUT_REPORTS = (1ULL << 2),
+};
+
+struct uhid_start_req {
+ __u64 dev_flags;
+};
+
#define UHID_DATA_MAX 4096
enum uhid_report_type {
@@ -182,6 +192,7 @@ struct uhid_event {
struct uhid_input2_req input2;
struct uhid_set_report_req set_report;
struct uhid_set_report_reply_req set_report_reply;
+ struct uhid_start_req start;
} u;
} __attribute__((__packed__));
--
2.0.3
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH 12/12] HID: uhid: update documentation
2014-07-29 15:14 [PATCH 00/12] HID: Convert UHID to new HID transport-layer David Herrmann
` (10 preceding siblings ...)
2014-07-29 15:14 ` [PATCH 11/12] HID: uhid: report to user-space whether reports are numbered David Herrmann
@ 2014-07-29 15:14 ` David Herrmann
2014-07-29 19:00 ` [PATCH 00/12] HID: Convert UHID to new HID transport-layer Jiri Kosina
2014-08-25 8:34 ` Jiri Kosina
13 siblings, 0 replies; 16+ messages in thread
From: David Herrmann @ 2014-07-29 15:14 UTC (permalink / raw)
To: linux-input; +Cc: Jiri Kosina, Benjamin Tissoires, David Herrmann
Remove legacy bits, refer people to hid-transport.txt and add descriptions
for all new features.
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
Documentation/hid/uhid.txt | 179 +++++++++++++++++++++++----------------------
1 file changed, 92 insertions(+), 87 deletions(-)
diff --git a/Documentation/hid/uhid.txt b/Documentation/hid/uhid.txt
index 54c8f97..c8656dd 100644
--- a/Documentation/hid/uhid.txt
+++ b/Documentation/hid/uhid.txt
@@ -1,28 +1,13 @@
UHID - User-space I/O driver support for HID subsystem
========================================================
-The HID subsystem needs two kinds of drivers. In this document we call them:
+UHID allows user-space to implement HID transport drivers. Please see
+hid-transport.txt for an introduction into HID transport drivers. This document
+relies heavily on the definitions declared there.
- 1. The "HID I/O Driver" is the driver that performs raw data I/O to the
- low-level device. Internally, they register an hid_ll_driver structure with
- the HID core. They perform device setup, read raw data from the device and
- push it into the HID subsystem and they provide a callback so the HID
- subsystem can send data to the device.
-
- 2. The "HID Device Driver" is the driver that parses HID reports and reacts on
- them. There are generic drivers like "generic-usb" and "generic-bluetooth"
- which adhere to the HID specification and provide the standardizes features.
- But there may be special drivers and quirks for each non-standard device out
- there. Internally, they use the hid_driver structure.
-
-Historically, the USB stack was the first subsystem to provide an HID I/O
-Driver. However, other standards like Bluetooth have adopted the HID specs and
-may provide HID I/O Drivers, too. The UHID driver allows to implement HID I/O
-Drivers in user-space and feed the data into the kernel HID-subsystem.
-
-This allows user-space to operate on the same level as USB-HID, Bluetooth-HID
-and similar. It does not provide a way to write HID Device Drivers, though. Use
-hidraw for this purpose.
+With UHID, a user-space transport driver can create kernel hid-devices for each
+device connected to the user-space controlled bus. The UHID API defines the I/O
+events provided from the kernel to user-space and vice versa.
There is an example user-space application in ./samples/uhid/uhid-example.c
@@ -42,8 +27,9 @@ by setting O_NONBLOCK.
struct uhid_event {
__u32 type;
union {
- struct uhid_create_req create;
- struct uhid_data_req data;
+ struct uhid_create2_req create2;
+ struct uhid_output_req output;
+ struct uhid_input2_req input2;
...
} u;
};
@@ -54,8 +40,11 @@ multiple write()'s. A single event must always be sent as a whole. Furthermore,
only a single event can be sent per read() or write(). Pending data is ignored.
If you want to handle multiple events in a single syscall, then use vectored
I/O with readv()/writev().
+The "type" field defines the payload. For each type, there is a
+payload-structure available in the union "u" (except for empty payloads). This
+payload contains management and/or device data.
-The first thing you should do is sending an UHID_CREATE event. This will
+The first thing you should do is sending an UHID_CREATE2 event. This will
register the device. UHID will respond with an UHID_START event. You can now
start sending data to and reading data from UHID. However, unless UHID sends the
UHID_OPEN event, the internally attached HID Device Driver has no user attached.
@@ -69,12 +58,20 @@ ref-counting for you.
You may decide to ignore UHID_OPEN/UHID_CLOSE, though. I/O is allowed even
though the device may have no users.
-If you want to send data to the HID subsystem, you send an HID_INPUT event with
-your raw data payload. If the kernel wants to send data to the device, you will
-read an UHID_OUTPUT or UHID_OUTPUT_EV event.
+If you want to send data on the interrupt channel to the HID subsystem, you send
+an HID_INPUT2 event with your raw data payload. If the kernel wants to send data
+on the interrupt channel to the device, you will read an UHID_OUTPUT event.
+Data requests on the control channel are currently limited to GET_REPORT and
+SET_REPORT (no other data reports on the control channel are defined so far).
+Those requests are always synchronous. That means, the kernel sends
+UHID_GET_REPORT and UHID_SET_REPORT events and requires you to forward them to
+the device on the control channel. Once the device responds, you must forward
+the response via UHID_GET_REPORT_REPLY and UHID_SET_REPORT_REPLY to the kernel.
+The kernel blocks internal driver-execution during such round-trips (times out
+after a hard-coded period).
If your device disconnects, you should send an UHID_DESTROY event. This will
-unregister the device. You can now send UHID_CREATE again to register a new
+unregister the device. You can now send UHID_CREATE2 again to register a new
device.
If you close() the fd, the device is automatically unregistered and destroyed
internally.
@@ -82,73 +79,79 @@ internally.
write()
-------
write() allows you to modify the state of the device and feed input data into
-the kernel. The following types are supported: UHID_CREATE, UHID_DESTROY and
-UHID_INPUT. The kernel will parse the event immediately and if the event ID is
+the kernel. The kernel will parse the event immediately and if the event ID is
not supported, it will return -EOPNOTSUPP. If the payload is invalid, then
-EINVAL is returned, otherwise, the amount of data that was read is returned and
-the request was handled successfully.
+the request was handled successfully. O_NONBLOCK does not affect write() as
+writes are always handled immediately in a non-blocking fashion. Future requests
+might make use of O_NONBLOCK, though.
- UHID_CREATE:
+ UHID_CREATE2:
This creates the internal HID device. No I/O is possible until you send this
- event to the kernel. The payload is of type struct uhid_create_req and
+ event to the kernel. The payload is of type struct uhid_create2_req and
contains information about your device. You can start I/O now.
- UHID_CREATE2:
- Same as UHID_CREATE, but the HID report descriptor data (rd_data) is an array
- inside struct uhid_create2_req, instead of a pointer to a separate array.
- Enables use from languages that don't support pointers, e.g. Python.
-
UHID_DESTROY:
This destroys the internal HID device. No further I/O will be accepted. There
may still be pending messages that you can receive with read() but no further
UHID_INPUT events can be sent to the kernel.
- You can create a new device by sending UHID_CREATE again. There is no need to
+ You can create a new device by sending UHID_CREATE2 again. There is no need to
reopen the character device.
- UHID_INPUT:
- You must send UHID_CREATE before sending input to the kernel! This event
- contains a data-payload. This is the raw data that you read from your device.
- The kernel will parse the HID reports and react on it.
-
UHID_INPUT2:
- Same as UHID_INPUT, but the data array is the last field of uhid_input2_req.
- Enables userspace to write only the required bytes to kernel (ev.type +
- ev.u.input2.size + the part of the data array that matters), instead of
- the entire struct uhid_input2_req.
-
- UHID_FEATURE_ANSWER:
- If you receive a UHID_FEATURE request you must answer with this request. You
- must copy the "id" field from the request into the answer. Set the "err" field
- to 0 if no error occurred or to EIO if an I/O error occurred.
+ You must send UHID_CREATE2 before sending input to the kernel! This event
+ contains a data-payload. This is the raw data that you read from your device
+ on the interrupt channel. The kernel will parse the HID reports.
+
+ UHID_GET_REPORT_REPLY:
+ If you receive a UHID_GET_REPORT request you must answer with this request.
+ You must copy the "id" field from the request into the answer. Set the "err"
+ field to 0 if no error occurred or to EIO if an I/O error occurred.
If "err" is 0 then you should fill the buffer of the answer with the results
- of the feature request and set "size" correspondingly.
+ of the GET_REPORT request and set "size" correspondingly.
+
+ UHID_SET_REPORT_REPLY:
+ This is the SET_REPORT equivalent of UHID_GET_REPORT_REPLY. Unlike GET_REPORT,
+ SET_REPORT never returns a data buffer, therefore, it's sufficient to set the
+ "id" and "err" fields correctly.
read()
------
-read() will return a queued output report. These output reports can be of type
-UHID_START, UHID_STOP, UHID_OPEN, UHID_CLOSE, UHID_OUTPUT or UHID_OUTPUT_EV. No
-reaction is required to any of them but you should handle them according to your
-needs. Only UHID_OUTPUT and UHID_OUTPUT_EV have payloads.
+read() will return a queued output report. No reaction is required to any of
+them but you should handle them according to your needs.
UHID_START:
This is sent when the HID device is started. Consider this as an answer to
- UHID_CREATE. This is always the first event that is sent.
+ UHID_CREATE2. This is always the first event that is sent. Note that this
+ event might not be available immediately after write(UHID_CREATE2) returns.
+ Device drivers might required delayed setups.
+ This event contains a payload of type uhid_start_req. The "dev_flags" field
+ describes special behaviors of a device. The following flags are defined:
+ UHID_DEV_NUMBERED_FEATURE_REPORTS:
+ UHID_DEV_NUMBERED_OUTPUT_REPORTS:
+ UHID_DEV_NUMBERED_INPUT_REPORTS:
+ Each of these flags defines whether a given report-type uses numbered
+ reports. If numbered reports are used for a type, all messages from
+ the kernel already have the report-number as prefix. Otherwise, no
+ prefix is added by the kernel.
+ For messages sent by user-space to the kernel, you must adjust the
+ prefixes according to these flags.
UHID_STOP:
This is sent when the HID device is stopped. Consider this as an answer to
UHID_DESTROY.
- If the kernel HID device driver closes the device manually (that is, you
- didn't send UHID_DESTROY) then you should consider this device closed and send
- an UHID_DESTROY event. You may want to reregister your device, though. This is
- always the last message that is sent to you unless you reopen the device with
- UHID_CREATE.
+ If you didn't destroy your device via UHID_DESTROY, but the kernel sends an
+ UHID_STOP event, this should usually be ignored. It means that the kernel
+ reloaded/changed the device driver loaded on your HID device (or some other
+ maintenance actions happened).
+ You can usually ignored any UHID_STOP events safely.
UHID_OPEN:
This is sent when the HID device is opened. That is, the data that the HID
device provides is read by some other process. You may ignore this event but
it is useful for power-management. As long as you haven't received this event
there is actually no other process that reads your data so there is no need to
- send UHID_INPUT events to the kernel.
+ send UHID_INPUT2 events to the kernel.
UHID_CLOSE:
This is sent when there are no more processes which read the HID data. It is
@@ -156,27 +159,29 @@ needs. Only UHID_OUTPUT and UHID_OUTPUT_EV have payloads.
UHID_OUTPUT:
This is sent if the HID device driver wants to send raw data to the I/O
- device. You should read the payload and forward it to the device. The payload
- is of type "struct uhid_data_req".
+ device on the interrupt channel. You should read the payload and forward it to
+ the device. The payload is of type "struct uhid_data_req".
This may be received even though you haven't received UHID_OPEN, yet.
- UHID_OUTPUT_EV (obsolete):
- Same as UHID_OUTPUT but this contains a "struct input_event" as payload. This
- is called for force-feedback, LED or similar events which are received through
- an input device by the HID subsystem. You should convert this into raw reports
- and send them to your device similar to events of type UHID_OUTPUT.
- This is no longer sent by newer kernels. Instead, HID core converts it into a
- raw output report and sends it via UHID_OUTPUT.
-
- UHID_FEATURE:
- This event is sent if the kernel driver wants to perform a feature request as
- described in the HID specs. The report-type and report-number are available in
- the payload.
- The kernel serializes feature requests so there will never be two in parallel.
- However, if you fail to respond with a UHID_FEATURE_ANSWER in a time-span of 5
- seconds, then the requests will be dropped and a new one might be sent.
- Therefore, the payload also contains an "id" field that identifies every
- request.
-
-Document by:
- David Herrmann <dh.herrmann@googlemail.com>
+ UHID_GET_REPORT:
+ This event is sent if the kernel driver wants to perform a GET_REPORT request
+ on the control channeld as described in the HID specs. The report-type and
+ report-number are available in the payload.
+ The kernel serializes GET_REPORT requests so there will never be two in
+ parallel. However, if you fail to respond with a UHID_GET_REPORT_REPLY, the
+ request might silently time out.
+ Once you read a GET_REPORT request, you shall forward it to the hid device and
+ remember the "id" field in the payload. Once your hid device responds to the
+ GET_REPORT (or if it fails), you must send a UHID_GET_REPORT_REPLY to the
+ kernel with the exact same "id" as in the request. If the request already
+ timed out, the kernel will ignore the response silently. The "id" field is
+ never re-used, so conflicts cannot happen.
+
+ UHID_SET_REPORT:
+ This is the SET_REPORT equivalent of UHID_GET_REPORT. On receipt, you shall
+ send a SET_REPORT request to your hid device. Once it replies, you must tell
+ the kernel about it via UHID_SET_REPORT_REPLY.
+ The same restrictions as for UHID_GET_REPORT apply.
+
+----------------------------------------------------
+Written 2012, David Herrmann <dh.herrmann@gmail.com>
--
2.0.3
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH 00/12] HID: Convert UHID to new HID transport-layer
2014-07-29 15:14 [PATCH 00/12] HID: Convert UHID to new HID transport-layer David Herrmann
` (11 preceding siblings ...)
2014-07-29 15:14 ` [PATCH 12/12] HID: uhid: update documentation David Herrmann
@ 2014-07-29 19:00 ` Jiri Kosina
2014-07-29 19:03 ` David Herrmann
2014-08-25 8:34 ` Jiri Kosina
13 siblings, 1 reply; 16+ messages in thread
From: Jiri Kosina @ 2014-07-29 19:00 UTC (permalink / raw)
To: David Herrmann; +Cc: linux-input, Benjamin Tissoires
On Tue, 29 Jul 2014, David Herrmann wrote:
> Hi
>
> When Benjamin cleanup up the transport layer, he left UHID mostly unconverted
> due to ABI issues. I promised to take a look, so here're the patches.
>
> Most of the patches are fairly straightforward. Comments welcome. I tested an
> old uhid-sample.c build and it still works, so I think I didn't break
> backwards-compatibility.
I will definitely take a look, but I can't promise I will be able to
finish a review in time for 3.17.
If you find this a severe issue, please let me know.
Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 00/12] HID: Convert UHID to new HID transport-layer
2014-07-29 19:00 ` [PATCH 00/12] HID: Convert UHID to new HID transport-layer Jiri Kosina
@ 2014-07-29 19:03 ` David Herrmann
0 siblings, 0 replies; 16+ messages in thread
From: David Herrmann @ 2014-07-29 19:03 UTC (permalink / raw)
To: Jiri Kosina; +Cc: open list:HID CORE LAYER, Benjamin Tissoires
Hi
On Tue, Jul 29, 2014 at 9:00 PM, Jiri Kosina <jkosina@suse.cz> wrote:
> On Tue, 29 Jul 2014, David Herrmann wrote:
>
>> Hi
>>
>> When Benjamin cleanup up the transport layer, he left UHID mostly unconverted
>> due to ABI issues. I promised to take a look, so here're the patches.
>>
>> Most of the patches are fairly straightforward. Comments welcome. I tested an
>> old uhid-sample.c build and it still works, so I think I didn't break
>> backwards-compatibility.
>
> I will definitely take a look, but I can't promise I will be able to
> finish a review in time for 3.17.
>
> If you find this a severe issue, please let me know.
No time-pressure at all. Take your time.
Thanks
David
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 00/12] HID: Convert UHID to new HID transport-layer
2014-07-29 15:14 [PATCH 00/12] HID: Convert UHID to new HID transport-layer David Herrmann
` (12 preceding siblings ...)
2014-07-29 19:00 ` [PATCH 00/12] HID: Convert UHID to new HID transport-layer Jiri Kosina
@ 2014-08-25 8:34 ` Jiri Kosina
13 siblings, 0 replies; 16+ messages in thread
From: Jiri Kosina @ 2014-08-25 8:34 UTC (permalink / raw)
To: David Herrmann; +Cc: linux-input, Benjamin Tissoires
On Tue, 29 Jul 2014, David Herrmann wrote:
> Hi
>
> When Benjamin cleanup up the transport layer, he left UHID mostly unconverted
> due to ABI issues. I promised to take a look, so here're the patches.
Hi David,
now queued for 3.18 in for-3.18/uhid branch.
Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply [flat|nested] 16+ messages in thread