Message ID | 1276467601-9066-1-git-send-email-alan@signal11.us |
---|---|
State | Not Applicable, archived |
Delegated to: | David Miller |
Headers | show |
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 > >
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 >
From: Antonio Ospite <ospite@studenti.unina.it> Date: Mon, 28 Jun 2010 13:14:37 +0200 > On Sun, 13 Jun 2010 18:20:01 -0400 > Alan Ott <alan@signal11.us> wrote: > >> This patch adds support or getting and setting feature reports for bluetooth >> HID devices from HIDRAW. >> >> Signed-off-by: Alan Ott <alan@signal11.us> >> --- > > Ping. We effectively don't have a bluetooth maintainer at the current point in time. I've tried to let patches sit for a while hoping the listed maintainer would do something, at least occaisionally, but that simply isn't happening. So I'll just pick patches up directly as I find time to review them, but I have to warn that for me it's going to be done in a very low priority way because I really don't find bluetooth all that exciting. :-) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
Hi, On Tue, Jun 29, 2010, Jiri Kosina wrote: > Frankly, I don't understand what exactly the current situation with > in-kernel bluetooth stack is anyway. > > What is the relation between what we have in net/bluetooth and the tree at > [1], which seems to be quite actively developed? The difference is that the userspace part has essentially two maintainers (me and Marcel) whereas the kernel side only has one (Marcel). So even when Marcel is inactive I can at least take care of the userspace side. I'd volunteer for the kernel also but I don't really have any experience there (only 3-4 patches so far). Johan -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Tue, Jun 29, 2010 at 10:12 AM, David Miller <davem@davemloft.net> wrote: > From: Antonio Ospite <ospite@studenti.unina.it> > Date: Mon, 28 Jun 2010 13:14:37 +0200 > >> On Sun, 13 Jun 2010 18:20:01 -0400 >> Alan Ott <alan@signal11.us> wrote: >> >>> This patch adds support or getting and setting feature reports for bluetooth >>> HID devices from HIDRAW. >>> >>> Signed-off-by: Alan Ott <alan@signal11.us> >>> --- >> >> Ping. > > We effectively don't have a bluetooth maintainer at the current point > in time. I've tried to let patches sit for a while hoping the listed > maintainer would do something, at least occaisionally, but that simply > isn't happening. > So I'll just pick patches up directly as I find time to review them, > but I have to warn that for me it's going to be done in a very low > priority way because I really don't find bluetooth all that exciting. :-) This would be good. We have a backlog of bluetooth kernel patches waiting for several months. This takes too much time to return to them again and again... Please keep this ML informed. Regards, Andrei Emeltchenko -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Andrei, > >> On Sun, 13 Jun 2010 18:20:01 -0400 > >> Alan Ott <alan@signal11.us> wrote: > >> > >>> This patch adds support or getting and setting feature reports for bluetooth > >>> HID devices from HIDRAW. > >>> > >>> Signed-off-by: Alan Ott <alan@signal11.us> > >>> --- > >> > >> Ping. > > > > We effectively don't have a bluetooth maintainer at the current point > > in time. I've tried to let patches sit for a while hoping the listed > > maintainer would do something, at least occaisionally, but that simply > > isn't happening. > > So I'll just pick patches up directly as I find time to review them, > > but I have to warn that for me it's going to be done in a very low > > priority way because I really don't find bluetooth all that exciting. :-) > > This would be good. We have a backlog of bluetooth kernel patches > waiting for several months. > This takes too much time to return to them again and again... > > Please keep this ML informed. as soon as Ville is back from vacation, he should be helping out with patch review and maintenance. Regards Marcel -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Alan, > This patch adds support or getting and setting feature reports for bluetooth > HID devices from HIDRAW. > > Signed-off-by: Alan Ott <alan@signal11.us> > --- > net/bluetooth/hidp/core.c | 121 +++++++++++++++++++++++++++++++++++++++++++-- > net/bluetooth/hidp/hidp.h | 8 +++ > 2 files changed, 125 insertions(+), 4 deletions(-) > > diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c > index bfe641b..0f068a0 100644 > --- a/net/bluetooth/hidp/core.c > +++ b/net/bluetooth/hidp/core.c > @@ -36,6 +36,7 @@ > #include <linux/file.h> > #include <linux/init.h> > #include <linux/wait.h> > +#include <linux/mutex.h> > #include <net/sock.h> > > #include <linux/input.h> > @@ -313,6 +314,93 @@ static int hidp_send_report(struct hidp_session *session, struct hid_report *rep > return hidp_queue_report(session, buf, rsize); > } > > +static int hidp_get_raw_report(struct hid_device *hid, > + unsigned char report_number, > + unsigned char *data, size_t count, > + unsigned char report_type) > +{ > + struct hidp_session *session = hid->driver_data; > + struct sk_buff *skb; > + size_t len; > + int numbered_reports = hid->report_enum[report_type].numbered; > + > + switch (report_type) { > + case HID_FEATURE_REPORT: > + report_type = HIDP_TRANS_GET_REPORT | HIDP_DATA_RTYPE_FEATURE; > + break; > + case HID_INPUT_REPORT: > + report_type = HIDP_TRANS_GET_REPORT | HIDP_DATA_RTYPE_INPUT; > + break; > + case HID_OUTPUT_REPORT: > + report_type = HIDP_TRANS_GET_REPORT | HIDP_DATA_RTYPE_OUPUT; > + break; > + default: > + return -EINVAL; > + } > + > + if (mutex_lock_interruptible(&session->report_mutex)) > + return -ERESTARTSYS; > + > + /* Set up our wait, and send the report request to the device. */ > + session->waiting_report_type = report_type & HIDP_DATA_RTYPE_MASK; > + session->waiting_report_number = numbered_reports ? report_number : -1; > + set_bit(HIDP_WAITING_FOR_RETURN, &session->flags); > + data[0] = report_number; > + if (hidp_send_ctrl_message(hid->driver_data, report_type, data, 1)) > + goto err_eio; > + > + /* Wait for the return of the report. The returned report > + gets put in session->report_return. */ > + while (test_bit(HIDP_WAITING_FOR_RETURN, &session->flags)) { > + int res; > + > + res = wait_event_interruptible_timeout(session->report_queue, > + !test_bit(HIDP_WAITING_FOR_RETURN, &session->flags), > + 5*HZ); > + if (res == 0) { > + /* timeout */ > + goto err_eio; > + } > + if (res < 0) { > + /* signal */ > + goto err_restartsys; > + } > + } > + > + skb = session->report_return; > + if (skb) { > + if (numbered_reports) { > + /* Strip off the report number. */ > + size_t rpt_len = skb->len-1; > + len = rpt_len < count ? rpt_len : count; > + memcpy(data, skb->data+1, len); > + } else { > + len = skb->len < count ? skb->len : count; > + memcpy(data, skb->data, len); > + } > + > + kfree_skb(skb); > + session->report_return = NULL; > + } else { > + /* Device returned a HANDSHAKE, indicating protocol error. */ > + len = -EIO; > + } > + > + clear_bit(HIDP_WAITING_FOR_RETURN, &session->flags); > + mutex_unlock(&session->report_mutex); > + > + return len; > + > +err_restartsys: > + clear_bit(HIDP_WAITING_FOR_RETURN, &session->flags); > + mutex_unlock(&session->report_mutex); > + return -ERESTARTSYS; > +err_eio: > + clear_bit(HIDP_WAITING_FOR_RETURN, &session->flags); > + mutex_unlock(&session->report_mutex); > + return -EIO; > +} > + > static int hidp_output_raw_report(struct hid_device *hid, unsigned char *data, size_t count, > unsigned char report_type) > { > @@ -367,6 +455,10 @@ static void hidp_process_handshake(struct hidp_session *session, > case HIDP_HSHK_ERR_INVALID_REPORT_ID: > case HIDP_HSHK_ERR_UNSUPPORTED_REQUEST: > case HIDP_HSHK_ERR_INVALID_PARAMETER: > + if (test_bit(HIDP_WAITING_FOR_RETURN, &session->flags)) { > + clear_bit(HIDP_WAITING_FOR_RETURN, &session->flags); > + wake_up_interruptible(&session->report_queue); > + } > /* FIXME: Call into SET_ GET_ handlers here */ > break; > > @@ -403,9 +495,11 @@ static void hidp_process_hid_control(struct hidp_session *session, > } > } > > -static void hidp_process_data(struct hidp_session *session, struct sk_buff *skb, > +/* Returns true if the passed-in skb should be freed by the caller. */ > +static int hidp_process_data(struct hidp_session *session, struct sk_buff *skb, > unsigned char param) > { > + int done_with_skb = 1; > BT_DBG("session %p skb %p len %d param 0x%02x", session, skb, skb->len, param); > > switch (param) { > @@ -417,7 +511,6 @@ static void hidp_process_data(struct hidp_session *session, struct sk_buff *skb, > > if (session->hid) > hid_input_report(session->hid, HID_INPUT_REPORT, skb->data, skb->len, 0); > - > break; > > case HIDP_DATA_RTYPE_OTHER: > @@ -429,12 +522,27 @@ static void hidp_process_data(struct hidp_session *session, struct sk_buff *skb, > __hidp_send_ctrl_message(session, > HIDP_TRANS_HANDSHAKE | HIDP_HSHK_ERR_INVALID_PARAMETER, NULL, 0); > } > + > + if (test_bit(HIDP_WAITING_FOR_RETURN, &session->flags) && > + param == session->waiting_report_type) { > + if (session->waiting_report_number < 0 || > + session->waiting_report_number == skb->data[0]) { > + /* hidp_get_raw_report() is waiting on this report. */ > + session->report_return = skb; > + done_with_skb = 0; > + clear_bit(HIDP_WAITING_FOR_RETURN, &session->flags); > + wake_up_interruptible(&session->report_queue); > + } > + } > + > + return done_with_skb; > } > > static void hidp_recv_ctrl_frame(struct hidp_session *session, > struct sk_buff *skb) > { > unsigned char hdr, type, param; > + int free_skb = 1; > > BT_DBG("session %p skb %p len %d", session, skb, skb->len); > > @@ -454,7 +562,7 @@ static void hidp_recv_ctrl_frame(struct hidp_session *session, > break; > > case HIDP_TRANS_DATA: > - hidp_process_data(session, skb, param); > + free_skb = hidp_process_data(session, skb, param); > break; > > default: > @@ -463,7 +571,8 @@ static void hidp_recv_ctrl_frame(struct hidp_session *session, > break; > } > > - kfree_skb(skb); > + if (free_skb) > + kfree_skb(skb); > } > > static void hidp_recv_intr_frame(struct hidp_session *session, > @@ -797,6 +906,7 @@ static int hidp_setup_hid(struct hidp_session *session, > hid->dev.parent = hidp_get_device(session); > hid->ll_driver = &hidp_hid_driver; > > + hid->hid_get_raw_report = hidp_get_raw_report; > hid->hid_output_raw_report = hidp_output_raw_report; > > err = hid_add_device(hid); > @@ -857,6 +967,9 @@ int hidp_add_connection(struct hidp_connadd_req *req, struct socket *ctrl_sock, > skb_queue_head_init(&session->ctrl_transmit); > skb_queue_head_init(&session->intr_transmit); > > + mutex_init(&session->report_mutex); > + init_waitqueue_head(&session->report_queue); > + > session->flags = req->flags & (1 << HIDP_BLUETOOTH_VENDOR_ID); > session->idle_to = req->idle_to; > > diff --git a/net/bluetooth/hidp/hidp.h b/net/bluetooth/hidp/hidp.h > index 8d934a1..00e71dd 100644 > --- a/net/bluetooth/hidp/hidp.h > +++ b/net/bluetooth/hidp/hidp.h > @@ -80,6 +80,7 @@ > #define HIDP_VIRTUAL_CABLE_UNPLUG 0 > #define HIDP_BOOT_PROTOCOL_MODE 1 > #define HIDP_BLUETOOTH_VENDOR_ID 9 > +#define HIDP_WAITING_FOR_RETURN 10 > > struct hidp_connadd_req { > int ctrl_sock; // Connected control socket > @@ -154,6 +155,13 @@ struct hidp_session { > struct sk_buff_head ctrl_transmit; > struct sk_buff_head intr_transmit; > > + /* Used in hidp_get_raw_report() */ > + int waiting_report_type; /* HIDP_DATA_RTYPE_* */ > + int waiting_report_number; /* -1 for not numbered */ > + struct mutex report_mutex; > + struct sk_buff *report_return; > + wait_queue_head_t report_queue; > + > /* Report descriptor */ > __u8 *rd_data; > uint rd_size; I looked at this and I am bit worried that this should not be done in this detail in the HIDP driver. Essentially HIDP is a pure transport driver. It should not handle all these details. Can we make this a bit easier for the transport drivers to support such features? Regards Marcel -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/08/2010 05:11 PM, Marcel Holtmann wrote: > I looked at this and I am bit worried that this should not be done in > this detail in the HIDP driver. Essentially HIDP is a pure transport > driver. It should not handle all these details. Can we make this a bit > easier for the transport drivers to support such features? > > Regards > > Marcel > Hi Marcel, I put these changes (most notably the addition of hidp_get_raw_report()) in hidp because that's where the parallel function hidp_output_raw_report() was already located. I figured the input should go with the output. That said, if there's a better place for both of them (input and output) to go, let me know where you think it should be, and I'll get them moved into the proper spot. I'm not sure what you mean about HIDP being a pure transport driver. Alan. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Alan, > > I looked at this and I am bit worried that this should not be done in > > this detail in the HIDP driver. Essentially HIDP is a pure transport > > driver. It should not handle all these details. Can we make this a bit > > easier for the transport drivers to support such features? > > I put these changes (most notably the addition of hidp_get_raw_report()) > in hidp because that's where the parallel function > hidp_output_raw_report() was already located. I figured the input should > go with the output. That said, if there's a better place for both of > them (input and output) to go, let me know where you think it should be, > and I'll get them moved into the proper spot. > > I'm not sure what you mean about HIDP being a pure transport driver. what is usb-hid.ko doing here? I would expect a bunch of code duplication with minor difference between USB and Bluetooth. Regards Marcel -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/09/2010 04:01 AM, Marcel Holtmann wrote: > Hi Alan, > > >>> I looked at this and I am bit worried that this should not be done in >>> this detail in the HIDP driver. Essentially HIDP is a pure transport >>> driver. It should not handle all these details. Can we make this a bit >>> easier for the transport drivers to support such features? >>> >> I put these changes (most notably the addition of hidp_get_raw_report()) >> in hidp because that's where the parallel function >> hidp_output_raw_report() was already located. I figured the input should >> go with the output. That said, if there's a better place for both of >> them (input and output) to go, let me know where you think it should be, >> and I'll get them moved into the proper spot. >> >> I'm not sure what you mean about HIDP being a pure transport driver. >> > what is usb-hid.ko doing here? I would expect a bunch of code > duplication with minor difference between USB and Bluetooth. > > Regards > > Marcel > Hi Marcel, usbhid doesn't have a lot of code for hidraw. Two functions are involved: usbhid_output_raw_report() - calls usb_control_msg() with Get_Report usbhid_get_raw_report() - calls usb_control_msg() with Set_Report OR - calls usb_interrupt_msg() on the Ouput pipe. This is of course easier than bluetooth because usb_control_msg() is synchronous, even when requesting reports, mostly because of the nature of USB, where the request and response are part of the same transfer. For Bluetooth, it's a bit more complicated since the kernel treats it more like a networking interface (and indeed it is). My understanding is that to make a synchronous transfer in bluetooth, one must: - send the request packet - block (wait_event_*()) - when the response is received in the input handler, wake_up_*(). There's not really any code duplication, mostly because initiating synchronous USB transfers (input and output) is easy (because of the usb_*_msg() functions), while making synchronous Bluetooth transfers must be done manually. If there's a nice, convenient, synchronous function in Bluetooth similar to usb_control_msg() that I've missed, then let me know, as it would simplify this whole thing. See the Set/Get Feature patch, including USB support, here: http://lkml.org/lkml/2010/6/9/222 Alan. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Alan, > >>> I looked at this and I am bit worried that this should not be done in > >>> this detail in the HIDP driver. Essentially HIDP is a pure transport > >>> driver. It should not handle all these details. Can we make this a bit > >>> easier for the transport drivers to support such features? > >>> > >> I put these changes (most notably the addition of hidp_get_raw_report()) > >> in hidp because that's where the parallel function > >> hidp_output_raw_report() was already located. I figured the input should > >> go with the output. That said, if there's a better place for both of > >> them (input and output) to go, let me know where you think it should be, > >> and I'll get them moved into the proper spot. > >> > >> I'm not sure what you mean about HIDP being a pure transport driver. > >> > > what is usb-hid.ko doing here? I would expect a bunch of code > > duplication with minor difference between USB and Bluetooth. > > usbhid doesn't have a lot of code for hidraw. Two functions are involved: > usbhid_output_raw_report() > - calls usb_control_msg() with Get_Report > usbhid_get_raw_report() > - calls usb_control_msg() with Set_Report > OR > - calls usb_interrupt_msg() on the Ouput pipe. > > This is of course easier than bluetooth because usb_control_msg() is > synchronous, even when requesting reports, mostly because of the nature > of USB, where the request and response are part of the same transfer. > > For Bluetooth, it's a bit more complicated since the kernel treats it > more like a networking interface (and indeed it is). My understanding is > that to make a synchronous transfer in bluetooth, one must: > - send the request packet > - block (wait_event_*()) > - when the response is received in the input handler, wake_up_*(). > > There's not really any code duplication, mostly because initiating > synchronous USB transfers (input and output) is easy (because of the > usb_*_msg() functions), while making synchronous Bluetooth transfers > must be done manually. If there's a nice, convenient, synchronous > function in Bluetooth similar to usb_control_msg() that I've missed, > then let me know, as it would simplify this whole thing. there is not and I don't think we ever get one. My question here was more in the direction why HID core is doing these synchronously in the first place. Especially since USB can do everything async as well. Regards Marcel -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/09/2010 09:06 AM, Marcel Holtmann wrote: >>>>> I looked at this and I am bit worried that this should not be done in >>>>> this detail in the HIDP driver. Essentially HIDP is a pure transport >>>>> driver. It should not handle all these details. Can we make this a bit >>>>> easier for the transport drivers to support such features? >>>>> >>>>> >>>> I put these changes (most notably the addition of hidp_get_raw_report()) >>>> in hidp because that's where the parallel function >>>> hidp_output_raw_report() was already located. I figured the input should >>>> go with the output. That said, if there's a better place for both of >>>> them (input and output) to go, let me know where you think it should be, >>>> and I'll get them moved into the proper spot. >>>> >>>> I'm not sure what you mean about HIDP being a pure transport driver. >>>> >>>> >>> what is usb-hid.ko doing here? I would expect a bunch of code >>> duplication with minor difference between USB and Bluetooth. >>> >> usbhid doesn't have a lot of code for hidraw. Two functions are involved: >> usbhid_output_raw_report() >> - calls usb_control_msg() with Get_Report >> usbhid_get_raw_report() >> - calls usb_control_msg() with Set_Report >> OR >> - calls usb_interrupt_msg() on the Ouput pipe. >> >> This is of course easier than bluetooth because usb_control_msg() is >> synchronous, even when requesting reports, mostly because of the nature >> of USB, where the request and response are part of the same transfer. >> >> For Bluetooth, it's a bit more complicated since the kernel treats it >> more like a networking interface (and indeed it is). My understanding is >> that to make a synchronous transfer in bluetooth, one must: >> - send the request packet >> - block (wait_event_*()) >> - when the response is received in the input handler, wake_up_*(). >> >> There's not really any code duplication, mostly because initiating >> synchronous USB transfers (input and output) is easy (because of the >> usb_*_msg() functions), while making synchronous Bluetooth transfers >> must be done manually. If there's a nice, convenient, synchronous >> function in Bluetooth similar to usb_control_msg() that I've missed, >> then let me know, as it would simplify this whole thing. >> > there is not and I don't think we ever get one. My question here was > more in the direction why HID core is doing these synchronously in the > first place. Especially since USB can do everything async as well. > > Regards > > Marcel > Hi Marcel, I'm open to suggestions. The way I see it is from a user space perspective. With Get_Feature being on an ioctl(), I don't see any clean way to do it other than synchronously. Other operating systems (I can say for sure Windows, Mac OS X, and FreeBSD) handle Get/Set Feature the same way (synchronously) from user space. You seem to be proposing an asynchronous interface. What would that look like from user space? Alan. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Alan, > >>>>> I looked at this and I am bit worried that this should not be done in > >>>>> this detail in the HIDP driver. Essentially HIDP is a pure transport > >>>>> driver. It should not handle all these details. Can we make this a bit > >>>>> easier for the transport drivers to support such features? > >>>>> > >>>>> > >>>> I put these changes (most notably the addition of hidp_get_raw_report()) > >>>> in hidp because that's where the parallel function > >>>> hidp_output_raw_report() was already located. I figured the input should > >>>> go with the output. That said, if there's a better place for both of > >>>> them (input and output) to go, let me know where you think it should be, > >>>> and I'll get them moved into the proper spot. > >>>> > >>>> I'm not sure what you mean about HIDP being a pure transport driver. > >>>> > >>>> > >>> what is usb-hid.ko doing here? I would expect a bunch of code > >>> duplication with minor difference between USB and Bluetooth. > >>> > >> usbhid doesn't have a lot of code for hidraw. Two functions are involved: > >> usbhid_output_raw_report() > >> - calls usb_control_msg() with Get_Report > >> usbhid_get_raw_report() > >> - calls usb_control_msg() with Set_Report > >> OR > >> - calls usb_interrupt_msg() on the Ouput pipe. > >> > >> This is of course easier than bluetooth because usb_control_msg() is > >> synchronous, even when requesting reports, mostly because of the nature > >> of USB, where the request and response are part of the same transfer. > >> > >> For Bluetooth, it's a bit more complicated since the kernel treats it > >> more like a networking interface (and indeed it is). My understanding is > >> that to make a synchronous transfer in bluetooth, one must: > >> - send the request packet > >> - block (wait_event_*()) > >> - when the response is received in the input handler, wake_up_*(). > >> > >> There's not really any code duplication, mostly because initiating > >> synchronous USB transfers (input and output) is easy (because of the > >> usb_*_msg() functions), while making synchronous Bluetooth transfers > >> must be done manually. If there's a nice, convenient, synchronous > >> function in Bluetooth similar to usb_control_msg() that I've missed, > >> then let me know, as it would simplify this whole thing. > >> > > there is not and I don't think we ever get one. My question here was > > more in the direction why HID core is doing these synchronously in the > > first place. Especially since USB can do everything async as well. > > I'm open to suggestions. The way I see it is from a user space > perspective. With Get_Feature being on an ioctl(), I don't see any clean > way to do it other than synchronously. Other operating systems (I can > say for sure Windows, Mac OS X, and FreeBSD) handle Get/Set Feature the > same way (synchronously) from user space. > > You seem to be proposing an asynchronous interface. What would that look > like from user space? not necessarily from user space, but at least from HID core to HIDP and usb-hid transports. At least that is what I would expect, Jiri? Regards Marcel -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/09/2010 01:33 PM, Marcel Holtmann wrote: >>>>>>> I looked at this and I am bit worried that this should not be done in >>>>>>> this detail in the HIDP driver. Essentially HIDP is a pure transport >>>>>>> driver. It should not handle all these details. Can we make this a bit >>>>>>> easier for the transport drivers to support such features? >>>>>>> >>>>>>> >>>>>>> >>>>>> I put these changes (most notably the addition of hidp_get_raw_report()) >>>>>> in hidp because that's where the parallel function >>>>>> hidp_output_raw_report() was already located. I figured the input should >>>>>> go with the output. That said, if there's a better place for both of >>>>>> them (input and output) to go, let me know where you think it should be, >>>>>> and I'll get them moved into the proper spot. >>>>>> >>>>>> I'm not sure what you mean about HIDP being a pure transport driver. >>>>>> >>>>>> >>>>>> >>>>> what is usb-hid.ko doing here? I would expect a bunch of code >>>>> duplication with minor difference between USB and Bluetooth. >>>>> >>>>> >>>> usbhid doesn't have a lot of code for hidraw. Two functions are involved: >>>> usbhid_output_raw_report() >>>> - calls usb_control_msg() with Get_Report >>>> usbhid_get_raw_report() >>>> - calls usb_control_msg() with Set_Report >>>> OR >>>> - calls usb_interrupt_msg() on the Ouput pipe. >>>> >>>> This is of course easier than bluetooth because usb_control_msg() is >>>> synchronous, even when requesting reports, mostly because of the nature >>>> of USB, where the request and response are part of the same transfer. >>>> >>>> For Bluetooth, it's a bit more complicated since the kernel treats it >>>> more like a networking interface (and indeed it is). My understanding is >>>> that to make a synchronous transfer in bluetooth, one must: >>>> - send the request packet >>>> - block (wait_event_*()) >>>> - when the response is received in the input handler, wake_up_*(). >>>> >>>> There's not really any code duplication, mostly because initiating >>>> synchronous USB transfers (input and output) is easy (because of the >>>> usb_*_msg() functions), while making synchronous Bluetooth transfers >>>> must be done manually. If there's a nice, convenient, synchronous >>>> function in Bluetooth similar to usb_control_msg() that I've missed, >>>> then let me know, as it would simplify this whole thing. >>>> >>>> >>> there is not and I don't think we ever get one. My question here was >>> more in the direction why HID core is doing these synchronously in the >>> first place. Especially since USB can do everything async as well. >>> >> I'm open to suggestions. The way I see it is from a user space >> perspective. With Get_Feature being on an ioctl(), I don't see any clean >> way to do it other than synchronously. Other operating systems (I can >> say for sure Windows, Mac OS X, and FreeBSD) handle Get/Set Feature the >> same way (synchronously) from user space. >> >> You seem to be proposing an asynchronous interface. What would that look >> like from user space? >> > not necessarily from user space, but at least from HID core to HIDP and > usb-hid transports. At least that is what I would expect, Jiri? > > Regards > > Marcel > Hi Marcel, So it sounds like you're mostly concerned about the sleeping (blocking), and where is the _right_ place for it to occur. It seems like it could either occur in hid/hidraw.c or in hidp/core.c. If it were to occur in hid/hidraw.c, what then would get passed back and forth between the bluetooth/hidp and hidraw? Maybe something like the following: hidraw: - get_report() (hypothetical) - calls a hypothetical hidp_initiate_get_report(), which: - sends the report request and returns immediately. - wait for response hidp: - whenever a report is returned, it calls back to hidraw, which wakes up the get_report() thread if the data matches the report being waited on. For this to work, we'd need 2 more function pointers in struct hid_device: 1. a way for hidp to call back into hidraw. 2. a pointer for hidp_initiate_get_report(). These of course would be in addition to the ones that USB already uses (like hid_get_raw_report()), and would cause USB and Bluetooth to use different APIs to each transport. Of course, there could be commonality if we used the asynchronous USB APIs like you suggested, although, I'm not sure I see the benefit of making the USB part more complicated. The USB part (hid/usb/hid-core.c) is currently _very_ simple. It seems like we have two options: 1. Move to asynchronous APIs in USB and Bluetooth. This involves: a. Move to asynchronous APIs in hid/usbhid/hid-core.c b. Adding support into hid/hidraw.c to do the waiting. c. Changing bluetooth/hidp to be asynchronous in nature. 2. Keep using synchronous USB APIs. a. hid/usbhid/hid-core remains really simple b. hid/hidraw.c remains really simple c. bluetooth/hidp has some complexity I'd argue that the complexity of bluetooth/hidp isn't really that complex, and further, it's mostly isolated to one (new) function (that's where the wait_event_*() is). Further, if we did option #2, some piece of code has to determine whether to wake up the blocking thread (which would then be in hid/hidraw.c). This piece of code would be notified for every packet received from Bluetooth to decide whether it should wake up the sleeping thread, and would have to have bluetooth-specific code in it (something like the block which calls wake_up_interruptible() in my patch). It seems like this code would _have_ to be in hidp. From a design standpoint, I can't see how it makes sense to push this code into hid/hidraw.c when it is bluetooth-specific. Further, I can't see how it makes sense to do the USB portion the hard way, when the current implementation is so compact and non-error-prone. Clients to hidraw provide two functions with very simple interfaces, one for outputting reports, and one for getting (requesting and receiving) reports. I think having clean interfaces between modules has a lot of value. All that said, I'm always open to better ideas. Maybe you have a better design idea that you can enlighten me with. Alan. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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,
Hi Jiri, > > > >>> what is usb-hid.ko doing here? I would expect a bunch of code > > > >>> duplication with minor difference between USB and Bluetooth. > > > >>> > > > >> usbhid doesn't have a lot of code for hidraw. Two functions are involved: > > > >> usbhid_output_raw_report() > > > >> - calls usb_control_msg() with Get_Report > > > >> usbhid_get_raw_report() > > > >> - calls usb_control_msg() with Set_Report > > > >> OR > > > >> - calls usb_interrupt_msg() on the Ouput pipe. > > > >> > > > >> This is of course easier than bluetooth because usb_control_msg() is > > > >> synchronous, even when requesting reports, mostly because of the nature > > > >> of USB, where the request and response are part of the same transfer. > > > >> > > > >> For Bluetooth, it's a bit more complicated since the kernel treats it > > > >> more like a networking interface (and indeed it is). My understanding is > > > >> that to make a synchronous transfer in bluetooth, one must: > > > >> - send the request packet > > > >> - block (wait_event_*()) > > > >> - when the response is received in the input handler, wake_up_*(). > > > >> > > > >> There's not really any code duplication, mostly because initiating > > > >> synchronous USB transfers (input and output) is easy (because of the > > > >> usb_*_msg() functions), while making synchronous Bluetooth transfers > > > >> must be done manually. If there's a nice, convenient, synchronous > > > >> function in Bluetooth similar to usb_control_msg() that I've missed, > > > >> then let me know, as it would simplify this whole thing. > > > >> > > > > there is not and I don't think we ever get one. My question here was > > > > more in the direction why HID core is doing these synchronously in the > > > > first place. Especially since USB can do everything async as well. > > > > > > I'm open to suggestions. The way I see it is from a user space > > > perspective. With Get_Feature being on an ioctl(), I don't see any clean > > > way to do it other than synchronously. Other operating systems (I can > > > say for sure Windows, Mac OS X, and FreeBSD) handle Get/Set Feature the > > > same way (synchronously) from user space. > > > > > > You seem to be proposing an asynchronous interface. What would that look > > > like from user space? > > > > not necessarily from user space, but at least from HID core to HIDP and > > usb-hid transports. At least that is what I would expect, Jiri? > > Sorry for this taking too long (vacations, conferences, you name it) for > me to respond. > > As all the _raw() callbacks are purely intended for userspace interaction > anyway, it's perfectly fine (and in fact desirable) for the low-level > transport drivers to perform these operations synchronously (and that's > what USB implementation does as well). > > Marcel, if your opposition to synchronous interface is strong, we'll have > to think about other aproaches, but from my POV, the patch is fine as-is > for Bluetooth. that the ioctl() API is synchronous is fine to me, however pushing that down to the transport drivers seems wrong to me. I think the HID core should be able to handle a fully asynchronous transport driver. I know that with the USB subsystem you are little bit spoiled here, but for Bluetooth it is not the case. And in the end even using the asynchronous USB URB calls would be nice as well. So why not make the core actually wait for responses from the transport driver. I would make the transport drivers a lot simpler in the long run. And I know that most likely besides Bluetooth and USB you won't see another, but you never know. Regards Marcel -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/22/2010 11:21 AM, Marcel Holtmann wrote: >>>>>>> what is usb-hid.ko doing here? I would expect a bunch of code >>>>>>> duplication with minor difference between USB and Bluetooth. >>>>>>> >>>>>>> >>>>>> usbhid doesn't have a lot of code for hidraw. Two functions are involved: >>>>>> usbhid_output_raw_report() >>>>>> - calls usb_control_msg() with Get_Report >>>>>> usbhid_get_raw_report() >>>>>> - calls usb_control_msg() with Set_Report >>>>>> OR >>>>>> - calls usb_interrupt_msg() on the Ouput pipe. >>>>>> >>>>>> This is of course easier than bluetooth because usb_control_msg() is >>>>>> synchronous, even when requesting reports, mostly because of the nature >>>>>> of USB, where the request and response are part of the same transfer. >>>>>> >>>>>> For Bluetooth, it's a bit more complicated since the kernel treats it >>>>>> more like a networking interface (and indeed it is). My understanding is >>>>>> that to make a synchronous transfer in bluetooth, one must: >>>>>> - send the request packet >>>>>> - block (wait_event_*()) >>>>>> - when the response is received in the input handler, wake_up_*(). >>>>>> >>>>>> There's not really any code duplication, mostly because initiating >>>>>> synchronous USB transfers (input and output) is easy (because of the >>>>>> usb_*_msg() functions), while making synchronous Bluetooth transfers >>>>>> must be done manually. If there's a nice, convenient, synchronous >>>>>> function in Bluetooth similar to usb_control_msg() that I've missed, >>>>>> then let me know, as it would simplify this whole thing. >>>>>> >>>>>> >>>>> there is not and I don't think we ever get one. My question here was >>>>> more in the direction why HID core is doing these synchronously in the >>>>> first place. Especially since USB can do everything async as well. >>>>> >>>> I'm open to suggestions. The way I see it is from a user space >>>> perspective. With Get_Feature being on an ioctl(), I don't see any clean >>>> way to do it other than synchronously. Other operating systems (I can >>>> say for sure Windows, Mac OS X, and FreeBSD) handle Get/Set Feature the >>>> same way (synchronously) from user space. >>>> >>>> You seem to be proposing an asynchronous interface. What would that look >>>> like from user space? >>>> >>> not necessarily from user space, but at least from HID core to HIDP and >>> usb-hid transports. At least that is what I would expect, Jiri? >>> >> Sorry for this taking too long (vacations, conferences, you name it) for >> me to respond. >> >> As all the _raw() callbacks are purely intended for userspace interaction >> anyway, it's perfectly fine (and in fact desirable) for the low-level >> transport drivers to perform these operations synchronously (and that's >> what USB implementation does as well). >> >> Marcel, if your opposition to synchronous interface is strong, we'll have >> to think about other aproaches, but from my POV, the patch is fine as-is >> for Bluetooth. >> > that the ioctl() API is synchronous is fine to me, however pushing that > down to the transport drivers seems wrong to me. I think the HID core > should be able to handle a fully asynchronous transport driver. I know > that with the USB subsystem you are little bit spoiled here, but for > Bluetooth it is not the case. And in the end even using the asynchronous > USB URB calls would be nice as well. > How are the URB calls better than using the synchronous calls? (see below) > So why not make the core actually wait for responses from the transport > driver. Because this makes the USB side a lot more complicated, and it would introduce transport specific code into the core. Further, it would involve the transport driver calling hidraw with _every_ single packet it receives. Further, it would have to call hidraw with HANDSHAKE protocol error packets as well. > I would make the transport drivers a lot simpler in the long > run. It would make the USB transport driver and drivers/hid/hidraw much more complicated right now, at the expense of making the BT transport driver marginally simpler (and I'm not even convinced whether it would really be simpler). (see below for more) > And I know that most likely besides Bluetooth and USB you won't see > another, but you never know. > > I just don't understand the objection. In each transport type, the waiting will have to be done in a different way. USB and BT are different enough that this is the case already, without having to imagine future buses which use HID. In BT, you have to check each packet which comes back from the BT network to see whether it is the response packet that hidraw is waiting for. Further, you have to check for HANDSHAKE packets indicating protocol error. Where better for this to be done than in hidp? Further, how can this possibly happen in drivers/hid/hidraw, as it doesn't know about the details of bluetooth to make this determination, and why should it? In my last email ( http://lkml.org/lkml/2010/7/9/231 ) to which I got no response, I laid out how doing the blocking in drivers/hid/hidraw would only make all the parts except bluetooth more complicated (including the core, and the USB side), and would also introduce bluetooth-specific things into the core. Further, you're saying that using the asynchronous USB URB calls would be a benefit. How is it a benefit to replace a single function call which does exactly what I want, with a set of asynchronous calls and then adding my own blocking to make it do the same thing? This sounds to me like it would be 1: more text, 2: duplication of code, 3: error prone. I can't understand how this is of benefit. Please explain to me what I'm missing. In theory, what you're saying makes sense. Making common code and logic actually common is always good. In practice though, in this case, I submit that there really isn't any commonality, and the only way for there to be commonality is to do the USB side the hard way. Further, drivers/hid/hidraw can't wait for a bluetooth packet without having code that's bluetooth-specific. It seems just that simple to me. I'll give it some more thought, and take another look at the code to see if there's something obvious that I'm missing. If you know what I'm missing in my understanding of the problem, please tell me :) Alan. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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,
Hi Jiri, > > > that the ioctl() API is synchronous is fine to me, however pushing that > > > down to the transport drivers seems wrong to me. I think the HID core > > > should be able to handle a fully asynchronous transport driver. I know > > > that with the USB subsystem you are little bit spoiled here, but for > > > Bluetooth it is not the case. And in the end even using the asynchronous > > > USB URB calls would be nice as well. > > > > > > > How are the URB calls better than using the synchronous calls? (see below) > > Hi, > > I think this was the last mail in the thread so far, right? > > > > So why not make the core actually wait for responses from the transport > > > driver. > > > > Because this makes the USB side a lot more complicated, and it would > > introduce transport specific code into the core. Further, it would > > involve the transport driver calling hidraw with _every_ single packet > > it receives. Further, it would have to call hidraw with HANDSHAKE > > protocol error packets as well. > > > > > I would make the transport drivers a lot simpler in the long > > > run. > > > > It would make the USB transport driver and drivers/hid/hidraw much more > > complicated right now, at the expense of making the BT transport driver > > marginally simpler (and I'm not even convinced whether it would really be > > simpler). (see below for more) > > Seconded. > > > > And I know that most likely besides Bluetooth and USB you won't see > > > another, but you never know. > > > > > I just don't understand the objection. In each transport type, the waiting > > will have to be done in a different way. USB and BT are different enough that > > this is the case already, without having to imagine future buses which use > > HID. In BT, you have to check each packet which comes back from the BT network > > to see whether it is the response packet that hidraw is waiting for. Further, > > you have to check for HANDSHAKE packets indicating protocol error. Where > > better for this to be done than in hidp? Further, how can this possibly happen > > in drivers/hid/hidraw, as it doesn't know about the details of bluetooth to > > make this determination, and why should it? In my last email ( > > http://lkml.org/lkml/2010/7/9/231 ) to which I got no response, I laid out how > > doing the blocking in drivers/hid/hidraw would only make all the parts except > > bluetooth more complicated (including the core, and the USB side), and would > > also introduce bluetooth-specific things into the core. > > > > Further, you're saying that using the asynchronous USB URB calls would be a > > benefit. How is it a benefit to replace a single function call which does > > exactly what I want, with a set of asynchronous calls and then adding my own > > blocking to make it do the same thing? This sounds to me like it would be 1: > > more text, 2: duplication of code, 3: error prone. I can't understand how this > > is of benefit. Please explain to me what I'm missing. > > > > In theory, what you're saying makes sense. Making common code and logic > > actually common is always good. In practice though, in this case, I submit > > that there really isn't any commonality, and the only way for there to be > > commonality is to do the USB side the hard way. Further, drivers/hid/hidraw > > can't wait for a bluetooth packet without having code that's > > bluetooth-specific. It seems just that simple to me. > > > > I'll give it some more thought, and take another look at the code to see if > > there's something obvious that I'm missing. If you know what I'm missing in my > > understanding of the problem, please tell me :) > > Marcel, did you have time to review Alan's explanation a little bit? > > I must say I would really like to have this feature merged, but of course > not if you completely disagree .. but then we'll have to find some > consensus. Currently Alan's summary above quite aligns with my opinion. my opinion is still that we should make the core do the async handling. I think that we let USB dictate how APIs for HID should look like. However that is maybe fine anyway since the Bluetooth HID guys where not really inventive since they copying USB HID for the better and mostly for the worst. Especially since Bluetooth doesn't have the endpoint direction limits like USB does. Anyhow, just get the patches re-based and re-submitted and I can have a second look. Regards Marcel -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
This is version 4. Built against 2.6.35+ revision 320b2b8de12698 . Alan Ott (2): HID: Add Support for Setting and Getting Feature Reports from hidraw Bluetooth: hidp: Add support for hidraw HIDIOCGFEATURE and HIDIOCSFEATURE drivers/hid/hidraw.c | 105 ++++++++++++++++++++++++++++++++++++-- drivers/hid/usbhid/hid-core.c | 37 +++++++++++++- include/linux/hid.h | 3 + include/linux/hidraw.h | 3 + net/bluetooth/hidp/core.c | 114 +++++++++++++++++++++++++++++++++++++++-- net/bluetooth/hidp/hidp.h | 8 +++ 6 files changed, 260 insertions(+), 10 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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,
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,
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,
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
On Thu, Sep 23, 2010 at 9:25 AM, Ville Tervo <ville.tervo@nokia.com> wrote: > Hi Alan, > > On Mon, Aug 16, 2010 at 10:20:57PM +0200, ext Alan Ott wrote: >> This is version 4. Built against 2.6.35+ revision 320b2b8de12698 . > > I gave a try to to this patch using your test tool [1] and very old BT > keyboard. I don't have anything else ATM to test with. Is there some BT hid > devices which support setting and getting features? As far as I know Wacom BT devices (Graphire and Intuos4) need to get and set features. Przemo, Do you have time to test the patchset with your Graphire BT and provide your result here? Thank you, Ping -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Dnia 2010-09-23, czw o godzinie 10:07 -0700, Ping Cheng pisze: > On Thu, Sep 23, 2010 at 9:25 AM, Ville Tervo <ville.tervo@nokia.com> wrote: > > Hi Alan, > > > > On Mon, Aug 16, 2010 at 10:20:57PM +0200, ext Alan Ott wrote: > >> This is version 4. Built against 2.6.35+ revision 320b2b8de12698 . > > > > I gave a try to to this patch using your test tool [1] and very old BT > > keyboard. I don't have anything else ATM to test with. Is there some BT hid > > devices which support setting and getting features? > > As far as I know Wacom BT devices (Graphire and Intuos4) need to get > and set features. > > Przemo, > > Do you have time to test the patchset with your Graphire BT and > provide your result here? Hi Ping, Speed switching works as expected. Tested on HP NC4200 with internal and external bluetooth modules: Linux pldmachine 2.6.35-07788-g320b2b8-dirty #15 Thu Sep 23 19:55:03 IST 2010 i686 Intel(R)_Pentium(R)_M_processor_1.86GHz PLD Linux ID 03f0:011d Hewlett-Packard Integrated Bluetooth Module ID 0a12:0001 Cambridge Silicon Radio, Ltd Bluetooth Dongle (HCI mode) and Wacom Pen Tablet CTE-630BT Tested-by: Przemo Firszt <przemo@firszt.eu>
On 09/23/2010 12:25 PM, Ville Tervo wrote: > Hi Alan, > > On Mon, Aug 16, 2010 at 10:20:57PM +0200, ext Alan Ott wrote: > >> This is version 4. Built against 2.6.35+ revision 320b2b8de12698 . >> > > I gave a try to to this patch using your test tool [1] and very old BT > keyboard. I don't have anything else ATM to test with. Is there some BT hid > devices which support setting and getting features? > A keyboard is the only BT device I've used which has feature reports. > Shouldn't HIDIOCSFEATURE's bt version have similar wait as HIDIOCGFEATURE to > get report status back from the device? or is there even any status coming back > in successful case? Sorry I'm a newbie with HID and trying to understand how > this should work. > > Now it just returns num of send bytes even if the remote device returned some > error. Which one is the expected behavior? > I made it function the same as the existing hidp_output_raw_report(). Nothing in net/bluetooth/hidp/core.c is acked at all. I'm not sure of the reasons. Jiri, Marcel, any ideas? > Other problem is that Get report is getting now handshake from set report. > > > Yeah, that makes sense. I hadn't considered the set_report failing. In that case, I guess it means we _must_ wait for an ack for sent packets. Anyone else agree or disagree? Alan. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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,
On Mon, 1 Nov 2010 15:23:34 -0400 (EDT) Jiri Kosina <jkosina@suse.cz> wrote: > On Wed, 22 Sep 2010, Jiri Kosina wrote: > > > > > > This is version 4. Built against 2.6.35+ revision 320b2b8de12698 . > > > > > > > > > > Alan Ott (2): > > > > > HID: Add Support for Setting and Getting Feature Reports from hidraw > > > > > Bluetooth: hidp: Add support for hidraw HIDIOCGFEATURE and > > > > > HIDIOCSFEATURE [...] > > > ... Marcel? > > > > > > I'd really like not to miss 2.6.37 merge window with this. > > > > Seemingly I have not enought powers to get statement from Marcel here > > these days/weeks. > > > > Davem, would you perhaps be able to step in here? > > Marcel, any word on this patchset by chance? > Hopefully Alan will manage to send a v5 sometime soon, Alan I don't want to pressure you, just remember to CC Gustavo F. Padovan (see MAINTAINERS) as he looks to be the most active bluetooth maintainer these days. Thanks, Antonio
diff --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;
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(-)