diff mbox

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

Message ID 1276467601-9066-1-git-send-email-alan@signal11.us
State Not Applicable, archived
Delegated to: David Miller
Headers show

Commit Message

Alan Ott June 13, 2010, 10:20 p.m. UTC 
This patch adds support or getting and setting feature reports for bluetooth
HID devices from HIDRAW.

Signed-off-by: Alan Ott <alan@signal11.us>
---
 net/bluetooth/hidp/core.c |  121 +++++++++++++++++++++++++++++++++++++++++++--
 net/bluetooth/hidp/hidp.h |    8 +++
 2 files changed, 125 insertions(+), 4 deletions(-)

Comments

Jiri Kosina June 19, 2010, 5:49 p.m. UTC | #1 
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 <alan@signal11.us>

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
> 
>
Antonio Ospite June 28, 2010, 11:14 a.m. UTC | #2 
On Sun, 13 Jun 2010 18:20:01 -0400
Alan Ott <alan@signal11.us> wrote:

> This patch adds support or getting and setting feature reports for bluetooth
> HID devices from HIDRAW.
> 
> Signed-off-by: Alan Ott <alan@signal11.us>
> ---

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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
David Miller June 29, 2010, 7:12 a.m. UTC | #3 
From: Antonio Ospite <ospite@studenti.unina.it>
Date: Mon, 28 Jun 2010 13:14:37 +0200

> On Sun, 13 Jun 2010 18:20:01 -0400
> Alan Ott <alan@signal11.us> wrote:
> 
>> This patch adds support or getting and setting feature reports for bluetooth
>> HID devices from HIDRAW.
>> 
>> Signed-off-by: Alan Ott <alan@signal11.us>
>> ---
> 
> 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. :-)

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jiri Kosina June 29, 2010, 8:50 a.m. UTC | #4 
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 <alan@signal11.us>
> >> ---
> > 
> > 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
Johan Hedberg June 29, 2010, 9:07 a.m. UTC | #5 
Hi,

On Tue, Jun 29, 2010, Jiri Kosina wrote:
> 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?

The difference is that the userspace part has essentially two
maintainers (me and Marcel) whereas the kernel side only has one
(Marcel). So even when Marcel is inactive I can at least take care of
the userspace side. I'd volunteer for the kernel also but I don't really
have any experience there (only 3-4 patches so far).

Johan
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andrei Emeltchenko June 29, 2010, 12:40 p.m. UTC | #6 
Hi,

On Tue, Jun 29, 2010 at 10:12 AM, David Miller <davem@davemloft.net> wrote:
> From: Antonio Ospite <ospite@studenti.unina.it>
> Date: Mon, 28 Jun 2010 13:14:37 +0200
>
>> On Sun, 13 Jun 2010 18:20:01 -0400
>> Alan Ott <alan@signal11.us> wrote:
>>
>>> This patch adds support or getting and setting feature reports for bluetooth
>>> HID devices from HIDRAW.
>>>
>>> Signed-off-by: Alan Ott <alan@signal11.us>
>>> ---
>>
>> 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.

Regards,
Andrei Emeltchenko
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Marcel Holtmann July 8, 2010, 9:08 p.m. UTC | #7 
Hi Andrei,

> >> On Sun, 13 Jun 2010 18:20:01 -0400
> >> Alan Ott <alan@signal11.us> wrote:
> >>
> >>> This patch adds support or getting and setting feature reports for bluetooth
> >>> HID devices from HIDRAW.
> >>>
> >>> Signed-off-by: Alan Ott <alan@signal11.us>
> >>> ---
> >>
> >> 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


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Marcel Holtmann July 8, 2010, 9:11 p.m. UTC | #8 
Hi Alan,

> This patch adds support or getting and setting feature reports for bluetooth
> HID devices from HIDRAW.
> 
> Signed-off-by: Alan Ott <alan@signal11.us>
> ---
>  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


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alan Ott July 9, 2010, 3:51 a.m. UTC | #9 
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.


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Marcel Holtmann July 9, 2010, 8:01 a.m. UTC | #10 
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


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alan Ott July 9, 2010, 1:02 p.m. UTC | #11 
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.



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Marcel Holtmann July 9, 2010, 1:06 p.m. UTC | #12 
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


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alan Ott July 9, 2010, 2:06 p.m. UTC | #13 
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.







--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Marcel Holtmann July 9, 2010, 5:33 p.m. UTC | #14 
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


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alan Ott July 9, 2010, 6:24 p.m. UTC | #15 
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.



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jiri Kosina July 22, 2010, 2:14 p.m. UTC | #16 
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,
Marcel Holtmann July 22, 2010, 3:21 p.m. UTC | #17 
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


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alan Ott July 22, 2010, 4:58 p.m. UTC | #18 
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.



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jiri Kosina Aug. 10, 2010, 11:46 a.m. UTC | #19 
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,
Marcel Holtmann Aug. 10, 2010, 12:12 p.m. UTC | #20 
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


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alan Ott Aug. 16, 2010, 8:20 p.m. UTC | #21 
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(-)


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jiri Kosina Aug. 23, 2010, 1 p.m. UTC | #22 
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 Sept. 2, 2010, 3:25 p.m. UTC | #23 
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 Sept. 22, 2010, 12:09 p.m. UTC | #24 
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,
Ville Tervo Sept. 23, 2010, 4:25 p.m. UTC | #25 
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
Ping Cheng Sept. 23, 2010, 5:07 p.m. UTC | #26 
On Thu, Sep 23, 2010 at 9:25 AM, Ville Tervo <ville.tervo@nokia.com> 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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Przemo Firszt Sept. 23, 2010, 8:16 p.m. UTC | #27 
Dnia 2010-09-23, czw o godzinie 10:07 -0700, Ping Cheng pisze:
> On Thu, Sep 23, 2010 at 9:25 AM, Ville Tervo <ville.tervo@nokia.com> 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 <przemo@firszt.eu>
Alan Ott Sept. 23, 2010, 11:40 p.m. UTC | #28 
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.


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jiri Kosina Nov. 1, 2010, 7:23 p.m. UTC | #29 
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
> > > > 
> > > >  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?

Marcel, any word on this patchset by chance?

Thanks,
Antonio Ospite Nov. 8, 2010, 11:17 a.m. UTC | #30 
On Mon, 1 Nov 2010 15:23:34 -0400 (EDT)
Jiri Kosina <jkosina@suse.cz> 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
diff mbox

Patch

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;