Source-Changes-HG archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

[src/jdolecek-ncq]: src/sys/dev refactor code so that xfer c_start() hook is ...



details:   https://anonhg.NetBSD.org/src/rev/cf4d40d57768
branches:  jdolecek-ncq
changeset: 352762:cf4d40d57768
user:      jdolecek <jdolecek%NetBSD.org@localhost>
date:      Sun Sep 10 19:31:15 2017 +0000

description:
refactor code so that xfer c_start() hook is called with channel mutex held,
and hence the controller submit code no longer relies on spl

tested all the affected drivers - wdc (via piixide), ahci, mvsata, siisata,
both disk and atapi I/O

diffstat:

 sys/dev/ata/TODO.ncq       |    6 +-
 sys/dev/ata/ata.c          |  246 ++++++++++++++++++++++++++++--------------
 sys/dev/ata/ata_wdc.c      |   88 ++++++++-------
 sys/dev/ata/atavar.h       |   15 ++-
 sys/dev/ic/ahcisata_core.c |  139 ++++++++++++++++-------
 sys/dev/ic/mvsata.c        |  259 ++++++++++++++++++++++++++++++--------------
 sys/dev/ic/siisata.c       |  131 +++++++++++++++-------
 sys/dev/ic/wdc.c           |   77 ++++++++----
 sys/dev/scsipi/atapi_wdc.c |  124 ++++++++++++++------
 9 files changed, 721 insertions(+), 364 deletions(-)

diffs (truncated from 2919 to 300 lines):

diff -r 37cb78945a3a -r cf4d40d57768 sys/dev/ata/TODO.ncq
--- a/sys/dev/ata/TODO.ncq      Sun Sep 10 19:22:56 2017 +0000
+++ b/sys/dev/ata/TODO.ncq      Sun Sep 10 19:31:15 2017 +0000
@@ -2,11 +2,13 @@
 ----
 test wd* at umass?, confirm the ata_channel kludge works
 
-c_start() needs to be called on splbio to avoid spurious irq during reset,
-is not e.g. in ata thread and may not in atastart() neither
+revise calls to atastart() - now called alsoafter ATASTART_ABORT(), call
+only from intr routine
 - wdc.c never calls atastart() (start always false)
 - ata_wdc.c calls atastart() regardless if error
 
+reconsider freeze/thaw in error recovery - can it screw up with thread?
+
 Other random notes (do outside the NCQ branch):
 -----------------------------------------------------
 do biodone() in wddone() starting the dump to not leak bufs when dumping from
diff -r 37cb78945a3a -r cf4d40d57768 sys/dev/ata/ata.c
--- a/sys/dev/ata/ata.c Sun Sep 10 19:22:56 2017 +0000
+++ b/sys/dev/ata/ata.c Sun Sep 10 19:31:15 2017 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: ata.c,v 1.132.8.30 2017/09/10 19:22:56 jdolecek Exp $  */
+/*     $NetBSD: ata.c,v 1.132.8.31 2017/09/10 19:31:15 jdolecek Exp $  */
 
 /*
  * Copyright (c) 1998, 2001 Manuel Bouyer.  All rights reserved.
@@ -25,7 +25,7 @@
  */
 
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: ata.c,v 1.132.8.30 2017/09/10 19:22:56 jdolecek Exp $");
+__KERNEL_RCSID(0, "$NetBSD: ata.c,v 1.132.8.31 2017/09/10 19:31:15 jdolecek Exp $");
 
 #include "opt_ata.h"
 
@@ -130,8 +130,12 @@
 static void atabusconfig_thread(void *);
 
 static void ata_channel_idle(struct ata_channel *);
+static void ata_channel_thaw_locked(struct ata_channel *);
 static void ata_activate_xfer_locked(struct ata_channel *, struct ata_xfer *);
 static void ata_channel_freeze_locked(struct ata_channel *);
