LinuxLists.cc - [PATCH 1/1] Bluetooth: hidp: Add support for hidraw HIDIOCGFEATURE and HIDIOCSFEATURE

[permalink] [raw]

Subject: Re: [PATCH 1/1] Bluetooth: hidp: Add support for hidraw HIDIOCGFEATURE and HIDIOCSFEATURE

On Tue, 29 Jun 2010, David Miller wrote:

> >> This patch adds support or getting and setting feature reports for bluetooth
> >> HID devices from HIDRAW.
> >>
> >> Signed-off-by: Alan Ott <[email protected]>
> >> ---
> >
> > Ping.
>
> We effectively don't have a bluetooth maintainer at the current point in
> time. I've tried to let patches sit for a while hoping the listed
> maintainer would do something, at least occaisionally, but that simply
> isn't happening.

Frankly, I don't understand what exactly the current situation with
in-kernel bluetooth stack is anyway.

What is the relation between what we have in net/bluetooth and the tree at
[1], which seems to be quite actively developed?

> So I'll just pick patches up directly as I find time to review them, but
> I have to warn that for me it's going to be done in a very low priority
> way because I really don't find bluetooth all that exciting. :-)

If needed, I can at least take over the net/bluetooth/hidp part, as I
maintain the rest of the HID code anyway.

[1] http://git.kernel.org/?p=bluetooth/bluez.git;a=summary

--
Jiri Kosina
SUSE Labs, Novell Inc.

2010-06-29 07:12:16

by David Miller

[permalink] [raw]

Subject: Re: [PATCH 1/1] Bluetooth: hidp: Add support for hidraw HIDIOCGFEATURE and HIDIOCSFEATURE

From: Antonio Ospite <[email protected]>
Date: Mon, 28 Jun 2010 13:14:37 +0200

> On Sun, 13 Jun 2010 18:20:01 -0400
> Alan Ott <[email protected]> wrote:
>
>> This patch adds support or getting and setting feature reports for bluetooth
>> HID devices from HIDRAW.
>>
>> Signed-off-by: Alan Ott <[email protected]>
>> ---
>
> Ping.

We effectively don't have a bluetooth maintainer at the current point
in time. I've tried to let patches sit for a while hoping the listed
maintainer would do something, at least occaisionally, but that simply
isn't happening.

So I'll just pick patches up directly as I find time to review them,
but I have to warn that for me it's going to be done in a very low
priority way because I really don't find bluetooth all that exciting. :-)

2010-06-28 11:14:37

[permalink] [raw]

Subject: Re: [PATCH 1/1] Bluetooth: hidp: Add support for hidraw HIDIOCGFEATURE and HIDIOCSFEATURE

On Sun, 13 Jun 2010 18:20:01 -0400
Alan Ott <[email protected]> wrote:

> This patch adds support or getting and setting feature reports for bluetooth
> HID devices from HIDRAW.
>
> Signed-off-by: Alan Ott <[email protected]>
> ---

Ping.

