Re: kern/60332: system crashed when ^C on midiplay hung and i turned the synth off

[Taylor R Campbell <riastradh%NetBSD.org@localhost>]

Looks like a combination of two issues:

1. Various paths set sc->pbus but don't obviously cv_broadcast
   sc->wchan.

2. midi detach path is all kinds of bogus.

First one should be easy to fix (if it is wrong to cv_broadcast -- or
at least cv_signal -- then it is wrong to set sc->pbus = 0).

Second one I started to work on a few years ago but didn't get around
to testing, and comments like this one make me suspect a bunch more
work is needed (along the lines of
https://www.NetBSD.org/gallery/presentations/riastradh/eurobsdcon2022/opendetach.pdf).

diff -r 19f84c00a20a sys/dev/midi.c
--- a/sys/dev/midi.c	Tue Jun 02 22:13:00 2026 +0000
+++ b/sys/dev/midi.c	Mon Jun 15 00:36:11 2026 +0000
@@ -1074,6 +1074,7 @@ midi_xmt_asense(void *arg)
 	}
 	if (!armed) {
 		sc->pbus = 0;
+		cv_broadcast(&sc->wchan);
 		callout_schedule(&sc->xmt_asense_co, MIDI_XMT_ASENSE_PERIOD);
 	}
 	mutex_exit(sc->lock);
@@ -1240,6 +1241,7 @@ midi_poll_out(struct midi_softc *sc)
 		MIDI_BUF_CONSUMER_WBACK(mb,buf);
 	}
 	sc->pbus = 0;
+	cv_broadcast(&sc->wchan);
 	callout_schedule(&sc->xmt_asense_co, MIDI_XMT_ASENSE_PERIOD);
 	return error;
 }

>From e9452840b638680300b876bb31223f432bbf873e Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
Date: Sun, 26 Dec 2021 12:17:30 +0000
Subject: [PATCH 1/4] midi(4): Force devices to close before waiting for refcnt
 to drain.

Might even render the refcnt unnecessary, not sure yet.
---
 sys/dev/midi.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/sys/dev/midi.c b/sys/dev/midi.c
index 6710c3a6dcca..6e644f23c6ec 100644
--- a/sys/dev/midi.c
+++ b/sys/dev/midi.c
@@ -208,14 +208,6 @@ mididetach(device_t self, int flags)
 	sc->dying = 1;
 	cv_broadcast(&sc->wchan);
 	cv_broadcast(&sc->rchan);
-
-	if (--sc->refcnt >= 0) {
-		(void)cv_timedwait(&sc->detach_cv, sc->lock, hz * 60);
-		if (sc->refcnt >= 0) {
-			aprint_error_dev(self, "refcnt failed to drain,"
-			    " bashing my brains out anyway\n");
-		}
-	}
 	mutex_exit(sc->lock);
 
 	/* locate the major number */
@@ -232,6 +224,16 @@ mididetach(device_t self, int flags)
 	mn = device_unit(self);
 	vdevgone(maj, mn, mn, VCHR);
 