+static struct ata_xfer *ata_queue_get_active_xfer_locked(struct ata_channel *);
+static void ata_thread_wake_locked(struct ata_channel *);
+
 /*
  * atabus_init:
  *
@@ -200,7 +204,7 @@
        struct ata_queue *chq = chp->ch_queue;
        struct ata_xfer *xfer = NULL;
 
-       mutex_enter(&chp->ch_lock);
+       ata_channel_lock(chp);
 
        KASSERTMSG(hwslot < chq->queue_openings, "hwslot %d > openings %d",
            hwslot, chq->queue_openings);
@@ -212,7 +216,7 @@
                        break;
        }
 
-       mutex_exit(&chp->ch_lock);
+       ata_channel_unlock(chp);
 
        KASSERTMSG((xfer != NULL),
            "%s: xfer with slot %d not found (active %x)", __func__,
@@ -221,6 +225,13 @@
        return xfer;
 }
 
+static struct ata_xfer *
+ata_queue_get_active_xfer_locked(struct ata_channel *chp)
+{
+       KASSERT(mutex_owned(&chp->ch_lock));
+       return TAILQ_FIRST(&chp->ch_queue->active_xfers);
+}
+
 /*
  * This interface is supposed only to be used when there is exactly
  * one outstanding command, when there is no information about the slot,
@@ -232,12 +243,12 @@
 {
        struct ata_xfer *xfer = NULL;
 
-       mutex_enter(&chp->ch_lock);
+       ata_channel_lock(chp);
 
        KASSERT(chp->ch_queue->queue_active <= 1);
-       xfer = TAILQ_FIRST(&chp->ch_queue->active_xfers);
+       xfer = ata_queue_get_active_xfer_locked(chp);
 
-       mutex_exit(&chp->ch_lock);
+       ata_channel_unlock(chp);
 
        return xfer;
 }
@@ -247,7 +258,7 @@
 {
        struct ata_xfer *xfer = NULL;
 
-       mutex_enter(&chp->ch_lock);
+       ata_channel_lock(chp);
 
        TAILQ_FOREACH(xfer, &chp->ch_queue->active_xfers, c_activechain) {
                if (xfer->c_drive == drive)
@@ -255,7 +266,7 @@
        }
        KASSERT(xfer != NULL);
 
-       mutex_exit(&chp->ch_lock);
+       ata_channel_unlock(chp);
 
        return xfer;
 }
@@ -374,9 +385,9 @@
        int i, error;
 
        /* we are in the atabus's thread context */
-       mutex_enter(&chp->ch_lock);
+       ata_channel_lock(chp);
        chp->ch_flags |= ATACH_TH_RUN;
-       mutex_exit(&chp->ch_lock);
+       ata_channel_unlock(chp);
 
        /*
         * Probe for the drives attached to controller, unless a PMP
@@ -393,9 +404,9 @@
        }
 
        /* next operations will occurs in a separate thread */
-       mutex_enter(&chp->ch_lock);
+       ata_channel_lock(chp);
        chp->ch_flags &= ~ATACH_TH_RUN;
-       mutex_exit(&chp->ch_lock);
+       ata_channel_unlock(chp);
 
        /* Make sure the devices probe in atabus order to avoid jitter. */
        mutex_enter(&atabus_qlock);
@@ -407,7 +418,7 @@
        }
        mutex_exit(&atabus_qlock);
 
-       mutex_enter(&chp->ch_lock);
+       ata_channel_lock(chp);
 
        /* If no drives, abort here */
        if (chp->ch_drive == NULL)
@@ -423,7 +434,7 @@
        if (chp->ch_flags & ATACH_SHUTDOWN)
                goto out;
 
