From 0773cbe172e81583e98b2b6f8e7ff0ecc8d87f7b Mon Sep 17 00:00:00 2001 From: riastradh Date: Sun, 2 Aug 2020 01:17:56 +0000 Subject: [PATCH] Remove unnecessary wait in ldbegindetach. Like disk_begindetach, ldbegindetach only commits to detaching but doesn't wait for existing xfers to drain; it is up to the driver to abort them, once we are committed, and then ldenddetach to wait for them to drain. --- sys/dev/ld.c | 51 ++++++++++++++++++++-------------------- sys/dev/ldvar.h | 4 ++-- sys/dev/sdmmc/ld_sdmmc.c | 28 ++++++++++++---------- 3 files changed, 43 insertions(+), 40 deletions(-) diff --git a/sys/dev/ld.c b/sys/dev/ld.c index e15e7b36742f..0ad9b1767a88 100644 --- a/sys/dev/ld.c +++ b/sys/dev/ld.c @@ -1,4 +1,4 @@ -/* $NetBSD: ld.c,v 1.110 2020/04/13 08:05:02 maxv Exp $ */ +/* $NetBSD: ld.c,v 1.111 2020/08/02 01:17:56 riastradh Exp $ */ /*- * Copyright (c) 1998, 2000 The NetBSD Foundation, Inc. @@ -34,7 +34,7 @@ */ #include -__KERNEL_RCSID(0, "$NetBSD: ld.c,v 1.110 2020/04/13 08:05:02 maxv Exp $"); +__KERNEL_RCSID(0, "$NetBSD: ld.c,v 1.111 2020/08/02 01:17:56 riastradh Exp $"); #include #include @@ -189,26 +189,25 @@ int ldbegindetach(struct ld_softc *sc, int flags) { struct dk_softc *dksc = &sc->sc_dksc; - int rv = 0; + int error; + /* If we never attached properly, no problem with detaching. */ if ((sc->sc_flags & LDF_ENABLED) == 0) - return (0); + return 0; - rv = disk_begindetach(&dksc->sc_dkdev, ld_lastclose, dksc->sc_dev, flags); + /* + * If the disk is still open, back out before we commit to + * detaching. + */ + error = disk_begindetach(&dksc->sc_dkdev, ld_lastclose, dksc->sc_dev, + flags); + if (error) + return error; - if (rv != 0) - return rv; + /* We are now committed to detaching. Prevent new xfers. */ + ldadjqparam(sc, 0); - mutex_enter(&sc->sc_mutex); - sc->sc_maxqueuecnt = 0; - - while (sc->sc_queuecnt > 0) { - sc->sc_flags |= LDF_DRAIN; - cv_wait(&sc->sc_drain, &sc->sc_mutex); - } - mutex_exit(&sc->sc_mutex); - - return (rv); + return 0; } void @@ -220,12 +219,17 @@ ldenddetach(struct ld_softc *sc) if ((sc->sc_flags & LDF_ENABLED) == 0) return; - mutex_enter(&sc->sc_mutex); - /* Wait for commands queued with the hardware to complete. */ - if (sc->sc_queuecnt != 0) { - if (cv_timedwait(&sc->sc_drain, &sc->sc_mutex, 30 * hz)) + mutex_enter(&sc->sc_mutex); + while (sc->sc_queuecnt > 0) { + if (cv_timedwait(&sc->sc_drain, &sc->sc_mutex, 30 * hz)) { + /* + * XXX This seems like a recipe for crashing on + * use after free... + */ printf("%s: not drained\n", dksc->sc_xname); + break; + } } mutex_exit(&sc->sc_mutex); @@ -467,10 +471,7 @@ lddone(struct ld_softc *sc, struct buf *bp) mutex_enter(&sc->sc_mutex); if (--sc->sc_queuecnt <= sc->sc_maxqueuecnt) { - if ((sc->sc_flags & LDF_DRAIN) != 0) { - sc->sc_flags &= ~LDF_DRAIN; - cv_broadcast(&sc->sc_drain); - } + cv_broadcast(&sc->sc_drain); mutex_exit(&sc->sc_mutex); dk_start(dksc, NULL); } else diff --git a/sys/dev/ldvar.h b/sys/dev/ldvar.h index aa40f3890a2c..84bd3c5166f6 100644 --- a/sys/dev/ldvar.h +++ b/sys/dev/ldvar.h @@ -1,4 +1,4 @@ -/* $NetBSD: ldvar.h,v 1.33 2019/03/19 07:01:14 mlelstv Exp $ */ +/* $NetBSD: ldvar.h,v 1.34 2020/08/02 01:17:56 riastradh Exp $ */ /*- * Copyright (c) 2000 The NetBSD Foundation, Inc. @@ -67,7 +67,7 @@ struct ld_softc { /* sc_flags */ #define LDF_ENABLED 0x001 /* device enabled */ -#define LDF_DRAIN 0x020 /* maxqueuecnt has changed; drain */ +#define LDF_UNUSED0 0x020 /* was LDF_DRAIN */ #define LDF_NO_RND 0x040 /* do not attach rnd source */ #define LDF_MPSAFE 0x080 /* backend is MPSAFE */ diff --git a/sys/dev/sdmmc/ld_sdmmc.c b/sys/dev/sdmmc/ld_sdmmc.c index 390161115369..575a57782e46 100644 --- a/sys/dev/sdmmc/ld_sdmmc.c +++ b/sys/dev/sdmmc/ld_sdmmc.c @@ -1,4 +1,4 @@ -/* $NetBSD: ld_sdmmc.c,v 1.40 2020/07/22 17:18:10 riastradh Exp $ */ +/* $NetBSD: ld_sdmmc.c,v 1.41 2020/08/02 01:17:56 riastradh Exp $ */ /* * Copyright (c) 2008 KIYOHARA Takashi @@ -28,7 +28,7 @@ */ #include -__KERNEL_RCSID(0, "$NetBSD: ld_sdmmc.c,v 1.40 2020/07/22 17:18:10 riastradh Exp $"); +__KERNEL_RCSID(0, "$NetBSD: ld_sdmmc.c,v 1.41 2020/08/02 01:17:56 riastradh Exp $"); #ifdef _KERNEL_OPT #include "opt_sdmmc.h" @@ -344,11 +344,20 @@ ld_sdmmc_detach(device_t dev, int flags) struct ld_sdmmc_softc *sc = device_private(dev); struct ld_softc *ld = &sc->sc_ld; struct ld_sdmmc_task *task; - int rv, i; + int error, i; /* - * Block new xfers, abort all pending tasks, and wait for all - * pending waiters to notice that we're gone. + * Block new xfers, or fail if the disk is still open and the + * detach isn't forced. After this point, we are committed to + * detaching. + */ + error = ldbegindetach(ld, flags); + if (error) + return error; + + /* + * Abort all pending tasks, and wait for all pending waiters to + * notice that we're gone. */ mutex_enter(&sc->sc_lock); sc->sc_dying = true; @@ -358,14 +367,7 @@ ld_sdmmc_detach(device_t dev, int flags) cv_wait(&sc->sc_cv, &sc->sc_lock); mutex_exit(&sc->sc_lock); - /* Do the ld detach dance. */ - if ((rv = ldbegindetach(ld, flags)) != 0) { - /* Detach failed -- back out. */ - mutex_enter(&sc->sc_lock); - sc->sc_dying = false; - mutex_exit(&sc->sc_lock); - return rv; - } + /* Done! Destroy the disk. */ ldenddetach(ld); KASSERT(TAILQ_EMPTY(&sc->sc_xferq));