Source-Changes-HG archive

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

[src/trunk]: src/sys/dev fix deadlock in wdcwait() when xfer timeout happens ...



details:   https://anonhg.NetBSD.org/src/rev/d27a623bebdc
branches:  trunk
changeset: 850472:d27a623bebdc
user:      jdolecek <jdolecek%NetBSD.org@localhost>
date:      Sat Apr 04 21:36:15 2020 +0000

description:
fix deadlock in wdcwait() when xfer timeout happens while the atabus
thread sleeps in wdcwait() - check current lwp rather than relying
on global ATACH_TH_RUN channel flag

should fix the hang part of the problem reported in
http://mail-index.netbsd.org/netbsd-users/2020/03/12/msg024249.html

thanks to Paul Ripke for providing extensive debugging info

diffstat:

 sys/dev/ata/ata.c          |  29 +++++++++++++++--------------
 sys/dev/ata/ata_wdc.c      |   9 ++++-----
 sys/dev/ata/atavar.h       |   4 ++--
 sys/dev/ic/mvsata.c        |  16 ++++++++--------
 sys/dev/ic/wdc.c           |   7 +++----
 sys/dev/scsipi/atapi_wdc.c |   9 ++++-----
 6 files changed, 36 insertions(+), 38 deletions(-)

diffs (261 lines):

diff -r 9f3ce832c7cb -r d27a623bebdc sys/dev/ata/ata.c
--- a/sys/dev/ata/ata.c Sat Apr 04 21:29:54 2020 +0000
+++ b/sys/dev/ata/ata.c Sat Apr 04 21:36:15 2020 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: ata.c,v 1.153 2019/10/21 18:58:57 christos Exp $       */
+/*     $NetBSD: ata.c,v 1.154 2020/04/04 21:36: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.153 2019/10/21 18:58:57 christos Exp $");
+__KERNEL_RCSID(0, "$NetBSD: ata.c,v 1.154 2020/04/04 21:36:15 jdolecek Exp $");
 
 #include "opt_ata.h"
 
@@ -44,6 +44,7 @@
 #include <sys/bus.h>
 #include <sys/once.h>
 #include <sys/bitops.h>
+#include <sys/cpu.h>
 
 #define ATABUS_PRIVATE
 
@@ -229,9 +230,6 @@
        int i, error;
 
        /* we are in the atabus's thread context */
