Source-Changes-HG archive

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

[src/trunk]: src/sys/dev PR kern/56403



details:   https://anonhg.NetBSD.org/src/rev/e875a887d862
branches:  trunk
changeset: 988239:e875a887d862
user:      rin <rin%NetBSD.org@localhost>
date:      Tue Oct 05 08:01:05 2021 +0000

description:
PR kern/56403

Fix kernel freeze for wdc(4) variants with ATAC_CAP_NOIRQ:

(1) Change ata_xfer_ops:c_poll from void to int function. When it returns
    ATAPOLL_AGAIN, let ata_xfer_start() iterate itself again.

(2) Let wdc_ata_bio_poll() return ATAPOLL_AGAIN until ATA_ITSDONE is
    achieved.

A similar change has been made for mvsata(4) (see mvsata_bio_poll()),
and no functional changes for other devices.

This is how the drivers worked before jdolecek-ncq branch was merged.

Note that this changes are less likely to cause infinite recursion:

(1) wdc_ata_bio_intr() called from wdc_ata_bio_poll() asserts ATA_ITSDONE
    in its error handling paths via wdc_ata_bio_done().

(2) Return value from c_start (= wdc_ata_bio_start()) is checked in
    ata_xfer_start().

Therefore, errors encountered in ata_xfer_ops:c_poll and c_start routines
terminate the recursion for wdc(4). The situation is similar for mvsata(4).

Still, there is a possibility where ata_xfer_start() takes long time to
finish a normal operation. This can result in a delayed response for lower
priority interrupts. But, I've never observed such a situation, even when
heavy thrashing takes place for swap partition in wd(4).

"Go ahead" by jdolecek@.

diffstat:

 sys/dev/ata/ata.c          |  11 +++++++----
 sys/dev/ata/ata_wdc.c      |  14 +++++++++-----
 sys/dev/ata/atavar.h       |   6 ++++--
 sys/dev/ic/ahcisata_core.c |  20 ++++++++++++--------
 sys/dev/ic/mvsata.c        |  33 +++++++++++++++++++--------------
 sys/dev/ic/siisata.c       |  22 ++++++++++++++--------
 sys/dev/ic/wdc.c           |   9 +++++----
 sys/dev/scsipi/atapi_wdc.c |  12 +++++++-----
 8 files changed, 77 insertions(+), 50 deletions(-)

diffs (truncated from 530 to 300 lines):

diff -r 75538406bebf -r e875a887d862 sys/dev/ata/ata.c
--- a/sys/dev/ata/ata.c Tue Oct 05 07:05:51 2021 +0000
+++ b/sys/dev/ata/ata.c Tue Oct 05 08:01:05 2021 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: ata.c,v 1.163 2021/08/29 23:49:32 rin Exp $    */
+/*     $NetBSD: ata.c,v 1.164 2021/10/05 08:01:05 rin 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.163 2021/08/29 23:49:32 rin Exp $");
+__KERNEL_RCSID(0, "$NetBSD: ata.c,v 1.164 2021/10/05 08:01:05 rin Exp $");
 
 #include "opt_ata.h"
 
@@ -1231,10 +1231,11 @@
 ata_xfer_start(struct ata_xfer *xfer)
 {
        struct ata_channel *chp = xfer->c_chp;
-       int rv;
+       int rv, status;
 
        KASSERT(mutex_owned(&chp->ch_lock));
 
+again:
        rv = xfer->ops->c_start(chp, xfer);
        switch (rv) {
        case ATASTART_STARTED:
@@ -1248,8 +1249,10 @@
                /* can happen even in thread context for some ATAPI devices */
                ata_channel_unlock(chp);
                KASSERT(xfer->ops != NULL && xfer->ops->c_poll != NULL);
-               xfer->ops->c_poll(chp, xfer);
+               status = xfer->ops->c_poll(chp, xfer);
                ata_channel_lock(chp);
+               if (status == ATAPOLL_AGAIN)
+                       goto again;
                break;
        case ATASTART_ABORT:
                ata_channel_unlock(chp);
