[mvs@openbsd.org: Please test; midi(4): make midi{read,write}_filtops mp safe]

Hi,

I'm searching help with midi(4) testing. If someone could test this
diff, let me know.

----- Forwarded message from Vitaliy Makkoveev <mvs@openbsd.org> -----

Date: Sun, 24 Sep 2023 23:03:54 +0300
From: Vitaliy Makkoveev <mvs@openbsd.org>
To: tech@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
}