-       ata_channel_lock(chp);
-       chp->ch_flags |= ATACH_TH_RUN;
-       ata_channel_unlock(chp);
 
        /*
         * Probe for the drives attached to controller, unless a PMP
@@ -247,11 +245,6 @@
                    DEBUG_PROBE);
        }
 
-       /* next operations will occurs in a separate thread */
-       ata_channel_lock(chp);
-       chp->ch_flags &= ~ATACH_TH_RUN;
-       ata_channel_unlock(chp);
-
        /* Make sure the devices probe in atabus order to avoid jitter. */
        mutex_enter(&atabus_qlock);
        for (;;) {
@@ -264,6 +257,8 @@
 
        ata_channel_lock(chp);
 
+       KASSERT(ata_is_thread_run(chp));
+
        /* If no drives, abort here */
        if (chp->ch_drive == NULL)
                goto out;
@@ -444,7 +439,7 @@
        int i, rv;
 
        ata_channel_lock(chp);
-       chp->ch_flags |= ATACH_TH_RUN;
+       KASSERT(ata_is_thread_run(chp));
 
        /*
         * Probe the drives.  Reset type to indicate to controllers
@@ -466,9 +461,7 @@
                if ((chp->ch_flags & (ATACH_TH_RESET | ATACH_TH_DRIVE_RESET
                    | ATACH_TH_RECOVERY | ATACH_SHUTDOWN)) == 0 &&
                    (chq->queue_active == 0 || chq->queue_freeze == 0)) {
-                       chp->ch_flags &= ~ATACH_TH_RUN;
                        cv_wait(&chp->ch_thr_idle, &chp->ch_lock);
-                       chp->ch_flags |= ATACH_TH_RUN;
                }
                if (chp->ch_flags & ATACH_SHUTDOWN) {
                        break;
@@ -547,6 +540,14 @@
        kthread_exit(0);
 }
 
+bool
+ata_is_thread_run(struct ata_channel *chp)
+{
+       KASSERT(mutex_owned(&chp->ch_lock));
+
+       return (chp->ch_thread == curlwp && !cpu_intr_p());
+}
+
 static void
 ata_thread_wake_locked(struct ata_channel *chp)
 {
@@ -1896,7 +1897,7 @@
                         */
                        if (atac->atac_set_modes)
                                /*
-                                * It's OK to pool here, it's fast enough
+                                * It's OK to poll here, it's fast enough
                                 * to not bother waiting for interrupt
                                 */
                                if (ata_set_mode(drvp, 0x08 | (i + 3),
diff -r 9f3ce832c7cb -r d27a623bebdc sys/dev/ata/ata_wdc.c
--- a/sys/dev/ata/ata_wdc.c     Sat Apr 04 21:29:54 2020 +0000
+++ b/sys/dev/ata/ata_wdc.c     Sat Apr 04 21:36:15 2020 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: ata_wdc.c,v 1.113 2018/11/12 18:51:01 jdolecek Exp $   */
+/*     $NetBSD: ata_wdc.c,v 1.114 2020/04/04 21:36:15 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.113 2018/11/12 18:51:01 jdolecek Exp $");
+__KERNEL_RCSID(0, "$NetBSD: ata_wdc.c,v 1.114 2020/04/04 21:36:15 jdolecek Exp $");
 
 #include "opt_ata.h"
 #include "opt_wdc.h"
@@ -206,10 +206,9 @@
                 * that we never get to this point if that's the case.
                 */
                /* If it's not a polled command, we need the kernel thread */
-               if ((xfer->c_flags & C_POLL) == 0 &&
-                   (chp->ch_flags & ATACH_TH_RUN) == 0) {
+               if ((xfer->c_flags & C_POLL) == 0 && !ata_is_thread_run(chp))
                        return ATASTART_TH;
-               }
+
                /*
                 * disable interrupts, all commands here should be quick
                 * enough to be able to poll, and we don't go here that often
diff -r 9f3ce832c7cb -r d27a623bebdc sys/dev/ata/atavar.h
--- a/sys/dev/ata/atavar.h      Sat Apr 04 21:29:54 2020 +0000
+++ b/sys/dev/ata/atavar.h      Sat Apr 04 21:36:15 2020 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: atavar.h,v 1.103 2019/04/05 21:31:44 bouyer Exp $      */
+/*     $NetBSD: atavar.h,v 1.104 2020/04/04 21:36:15 jdolecek Exp $    */
 
 /*
  * Copyright (c) 1998, 2001 Manuel Bouyer.
@@ -406,7 +406,6 @@
 #define ATACH_DMA_WAIT 0x20    /* controller is waiting for DMA */
 #define ATACH_PIOBM_WAIT 0x40  /* controller is waiting for busmastering PIO */
 #define        ATACH_DISABLED 0x80     /* channel is disabled */
-#define ATACH_TH_RUN   0x100   /* the kernel thread is working */
 #define ATACH_TH_RESET 0x200   /* someone ask the thread to reset */
 #define ATACH_TH_RESCAN 0x400  /* rescan requested */
 #define ATACH_NCQ      0x800   /* channel executing NCQ commands */
@@ -549,6 +548,7 @@
 void   ata_kill_pending(struct ata_drive_datas *);
 void   ata_kill_active(struct ata_channel *, int, int);
 void   ata_thread_run(struct ata_channel *, int, int, int);
+bool   ata_is_thread_run(struct ata_channel *);
 void   ata_channel_freeze(struct ata_channel *);
 void   ata_channel_thaw_locked(struct ata_channel *);
 void   ata_channel_lock(struct ata_channel *);
diff -r 9f3ce832c7cb -r d27a623bebdc sys/dev/ic/mvsata.c
--- a/sys/dev/ic/mvsata.c       Sat Apr 04 21:29:54 2020 +0000
+++ b/sys/dev/ic/mvsata.c       Sat Apr 04 21:36:15 2020 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: mvsata.c,v 1.54 2020/03/22 16:46:30 macallan Exp $     */
+/*     $NetBSD: mvsata.c,v 1.55 2020/04/04 21:36:15 jdolecek Exp $     */
 /*
  * Copyright (c) 2008 KIYOHARA Takashi
  * All rights reserved.
@@ -26,7 +26,7 @@
  */
 
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: mvsata.c,v 1.54 2020/03/22 16:46:30 macallan Exp $");
+__KERNEL_RCSID(0, "$NetBSD: mvsata.c,v 1.55 2020/04/04 21:36:15 jdolecek Exp $");
 
 #include "opt_mvsata.h"
 
@@ -1165,10 +1165,10 @@
                         * If it's not a polled command, we need the kernel
                         * thread
                         */
-                       if ((xfer->c_flags & C_POLL) == 0 &&
-                           (chp->ch_flags & ATACH_TH_RUN) == 0) {
+                       if ((xfer->c_flags & C_POLL) == 0
+                           && !ata_is_thread_run(chp))
                                return ATASTART_TH;
-                       }
+
                        if (mvsata_bio_ready(mvport, ata_bio, xfer->c_drive,
                            (xfer->c_flags & C_POLL) ? AT_POLL : 0) != 0) {
                                return ATASTART_ABORT;
@@ -2052,10 +2052,10 @@
        /* Do control operations specially. */
        if (__predict_false(drvp->state < READY)) {
                /* If it's not a polled command, we need the kernel thread */
-               if ((sc_xfer->xs_control & XS_CTL_POLL) == 0 &&
-                   (chp->ch_flags & ATACH_TH_RUN) == 0) {
+               if ((sc_xfer->xs_control & XS_CTL_POLL) == 0
+                   && !ata_is_thread_run(chp))
                        return ATASTART_TH;
-               }
+
                /*
                 * disable interrupts, all commands here should be quick
                 * enough to be able to poll, and we don't go here that often
diff -r 9f3ce832c7cb -r d27a623bebdc sys/dev/ic/wdc.c
--- a/sys/dev/ic/wdc.c  Sat Apr 04 21:29:54 2020 +0000
+++ b/sys/dev/ic/wdc.c  Sat Apr 04 21:36:15 2020 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: wdc.c,v 1.297 2020/02/10 20:11:48 jdolecek Exp $ */
+/*     $NetBSD: wdc.c,v 1.298 2020/04/04 21:36:15 jdolecek Exp $ */
 
 /*
  * Copyright (c) 1998, 2001, 2003 Manuel Bouyer.  All rights reserved.
@@ -58,7 +58,7 @@
  */
 
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: wdc.c,v 1.297 2020/02/10 20:11:48 jdolecek Exp $");
+__KERNEL_RCSID(0, "$NetBSD: wdc.c,v 1.298 2020/04/04 21:36:15 jdolecek Exp $");
 
 #include "opt_ata.h"
 #include "opt_wdc.h"
@@ -1278,8 +1278,7 @@
        else {
                error = __wdcwait(chp, mask, bits, WDCDELAY_POLL, tfd);
                if (error != 0) {
-                       if ((chp->ch_flags & ATACH_TH_RUN) ||
-                           (flags & AT_WAIT)) {
+                       if (ata_is_thread_run(chp) || (flags & AT_WAIT)) {
                                /*
                                 * we're running in the channel thread
                                 * or some userland thread context
diff -r 9f3ce832c7cb -r d27a623bebdc sys/dev/scsipi/atapi_wdc.c
--- a/sys/dev/scsipi/atapi_wdc.c        Sat Apr 04 21:29:54 2020 +0000
+++ b/sys/dev/scsipi/atapi_wdc.c        Sat Apr 04 21:36:15 2020 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: atapi_wdc.c,v 1.136 2020/02/19 16:04:39 riastradh Exp $        */
+/*     $NetBSD: atapi_wdc.c,v 1.137 2020/04/04 21:36:15 jdolecek Exp $ */
 
 /*
  * Copyright (c) 1998, 2001 Manuel Bouyer.
@@ -25,7 +25,7 @@
  */
 
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: atapi_wdc.c,v 1.136 2020/02/19 16:04:39 riastradh Exp $");
+__KERNEL_RCSID(0, "$NetBSD: atapi_wdc.c,v 1.137 2020/04/04 21:36:15 jdolecek Exp $");
 
 #ifndef ATADEBUG
 #define ATADEBUG
@@ -501,10 +501,9 @@
        /* Do control operations specially. */
        if (__predict_false(drvp->state < READY)) {
                /* If it's not a polled command, we need the kernel thread */
-               if ((sc_xfer->xs_control & XS_CTL_POLL) == 0 &&
-                   (chp->ch_flags & ATACH_TH_RUN) == 0) {
+               if ((sc_xfer->xs_control & XS_CTL_POLL) == 0
+                   && !ata_is_thread_run(chp))
                        return ATASTART_TH;
-               }
                /*
                 * disable interrupts, all commands here should be quick
                 * enough to be able to poll, and we don't go here that often



Home | Main Index | Thread Index | Old Index