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

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

[Jiri Kosina]

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
> 
> 

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

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

[Antonio Ospite]

[-- Attachment #1: Type: text/plain, Size: 8758 bytes --]

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
> 


-- 
Antonio Ospite
http://ao2.it

PGP public key ID: 0x4553B001

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

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

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

[David Miller]

From: Antonio Ospite <ospite-aNJ+ML1ZbiP93QAQaVx+gl6hYfS7NtTn@public.gmane.org>
Date: Mon, 28 Jun 2010 13:14:37 +0200

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

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

[Jiri Kosina]

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

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

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

[Johan Hedberg]

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

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

[Andrei Emeltchenko]

Hi,

On Tue, Jun 29, 2010 at 10:12 AM, David Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org> wrote:
> From: Antonio Ospite <ospite-aNJ+ML1ZbiP93QAQaVx+gl6hYfS7NtTn@public.gmane.org>
> Date: Mon, 28 Jun 2010 13:14:37 +0200
>
>> On Sun, 13 Jun 2010 18:20:01 -0400
>> Alan Ott <alan-yzvJWuRpmD1zbRFIqnYvSA@public.gmane.org> wrote:
>>
>>> This patch adds support or getting and setting feature reports for bluetooth
>>> HID devices from HIDRAW.
>>>
>>> Signed-off-by: Alan Ott <alan-yzvJWuRpmD1zbRFIqnYvSA@public.gmane.org>
>>> ---
>>
>> 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

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

[Marcel Holtmann]

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

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

[Marcel Holtmann]

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

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

[Alan Ott]

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.

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

[Marcel Holtmann]

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

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

[Alan Ott]

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.

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

[Marcel Holtmann]

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

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

[Alan Ott]

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.

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

[Marcel Holtmann]

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

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

[Alan Ott]

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.

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

[Jiri Kosina]

On Fri, 9 Jul 2010, Marcel Holtmann wrote:

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

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

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

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

Thanks,

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

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

[Marcel Holtmann]

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

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

[Alan Ott]

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.

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

[Jiri Kosina]

On Thu, 22 Jul 2010, Alan Ott wrote:

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

Hi,

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

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

Seconded.

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

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

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

Thanks,

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

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

[Marcel Holtmann]

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

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

[Alan Ott]

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(-)

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

[Alan Ott]

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

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

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

diff --git a/drivers/hid/hidraw.c b/drivers/hid/hidraw.c
index 47d70c5..7df1310 100644
--- a/drivers/hid/hidraw.c
+++ b/drivers/hid/hidraw.c
@@ -101,14 +101,14 @@ out:
 }
 
 /* the first byte is expected to be a report number */