diff -r 75538406bebf -r e875a887d862 sys/dev/ata/ata_wdc.c
--- a/sys/dev/ata/ata_wdc.c     Tue Oct 05 07:05:51 2021 +0000
+++ b/sys/dev/ata/ata_wdc.c     Tue Oct 05 08:01:05 2021 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: ata_wdc.c,v 1.119 2020/12/25 08:55:40 skrll Exp $      */
+/*     $NetBSD: ata_wdc.c,v 1.120 2021/10/05 08:01:05 rin 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.119 2020/12/25 08:55:40 skrll Exp $");
+__KERNEL_RCSID(0, "$NetBSD: ata_wdc.c,v 1.120 2021/10/05 08:01:05 rin Exp $");
 
 #include "opt_ata.h"
 #include "opt_wdc.h"
@@ -105,7 +105,7 @@
 static void    wdc_ata_bio(struct ata_drive_datas*, struct ata_xfer *);
 static int     wdc_ata_bio_start(struct ata_channel *,struct ata_xfer *);
 static int     _wdc_ata_bio_start(struct ata_channel *,struct ata_xfer *);
-static void    wdc_ata_bio_poll(struct ata_channel *,struct ata_xfer *);
+static int     wdc_ata_bio_poll(struct ata_channel *,struct ata_xfer *);
 static int     wdc_ata_bio_intr(struct ata_channel *, struct ata_xfer *,
                                 int);
 static void    wdc_ata_bio_kill_xfer(struct ata_channel *,
@@ -609,7 +609,7 @@
        return ATASTART_ABORT;
 }
 
-static void
+static int
 wdc_ata_bio_poll(struct ata_channel *chp, struct ata_xfer *xfer)
 {
        /* Wait for at last 400ns for status bit to be valid */
@@ -621,6 +621,7 @@
        }
 #endif
        wdc_ata_bio_intr(chp, xfer, 0);
+       return (xfer->c_bio.flags & ATA_ITSDONE) ? ATAPOLL_DONE : ATAPOLL_AGAIN;
 }
 
 static int
@@ -773,7 +774,10 @@
                        callout_stop(&chp->c_timo_callout);
                        ata_xfer_start(xfer);
                } else {
-                       /* Let _wdc_ata_bio_start do the loop */
+                       /*
+                        * Let ata_xfer_start() do the loop;
+                        * see wdc_ata_bio_poll().
+                        */
                }
                ata_channel_unlock(chp);
                return 1;
diff -r 75538406bebf -r e875a887d862 sys/dev/ata/atavar.h
--- a/sys/dev/ata/atavar.h      Tue Oct 05 07:05:51 2021 +0000
+++ b/sys/dev/ata/atavar.h      Tue Oct 05 08:01:05 2021 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: atavar.h,v 1.108 2020/05/25 18:29:25 jdolecek Exp $    */
+/*     $NetBSD: atavar.h,v 1.109 2021/10/05 08:01:05 rin Exp $ */
 
 /*
  * Copyright (c) 1998, 2001 Manuel Bouyer.
@@ -178,7 +178,9 @@
 #define ATASTART_TH            1       /* xfer needs to be run in thread */
 #define ATASTART_POLL          2       /* xfer needs to be polled */
 #define ATASTART_ABORT         3       /* error occurred, abort xfer */
-       void    (*c_poll)(struct ata_channel *, struct ata_xfer *);
+       int     (*c_poll)(struct ata_channel *, struct ata_xfer *);
+#define        ATAPOLL_DONE            0
+#define        ATAPOLL_AGAIN           1
        void    (*c_abort)(struct ata_channel *, struct ata_xfer *);
        int     (*c_intr)(struct ata_channel *, struct ata_xfer *, int);
        void    (*c_kill_xfer)(struct ata_channel *, struct ata_xfer *, int);
diff -r 75538406bebf -r e875a887d862 sys/dev/ic/ahcisata_core.c
--- a/sys/dev/ic/ahcisata_core.c        Tue Oct 05 07:05:51 2021 +0000
+++ b/sys/dev/ic/ahcisata_core.c        Tue Oct 05 08:01:05 2021 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: ahcisata_core.c,v 1.101 2021/09/03 01:23:33 mrg Exp $  */
+/*     $NetBSD: ahcisata_core.c,v 1.102 2021/10/05 08:01:05 rin Exp $  */
 
 /*
  * Copyright (c) 2006 Manuel Bouyer.
@@ -26,7 +26,7 @@
  */
 
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: ahcisata_core.c,v 1.101 2021/09/03 01:23:33 mrg Exp $");
+__KERNEL_RCSID(0, "$NetBSD: ahcisata_core.c,v 1.102 2021/10/05 08:01:05 rin Exp $");
 
 #include <sys/types.h>
 #include <sys/malloc.h>
