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

[Radoslaw Kujawa <radoslaw.kujawa%c0ff33.net@localhost>]

Hi.

On 2 sty 2012, at 11:00, Iain Hibbert wrote:

> On Sun, 1 Jan 2012, Radoslaw Kujawa wrote:
> 
>> 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've just started. I was using Bluetooth stack for other things for a long 
time, but never really used keyboard on NetBSD (until now).

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

I originally saw this problem on a kernel from August 9. Then I've updated to 
current, to see if this issue persists. So it's not a recent problem.

> Also, what kernel options do you have? I am using i386

Do you have MP machine? Maybe this problem is triggered only on MP?

> and have always
> used DIAGNOSTIC (built in now) and I have run with LOCKDEBUG in the past
> though am not at the moment

I do have DIAGNOSTIC. Full config: http://c0ff33.net/drop/NBWS

Now I also added LOCKDEBUG.

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

Ok, so I shall revert this once we find the correct solution.

>>> 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?

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

Ok, I understood later that it is already held in bthidev_input(), mrg pointed 
out that it was probably acquired in hci_intr(). 

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

Yesterday I've tried dropping the lock this way (around sc_input/sc_feature 
loop), however this results in MUTEX_OWNER(mtx->mtx_owner) == curthread 
assertion failed. Looks like the lock is held by another thread? 

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

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

-- 
Best regards,
Radoslaw Kujawa