Source-Changes-HG archive

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

[src/jdolecek-ncq]: src/sys/dev attend error paths, more strict asserts and c...



details:   https://anonhg.NetBSD.org/src/rev/aee1d3443ccf
branches:  jdolecek-ncq
changeset: 352700:aee1d3443ccf
user:      jdolecek <jdolecek%NetBSD.org@localhost>
date:      Tue Jun 27 18:36:03 2017 +0000

description:
attend error paths, more strict asserts and code consistency

- atastart() and ata_kill_pending() now KASSERT() that all xfers on queue
  have same channel
- inactive xfers are killed via new reason KILL_GONE_INACTIVE, controller
  code must not call any resource deactivation in that case
- c_intr() must call ata_waitdrain_xfer_check() as first thing, and must not
  further touch any xfer structures on exit path; any resource cleanup
  is supposed to be done in c_kill_xfer()
- c_kill_xfer() should never call atastart()
- ata_waitdrain_check() removed, replaced by ata_waitdrain_xfer_check()
- ATA_DRIVE_WAITDRAIN handling converted to use condvar
- removed unused ata_c callback

diffstat:

 sys/dev/ata/ata.c          |  112 +++++++++++++++++++++++++++++---------------
 sys/dev/ata/ata_wdc.c      |   20 ++++---
 sys/dev/ata/atavar.h       |   11 ++--
 sys/dev/ic/ahcisata_core.c |   98 +++++++++++++++++++++++++--------------
 sys/dev/ic/mvsata.c        |   71 +++++++++++++++++-----------
 sys/dev/ic/siisata.c       |   99 ++++++++++++++++++++++++---------------
 sys/dev/ic/wdc.c           |   30 ++++++++----
 sys/dev/scsipi/atapi_wdc.c |   21 +++++---
 8 files changed, 290 insertions(+), 172 deletions(-)

diffs (truncated from 1173 to 300 lines):