@@ -69,13 +69,13 @@
 
 static int  ahci_cmd_start(struct ata_channel *, struct ata_xfer *);
 static int  ahci_cmd_complete(struct ata_channel *, struct ata_xfer *, int);
-static void ahci_cmd_poll(struct ata_channel *, struct ata_xfer *);
+static int  ahci_cmd_poll(struct ata_channel *, struct ata_xfer *);
 static void ahci_cmd_abort(struct ata_channel *, struct ata_xfer *);
 static void ahci_cmd_done(struct ata_channel *, struct ata_xfer *);
 static void ahci_cmd_done_end(struct ata_channel *, struct ata_xfer *);
 static void ahci_cmd_kill_xfer(struct ata_channel *, struct ata_xfer *, int);
 static int  ahci_bio_start(struct ata_channel *, struct ata_xfer *);
-static void ahci_bio_poll(struct ata_channel *, struct ata_xfer *);
+static int  ahci_bio_poll(struct ata_channel *, struct ata_xfer *);
 static void ahci_bio_abort(struct ata_channel *, struct ata_xfer *);
 static int  ahci_bio_complete(struct ata_channel *, struct ata_xfer *, int);
 static void ahci_bio_kill_xfer(struct ata_channel *, struct ata_xfer *, int) ;
@@ -93,7 +93,7 @@
 static void ahci_atapi_scsipi_request(struct scsipi_channel *,
     scsipi_adapter_req_t, void *);
 static int  ahci_atapi_start(struct ata_channel *, struct ata_xfer *);
-static void ahci_atapi_poll(struct ata_channel *, struct ata_xfer *);
+static int  ahci_atapi_poll(struct ata_channel *, struct ata_xfer *);
 static void ahci_atapi_abort(struct ata_channel *, struct ata_xfer *);
 static int  ahci_atapi_complete(struct ata_channel *, struct ata_xfer *, int);
 static void ahci_atapi_kill_xfer(struct ata_channel *, struct ata_xfer *, int);
@@ -1208,7 +1208,7 @@
                return ATASTART_POLL;
 }
 
-static void
+static int
 ahci_cmd_poll(struct ata_channel *chp, struct ata_xfer *xfer)
 {
        struct ahci_softc *sc = AHCI_CH2SC(chp);
@@ -1245,6 +1245,8 @@
        }
        /* reenable interrupts */
        AHCI_WRITE(sc, AHCI_GHC, AHCI_READ(sc, AHCI_GHC) | AHCI_GHC_IE);
+
+       return ATAPOLL_DONE;
 }
 
 static void
@@ -1456,7 +1458,7 @@
                return ATASTART_POLL;
 }
 
-static void
+static int
 ahci_bio_poll(struct ata_channel *chp, struct ata_xfer *xfer)
 {
        struct ahci_softc *sc = (struct ahci_softc *)chp->ch_atac;
@@ -1486,6 +1488,7 @@
        }
        /* reenable interrupts */
        AHCI_WRITE(sc, AHCI_GHC, AHCI_READ(sc, AHCI_GHC) | AHCI_GHC_IE);
+       return ATAPOLL_DONE;
 }
 
 static void
@@ -1953,7 +1956,7 @@
                return ATASTART_POLL;
 }
 
-static void
+static int
 ahci_atapi_poll(struct ata_channel *chp, struct ata_xfer *xfer)
 {
        struct ahci_softc *sc = (struct ahci_softc *)chp->ch_atac;
@@ -1983,6 +1986,7 @@
        }
        /* reenable interrupts */
        AHCI_WRITE(sc, AHCI_GHC, AHCI_READ(sc, AHCI_GHC) | AHCI_GHC_IE);
+       return ATAPOLL_DONE;
 }
 
 static void
