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

[matthew green <mrg%eterna.com.au@localhost>]

> > >> Module Name:     src
> > >> Committed By:    rkujawa
> > >> Date:            Sat Dec 31 01:16:09 UTC 2011
> > >>
> > >> Modified Files:
> > >>  src/sys/dev/bluetooth: bthidev.c btkbd.c
> > >>
> > >> Log Message:
> > >> Fix panic triggered by pressing the caps lock key:
> > >> http://c0ff33.net/drop/bt_caps_panic.jpg
> > >
> > > this does not seem right - I've never had such a panic,
> >
> > I can easily reproduce this problem with Apple Wireless Keyboard
> > (version without numeric keypad) on NetBSD/amd64 HEAD. If this patch is
> > not applied, pressing caps lock always results in panic, as in photo.
> 
> Hmm, how long have you been using a Bluetooth keyboard?  I wonder if
> something else has changed recently (I've been busy lately - my source is
> from early October, and I want to test something else before updating)
> 
> Also, what kernel options do you have? I am using i386 and have always
> used DIAGNOSTIC (built in now) and I have run with LOCKDEBUG in the past
> though am not at the moment
> 
> I can't see that the type of keyboard will be relevant (I use an older
> apple keyboard here)

for reference, the stack trace from the image is:

lockdebug_abort()
mutex_vector_enter()
bthidev_output()
btkbd_set_lets()
wskbd_translate()
wskbd_input()
btkbd_input()
l2cap_recv_frame()
hci_acl_recv()
hci_intr()
softint_dispatch()

the lock that triggers the panic is bt_lock, which was taken in hci_intr()
and attempted to be re-taken in bthidev_output().

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

the above trace is from when pressing the caps-lock key, i believe.

> > > probably it should be that bt_lock is released in bthidev_input()
> > > before calling the hid_output function..
> >
> > I'm not sure if I understand correctly. The bthidev_input() does not
> > acquire bt_lock at all.
> 
> it is called from within the bluetooth protocol stack, so will be holding
> it already (thats where the 'locking against myself' originates)
> 
> Just dropping the lock and reaquiring it around the sc_input/sc_feature
> call is probably not exactly the right thing to do though (as the stack
> will not be aware that that might have happened and data structures could
> have changed), it really should disassociate from the context, maybe doing
> the call from a callout or separate task instead. (want that kcont(9) now
> pretty please, gimpy :)

i considered attempt to drop and allow re-taking the bt_lock like you
have described, but i didn't know if it was safe, and there's a lot of
code to read to check.  maybe with the above info you can suggest a
better way to solve this problem.


.mrg.