> net/bluetooth/hidp/core.c | 121 +++++++++++++++++++++++++++++++++++++++++++--
> net/bluetooth/hidp/hidp.h | 8 +++
> 2 files changed, 125 insertions(+), 4 deletions(-)
>
> diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
> index bfe641b..0f068a0 100644
> --- a/net/bluetooth/hidp/core.c
> +++ b/net/bluetooth/hidp/core.c
> @@ -36,6 +36,7 @@
> #include <linux/file.h>
> #include <linux/init.h>
> #include <linux/wait.h>
> +#include <linux/mutex.h>
> #include <net/sock.h>
>
> #include <linux/input.h>
> @@ -313,6 +314,93 @@ static int hidp_send_report(struct hidp_session *session, struct hid_report *rep
> return hidp_queue_report(session, buf, rsize);
> }
>
> +static int hidp_get_raw_report(struct hid_device *hid,
> + unsigned char report_number,
> + unsigned char *data, size_t count,
> + unsigned char report_type)
> +{
> + struct hidp_session *session = hid->driver_data;
> + struct sk_buff *skb;
> + size_t len;
> + int numbered_reports = hid->report_enum[report_type].numbered;
> +
> + switch (report_type) {
> + case HID_FEATURE_REPORT:
> + report_type = HIDP_TRANS_GET_REPORT | HIDP_DATA_RTYPE_FEATURE;
> + break;
> + case HID_INPUT_REPORT:
> + report_type = HIDP_TRANS_GET_REPORT | HIDP_DATA_RTYPE_INPUT;
> + break;
> + case HID_OUTPUT_REPORT:
> + report_type = HIDP_TRANS_GET_REPORT | HIDP_DATA_RTYPE_OUPUT;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + if (mutex_lock_interruptible(&session->report_mutex))
> + return -ERESTARTSYS;
> +
> + /* Set up our wait, and send the report request to the device. */
> + session->waiting_report_type = report_type & HIDP_DATA_RTYPE_MASK;
> + session->waiting_report_number = numbered_reports ? report_number : -1;
> + set_bit(HIDP_WAITING_FOR_RETURN, &session->flags);
> + data[0] = report_number;
> + if (hidp_send_ctrl_message(hid->driver_data, report_type, data, 1))
> + goto err_eio;
> +
> + /* Wait for the return of the report. The returned report
> + gets put in session->report_return. */
> + while (test_bit(HIDP_WAITING_FOR_RETURN, &session->flags)) {
> + int res;
> +
> + res = wait_event_interruptible_timeout(session->report_queue,
> + !test_bit(HIDP_WAITING_FOR_RETURN, &session->flags),
> + 5*HZ);
> + if (res == 0) {
> + /* timeout */
> + goto err_eio;
> + }
> + if (res < 0) {
> + /* signal */
> + goto err_restartsys;
> + }
> + }
> +
> + skb = session->report_return;
> + if (skb) {
> + if (numbered_reports) {
> + /* Strip off the report number. */
> + size_t rpt_len = skb->len-1;
> + len = rpt_len < count ? rpt_len : count;
> + memcpy(data, skb->data+1, len);
> + } else {
> + len = skb->len < count ? skb->len : count;
> + memcpy(data, skb->data, len);
> + }
> +
> + kfree_skb(skb);
> + session->report_return = NULL;
> + } else {
> + /* Device returned a HANDSHAKE, indicating protocol error. */
> + len = -EIO;
> + }
> +
> + clear_bit(HIDP_WAITING_FOR_RETURN, &session->flags);
> + mutex_unlock(&session->report_mutex);
> +
> + return len;
> +
> +err_restartsys:
> + clear_bit(HIDP_WAITING_FOR_RETURN, &session->flags);
> + mutex_unlock(&session->report_mutex);
> + return -ERESTARTSYS;
> +err_eio:
> + clear_bit(HIDP_WAITING_FOR_RETURN, &session->flags);
> + mutex_unlock(&session->report_mutex);
> + return -EIO;
> +}
> +
> static int hidp_output_raw_report(struct hid_device *hid, unsigned char *data, size_t count,
> unsigned char report_type)
> {
> @@ -367,6 +455,10 @@ static void hidp_process_handshake(struct hidp_session *session,
> case HIDP_HSHK_ERR_INVALID_REPORT_ID:
> case HIDP_HSHK_ERR_UNSUPPORTED_REQUEST:
> case HIDP_HSHK_ERR_INVALID_PARAMETER:
> + if (test_bit(HIDP_WAITING_FOR_RETURN, &session->flags)) {
> + clear_bit(HIDP_WAITING_FOR_RETURN, &session->flags);
> + wake_up_interruptible(&session->report_queue);
> + }
> /* FIXME: Call into SET_ GET_ handlers here */
> break;
>
> @@ -403,9 +495,11 @@ static void hidp_process_hid_control(struct hidp_session *session,
> }
> }
>
> -static void hidp_process_data(struct hidp_session *session, struct sk_buff *skb,
> +/* Returns true if the passed-in skb should be freed by the caller. */
> +static int hidp_process_data(struct hidp_session *session, struct sk_buff *skb,
> unsigned char param)
> {
> + int done_with_skb = 1;
> BT_DBG("session %p skb %p len %d param 0x%02x", session, skb, skb->len, param);
>
> switch (param) {
> @@ -417,7 +511,6 @@ static void hidp_process_data(struct hidp_session *session, struct sk_buff *skb,
>
> if (session->hid)
> hid_input_report(session->hid, HID_INPUT_REPORT, skb->data, skb->len, 0);
> -
> break;
>
> case HIDP_DATA_RTYPE_OTHER:
> @@ -429,12 +522,27 @@ static void hidp_process_data(struct hidp_session *session, struct sk_buff *skb,
> __hidp_send_ctrl_message(session,
> HIDP_TRANS_HANDSHAKE | HIDP_HSHK_ERR_INVALID_PARAMETER, NULL, 0);
> }
> +
> + if (test_bit(HIDP_WAITING_FOR_RETURN, &session->flags) &&
> + param == session->waiting_report_type) {
> + if (session->waiting_report_number < 0 ||
> + session->waiting_report_number == skb->data[0]) {
> + /* hidp_get_raw_report() is waiting on this report. */
> + session->report_return = skb;
> + done_with_skb = 0;
> + clear_bit(HIDP_WAITING_FOR_RETURN, &session->flags);
> + wake_up_interruptible(&session->report_queue);
> + }
> + }
> +
> + return done_with_skb;
> }
>
> static void hidp_recv_ctrl_frame(struct hidp_session *session,
> struct sk_buff *skb)
> {
> unsigned char hdr, type, param;
> + int free_skb = 1;
>
> BT_DBG("session %p skb %p len %d", session, skb, skb->len);
>
> @@ -454,7 +562,7 @@ static void hidp_recv_ctrl_frame(struct hidp_session *session,
> break;
>
> case HIDP_TRANS_DATA:
> - hidp_process_data(session, skb, param);
> + free_skb = hidp_process_data(session, skb, param);
> break;
>
> default:
> @@ -463,7 +571,8 @@ static void hidp_recv_ctrl_frame(struct hidp_session *session,
> break;
> }
>
> - kfree_skb(skb);
> + if (free_skb)
> + kfree_skb(skb);
> }
>
> static void hidp_recv_intr_frame(struct hidp_session *session,
> @@ -797,6 +906,7 @@ static int hidp_setup_hid(struct hidp_session *session,
> hid->dev.parent = hidp_get_device(session);
> hid->ll_driver = &hidp_hid_driver;
>
> + hid->hid_get_raw_report = hidp_get_raw_report;
> hid->hid_output_raw_report = hidp_output_raw_report;
>
> err = hid_add_device(hid);
> @@ -857,6 +967,9 @@ int hidp_add_connection(struct hidp_connadd_req *req, struct socket *ctrl_sock,
> skb_queue_head_init(&session->ctrl_transmit);
> skb_queue_head_init(&session->intr_transmit);
>
> + mutex_init(&session->report_mutex);
> + init_waitqueue_head(&session->report_queue);
> +
> session->flags = req->flags & (1 << HIDP_BLUETOOTH_VENDOR_ID);
> session->idle_to = req->idle_to;
>
> diff --git a/net/bluetooth/hidp/hidp.h b/net/bluetooth/hidp/hidp.h
> index 8d934a1..00e71dd 100644
> --- a/net/bluetooth/hidp/hidp.h
> +++ b/net/bluetooth/hidp/hidp.h
> @@ -80,6 +80,7 @@
> #define HIDP_VIRTUAL_CABLE_UNPLUG 0
> #define HIDP_BOOT_PROTOCOL_MODE 1
> #define HIDP_BLUETOOTH_VENDOR_ID 9
> +#define HIDP_WAITING_FOR_RETURN 10
>
> struct hidp_connadd_req {
> int ctrl_sock; // Connected control socket
> @@ -154,6 +155,13 @@ struct hidp_session {
> struct sk_buff_head ctrl_transmit;
> struct sk_buff_head intr_transmit;
>
> + /* Used in hidp_get_raw_report() */
> + int waiting_report_type; /* HIDP_DATA_RTYPE_* */
> + int waiting_report_number; /* -1 for not numbered */
> + struct mutex report_mutex;
> + struct sk_buff *report_return;
> + wait_queue_head_t report_queue;
> +
> /* Report descriptor */
> __u8 *rd_data;
> uint rd_size;
> --
> 1.7.0.4
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>

--
Antonio Ospite
http://ao2.it

PGP public key ID: 0x4553B001

A: Because it messes up the order in which people normally read text.
See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?

Attachments:

(No filename) (8.29 kB)
(No filename) (198.00 B)
Download all attachments

2010-06-19 17:49:57

[permalink] [raw]

Subject: Re: [PATCH 1/1] Bluetooth: hidp: Add support for hidraw HIDIOCGFEATURE and HIDIOCSFEATURE

On Sun, 13 Jun 2010, Alan Ott wrote:

> This patch adds support or getting and setting feature reports for bluetooth
> HID devices from HIDRAW.
>
> Signed-off-by: Alan Ott <[email protected]>

Marcel, any word on this please? We already have USB counterpart in, so
it'd be nice to finalize the Bluetooth part as well.

Thanks.

> ---
> net/bluetooth/hidp/core.c | 121 +++++++++++++++++++++++++++++++++++++++++++--
> net/bluetooth/hidp/hidp.h | 8 +++
> 2 files changed, 125 insertions(+), 4 deletions(-)
>
> diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
> index bfe641b..0f068a0 100644
> --- a/net/bluetooth/hidp/core.c
> +++ b/net/bluetooth/hidp/core.c
> @@ -36,6 +36,7 @@
> #include <linux/file.h>
> #include <linux/init.h>
> #include <linux/wait.h>
> +#include <linux/mutex.h>
> #include <net/sock.h>
>
> #include <linux/input.h>
> @@ -313,6 +314,93 @@ static int hidp_send_report(struct hidp_session *session, struct hid_report *rep
> return hidp_queue_report(session, buf, rsize);
> }
>
> +static int hidp_get_raw_report(struct hid_device *hid,
> + unsigned char report_number,
> + unsigned char *data, size_t count,
> + unsigned char report_type)
> +{
> + struct hidp_session *session = hid->driver_data;
> + struct sk_buff *skb;
> + size_t len;
> + int numbered_reports = hid->report_enum[report_type].numbered;
> +
> + switch (report_type) {
> + case HID_FEATURE_REPORT:
> + report_type = HIDP_TRANS_GET_REPORT | HIDP_DATA_RTYPE_FEATURE;
> + break;
> + case HID_INPUT_REPORT:
> + report_type = HIDP_TRANS_GET_REPORT | HIDP_DATA_RTYPE_INPUT;
> + break;
> + case HID_OUTPUT_REPORT:
> + report_type = HIDP_TRANS_GET_REPORT | HIDP_DATA_RTYPE_OUPUT;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + if (mutex_lock_interruptible(&session->report_mutex))
> + return -ERESTARTSYS;
> +
> + /* Set up our wait, and send the report request to the device. */
> + session->waiting_report_type = report_type & HIDP_DATA_RTYPE_MASK;
> + session->waiting_report_number = numbered_reports ? report_number : -1;
> + set_bit(HIDP_WAITING_FOR_RETURN, &session->flags);
> + data[0] = report_number;
> + if (hidp_send_ctrl_message(hid->driver_data, report_type, data, 1))
> + goto err_eio;
> +
> + /* Wait for the return of the report. The returned report
> + gets put in session->report_return. */
> + while (test_bit(HIDP_WAITING_FOR_RETURN, &session->flags)) {
> + int res;
> +
> + res = wait_event_interruptible_timeout(session->report_queue,
> + !test_bit(HIDP_WAITING_FOR_RETURN, &session->flags),
> + 5*HZ);
> + if (res == 0) {
> + /* timeout */
> + goto err_eio;
> + }
> + if (res < 0) {
> + /* signal */
> + goto err_restartsys;
> + }
> + }
> +
> + skb = session->report_return;
> + if (skb) {
> + if (numbered_reports) {
> + /* Strip off the report number. */
> + size_t rpt_len = skb->len-1;
> + len = rpt_len < count ? rpt_len : count;
> + memcpy(data, skb->data+1, len);
> + } else {
> + len = skb->len < count ? skb->len : count;
> + memcpy(data, skb->data, len);
> + }
> +
> + kfree_skb(skb);
> + session->report_return = NULL;
> + } else {
> + /* Device returned a HANDSHAKE, indicating protocol error. */
> + len = -EIO;
> + }
> +
> + clear_bit(HIDP_WAITING_FOR_RETURN, &session->flags);
> + mutex_unlock(&session->report_mutex);
> +
> + return len;
> +
> +err_restartsys:
> + clear_bit(HIDP_WAITING_FOR_RETURN, &session->flags);
> + mutex_unlock(&session->report_mutex);
> + return -ERESTARTSYS;
> +err_eio:
> + clear_bit(HIDP_WAITING_FOR_RETURN, &session->flags);
> + mutex_unlock(&session->report_mutex);
> + return -EIO;
> +}
> +
> static int hidp_output_raw_report(struct hid_device *hid, unsigned char *data, size_t count,
> unsigned char report_type)
> {
> @@ -367,6 +455,10 @@ static void hidp_process_handshake(struct hidp_session *session,
> case HIDP_HSHK_ERR_INVALID_REPORT_ID:
> case HIDP_HSHK_ERR_UNSUPPORTED_REQUEST:
> case HIDP_HSHK_ERR_INVALID_PARAMETER:
> + if (test_bit(HIDP_WAITING_FOR_RETURN, &session->flags)) {
> + clear_bit(HIDP_WAITING_FOR_RETURN, &session->flags);
> + wake_up_interruptible(&session->report_queue);
> + }
> /* FIXME: Call into SET_ GET_ handlers here */
> break;
>
> @@ -403,9 +495,11 @@ static void hidp_process_hid_control(struct hidp_session *session,
> }
> }
>
> -static void hidp_process_data(struct hidp_session *session, struct sk_buff *skb,
> +/* Returns true if the passed-in skb should be freed by the caller. */
> +static int hidp_process_data(struct hidp_session *session, struct sk_buff *skb,
> unsigned char param)
> {
> + int done_with_skb = 1;
> BT_DBG("session %p skb %p len %d param 0x%02x", session, skb, skb->len, param);
>
> switch (param) {
> @@ -417,7 +511,6 @@ static void hidp_process_data(struct hidp_session *session, struct sk_buff *skb,
>
> if (session->hid)
> hid_input_report(session->hid, HID_INPUT_REPORT, skb->data, skb->len, 0);
> -
> break;
>
> case HIDP_DATA_RTYPE_OTHER:
> @@ -429,12 +522,27 @@ static void hidp_process_data(struct hidp_session *session, struct sk_buff *skb,
> __hidp_send_ctrl_message(session,
> HIDP_TRANS_HANDSHAKE | HIDP_HSHK_ERR_INVALID_PARAMETER, NULL, 0);
> }
> +
> + if (test_bit(HIDP_WAITING_FOR_RETURN, &session->flags) &&
> + param == session->waiting_report_type) {
> + if (session->waiting_report_number < 0 ||
> + session->waiting_report_number == skb->data[0]) {
> + /* hidp_get_raw_report() is waiting on this report. */
> + session->report_return = skb;
> + done_with_skb = 0;
> + clear_bit(HIDP_WAITING_FOR_RETURN, &session->flags);
> + wake_up_interruptible(&session->report_queue);
> + }
> + }
> +
> + return done_with_skb;
> }
>
> static void hidp_recv_ctrl_frame(struct hidp_session *session,
> struct sk_buff *skb)
> {
> unsigned char hdr, type, param;
> + int free_skb = 1;
>
> BT_DBG("session %p skb %p len %d", session, skb, skb->len);
>
> @@ -454,7 +562,7 @@ static void hidp_recv_ctrl_frame(struct hidp_session *session,
> break;
>
> case HIDP_TRANS_DATA:
> - hidp_process_data(session, skb, param);
> + free_skb = hidp_process_data(session, skb, param);
> break;
>
> default:
> @@ -463,7 +571,8 @@ static void hidp_recv_ctrl_frame(struct hidp_session *session,
> break;
> }
>
> - kfree_skb(skb);
> + if (free_skb)
> + kfree_skb(skb);
> }
>
> static void hidp_recv_intr_frame(struct hidp_session *session,
> @@ -797,6 +906,7 @@ static int hidp_setup_hid(struct hidp_session *session,
> hid->dev.parent = hidp_get_device(session);
> hid->ll_driver = &hidp_hid_driver;
>
> + hid->hid_get_raw_report = hidp_get_raw_report;
> hid->hid_output_raw_report = hidp_output_raw_report;
>
> err = hid_add_device(hid);
> @@ -857,6 +967,9 @@ int hidp_add_connection(struct hidp_connadd_req *req, struct socket *ctrl_sock,
> skb_queue_head_init(&session->ctrl_transmit);
> skb_queue_head_init(&session->intr_transmit);
>
> + mutex_init(&session->report_mutex);
> + init_waitqueue_head(&session->report_queue);
> +
> session->flags = req->flags & (1 << HIDP_BLUETOOTH_VENDOR_ID);
> session->idle_to = req->idle_to;
>
> diff --git a/net/bluetooth/hidp/hidp.h b/net/bluetooth/hidp/hidp.h
> index 8d934a1..00e71dd 100644
> --- a/net/bluetooth/hidp/hidp.h
> +++ b/net/bluetooth/hidp/hidp.h
> @@ -80,6 +80,7 @@
> #define HIDP_VIRTUAL_CABLE_UNPLUG 0
> #define HIDP_BOOT_PROTOCOL_MODE 1
> #define HIDP_BLUETOOTH_VENDOR_ID 9
> +#define HIDP_WAITING_FOR_RETURN 10
>
> struct hidp_connadd_req {
> int ctrl_sock; // Connected control socket
> @@ -154,6 +155,13 @@ struct hidp_session {
> struct sk_buff_head ctrl_transmit;
> struct sk_buff_head intr_transmit;
>
> + /* Used in hidp_get_raw_report() */
> + int waiting_report_type; /* HIDP_DATA_RTYPE_* */
> + int waiting_report_number; /* -1 for not numbered */
> + struct mutex report_mutex;
> + struct sk_buff *report_return;
> + wait_queue_head_t report_queue;
> +
> /* Report descriptor */
> __u8 *rd_data;
> uint rd_size;
> --
> 1.7.0.4
>
>

--
Jiri Kosina
SUSE Labs, Novell Inc.

2010-07-22 16:58:42

[permalink] [raw]

Subject: Re: [PATCH 1/1] Bluetooth: hidp: Add support for hidraw HIDIOCGFEATURE and HIDIOCSFEATURE

On 07/22/2010 11:21 AM, Marcel Holtmann wrote:
>>>>>>> what is usb-hid.ko doing here? I would expect a bunch of code
>>>>>>> duplication with minor difference between USB and Bluetooth.
>>>>>>>
>>>>>>>
>>>>>> usbhid doesn't have a lot of code for hidraw. Two functions are involved:
>>>>>> usbhid_output_raw_report()
>>>>>> - calls usb_control_msg() with Get_Report
>>>>>> usbhid_get_raw_report()
>>>>>> - calls usb_control_msg() with Set_Report
>>>>>> OR
>>>>>> - calls usb_interrupt_msg() on the Ouput pipe.
>>>>>>
>>>>>> This is of course easier than bluetooth because usb_control_msg() is
>>>>>> synchronous, even when requesting reports, mostly because of the nature
>>>>>> of USB, where the request and response are part of the same transfer.
>>>>>>
>>>>>> For Bluetooth, it's a bit more complicated since the kernel treats it
>>>>>> more like a networking interface (and indeed it is). My understanding is
>>>>>> that to make a synchronous transfer in bluetooth, one must:
>>>>>> - send the request packet
>>>>>> - block (wait_event_*())
>>>>>> - when the response is received in the input handler, wake_up_*().
>>>>>>
>>>>>> There's not really any code duplication, mostly because initiating
>>>>>> synchronous USB transfers (input and output) is easy (because of the
>>>>>> usb_*_msg() functions), while making synchronous Bluetooth transfers
>>>>>> must be done manually. If there's a nice, convenient, synchronous
>>>>>> function in Bluetooth similar to usb_control_msg() that I've missed,
>>>>>> then let me know, as it would simplify this whole thing.
>>>>>>
>>>>>>
>>>>> there is not and I don't think we ever get one. My question here was
>>>>> more in the direction why HID core is doing these synchronously in the
>>>>> first place. Especially since USB can do everything async as well.
>>>>>
>>>> I'm open to suggestions. The way I see it is from a user space
>>>> perspective. With Get_Feature being on an ioctl(), I don't see any clean
>>>> way to do it other than synchronously. Other operating systems (I can
>>>> say for sure Windows, Mac OS X, and FreeBSD) handle Get/Set Feature the
>>>> same way (synchronously) from user space.
>>>>
>>>> You seem to be proposing an asynchronous interface. What would that look
>>>> like from user space?
>>>>
>>> not necessarily from user space, but at least from HID core to HIDP and
>>> usb-hid transports. At least that is what I would expect, Jiri?
>>>
>> Sorry for this taking too long (vacations, conferences, you name it) for
>> me to respond.
>>
>> As all the _raw() callbacks are purely intended for userspace interaction
>> anyway, it's perfectly fine (and in fact desirable) for the low-level
>> transport drivers to perform these operations synchronously (and that's
>> what USB implementation does as well).
>>
>> Marcel, if your opposition to synchronous interface is strong, we'll have
>> to think about other aproaches, but from my POV, the patch is fine as-is
>> for Bluetooth.
>>
> that the ioctl() API is synchronous is fine to me, however pushing that
> down to the transport drivers seems wrong to me. I think the HID core
> should be able to handle a fully asynchronous transport driver. I know
> that with the USB subsystem you are little bit spoiled here, but for
> Bluetooth it is not the case. And in the end even using the asynchronous
> USB URB calls would be nice as well.
>

How are the URB calls better than using the synchronous calls? (see below)

> So why not make the core actually wait for responses from the transport
> driver.

Because this makes the USB side a lot more complicated, and it would
introduce transport specific code into the core. Further, it would
involve the transport driver calling hidraw with _every_ single packet
it receives. Further, it would have to call hidraw with HANDSHAKE
protocol error packets as well.

> I would make the transport drivers a lot simpler in the long
> run.

It would make the USB transport driver and drivers/hid/hidraw much more
complicated right now, at the expense of making the BT transport driver
marginally simpler (and I'm not even convinced whether it would really
be simpler). (see below for more)

> And I know that most likely besides Bluetooth and USB you won't see
> another, but you never know.
>
>

I just don't understand the objection. In each transport type, the
waiting will have to be done in a different way. USB and BT are
different enough that this is the case already, without having to
imagine future buses which use HID. In BT, you have to check each packet
which comes back from the BT network to see whether it is the response
packet that hidraw is waiting for. Further, you have to check for
HANDSHAKE packets indicating protocol error. Where better for this to be
done than in hidp? Further, how can this possibly happen in
drivers/hid/hidraw, as it doesn't know about the details of bluetooth to
make this determination, and why should it? In my last email (
http://lkml.org/lkml/2010/7/9/231 ) to which I got no response, I laid
out how doing the blocking in drivers/hid/hidraw would only make all the
parts except bluetooth more complicated (including the core, and the USB
side), and would also introduce bluetooth-specific things into the core.

Further, you're saying that using the asynchronous USB URB calls would
be a benefit. How is it a benefit to replace a single function call
which does exactly what I want, with a set of asynchronous calls and
then adding my own blocking to make it do the same thing? This sounds to
me like it would be 1: more text, 2: duplication of code, 3: error
prone. I can't understand how this is of benefit. Please explain to me
what I'm missing.

In theory, what you're saying makes sense. Making common code and logic
actually common is always good. In practice though, in this case, I
submit that there really isn't any commonality, and the only way for
there to be commonality is to do the USB side the hard way. Further,
drivers/hid/hidraw can't wait for a bluetooth packet without having code
that's bluetooth-specific. It seems just that simple to me.

I'll give it some more thought, and take another look at the code to see
if there's something obvious that I'm missing. If you know what I'm
missing in my understanding of the problem, please tell me :)

Alan.

2010-07-22 15:21:55

[permalink] [raw]

Subject: Re: [PATCH 1/1] Bluetooth: hidp: Add support for hidraw HIDIOCGFEATURE and HIDIOCSFEATURE

Hi Jiri,

> > > >>> what is usb-hid.ko doing here? I would expect a bunch of code
> > > >>> duplication with minor difference between USB and Bluetooth.
> > > >>>
> > > >> usbhid doesn't have a lot of code for hidraw. Two functions are involved:
> > > >> usbhid_output_raw_report()
> > > >> - calls usb_control_msg() with Get_Report
> > > >> usbhid_get_raw_report()
> > > >> - calls usb_control_msg() with Set_Report
> > > >> OR
> > > >> - calls usb_interrupt_msg() on the Ouput pipe.
> > > >>
> > > >> This is of course easier than bluetooth because usb_control_msg() is
> > > >> synchronous, even when requesting reports, mostly because of the nature
> > > >> of USB, where the request and response are part of the same transfer.
> > > >>
> > > >> For Bluetooth, it's a bit more complicated since the kernel treats it
> > > >> more like a networking interface (and indeed it is). My understanding is
> > > >> that to make a synchronous transfer in bluetooth, one must:
> > > >> - send the request packet
> > > >> - block (wait_event_*())
> > > >> - when the response is received in the input handler, wake_up_*().
> > > >>
> > > >> There's not really any code duplication, mostly because initiating
> > > >> synchronous USB transfers (input and output) is easy (because of the
> > > >> usb_*_msg() functions), while making synchronous Bluetooth transfers
> > > >> must be done manually. If there's a nice, convenient, synchronous
> > > >> function in Bluetooth similar to usb_control_msg() that I've missed,
> > > >> then let me know, as it would simplify this whole thing.
> > > >>
> > > > there is not and I don't think we ever get one. My question here was
> > > > more in the direction why HID core is doing these synchronously in the
> > > > first place. Especially since USB can do everything async as well.
> > >
> > > I'm open to suggestions. The way I see it is from a user space
> > > perspective. With Get_Feature being on an ioctl(), I don't see any clean
> > > way to do it other than synchronously. Other operating systems (I can
> > > say for sure Windows, Mac OS X, and FreeBSD) handle Get/Set Feature the
> > > same way (synchronously) from user space.
> > >
> > > You seem to be proposing an asynchronous interface. What would that look
> > > like from user space?
> >
> > not necessarily from user space, but at least from HID core to HIDP and
> > usb-hid transports. At least that is what I would expect, Jiri?
>
> Sorry for this taking too long (vacations, conferences, you name it) for
> me to respond.
>
> As all the _raw() callbacks are purely intended for userspace interaction
> anyway, it's perfectly fine (and in fact desirable) for the low-level
> transport drivers to perform these operations synchronously (and that's
> what USB implementation does as well).
>
> Marcel, if your opposition to synchronous interface is strong, we'll have
> to think about other aproaches, but from my POV, the patch is fine as-is
> for Bluetooth.

that the ioctl() API is synchronous is fine to me, however pushing that
down to the transport drivers seems wrong to me. I think the HID core
should be able to handle a fully asynchronous transport driver. I know
that with the USB subsystem you are little bit spoiled here, but for
Bluetooth it is not the case. And in the end even using the asynchronous
USB URB calls would be nice as well.

So why not make the core actually wait for responses from the transport
driver. I would make the transport drivers a lot simpler in the long
run. And I know that most likely besides Bluetooth and USB you won't see
another, but you never know.

Regards

Marcel

2010-07-22 14:14:57

[permalink] [raw]

Subject: Re: [PATCH 1/1] Bluetooth: hidp: Add support for hidraw HIDIOCGFEATURE and HIDIOCSFEATURE

On Fri, 9 Jul 2010, Marcel Holtmann wrote:

> > >>> what is usb-hid.ko doing here? I would expect a bunch of code
> > >>> duplication with minor difference between USB and Bluetooth.
> > >>>
> > >> usbhid doesn't have a lot of code for hidraw. Two functions are involved:
> > >> usbhid_output_raw_report()
> > >> - calls usb_control_msg() with Get_Report
> > >> usbhid_get_raw_report()
> > >> - calls usb_control_msg() with Set_Report
> > >> OR
> > >> - calls usb_interrupt_msg() on the Ouput pipe.
> > >>
> > >> This is of course easier than bluetooth because usb_control_msg() is
> > >> synchronous, even when requesting reports, mostly because of the nature
> > >> of USB, where the request and response are part of the same transfer.
> > >>
> > >> For Bluetooth, it's a bit more complicated since the kernel treats it
> > >> more like a networking interface (and indeed it is). My understanding is
> > >> that to make a synchronous transfer in bluetooth, one must:
> > >> - send the request packet
> > >> - block (wait_event_*())
> > >> - when the response is received in the input handler, wake_up_*().
> > >>
> > >> There's not really any code duplication, mostly because initiating
> > >> synchronous USB transfers (input and output) is easy (because of the
> > >> usb_*_msg() functions), while making synchronous Bluetooth transfers
> > >> must be done manually. If there's a nice, convenient, synchronous
> > >> function in Bluetooth similar to usb_control_msg() that I've missed,
> > >> then let me know, as it would simplify this whole thing.
> > >>
> > > there is not and I don't think we ever get one. My question here was
> > > more in the direction why HID core is doing these synchronously in the
> > > first place. Especially since USB can do everything async as well.
> >
> > I'm open to suggestions. The way I see it is from a user space
> > perspective. With Get_Feature being on an ioctl(), I don't see any clean
> > way to do it other than synchronously. Other operating systems (I can
> > say for sure Windows, Mac OS X, and FreeBSD) handle Get/Set Feature the
> > same way (synchronously) from user space.
> >
> > You seem to be proposing an asynchronous interface. What would that look
> > like from user space?
>
> not necessarily from user space, but at least from HID core to HIDP and
> usb-hid transports. At least that is what I would expect, Jiri?

Sorry for this taking too long (vacations, conferences, you name it) for
me to respond.

As all the _raw() callbacks are purely intended for userspace interaction
anyway, it's perfectly fine (and in fact desirable) for the low-level
transport drivers to perform these operations synchronously (and that's
what USB implementation does as well).

Marcel, if your opposition to synchronous interface is strong, we'll have
to think about other aproaches, but from my POV, the patch is fine as-is
for Bluetooth.

Thanks,

--
Jiri Kosina
SUSE Labs, Novell Inc.

2010-07-09 18:24:37

[permalink] [raw]

Subject: Re: [PATCH 1/1] Bluetooth: hidp: Add support for hidraw HIDIOCGFEATURE and HIDIOCSFEATURE

On 07/09/2010 01:33 PM, Marcel Holtmann wrote:
>>>>>>> I looked at this and I am bit worried that this should not be done in
>>>>>>> this detail in the HIDP driver. Essentially HIDP is a pure transport
>>>>>>> driver. It should not handle all these details. Can we make this a bit
>>>>>>> easier for the transport drivers to support such features?
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>> I put these changes (most notably the addition of hidp_get_raw_report())
>>>>>> in hidp because that's where the parallel function
>>>>>> hidp_output_raw_report() was already located. I figured the input should
>>>>>> go with the output. That said, if there's a better place for both of
>>>>>> them (input and output) to go, let me know where you think it should be,
>>>>>> and I'll get them moved into the proper spot.
>>>>>>
>>>>>> I'm not sure what you mean about HIDP being a pure transport driver.
>>>>>>
>>>>>>
>>>>>>
>>>>> what is usb-hid.ko doing here? I would expect a bunch of code
>>>>> duplication with minor difference between USB and Bluetooth.
>>>>>
>>>>>
>>>> usbhid doesn't have a lot of code for hidraw. Two functions are involved:
>>>> usbhid_output_raw_report()
>>>> - calls usb_control_msg() with Get_Report
>>>> usbhid_get_raw_report()
>>>> - calls usb_control_msg() with Set_Report
>>>> OR
>>>> - calls usb_interrupt_msg() on the Ouput pipe.
>>>>
>>>> This is of course easier than bluetooth because usb_control_msg() is
>>>> synchronous, even when requesting reports, mostly because of the nature
>>>> of USB, where the request and response are part of the same transfer.
>>>>
>>>> For Bluetooth, it's a bit more complicated since the kernel treats it
>>>> more like a networking interface (and indeed it is). My understanding is
>>>> that to make a synchronous transfer in bluetooth, one must:
>>>> - send the request packet
>>>> - block (wait_event_*())
>>>> - when the response is received in the input handler, wake_up_*().
>>>>
>>>> There's not really any code duplication, mostly because initiating
>>>> synchronous USB transfers (input and output) is easy (because of the
>>>> usb_*_msg() functions), while making synchronous Bluetooth transfers
>>>> must be done manually. If there's a nice, convenient, synchronous
>>>> function in Bluetooth similar to usb_control_msg() that I've missed,
>>>> then let me know, as it would simplify this whole thing.
>>>>
>>>>
>>> there is not and I don't think we ever get one. My question here was
>>> more in the direction why HID core is doing these synchronously in the
>>> first place. Especially since USB can do everything async as well.
>>>
>> I'm open to suggestions. The way I see it is from a user space
>> perspective. With Get_Feature being on an ioctl(), I don't see any clean
>> way to do it other than synchronously. Other operating systems (I can
>> say for sure Windows, Mac OS X, and FreeBSD) handle Get/Set Feature the
>> same way (synchronously) from user space.
>>
>> You seem to be proposing an asynchronous interface. What would that look
>> like from user space?
>>
> not necessarily from user space, but at least from HID core to HIDP and
> usb-hid transports. At least that is what I would expect, Jiri?
>
> Regards
>
> Marcel
>

Hi Marcel,

So it sounds like you're mostly concerned about the sleeping (blocking),
and where is the _right_ place for it to occur. It seems like it could
either occur in hid/hidraw.c or in hidp/core.c. If it were to occur in
hid/hidraw.c, what then would get passed back and forth between the
bluetooth/hidp and hidraw?

Maybe something like the following:
hidraw:
- get_report() (hypothetical)
- calls a hypothetical hidp_initiate_get_report(), which:
- sends the report request and returns immediately.
- wait for response

hidp:
- whenever a report is returned, it calls back to hidraw,
which wakes up the get_report() thread if
the data matches the report being waited on.

For this to work, we'd need 2 more function pointers in struct hid_device:
1. a way for hidp to call back into hidraw.
2. a pointer for hidp_initiate_get_report().

These of course would be in addition to the ones that USB already uses
(like hid_get_raw_report()), and would cause USB and Bluetooth to use
different APIs to each transport.

Of course, there could be commonality if we used the asynchronous USB
APIs like you suggested, although, I'm not sure I see the benefit of
making the USB part more complicated. The USB part (hid/usb/hid-core.c)
is currently _very_ simple.

It seems like we have two options:
1. Move to asynchronous APIs in USB and Bluetooth. This involves:
a. Move to asynchronous APIs in hid/usbhid/hid-core.c
b. Adding support into hid/hidraw.c to do the waiting.
c. Changing bluetooth/hidp to be asynchronous in nature.

2. Keep using synchronous USB APIs.
a. hid/usbhid/hid-core remains really simple
b. hid/hidraw.c remains really simple
c. bluetooth/hidp has some complexity

I'd argue that the complexity of bluetooth/hidp isn't really that
complex, and further, it's mostly isolated to one (new) function (that's
where the wait_event_*() is).

Further, if we did option #2, some piece of code has to determine
whether to wake up the blocking thread (which would then be in
hid/hidraw.c). This piece of code would be notified for every packet
received from Bluetooth to decide whether it should wake up the sleeping
thread, and would have to have bluetooth-specific code in it (something
like the block which calls wake_up_interruptible() in my patch). It
seems like this code would _have_ to be in hidp.

From a design standpoint, I can't see how it makes sense to push this
code into hid/hidraw.c when it is bluetooth-specific. Further, I can't
see how it makes sense to do the USB portion the hard way, when the
current implementation is so compact and non-error-prone.

Clients to hidraw provide two functions with very simple interfaces, one
for outputting reports, and one for getting (requesting and receiving)
reports. I think having clean interfaces between modules has a lot of value.

All that said, I'm always open to better ideas. Maybe you have a better
design idea that you can enlighten me with.

Alan.

2010-07-09 17:33:35

[permalink] [raw]

Subject: Re: [PATCH 1/1] Bluetooth: hidp: Add support for hidraw HIDIOCGFEATURE and HIDIOCSFEATURE

Hi Alan,

> >>>>> I looked at this and I am bit worried that this should not be done in
> >>>>> this detail in the HIDP driver. Essentially HIDP is a pure transport
> >>>>> driver. It should not handle all these details. Can we make this a bit
> >>>>> easier for the transport drivers to support such features?
> >>>>>
> >>>>>
> >>>> I put these changes (most notably the addition of hidp_get_raw_report())
> >>>> in hidp because that's where the parallel function
> >>>> hidp_output_raw_report() was already located. I figured the input should
> >>>> go with the output. That said, if there's a better place for both of
> >>>> them (input and output) to go, let me know where you think it should be,
> >>>> and I'll get them moved into the proper spot.
> >>>>
> >>>> I'm not sure what you mean about HIDP being a pure transport driver.
> >>>>
> >>>>
> >>> what is usb-hid.ko doing here? I would expect a bunch of code
> >>> duplication with minor difference between USB and Bluetooth.
> >>>
> >> usbhid doesn't have a lot of code for hidraw. Two functions are involved:
> >> usbhid_output_raw_report()
> >> - calls usb_control_msg() with Get_Report
> >> usbhid_get_raw_report()
> >> - calls usb_control_msg() with Set_Report
> >> OR
> >> - calls usb_interrupt_msg() on the Ouput pipe.
> >>
> >> This is of course easier than bluetooth because usb_control_msg() is
> >> synchronous, even when requesting reports, mostly because of the nature
> >> of USB, where the request and response are part of the same transfer.
> >>
> >> For Bluetooth, it's a bit more complicated since the kernel treats it
> >> more like a networking interface (and indeed it is). My understanding is
> >> that to make a synchronous transfer in bluetooth, one must:
> >> - send the request packet
> >> - block (wait_event_*())
> >> - when the response is received in the input handler, wake_up_*().
> >>
> >> There's not really any code duplication, mostly because initiating
> >> synchronous USB transfers (input and output) is easy (because of the
> >> usb_*_msg() functions), while making synchronous Bluetooth transfers
> >> must be done manually. If there's a nice, convenient, synchronous
> >> function in Bluetooth similar to usb_control_msg() that I've missed,
> >> then let me know, as it would simplify this whole thing.
> >>
> > there is not and I don't think we ever get one. My question here was
> > more in the direction why HID core is doing these synchronously in the
> > first place. Especially since USB can do everything async as well.
>
> I'm open to suggestions. The way I see it is from a user space
> perspective. With Get_Feature being on an ioctl(), I don't see any clean
> way to do it other than synchronously. Other operating systems (I can
> say for sure Windows, Mac OS X, and FreeBSD) handle Get/Set Feature the
> same way (synchronously) from user space.
>
> You seem to be proposing an asynchronous interface. What would that look
> like from user space?

not necessarily from user space, but at least from HID core to HIDP and
usb-hid transports. At least that is what I would expect, Jiri?

Regards

Marcel

2010-07-09 14:06:38

[permalink] [raw]

Subject: Re: [PATCH 1/1] Bluetooth: hidp: Add support for hidraw HIDIOCGFEATURE and HIDIOCSFEATURE

On 07/09/2010 09:06 AM, Marcel Holtmann wrote:
>>>>> I looked at this and I am bit worried that this should not be done in
>>>>> this detail in the HIDP driver. Essentially HIDP is a pure transport
>>>>> driver. It should not handle all these details. Can we make this a bit
>>>>> easier for the transport drivers to support such features?
>>>>>
>>>>>
>>>> I put these changes (most notably the addition of hidp_get_raw_report())
>>>> in hidp because that's where the parallel function
>>>> hidp_output_raw_report() was already located. I figured the input should
>>>> go with the output. That said, if there's a better place for both of
>>>> them (input and output) to go, let me know where you think it should be,
>>>> and I'll get them moved into the proper spot.
>>>>
>>>> I'm not sure what you mean about HIDP being a pure transport driver.
>>>>
>>>>
>>> what is usb-hid.ko doing here? I would expect a bunch of code
>>> duplication with minor difference between USB and Bluetooth.
>>>
>> usbhid doesn't have a lot of code for hidraw. Two functions are involved:
>> usbhid_output_raw_report()
>> - calls usb_control_msg() with Get_Report
>> usbhid_get_raw_report()
>> - calls usb_control_msg() with Set_Report
>> OR
>> - calls usb_interrupt_msg() on the Ouput pipe.
>>
>> This is of course easier than bluetooth because usb_control_msg() is
>> synchronous, even when requesting reports, mostly because of the nature
>> of USB, where the request and response are part of the same transfer.
>>
>> For Bluetooth, it's a bit more complicated since the kernel treats it
>> more like a networking interface (and indeed it is). My understanding is
>> that to make a synchronous transfer in bluetooth, one must:
>> - send the request packet
>> - block (wait_event_*())
>> - when the response is received in the input handler, wake_up_*().
>>
>> There's not really any code duplication, mostly because initiating
>> synchronous USB transfers (input and output) is easy (because of the
>> usb_*_msg() functions), while making synchronous Bluetooth transfers
>> must be done manually. If there's a nice, convenient, synchronous
>> function in Bluetooth similar to usb_control_msg() that I've missed,
>> then let me know, as it would simplify this whole thing.
>>
> there is not and I don't think we ever get one. My question here was
> more in the direction why HID core is doing these synchronously in the
> first place. Especially since USB can do everything async as well.
>
> Regards
>
> Marcel
>

Hi Marcel,

I'm open to suggestions. The way I see it is from a user space
perspective. With Get_Feature being on an ioctl(), I don't see any clean
way to do it other than synchronously. Other operating systems (I can
say for sure Windows, Mac OS X, and FreeBSD) handle Get/Set Feature the
same way (synchronously) from user space.

You seem to be proposing an asynchronous interface. What would that look
like from user space?

Alan.

2010-07-09 13:06:30

[permalink] [raw]

Subject: Re: [PATCH 1/1] Bluetooth: hidp: Add support for hidraw HIDIOCGFEATURE and HIDIOCSFEATURE

Hi Alan,

> >>> I looked at this and I am bit worried that this should not be done in
> >>> this detail in the HIDP driver. Essentially HIDP is a pure transport
> >>> driver. It should not handle all these details. Can we make this a bit
> >>> easier for the transport drivers to support such features?
> >>>
> >> I put these changes (most notably the addition of hidp_get_raw_report())
> >> in hidp because that's where the parallel function
> >> hidp_output_raw_report() was already located. I figured the input should
> >> go with the output. That said, if there's a better place for both of
> >> them (input and output) to go, let me know where you think it should be,
> >> and I'll get them moved into the proper spot.
> >>
> >> I'm not sure what you mean about HIDP being a pure transport driver.
> >>
> > what is usb-hid.ko doing here? I would expect a bunch of code
> > duplication with minor difference between USB and Bluetooth.
>
> usbhid doesn't have a lot of code for hidraw. Two functions are involved:
> usbhid_output_raw_report()
> - calls usb_control_msg() with Get_Report
> usbhid_get_raw_report()
> - calls usb_control_msg() with Set_Report
> OR
> - calls usb_interrupt_msg() on the Ouput pipe.
>
> This is of course easier than bluetooth because usb_control_msg() is
> synchronous, even when requesting reports, mostly because of the nature
> of USB, where the request and response are part of the same transfer.
>
> For Bluetooth, it's a bit more complicated since the kernel treats it
> more like a networking interface (and indeed it is). My understanding is
> that to make a synchronous transfer in bluetooth, one must:
> - send the request packet
> - block (wait_event_*())
> - when the response is received in the input handler, wake_up_*().
>
> There's not really any code duplication, mostly because initiating
> synchronous USB transfers (input and output) is easy (because of the
> usb_*_msg() functions), while making synchronous Bluetooth transfers
> must be done manually. If there's a nice, convenient, synchronous
> function in Bluetooth similar to usb_control_msg() that I've missed,
> then let me know, as it would simplify this whole thing.

there is not and I don't think we ever get one. My question here was
more in the direction why HID core is doing these synchronously in the
first place. Especially since USB can do everything async as well.

Regards

Marcel

2010-07-09 13:02:32

[permalink] [raw]

Subject: Re: [PATCH 1/1] Bluetooth: hidp: Add support for hidraw HIDIOCGFEATURE and HIDIOCSFEATURE

On 07/09/2010 04:01 AM, Marcel Holtmann wrote:
> Hi Alan,
>
>
>>> I looked at this and I am bit worried that this should not be done in
>>> this detail in the HIDP driver. Essentially HIDP is a pure transport
>>> driver. It should not handle all these details. Can we make this a bit
>>> easier for the transport drivers to support such features?
>>>
>> I put these changes (most notably the addition of hidp_get_raw_report())
>> in hidp because that's where the parallel function
>> hidp_output_raw_report() was already located. I figured the input should
>> go with the output. That said, if there's a better place for both of
>> them (input and output) to go, let me know where you think it should be,
>> and I'll get them moved into the proper spot.
>>
>> I'm not sure what you mean about HIDP being a pure transport driver.
>>
> what is usb-hid.ko doing here? I would expect a bunch of code
> duplication with minor difference between USB and Bluetooth.
>
> Regards
>
> Marcel
>

Hi Marcel,

usbhid doesn't have a lot of code for hidraw. Two functions are involved:
usbhid_output_raw_report()
- calls usb_control_msg() with Get_Report
usbhid_get_raw_report()
- calls usb_control_msg() with Set_Report
OR
- calls usb_interrupt_msg() on the Ouput pipe.

This is of course easier than bluetooth because usb_control_msg() is
synchronous, even when requesting reports, mostly because of the nature
of USB, where the request and response are part of the same transfer.

For Bluetooth, it's a bit more complicated since the kernel treats it
more like a networking interface (and indeed it is). My understanding is
that to make a synchronous transfer in bluetooth, one must:
- send the request packet
- block (wait_event_*())
- when the response is received in the input handler, wake_up_*().

There's not really any code duplication, mostly because initiating
synchronous USB transfers (input and output) is easy (because of the
usb_*_msg() functions), while making synchronous Bluetooth transfers
must be done manually. If there's a nice, convenient, synchronous
function in Bluetooth similar to usb_control_msg() that I've missed,
then let me know, as it would simplify this whole thing.

See the Set/Get Feature patch, including USB support, here:
http://lkml.org/lkml/2010/6/9/222

Alan.

2010-07-09 08:01:37

[permalink] [raw]

Subject: Re: [PATCH 1/1] Bluetooth: hidp: Add support for hidraw HIDIOCGFEATURE and HIDIOCSFEATURE

Hi Alan,

> > I looked at this and I am bit worried that this should not be done in
> > this detail in the HIDP driver. Essentially HIDP is a pure transport
> > driver. It should not handle all these details. Can we make this a bit
> > easier for the transport drivers to support such features?
>
> I put these changes (most notably the addition of hidp_get_raw_report())
> in hidp because that's where the parallel function
> hidp_output_raw_report() was already located. I figured the input should
> go with the output. That said, if there's a better place for both of
> them (input and output) to go, let me know where you think it should be,
> and I'll get them moved into the proper spot.
>
> I'm not sure what you mean about HIDP being a pure transport driver.

what is usb-hid.ko doing here? I would expect a bunch of code
duplication with minor difference between USB and Bluetooth.

Regards

Marcel

2010-07-09 03:51:56

[permalink] [raw]

Subject: Re: [PATCH 1/1] Bluetooth: hidp: Add support for hidraw HIDIOCGFEATURE and HIDIOCSFEATURE

On 07/08/2010 05:11 PM, Marcel Holtmann wrote:
> I looked at this and I am bit worried that this should not be done in
> this detail in the HIDP driver. Essentially HIDP is a pure transport
> driver. It should not handle all these details. Can we make this a bit
> easier for the transport drivers to support such features?
>
> Regards
>
> Marcel
>

Hi Marcel,

I put these changes (most notably the addition of hidp_get_raw_report())
in hidp because that's where the parallel function
hidp_output_raw_report() was already located. I figured the input should
go with the output. That said, if there's a better place for both of
them (input and output) to go, let me know where you think it should be,
and I'll get them moved into the proper spot.

I'm not sure what you mean about HIDP being a pure transport driver.

Alan.

2010-07-08 21:11:25

[permalink] [raw]

Subject: Re: [PATCH 1/1] Bluetooth: hidp: Add support for hidraw HIDIOCGFEATURE and HIDIOCSFEATURE

Hi Alan,

> This patch adds support or getting and setting feature reports for bluetooth
> HID devices from HIDRAW.
>
> Signed-off-by: Alan Ott <[email protected]>
> ---
> net/bluetooth/hidp/core.c | 121 +++++++++++++++++++++++++++++++++++++++++++--
> net/bluetooth/hidp/hidp.h | 8 +++
> 2 files changed, 125 insertions(+), 4 deletions(-)
>
> diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
> index bfe641b..0f068a0 100644
> --- a/net/bluetooth/hidp/core.c
> +++ b/net/bluetooth/hidp/core.c
> @@ -36,6 +36,7 @@
> #include <linux/file.h>
> #include <linux/init.h>
> #include <linux/wait.h>
> +#include <linux/mutex.h>
> #include <net/sock.h>
>
> #include <linux/input.h>
> @@ -313,6 +314,93 @@ static int hidp_send_report(struct hidp_session *session, struct hid_report *rep
> return hidp_queue_report(session, buf, rsize);
> }
>
> +static int hidp_get_raw_report(struct hid_device *hid,
> + unsigned char report_number,
> + unsigned char *data, size_t count,
> + unsigned char report_type)
> +{
> + struct hidp_session *session = hid->driver_data;
> + struct sk_buff *skb;
> + size_t len;
> + int numbered_reports = hid->report_enum[report_type].numbered;
> +
> + switch (report_type) {
> + case HID_FEATURE_REPORT:
> + report_type = HIDP_TRANS_GET_REPORT | HIDP_DATA_RTYPE_FEATURE;
> + break;
> + case HID_INPUT_REPORT:
> + report_type = HIDP_TRANS_GET_REPORT | HIDP_DATA_RTYPE_INPUT;
> + break;
> + case HID_OUTPUT_REPORT:
> + report_type = HIDP_TRANS_GET_REPORT | HIDP_DATA_RTYPE_OUPUT;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + if (mutex_lock_interruptible(&session->report_mutex))
> + return -ERESTARTSYS;
> +
> + /* Set up our wait, and send the report request to the device. */
> + session->waiting_report_type = report_type & HIDP_DATA_RTYPE_MASK;
> + session->waiting_report_number = numbered_reports ? report_number : -1;
> + set_bit(HIDP_WAITING_FOR_RETURN, &session->flags);
> + data[0] = report_number;
> + if (hidp_send_ctrl_message(hid->driver_data, report_type, data, 1))
> + goto err_eio;
> +
> + /* Wait for the return of the report. The returned report
> + gets put in session->report_return. */
> + while (test_bit(HIDP_WAITING_FOR_RETURN, &session->flags)) {
> + int res;
> +
> + res = wait_event_interruptible_timeout(session->report_queue,
> + !test_bit(HIDP_WAITING_FOR_RETURN, &session->flags),
> + 5*HZ);
> + if (res == 0) {
> + /* timeout */
> + goto err_eio;
> + }
> + if (res < 0) {
> + /* signal */
> + goto err_restartsys;
> + }
> + }
> +
> + skb = session->report_return;
> + if (skb) {
> + if (numbered_reports) {
> + /* Strip off the report number. */
> + size_t rpt_len = skb->len-1;
> + len = rpt_len < count ? rpt_len : count;
> + memcpy(data, skb->data+1, len);
> + } else {
> + len = skb->len < count ? skb->len : count;
> + memcpy(data, skb->data, len);
> + }
> +
> + kfree_skb(skb);
> + session->report_return = NULL;
> + } else {
> + /* Device returned a HANDSHAKE, indicating protocol error. */
> + len = -EIO;
> + }
> +
> + clear_bit(HIDP_WAITING_FOR_RETURN, &session->flags);
> + mutex_unlock(&session->report_mutex);
> +
> + return len;
> +
> +err_restartsys:
> + clear_bit(HIDP_WAITING_FOR_RETURN, &session->flags);
> + mutex_unlock(&session->report_mutex);
> + return -ERESTARTSYS;
> +err_eio:
> + clear_bit(HIDP_WAITING_FOR_RETURN, &session->flags);
> + mutex_unlock(&session->report_mutex);
> + return -EIO;
> +}
> +
> static int hidp_output_raw_report(struct hid_device *hid, unsigned char *data, size_t count,
> unsigned char report_type)
> {
> @@ -367,6 +455,10 @@ static void hidp_process_handshake(struct hidp_session *session,
> case HIDP_HSHK_ERR_INVALID_REPORT_ID:
> case HIDP_HSHK_ERR_UNSUPPORTED_REQUEST:
> case HIDP_HSHK_ERR_INVALID_PARAMETER:
> + if (test_bit(HIDP_WAITING_FOR_RETURN, &session->flags)) {
> + clear_bit(HIDP_WAITING_FOR_RETURN, &session->flags);
> + wake_up_interruptible(&session->report_queue);
> + }
> /* FIXME: Call into SET_ GET_ handlers here */
> break;
>
> @@ -403,9 +495,11 @@ static void hidp_process_hid_control(struct hidp_session *session,
> }
> }
>
> -static void hidp_process_data(struct hidp_session *session, struct sk_buff *skb,
> +/* Returns true if the passed-in skb should be freed by the caller. */
> +static int hidp_process_data(struct hidp_session *session, struct sk_buff *skb,
> unsigned char param)
> {
> + int done_with_skb = 1;
> BT_DBG("session %p skb %p len %d param 0x%02x", session, skb, skb->len, param);
>
> switch (param) {
> @@ -417,7 +511,6 @@ static void hidp_process_data(struct hidp_session *session, struct sk_buff *skb,
>
> if (session->hid)
> hid_input_report(session->hid, HID_INPUT_REPORT, skb->data, skb->len, 0);
> -
> break;
>
> case HIDP_DATA_RTYPE_OTHER:
> @@ -429,12 +522,27 @@ static void hidp_process_data(struct hidp_session *session, struct sk_buff *skb,
> __hidp_send_ctrl_message(session,
> HIDP_TRANS_HANDSHAKE | HIDP_HSHK_ERR_INVALID_PARAMETER, NULL, 0);
> }
> +
> + if (test_bit(HIDP_WAITING_FOR_RETURN, &session->flags) &&
> + param == session->waiting_report_type) {
> + if (session->waiting_report_number < 0 ||
> + session->waiting_report_number == skb->data[0]) {
> + /* hidp_get_raw_report() is waiting on this report. */
> + session->report_return = skb;
> + done_with_skb = 0;
> + clear_bit(HIDP_WAITING_FOR_RETURN, &session->flags);
> + wake_up_interruptible(&session->report_queue);
> + }
> + }
> +
> + return done_with_skb;
> }
>
> static void hidp_recv_ctrl_frame(struct hidp_session *session,
> struct sk_buff *skb)
> {
> unsigned char hdr, type, param;
> + int free_skb = 1;
>
> BT_DBG("session %p skb %p len %d", session, skb, skb->len);
>
> @@ -454,7 +562,7 @@ static void hidp_recv_ctrl_frame(struct hidp_session *session,
> break;
>
> case HIDP_TRANS_DATA:
> - hidp_process_data(session, skb, param);
> + free_skb = hidp_process_data(session, skb, param);
> break;
>
> default:
> @@ -463,7 +571,8 @@ static void hidp_recv_ctrl_frame(struct hidp_session *session,
> break;
> }
>
> - kfree_skb(skb);
> + if (free_skb)
> + kfree_skb(skb);
> }
>
> static void hidp_recv_intr_frame(struct hidp_session *session,
> @@ -797,6 +906,7 @@ static int hidp_setup_hid(struct hidp_session *session,
> hid->dev.parent = hidp_get_device(session);
> hid->ll_driver = &hidp_hid_driver;
>
> + hid->hid_get_raw_report = hidp_get_raw_report;
> hid->hid_output_raw_report = hidp_output_raw_report;
>
> err = hid_add_device(hid);
> @@ -857,6 +967,9 @@ int hidp_add_connection(struct hidp_connadd_req *req, struct socket *ctrl_sock,
> skb_queue_head_init(&session->ctrl_transmit);
> skb_queue_head_init(&session->intr_transmit);
>
> + mutex_init(&session->report_mutex);
> + init_waitqueue_head(&session->report_queue);
> +
> session->flags = req->flags & (1 << HIDP_BLUETOOTH_VENDOR_ID);
> session->idle_to = req->idle_to;
>
> diff --git a/net/bluetooth/hidp/hidp.h b/net/bluetooth/hidp/hidp.h
> index 8d934a1..00e71dd 100644
> --- a/net/bluetooth/hidp/hidp.h
> +++ b/net/bluetooth/hidp/hidp.h
> @@ -80,6 +80,7 @@
> #define HIDP_VIRTUAL_CABLE_UNPLUG 0
> #define HIDP_BOOT_PROTOCOL_MODE 1
> #define HIDP_BLUETOOTH_VENDOR_ID 9
> +#define HIDP_WAITING_FOR_RETURN 10
>
> struct hidp_connadd_req {
> int ctrl_sock; // Connected control socket
> @@ -154,6 +155,13 @@ struct hidp_session {
> struct sk_buff_head ctrl_transmit;
> struct sk_buff_head intr_transmit;
>
> + /* Used in hidp_get_raw_report() */
> + int waiting_report_type; /* HIDP_DATA_RTYPE_* */
> + int waiting_report_number; /* -1 for not numbered */
> + struct mutex report_mutex;
> + struct sk_buff *report_return;
> + wait_queue_head_t report_queue;
> +
> /* Report descriptor */
> __u8 *rd_data;
> uint rd_size;

I looked at this and I am bit worried that this should not be done in
this detail in the HIDP driver. Essentially HIDP is a pure transport
driver. It should not handle all these details. Can we make this a bit
easier for the transport drivers to support such features?

Regards

Marcel

2010-07-08 21:08:52

[permalink] [raw]

Subject: Re: [PATCH 1/1] Bluetooth: hidp: Add support for hidraw HIDIOCGFEATURE and HIDIOCSFEATURE

Hi Andrei,

> >> On Sun, 13 Jun 2010 18:20:01 -0400
> >> Alan Ott <[email protected]> wrote:
> >>
> >>> This patch adds support or getting and setting feature reports for bluetooth
> >>> HID devices from HIDRAW.
> >>>
> >>> Signed-off-by: Alan Ott <[email protected]>
> >>> ---
> >>
> >> Ping.
> >
> > We effectively don't have a bluetooth maintainer at the current point
> > in time. I've tried to let patches sit for a while hoping the listed
> > maintainer would do something, at least occaisionally, but that simply
> > isn't happening.
> > So I'll just pick patches up directly as I find time to review them,
> > but I have to warn that for me it's going to be done in a very low
> > priority way because I really don't find bluetooth all that exciting. :-)
>
> This would be good. We have a backlog of bluetooth kernel patches
> waiting for several months.
> This takes too much time to return to them again and again...
>
> Please keep this ML informed.

as soon as Ville is back from vacation, he should be helping out with
patch review and maintenance.

Regards

Marcel

2010-08-23 13:00:14

[permalink] [raw]

Subject: Re: [PATCH v4 0/2] Get and Set Feature Reports on HIDRAW (USB and Bluetooth)

On Mon, 16 Aug 2010, Alan Ott wrote:

> This is version 4. Built against 2.6.35+ revision 320b2b8de12698 .
>
> Alan Ott (2):
> HID: Add Support for Setting and Getting Feature Reports from hidraw
> Bluetooth: hidp: Add support for hidraw HIDIOCGFEATURE and
> HIDIOCSFEATURE
>
> drivers/hid/hidraw.c | 105 ++++++++++++++++++++++++++++++++++++--
> drivers/hid/usbhid/hid-core.c | 37 +++++++++++++-
> include/linux/hid.h | 3 +
> include/linux/hidraw.h | 3 +
> net/bluetooth/hidp/core.c | 114 +++++++++++++++++++++++++++++++++++++++--
> net/bluetooth/hidp/hidp.h | 8 +++
> 6 files changed, 260 insertions(+), 10 deletions(-)

Marcel, as per our previous discussion -- what is your word on this? I'd
be glad taking it once you Ack the bluetooth bits (which, as far as I
understood from your last mail, don't have strong objections against any
more).

Thanks,

--
Jiri Kosina
SUSE Labs, Novell Inc.

2010-08-16 20:20:59

[permalink] [raw]

Subject: [PATCH v4 2/2] Bluetooth: hidp: Add support for hidraw HIDIOCGFEATURE and HIDIOCSFEATURE

This patch adds support or getting and setting feature reports for bluetooth
HID devices from HIDRAW.

Signed-off-by: Alan Ott <[email protected]>
---
net/bluetooth/hidp/core.c | 114 +++++++++++++++++++++++++++++++++++++++++++--
net/bluetooth/hidp/hidp.h | 8 +++
2 files changed, 118 insertions(+), 4 deletions(-)

diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index bfe641b..0e4880e 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -36,6 +36,7 @@
#include <linux/file.h>
#include <linux/init.h>
#include <linux/wait.h>
+#include <linux/mutex.h>
#include <net/sock.h>

#include <linux/input.h>
@@ -313,6 +314,86 @@ static int hidp_send_report(struct hidp_session *session, struct hid_report *rep
return hidp_queue_report(session, buf, rsize);
}

+static int hidp_get_raw_report(struct hid_device *hid,
+ unsigned char report_number,
+ unsigned char *data, size_t count,
+ unsigned char report_type)
+{
+ struct hidp_session *session = hid->driver_data;
+ struct sk_buff *skb;
+ size_t len;
+ int numbered_reports = hid->report_enum[report_type].numbered;
+
+ switch (report_type) {
+ case HID_FEATURE_REPORT:
+ report_type = HIDP_TRANS_GET_REPORT | HIDP_DATA_RTYPE_FEATURE;
+ break;
+ case HID_INPUT_REPORT:
+ report_type = HIDP_TRANS_GET_REPORT | HIDP_DATA_RTYPE_INPUT;
+ break;
+ case HID_OUTPUT_REPORT:
+ report_type = HIDP_TRANS_GET_REPORT | HIDP_DATA_RTYPE_OUPUT;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ if (mutex_lock_interruptible(&session->report_mutex))
+ return -ERESTARTSYS;
+
+ /* Set up our wait, and send the report request to the device. */
+ session->waiting_report_type = report_type & HIDP_DATA_RTYPE_MASK;
+ session->waiting_report_number = numbered_reports ? report_number : -1;
+ set_bit(HIDP_WAITING_FOR_RETURN, &session->flags);
+ data[0] = report_number;
+ if (hidp_send_ctrl_message(hid->driver_data, report_type, data, 1))
+ goto err_eio;
+
+ /* Wait for the return of the report. The returned report
+ gets put in session->report_return. */
+ while (test_bit(HIDP_WAITING_FOR_RETURN, &session->flags)) {
+ int res;
+
+ res = wait_event_interruptible_timeout(session->report_queue,
+ !test_bit(HIDP_WAITING_FOR_RETURN, &session->flags),
+ 5*HZ);
+ if (res == 0) {
+ /* timeout */
+ goto err_eio;
+ }
+ if (res < 0) {
+ /* signal */
+ goto err_restartsys;
+ }
+ }
+
+ skb = session->report_return;
+ if (skb) {
+ len = skb->len < count ? skb->len : count;
+ memcpy(data, skb->data, len);
+
+ kfree_skb(skb);
+ session->report_return = NULL;
+ } else {
+ /* Device returned a HANDSHAKE, indicating protocol error. */
+ len = -EIO;
+ }
+
+ clear_bit(HIDP_WAITING_FOR_RETURN, &session->flags);
+ mutex_unlock(&session->report_mutex);
+
+ return len;
+
+err_restartsys:
+ clear_bit(HIDP_WAITING_FOR_RETURN, &session->flags);
+ mutex_unlock(&session->report_mutex);
+ return -ERESTARTSYS;
+err_eio:
+ clear_bit(HIDP_WAITING_FOR_RETURN, &session->flags);
+ mutex_unlock(&session->report_mutex);
+ return -EIO;
+}
+
static int hidp_output_raw_report(struct hid_device *hid, unsigned char *data, size_t count,
unsigned char report_type)
{
@@ -367,6 +448,10 @@ static void hidp_process_handshake(struct hidp_session *session,
case HIDP_HSHK_ERR_INVALID_REPORT_ID:
case HIDP_HSHK_ERR_UNSUPPORTED_REQUEST:
case HIDP_HSHK_ERR_INVALID_PARAMETER:
+ if (test_bit(HIDP_WAITING_FOR_RETURN, &session->flags)) {
+ clear_bit(HIDP_WAITING_FOR_RETURN, &session->flags);
+ wake_up_interruptible(&session->report_queue);
+ }
/* FIXME: Call into SET_ GET_ handlers here */
break;

@@ -403,9 +488,11 @@ static void hidp_process_hid_control(struct hidp_session *session,
}
}

-static void hidp_process_data(struct hidp_session *session, struct sk_buff *skb,
+/* Returns true if the passed-in skb should be freed by the caller. */
+static int hidp_process_data(struct hidp_session *session, struct sk_buff *skb,
unsigned char param)
{
+ int done_with_skb = 1;
BT_DBG("session %p skb %p len %d param 0x%02x", session, skb, skb->len, param);

switch (param) {
@@ -417,7 +504,6 @@ static void hidp_process_data(struct hidp_session *session, struct sk_buff *skb,

if (session->hid)
hid_input_report(session->hid, HID_INPUT_REPORT, skb->data, skb->len, 0);
-
break;

case HIDP_DATA_RTYPE_OTHER:
@@ -429,12 +515,27 @@ static void hidp_process_data(struct hidp_session *session, struct sk_buff *skb,
__hidp_send_ctrl_message(session,
HIDP_TRANS_HANDSHAKE | HIDP_HSHK_ERR_INVALID_PARAMETER, NULL, 0);
}
+
+ if (test_bit(HIDP_WAITING_FOR_RETURN, &session->flags) &&
+ param == session->waiting_report_type) {
+ if (session->waiting_report_number < 0 ||
+ session->waiting_report_number == skb->data[0]) {
+ /* hidp_get_raw_report() is waiting on this report. */
+ session->report_return = skb;
+ done_with_skb = 0;
+ clear_bit(HIDP_WAITING_FOR_RETURN, &session->flags);
+ wake_up_interruptible(&session->report_queue);
+ }
+ }
+
+ return done_with_skb;
}

static void hidp_recv_ctrl_frame(struct hidp_session *session,
struct sk_buff *skb)
{
unsigned char hdr, type, param;
+ int free_skb = 1;

BT_DBG("session %p skb %p len %d", session, skb, skb->len);

@@ -454,7 +555,7 @@ static void hidp_recv_ctrl_frame(struct hidp_session *session,
break;

case HIDP_TRANS_DATA:
- hidp_process_data(session, skb, param);
+ free_skb = hidp_process_data(session, skb, param);
break;

default:
@@ -463,7 +564,8 @@ static void hidp_recv_ctrl_frame(struct hidp_session *session,
break;
}

- kfree_skb(skb);
+ if (free_skb)
+ kfree_skb(skb);
}

static void hidp_recv_intr_frame(struct hidp_session *session,
@@ -797,6 +899,7 @@ static int hidp_setup_hid(struct hidp_session *session,
hid->dev.parent = hidp_get_device(session);
hid->ll_driver = &hidp_hid_driver;

+ hid->hid_get_raw_report = hidp_get_raw_report;
hid->hid_output_raw_report = hidp_output_raw_report;

err = hid_add_device(hid);
@@ -857,6 +960,9 @@ int hidp_add_connection(struct hidp_connadd_req *req, struct socket *ctrl_sock,
skb_queue_head_init(&session->ctrl_transmit);
skb_queue_head_init(&session->intr_transmit);

+ mutex_init(&session->report_mutex);
+ init_waitqueue_head(&session->report_queue);
+
session->flags = req->flags & (1 << HIDP_BLUETOOTH_VENDOR_ID);
session->idle_to = req->idle_to;

diff --git a/net/bluetooth/hidp/hidp.h b/net/bluetooth/hidp/hidp.h
index 8d934a1..00e71dd 100644
--- a/net/bluetooth/hidp/hidp.h
+++ b/net/bluetooth/hidp/hidp.h
@@ -80,6 +80,7 @@
#define HIDP_VIRTUAL_CABLE_UNPLUG 0
#define HIDP_BOOT_PROTOCOL_MODE 1
#define HIDP_BLUETOOTH_VENDOR_ID 9
+#define HIDP_WAITING_FOR_RETURN 10

struct hidp_connadd_req {
int ctrl_sock; // Connected control socket
@@ -154,6 +155,13 @@ struct hidp_session {
struct sk_buff_head ctrl_transmit;
struct sk_buff_head intr_transmit;

+ /* Used in hidp_get_raw_report() */
+ int waiting_report_type; /* HIDP_DATA_RTYPE_* */
+ int waiting_report_number; /* -1 for not numbered */
+ struct mutex report_mutex;
+ struct sk_buff *report_return;
+ wait_queue_head_t report_queue;
+
/* Report descriptor */
__u8 *rd_data;
uint rd_size;
--
1.7.0.4

2010-08-16 20:20:58

[permalink] [raw]

Subject: [PATCH v4 1/2] HID: Add Support for Setting and Getting Feature Reports from hidraw

Per the HID Specification, Feature reports must be sent and received on
the Configuration endpoint (EP 0) through the Set_Report/Get_Report
interfaces. This patch adds two ioctls to hidraw to set and get feature
reports to and from the device. Modifications were made to hidraw and
usbhid.

New hidraw ioctls:
HIDIOCSFEATURE - Perform a Set_Report transfer of a Feature report.
HIDIOCGFEATURE - Perform a Get_Report transfer of a Feature report.

Signed-off-by: Alan Ott <[email protected]>
---
drivers/hid/hidraw.c | 105 +++++++++++++++++++++++++++++++++++++++--
drivers/hid/usbhid/hid-core.c | 37 ++++++++++++++-
include/linux/hid.h | 3 +
include/linux/hidraw.h | 3 +
4 files changed, 142 insertions(+), 6 deletions(-)

diff --git a/drivers/hid/hidraw.c b/drivers/hid/hidraw.c
index 47d70c5..7df1310 100644
--- a/drivers/hid/hidraw.c
+++ b/drivers/hid/hidraw.c
@@ -101,14 +101,14 @@ out:
}

/* the first byte is expected to be a report number */
-static ssize_t hidraw_write(struct file *file, const char __user *buffer, size_t count, loff_t *ppos)
+/* This function is to be called with the minors_lock mutex held */
+static ssize_t hidraw_send_report(struct file *file, const char __user *buffer, size_t count, unsigned char report_type)
{
unsigned int minor = iminor(file->f_path.dentry->d_inode);
struct hid_device *dev;
__u8 *buf;
int ret = 0;

- mutex_lock(&minors_lock);
dev = hidraw_table[minor]->hid;

if (!dev->hid_output_raw_report) {
@@ -141,14 +141,92 @@ static ssize_t hidraw_write(struct file *file, const char __user *buffer, size_t
goto out_free;
}

- ret = dev->hid_output_raw_report(dev, buf, count, HID_OUTPUT_REPORT);
+ ret = dev->hid_output_raw_report(dev, buf, count, report_type);
out_free:
kfree(buf);
out:
+ return ret;
+}
+
+/* the first byte is expected to be a report number */
+static ssize_t hidraw_write(struct file *file, const char __user *buffer, size_t count, loff_t *ppos)
+{
+ ssize_t ret;
+ mutex_lock(&minors_lock);
+ ret = hidraw_send_report(file, buffer, count, HID_OUTPUT_REPORT);
mutex_unlock(&minors_lock);
return ret;
}

+
+/* This function performs a Get_Report transfer over the control endpoint
+ per section 7.2.1 of the HID specification, version 1.1. The first byte
+ of buffer is the report number to request, or 0x0 if the defice does not
+ use numbered reports. The report_type parameter can be HID_FEATURE_REPORT
+ or HID_INPUT_REPORT. This function is to be called with the minors_lock
+ mutex held. */
+static ssize_t hidraw_get_report(struct file *file, char __user *buffer, size_t count, unsigned char report_type)
+{
+ unsigned int minor = iminor(file->f_path.dentry->d_inode);
+ struct hid_device *dev;
+ __u8 *buf;
+ int ret = 0, len;
+ unsigned char report_number;
+
+ dev = hidraw_table[minor]->hid;
+
+ if (!dev->hid_get_raw_report) {
+ ret = -ENODEV;
+ goto out;
+ }
+
+ if (count > HID_MAX_BUFFER_SIZE) {
+ printk(KERN_WARNING "hidraw: pid %d passed too large report\n",
+ task_pid_nr(current));
+ ret = -EINVAL;
+ goto out;
+ }
+
+ if (count < 2) {
+ printk(KERN_WARNING "hidraw: pid %d passed too short report\n",
+ task_pid_nr(current));
+ ret = -EINVAL;
+ goto out;
+ }
+
+ buf = kmalloc(count * sizeof(__u8), GFP_KERNEL);
+ if (!buf) {
+ ret = -ENOMEM;
+ goto out;
+ }
+
+ /* Read the first byte from the user. This is the report number,
+ which is passed to dev->hid_get_raw_report(). */
+ if (copy_from_user(&report_number, buffer, 1)) {
+ ret = -EFAULT;
+ goto out_free;
+ }
+
+ ret = dev->hid_get_raw_report(dev, report_number, buf, count, report_type);
+
+ if (ret < 0)
+ goto out_free;
+
+ len = (ret < count) ? ret : count;
+
+ if (copy_to_user(buffer, buf, len)) {
+ ret = -EFAULT;
+ goto out_free;
+ }
+
+ ret = len;
+
+out_free:
+ kfree(buf);
+out:
+ return ret;
+}
+
static unsigned int hidraw_poll(struct file *file, poll_table *wait)
{
struct hidraw_list *list = file->private_data;
@@ -281,7 +359,24 @@ static long hidraw_ioctl(struct file *file, unsigned int cmd,
default:
{
struct hid_device *hid = dev->hid;
- if (_IOC_TYPE(cmd) != 'H' || _IOC_DIR(cmd) != _IOC_READ) {
+ if (_IOC_TYPE(cmd) != 'H') {
+ ret = -EINVAL;
+ break;
+ }
+
+ if (_IOC_NR(cmd) == _IOC_NR(HIDIOCSFEATURE(0))) {
+ int len = _IOC_SIZE(cmd);
+ ret = hidraw_send_report(file, user_arg, len, HID_FEATURE_REPORT);
+ break;
+ }
+ if (_IOC_NR(cmd) == _IOC_NR(HIDIOCGFEATURE(0))) {
+ int len = _IOC_SIZE(cmd);
+ ret = hidraw_get_report(file, user_arg, len, HID_FEATURE_REPORT);
+ break;
+ }
+
+ /* Begin Read-only ioctls. */
+ if (_IOC_DIR(cmd) != _IOC_READ) {
ret = -EINVAL;
break;
}
@@ -313,7 +408,7 @@ static long hidraw_ioctl(struct file *file, unsigned int cmd,
-EFAULT : len;
break;
}
- }
+ }

ret = -ENOTTY;
}
diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index b729c02..93db724 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -798,6 +798,40 @@ static int hid_alloc_buffers(struct usb_device *dev, struct hid_device *hid)
return 0;
}

+static int usbhid_get_raw_report(struct hid_device *hid,
+ unsigned char report_number, __u8 *buf, size_t count,
+ unsigned char report_type)
+{
+ struct usbhid_device *usbhid = hid->driver_data;
+ struct usb_device *dev = hid_to_usb_dev(hid);
+ struct usb_interface *intf = usbhid->intf;
+ struct usb_host_interface *interface = intf->cur_altsetting;
+ int skipped_report_id = 0;
+ int ret;
+
+ /* Byte 0 is the report number. Report data starts at byte 1.*/
+ buf[0] = report_number;
+ if (report_number == 0x0) {
+ /* Offset the return buffer by 1, so that the report ID
+ will remain in byte 0. */
+ buf++;
+ count--;
+ skipped_report_id = 1;
+ }
+ ret = usb_control_msg(dev, usb_rcvctrlpipe(dev, 0),
+ HID_REQ_GET_REPORT,
+ USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
+ ((report_type + 1) << 8) | report_number,
+ interface->desc.bInterfaceNumber, buf, count,
+ USB_CTRL_SET_TIMEOUT);
+
+ /* count also the report id */
+ if (ret > 0 && skipped_report_id)
+ ret++;
+
+ return ret;
+}
+
static int usbhid_output_raw_report(struct hid_device *hid, __u8 *buf, size_t count,
unsigned char report_type)
{
@@ -807,7 +841,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) {
@@ -1149,6 +1183,7 @@ static int usbhid_probe(struct usb_interface *intf, const struct usb_device_id *

usb_set_intfdata(intf, hid);
hid->ll_driver = &usb_hid_driver;
+ hid->hid_get_raw_report = usbhid_get_raw_report;
hid->hid_output_raw_report = usbhid_output_raw_report;
hid->ff_init = hid_pidff_init;
#ifdef CONFIG_USB_HIDDEV
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 42a0f1d..1b95b0b 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -503,6 +503,9 @@ struct hid_device { /* device report descriptor */
struct hid_usage *, __s32);
void (*hiddev_report_event) (struct hid_device *, struct hid_report *);

+ /* handler for raw input (Get_Report) data, used by hidraw */
+ int (*hid_get_raw_report) (struct hid_device *, unsigned char, __u8 *, size_t, unsigned char);
+
/* handler for raw output data, used by hidraw */
int (*hid_output_raw_report) (struct hid_device *, __u8 *, size_t, unsigned char);

diff --git a/include/linux/hidraw.h b/include/linux/hidraw.h
index dd8d692..4b88e69 100644
--- a/include/linux/hidraw.h
+++ b/include/linux/hidraw.h
@@ -35,6 +35,9 @@ struct hidraw_devinfo {
#define HIDIOCGRAWINFO _IOR('H', 0x03, struct hidraw_devinfo)
#define HIDIOCGRAWNAME(len) _IOC(_IOC_READ, 'H', 0x04, len)
#define HIDIOCGRAWPHYS(len) _IOC(_IOC_READ, 'H', 0x05, len)
+/* The first byte of SFEATURE and GFEATURE is the report number */
+#define HIDIOCSFEATURE(len) _IOC(_IOC_WRITE|_IOC_READ, 'H', 0x06, len)
+#define HIDIOCGFEATURE(len) _IOC(_IOC_WRITE|_IOC_READ, 'H', 0x07, len)

#define HIDRAW_FIRST_MINOR 0
#define HIDRAW_MAX_DEVICES 64
--
1.7.0.4

2010-08-16 20:20:57

[permalink] [raw]

Subject: [PATCH v4 0/2] Get and Set Feature Reports on HIDRAW (USB and Bluetooth)

This is version 4. Built against 2.6.35+ revision 320b2b8de12698 .

Alan Ott (2):
HID: Add Support for Setting and Getting Feature Reports from hidraw
Bluetooth: hidp: Add support for hidraw HIDIOCGFEATURE and
HIDIOCSFEATURE

drivers/hid/hidraw.c | 105 ++++++++++++++++++++++++++++++++++++--
drivers/hid/usbhid/hid-core.c | 37 +++++++++++++-
include/linux/hid.h | 3 +
include/linux/hidraw.h | 3 +
net/bluetooth/hidp/core.c | 114 +++++++++++++++++++++++++++++++++++++++--
net/bluetooth/hidp/hidp.h | 8 +++
6 files changed, 260 insertions(+), 10 deletions(-)

2010-08-10 12:12:47

[permalink] [raw]

Subject: Re: [PATCH 1/1] Bluetooth: hidp: Add support for hidraw HIDIOCGFEATURE and HIDIOCSFEATURE

Hi Jiri,

> > > that the ioctl() API is synchronous is fine to me, however pushing that
> > > down to the transport drivers seems wrong to me. I think the HID core
> > > should be able to handle a fully asynchronous transport driver. I know
> > > that with the USB subsystem you are little bit spoiled here, but for
> > > Bluetooth it is not the case. And in the end even using the asynchronous
> > > USB URB calls would be nice as well.
> > >
> >
> > How are the URB calls better than using the synchronous calls? (see below)
>
> Hi,
>
> I think this was the last mail in the thread so far, right?
>
> > > So why not make the core actually wait for responses from the transport
> > > driver.
> >
> > Because this makes the USB side a lot more complicated, and it would
> > introduce transport specific code into the core. Further, it would
> > involve the transport driver calling hidraw with _every_ single packet
> > it receives. Further, it would have to call hidraw with HANDSHAKE
> > protocol error packets as well.
> >
> > > I would make the transport drivers a lot simpler in the long
> > > run.
> >
> > It would make the USB transport driver and drivers/hid/hidraw much more
> > complicated right now, at the expense of making the BT transport driver
> > marginally simpler (and I'm not even convinced whether it would really be
> > simpler). (see below for more)
>
> Seconded.
>
> > > And I know that most likely besides Bluetooth and USB you won't see
> > > another, but you never know.
> > >
> > I just don't understand the objection. In each transport type, the waiting
> > will have to be done in a different way. USB and BT are different enough that
> > this is the case already, without having to imagine future buses which use
> > HID. In BT, you have to check each packet which comes back from the BT network
> > to see whether it is the response packet that hidraw is waiting for. Further,
> > you have to check for HANDSHAKE packets indicating protocol error. Where
> > better for this to be done than in hidp? Further, how can this possibly happen
> > in drivers/hid/hidraw, as it doesn't know about the details of bluetooth to
> > make this determination, and why should it? In my last email (
> > http://lkml.org/lkml/2010/7/9/231 ) to which I got no response, I laid out how
> > doing the blocking in drivers/hid/hidraw would only make all the parts except
> > bluetooth more complicated (including the core, and the USB side), and would
> > also introduce bluetooth-specific things into the core.
> >
> > Further, you're saying that using the asynchronous USB URB calls would be a
> > benefit. How is it a benefit to replace a single function call which does
> > exactly what I want, with a set of asynchronous calls and then adding my own
> > blocking to make it do the same thing? This sounds to me like it would be 1:
> > more text, 2: duplication of code, 3: error prone. I can't understand how this
> > is of benefit. Please explain to me what I'm missing.
> >
> > In theory, what you're saying makes sense. Making common code and logic
> > actually common is always good. In practice though, in this case, I submit
> > that there really isn't any commonality, and the only way for there to be
> > commonality is to do the USB side the hard way. Further, drivers/hid/hidraw
> > can't wait for a bluetooth packet without having code that's
> > bluetooth-specific. It seems just that simple to me.
> >
> > I'll give it some more thought, and take another look at the code to see if
> > there's something obvious that I'm missing. If you know what I'm missing in my
> > understanding of the problem, please tell me :)
>
> Marcel, did you have time to review Alan's explanation a little bit?
>
> I must say I would really like to have this feature merged, but of course
> not if you completely disagree .. but then we'll have to find some
> consensus. Currently Alan's summary above quite aligns with my opinion.

my opinion is still that we should make the core do the async handling.
I think that we let USB dictate how APIs for HID should look like.

However that is maybe fine anyway since the Bluetooth HID guys where not
really inventive since they copying USB HID for the better and mostly
for the worst. Especially since Bluetooth doesn't have the endpoint
direction limits like USB does. Anyhow, just get the patches re-based
and re-submitted and I can have a second look.

Regards

Marcel

2010-08-10 11:46:51

[permalink] [raw]

Subject: Re: [PATCH 1/1] Bluetooth: hidp: Add support for hidraw HIDIOCGFEATURE and HIDIOCSFEATURE

On Thu, 22 Jul 2010, Alan Ott wrote:

> > that the ioctl() API is synchronous is fine to me, however pushing that
> > down to the transport drivers seems wrong to me. I think the HID core
> > should be able to handle a fully asynchronous transport driver. I know
> > that with the USB subsystem you are little bit spoiled here, but for
> > Bluetooth it is not the case. And in the end even using the asynchronous
> > USB URB calls would be nice as well.
> >
>
> How are the URB calls better than using the synchronous calls? (see below)

Hi,

I think this was the last mail in the thread so far, right?

> > So why not make the core actually wait for responses from the transport
> > driver.
>
> Because this makes the USB side a lot more complicated, and it would
> introduce transport specific code into the core. Further, it would
> involve the transport driver calling hidraw with _every_ single packet
> it receives. Further, it would have to call hidraw with HANDSHAKE
> protocol error packets as well.
>
> > I would make the transport drivers a lot simpler in the long
> > run.
>
> It would make the USB transport driver and drivers/hid/hidraw much more
> complicated right now, at the expense of making the BT transport driver
> marginally simpler (and I'm not even convinced whether it would really be
> simpler). (see below for more)

Seconded.

> > And I know that most likely besides Bluetooth and USB you won't see
> > another, but you never know.
> >
> I just don't understand the objection. In each transport type, the waiting
> will have to be done in a different way. USB and BT are different enough that
> this is the case already, without having to imagine future buses which use
> HID. In BT, you have to check each packet which comes back from the BT network
> to see whether it is the response packet that hidraw is waiting for. Further,
> you have to check for HANDSHAKE packets indicating protocol error. Where
> better for this to be done than in hidp? Further, how can this possibly happen
> in drivers/hid/hidraw, as it doesn't know about the details of bluetooth to
> make this determination, and why should it? In my last email (
> http://lkml.org/lkml/2010/7/9/231 ) to which I got no response, I laid out how
> doing the blocking in drivers/hid/hidraw would only make all the parts except
> bluetooth more complicated (including the core, and the USB side), and would
> also introduce bluetooth-specific things into the core.
>
> Further, you're saying that using the asynchronous USB URB calls would be a
> benefit. How is it a benefit to replace a single function call which does
> exactly what I want, with a set of asynchronous calls and then adding my own
> blocking to make it do the same thing? This sounds to me like it would be 1:
> more text, 2: duplication of code, 3: error prone. I can't understand how this
> is of benefit. Please explain to me what I'm missing.
>
> In theory, what you're saying makes sense. Making common code and logic
> actually common is always good. In practice though, in this case, I submit
> that there really isn't any commonality, and the only way for there to be
> commonality is to do the USB side the hard way. Further, drivers/hid/hidraw
> can't wait for a bluetooth packet without having code that's
> bluetooth-specific. It seems just that simple to me.
>
> I'll give it some more thought, and take another look at the code to see if
> there's something obvious that I'm missing. If you know what I'm missing in my
> understanding of the problem, please tell me :)

Marcel, did you have time to review Alan's explanation a little bit?

I must say I would really like to have this feature merged, but of course
not if you completely disagree .. but then we'll have to find some
consensus. Currently Alan's summary above quite aligns with my opinion.

Thanks,

--
Jiri Kosina
SUSE Labs, Novell Inc.

2010-09-28 13:30:11

[permalink] [raw]

Subject: Re: [PATCH v4 1/2] HID: Add Support for Setting and Getting Feature Reports from hidraw

On Mon, 16 Aug 2010 16:20:58 -0400
Alan Ott <[email protected]> wrote:

> Per the HID Specification, Feature reports must be sent and received on
> the Configuration endpoint (EP 0) through the Set_Report/Get_Report
> interfaces. This patch adds two ioctls to hidraw to set and get feature
> reports to and from the device. Modifications were made to hidraw and
> usbhid.
>
> New hidraw ioctls:
> HIDIOCSFEATURE - Perform a Set_Report transfer of a Feature report.
> HIDIOCGFEATURE - Perform a Get_Report transfer of a Feature report.
>
> Signed-off-by: Alan Ott <[email protected]>

Hi Alan, I am doing some stress testing on hidraw, if I have a loop with
HIDIOCGFEATURE on a given report and I disconnect the device while the
loop is running I get this:

BUG: unable to handle kernel NULL pointer dereference at 0000000000000028
IP: [<ffffffffa02c66b4>] hidraw_ioctl+0xfc/0x32c [hid]

Full log attached along with the test program, the device is a Sony PS3
Controller (sixaxis).

If my objdump analysis is right, hidraw_ioctl+0xfc should be around line
361 in hidraw.c (with your patch applied):

struct hid_device *hid = dev->hid;

It looks like 'dev' (which is hidraw_table[minor]) can be NULL
sometimes, can't it?
This is not introduced by your changes tho.

Just as a side note, the bug does not show up if the userspace program
handles return values properly and exits as soon as it gets an error
from the HID layer, see the XXX comment in test_hidraw_feature.c.

This fixes it, if it looks ok I will resend the patch rebased on
mainline code:

diff --git a/drivers/hid/hidraw.c b/drivers/hid/hidraw.c
index 7df1310..3c040c6 100644
--- a/drivers/hid/hidraw.c
+++ b/drivers/hid/hidraw.c
@@ -322,6 +322,10 @@ static long hidraw_ioctl(struct file *file, unsigned int cmd,

mutex_lock(&minors_lock);
dev = hidraw_table[minor];
+ if (!dev) {
+ ret = -ENODEV;
+ goto out;
+ }

switch (cmd) {
case HIDIOCGRDESCSIZE:
@@ -412,6 +416,7 @@ static long hidraw_ioctl(struct file *file, unsigned int cmd,

ret = -ENOTTY;
}
+out:
mutex_unlock(&minors_lock);
return ret;
}

this change covers also the other uses of hidraw_table[minor] in
hidraw_send_report/hidraw_get_report.

Regards,
Antonio

--
Antonio Ospite
http://ao2.it

PGP public key ID: 0x4553B001

A: Because it messes up the order in which people normally read text.
See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

Attachments:

(No filename) (2.48 kB)
dmesg_hidraw_feature_bug.log (3.82 kB)
test_hidraw_feature.c (1.17 kB)
(No filename) (198.00 B)
Download all attachments

2010-09-24 10:47:57

[permalink] [raw]

Subject: Re: [PATCH v4 2/2] Bluetooth: hidp: Add support for hidraw HIDIOCGFEATURE and HIDIOCSFEATURE

On Thu, 23 Sep 2010 10:16:43 -0400
Alan Ott <[email protected]> wrote:

>
> On Sep 23, 2010, at 7:51 AM, Ville Tervo wrote:
>
> > Hi Alan,
> >
> > One comment.
> >
> > How about a variable called ret and using that to return len or
> > errno? It
> > would eliminate code dublication.
> >
>
> Hi Ville,
>
> Where specifically? In which function? I've gone through it a couple
> of times and failed to find return statements which are superfluous.
> Maybe I'm missing something fundamental?
>

I guess he's referring to the return paths (normal, err_restartsys,
err_eio) in hidp_get_raw_report(), they could be merged into a
generic "out:" path by parametrizing the return value.

I don't know if Alan prefers explicit error paths, for me it's OK either
way.

Regards,
Antonio

--
Antonio Ospite
http://ao2.it

PGP public key ID: 0x4553B001

A: Because it messes up the order in which people normally read text.
See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

Attachments:

(No filename) (0.99 kB)
(No filename) (198.00 B)
Download all attachments

2010-09-23 23:40:02

[permalink] [raw]

Subject: Re: [PATCH v4 0/2] Get and Set Feature Reports on HIDRAW (USB and Bluetooth)

On 09/23/2010 12:25 PM, Ville Tervo wrote:
> Hi Alan,
>
> On Mon, Aug 16, 2010 at 10:20:57PM +0200, ext Alan Ott wrote:
>
>> This is version 4. Built against 2.6.35+ revision 320b2b8de12698 .
>>
>
> I gave a try to to this patch using your test tool [1] and very old BT
> keyboard. I don't have anything else ATM to test with. Is there some BT hid
> devices which support setting and getting features?
>

A keyboard is the only BT device I've used which has feature reports.

> Shouldn't HIDIOCSFEATURE's bt version have similar wait as HIDIOCGFEATURE to
> get report status back from the device? or is there even any status coming back
> in successful case? Sorry I'm a newbie with HID and trying to understand how
> this should work.
>
> Now it just returns num of send bytes even if the remote device returned some
> error. Which one is the expected behavior?
>

I made it function the same as the existing hidp_output_raw_report().
Nothing in net/bluetooth/hidp/core.c is acked at all. I'm not sure of
the reasons.

Jiri, Marcel, any ideas?

> Other problem is that Get report is getting now handshake from set report.
>
>
>

Yeah, that makes sense. I hadn't considered the set_report failing. In
that case, I guess it means we _must_ wait for an ack for sent packets.

Anyone else agree or disagree?

Alan.

2010-09-23 20:16:19

by Przemo Firszt

[permalink] [raw]

Subject: Re: [PATCH v4 0/2] Get and Set Feature Reports on HIDRAW (USB and Bluetooth)

Dnia 2010-09-23, czw o godzinie 10:07 -0700, Ping Cheng pisze:
> On Thu, Sep 23, 2010 at 9:25 AM, Ville Tervo <[email protected]> wrote:
> > Hi Alan,
> >
> > On Mon, Aug 16, 2010 at 10:20:57PM +0200, ext Alan Ott wrote:
> >> This is version 4. Built against 2.6.35+ revision 320b2b8de12698 .
> >
> > I gave a try to to this patch using your test tool [1] and very old BT
> > keyboard. I don't have anything else ATM to test with. Is there some BT hid
> > devices which support setting and getting features?
>
> As far as I know Wacom BT devices (Graphire and Intuos4) need to get
> and set features.
>
> Przemo,
>
> Do you have time to test the patchset with your Graphire BT and
> provide your result here?
Hi Ping,
Speed switching works as expected.

Tested on HP NC4200 with internal and external bluetooth modules:

Linux pldmachine 2.6.35-07788-g320b2b8-dirty #15 Thu Sep 23 19:55:03 IST
2010 i686 Intel(R)_Pentium(R)_M_processor_1.86GHz PLD Linux

ID 03f0:011d Hewlett-Packard Integrated Bluetooth Module
ID 0a12:0001 Cambridge Silicon Radio, Ltd Bluetooth Dongle (HCI mode)

and Wacom Pen Tablet CTE-630BT

Tested-by: Przemo Firszt <[email protected]>
--
Regards,
Przemo Firszt <[email protected]>

2010-09-23 17:07:07

by Ping Cheng

[permalink] [raw]

Subject: Re: [PATCH v4 0/2] Get and Set Feature Reports on HIDRAW (USB and Bluetooth)

On Thu, Sep 23, 2010 at 9:25 AM, Ville Tervo <[email protected]> wrote:
> Hi Alan,
>
> On Mon, Aug 16, 2010 at 10:20:57PM +0200, ext Alan Ott wrote:
>> This is version 4. Built against 2.6.35+ revision 320b2b8de12698 .
>
> I gave a try to to this patch using your test tool [1] and very old BT
> keyboard. I don't have anything else ATM to test with. Is there some BT hid
> devices which support setting and getting features?

As far as I know Wacom BT devices (Graphire and Intuos4) need to get
and set features.

Przemo,

Do you have time to test the patchset with your Graphire BT and
provide your result here?

Thank you,

Ping

2010-09-23 16:25:21

by Ville Tervo

[permalink] [raw]

Subject: Re: [PATCH v4 0/2] Get and Set Feature Reports on HIDRAW (USB and Bluetooth)

Hi Alan,

On Mon, Aug 16, 2010 at 10:20:57PM +0200, ext Alan Ott wrote:
> This is version 4. Built against 2.6.35+ revision 320b2b8de12698 .

I gave a try to to this patch using your test tool [1] and very old BT
keyboard. I don't have anything else ATM to test with. Is there some BT hid
devices which support setting and getting features?

Shouldn't HIDIOCSFEATURE's bt version have similar wait as HIDIOCGFEATURE to
get report status back from the device? or is there even any status coming back
in successful case? Sorry I'm a newbie with HID and trying to understand how
this should work.

Now it just returns num of send bytes even if the remote device returned some
error. Which one is the expected behavior?

Other problem is that Get report is getting now handshake from set report.

2010-09-23 17:55:46.680612 < ACL data: handle 38 flags 0x02 dlen 9
L2CAP(d): cid 0x008b len 5 [psm 17]
HIDP: Set report: Feature report
0000: 09 ff ff ff ....
2010-09-23 17:55:46.680653 < ACL data: handle 38 flags 0x02 dlen 6
L2CAP(d): cid 0x008b len 2 [psm 17]
HIDP: Get report: Feature report
0000: 09 .
2010-09-23 17:55:46.697577 > HCI Event: Number of Completed Packets (0x13) plen 5
handle 38 packets 1
2010-09-23 17:55:46.698579 > HCI Event: Number of Completed Packets (0x13) plen 5
handle 38 packets 1
2010-09-23 17:55:46.776827 > ACL data: handle 38 flags 0x02 dlen 5
L2CAP(d): cid 0x0040 len 1 [psm 17]
HIDP: Handshake: Invalid parameter
2010-09-23 17:55:46.777069 < ACL data: handle 38 flags 0x02 dlen 7
L2CAP(d): cid 0x008b len 3 [psm 17]
HIDP: Data: Output report
0000: 01 77 .w
2010-09-23 17:55:46.797577 > HCI Event: Number of Completed Packets (0x13) plen 5
handle 38 packets 1
2010-09-23 17:55:46.816826 > ACL data: handle 38 flags 0x02 dlen 5
L2CAP(d): cid 0x0040 len 1 [psm 17]
HIDP: Handshake: Invalid parameter
2010-09-23 17:55:46.856828 > ACL data: handle 38 flags 0x02 dlen 5
L2CAP(d): cid 0x0040 len 1 [psm 17]
HIDP: Handshake: Unsupported request

[1] http://lkml.org/lkml/2010/6/17/414

--
Ville the HID newbie

2010-09-23 14:16:43

[permalink] [raw]

Subject: Re: [PATCH v4 2/2] Bluetooth: hidp: Add support for hidraw HIDIOCGFEATURE and HIDIOCSFEATURE

On Sep 23, 2010, at 7:51 AM, Ville Tervo wrote:

> Hi Alan,
>
> One comment.
>
> How about a variable called ret and using that to return len or
> errno? It
> would eliminate code dublication.
>

Hi Ville,

Where specifically? In which function? I've gone through it a couple
of times and failed to find return statements which are superfluous.
Maybe I'm missing something fundamental?

Alan.

2010-09-23 11:51:08

by Ville Tervo

[permalink] [raw]

Subject: Re: [PATCH v4 2/2] Bluetooth: hidp: Add support for hidraw HIDIOCGFEATURE and HIDIOCSFEATURE

Hi Alan,

One comment.

On Mon, Aug 16, 2010 at 10:20:59PM +0200, ext Alan Ott wrote:
> This patch adds support or getting and setting feature reports for bluetooth
> HID devices from HIDRAW.
>
> Signed-off-by: Alan Ott <[email protected]>
> ---
> net/bluetooth/hidp/core.c | 114 +++++++++++++++++++++++++++++++++++++++++++--
> net/bluetooth/hidp/hidp.h | 8 +++
> 2 files changed, 118 insertions(+), 4 deletions(-)
>
> diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
> index bfe641b..0e4880e 100644
> --- a/net/bluetooth/hidp/core.c
> +++ b/net/bluetooth/hidp/core.c
> @@ -36,6 +36,7 @@
> #include <linux/file.h>
> #include <linux/init.h>
> #include <linux/wait.h>
> +#include <linux/mutex.h>
> #include <net/sock.h>
>
> #include <linux/input.h>
> @@ -313,6 +314,86 @@ static int hidp_send_report(struct hidp_session *session, struct hid_report *rep
> return hidp_queue_report(session, buf, rsize);
> }
>
> +static int hidp_get_raw_report(struct hid_device *hid,
> + unsigned char report_number,
> + unsigned char *data, size_t count,
> + unsigned char report_type)
> +{
> + struct hidp_session *session = hid->driver_data;
> + struct sk_buff *skb;
> + size_t len;
> + int numbered_reports = hid->report_enum[report_type].numbered;
> +
> + switch (report_type) {
> + case HID_FEATURE_REPORT:
> + report_type = HIDP_TRANS_GET_REPORT | HIDP_DATA_RTYPE_FEATURE;
> + break;
> + case HID_INPUT_REPORT:
> + report_type = HIDP_TRANS_GET_REPORT | HIDP_DATA_RTYPE_INPUT;
> + break;
> + case HID_OUTPUT_REPORT:
> + report_type = HIDP_TRANS_GET_REPORT | HIDP_DATA_RTYPE_OUPUT;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + if (mutex_lock_interruptible(&session->report_mutex))
> + return -ERESTARTSYS;
> +
> + /* Set up our wait, and send the report request to the device. */
> + session->waiting_report_type = report_type & HIDP_DATA_RTYPE_MASK;
> + session->waiting_report_number = numbered_reports ? report_number : -1;
> + set_bit(HIDP_WAITING_FOR_RETURN, &session->flags);
> + data[0] = report_number;
> + if (hidp_send_ctrl_message(hid->driver_data, report_type, data, 1))
> + goto err_eio;
> +
> + /* Wait for the return of the report. The returned report
> + gets put in session->report_return. */
> + while (test_bit(HIDP_WAITING_FOR_RETURN, &session->flags)) {
> + int res;
> +
> + res = wait_event_interruptible_timeout(session->report_queue,
> + !test_bit(HIDP_WAITING_FOR_RETURN, &session->flags),
> + 5*HZ);
> + if (res == 0) {
> + /* timeout */
> + goto err_eio;
> + }
> + if (res < 0) {
> + /* signal */
> + goto err_restartsys;
> + }
> + }
> +
> + skb = session->report_return;
> + if (skb) {
> + len = skb->len < count ? skb->len : count;
> + memcpy(data, skb->data, len);
> +
> + kfree_skb(skb);
> + session->report_return = NULL;
> + } else {
> + /* Device returned a HANDSHAKE, indicating protocol error. */
> + len = -EIO;
> + }
> +
> + clear_bit(HIDP_WAITING_FOR_RETURN, &session->flags);
> + mutex_unlock(&session->report_mutex);
> +
> + return len;
> +
> +err_restartsys:
> + clear_bit(HIDP_WAITING_FOR_RETURN, &session->flags);
> + mutex_unlock(&session->report_mutex);
> + return -ERESTARTSYS;
> +err_eio:
> + clear_bit(HIDP_WAITING_FOR_RETURN, &session->flags);
> + mutex_unlock(&session->report_mutex);
> + return -EIO;
> +}

How about a variable called ret and using that to return len or errno? It
would eliminate code dublication.

--
Ville

2010-09-22 12:09:25

[permalink] [raw]

Subject: Re: [PATCH v4 0/2] Get and Set Feature Reports on HIDRAW (USB and Bluetooth)

On Thu, 2 Sep 2010, Jiri Kosina wrote:

> > > This is version 4. Built against 2.6.35+ revision 320b2b8de12698 .
> > >
> > > Alan Ott (2):
> > > HID: Add Support for Setting and Getting Feature Reports from hidraw
> > > Bluetooth: hidp: Add support for hidraw HIDIOCGFEATURE and
> > > HIDIOCSFEATURE
> > >
> > > drivers/hid/hidraw.c | 105 ++++++++++++++++++++++++++++++++++++--
> > > drivers/hid/usbhid/hid-core.c | 37 +++++++++++++-
> > > include/linux/hid.h | 3 +
> > > include/linux/hidraw.h | 3 +
> > > net/bluetooth/hidp/core.c | 114 +++++++++++++++++++++++++++++++++++++++--
> > > net/bluetooth/hidp/hidp.h | 8 +++
> > > 6 files changed, 260 insertions(+), 10 deletions(-)
> >
> > Marcel, as per our previous discussion -- what is your word on this? I'd
> > be glad taking it once you Ack the bluetooth bits (which, as far as I
> > understood from your last mail, don't have strong objections against any
> > more).
>
> ... Marcel?
>
> I'd really like not to miss 2.6.37 merge window with this.

Seemingly I have not enought powers to get statement from Marcel here
these days/weeks.

Davem, would you perhaps be able to step in here?

Thanks,

--
Jiri Kosina
SUSE Labs, Novell Inc.

2010-09-02 15:25:18

[permalink] [raw]

Subject: Re: [PATCH v4 0/2] Get and Set Feature Reports on HIDRAW (USB and Bluetooth)

On Mon, 23 Aug 2010, Jiri Kosina wrote:

> > This is version 4. Built against 2.6.35+ revision 320b2b8de12698 .
> >
> > Alan Ott (2):
> > HID: Add Support for Setting and Getting Feature Reports from hidraw
> > Bluetooth: hidp: Add support for hidraw HIDIOCGFEATURE and
> > HIDIOCSFEATURE
> >
> > drivers/hid/hidraw.c | 105 ++++++++++++++++++++++++++++++++++++--
> > drivers/hid/usbhid/hid-core.c | 37 +++++++++++++-
> > include/linux/hid.h | 3 +
> > include/linux/hidraw.h | 3 +
> > net/bluetooth/hidp/core.c | 114 +++++++++++++++++++++++++++++++++++++++--
> > net/bluetooth/hidp/hidp.h | 8 +++
> > 6 files changed, 260 insertions(+), 10 deletions(-)
>
> Marcel, as per our previous discussion -- what is your word on this? I'd
> be glad taking it once you Ack the bluetooth bits (which, as far as I
> understood from your last mail, don't have strong objections against any
> more).

... Marcel?

I'd really like not to miss 2.6.37 merge window with this.

Thanks,

--
Jiri Kosina
SUSE Labs, Novell Inc.

2010-10-01 13:30:21

[permalink] [raw]

Subject: Re: [PATCH v4 1/2] HID: Add Support for Setting and Getting Feature Reports from hidraw

On Tue, 28 Sep 2010, Antonio Ospite wrote:

> Hi Alan, I am doing some stress testing on hidraw, if I have a loop with
> HIDIOCGFEATURE on a given report and I disconnect the device while the
> loop is running I get this:
>
> BUG: unable to handle kernel NULL pointer dereference at 0000000000000028
> IP: [<ffffffffa02c66b4>] hidraw_ioctl+0xfc/0x32c [hid]
>
> Full log attached along with the test program, the device is a Sony PS3
> Controller (sixaxis).
>
> If my objdump analysis is right, hidraw_ioctl+0xfc should be around line
> 361 in hidraw.c (with your patch applied):
>
> struct hid_device *hid = dev->hid;
>
> It looks like 'dev' (which is hidraw_table[minor]) can be NULL
> sometimes, can't it?
> This is not introduced by your changes tho.
>
> Just as a side note, the bug does not show up if the userspace program
> handles return values properly and exits as soon as it gets an error
> from the HID layer, see the XXX comment in test_hidraw_feature.c.
>
> This fixes it, if it looks ok I will resend the patch rebased on
> mainline code:
>
> diff --git a/drivers/hid/hidraw.c b/drivers/hid/hidraw.c
> index 7df1310..3c040c6 100644
> --- a/drivers/hid/hidraw.c
> +++ b/drivers/hid/hidraw.c
> @@ -322,6 +322,10 @@ static long hidraw_ioctl(struct file *file, unsigned int cmd,
>
> mutex_lock(&minors_lock);
> dev = hidraw_table[minor];
> + if (!dev) {
> + ret = -ENODEV;
> + goto out;
> + }
>
> switch (cmd) {
> case HIDIOCGRDESCSIZE:
> @@ -412,6 +416,7 @@ static long hidraw_ioctl(struct file *file, unsigned int cmd,
>
> ret = -ENOTTY;
> }
> +out:
> mutex_unlock(&minors_lock);
> return ret;
> }

Yes, this patch makes sense even for current mainline code. Could you
please resend it to me with Signed-off-by: and changelog text, so that I
could apply it?

Thanks!

--
Jiri Kosina
SUSE Labs, Novell Inc.

2010-11-08 11:17:15

[permalink] [raw]

Subject: Re: [PATCH v4 0/2] Get and Set Feature Reports on HIDRAW (USB and Bluetooth)

On Mon, 1 Nov 2010 15:23:34 -0400 (EDT)
Jiri Kosina <[email protected]> wrote:

> On Wed, 22 Sep 2010, Jiri Kosina wrote:
>
> > > > > This is version 4. Built against 2.6.35+ revision 320b2b8de12698 .
> > > > >
> > > > > Alan Ott (2):
> > > > > HID: Add Support for Setting and Getting Feature Reports from hidraw
> > > > > Bluetooth: hidp: Add support for hidraw HIDIOCGFEATURE and
> > > > > HIDIOCSFEATURE
[...]
> > > ... Marcel?
> > >
> > > I'd really like not to miss 2.6.37 merge window with this.
> >
> > Seemingly I have not enought powers to get statement from Marcel here
> > these days/weeks.
> >
> > Davem, would you perhaps be able to step in here?
>
> Marcel, any word on this patchset by chance?
>

Hopefully Alan will manage to send a v5 sometime soon, Alan I don't want
to pressure you, just remember to CC Gustavo F. Padovan (see
MAINTAINERS) as he looks to be the most active bluetooth maintainer
these days.

Thanks,
Antonio

--
Antonio Ospite
http://ao2.it

PGP public key ID: 0x4553B001

A: Because it messes up the order in which people normally read text.
See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

Attachments:

(No filename) (1.16 kB)
(No filename) (198.00 B)
Download all attachments

2010-11-01 19:23:34