Module Name: src
Committed By: martin
Date: Fri Oct 25 16:25:14 UTC 2019
Modified Files:
src/sys/dev/onewire: onewire.c onewirereg.h owtemp.c
Log Message:
PR kern/54617: onewire(4):
- Alter locking strategy to avoid deadlock on detach.
- Auto bus probe chews CPU. Increase interval from 3s to 10s.
- Put temp sensor S/N in dev description so it can be identified.
- Use mutex/condvar.
Patch from Andrew Doran.
To generate a diff of this commit:
cvs rdiff -u -r1.16 -r1.17 src/sys/dev/onewire/onewire.c
cvs rdiff -u -r1.3 -r1.4 src/sys/dev/onewire/onewirereg.h
cvs rdiff -u -r1.17 -r1.18 src/sys/dev/onewire/owtemp.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
Modified files:
Index: src/sys/dev/onewire/onewire.c
diff -u src/sys/dev/onewire/onewire.c:1.16 src/sys/dev/onewire/onewire.c:1.17
--- src/sys/dev/onewire/onewire.c:1.16 Fri Jul 25 08:10:38 2014
+++ src/sys/dev/onewire/onewire.c Fri Oct 25 16:25:14 2019
@@ -1,4 +1,4 @@
-/* $NetBSD: onewire.c,v 1.16 2014/07/25 08:10:38 dholland Exp $ */
+/* $NetBSD: onewire.c,v 1.17 2019/10/25 16:25:14 martin Exp $ */
/* $OpenBSD: onewire.c,v 1.1 2006/03/04 16:27:03 grange Exp $ */
/*
@@ -18,7 +18,7 @@
*/
#include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: onewire.c,v 1.16 2014/07/25 08:10:38 dholland Exp $");
+__KERNEL_RCSID(0, "$NetBSD: onewire.c,v 1.17 2019/10/25 16:25:14 martin Exp $");
/*
* 1-Wire bus driver.
@@ -30,8 +30,7 @@ __KERNEL_RCSID(0, "$NetBSD: onewire.c,v
#include <sys/device.h>
#include <sys/kernel.h>
#include <sys/kthread.h>
-#include <sys/rwlock.h>
-#include <sys/malloc.h>
+#include <sys/kmem.h>
#include <sys/proc.h>
#include <sys/queue.h>
#include <sys/module.h>
@@ -45,18 +44,16 @@ __KERNEL_RCSID(0, "$NetBSD: onewire.c,v
#define DPRINTF(x)
#endif
-//#define ONEWIRE_MAXDEVS 256
-#define ONEWIRE_MAXDEVS 8
-#define ONEWIRE_SCANTIME 3
+int onewire_maxdevs = 8;
+int onewire_scantime = 10; /* was 3 seconds - too often */
struct onewire_softc {
device_t sc_dev;
-
struct onewire_bus * sc_bus;
- krwlock_t sc_rwlock;
+ kmutex_t sc_lock;
+ kcondvar_t sc_scancv;
struct lwp * sc_thread;
TAILQ_HEAD(, onewire_device) sc_devs;
-
int sc_dying;
};
@@ -64,7 +61,7 @@ struct onewire_device {
TAILQ_ENTRY(onewire_device) d_list;
device_t d_dev;
u_int64_t d_rom;
- int d_present;
+ bool d_present;
};
static int onewire_match(device_t, cfdata_t, void *);
@@ -79,21 +76,6 @@ static void onewire_scan(struct onewire_
CFATTACH_DECL_NEW(onewire, sizeof(struct onewire_softc),
onewire_match, onewire_attach, onewire_detach, onewire_activate);
-const struct cdevsw onewire_cdevsw = {
- .d_open = noopen,
- .d_close = noclose,
- .d_read = noread,
- .d_write = nowrite,
- .d_ioctl = noioctl,
- .d_stop = nostop,
- .d_tty = notty,
- .d_poll = nopoll,
- .d_mmap = nommap,
- .d_kqfilter = nokqfilter,
- .d_discard = nodiscard,
- .d_flag = D_OTHER
-};
-
extern struct cfdriver onewire_cd;
static int
@@ -110,14 +92,19 @@ onewire_attach(device_t parent, device_t
sc->sc_dev = self;
sc->sc_bus = oba->oba_bus;
- rw_init(&sc->sc_rwlock);
+ mutex_init(&sc->sc_lock, MUTEX_DEFAULT, IPL_NONE);
+ cv_init(&sc->sc_scancv, "owscan");
TAILQ_INIT(&sc->sc_devs);
aprint_normal("\n");
- if (kthread_create(PRI_NONE, 0, NULL, onewire_thread, sc,
- &sc->sc_thread, "%s", device_xname(self)) != 0)
+ if (kthread_create(PRI_NONE, KTHREAD_MUSTJOIN | KTHREAD_MPSAFE, NULL,
+ onewire_thread, sc, &sc->sc_thread, "%s", device_xname(self)) != 0) {
aprint_error_dev(self, "can't create kernel thread\n");
+ /* Normally the kthread destroys these. */
+ mutex_destroy(&sc->sc_lock);
+ cv_destroy(&sc->sc_scancv);
+ }
}
static int
@@ -126,17 +113,17 @@ onewire_detach(device_t self, int flags)
struct onewire_softc *sc = device_private(self);
int rv;
- sc->sc_dying = 1;
if (sc->sc_thread != NULL) {
- wakeup(sc->sc_thread);
- tsleep(&sc->sc_dying, PWAIT, "owdt", 0);
+ mutex_enter(&sc->sc_lock);
+ sc->sc_dying = 1;
+ cv_broadcast(&sc->sc_scancv);
+ mutex_exit(&sc->sc_lock);
+ /* Must no longer touch sc_lock nor sc_scancv. */
+ kthread_join(sc->sc_thread);
}
- onewire_lock(sc);
//rv = config_detach_children(self, flags);
rv = 0; /* XXX riz */
- onewire_unlock(sc);
- rw_destroy(&sc->sc_rwlock);
return rv;
}
@@ -170,7 +157,7 @@ onewire_print(void *aux, const char *pnp
(uint)ONEWIRE_ROM_FAMILY_TYPE(oa->oa_rom));
else
aprint_normal("\"%s\"", famname);
- aprint_normal(" sn %012llx", ONEWIRE_ROM_SN(oa->oa_rom));
+ aprint_normal(" sn %012" PRIx64, ONEWIRE_ROM_SN(oa->oa_rom));
if (pnp != NULL)
aprint_normal(" at %s", pnp);
@@ -192,7 +179,7 @@ onewire_lock(void *arg)
{
struct onewire_softc *sc = arg;
- rw_enter(&sc->sc_rwlock, RW_WRITER);
+ mutex_enter(&sc->sc_lock);
}
void
@@ -200,7 +187,7 @@ onewire_unlock(void *arg)
{
struct onewire_softc *sc = arg;
- rw_exit(&sc->sc_rwlock);
+ mutex_exit(&sc->sc_lock);
}
int
@@ -209,6 +196,8 @@ onewire_reset(void *arg)
struct onewire_softc *sc = arg;
struct onewire_bus *bus = sc->sc_bus;
+ KASSERT(mutex_owned(&sc->sc_lock));
+
return bus->bus_reset(bus->bus_cookie);
}
@@ -218,6 +207,8 @@ onewire_bit(void *arg, int value)
struct onewire_softc *sc = arg;
struct onewire_bus *bus = sc->sc_bus;
+ KASSERT(mutex_owned(&sc->sc_lock));
+
return bus->bus_bit(bus->bus_cookie, value);
}
@@ -229,6 +220,8 @@ onewire_read_byte(void *arg)
uint8_t value = 0;
int i;
+ KASSERT(mutex_owned(&sc->sc_lock));
+
if (bus->bus_read_byte != NULL)
return bus->bus_read_byte(bus->bus_cookie);
@@ -245,6 +238,8 @@ onewire_write_byte(void *arg, int value)
struct onewire_bus *bus = sc->sc_bus;
int i;
+ KASSERT(mutex_owned(&sc->sc_lock));
+
if (bus->bus_write_byte != NULL)
return bus->bus_write_byte(bus->bus_cookie, value);
@@ -259,6 +254,8 @@ onewire_triplet(void *arg, int dir)
struct onewire_bus *bus = sc->sc_bus;
int rv;
+ KASSERT(mutex_owned(&sc->sc_lock));
+
if (bus->bus_triplet != NULL)
return bus->bus_triplet(bus->bus_cookie, dir);
@@ -283,29 +280,38 @@ onewire_triplet(void *arg, int dir)
void
onewire_read_block(void *arg, void *buf, int len)
{
+ struct onewire_softc *sc = arg;
uint8_t *p = buf;
+ KASSERT(mutex_owned(&sc->sc_lock));
+
while (len--)
- *p++ = onewire_read_byte(arg);
+ *p++ = onewire_read_byte(sc);
}
void
onewire_write_block(void *arg, const void *buf, int len)
{
+ struct onewire_softc *sc = arg;
const uint8_t *p = buf;
+ KASSERT(mutex_owned(&sc->sc_lock));
+
while (len--)
- onewire_write_byte(arg, *p++);
+ onewire_write_byte(sc, *p++);
}
void
onewire_matchrom(void *arg, u_int64_t rom)
{
+ struct onewire_softc *sc = arg;
int i;
- onewire_write_byte(arg, ONEWIRE_CMD_MATCH_ROM);
+ KASSERT(mutex_owned(&sc->sc_lock));
+
+ onewire_write_byte(sc, ONEWIRE_CMD_MATCH_ROM);
for (i = 0; i < 8; i++)
- onewire_write_byte(arg, (rom >> (i * 8)) & 0xff);
+ onewire_write_byte(sc, (rom >> (i * 8)) & 0xff);
}
static void
@@ -313,13 +319,17 @@ onewire_thread(void *arg)
{
struct onewire_softc *sc = arg;
+ mutex_enter(&sc->sc_lock);
while (!sc->sc_dying) {
onewire_scan(sc);
- tsleep(sc->sc_thread, PWAIT, "owidle", ONEWIRE_SCANTIME * hz);
+ (void)cv_timedwait(&sc->sc_scancv, &sc->sc_lock,
+ onewire_scantime * hz);
}
+ mutex_exit(&sc->sc_lock);
- sc->sc_thread = NULL;
- wakeup(&sc->sc_dying);
+ /* Caller has set sc_dying and will no longer touch these. */
+ cv_destroy(&sc->sc_scancv);
+ mutex_destroy(&sc->sc_lock);
kthread_exit(0);
}
@@ -328,29 +338,28 @@ onewire_scan(struct onewire_softc *sc)
{
struct onewire_device *d, *next, *nd;
struct onewire_attach_args oa;
- device_t dev;
int search = 1, count = 0, present;
int dir, rv;
uint64_t mask, rom = 0, lastrom;
uint8_t data[8];
int i, i0 = -1, lastd = -1;
- TAILQ_FOREACH(d, &sc->sc_devs, d_list)
- d->d_present = 0;
+ TAILQ_FOREACH(d, &sc->sc_devs, d_list) {
+ d->d_present = false;
+ KASSERT(d->d_dev != NULL);
+ }
- while (search && count++ < ONEWIRE_MAXDEVS) {
- /* XXX: yield processor */
- tsleep(sc, PWAIT, "owscan", hz / 10);
+ KASSERT(mutex_owned(&sc->sc_lock));
+ KASSERT(curlwp == sc->sc_thread);
+ while (search && count++ < onewire_maxdevs) {
/*
* Reset the bus. If there's no presence pulse
* don't search for any devices.
*/
- onewire_lock(sc);
if (onewire_reset(sc) != 0) {
DPRINTF(("%s: scan: no presence pulse\n",
device_xname(sc->sc_dev)));
- onewire_unlock(sc);
break;
}
@@ -390,13 +399,11 @@ onewire_scan(struct onewire_softc *sc)
DPRINTF(("%s: scan: triplet error 0x%x, "
"step %d\n",
device_xname(sc->sc_dev), rv, i));
- onewire_unlock(sc);
return;
}
rom |= (mask << i);
}
lastd = i0;
- onewire_unlock(sc);
if (rom == 0)
continue;
@@ -418,42 +425,60 @@ onewire_scan(struct onewire_softc *sc)
present = 0;
TAILQ_FOREACH(d, &sc->sc_devs, d_list) {
if (d->d_rom == rom) {
- d->d_present = 1;
+ d->d_present = true;
present = 1;
break;
}
}
if (!present) {
- memset(&oa, 0, sizeof(oa));
- oa.oa_onewire = sc;
- oa.oa_rom = rom;
- if ((dev = config_found(sc->sc_dev, &oa,
- onewire_print)) == NULL)
- continue;
-
- nd = malloc(sizeof(struct onewire_device),
- M_DEVBUF, M_NOWAIT);
- if (nd == NULL)
- continue;
- nd->d_dev = dev;
+ nd = kmem_alloc(sizeof(*nd), KM_SLEEP);
+ nd->d_dev = NULL;
nd->d_rom = rom;
- nd->d_present = 1;
+ nd->d_present = true;
TAILQ_INSERT_TAIL(&sc->sc_devs, nd, d_list);
}
+
+ /*
+ * Yield processor, but continue to hold the lock
+ * so that scan is not interrupted.
+ */
+ kpause("owscan", false, 1, NULL);
}
- /* Detach disappeared devices */
- onewire_lock(sc);
- for (d = TAILQ_FIRST(&sc->sc_devs);
- d != NULL; d = next) {
+ /*
+ * Detach disappeared devices, and attach new devices. Drop the
+ * lock when doing this in order to prevent lock order reversal
+ * against sysmon. This is safe because nothing other than this
+ * kthread modifies our device list.
+ */
+ for (d = TAILQ_FIRST(&sc->sc_devs); d != NULL; d = next) {
next = TAILQ_NEXT(d, d_list);
if (!d->d_present) {
+ mutex_exit(&sc->sc_lock);
+
+ KERNEL_LOCK(1, NULL); /* XXXSMP */
config_detach(d->d_dev, DETACH_FORCE);
+ d->d_dev = NULL;
+ KERNEL_UNLOCK_ONE(NULL); /* XXXSMP */
+
+ mutex_enter(&sc->sc_lock);
+ } else if (d->d_dev == NULL) {
+ memset(&oa, 0, sizeof(oa));
+ oa.oa_onewire = sc;
+ oa.oa_rom = d->d_rom;
+ mutex_exit(&sc->sc_lock);
+
+ KERNEL_LOCK(1, NULL); /* XXXSMP */
+ d->d_dev = config_found(sc->sc_dev, &oa, onewire_print);
+ KERNEL_UNLOCK_ONE(NULL); /* XXXSMP */
+
+ mutex_enter(&sc->sc_lock);
+ }
+ if (d->d_dev == NULL) {
TAILQ_REMOVE(&sc->sc_devs, d, d_list);
- free(d, M_DEVBUF);
+ kmem_free(d, sizeof(*d));
}
}
- onewire_unlock(sc);
}
MODULE(MODULE_CLASS_DRIVER, onewire, NULL);
Index: src/sys/dev/onewire/onewirereg.h
diff -u src/sys/dev/onewire/onewirereg.h:1.3 src/sys/dev/onewire/onewirereg.h:1.4
--- src/sys/dev/onewire/onewirereg.h:1.3 Fri Apr 14 18:38:50 2006
+++ src/sys/dev/onewire/onewirereg.h Fri Oct 25 16:25:14 2019
@@ -1,4 +1,4 @@
-/* $NetBSD: onewirereg.h,v 1.3 2006/04/14 18:38:50 riz Exp $ */
+/* $NetBSD: onewirereg.h,v 1.4 2019/10/25 16:25:14 martin Exp $ */
/* $OpenBSD: onewirereg.h,v 1.1 2006/03/04 16:27:03 grange Exp $ */
/*
@@ -34,7 +34,7 @@
#define ONEWIRE_ROM_FAMILY_CUSTOM(x) (((x) >> 7) & 0x1)
/* Serial number */
-#define ONEWIRE_ROM_SN(x) (((x) >> 8) & 0xffffffffffffULL)
+#define ONEWIRE_ROM_SN(x) (((x) >> 8) & ((uint64_t)0xffffffffffffULL))
/* CRC */
#define ONEWIRE_ROM_CRC(x) (((x) >> 56) & 0xff)
Index: src/sys/dev/onewire/owtemp.c
diff -u src/sys/dev/onewire/owtemp.c:1.17 src/sys/dev/onewire/owtemp.c:1.18
--- src/sys/dev/onewire/owtemp.c:1.17 Wed May 14 08:14:56 2014
+++ src/sys/dev/onewire/owtemp.c Fri Oct 25 16:25:14 2019
@@ -1,4 +1,4 @@
-/* $NetBSD: owtemp.c,v 1.17 2014/05/14 08:14:56 kardel Exp $ */
+/* $NetBSD: owtemp.c,v 1.18 2019/10/25 16:25:14 martin Exp $ */
/* $OpenBSD: owtemp.c,v 1.1 2006/03/04 16:27:03 grange Exp $ */
/*
@@ -22,7 +22,7 @@
*/
#include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: owtemp.c,v 1.17 2014/05/14 08:14:56 kardel Exp $");
+__KERNEL_RCSID(0, "$NetBSD: owtemp.c,v 1.18 2019/10/25 16:25:14 martin Exp $");
#include <sys/param.h>
#include <sys/systm.h>
@@ -40,8 +40,10 @@ __KERNEL_RCSID(0, "$NetBSD: owtemp.c,v 1
#define DS_CMD_READ_SCRATCHPAD 0xbe
struct owtemp_softc {
- void * sc_onewire;
+ device_t sc_dv;
+ void *sc_onewire;
u_int64_t sc_rom;
+ const char *sc_chipname;
envsys_data_t sc_sensor;
struct sysmon_envsys *sc_sme;
@@ -89,15 +91,21 @@ owtemp_attach(device_t parent, device_t
aprint_naive("\n");
+ sc->sc_dv = self;
sc->sc_onewire = oa->oa_onewire;
sc->sc_rom = oa->oa_rom;
switch(ONEWIRE_ROM_FAMILY_TYPE(sc->sc_rom)) {
case ONEWIRE_FAMILY_DS18B20:
+ sc->sc_chipname = "DS18B20";
+ sc->sc_owtemp_decode = owtemp_decode_ds18b20;
+ break;
case ONEWIRE_FAMILY_DS1822:
+ sc->sc_chipname = "DS1822";
sc->sc_owtemp_decode = owtemp_decode_ds18b20;
break;
case ONEWIRE_FAMILY_DS1920:
+ sc->sc_chipname = "DS1920";
sc->sc_owtemp_decode = owtemp_decode_ds1920;
break;
}
@@ -109,6 +117,8 @@ owtemp_attach(device_t parent, device_t
sc->sc_sensor.state = ENVSYS_SINVALID;
(void)strlcpy(sc->sc_sensor.desc,
device_xname(self), sizeof(sc->sc_sensor.desc));
+ (void)snprintf(sc->sc_sensor.desc, sizeof(sc->sc_sensor.desc),
+ "%s S/N %012" PRIx64, sc->sc_chipname, ONEWIRE_ROM_SN(sc->sc_rom));
if (sysmon_envsys_sensor_attach(sc->sc_sme, &sc->sc_sensor)) {
sysmon_envsys_destroy(sc->sc_sme);
return;
@@ -159,8 +169,10 @@ owtemp_update(void *arg)
u_int8_t data[9];
onewire_lock(sc->sc_onewire);
- if (onewire_reset(sc->sc_onewire) != 0)
+ if (onewire_reset(sc->sc_onewire) != 0) {
+ aprint_error_dev(sc->sc_dv, "owtemp_update: 1st reset failed\n");
goto done;
+ }
onewire_matchrom(sc->sc_onewire, sc->sc_rom);
/*
@@ -168,13 +180,15 @@ owtemp_update(void *arg)
* After sending the command, the data line must be held high for
* at least 750ms to provide power during the conversion process.
* As such, no other activity may take place on the 1-Wire bus for
- * at least this period.
+ * at least this period. Keep the parent bus locked while waiting.
*/
onewire_write_byte(sc->sc_onewire, DS_CMD_CONVERT);
- tsleep(sc, PRIBIO, "owtemp", hz);
+ kpause("owtemp", false, mstohz(750 + 10), NULL);
- if (onewire_reset(sc->sc_onewire) != 0)
+ if (onewire_reset(sc->sc_onewire) != 0) {
+ aprint_error_dev(sc->sc_dv, "owtemp_update: 2nd reset failed\n");
goto done;
+ }
onewire_matchrom(sc->sc_onewire, sc->sc_rom);
/*
