Re: CVS commit: src/sys/dev/bluetooth
Le 30/04/2015 18:01, Christos Zoulas a écrit : Module Name: src Committed By: christos Date: Thu Apr 30 16:01:52 UTC 2015 Modified Files: src/sys/dev/bluetooth: bcsp.c Log Message: Fix more memory leaks by changing the transmit routines to always free the mbuf to send. To generate a diff of this commit: cvs rdiff -u -r1.26 -r1.27 src/sys/dev/bluetooth/bcsp.c Why didn't you put a goto out here: 1369:if (_m-m_len 0) 1370:return false; ?
Re: CVS commit: src/sys/dev/bluetooth
On May 8, 12:05pm, m...@m00nbsd.net (Maxime Villard) wrote: -- Subject: Re: CVS commit: src/sys/dev/bluetooth | Why didn't you put a goto out here: | | 1369:if (_m-m_len 0) | 1370:return false; I missed it. I don't know if the test is valid though. If this condition is true it should probably cause an assert... The standard mbuf operations don't check for this. Nevertheless I will fix it to be consistent for now. christos
Re: CVS commit: src/sys/dev/bluetooth
On Fri, May 01, 2015 at 08:42:57AM +0200, Maxime Villard wrote: Yes, ffs_oldfscompat_read() should theoretically be called before ffs_superblock_validate(), otherwise, several fields being overwritten, you either end up with unsanitized values or your disk gets kicked out. I am not sure what ffs_oldfscompat_read() does, but several people (me included) have now unmountable disks that used to work fine in production before, and it is not quite clear what causes the inconsistency, see the thread at https://mail-index.netbsd.org/current-users/2015/04/30/msg027310.html I am pretty sure (but not 100%) that my root filesystem in question has been created past the newfs change cited in that thread, but it is clear that a -current newfs would not trigger the issue. Martin
Re: CVS commit: src/sys/dev/bluetooth
Le 30/04/2015 16:07, Christos Zoulas a écrit : On Apr 30, 8:57am, m...@m00nbsd.net (Maxime Villard) wrote: -- Subject: Re: CVS commit: src/sys/dev/bluetooth How about helping investigate what's wrong with: /* Check the size of cylinder groups */ fs_cgsize = ffs_fragroundup(fs, CGSIZE(fs)); if (fs-fs_cgsize != fs_cgsize) return 0; christos Yes, ffs_oldfscompat_read() should theoretically be called before ffs_superblock_validate(), otherwise, several fields being overwritten, you either end up with unsanitized values or your disk gets kicked out. It's true for almost all the changes I have made so far, and it is true for this one too: fs_qfmask is overwritten for FS_44INODEFMT compat. But given how broken it was (and is still a bit), how untested the whole FFS code was, and given the glamorous XXX in ffs_oldfscompat_read, I didn't touch this.
Re: CVS commit: src/sys/dev/bluetooth
What about the M_PREPEND()? Le 29/04/2015 23:07, Iain Hibbert a écrit : On Wed, 29 Apr 2015, Christos Zoulas wrote: On Apr 29, 4:27pm, m...@m00nbsd.net (Maxime Villard) wrote: -- Subject: Re: CVS commit: src/sys/dev/bluetooth | You didn't test this change, nor did you test the five other fixes | you committed. I checked the code to the best of my ability; perhaps I missed something. by the way, the Brainy code scanner did not pick them up (perhaps it stops at the first one?) but when I looked briefly at this code, there seem other calls to bcsp_tx_unreliable_pkt() which also leak the mbuf on failure, since if it fails it does not free it? iain
Re: CVS commit: src/sys/dev/bluetooth
On Apr 30, 8:57am, m...@m00nbsd.net (Maxime Villard) wrote:
-- Subject: Re: CVS commit: src/sys/dev/bluetooth
| What about the M_PREPEND()?
void
m_freem(struct mbuf *m)
{
struct mbuf *n;
if (m == NULL)
return;
do {
MFREE(m, n);
m = n;
} while (m);
}
How about helping investigate what's wrong with:
/* Check the size of cylinder groups */
fs_cgsize = ffs_fragroundup(fs, CGSIZE(fs));
if (fs-fs_cgsize != fs_cgsize)
return 0;
christos
Re: CVS commit: src/sys/dev/bluetooth
On Apr 29, 10:07pm, plu...@ogmig.net (Iain Hibbert) wrote: -- Subject: Re: CVS commit: src/sys/dev/bluetooth | On Wed, 29 Apr 2015, Christos Zoulas wrote: | | On Apr 29, 4:27pm, m...@m00nbsd.net (Maxime Villard) wrote: | -- Subject: Re: CVS commit: src/sys/dev/bluetooth | | | You didn't test this change, nor did you test the five other fixes | | you committed. | | I checked the code to the best of my ability; perhaps I missed something. | | by the way, the Brainy code scanner did not pick them up (perhaps it stops | at the first one?) but when I looked briefly at this code, there seem | other calls to bcsp_tx_unreliable_pkt() which also leak the mbuf on | failure, since if it fails it does not free it? Exactly, there are more problems. It would help if the original e-mail had less emotional context and more technical. christos
Re: CVS commit: src/sys/dev/bluetooth
Le 27/04/2015 19:36, Christos Zoulas a écrit : Module Name: src Committed By: christos Date: Mon Apr 27 17:36:41 UTC 2015 Modified Files: src/sys/dev/bluetooth: bcsp.c Log Message: free mbuf on failure (Brainy) To generate a diff of this commit: cvs rdiff -u -r1.25 -r1.26 src/sys/dev/bluetooth/bcsp.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files. When I say Please carefully test/investigate these bugs before fixing them I mean Please carefully test/investigate these bugs before fixing them. You didn't test this change, nor did you test the five other fixes you committed. So now please revert them all.
Re: CVS commit: src/sys/dev/bluetooth
On Wed, 29 Apr 2015, Christos Zoulas wrote: On Apr 29, 4:27pm, m...@m00nbsd.net (Maxime Villard) wrote: -- Subject: Re: CVS commit: src/sys/dev/bluetooth | You didn't test this change, nor did you test the five other fixes | you committed. I checked the code to the best of my ability; perhaps I missed something. by the way, the Brainy code scanner did not pick them up (perhaps it stops at the first one?) but when I looked briefly at this code, there seem other calls to bcsp_tx_unreliable_pkt() which also leak the mbuf on failure, since if it fails it does not free it? iain
Re: CVS commit: src/sys/dev/bluetooth
On Apr 29, 4:27pm, m...@m00nbsd.net (Maxime Villard) wrote: -- Subject: Re: CVS commit: src/sys/dev/bluetooth | You didn't test this change, nor did you test the five other fixes | you committed. I checked the code to the best of my ability; perhaps I missed something. | So now please revert them all. Is the reason for that punitive? Because I fail to understand how that helps. christos
Re: CVS commit: src/sys/dev/bluetooth
Hi Iain. On 7 sty 2012, at 09:42, Iain Hibbert wrote: I don't have a USB keyboard, but pressing the caps lock on my laptop or Bluetooth keyboard does always toggle both LEDs. Even after applying your patch it doesn't work this way for me, so probably the problem with this is somewhere else. 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? It works for me. Looks good even with LOCKDEBUG, but I am not a locking expert ;). Since it works, should I commit it, or do you prefer doing it yourself? 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.. I've attached the output of btdevctl. local bdaddr: 00:18:5d:01:17:ad remote bdaddr: 00:1f:5b:fc:ac:86 link mode: encrypt vendor id: 0x05ac product id: 0x022d device type: bthidev control psm: 0x0011 interrupt psm: 0x0013 Collection page=Generic_Desktop usage=Keyboard Input id=1 size=1 count=1 page=Keyboard usage=Keyboard_LeftControl Variable, logical range 0..1 Input id=1 size=1 count=1 page=Keyboard usage=Keyboard_LeftShift Variable, logical range 0..1 Input id=1 size=1 count=1 page=Keyboard usage=Keyboard_LeftAlt Variable, logical range 0..1 Input id=1 size=1 count=1 page=Keyboard usage=Keyboard_Left_GUI Variable, logical range 0..1 Input id=1 size=1 count=1 page=Keyboard usage=Keyboard_RightControl Variable, logical range 0..1 Input id=1 size=1 count=1 page=Keyboard usage=Keyboard_RightShift Variable, logical range 0..1 Input id=1 size=1 count=1 page=Keyboard usage=Keyboard_RightAlt Variable, logical range 0..1 Input id=1 size=1 count=1 page=Keyboard usage=Keyboard_Right_GUI Variable, logical range 0..1 Input id=1 size=8 count=1 page=0x usage=0x Const, logical range 0..1 Output id=1 size=1 count=1 page=LEDs usage=Num_Lock Variable, logical range 0..1 Output id=1 size=1 count=1 page=LEDs usage=Caps_Lock Variable, logical range 0..1 Output id=1 size=1 count=1 page=LEDs usage=Scroll_Lock Variable, logical range 0..1 Output id=1 size=1 count=1 page=LEDs usage=Compose Variable, logical range 0..1 Output id=1 size=1 count=1 page=LEDs usage=Kana Variable, logical range 0..1 Output id=1 size=3 count=1 page=0x usage=0x Const, logical range 0..1 Input id=1 size=8 count=6 page=Keyboard usage=No_Event, logical range 0..255 End collection Collection page=Consumer usage=Consumer_Control Collection page=Generic_Desktop usage=Keyboard Input id=71 size=8 count=1 page=Device_Controls usage=Battery_Strength Variable, logical range 0..255 End collection End collection Input id=17 size=1 count=3 page=0x usage=0x Const, logical range 0..1 Collection page=Consumer usage=Consumer_Control Input id=17 size=1 count=1 page=Consumer usage=Eject Variable, logical range 0..1 Input id=17 size=1 count=1 page=0x00ff usage=0x0003 Variable, logical range 0..1 Input id=17 size=1 count=3 page=0x usage=0x Const, logical range 0..1 Input id=18 size=1 count=1 page=Consumer usage=Pause/Play Variable, logical range 0..1 Input id=18 size=1 count=1 page=Consumer usage=Fast_Forward Variable, logical range 0..1 Input id=18 size=1 count=1 page=Consumer usage=Rewind Variable, logical range 0..1 Input id=18 size=1 count=1 page=Consumer usage=Scan_Next_Track Variable, logical range 0..1 Input id=18 size=1 count=1 page=Consumer usage=Scan_Previous_Track Variable, logical range 0..1 Input id=18 size=1 count=1 page=0x usage=0x Const, logical range 0..1 Input id=18 size=1 count=1 page=0x usage=0x Const, logical range 0..1 Input id=18 size=1 count=1 page=0x usage=0x Const, logical range 0..1 Input id=19 size=1 count=1 page=0xff01 usage=0x000a Variable, logical range 0..1 Input id=19 size=1 count=7 page=0x usage=0x Const, logical range 0..1 Feature id=9 size=8 count=1 page=0xff01 usage=0x000b Variable, logical range 0..1 Feature id=9 size=8 count=2 page=0x
Re: CVS commit: src/sys/dev/bluetooth
On Wed, Jan 11, 2012 at 05:27:45PM +, Iain Hibbert wrote: Module Name: src Committed By: plunky Date: Wed Jan 11 17:27:45 UTC 2012 Modified Files: src/sys/dev/bluetooth: bthidev.c btkbd.c Log Message: offset processing of input reports to a kernel thread, to avoid locking issues when a child device needs to call back into the Bluetooth stack (eg when caps-lock is pressed, and wskbd wants to change a LED) Actually, I suspect it would be better to make X (etc) always use a separate context when reflecting indications back to the driver. As has been pointed out changing the LED for caps-lock (and other locks) is pressed should change the LEDs on all keyboards, and the inter-device cross call could also cause problems. David -- David Laight: da...@l8s.co.uk
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..
iainIndex: 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 - 1.20
+++ bthidev.c 7 Jan 2012 08:14:36 -
@@ -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_tsc_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;
Re: CVS commit: src/sys/dev/bluetooth
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?
re: CVS commit: src/sys/dev/bluetooth
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.
Re: CVS commit: src/sys/dev/bluetooth
On Mon, 2 Jan 2012, Iain Hibbert wrote: 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 booting a LOCKDEBUG kernel shows the same problem here btw iain
re: CVS commit: src/sys/dev/bluetooth
On Mon, 2 Jan 2012, matthew green wrote: 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. Yes I think even if analysis showed it was safe currently, doing that is not safe in the long term.. The correct thing to do is to handle the upcall in a separate context. I can look into it later but I've got to go catch a train in a minute (and, will be without Bluetooth keyboard for a few days :) iain
Re: CVS commit: src/sys/dev/bluetooth
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
Re: CVS commit: src/sys/dev/bluetooth
Hi Iain. Sorry, I should have consulted you before committing this. 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. 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 ? 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. 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. -- Best regards, Radoslaw Kujawa
Re: CVS commit: src/sys/dev/bluetooth
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, and btkbd should really be Bluetooth agnostic since its the same as ukbd (except that ukbd is tied too closely to the USB stack) btkbd_set_leds() may be called from wskbd directly (by pressing caps lock on your built-in keyboard for instance) probably it should be that bt_lock is released in bthidev_input() before calling the hid_output function.. regards, iain
