wd33c93 cleanup patch set
Adrian Chadd <[email protected]> Mon, 1 Dec 2025 08:47:10 -0800
| Newsgroups | gmane.os.netbsd.ports.mips.devel |
|---|---|
| Message-ID | <CAJ-Vmom8M5oCDWQ3QiDpmUc_e5RZMaL-GR8WA7tYr-Mu5tELpA@mail.gmail.com> |
hi! I have four patches I'd like to be reviewed and considered for inclusion. There's one bug and some clean-ups based on what I'm doing in my local branch. * patch-1 fixes a panic if the timed out command isn't currently active. This can happen if the device disconnects mid-transfer and the controller is then idle (or active on another transfer to another device.) * patch-2 adds a string parameter to wd33c93_error() so the caller can specify what the SCSI error was. * patch-3 adds a new debug flag to log submitted SCSI requests to the driver with some details about what the underlying request is (which i found useful for debugging transfer vs sync/async setup stuff.) * patch-4 splits wd33c93_timeout() into wd33c93_timeout() and wd33c93_timeout_callback(). It's being used both as a timeout and as a callback, and whilst tinkering with error handling locally I found it was useful to have them split so I could do stuff like abort/complete the transfer if it's a timeout as there's no subsequent code in that path that would actually complete the transfer. I still have an outstanding question on the kern list about SCSI timeouts and where they're supposed to be handled - for patch-4, I don't think the scsipi layer is running its own timeouts on transfers - it's asking the driver to do it - and if the driver hits the timeout but never completes the transfer it will just be hung/lost for good. Thanks! -adrian
20251201-netbsd-wd33c91-patch-4.diff
(application/octet-stream, 3 KB)
commit c441630abdda9541f3408e02809e10470c1a9841 Author: Adrian Chadd <[email protected]> Date: Sun Nov 23 17:22:21 2025 -0800 wd33c93: split wd33c93_timeout() into timeout and callback The code actually directly calls the same function for both processing a timeout (and thus sending an abort, bus reset, etc) and the callout running. However, the timeout code doesn't actually complete an acb. So if an acb times out, it seems to just .. be timed out. Forever. So in preparation for adding support for completing an acb with timeout, split wd33c93_timeout() into timeout and callout. This should be a no-op. diff --git a/sys/dev/ic/wd33c93.c b/sys/dev/ic/wd33c93.c index dc27f43df92f..fc3024299fdf 100644 --- a/sys/dev/ic/wd33c93.c +++ b/sys/dev/ic/wd33c93.c @@ -141,7 +141,8 @@ void wd33c93_msgin (struct wd33c93_softc *, u_char *, int); void wd33c93_reselect (struct wd33c93_softc *, int, int, int, int); void wd33c93_sched_msgout (struct wd33c93_softc *, u_short); void wd33c93_msgout (struct wd33c93_softc *); -void wd33c93_timeout (void *arg); +void wd33c93_timeout_callout (void *arg); +int wd33c93_timeout (struct wd33c93_acb *); void wd33c93_watchdog (void *arg); u_char wd33c93_stp2syn (struct wd33c93_softc *, struct wd33c93_tinfo *); void wd33c93_setsync (struct wd33c93_softc *, struct wd33c93_tinfo *); @@ -926,7 +927,7 @@ wd33c93_abort(struct wd33c93_softc *sc, struct wd33c93_acb *acb, if (sc->sc_nexus == acb) { /* Reschedule timeout. */ callout_reset(&acb->xs->xs_callout, mstohz(acb->timeout), - wd33c93_timeout, acb); + wd33c93_timeout_callout, acb); while (asr & SBIC_ASR_DBR) { /* @@ -1008,7 +1009,7 @@ wd33c93_selectbus(struct wd33c93_softc *sc, struct wd33c93_acb *acb) if ((xs->xs_control & XS_CTL_POLL) == 0) callout_reset(&xs->xs_callout, mstohz(acb->timeout), - wd33c93_timeout, acb); + wd33c93_timeout_callout, acb); /* * issue select @@ -2285,14 +2286,33 @@ wd33c93_update_xfer_mode(struct wd33c93_softc *sc, int target) } void -wd33c93_timeout(void *arg) +wd33c93_timeout_callout(void *arg) { struct wd33c93_acb *acb = arg; + int x; + + x = splbio(); + wd33c93_timeout(acb); + splx(x); +} + +/** + * @brief Handle timeout events. + * + * Note: this can be run from outside splbio(), it will acquire it as needed. + * + * Returns 1 if the transfer was aborted and the caller should + * reschedule or complete the acb, 0 otherwise. + */ +int +wd33c93_timeout(struct wd33c93_acb *acb) +{ struct scsipi_xfer *xs = acb->xs; struct scsipi_periph *periph = xs->xs_periph; struct wd33c93_softc *sc = device_private(periph->periph_channel->chan_adapter->adapt_dev); int s, asr; + int ret; s = splbio(); @@ -2308,10 +2328,14 @@ wd33c93_timeout(void *arg) if (asr & SBIC_ASR_INT) { /* We need to service a missed IRQ */ wd33c93_intr(sc); + ret = 0; } else { (void) wd33c93_abort(sc, acb, "timeout"); + ret = 1; } splx(s); + + return (ret); }
20251201-netbsd-wd33c91-patch-1.diff
(application/octet-stream, 825 B)
commit fcf2e32a13ef6ea6f5248da8ed98240e8e2cdb87 Author: Adrian Chadd <[email protected]> Date: Fri Nov 21 13:06:36 2025 -0800 wd33c93: abort the given transfer, not what's on or not currently active The timeout is per-transfer, not global. And if this happens after a state changing event - eg the device disconnecting - then sc->sc_nexus is NULL. Calling wd33c93_abort() with a NULL acb leads to a panic. diff --git a/sys/dev/ic/wd33c93.c b/sys/dev/ic/wd33c93.c index 4f7207c509d9..cdbffe5a56a4 100644 --- a/sys/dev/ic/wd33c93.c +++ b/sys/dev/ic/wd33c93.c @@ -2299,7 +2299,7 @@ wd33c93_timeout(void *arg) /* We need to service a missed IRQ */ wd33c93_intr(sc); } else { - (void) wd33c93_abort(sc, sc->sc_nexus, "timeout"); + (void) wd33c93_abort(sc, acb, "timeout"); } splx(s); }
20251201-netbsd-wd33c91-patch-3.diff
(application/octet-stream, 2.4 KB)
commit 1857fc23c8880322dbcd6403d75556140db965bc Author: Adrian Chadd <[email protected]> Date: Sun Nov 23 11:52:27 2025 -0800 wd33c93: add a debug section for submitted SCSI requests from scsipi Whilst debugging things like hangs I found it useful to know the class of scsi request coming in from the upper layer. * Add a new debug bit entry - SCSIREQ * Use it to echo the submitted SCSI requests * Note that the unhandled one will just .. never be serviced, which is problematic and should be addressed in a subsequent commit. diff --git a/sys/dev/ic/wd33c93.c b/sys/dev/ic/wd33c93.c index 822019733dbc..dc27f43df92f 100644 --- a/sys/dev/ic/wd33c93.c +++ b/sys/dev/ic/wd33c93.c @@ -557,7 +557,7 @@ wd33c93_scsi_request(struct scsipi_channel *chan, scsipi_adapter_req_t req, void struct wd33c93_acb *acb; int flags, s; - SBIC_DEBUG(MISC, ("wd33c93_scsi_request: req 0x%x\n", (int)req)); + SBIC_DEBUG(SCSIREQ, ("%s: req 0x%x\n", __func__, (int)req)); switch (req) { case ADAPTER_REQ_RUN_XFER: @@ -583,6 +583,9 @@ wd33c93_scsi_request(struct scsipi_channel *chan, scsipi_adapter_req_t req, void return; } + SBIC_DEBUG(SCSIREQ, + ("wd33c93_scsi_request: RUN_XFER: acb %p\n", acb)); + acb->flags = ACB_ACTIVE; acb->xs = xs; acb->clen = xs->cmdlen; @@ -620,7 +623,10 @@ wd33c93_scsi_request(struct scsipi_channel *chan, scsipi_adapter_req_t req, void return; case ADAPTER_REQ_GROW_RESOURCES: + SBIC_DEBUG(SCSIREQ, ("%s: GROW_RESOURCES, UNSUPPORTED\n", + __func__)); /* XXX Not supported. */ + /* XXX TODO: should fail the request */ return; case ADAPTER_REQ_SET_XFER_MODE: @@ -631,6 +637,8 @@ wd33c93_scsi_request(struct scsipi_channel *chan, scsipi_adapter_req_t req, void ti = &sc->sc_tinfo[xm->xm_target]; ti->flags &= ~T_WANTSYNC; + SBIC_DEBUG(SCSIREQ, ("%s: REQ_SET_XFER_MODE\n", __func__)); + if ((CFFLAGS_NOTAGS(sc->sc_cfflags, xm->xm_target) == 0) && (xm->xm_mode & PERIPH_CAP_TQING) && !wd33c93_notags) ti->flags |= T_TAG; diff --git a/sys/dev/ic/wd33c93var.h b/sys/dev/ic/wd33c93var.h index 20076fb3df65..fa559935c2fd 100644 --- a/sys/dev/ic/wd33c93var.h +++ b/sys/dev/ic/wd33c93var.h @@ -249,6 +249,7 @@ struct wd33c93_softc { #define DEBUG_MSGS 0x200 #define DEBUG_TAGS 0x400 #define DEBUG_SYNC 0x800 +#define DEBUG_SCSIREQ 0x1000 #ifdef DEBUG extern int wd33c93_debug;
20251201-netbsd-wd33c91-patch-2.diff
(application/octet-stream, 2 KB)
commit f8939b897b8acae728f87e89701791ef68f99799 Author: Adrian Chadd <[email protected]> Date: Fri Nov 21 14:39:57 2025 -0800 wd33c93: extend wd33c93_error() to include a reason Add in a string argument to allow the caller to describe the error. diff --git a/sys/dev/ic/wd33c93.c b/sys/dev/ic/wd33c93.c index cdbffe5a56a4..822019733dbc 100644 --- a/sys/dev/ic/wd33c93.c +++ b/sys/dev/ic/wd33c93.c @@ -129,7 +129,8 @@ int wd33c93_nextstate (struct wd33c93_softc *, struct wd33c93_acb *, int wd33c93_abort (struct wd33c93_softc *, struct wd33c93_acb *, const char *); void wd33c93_xferdone (struct wd33c93_softc *); -void wd33c93_error (struct wd33c93_softc *, struct wd33c93_acb *); +void wd33c93_error (struct wd33c93_softc *, struct wd33c93_acb *, + const char *); void wd33c93_scsidone (struct wd33c93_softc *, struct wd33c93_acb *, int); void wd33c93_sched (struct wd33c93_softc *); void wd33c93_dequeue (struct wd33c93_softc *, struct wd33c93_acb *); @@ -394,7 +395,8 @@ wd33c93_reset(struct wd33c93_softc *sc) } void -wd33c93_error(struct wd33c93_softc *sc, struct wd33c93_acb *acb) +wd33c93_error(struct wd33c93_softc *sc, struct wd33c93_acb *acb, + const char *str) { struct scsipi_xfer *xs = acb->xs; @@ -404,7 +406,7 @@ wd33c93_error(struct wd33c93_softc *sc, struct wd33c93_acb *acb) return; scsipi_printaddr(xs->xs_periph); - printf("SCSI Error\n"); + printf("SCSI Error (%s)\n", str); } /* @@ -1298,7 +1300,7 @@ wd33c93_xferdone(struct wd33c93_softc *sc) if (phase == 0x60) GET_SBIC_tlun(sc, sc->sc_status); else - wd33c93_error(sc, sc->sc_nexus); + wd33c93_error(sc, sc->sc_nexus, "unexpected phase"); QPRINTF(("=STS:%02x=\n", sc->sc_status)); splx(s); @@ -2150,7 +2152,7 @@ wd33c93_nextstate(struct wd33c93_softc *sc, struct wd33c93_acb *acb, u_char csr, SET_SBIC_control(sc, SBIC_CTL_EDI | SBIC_CTL_IDI); if (acb->xs) - wd33c93_error(sc, acb); + wd33c93_error(sc, acb, "unexpected/abort"); wd33c93_abort(sc, acb, "next"); if (sc->sc_flags & SBICF_INDMA) {