Source-Changes-D archive

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

Re: CVS commit: src/sys/dev/bluetooth



On Mon, 2 Jan 2012, Radoslaw Kujawa wrote:

> On 2 sty 2012, at 11:00, Iain Hibbert wrote:
>
> > On Sun, 1 Jan 2012, Radoslaw Kujawa wrote:
>
> >>> btkbd_set_leds() may be called from wskbd directly (by pressing caps lock
> >>> on your built-in keyboard for instance)
> >>
> >> I've tested this patch by pressing the caps lock key on bt keyboard and
> >> issuing wsconsctl ledstate=1, and both methods work.
> >
> > Do you have a built in keyboard?  wsconsctl uses the ioctl, and bt
> > keyboard calls the function with the bt_lock held, but if you press the
> > caps-lock on the built in keyboard then wscons calls the set_leds routine
> > directly, I think? (thus, no lock will be acquired, when sending the
> > output report, which is not obviously going to break immediately, but..)
>
> I've connected the USB keyboard again to work on this problem ;). I'll
> search for PS/2 keyboard... Pressing the caps lock on USB keyboard does
> not light the LED on bluetooth keyboard. However, using wsconsctl -w
> ledstate=1 does light the LED on both keyboards. AFAIK it should use the
> same method to light the led, no matter if it was from userland app or
> by pressing the key on other keyboard?

I don't have a USB keyboard, but pressing the caps lock on my laptop or
Bluetooth keyboard does always toggle both LEDs. Using wsconsctl does
change both too, but thats not quite the same since it does not alter the
caps-lock status

> > it really should disassociate from the context, maybe doing
> > the call from a callout or separate task instead.
>
> Wouldn't that again lead to the situation where mtx_owner assertion will
> fail? If we call mutex_exit() from other thread than the one which
> acquired the lock, it will certainly fail. I understand that callout or
> separate task will be executed in another thread.

Should not, I have reworked it to offset the processing of reports to a
thread (patch attached) which does not trigger any mutex failures with
LOCKDEBUG. I'm still testing but I think its correct.. any comments?

> > bthidev0: report ID 17, len = 1 ignored
> >
> > messages you got.. that is not normal (likely not relevant to the locking
> > issue though), how did you trigger them, is it just caps lock?
>
> No, that's the FN key.

Ok thats separate issue, does the output of 'btdevctl -a <keyboard> -d
<device> -s hid' show anything for ID 17?  Unfortunately, there is no way
to handle things like that in a generic way from within the current HID
framework if the device does not report its capabilities in the descriptor
(Apple devices at least seem to be prone to this) so we need a special
driver (eg btmagic which handles several such reports) - Linux has a
concept where a driver can attach and register for other reports that
might be sent, but I'm not sure I liked their method, surely something
better can be designed..

