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?