diff -r 7f334a92c906 -r aee1d3443ccf sys/dev/ata/ata.c
--- a/sys/dev/ata/ata.c Tue Jun 27 18:16:50 2017 +0000
+++ b/sys/dev/ata/ata.c Tue Jun 27 18:36:03 2017 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: ata.c,v 1.132.8.17 2017/06/24 14:57:17 jdolecek Exp $  */
+/*     $NetBSD: ata.c,v 1.132.8.18 2017/06/27 18:36:03 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.17 2017/06/24 14:57:17 jdolecek Exp $");
+__KERNEL_RCSID(0, "$NetBSD: ata.c,v 1.132.8.18 2017/06/27 18:36:03 jdolecek Exp $");
 
 #include "opt_ata.h"
 
@@ -275,6 +275,7 @@
        ata_queue_reset(chq);
 
        cv_init(&chq->queue_busy, "ataqbusy");
+       cv_init(&chq->queue_drain, "atdrn");
 
        for (uint8_t i = 0; i < openings; i++)
                ata_xfer_init(&chq->queue_xfers[i], false);
@@ -288,6 +289,9 @@
        for (uint8_t i = 0; i < chq->queue_openings; i++)
                ata_xfer_destroy(&chq->queue_xfers[i]);
 
+       cv_destroy(&chq->queue_busy);
+       cv_destroy(&chq->queue_drain);
+
        free(chq, M_DEVBUF);
 }
 
@@ -1140,6 +1144,9 @@
        if ((xfer = TAILQ_FIRST(&chp->ch_queue->queue_xfer)) == NULL)
                goto out;
 
+       /* all xfers on same queue must belong to the same channel */
+       KASSERT(xfer->c_chp == chp);
+
        /*
         * Can only take NCQ command if there are no current active
         * commands, or if the active commands are NCQ. Need only check
@@ -1149,10 +1156,6 @@
        if (axfer && (axfer->c_flags & C_NCQ) == 0)
                goto out;
 
-       /* adjust chp, in case we have a shared queue */
-       chp = xfer->c_chp;
-       KASSERT(chp->ch_queue == chq);
-
        struct ata_drive_datas * const drvp = &chp->ch_drive[xfer->c_drive];
 
        /*
@@ -1322,29 +1325,37 @@
        mutex_exit(&chp->ch_lock);
 }
 
-
-bool
-ata_waitdrain_check(struct ata_channel *chp, int drive)
-{
-       if (chp->ch_drive[drive].drive_flags & ATA_DRIVE_WAITDRAIN) {
-               chp->ch_drive[drive].drive_flags &= ~ATA_DRIVE_WAITDRAIN;
-               wakeup(&chp->ch_queue->active_xfers);
-               return true;
-       }
-       return false;
-}
-
+/*
+ * Called in c_intr hook. Must be called before before any deactivations
+ * are done - if there is drain pending, it calls c_kill_xfer hook which
+ * deactivates the xfer.
+ * Calls c_kill_xfer with channel lock free.
+ * Returns true if caller should just exit without further processing.
+ * Caller must not further access any part of xfer or any related controller
+ * structures in that case, it should just return.
+ */
 bool
 ata_waitdrain_xfer_check(struct ata_channel *chp, struct ata_xfer *xfer)
 {
        int drive = xfer->c_drive;
+       bool draining = false;
+
+       mutex_enter(&chp->ch_lock);
+
        if (chp->ch_drive[drive].drive_flags & ATA_DRIVE_WAITDRAIN) {
+               mutex_exit(&chp->ch_lock);
+
                (*xfer->c_kill_xfer)(chp, xfer, KILL_GONE);
+
+               mutex_enter(&chp->ch_lock);
                chp->ch_drive[drive].drive_flags &= ~ATA_DRIVE_WAITDRAIN;
-               wakeup(&chp->ch_queue->active_xfers);
-               return true;
+               cv_signal(&chp->ch_queue->queue_drain);
+               draining = true;
        }
-       return false;
+
+       mutex_exit(&chp->ch_lock);
+
+       return draining;
 }
 
 /*
@@ -1367,38 +1378,58 @@
 }
 
 /*
- * Kill off all pending xfers for a ata_channel.
- *
- * Must be called at splbio().
+ * Kill off all pending xfers for a drive.
  */
 void
 ata_kill_pending(struct ata_drive_datas *drvp)
 {
        struct ata_channel * const chp = drvp->chnl_softc;
        struct ata_queue * const chq = chp->ch_queue;
-       struct ata_xfer *xfer, *next_xfer;
-       int s = splbio();
+       struct ata_xfer *xfer, *xfernext;
+
+       mutex_enter(&chp->ch_lock);
+
+       /* Kill all pending transfers */
+       TAILQ_FOREACH_SAFE(xfer, &chq->queue_xfer, c_xferchain, xfernext) {
+               KASSERT(xfer->c_chp == chp);
 
-       for (xfer = TAILQ_FIRST(&chq->queue_xfer);
-           xfer != NULL; xfer = next_xfer) {
-               next_xfer = TAILQ_NEXT(xfer, c_xferchain);
-               if (xfer->c_chp != chp || xfer->c_drive != drvp->drive)
+               if (xfer->c_drive != drvp->drive)
                        continue;
-               TAILQ_REMOVE(&chq->queue_xfer, xfer, c_xferchain);
-               (*xfer->c_kill_xfer)(chp, xfer, KILL_GONE);
+
+               TAILQ_REMOVE(&chp->ch_queue->queue_xfer, xfer, c_xferchain);
+
+               /*
+                * Keep the lock, so that we get deadlock (and 'locking against
+                * myself' with LOCKDEBUG), instead of silent
+                * data corruption, if the hook tries to call back into
+                * middle layer for inactive xfer.
+                */
+               (*xfer->c_kill_xfer)(chp, xfer, KILL_GONE_INACTIVE);
        }
 
+       /* Wait until all active transfers on the drive finish */
        while (chq->queue_active > 0) {
-               if (chq->queue_openings == 1 && chp->ch_ndrives > 1) {
-                       xfer = TAILQ_FIRST(&chq->active_xfers);
-                       KASSERT(xfer != NULL);
-                       if (xfer->c_chp != chp || xfer->c_drive != drvp->drive)
+               bool drv_active = false;
+
+               TAILQ_FOREACH(xfer, &chq->active_xfers, c_activechain) {
+                       KASSERT(xfer->c_chp == chp);
+
+                       if (xfer->c_drive == drvp->drive) {
+                               drv_active = true;
                                break;
+                       }
                }
+               
+               if (!drv_active) {
+                       /* all finished */
+                       break;
+               }
+
                drvp->drive_flags |= ATA_DRIVE_WAITDRAIN;
-               (void) tsleep(&chq->active_xfers, PRIBIO, "atdrn", 0);
+               cv_wait(&chq->queue_drain, &chp->ch_lock);
        }
-       splx(s);
+
+       mutex_exit(&chp->ch_lock);
 }
 
 void
@@ -2152,9 +2183,11 @@
 void
 ata_channel_start(struct ata_channel *chp, int drive)
 {
-       int i;
+       int i, s;
        struct ata_drive_datas *drvp;
 
+       s = splbio();
+
        KASSERT(chp->ch_ndrives > 0);
 
 #define ATA_DRIVE_START(chp, drive) \
@@ -2186,5 +2219,6 @@
        /* Now try to kick off xfers on the current drive */
        ATA_DRIVE_START(chp, drive);
 
+       splx(s);
 #undef ATA_DRIVE_START
 }
diff -r 7f334a92c906 -r aee1d3443ccf sys/dev/ata/ata_wdc.c
--- a/sys/dev/ata/ata_wdc.c     Tue Jun 27 18:16:50 2017 +0000
+++ b/sys/dev/ata/ata_wdc.c     Tue Jun 27 18:36:03 2017 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: ata_wdc.c,v 1.105.6.5 2017/06/20 20:58:22 jdolecek Exp $       */
+/*     $NetBSD: ata_wdc.c,v 1.105.6.6 2017/06/27 18:36:03 jdolecek Exp $       */
 
 /*
  * Copyright (c) 1998, 2001, 2003 Manuel Bouyer.
@@ -54,7 +54,7 @@
  */
 
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: ata_wdc.c,v 1.105.6.5 2017/06/20 20:58:22 jdolecek Exp $");
+__KERNEL_RCSID(0, "$NetBSD: ata_wdc.c,v 1.105.6.6 2017/06/27 18:36:03 jdolecek Exp $");
 
 #include "opt_ata.h"
 #include "opt_wdc.h"
@@ -774,11 +774,13 @@
 {
        struct ata_bio *ata_bio = &xfer->c_bio;
        int drive = xfer->c_drive;
-
-       ata_deactivate_xfer(chp, xfer);
+       bool deactivate = true;
 
        ata_bio->flags |= ATA_ITSDONE;
        switch (reason) {
+       case KILL_GONE_INACTIVE:
+               deactivate = false;
+               /* FALLTHROUGH */
        case KILL_GONE:
                ata_bio->error = ERR_NODEV;
                break;
@@ -791,6 +793,10 @@
                panic("wdc_ata_bio_kill_xfer");
        }
        ata_bio->r_error = WDCE_ABRT;
+
+       if (deactivate)
+               ata_deactivate_xfer(chp, xfer);
+
        ATADEBUG_PRINT(("wdc_ata_bio_kill_xfer: drv_done\n"), DEBUG_XFERS);
        (*chp->ch_drive[drive].drv_done)(chp->ch_drive[drive].drv_softc, xfer);
 }
@@ -806,7 +812,8 @@
            xfer->c_drive, (u_int)xfer->c_flags),
            DEBUG_XFERS);
 
