NetBSD-Bugs archive
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]
Re: kern/60332: system crashed when ^C on midiplay hung and i turned the synth off
The following reply was made to PR kern/60332; it has been noted by GNATS.
From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
To: mrg%eterna23.net@localhost
Cc: gnats-bugs%NetBSD.org@localhost, netbsd-bugs%NetBSD.org@localhost
Subject: Re: kern/60332: system crashed when ^C on midiplay hung and i turned the synth off
Date: Mon, 15 Jun 2026 00:38:23 +0000
This is a multi-part message in MIME format.
--=_U+89j2l9CEBqI2aSXBW9qNw7GOwXDlcv
Content-Transfer-Encoding: quoted-printable
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 =3D 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/opend=
etach.pdf).
--=_U+89j2l9CEBqI2aSXBW9qNw7GOwXDlcv
Content-Type: text/plain; charset="ISO-8859-1"; name="midipbuswakeup"
Content-Transfer-Encoding: quoted-printable
Content-Disposition: attachment; filename="midipbuswakeup.patch"
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 =3D 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 =3D 0;
+ cv_broadcast(&sc->wchan);
callout_schedule(&sc->xmt_asense_co, MIDI_XMT_ASENSE_PERIOD);
return error;
}
--=_U+89j2l9CEBqI2aSXBW9qNw7GOwXDlcv
Content-Type: text/plain; charset="ISO-8859-1"; name="mididetach"
Content-Transfer-Encoding: quoted-printable
Content-Disposition: attachment; filename="mididetach.patch"
>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 ref=
cnt
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 =3D 1;
cv_broadcast(&sc->wchan);
cv_broadcast(&sc->rchan);
-
- if (--sc->refcnt >=3D 0) {
- (void)cv_timedwait(&sc->detach_cv, sc->lock, hz * 60);
- if (sc->refcnt >=3D 0) {
- aprint_error_dev(self, "refcnt failed to drain,"
- " bashing my brains out anyway\n");
- }
- }
mutex_exit(sc->lock);
=20
/* locate the major number */
@@ -232,6 +224,16 @@ mididetach(device_t self, int flags)
mn =3D device_unit(self);
vdevgone(maj, mn, mn, VCHR);
=20
+ mutex_enter(sc->lock);
+ if (--sc->refcnt >=3D 0) {
+ (void)cv_timedwait(&sc->detach_cv, sc->lock, hz * 60);
+ if (sc->refcnt >=3D 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 {
=20
kmutex_t sc_lock;
kcondvar_t sc_cv;
- kcondvar_t sc_detach_cv;
-
- int sc_refcnt;
};
=20
#ifdef UMIDI_DEBUG
@@ -365,8 +362,6 @@ umidi_attach(device_t parent, device_t self, void *aux)
=20
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 =3D 0;
=20
err =3D alloc_all_endpoints(sc);
if (err !=3D USBD_NORMAL_COMPLETION) {
@@ -459,9 +454,6 @@ umidi_detach(device_t self, int flags)
=20
mutex_enter(&sc->sc_lock);
sc->sc_dying =3D 1;
- if (--sc->sc_refcnt >=3D 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);
=20
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);
=20
mutex_destroy(&sc->sc_lock);
- cv_destroy(&sc->sc_detach_cv);
cv_destroy(&sc->sc_cv);
=20
return 0;
@@ -531,7 +522,7 @@ void
umidi_close(void *addr)
{
struct umidi_mididev *mididev =3D addr;
- struct umidi_softc *sc =3D mididev->sc;
+ struct umidi_softc *sc __diagused =3D mididev->sc;
=20
KASSERT(mutex_owned(&sc->sc_lock));
=20
@@ -540,16 +531,11 @@ umidi_close(void *addr)
=20
mididev->closing =3D 1;
=20
- 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);
=20
- if (--sc->sc_refcnt < 0)
- cv_broadcast(&sc->sc_detach_cv);
-
mididev->opened =3D 0;
mididev->closing =3D 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? */
=20
- sc->sc_refcnt++;
-
#ifdef UMIDI_DEBUG
if (umididebug >=3D 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();
}
=20
- if (--sc->sc_refcnt < 0)
- cv_broadcast(&sc->sc_detach_cv);
-
return 0;
}
=20
>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;
=20
- if (ep->sc->sc_dying || !ep->num_open)
+ if (!ep->num_open)
return;
=20
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 =3D=3D 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 =3D ep->sc;
uint32_t count;
=20
- 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 >=3D 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 =3D 0;
sc->rcv_quiescent =3D 0;
sc->rcv_eof =3D 0;
- sc->isopen++;
+ sc->isopen =3D 1;
sc->flags =3D flags;
sc->pbus =3D 0;
sc->async =3D 0;
@@ -863,6 +863,8 @@ midiopen(dev_t dev, int flags, int ifmt, struct lwp *l)
=20
error =3D hw->open(sc->hw_hdl, flags, midi_in, midi_out, sc);
if (error) {
+ KASSERT(sc->sc_isopen =3D=3D 1);
+ sc->isopen =3D 0;
mutex_exit(sc->lock);
return error;
}
--=_U+89j2l9CEBqI2aSXBW9qNw7GOwXDlcv--
Home |
Main Index |
Thread Index |
Old Index