hidraw: Wait for Ack when Sending to Device

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hello,

As many of you know, I'm working on getting support for Feature Reports into hidraw[1]. The USB side has been done and functional for quite some time, but the Bluetooth side has been met with some resistance.

A bit of background on the bluetooth side: Before I got here, hidraw on bluetooth only supported sending reports (not requesting them, as is done for a feature report). The report would be sent, but hidp_output_raw_report() would not wait for an Ack from the device to ensure that it got delivered correctly (or reported an error). It was kind of fire-and-forget and returned success every time.

To implement request of reports (needed for feature reports), a wait queue had to be added, and the receiving thread (hidp_session()) would wake up the wait queue when a response had been received[2].

In [2], I left the output function alone (so it only would send data but not wait for the ack), but there was an issue reported by Ville Tervo that in the case of a failed call to output_raw_report() followed by a successful call to get_feature_report(), the get_feature_report() call would receive the NAK from the failed output report and would return failure (even though the request of the feature report otherwise would have succeeded).

To fix this, I changed the output function to have a wait queue and wait for an ACK/NAK from the device before returning[3]. I have this working for normal devices.

Some HID devices however, (hid-sony.c for one), call hid_output_raw_report() (which on a bluetooth HID device will call hidp_output_raw_report()) from their respective probe() functions (sony_probe() in this case). The problem with this is that hidp_session() is not started until _after_ the call to a device's probe() function returns. This means that with my new code[3] outputting a report from a probe() function on a bluetooth HID device will _never_ return success.

In hidp_add_connection() the call to hidp_setup_hid() is what eventually calls the device's probe() function. As you can see from the snippet below, it's called before the call to kernel_thread() (which creates a thread for hidp_session().

       if (req->rd_size > 0) {
                err = hidp_setup_hid(session, req);
                if (err && err != -ENODEV)
                        goto purge;
        }

        if (!session->hid) {
                err = hidp_setup_input(session, req);
                if (err < 0)
                        goto purge;
        }

        __hidp_link_session(session);

        hidp_set_timer(session);

        err = kernel_thread(hidp_session, session, CLONE_KERNEL);
        if (err < 0)
                goto unlink;

I'd like to move the call to kernel_thread() above the call to setup_hid(). Of course the error handling would have to be shifted a bit as well. Does anyone here have a fundamental problem with this?

Alan.

[1] https://lkml.org/lkml/2010/8/16/340
[2] https://lkml.org/lkml/2010/8/16/343
[3] see the inline patch below


diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c

index 0e4880e..b55562a 100644

--- a/net/bluetooth/hidp/core.c

+++ b/net/bluetooth/hidp/core.c

@@ -397,6 +397,9 @@ err_eio:

 static int hidp_output_raw_report(struct hid_device *hid, unsigned char *data, size_t count,

         unsigned char report_type)

 {

+    struct hidp_session *session = hid->driver_data;

+    int ret;

+

     switch (report_type) {

     case HID_FEATURE_REPORT:

         report_type = HIDP_TRANS_SET_REPORT | HIDP_DATA_RTYPE_FEATURE;

@@ -408,10 +411,59 @@ static int hidp_output_raw_report(struct hid_device *hid, unsigned char *data, s

         return -EINVAL;

     }

+    if (mutex_lock_interruptible(&session->report_mutex))

+        return -ERESTARTSYS;

+

+    /* Set up our wait, and send the report request to the device. */

+    set_bit(HIDP_WAITING_FOR_SEND_ACK,&session->flags);

     if (hidp_send_ctrl_message(hid->driver_data, report_type,

-            data, count))

-        return -ENOMEM;

-    return count;

+            data, count)) {

+        ret = -ENOMEM;

+        goto err;

+    }

+

+    {

+        int i;

+        printk(KERN_WARNING "Sent %d message: %02hhx len: %d\n", report_type, data[0], (int)count);

+        for (i = 0; i<  count; i++) {

+            printk(KERN_WARNING " %02hhx", data[i]);

+        }

+        printk(KERN_WARNING "\n");

+    }

+

+    /* Wait for the ACK from the device. */

+    while (test_bit(HIDP_WAITING_FOR_SEND_ACK,&session->flags)) {

+        int res;

+

+        res = wait_event_interruptible_timeout(session->report_queue,

+            !test_bit(HIDP_WAITING_FOR_SEND_ACK,&session->flags),

+            10*HZ);

+        if (res == 0) {

+            /* timeout */

+            printk(KERN_WARNING "TIMEOUT\n");

+            ret = -EIO;

+            goto err;

+        }

+        if (res<  0) {

+            /* signal */

+            printk(KERN_WARNING "SIGNAL\n");

+            ret = -ERESTARTSYS;

+            goto err;

+        }

+    }

+

+    if (!session->output_report_success) {

+        printk(KERN_WARNING "NOT SUCCESS: Returning -EIO\n");

+        ret = -EIO;

+        goto err;

+    }

+

+    ret = count;

+

+err:

+    clear_bit(HIDP_WAITING_FOR_SEND_ACK,&session->flags);

+    mutex_unlock(&session->report_mutex);

+    return ret;

 }

 static void hidp_idle_timeout(unsigned long arg)