-       callout_stop(&xfer->c_timo_callout);
+       if (ata_waitdrain_xfer_check(chp, xfer))
+               return;
 
        /* feed back residual bcount to our caller */
        ata_bio->bcount = xfer->c_bcount;
@@ -814,9 +821,6 @@
        /* mark controller inactive and free xfer */
        ata_deactivate_xfer(chp, xfer);
 
-       if (ata_waitdrain_check(chp, drive)) {
-               ata_bio->error = ERR_NODEV;
-       }
        ata_bio->flags |= ATA_ITSDONE;
        ATADEBUG_PRINT(("wdc_ata_done: drv_done\n"), DEBUG_XFERS);
        (*chp->ch_drive[drive].drv_done)(chp->ch_drive[drive].drv_softc, xfer);
diff -r 7f334a92c906 -r aee1d3443ccf sys/dev/ata/atavar.h
--- a/sys/dev/ata/atavar.h      Tue Jun 27 18:16:50 2017 +0000
+++ b/sys/dev/ata/atavar.h      Tue Jun 27 18:36:03 2017 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: atavar.h,v 1.92.8.15 2017/06/23 20:40:51 jdolecek Exp $        */
+/*     $NetBSD: atavar.h,v 1.92.8.16 2017/06/27 18:36:03 jdolecek Exp $        */
 
 /*
  * Copyright (c) 1998, 2001 Manuel Bouyer.
@@ -121,8 +121,6 @@
        int timeout;            /* timeout (in ms) */
        void *data;             /* Data buffer address */
        int bcount;             /* number of bytes to transfer */
-       void (*callback)(void *); /* command to call once command completed */
-       void *callback_arg;     /* argument passed to *callback() */
 };
 
 /* Forward declaration for ata_xfer */
@@ -181,8 +179,9 @@
 #define        C_NCQ           0x0100          /* command is queued  */
 
 /* reasons for c_kill_xfer() */
-#define KILL_GONE 1 /* device is gone */
-#define KILL_RESET 2 /* xfer was reset */
+#define KILL_GONE 1            /* device is gone while xfer was active */
+#define KILL_RESET 2           /* xfer was reset */
+#define KILL_GONE_INACTIVE 3   /* device is gone while xfer was pending */
 
 /*



Home | Main Index | Thread Index | Old Index