From f6497663fff0c1f512cc39e763d61c404b7fe407 Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Sun, 26 Dec 2021 12:12:50 +0000 Subject: [PATCH 1/4] midi(4): Wake waiters _before_ waiting for them. Otherwise we may deadlock waiting for them to give up while they're waiting for I/O, not knowing they need to give up. --- sys/dev/midi.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/sys/dev/midi.c b/sys/dev/midi.c index f3f0ffe73876..496223fec22a 100644 --- a/sys/dev/midi.c +++ b/sys/dev/midi.c @@ -207,13 +207,12 @@ mididetach(device_t self, int flags) mutex_enter(sc->lock); sc->dying = 1; + cv_broadcast(&sc->wchan); + cv_broadcast(&sc->rchan); if (--sc->refcnt >= 0) { - /* Wake anything? */ (void)cv_timedwait(&sc->detach_cv, sc->lock, hz * 60); } - cv_broadcast(&sc->wchan); - cv_broadcast(&sc->rchan); mutex_exit(sc->lock); /* locate the major number */ From d157bee5b3419c2120e7b13da4f0f63ca1bd0a25 Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Sun, 26 Dec 2021 12:14:33 +0000 Subject: [PATCH 2/4] midi(4): Explain what's about to happen when refcnt drain fails. This should really be an unconditional cv_wait loop, but for now let's just give fair warning when the driver is about to puke its guts out in case the I/O paths I haven't audited yet are buggy. --- sys/dev/midi.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/sys/dev/midi.c b/sys/dev/midi.c index 496223fec22a..7ca0700d4f18 100644 --- a/sys/dev/midi.c +++ b/sys/dev/midi.c @@ -212,6 +212,10 @@ mididetach(device_t self, int flags) 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); From 6444c87f8336f64e4badd60302834eaa09670d8e Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Sun, 26 Dec 2021 12:16:28 +0000 Subject: [PATCH 3/4] midi(4): No need to take lock just for callout_halt. Lock is relevant only if we're doing something _else_ under the lock that must be serialized with the callout's own action. --- sys/dev/midi.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/sys/dev/midi.c b/sys/dev/midi.c index 7ca0700d4f18..98cf03eb3546 100644 --- a/sys/dev/midi.c +++ b/sys/dev/midi.c @@ -247,10 +247,8 @@ mididetach(device_t self, int flags) sc->sih = NULL; } - mutex_enter(sc->lock); - callout_halt(&sc->xmt_asense_co, sc->lock); - callout_halt(&sc->rcv_asense_co, sc->lock); - mutex_exit(sc->lock); + callout_halt(&sc->xmt_asense_co, NULL); + callout_halt(&sc->rcv_asense_co, NULL); callout_destroy(&sc->xmt_asense_co); callout_destroy(&sc->rcv_asense_co); From c6875dd42222f118109d02cd7b3383ed459c6569 Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Sun, 26 Dec 2021 12:17:30 +0000 Subject: [PATCH 4/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 98cf03eb3546..ce824813367e 100644 --- a/sys/dev/midi.c +++ b/sys/dev/midi.c @@ -209,14 +209,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 */ @@ -233,6 +225,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);