[src/trunk]: src/sys/dev/onewire PR kern/54617: onewire(4):

[martin <martin%NetBSD.org@localhost>]

details:   https://anonhg.NetBSD.org/src/rev/c432f46885e2
branches:  trunk
changeset: 1004344:c432f46885e2
user:      martin <martin%NetBSD.org@localhost>
date:      Fri Oct 25 16:25:14 2019 +0000

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

diffstat:

 sys/dev/onewire/onewire.c    |  187 ++++++++++++++++++++++++------------------
 sys/dev/onewire/onewirereg.h |    4 +-
 sys/dev/onewire/owtemp.c     |   28 ++++-
 3 files changed, 129 insertions(+), 90 deletions(-)

diffs (truncated from 513 to 300 lines):

diff -r 175d960c4785 -r c432f46885e2 sys/dev/onewire/onewire.c
--- a/sys/dev/onewire/onewire.c Fri Oct 25 16:22:48 2019 +0000
+++ b/sys/dev/onewire/onewire.c Fri Oct 25 16:25:14 2019 +0000
@@ -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 @@
 #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 @@
 #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 @@
        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 @@
 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 @@
 
        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 @@
        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 @@
                    (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 @@
 {
        struct onewire_softc *sc = arg;
 
-       rw_enter(&sc->sc_rwlock, RW_WRITER);
+       mutex_enter(&sc->sc_lock);
 }
 
 void
@@ -200,7 +187,7 @@
 {
        struct onewire_softc *sc = arg;
 
-       rw_exit(&sc->sc_rwlock);
+       mutex_exit(&sc->sc_lock);
 }
 
 int
@@ -209,6 +196,8 @@
        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 @@
        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 @@
        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 @@
        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 @@
        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 @@
 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 @@
 {
        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 @@
 {
        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;