Hi,
I'm searching help with midi(4) testing. If someone could test this
diff, let me know.
----- Forwarded message from Vitaliy Makkoveev <m...@openbsd.org> -----
Date: Sun, 24 Sep 2023 23:03:54 +0300
From: Vitaliy Makkoveev <m...@openbsd.org>
To: t...@openbsd.org
Subject: Please test; midi(4): make midi{read,write}_filtops mp safe
Please test this diff, I have no midi(4) devices.
midi(4) already uses `audio_lock' mutex(9) for filterops, but they are
still kernel locked. Wipe out old selwakeup API and make them MP safe.
knote_locked(9) will not grab kernel lock, so call it directly from
interrupt handlers instead of scheduling software interrupts.
Index: sys/dev/midi.c
===================================================================
RCS file: /cvs/src/sys/dev/midi.c,v
retrieving revision 1.55
diff -u -p -r1.55 midi.c
--- sys/dev/midi.c 2 Jul 2022 08:50:41 -0000 1.55
+++ sys/dev/midi.c 24 Sep 2023 19:57:56 -0000
@@ -31,7 +31,6 @@
#include <dev/audio_if.h>
#include <dev/midivar.h>
-#define IPL_SOFTMIDI IPL_SOFTNET
#define DEVNAME(sc) ((sc)->dev.dv_xname)
int midiopen(dev_t, int, int, struct proc *);
@@ -65,41 +64,38 @@ struct cfdriver midi_cd = {
void filt_midiwdetach(struct knote *);
int filt_midiwrite(struct knote *, long);
+int filt_midimodify(struct kevent *, struct knote *);
+int filt_midiprocess(struct knote *, struct kevent *);
const struct filterops midiwrite_filtops = {
- .f_flags = FILTEROP_ISFD,
+ .f_flags = FILTEROP_ISFD | FILTEROP_MPSAFE,
.f_attach = NULL,
.f_detach = filt_midiwdetach,
.f_event = filt_midiwrite,
+ .f_modify = filt_midimodify,
+ .f_process = filt_midiprocess,
};
void filt_midirdetach(struct knote *);
int filt_midiread(struct knote *, long);
const struct filterops midiread_filtops = {
- .f_flags = FILTEROP_ISFD,
+ .f_flags = FILTEROP_ISFD | FILTEROP_MPSAFE,
.f_attach = NULL,
.f_detach = filt_midirdetach,
.f_event = filt_midiread,
+ .f_modify = filt_midimodify,
+ .f_process = filt_midiprocess,
};
void
-midi_buf_wakeup(void *addr)
+midi_buf_wakeup(struct midi_buffer *buf)
{
- struct midi_buffer *buf = addr;
-
if (buf->blocking) {
wakeup(&buf->blocking);
buf->blocking = 0;
}
- /*
- * As long as selwakeup() grabs the KERNEL_LOCK() make sure it is
- * already held here to avoid lock ordering problems with `audio_lock'
- */
- KERNEL_ASSERT_LOCKED();
- mtx_enter(&audio_lock);
- selwakeup(&buf->sel);
- mtx_leave(&audio_lock);
+ knote_locked(&buf->klist, 0);
}
void
@@ -117,13 +113,7 @@ midi_iintr(void *addr, int data)
MIDIBUF_WRITE(mb, data);
- /*
- * As long as selwakeup() needs to be protected by the
- * KERNEL_LOCK() we have to delay the wakeup to another
- * context to keep the interrupt context KERNEL_LOCK()
- * free.
- */
- softintr_schedule(sc->inbuf.softintr);
+ midi_buf_wakeup(mb);
}
int
@@ -226,14 +216,7 @@ void
midi_out_stop(struct midi_softc *sc)
{
sc->isbusy = 0;
-
- /*
- * As long as selwakeup() needs to be protected by the
- * KERNEL_LOCK() we have to delay the wakeup to another
- * context to keep the interrupt context KERNEL_LOCK()
- * free.
- */
- softintr_schedule(sc->outbuf.softintr);
+ midi_buf_wakeup(&sc->outbuf);
}
void
@@ -342,11 +325,11 @@ midikqfilter(dev_t dev, struct knote *kn
error = 0;
switch (kn->kn_filter) {
case EVFILT_READ:
- klist = &sc->inbuf.sel.si_note;
+ klist = &sc->inbuf.klist;
kn->kn_fop = &midiread_filtops;
break;
case EVFILT_WRITE:
- klist = &sc->outbuf.sel.si_note;
+ klist = &sc->outbuf.klist;
kn->kn_fop = &midiwrite_filtops;
break;
default:
@@ -355,9 +338,7 @@ midikqfilter(dev_t dev, struct knote *kn
}
kn->kn_hook = (void *)sc;
- mtx_enter(&audio_lock);
- klist_insert_locked(klist, kn);
- mtx_leave(&audio_lock);
+ klist_insert(klist, kn);
done:
device_unref(&sc->dev);
return error;
@@ -368,24 +349,15 @@ filt_midirdetach(struct knote *kn)
{
struct midi_softc *sc = (struct midi_softc *)kn->kn_hook;
- mtx_enter(&audio_lock);
- klist_remove_locked(&sc->inbuf.sel.si_note, kn);
- mtx_leave(&audio_lock);
+ klist_remove(&sc->inbuf.klist, kn);
}
int
filt_midiread(struct knote *kn, long hint)
{
struct midi_softc *sc = (struct midi_softc *)kn->kn_hook;
- int retval;
-
- if ((hint & NOTE_SUBMIT) == 0)
- mtx_enter(&audio_lock);
- retval = !MIDIBUF_ISEMPTY(&sc->inbuf);
- if ((hint & NOTE_SUBMIT) == 0)
- mtx_leave(&audio_lock);
- return (retval);
+ return (!MIDIBUF_ISEMPTY(&sc->inbuf));
}
void
@@ -393,24 +365,39 @@ filt_midiwdetach(struct knote *kn)
{
struct midi_softc *sc = (struct midi_softc *)kn->kn_hook;
- mtx_enter(&audio_lock);
- klist_remove_locked(&sc->outbuf.sel.si_note, kn);
- mtx_leave(&audio_lock);
+ klist_remove(&sc->outbuf.klist, kn);
}
int
filt_midiwrite(struct knote *kn, long hint)
{
struct midi_softc *sc = (struct midi_softc *)kn->kn_hook;
- int retval;
- if ((hint & NOTE_SUBMIT) == 0)
- mtx_enter(&audio_lock);
- retval = !MIDIBUF_ISFULL(&sc->outbuf);
- if ((hint & NOTE_SUBMIT) == 0)
- mtx_leave(&audio_lock);
+ return (!MIDIBUF_ISFULL(&sc->outbuf));
+}
+
+int
+filt_midimodify(struct kevent *kev, struct knote *kn)
+{
+ int active;
+
+ mtx_enter(&audio_lock);
+ active = knote_modify(kev, kn);
+ mtx_leave(&audio_lock);
+
+ return active;
+}
+
+int
+filt_midiprocess(struct knote *kn, struct kevent *kev)
+{
+ int active;
+
+ mtx_enter(&audio_lock);
+ active = knote_process(kn, kev);
+ mtx_leave(&audio_lock);
- return (retval);
+ return active;
}
int
@@ -531,20 +518,8 @@ midiattach(struct device *parent, struct
}
#endif
- sc->inbuf.softintr = softintr_establish(IPL_SOFTMIDI,
- midi_buf_wakeup, &sc->inbuf);
- if (sc->inbuf.softintr == NULL) {
- printf("%s: can't establish input softintr\n", DEVNAME(sc));
- return;
- }
-
- sc->outbuf.softintr = softintr_establish(IPL_SOFTMIDI,
- midi_buf_wakeup, &sc->outbuf);
- if (sc->outbuf.softintr == NULL) {
- printf("%s: can't establish output softintr\n", DEVNAME(sc));
- softintr_disestablish(sc->inbuf.softintr);
- return;
- }
+ klist_init_mutex(&sc->inbuf.klist, &audio_lock);
+ klist_init_mutex(&sc->outbuf.klist, &audio_lock);
sc->hw_if = hwif;
sc->hw_hdl = hdl;
@@ -580,27 +555,19 @@ mididetach(struct device *self, int flag
KERNEL_ASSERT_LOCKED();
if (sc->flags & FREAD) {
wakeup(&sc->inbuf.blocking);
- mtx_enter(&audio_lock);
- selwakeup(&sc->inbuf.sel);
- mtx_leave(&audio_lock);
+ knote(&sc->inbuf.klist, 0);
}
if (sc->flags & FWRITE) {
wakeup(&sc->outbuf.blocking);
- mtx_enter(&audio_lock);
- selwakeup(&sc->outbuf.sel);
- mtx_leave(&audio_lock);
+ knote(&sc->outbuf.klist, 0);
}
sc->hw_if->close(sc->hw_hdl);
sc->flags = 0;
}
- klist_invalidate(&sc->inbuf.sel.si_note);
- klist_invalidate(&sc->outbuf.sel.si_note);
+ klist_invalidate(&sc->inbuf.klist);
+ klist_invalidate(&sc->outbuf.klist);
- if (sc->inbuf.softintr)
- softintr_disestablish(sc->inbuf.softintr);
- if (sc->outbuf.softintr)
- softintr_disestablish(sc->outbuf.softintr);
return 0;
}
Index: sys/dev/midivar.h
===================================================================
RCS file: /cvs/src/sys/dev/midivar.h,v
retrieving revision 1.13
diff -u -p -r1.13 midivar.h
--- sys/dev/midivar.h 21 Mar 2022 19:22:40 -0000 1.13
+++ sys/dev/midivar.h 24 Sep 2023 19:57:56 -0000
@@ -21,7 +21,7 @@
#include <dev/midi_if.h>
#include <sys/device.h>
-#include <sys/selinfo.h>
+#include <sys/event.h>
#include <sys/proc.h>
#include <sys/timeout.h>
@@ -34,8 +34,7 @@
#define MIDIBUF_MASK (MIDIBUF_SIZE - 1)
struct midi_buffer {
- void *softintr; /* context to call selwakeup() */
- struct selinfo sel; /* to record & wakeup poll(2) */
+ struct klist klist; /* to record & wakeup poll(2) */
int blocking; /* read/write blocking */
unsigned char data[MIDIBUF_SIZE];
unsigned start, used;
----- End forwarded message -----
