On Fri, Feb 10, 2023 at 02:56:21PM +0000, Visa Hankala wrote:
> This makes midi(4) event filter MP-safe.
>
> The logic is similar to audio(4). As knote(9) is safe to use
> at IPL_AUDIO, the deferring through soft interrupts is not needed
> any longer.
>
> In mididetach(), the separate selwakeup() calls are covered by
> klist_invalidate().
>
> Could someone with actual midi(4) hardware test this?
The initial posting misses sys/dev/midivar.h.
Here is the full patch:
Index: dev/midi.c
===================================================================
RCS file: src/sys/dev/midi.c,v
retrieving revision 1.55
diff -u -p -r1.55 midi.c
--- dev/midi.c 2 Jul 2022 08:50:41 -0000 1.55
+++ dev/midi.c 10 Feb 2023 15:14:21 -0000
@@ -22,6 +22,8 @@
#include <sys/ioctl.h>
#include <sys/conf.h>
#include <sys/kernel.h>
+#include <sys/event.h>
+#include <sys/mutex.h>
#include <sys/timeout.h>
#include <sys/vnode.h>
#include <sys/signalvar.h>
@@ -65,41 +67,40 @@ 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;
+ MUTEX_ASSERT_LOCKED(&audio_lock);
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 +118,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(&sc->inbuf);
}
int
@@ -227,13 +222,7 @@ 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 +331,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 +344,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;
@@ -366,51 +353,61 @@ done:
void
filt_midirdetach(struct knote *kn)
{
- struct midi_softc *sc = (struct midi_softc *)kn->kn_hook;
+ struct midi_softc *sc = 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;
+ struct midi_softc *sc = kn->kn_hook;
- if ((hint & NOTE_SUBMIT) == 0)
- mtx_enter(&audio_lock);
- retval = !MIDIBUF_ISEMPTY(&sc->inbuf);
- if ((hint & NOTE_SUBMIT) == 0)
- mtx_leave(&audio_lock);
+ MUTEX_ASSERT_LOCKED(&audio_lock);
- return (retval);
+ return !MIDIBUF_ISEMPTY(&sc->inbuf);
}
void
filt_midiwdetach(struct knote *kn)
{
- struct midi_softc *sc = (struct midi_softc *)kn->kn_hook;
+ struct midi_softc *sc = kn->kn_hook;
+
+ klist_remove(&sc->outbuf.klist, kn);
+}
+
+int
+filt_midiwrite(struct knote *kn, long hint)
+{
+ struct midi_softc *sc = kn->kn_hook;
+
+ MUTEX_ASSERT_LOCKED(&audio_lock);
+
+ return !MIDIBUF_ISFULL(&sc->outbuf);
+}
+
+int
+filt_midimodify(struct kevent *kev, struct knote *kn)
+{
+ int active;
mtx_enter(&audio_lock);
- klist_remove_locked(&sc->outbuf.sel.si_note, kn);
+ active = knote_modify(kev, kn);
mtx_leave(&audio_lock);
+
+ return active;
}
int
-filt_midiwrite(struct knote *kn, long hint)
+filt_midiprocess(struct knote *kn, struct kevent *kev)
{
- struct midi_softc *sc = (struct midi_softc *)kn->kn_hook;
- int retval;
+ int active;
- if ((hint & NOTE_SUBMIT) == 0)
- mtx_enter(&audio_lock);
- retval = !MIDIBUF_ISFULL(&sc->outbuf);
- if ((hint & NOTE_SUBMIT) == 0)
- mtx_leave(&audio_lock);
+ mtx_enter(&audio_lock);
+ active = knote_process(kn, kev);
+ mtx_leave(&audio_lock);
- return (retval);
+ return active;
}
int
@@ -531,20 +528,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;
@@ -577,30 +562,20 @@ mididetach(struct device *self, int flag
* in read/write/ioctl, which return EIO.
*/
if (sc->flags) {
- KERNEL_ASSERT_LOCKED();
- if (sc->flags & FREAD) {
+ if (sc->flags & FREAD)
wakeup(&sc->inbuf.blocking);
- mtx_enter(&audio_lock);
- selwakeup(&sc->inbuf.sel);
- mtx_leave(&audio_lock);
- }
- if (sc->flags & FWRITE) {
+ if (sc->flags & FWRITE)
wakeup(&sc->outbuf.blocking);
- mtx_enter(&audio_lock);
- selwakeup(&sc->outbuf.sel);
- mtx_leave(&audio_lock);
- }
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);
+
+ klist_free(&sc->inbuf.klist);
+ klist_free(&sc->outbuf.klist);
- if (sc->inbuf.softintr)
- softintr_disestablish(sc->inbuf.softintr);
- if (sc->outbuf.softintr)
- softintr_disestablish(sc->outbuf.softintr);
return 0;
}
Index: dev/midivar.h
===================================================================
RCS file: src/sys/dev/midivar.h,v
retrieving revision 1.13
diff -u -p -r1.13 midivar.h
--- dev/midivar.h 21 Mar 2022 19:22:40 -0000 1.13
+++ dev/midivar.h 10 Feb 2023 15:14:21 -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; /* list of knotes */
int blocking; /* read/write blocking */
unsigned char data[MIDIBUF_SIZE];
unsigned start, used;