-       mutex_exit(&chp->ch_lock);
+       ata_channel_unlock(chp);
 
        if ((error = kthread_create(PRI_NONE, 0, NULL, atabusconfig_thread,
            atabus_sc, &atabus_cfg_lwp,
@@ -433,7 +444,7 @@
        return;
 
  out:
-       mutex_exit(&chp->ch_lock);
+       ata_channel_unlock(chp);
 
        mutex_enter(&atabus_qlock);
        TAILQ_REMOVE(&atabus_initq_head, atabus_initq, atabus_initq);
@@ -586,9 +597,9 @@
        struct ata_channel *chp = sc->sc_chan;
        struct ata_queue *chq = chp->ch_queue;
        struct ata_xfer *xfer;
-       int i, s;
+       int i, rv, s;
 
-       mutex_enter(&chp->ch_lock);
+       ata_channel_lock(chp);
        chp->ch_flags |= ATACH_TH_RUN;
 
        /*
@@ -602,11 +613,11 @@
                chp->ch_drive[i].drive_flags = 0;
                chp->ch_drive[i].drive_type = ATA_DRIVET_NONE;
        }
-       mutex_exit(&chp->ch_lock);
+       ata_channel_unlock(chp);
 
        atabusconfig(sc);
 
-       mutex_enter(&chp->ch_lock);
+       ata_channel_lock(chp);
        for (;;) {
                if ((chp->ch_flags & (ATACH_TH_RESET | ATACH_SHUTDOWN)) == 0 &&
                    (chq->queue_active == 0 || chq->queue_freeze == 0)) {
@@ -619,21 +630,21 @@
                }
                if (chp->ch_flags & ATACH_TH_RESCAN) {
                        chp->ch_flags &= ~ATACH_TH_RESCAN;
-                       mutex_exit(&chp->ch_lock);
+                       ata_channel_unlock(chp);
                        atabusconfig(sc);
-                       mutex_enter(&chp->ch_lock);
+                       ata_channel_lock(chp);
                }
                if (chp->ch_flags & ATACH_TH_RESET) {
                        /*
                         * ata_reset_channel() will freeze 2 times, so
                         * unfreeze one time. Not a problem as we're at splbio
                         */
-                       mutex_exit(&chp->ch_lock);
-                       ata_channel_thaw(chp);
+                       ata_channel_thaw_locked(chp);
+                       ata_channel_unlock(chp);
                        s = splbio();
                        ata_reset_channel(chp, AT_WAIT | chp->ch_reset_flags);
                        splx(s);
-                       mutex_enter(&chp->ch_lock);
+                       ata_channel_lock(chp);
                } else if (chq->queue_active > 0 && chq->queue_freeze == 1) {
                        /*
                         * Caller has bumped queue_freeze, decrease it. This
@@ -641,31 +652,39 @@
                         */
                        KASSERT((chp->ch_flags & ATACH_NCQ) == 0);
                        KASSERT(chq->queue_active == 1);
-                       mutex_exit(&chp->ch_lock);
 
-                       ata_channel_thaw(chp);
-                       xfer = ata_queue_get_active_xfer(chp);
+                       ata_channel_thaw_locked(chp);
+                       xfer = ata_queue_get_active_xfer_locked(chp);
+
                        KASSERT(xfer != NULL);
-                       s = splbio();
-                       (*xfer->c_start)(xfer->c_chp, xfer);
-                       splx(s);
-                       mutex_enter(&chp->ch_lock);
+                       KASSERT((xfer->c_flags & C_POLL) == 0);
+
+                       switch ((rv = ata_xfer_start(xfer))) {
+                       case ATASTART_STARTED:
+                       case ATASTART_POLL:
+                       case ATASTART_ABORT:
+                               break;
+                       case ATASTART_TH:
+                       default:
+                               panic("%s: ata_xfer_start() unexpected rv %d",
+                                   __func__, rv);
+                               /* NOTREACHED */
+                       }
                } else if (chq->queue_freeze > 1)
                        panic("%s: queue_freeze", __func__);
        }
        chp->ch_thread = NULL;
        cv_signal(&chp->ch_thr_idle);
-       mutex_exit(&chp->ch_lock);
+       ata_channel_unlock(chp);
        kthread_exit(0);
 }
 
-void
-ata_thread_wake(struct ata_channel *chp)
+static void
+ata_thread_wake_locked(struct ata_channel *chp)
 {
-       mutex_enter(&chp->ch_lock);
+       KASSERT(mutex_owned(&chp->ch_lock));
        ata_channel_freeze_locked(chp);
        cv_signal(&chp->ch_thr_idle);
-       mutex_exit(&chp->ch_lock);
 }
 
 /*
@@ -743,13 +762,13 @@
        int i, error = 0;
 
        /* Shutdown the channel. */
-       mutex_enter(&chp->ch_lock);
+       ata_channel_lock(chp);
        chp->ch_flags |= ATACH_SHUTDOWN;
        while (chp->ch_thread != NULL) {
                cv_signal(&chp->ch_thr_idle);
                cv_wait(&chp->ch_thr_idle, &chp->ch_lock);
        }
-       mutex_exit(&chp->ch_lock);
+       ata_channel_unlock(chp);
 
        /*
         * Detach atapibus and its children.
@@ -1199,7 +1218,7 @@
        /* complete xfer setup */
        xfer->c_chp = chp;
 
-       mutex_enter(&chp->ch_lock);
+       ata_channel_lock(chp);
 
        /*
         * Standard commands are added to the end of command list, but
@@ -1235,7 +1254,7 @@
                }
        }



Home | Main Index | Thread Index | Old Index