+	mutex_enter(sc->lock);
+	if (--sc->refcnt >= 0) {
+		(void)cv_timedwait(&sc->detach_cv, sc->lock, hz * 60);
+		if (sc->refcnt >= 0) {
+			aprint_error_dev(self, "refcnt failed to drain,"
+			    " bashing my brains out anyway\n");
+		}
+	}
+	mutex_exit(sc->lock);
+
 	if (!(sc->props & MIDI_PROP_NO_OUTPUT)) {
 		evcnt_detach(&sc->xmt.bytesDiscarded);
 		evcnt_detach(&sc->xmt.incompleteMessages);

>From c47e7566d2458cc868951e9dd1853b54cf612904 Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
Date: Sun, 26 Dec 2021 12:19:50 +0000
Subject: [PATCH 2/4] umidi(4): Omit needless reference counting.

Detaching the mididevs waits for these operations to complete anyway,
so there's no need for another layer of reference counting.
---
 sys/dev/usb/umidi.c | 21 +--------------------
 1 file changed, 1 insertion(+), 20 deletions(-)

diff --git a/sys/dev/usb/umidi.c b/sys/dev/usb/umidi.c
index 2dccf19a6f31..ef5a32302ce8 100644
--- a/sys/dev/usb/umidi.c
+++ b/sys/dev/usb/umidi.c
@@ -209,9 +209,6 @@ struct umidi_softc {
 
 	kmutex_t		sc_lock;
 	kcondvar_t		sc_cv;
-	kcondvar_t		sc_detach_cv;
-
-	int			sc_refcnt;
 };
 
 #ifdef UMIDI_DEBUG
@@ -365,8 +362,6 @@ umidi_attach(device_t parent, device_t self, void *aux)
 
 	mutex_init(&sc->sc_lock, MUTEX_DEFAULT, IPL_SOFTUSB);
 	cv_init(&sc->sc_cv, "umidopcl");
-	cv_init(&sc->sc_detach_cv, "umidetcv");
-	sc->sc_refcnt = 0;
 
 	err = alloc_all_endpoints(sc);
 	if (err != USBD_NORMAL_COMPLETION) {
@@ -459,9 +454,6 @@ umidi_detach(device_t self, int flags)
 
 	mutex_enter(&sc->sc_lock);
 	sc->sc_dying = 1;
-	if (--sc->sc_refcnt >= 0)
-		if (cv_timedwait(&sc->sc_detach_cv, &sc->sc_lock, hz * 60))
-			aprint_error_dev(self, ": didn't detach\n");
 	mutex_exit(&sc->sc_lock);
 
 	detach_all_mididevs(sc, flags);
@@ -472,7 +464,6 @@ umidi_detach(device_t self, int flags)
 	usbd_add_drv_event(USB_EVENT_DRIVER_DETACH, sc->sc_udev, sc->sc_dev);
 
 	mutex_destroy(&sc->sc_lock);
-	cv_destroy(&sc->sc_detach_cv);
 	cv_destroy(&sc->sc_cv);
 
 	return 0;
@@ -531,7 +522,7 @@ void
 umidi_close(void *addr)
 {
 	struct umidi_mididev *mididev = addr;
-	struct umidi_softc *sc = mididev->sc;
+	struct umidi_softc *sc __diagused = mididev->sc;
 
 	KASSERT(mutex_owned(&sc->sc_lock));
 
@@ -540,16 +531,11 @@ umidi_close(void *addr)
 
 	mididev->closing = 1;
 
-	sc->sc_refcnt++;
-
 	if ((mididev->flags & FWRITE) && mididev->out_jack)
 		close_out_jack(mididev->out_jack);
 	if ((mididev->flags & FREAD) && mididev->in_jack)
 		close_in_jack(mididev->in_jack);
 
-	if (--sc->sc_refcnt < 0)
-		cv_broadcast(&sc->sc_detach_cv);
-
 	mididev->opened = 0;
 	mididev->closing = 0;
 }
@@ -1754,8 +1740,6 @@ out_jack_output(struct umidi_jack *out_jack, u_char *src, int len, int cin)
 	if (!out_jack->opened)
 		return ENODEV; /* XXX as it was, is this the right errno? */
 
-	sc->sc_refcnt++;
-
 #ifdef UMIDI_DEBUG
 	if (umididebug >= 100)
 		microtime(&umidi_tv);
@@ -1814,9 +1798,6 @@ out_jack_output(struct umidi_jack *out_jack, u_char *src, int len, int cin)
 		kpreempt_enable();
 	}
 
-	if (--sc->sc_refcnt < 0)
-		cv_broadcast(&sc->sc_detach_cv);
-
 	return 0;
 }
 

>From c80fc9a00fff9723d4027cb115789f87b875dd61 Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
Date: Sun, 26 Dec 2021 12:22:13 +0000
Subject: [PATCH 3/4] umidi(4): Read sc_dying only under the lock.

---
 sys/dev/usb/umidi.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/sys/dev/usb/umidi.c b/sys/dev/usb/umidi.c
index ef5a32302ce8..894d4f73bc5a 100644
--- a/sys/dev/usb/umidi.c
+++ b/sys/dev/usb/umidi.c
@@ -1815,10 +1815,14 @@ in_intr(struct usbd_xfer *xfer, void *priv,
 	unsigned char *data;
 	uint32_t count;
 
-	if (ep->sc->sc_dying || !ep->num_open)
+	if (!ep->num_open)
 		return;
 
 	mutex_enter(&sc->sc_lock);
+	if (sc->sc_dying) {
+		mutex_exit(&sc->sc_lock);
+		return;
+	}
 	usbd_get_xfer_status(xfer, NULL, NULL, &count, NULL);
 	if (0 == count % UMIDI_PACKET_SIZE) {
 		DPRINTFN(200,("%s: input endpoint %p transfer length %u\n",
@@ -1884,10 +1888,11 @@ out_intr(struct usbd_xfer *xfer, void *priv,
 	struct umidi_softc *sc = ep->sc;
 	uint32_t count;
 
-	if (sc->sc_dying)
-		return;
-
 	mutex_enter(&sc->sc_lock);
+	if (sc->sc_dying) {
+		mutex_exit(&sc->sc_lock);
+		return;
+	}
 #ifdef UMIDI_DEBUG
 	if (umididebug >= 200)
 		microtime(&umidi_tv);

>From 36a62333fb44e8a584117429e2ff63e33f59e9e2 Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
Date: Mon, 1 May 2023 10:00:05 +0000
Subject: [PATCH 4/4] midi(4): Make sure open failure doesn't leave the state
 open.

---
 sys/dev/midi.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/sys/dev/midi.c b/sys/dev/midi.c
index 6e644f23c6ec..027a868064ef 100644
--- a/sys/dev/midi.c
+++ b/sys/dev/midi.c
@@ -849,7 +849,7 @@ midiopen(dev_t dev, int flags, int ifmt, struct lwp *l)
 	sc->rcv_expect_asense = 0;
 	sc->rcv_quiescent = 0;
 	sc->rcv_eof = 0;
-	sc->isopen++;
+	sc->isopen = 1;
 	sc->flags = flags;
 	sc->pbus = 0;
 	sc->async = 0;
@@ -863,6 +863,8 @@ midiopen(dev_t dev, int flags, int ifmt, struct lwp *l)
 
 	error = hw->open(sc->hw_hdl, flags, midi_in, midi_out, sc);
 	if (error) {
+		KASSERT(sc->sc_isopen == 1);
+		sc->isopen = 0;
 		mutex_exit(sc->lock);
 		return error;
 	}