diff -r 75538406bebf -r e875a887d862 sys/dev/ic/mvsata.c
--- a/sys/dev/ic/mvsata.c       Tue Oct 05 07:05:51 2021 +0000
+++ b/sys/dev/ic/mvsata.c       Tue Oct 05 08:01:05 2021 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: mvsata.c,v 1.60 2021/08/07 16:19:12 thorpej Exp $      */
+/*     $NetBSD: mvsata.c,v 1.61 2021/10/05 08:01:05 rin 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.60 2021/08/07 16:19:12 thorpej Exp $");
+__KERNEL_RCSID(0, "$NetBSD: mvsata.c,v 1.61 2021/10/05 08:01:05 rin Exp $");
 
 #include "opt_mvsata.h"
 
@@ -143,14 +143,14 @@
 #ifndef MVSATA_WITHOUTDMA
 static int mvsata_bio_start(struct ata_channel *, struct ata_xfer *);
 static int mvsata_bio_intr(struct ata_channel *, struct ata_xfer *, int);
-static void mvsata_bio_poll(struct ata_channel *, struct ata_xfer *);
+static int mvsata_bio_poll(struct ata_channel *, struct ata_xfer *);
 static void mvsata_bio_kill_xfer(struct ata_channel *, struct ata_xfer *, int);
 static void mvsata_bio_done(struct ata_channel *, struct ata_xfer *);
 static int mvsata_bio_ready(struct mvsata_port *, struct ata_bio *, int,
                            int);
 static int mvsata_wdc_cmd_start(struct ata_channel *, struct ata_xfer *);
 static int mvsata_wdc_cmd_intr(struct ata_channel *, struct ata_xfer *, int);
-static void mvsata_wdc_cmd_poll(struct ata_channel *, struct ata_xfer *);
+static int mvsata_wdc_cmd_poll(struct ata_channel *, struct ata_xfer *);
 static void mvsata_wdc_cmd_kill_xfer(struct ata_channel *, struct ata_xfer *,
                                     int);
 static void mvsata_wdc_cmd_done(struct ata_channel *, struct ata_xfer *);
@@ -158,7 +158,7 @@
 #if NATAPIBUS > 0
 static int mvsata_atapi_start(struct ata_channel *, struct ata_xfer *);
 static int mvsata_atapi_intr(struct ata_channel *, struct ata_xfer *, int);
-static void mvsata_atapi_poll(struct ata_channel *, struct ata_xfer *);
+static int mvsata_atapi_poll(struct ata_channel *, struct ata_xfer *);
 static void mvsata_atapi_kill_xfer(struct ata_channel *, struct ata_xfer *,
                                   int);
 static void mvsata_atapi_reset(struct ata_channel *, struct ata_xfer *);
@@ -1245,7 +1245,7 @@
        return ATASTART_ABORT;
 }
 
-static void
+static int
 mvsata_bio_poll(struct ata_channel *chp, struct ata_xfer *xfer)
 {
        struct mvsata_port *mvport = (struct mvsata_port *)chp;
@@ -1259,10 +1259,9 @@
                chp->ch_flags &= ~ATACH_DMA_WAIT;
        }
 
-       if ((xfer->c_bio.flags & ATA_ITSDONE) == 0) {
-               KASSERT(xfer->c_flags & C_TIMEOU);
-               mvsata_bio_intr(chp, xfer, 0);
-       }
+       mvsata_bio_intr(chp, xfer, 0);
+
+       return (xfer->c_bio.flags & ATA_ITSDONE) ? ATAPOLL_DONE : ATAPOLL_AGAIN;
 }
 
 static int
@@ -1385,7 +1384,10 @@
                        /* Start the next operation */
                        ata_xfer_start(xfer);
                } else {
-                       /* Let mvsata_bio_start do the loop */
+                       /*
+                        * Let ata_xfer_start() do the loop;
+                        * see mvsata_bio_poll().
+                        */
                }
                ata_channel_unlock(chp);
        } else { /* Done with this transfer */
@@ -1680,7 +1682,7 @@
        return ATASTART_POLL;
 }
 
-static void
+static int
 mvsata_wdc_cmd_poll(struct ata_channel *chp, struct ata_xfer *xfer)



Home | Main Index | Thread Index | Old Index