iain
Index: bthidev.c
===================================================================
RCS file: /cvsroot/src/sys/dev/bluetooth/bthidev.c,v
retrieving revision 1.20
diff -u -p -r1.20 bthidev.c
--- bthidev.c   31 Dec 2011 01:16:09 -0000      1.20
+++ bthidev.c   7 Jan 2012 08:14:36 -0000
@@ -35,13 +35,16 @@
 __KERNEL_RCSID(0, "$NetBSD: bthidev.c,v 1.20 2011/12/31 01:16:09 rkujawa Exp 
$");
 
 #include <sys/param.h>
+#include <sys/condvar.h>
 #include <sys/conf.h>
 #include <sys/device.h>
 #include <sys/fcntl.h>
 #include <sys/kernel.h>
+#include <sys/kthread.h>
 #include <sys/queue.h>
 #include <sys/malloc.h>
 #include <sys/mbuf.h>
+#include <sys/mutex.h>
 #include <sys/proc.h>
 #include <sys/socketvar.h>
 #include <sys/systm.h>
@@ -83,6 +86,12 @@ struct bthidev_softc {
        struct l2cap_channel    *sc_int;        /* interrupt channel */
        struct l2cap_channel    *sc_int_l;      /* interrupt listen */
 
+       MBUFQ_HEAD()            sc_inq;         /* input queue */
+       kmutex_t                sc_lock;        /* input queue lock */
+       kcondvar_t              sc_cv;          /* input queue trigger */
+       lwp_t                   *sc_lwp;        /* input queue processor */
+       int                     sc_detach;
+
        LIST_HEAD(,bthidev)     sc_list;        /* child list */
 
        callout_t               sc_reconnect;
@@ -107,6 +116,8 @@ static int  bthidev_listen(struct bthide
 static int  bthidev_connect(struct bthidev_softc *);
 static int  bthidev_output(struct bthidev *, uint8_t *, int);
 static void bthidev_null(struct bthidev *, uint8_t *, int);
+static void bthidev_process(void *);
+static void bthidev_process_one(struct bthidev_softc *, struct mbuf *);
 
 /* autoconf(9) glue */
 static int  bthidev_match(device_t, cfdata_t, void *);
@@ -188,6 +199,7 @@ bthidev_attach(device_t parent, device_t
         */
        sc->sc_dev = self;
        LIST_INIT(&sc->sc_list);
+       MBUFQ_INIT(&sc->sc_inq);
        callout_init(&sc->sc_reconnect, 0);
        callout_setfunc(&sc->sc_reconnect, bthidev_timeout, sc);
        sc->sc_state = BTHID_CLOSED;
@@ -196,6 +208,8 @@ bthidev_attach(device_t parent, device_t
        sc->sc_intpsm = L2CAP_PSM_HID_INTR;
 
        sockopt_init(&sc->sc_mode, BTPROTO_L2CAP, SO_L2CAP_LM, 0);
+       mutex_init(&sc->sc_lock, MUTEX_DEFAULT, IPL_NONE);
+       cv_init(&sc->sc_cv, device_xname(self));
 
        /*
         * extract config from proplist
@@ -280,6 +294,12 @@ bthidev_attach(device_t parent, device_t
 
        aprint_normal("\n");
 
+       if (kthread_create(PRI_NONE, 0, NULL, bthidev_process, sc,
+           &sc->sc_lwp, "%s", device_xname(self)) != 0) {
+               aprint_error_dev(self, "failed to create input thread\n");
+               return;
+       }
+
        for (rep = 0 ; rep <= maxid ; rep++) {
                if (hid_report_size(desc, dlen, hid_feature, rep) == 0
                    && hid_report_size(desc, dlen, hid_input, rep) == 0
@@ -362,12 +382,25 @@ bthidev_detach(device_t self, int flags)
 
        mutex_exit(bt_lock);
 
+       /* kill off the input processor */
+       if (sc->sc_lwp != NULL) {
+               mutex_enter(&sc->sc_lock);
+               sc->sc_detach = 1;
+               cv_signal(&sc->sc_cv);
+               mutex_exit(&sc->sc_lock);
+               kthread_join(sc->sc_lwp);
+               sc->sc_lwp = NULL;
+       }
+
        /* detach children */
        while ((hidev = LIST_FIRST(&sc->sc_list)) != NULL) {
                LIST_REMOVE(hidev, sc_next);
                config_detach(hidev->sc_dev, flags);
        }
 
+       MBUFQ_DRAIN(&sc->sc_inq);
+       cv_destroy(&sc->sc_cv);
+       mutex_destroy(&sc->sc_lock);
        sockopt_destroy(&sc->sc_mode);
 
        return 0;
@@ -545,6 +578,123 @@ bthidev_connect(struct bthidev_softc *sc
        return 0;
 }
 
+/*
+ * The LWP which processes input reports, forwarding to child devices.
+ * We are always either processing input reports, holding the lock, or
+ * waiting for a signal on condvar.
+ */
+static void
+bthidev_process(void *arg)
+{
+       struct bthidev_softc *sc = arg;
+       struct mbuf *m;
+
+       mutex_enter(&sc->sc_lock);
+       while (sc->sc_detach == 0) {
+               MBUFQ_DEQUEUE(&sc->sc_inq, m);
+               if (m == NULL) {
+                       cv_wait(&sc->sc_cv, &sc->sc_lock);
+                       continue;
+               }
+
+               mutex_exit(&sc->sc_lock);
+               bthidev_process_one(sc, m);
+               m_freem(m);
+               mutex_enter(&sc->sc_lock);
+       }
+       mutex_exit(&sc->sc_lock);
+       kthread_exit(0);
+}
+
+static void
+bthidev_process_one(struct bthidev_softc *sc, struct mbuf *m)
+{
+       struct bthidev *hidev;
+       uint8_t *data;
+       int len;
+
+       if (sc->sc_state != BTHID_OPEN)
+               return;
+
+       if (m->m_pkthdr.len > m->m_len)
+               aprint_error_dev(sc->sc_dev, "truncating HID report\n");
+
+       len = m->m_len;
+       data = mtod(m, uint8_t *);
+
+       switch (BTHID_TYPE(data[0])) {
+       case BTHID_DATA:
+               /*
+                * data[0] == type / parameter
+                * data[1] == id
+                * data[2..len] == report
+                */
+               if (len < 3)
+                       break;
+
+               LIST_FOREACH(hidev, &sc->sc_list, sc_next)
+                       if (data[1] == hidev->sc_id)
+                               break;
+
+               if (hidev == NULL) {
+                       aprint_error_dev(sc->sc_dev,
+                           "report id %d, len = %d ignored\n", data[1], len - 
2);
+
+                       break;
+               }
+
+               switch (BTHID_DATA_PARAM(data[0])) {
+               case BTHID_DATA_INPUT:
+                       (*hidev->sc_input)(hidev, data + 2, len - 2);
+                       break;
+
+               case BTHID_DATA_FEATURE:
+                       (*hidev->sc_feature)(hidev, data + 2, len - 2);
+                       break;
+
+               default:
+                       break;
+               }
+
+               break;
+
+       case BTHID_CONTROL:
+               if (len < 1)
+                       break;
+
+               switch (BTHID_DATA_PARAM(data[0])) {
+               case BTHID_CONTROL_UNPLUG:
+                       aprint_normal_dev(sc->sc_dev, "unplugged\n");
+
+                       mutex_enter(bt_lock);
+                       /* close interrupt channel */
+                       if (sc->sc_int != NULL) {
+                               l2cap_disconnect(sc->sc_int, 0);
+                               l2cap_detach(&sc->sc_int);
+                               sc->sc_int = NULL;
+                       }
+
+                       /* close control channel */
+                       if (sc->sc_ctl != NULL) {
+                               l2cap_disconnect(sc->sc_ctl, 0);
+                               l2cap_detach(&sc->sc_ctl);
+                               sc->sc_ctl = NULL;
+                       }
+                       mutex_exit(bt_lock);
+
+                       break;
+
+               default:
+                       break;
+               }
+
+               break;
+
+       default:
+               break;
+       }
+}
+
 /*****************************************************************************
  *
  *     bluetooth(9) callback methods for L2CAP
@@ -787,85 +937,23 @@ bthidev_linkmode(void *arg, int new)
 }
 
 /*
- * Receive reports from the protocol stack.
+ * Receive reports from the protocol stack. Because this will be called
+ * with bt_lock held, we queue the mbuf and process it with a kernel thread
  */
 static void
 bthidev_input(void *arg, struct mbuf *m)
 {
        struct bthidev_softc *sc = arg;
-       struct bthidev *hidev;
-       uint8_t *data;
-       int len;
-
-       if (sc->sc_state != BTHID_OPEN)
-               goto release;
-
-       if (m->m_pkthdr.len > m->m_len)
-               aprint_error_dev(sc->sc_dev, "truncating HID report\n");
-
-       len = m->m_len;
-       data = mtod(m, uint8_t *);
-
-       if (BTHID_TYPE(data[0]) == BTHID_DATA) {
-               /*
-                * data[0] == type / parameter
-                * data[1] == id
-                * data[2..len] == report
-                */
-               if (len < 3)
-                       goto release;
-
-               LIST_FOREACH(hidev, &sc->sc_list, sc_next) {
-                       if (data[1] == hidev->sc_id) {
-                               switch (BTHID_DATA_PARAM(data[0])) {
-                               case BTHID_DATA_INPUT:
-                                       (*hidev->sc_input)(hidev, data + 2, len 
- 2);
-                                       break;
-
-                               case BTHID_DATA_FEATURE:
-                                       (*hidev->sc_feature)(hidev, data + 2, 
len - 2);
-                                       break;
-
-                               default:
-                                       break;
-                               }
-
-                               goto release;
-                       }
-               }
-               aprint_error_dev(sc->sc_dev, "report id %d, len = %d ignored\n",
-                   data[1], len - 2);
 
-               goto release;
-       }
-
-       if (BTHID_TYPE(data[0]) == BTHID_CONTROL) {
-               if (len < 1)
-                       goto release;
-
-               if (BTHID_DATA_PARAM(data[0]) == BTHID_CONTROL_UNPLUG) {
-                       aprint_normal_dev(sc->sc_dev, "unplugged\n");
-
-                       /* close interrupt channel */
-                       if (sc->sc_int != NULL) {
-                               l2cap_disconnect(sc->sc_int, 0);
-                               l2cap_detach(&sc->sc_int);
-                               sc->sc_int = NULL;
-                       }
-
-                       /* close control channel */
-                       if (sc->sc_ctl != NULL) {
-                               l2cap_disconnect(sc->sc_ctl, 0);
-                               l2cap_detach(&sc->sc_ctl);
-                               sc->sc_ctl = NULL;
-                       }
-               }
-
-               goto release;
+       if (sc->sc_state != BTHID_OPEN) {
+               m_freem(m);
+               return;
        }
 
-release:
-       m_freem(m);
+       mutex_enter(&sc->sc_lock);
+       MBUFQ_ENQUEUE(&sc->sc_inq, m);
+       cv_signal(&sc->sc_cv);
+       mutex_exit(&sc->sc_lock);
 }
 
 /*****************************************************************************
@@ -919,8 +1007,9 @@ bthidev_output(struct bthidev *hidev, ui
        memcpy(mtod(m, uint8_t *) + 2, report, rlen);
        m->m_pkthdr.len = m->m_len = rlen + 2;
 
-       KASSERT(mutex_owned(bt_lock));
+       mutex_enter(bt_lock);
        err = l2cap_send(sc->sc_int, m);
+       mutex_exit(bt_lock);
 
        return err;
 }
Index: btkbd.c
===================================================================
RCS file: /cvsroot/src/sys/dev/bluetooth/btkbd.c,v
retrieving revision 1.12
diff -u -p -r1.12 btkbd.c
--- btkbd.c     31 Dec 2011 01:16:09 -0000      1.12
+++ btkbd.c     7 Jan 2012 08:14:36 -0000
@@ -378,9 +378,7 @@ btkbd_ioctl(void *self, unsigned long cm
                break;
 
        case WSKBDIO_SETLEDS:
-               mutex_enter(bt_lock);
                btkbd_set_leds(sc, *(int *)data);
-               mutex_exit(bt_lock);
                break;
 
        case WSKBDIO_GETLEDS:


Home | Main Index | Thread Index | Old Index