@@ -438,10 +490,14 @@ static void hidp_process_handshake(struct hidp_session *session,

                     unsigned char param)

 {

     BT_DBG("session %p param 0x%02x", session, param);

+    printk(KERN_WARNING "Handshake Packet %d\n", param);

+    session->output_report_success = 0; /* default condition */

     switch (param) {

     case HIDP_HSHK_SUCCESSFUL:

         /* FIXME: Call into SET_ GET_ handlers here */

+        printk(KERN_WARNING "   (Successful)\n");

+        session->output_report_success = 1;

         break;

     case HIDP_HSHK_NOT_READY:

@@ -452,6 +508,7 @@ static void hidp_process_handshake(struct hidp_session *session,

             clear_bit(HIDP_WAITING_FOR_RETURN,&session->flags);

             wake_up_interruptible(&session->report_queue);

         }

+        printk(KERN_WARNING "   (not-successful %d)\n", param);

         /* FIXME: Call into SET_ GET_ handlers here */

         break;

@@ -470,6 +527,12 @@ static void hidp_process_handshake(struct hidp_session *session,

             HIDP_TRANS_HANDSHAKE | HIDP_HSHK_ERR_INVALID_PARAMETER, NULL, 0);

         break;

     }

+

+    /* Wake up the waiting thread. */

+    if (test_bit(HIDP_WAITING_FOR_SEND_ACK,&session->flags)) {

+        clear_bit(HIDP_WAITING_FOR_SEND_ACK,&session->flags);

+        wake_up_interruptible(&session->report_queue);

+    }

 }

 static void hidp_process_hid_control(struct hidp_session *session,

@@ -494,6 +557,7 @@ static int hidp_process_data(struct hidp_session *session, struct sk_buff *skb,

 {

     int done_with_skb = 1;

     BT_DBG("session %p skb %p len %d param 0x%02x", session, skb, skb->len, param);

+    printk(KERN_WARNING "Got Data packet. param: %d\n", param);

     switch (param) {

     case HIDP_DATA_RTYPE_INPUT:

@@ -647,6 +711,7 @@ static int hidp_session(void *arg)

     wait_queue_t ctrl_wait, intr_wait;

     BT_DBG("session %p", session);

+    printk(KERN_WARNING "SESSION STARTING\n");

     if (session->input) {

         vendor  = session->input->id.vendor;

@@ -665,6 +730,7 @@ static int hidp_session(void *arg)

     init_waitqueue_entry(&intr_wait, current);

     add_wait_queue(sk_sleep(ctrl_sk),&ctrl_wait);

     add_wait_queue(sk_sleep(intr_sk),&intr_wait);

+    printk(KERN_WARNING "session entering main loop\n");

     while (!atomic_read(&session->terminate)) {

         set_current_state(TASK_INTERRUPTIBLE);

@@ -673,6 +739,7 @@ static int hidp_session(void *arg)

         while ((skb = skb_dequeue(&ctrl_sk->sk_receive_queue))) {

             skb_orphan(skb);

+            printk(KERN_WARNING "Got CTRL Frame\n");

             hidp_recv_ctrl_frame(session, skb);

         }

@@ -925,6 +992,7 @@ int hidp_add_connection(struct hidp_connadd_req *req, struct socket *ctrl_sock,

     int err;

     BT_DBG("");

+    printk(KERN_WARNING "HIDP ADD CONNECTION");

     if (bacmp(&bt_sk(ctrl_sock->sk)->src,&bt_sk(intr_sock->sk)->src) ||

             bacmp(&bt_sk(ctrl_sock->sk)->dst,&bt_sk(intr_sock->sk)->dst))

diff --git a/net/bluetooth/hidp/hidp.h b/net/bluetooth/hidp/hidp.h

index 00e71dd..2f16eea 100644

--- a/net/bluetooth/hidp/hidp.h

+++ b/net/bluetooth/hidp/hidp.h

@@ -81,6 +81,7 @@

 #define HIDP_BOOT_PROTOCOL_MODE        1

 #define HIDP_BLUETOOTH_VENDOR_ID    9

 #define    HIDP_WAITING_FOR_RETURN        10

+#define HIDP_WAITING_FOR_SEND_ACK    11

 struct hidp_connadd_req {

     int   ctrl_sock;    // Connected control socket

@@ -161,6 +162,9 @@ struct hidp_session {

     struct mutex report_mutex;

     struct sk_buff *report_return;

     wait_queue_head_t report_queue;

+

+    /* Used in hidp_output_raw_report() */

+    int output_report_success; /* boolean */

     /* Report descriptor */

     __u8 *rd_data;



--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Media Devel]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Linux Wireless Networking]     [Linux Omap]

  Powered by Linux