On Sun, Sep 24, 2023 at 11:03:54PM +0300, Vitaliy Makkoveev wrote:
> 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.

https://marc.info/?l=openbsd-tech&m=167604232828221 has minor takeaways
if you pay attention.

> 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;
> 

Reply via email to