-static ssize_t hidraw_write(struct file *file, const char __user *buffer, size_t count, loff_t *ppos)
+/* This function is to be called with the minors_lock mutex held */
+static ssize_t hidraw_send_report(struct file *file, const char __user *buffer, size_t count, unsigned char report_type)
 {
 	unsigned int minor = iminor(file->f_path.dentry->d_inode);
 	struct hid_device *dev;
 	__u8 *buf;
 	int ret = 0;
 
-	mutex_lock(&minors_lock);
 	dev = hidraw_table[minor]->hid;
 
 	if (!dev->hid_output_raw_report) {
@@ -141,14 +141,92 @@ static ssize_t hidraw_write(struct file *file, const char __user *buffer, size_t
 		goto out_free;
 	}
 
-	ret = dev->hid_output_raw_report(dev, buf, count, HID_OUTPUT_REPORT);
+	ret = dev->hid_output_raw_report(dev, buf, count, report_type);
 out_free:
 	kfree(buf);
 out:
+	return ret;
+}
+
+/* the first byte is expected to be a report number */
+static ssize_t hidraw_write(struct file *file, const char __user *buffer, size_t count, loff_t *ppos)
+{
+	ssize_t ret;
+	mutex_lock(&minors_lock);
+	ret = hidraw_send_report(file, buffer, count, HID_OUTPUT_REPORT);
 	mutex_unlock(&minors_lock);
 	return ret;
 }
 
+
+/* This function performs a Get_Report transfer over the control endpoint
+   per section 7.2.1 of the HID specification, version 1.1.  The first byte
+   of buffer is the report number to request, or 0x0 if the defice does not
+   use numbered reports. The report_type parameter can be HID_FEATURE_REPORT
+   or HID_INPUT_REPORT.  This function is to be called with the minors_lock
+   mutex held.  */
+static ssize_t hidraw_get_report(struct file *file, char __user *buffer, size_t count, unsigned char report_type)
+{
+	unsigned int minor = iminor(file->f_path.dentry->d_inode);
+	struct hid_device *dev;
+	__u8 *buf;
+	int ret = 0, len;
+	unsigned char report_number;
+
+	dev = hidraw_table[minor]->hid;
+
+	if (!dev->hid_get_raw_report) {
+		ret = -ENODEV;
+		goto out;
+	}
+
+	if (count > HID_MAX_BUFFER_SIZE) {
+		printk(KERN_WARNING "hidraw: pid %d passed too large report\n",
+				task_pid_nr(current));
+		ret = -EINVAL;
+		goto out;
+	}
+
+	if (count < 2) {
+		printk(KERN_WARNING "hidraw: pid %d passed too short report\n",
+				task_pid_nr(current));
+		ret = -EINVAL;
+		goto out;
+	}
+
+	buf = kmalloc(count * sizeof(__u8), GFP_KERNEL);
+	if (!buf) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	/* Read the first byte from the user. This is the report number,
+	   which is passed to dev->hid_get_raw_report(). */
+	if (copy_from_user(&report_number, buffer, 1)) {
+		ret = -EFAULT;
+		goto out_free;
+	}
+
+	ret = dev->hid_get_raw_report(dev, report_number, buf, count, report_type);
+
+	if (ret < 0)
+		goto out_free;
+
+	len = (ret < count) ? ret : count;
+
+	if (copy_to_user(buffer, buf, len)) {
+		ret = -EFAULT;
+		goto out_free;
+	}
+
+	ret = len;
+
+out_free:
+	kfree(buf);
+out:
+	return ret;
+}
+
 static unsigned int hidraw_poll(struct file *file, poll_table *wait)
 {
 	struct hidraw_list *list = file->private_data;
@@ -281,7 +359,24 @@ static long hidraw_ioctl(struct file *file, unsigned int cmd,
 		default:
 			{
 				struct hid_device *hid = dev->hid;
-				if (_IOC_TYPE(cmd) != 'H' || _IOC_DIR(cmd) != _IOC_READ) {
+				if (_IOC_TYPE(cmd) != 'H') {
+					ret = -EINVAL;
+					break;
+				}
+
+				if (_IOC_NR(cmd) == _IOC_NR(HIDIOCSFEATURE(0))) {
+					int len = _IOC_SIZE(cmd);
+					ret = hidraw_send_report(file, user_arg, len, HID_FEATURE_REPORT);
+					break;
+				}
+				if (_IOC_NR(cmd) == _IOC_NR(HIDIOCGFEATURE(0))) {
+					int len = _IOC_SIZE(cmd);
+					ret = hidraw_get_report(file, user_arg, len, HID_FEATURE_REPORT);
+					break;
+				}
+
+				/* Begin Read-only ioctls. */
+				if (_IOC_DIR(cmd) != _IOC_READ) {
 					ret = -EINVAL;
 					break;
 				}
@@ -313,7 +408,7 @@ static long hidraw_ioctl(struct file *file, unsigned int cmd,
 						-EFAULT : len;
 					break;
 				}
-		}
+			}
 
 		ret = -ENOTTY;
 	}
diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index b729c02..93db724 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -798,6 +798,40 @@ static int hid_alloc_buffers(struct usb_device *dev, struct hid_device *hid)
 	return 0;
 }
 
+static int usbhid_get_raw_report(struct hid_device *hid,
+		unsigned char report_number, __u8 *buf, size_t count,
+		unsigned char report_type)
+{
+	struct usbhid_device *usbhid = hid->driver_data;
+	struct usb_device *dev = hid_to_usb_dev(hid);
+	struct usb_interface *intf = usbhid->intf;
+	struct usb_host_interface *interface = intf->cur_altsetting;
+	int skipped_report_id = 0;
+	int ret;
+
+	/* Byte 0 is the report number. Report data starts at byte 1.*/
+	buf[0] = report_number;
+	if (report_number == 0x0) {
+		/* Offset the return buffer by 1, so that the report ID
+		   will remain in byte 0. */
+		buf++;
+		count--;
+		skipped_report_id = 1;
+	}
+	ret = usb_control_msg(dev, usb_rcvctrlpipe(dev, 0),
+		HID_REQ_GET_REPORT,
+		USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
+		((report_type + 1) << 8) | report_number,
+		interface->desc.bInterfaceNumber, buf, count,
+		USB_CTRL_SET_TIMEOUT);
+
+	/* count also the report id */
+	if (ret > 0 && skipped_report_id)
+		ret++;
+
+	return ret;
+}
+
 static int usbhid_output_raw_report(struct hid_device *hid, __u8 *buf, size_t count,
 		unsigned char report_type)
 {
@@ -807,7 +841,7 @@ static int usbhid_output_raw_report(struct hid_device *hid, __u8 *buf, size_t co
 	struct usb_host_interface *interface = intf->cur_altsetting;
 	int ret;
 
-	if (usbhid->urbout) {
+	if (usbhid->urbout && report_type != HID_FEATURE_REPORT) {
 		int actual_length;
 		int skipped_report_id = 0;
 		if (buf[0] == 0x0) {
@@ -1149,6 +1183,7 @@ static int usbhid_probe(struct usb_interface *intf, const struct usb_device_id *
 
 	usb_set_intfdata(intf, hid);
 	hid->ll_driver = &usb_hid_driver;
+	hid->hid_get_raw_report = usbhid_get_raw_report;
 	hid->hid_output_raw_report = usbhid_output_raw_report;
 	hid->ff_init = hid_pidff_init;
 #ifdef CONFIG_USB_HIDDEV
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 42a0f1d..1b95b0b 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -503,6 +503,9 @@ struct hid_device {							/* device report descriptor */
 				  struct hid_usage *, __s32);
 	void (*hiddev_report_event) (struct hid_device *, struct hid_report *);
 
+	/* handler for raw input (Get_Report) data, used by hidraw */
+	int (*hid_get_raw_report) (struct hid_device *, unsigned char, __u8 *, size_t, unsigned char);
+
 	/* handler for raw output data, used by hidraw */
 	int (*hid_output_raw_report) (struct hid_device *, __u8 *, size_t, unsigned char);
 
diff --git a/include/linux/hidraw.h b/include/linux/hidraw.h
index dd8d692..4b88e69 100644
--- a/include/linux/hidraw.h
+++ b/include/linux/hidraw.h
@@ -35,6 +35,9 @@ struct hidraw_devinfo {
 #define HIDIOCGRAWINFO		_IOR('H', 0x03, struct hidraw_devinfo)
 #define HIDIOCGRAWNAME(len)     _IOC(_IOC_READ, 'H', 0x04, len)
 #define HIDIOCGRAWPHYS(len)     _IOC(_IOC_READ, 'H', 0x05, len)
+/* The first byte of SFEATURE and GFEATURE is the report number */
+#define HIDIOCSFEATURE(len)    _IOC(_IOC_WRITE|_IOC_READ, 'H', 0x06, len)
+#define HIDIOCGFEATURE(len)    _IOC(_IOC_WRITE|_IOC_READ, 'H', 0x07, len)
 
 #define HIDRAW_FIRST_MINOR 0
 #define HIDRAW_MAX_DEVICES 64
-- 
1.7.0.4

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

[Antonio Ospite]

[-- Attachment #1.1: Type: text/plain, Size: 2624 bytes --]

On Mon, 16 Aug 2010 16:20:58 -0400
Alan Ott <alan@signal11.us> wrote:

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

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

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

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

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

struct hid_device *hid = dev->hid;

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

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

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

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

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

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

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


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

Regards,
   Antonio

-- 
Antonio Ospite
http://ao2.it

PGP public key ID: 0x4553B001

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

[-- Attachment #1.2: dmesg_hidraw_feature_bug.log --]
[-- Type: application/octet-stream, Size: 3914 bytes --]

[  111.645836] usb 4-1: USB disconnect, address 2
[  111.669466] BUG: unable to handle kernel NULL pointer dereference at 0000000000000028
[  111.669484] IP: [<ffffffffa02c66b4>] hidraw_ioctl+0xfc/0x32c [hid]
[  111.669529] PGD 5a953067 PUD 6d741067 PMD 0 
[  111.669539] Oops: 0000 [#1] SMP 
[  111.669545] last sysfs file: /sys/devices/pci0000:00/0000:00:04.0/usb4/idVendor
[  111.669556] CPU 0 
[  111.669559] Modules linked in: hidp powernow_k8 mperf cpufreq_powersave cpufreq_conservative cpufreq_stats cpufreq_userspace lirc_serial(C) lirc_dev ipt_MASQUERADE bridge stp ppdev lp sco bnep rfcomm l2cap crc16 tun sit tunnel4 kvm_amd kvm binfmt_misc uinput fuse nfsd ip6table_raw ip6table_mangle ip6t_REJECT exportfs nfs ip6t_LOG lockd fscache nf_conntrack_ipv6 nfs_acl auth_rpcgss ip6table_filter sunrpc ip6_tables xt_tcpudp ipt_REJECT ipt_ULOG xt_limit xt_state xt_multiport iptable_filter iptable_nat nf_nat nf_conntrack_ipv4 nf_conntrack nf_defrag_ipv4 iptable_mangle iptable_raw ip_tables x_tables hwmon_vid loop snd_hda_codec_nvhdmi snd_hda_codec_via snd_hda_intel snd_hda_codec snd_hwdep snd_pcm_oss snd_mixer_oss snd_pcm snd_seq_midi snd_rawmidi nvidia(P) snd_seq_midi_event snd_seq parport_pc btusb snd_timer snd_seq_device asus_atk0110 joydev video snd bluetooth rfkill hid_sony output wmi parport edac_core i2c_nforce2 tpm_tis shpchp pci_hotplug k8temp edac_mce_amd pcspkr tpm tpm_bios evdev soundcore snd_page_alloc i2c_core button processor ext3 jbd mbcache dm_mod sg sr_mod sd_mod crc_t10dif cdrom ata_generic usbhid hid ahci libahci pata_amd ohci_hcd libata ehci_hcd scsi_mod usbcore thermal forcedeth nls_base thermal_sys floppy [last unloaded: scsi_wait_scan]
[  111.669736] 
[  111.669746] Pid: 2969, comm: test_hidraw_fea Tainted: P         C  2.6.36-rc5-ao2+ #1 M3N78-VM/System Product Name
[  111.669753] RIP: 0010:[<ffffffffa02c66b4>]  [<ffffffffa02c66b4>] hidraw_ioctl+0xfc/0x32c [hid]
[  111.669771] RSP: 0018:ffff880056831e78  EFLAGS: 00010206
[  111.669776] RAX: 0000000000000048 RBX: 00000000c02d4807 RCX: 00000000022c7e50
[  111.669783] RDX: 0000000100000000 RSI: ffffffff810377e0 RDI: ffffffffa02ceaa0
[  111.669789] RBP: 00000000022c7e50 R08: 00007f983f1cbec8 R09: 0000000000400aca
[  111.669795] R10: ffffffff8100769f R11: ffff880037a78dd8 R12: ffff88005a85c300
[  111.669801] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
[  111.669808] FS:  00007f983f3cc700(0000) GS:ffff880001a00000(0000) knlGS:0000000000000000
[  111.669815] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  111.669821] CR2: 0000000000000028 CR3: 000000005aba9000 CR4: 00000000000006f0
[  111.669827] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[  111.669833] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
[  111.669840] Process test_hidraw_fea (pid: 2969, threadinfo ffff880056830000, task ffff88005ab93680)
[  111.669844] Stack:
[  111.669848]  00000000010aad14 ffff88005ab936b8 ffff88005a85c300 ffff880037fc6700
[  111.669857] <0> 00000000022c7e50 00000000022c7e50 0000000000000000 ffffffff810f64a5
[  111.669865] <0> ffff880001a14880 ffff88005ab93680 ffffffff81302776 0000000000000000
[  111.669875] Call Trace:
[  111.669898]  [<ffffffff810f64a5>] ? do_vfs_ioctl+0x4a2/0x4ef
[  111.669915]  [<ffffffff81302776>] ? schedule+0x573/0x5b7
[  111.669923]  [<ffffffff810f653d>] ? sys_ioctl+0x4b/0x72
[  111.669933]  [<ffffffff810ea002>] ? sys_write+0x5f/0x6b
[  111.669947]  [<ffffffff81008a02>] ? system_call_fastpath+0x16/0x1b
[  111.669952] Code: 2c 66 89 44 24 06 e8 1a c2 03 e1 48 89 e6 ba 08 00 00 00 48 89 ef e8 cc 79 ec e0 85 c0 0f 84 18 02 00 00 e9 01 02 00 00 0f b6 c7 <49> 8b 7d 28 83 f8 48 0f 85 fa 01 00 00 0f b6 c3 83 f8 06 75 24 
[  111.670011] RIP  [<ffffffffa02c66b4>] hidraw_ioctl+0xfc/0x32c [hid]
[  111.670025]  RSP <ffff880056831e78>
[  111.670029] CR2: 0000000000000028
[  111.670036] ---[ end trace 9c7530cfc7009202 ]---

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.3: test_hidraw_feature.c --]
[-- Type: text/x-csrc; name="test_hidraw_feature.c", Size: 1265 bytes --]

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <stdint.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <unistd.h>
#include <sys/ioctl.h>

/*
#include <linux/hidraw.h>
*/
#include "/home/ao2/Proj/linux/linux-2.6/include/linux/hidraw.h"

void dump_hex_string(unsigned char *buf, unsigned int len)
{
	unsigned int i;

	for (i = 0; i < len; i++)
		printf("%02x%c", buf[i], i < len - 1 ? ' ' : 0);
}

void dump_feature_report(int fd, uint8_t report_number, unsigned int len)
{
	unsigned char *buf;
	int ret;

	buf = calloc(len, sizeof(*buf));

	buf[0] = report_number;

	ret = ioctl(fd, HIDIOCGFEATURE(len), buf);
	if (ret < 0) {
		fprintf(stderr, "report: 0x%02x ret: %d\n", report_number, ret);
		/* XXX: if I put exit(1) here the bug is masked */
		return;
	}

	dump_hex_string(buf, len);
	printf("\r");
	fflush(stdout);

	free(buf);
}

int main(int argc, char *argv[])
{
	int fd = -1;

	if (argc != 2) {
		fprintf(stderr, "usage: %s </dev/hidrawX>\n", argv[0]);
		exit(1);
	}

	fd = open(argv[1], O_RDWR);
	if (fd < 0) {
		perror("hidraw open");
		exit(1);
	}

	while (1)
		dump_feature_report(fd, 0x01, 45);
	printf("\n");

	close(fd);
	exit(0);
}

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

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

[Jiri Kosina]

On Tue, 28 Sep 2010, Antonio Ospite wrote:

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

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

Thanks!

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

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

[Alan Ott]

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 |  114 +++++++++++++++++++++++++++++++++++++++++++--
 net/bluetooth/hidp/hidp.h |    8 +++
 2 files changed, 118 insertions(+), 4 deletions(-)

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

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

[Jiri Kosina]

On Mon, 16 Aug 2010, Alan Ott wrote:

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

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

Thanks,

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

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

[Jiri Kosina]

On Mon, 23 Aug 2010, Jiri Kosina wrote:

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

... Marcel?

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

Thanks,

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

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

[Jiri Kosina]

On Thu, 2 Sep 2010, Jiri Kosina wrote:

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

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

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

Thanks,

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

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

[Jiri Kosina]

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,

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

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

[Antonio Ospite]

[-- Attachment #1: Type: text/plain, Size: 1251 bytes --]

On Mon, 1 Nov 2010 15:23:34 -0400 (EDT)
Jiri Kosina <jkosina-AlSwsSmVLrQ@public.gmane.org> wrote:

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

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

Thanks,
   Antonio

-- 
Antonio Ospite
http://ao2.it

PGP public key ID: 0x4553B001

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

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

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

[Ville Tervo]

Hi Alan,

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


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

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

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

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


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



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

-- 
Ville the HID newbie

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

[Ping Cheng]

On Thu, Sep 23, 2010 at 9:25 AM, Ville Tervo <ville.tervo-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org> 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

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

[Przemo Firszt]

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-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org> 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-q9SP4D9nreWHXe+LvDLADg@public.gmane.org>
-- 
Regards,
Przemo Firszt <przemo-q9SP4D9nreWHXe+LvDLADg@public.gmane.org>

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

[Alan Ott]

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 linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Ville Tervo]

Hi Alan,

One comment.

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

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

-- 
Ville

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

[Alan Ott]

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

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

Hi Ville,

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

Alan.

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

[Antonio Ospite]

[-- Attachment #1: Type: text/plain, Size: 1055 bytes --]

On Thu, 23 Sep 2010 10:16:43 -0400
Alan Ott <alan@signal11.us> wrote:

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

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

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

Regards,
   Antonio

-- 
Antonio Ospite
http://ao2.it

PGP public key ID: 0x4553B001

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

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

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

[Alan Ott]

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;
-- 
1.7.0.4