tech-kern archive
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]
Re: Request for testers - socket locking diff
On Wed, 23 Apr 2008, Andrew Doran wrote:
> On Wed, Apr 23, 2008 at 08:18:33AM +0100, Iain Hibbert wrote:
>
> > On Tue, 22 Apr 2008, Andrew Doran wrote:
> >
> > > Latest version of the patch, against -current today:
> > >
> > > http://www.netbsd.org/~ad/socklock-2008042201.diff
> >
> > Sorry was unable to boot a -current kernel for a couple of weeks (acpi
> > issue) but will test the bluetooth as far as I'm able soon (visually it
> > looks ok)
Ok there was some tsleep that needed to be interlocked (during device
setup), patch below. It seems to be working ok now though my machine is
an old P3 laptop so no MP.
> o The kernel_lock covering anything to do with the hardware, processing that
> occurs from hardware interrupts (e.g. ip_flow), and so on. For bluetooth
> I'd expect that only needs to lock against device driver interrupts, and
> could be replaced with mutexes in the hardware drivers or something
> along those lines. Again, I don't understand how it interfaces with
> hardware so for now I've left it at the protocol boundary just to be on
> the safe side. In a lot of situations it's probably not necessary to take
> kernel_lock because we are never going to touch any of that state.
I tried to make the interface between device drivers and bluetooth stack
safe a while ago:
- data being passed upwards on device context is stored on a queue
protected by a per-device spin mutex with driver-specified IPL, then a
softint triggered which takes the data off the queue with the spin mutex
and handles it in the bluetooth stack context.
- data being passed downwards is handed to the driver and it is up to the
driver to arbitrate access. currently they all use splxxx() as
appropriate.
perhaps this means the kernel_lock wrappers can come off the bluetooth
code?
iain
--- netbt/hci.h.orig 2008-04-23 20:33:03.000000000 +0100
+++ netbt/hci.h 2008-04-23 20:36:26.000000000 +0100
@@ -2360,6 +2360,7 @@
#ifdef _KERNEL
+#include <sys/condvar.h>
#include <sys/device.h>
struct l2cap_channel;
@@ -2463,6 +2464,7 @@
/* device info */
bdaddr_t hci_bdaddr; /* device address */
uint16_t hci_flags; /* see BTF_ above */
+ kcondvar_t hci_init; /* sleep on this */
uint16_t hci_packet_type; /* packet types */
uint16_t hci_acl_mask; /* ACL packet capabilities */
--- netbt/hci_unit.c.orig 2008-04-23 20:28:24.000000000 +0100
+++ netbt/hci_unit.c 2008-04-23 20:42:35.000000000 +0100
@@ -101,6 +101,7 @@
unit->hci_flags = flags;
mutex_init(&unit->hci_devlock, MUTEX_DRIVER, hci_if->ipl);
+ cv_init(&unit->hci_init, "hci_init");
MBUFQ_INIT(&unit->hci_eventq);
MBUFQ_INIT(&unit->hci_aclrxq);
@@ -128,6 +129,7 @@
SIMPLEQ_REMOVE(&hci_unit_list, unit, hci_unit, hci_next);
mutex_exit(bt_lock);
+ cv_destroy(&unit->hci_init);
mutex_destroy(&unit->hci_devlock);
free(unit, M_BLUETOOTH);
}
@@ -178,7 +180,7 @@
goto bad2;
while (unit->hci_flags & BTF_INIT) {
- err = tsleep(unit, PWAIT | PCATCH, __func__, 5 * hz);
+ err = cv_timedwait_sig(&unit->hci_init, bt_lock, 5 * hz);
if (err)
goto bad2;
--- netbt/hci_event.c.orig 2008-04-23 20:28:24.000000000 +0100
+++ netbt/hci_event.c 2008-04-23 20:40:26.000000000 +0100
@@ -839,7 +839,7 @@
unit->hci_flags &= ~BTF_INIT_BDADDR;
- wakeup(unit);
+ cv_broadcast(&unit->hci_init);
}
/*
@@ -867,7 +867,7 @@
unit->hci_flags &= ~BTF_INIT_BUFFER_SIZE;
- wakeup(unit);
+ cv_broadcast(&unit->hci_init);
}
/*
@@ -955,7 +955,7 @@
unit->hci_flags &= ~BTF_INIT_FEATURES;
- wakeup(unit);
+ cv_broadcast(&unit->hci_init);
DPRINTFN(1, "%s: lmp_mask %4.4x, acl_mask %4.4x, sco_mask %4.4x\n",
device_xname(unit->hci_dev), unit->hci_lmp_mask,
@@ -984,7 +984,7 @@
if (rp.hci_version < HCI_SPEC_V12) {
unit->hci_flags &= ~BTF_INIT_COMMANDS;
- wakeup(unit);
+ cv_broadcast(&unit->hci_init);
return;
}
@@ -1012,7 +1012,7 @@
unit->hci_flags &= ~BTF_INIT_COMMANDS;
memcpy(unit->hci_cmds, rp.commands, HCI_COMMANDS_SIZE);
- wakeup(unit);
+ cv_broadcast(&unit->hci_init);
}
/*
--- dev/bluetooth/bthidev.c.orig 2008-04-23 20:54:06.000000000 +0100
+++ dev/bluetooth/bthidev.c 2008-04-23 20:55:20.000000000 +0100
@@ -97,7 +97,6 @@
#define BTHID_WAIT_CTL 1
#define BTHID_WAIT_INT 2
#define BTHID_OPEN 3
-#define BTHID_DETACHING 4
#define BTHID_RETRY_INTERVAL 5 /* seconds between connection
attempts */
@@ -346,12 +345,7 @@
sc->sc_ctl = NULL;
}
- /* remove callout */
- sc->sc_state = BTHID_DETACHING;
- callout_stop(&sc->sc_reconnect);
- if (callout_invoking(&sc->sc_reconnect))
- tsleep(sc, PWAIT, "bthidetach", 0);
-
+ callout_halt(&sc->sc_reconnect, bt_lock);
callout_destroy(&sc->sc_reconnect);
mutex_exit(bt_lock);
@@ -428,10 +422,6 @@
case BTHID_OPEN:
break;
- case BTHID_DETACHING:
- wakeup(sc);
- break;
-
default:
break;
}
--- dev/bluetooth/btsco.c.orig 2008-04-23 20:58:33.000000000 +0100
+++ dev/bluetooth/btsco.c 2008-04-23 21:04:18.000000000 +0100
@@ -91,6 +91,7 @@
device_t sc_audio; /* MI audio device */
void *sc_intr; /* interrupt cookie */
+ kcondvar_t sc_connect; /* connect wait */
/* Bluetooth */
bdaddr_t sc_laddr; /* local address */
@@ -286,6 +287,7 @@
sc->sc_vgm = 200;
sc->sc_state = BTSCO_CLOSED;
sc->sc_name = device_xname(self);
+ cv_init(&sc->sc_connect, "connect");
/*
* copy in our configuration info
@@ -380,6 +382,8 @@
return EAGAIN;
}
+ cv_destroy(&sc->sc_connect);
+
return 0;
}
@@ -416,7 +420,7 @@
sco_detach(&sc->sc_sco_l);
sc->sc_state = BTSCO_OPEN;
- wakeup(sc);
+ cv_broadcast(&sc->sc_connect);
}
static void
@@ -437,7 +441,7 @@
break;
case BTSCO_WAIT_CONNECT: /* connect failed */
- wakeup(sc);
+ cv_broadcast(&sc->sc_connect);
break;
case BTSCO_OPEN: /* link lost */
@@ -612,7 +616,7 @@
sc->sc_state = BTSCO_WAIT_CONNECT;
while (err == 0 && sc->sc_state == BTSCO_WAIT_CONNECT)
- err = tsleep(sc, PWAIT | PCATCH, "btsco", timo);
+ err = cv_timedwait_sig(&sc->sc_connect, bt_lock, timo);
switch (sc->sc_state) {
case BTSCO_CLOSED: /* disconnected */
Home |
Main Index |
Thread Index |
Old Index