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

[Iain Hibbert <plunky%rya-online.net@localhost>]

On Sun, 1 Jan 2012, Radoslaw Kujawa wrote:

> On 1 sty 2012, at 20:44, Iain Hibbert wrote:
>
> > On Sat, 31 Dec 2011, Radoslaw Kujawa wrote:
> >
> >> 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)

> > and btkbd should really be Bluetooth agnostic since its the same as
> > ukbd (except that ukbd is tied too closely to the USB stack)
>
> So we shouldn't fiddle with bt_lock in btkbd.c ?

Really, no we shouldn't.. it is still on my jobs list to merge USB and
Bluetooth HID drivers as it is the same protocol, but the USB parts were
too dependent on the USB stack, reaching over the HID part to eg fetch the
HID descriptors (and some other weird things, in ucycom at least)

So, everything to do with 'Bluetooth' should be handled in bthidev.c (and
everything for 'USB" in uhidev.c) leaving the (kbd, ms, ..) drivers to
interface with the 'HID' API of their parent.. thats why btkbd.c calls the
output method that was passed to it via the autoconf attach routine,
rather than using 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..)

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

iain

PS

I am also looking at the

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?