[PATCH] ALSA: seq: midi: wait for output buffer space on non-atomic delivery

[PATCH] ALSA: seq: midi: wait for output buffer space on non-atomic delivery

[Junjie Cao]

dump_midi() drops a chunk with -ENOMEM as soon as it does not fit in the
rawmidi output buffer, whatever context it runs in.  When a client
write(2)s one large SysEx event, the event is dispatched directly with
atomic == 0 and the payload is copied from user memory chunk by chunk
(snd_seq_deliver_event() rejects USRPTR events with atomic set), so the
caller can sleep, yet the transfer still dies as soon as it gets ahead
of the device by output_buffer_size bytes:

  ALSA: seq_midi: MIDI output buffer overrun

The report is a sample upload to a Novation Circuit Tracks over Web
MIDI, failing after about a second with the default 4096-byte buffer.
Raising output_buffer_size only moves the limit.

When atomic is clear, wait for the device to drain instead, the way
snd_rawmidi_write() does for a userspace writer: sleep on
runtime->sleep with the same 30-second no-progress timeout and feed the
buffer in pieces.  Atomic delivery keeps the fail-fast path.

A writer sleeping here still holds the source port's subscriber list
read-side, so unsubscribing that connection waits for the transfer to
finish or the timeout to fire.  midisynth_unuse() wakes a sleeping
writer after clearing output_substream, so once teardown gets there it
does not sit in snd_use_lock_sync() waiting out a stalled device.

Compile-tested only; no device with a large SysEx sink here.

Reported-by: Ruslan <ruslan.panasiuk@gmail.com>
Closes: https://bugzilla.redhat.com/show_bug.cgi?id=2517136
Signed-off-by: Junjie Cao <junjie.cao@intel.com>
---
 sound/core/seq/seq_midi.c | 81 ++++++++++++++++++++++++++++++++-------
 1 file changed, 67 insertions(+), 14 deletions(-)

diff --git a/sound/core/seq/seq_midi.c b/sound/core/seq/seq_midi.c
index a16a5debf339..ea8dc6a2b06f 100644
--- a/sound/core/seq/seq_midi.c
+++ b/sound/core/seq/seq_midi.c
@@ -109,22 +109,74 @@ static void snd_midi_input_event(struct snd_rawmidi_substream *substream)
 	snd_use_lock_free(&msynth->input_use_lock);
 }
 
-static int dump_midi(struct snd_rawmidi_substream *substream, const char *buf, int count)
+/* context for __dump_midi(), passed as the snd_seq_dump_var_event() ptr arg */
+struct seq_midi_dump_ctx {
+	struct seq_midisynth *msynth;
+	struct snd_rawmidi_substream *substream;
+	bool atomic;
+};
+
+/*
+ * Sleep until the device has drained some of the output buffer.  Returns
+ * early once midisynth_unuse() has cleared output_substream or the card
+ * is gone.
+ */
+static int wait_output_space(struct seq_midisynth *msynth,
+			     struct snd_rawmidi_substream *substream)
+{
+	struct snd_rawmidi_runtime *runtime = substream->runtime;
+	long ret;
+
+	ret = wait_event_interruptible_timeout(runtime->sleep,
+					       READ_ONCE(runtime->avail) > 0 ||
+					       !rcu_access_pointer(msynth->output_substream) ||
+					       substream->rmidi->card->shutdown,
+					       30 * HZ);
+	if (ret < 0)
+		return ret;
+	if (!ret)
+		return -EIO;
+	if (!rcu_access_pointer(msynth->output_substream) ||
+	    substream->rmidi->card->shutdown)
+		return -ENODEV;
+	return 0;
+}
+
+static int dump_midi(struct seq_midi_dump_ctx *ctx, const char *buf, int count)
 {
+	struct snd_rawmidi_substream *substream = ctx->substream;
 	struct snd_rawmidi_runtime *runtime;
-	int tmp;
+	long written;
+	int tmp, err;
 
 	if (snd_BUG_ON(!substream || !buf))
 		return -EINVAL;
 	runtime = substream->runtime;
-	tmp = runtime->avail;
-	if (tmp < count) {
-		if (printk_ratelimit())
-			pr_err("ALSA: seq_midi: MIDI output buffer overrun\n");
-		return -ENOMEM;
+
+	if (ctx->atomic) {
+		tmp = runtime->avail;
+		if (tmp < count) {
+			if (printk_ratelimit())
+				pr_err("ALSA: seq_midi: MIDI output buffer overrun\n");
+			return -ENOMEM;
+		}
+		if (snd_rawmidi_kernel_write(substream, buf, count) < count)
+			return -EINVAL;
+		return 0;
+	}
+
+	while (count > 0) {
+		written = snd_rawmidi_kernel_write(substream, buf, count);
+		if (written < 0)
+			return written;
+		buf += written;
+		count -= written;
+		if (count > 0) {
+			err = wait_output_space(ctx->msynth, substream);
+			if (err < 0)
+				return err;
+		}
 	}
-	if (snd_rawmidi_kernel_write(substream, buf, count) < count)
-		return -EINVAL;
 	return 0;
 }
 
@@ -139,7 +191,7 @@ static int event_process_midi(struct snd_seq_event *ev, int direct,
 {
 	struct seq_midisynth *msynth = private_data;
 	unsigned char msg[10];	/* buffer for constructing midi messages */
-	struct snd_rawmidi_substream *substream;
+	struct seq_midi_dump_ctx ctx = { .msynth = msynth, .atomic = atomic };
 	int err = 0;
 	int len;
 
@@ -147,8 +199,8 @@ static int event_process_midi(struct snd_seq_event *ev, int direct,
 		return -EINVAL;
 
 	scoped_guard(rcu) {
-		substream = rcu_dereference(msynth->output_substream);
-		if (!substream)
+		ctx.substream = rcu_dereference(msynth->output_substream);
+		if (!ctx.substream)
 			return -ENODEV;
 		snd_use_lock_use(&msynth->output_use_lock);
 	}
@@ -159,7 +211,7 @@ static int event_process_midi(struct snd_seq_event *ev, int direct,
 			pr_debug("ALSA: seq_midi: invalid sysex event flags = 0x%x\n", ev->flags);
 			goto out;
 		}
-		snd_seq_dump_var_event(ev, __dump_midi, substream);
+		snd_seq_dump_var_event(ev, __dump_midi, &ctx);
 		snd_midi_event_reset_decode(msynth->parser);
 	} else {
 		if (!msynth->parser) {
@@ -169,7 +221,7 @@ static int event_process_midi(struct snd_seq_event *ev, int direct,
 		len = snd_midi_event_decode(msynth->parser, msg, sizeof(msg), ev);
 		if (len < 0)
 			goto out;
-		if (dump_midi(substream, msg, len) < 0)
+		if (dump_midi(&ctx, msg, len) < 0)
 			snd_midi_event_reset_decode(msynth->parser);
 	}
 
@@ -289,6 +341,7 @@ static int midisynth_unuse(void *private_data, struct snd_seq_port_subscribe *in
 
 	rcu_assign_pointer(msynth->output_substream, NULL);
 	synchronize_rcu();
+	wake_up(&msynth->output_rfile.output->runtime->sleep);
 	snd_use_lock_sync(&msynth->output_use_lock);
 	rfile = msynth->output_rfile;
 	msynth->output_rfile = (struct snd_rawmidi_file){};
-- 
2.43.0

Re: [PATCH] ALSA: seq: midi: wait for output buffer space on non-atomic delivery

[Takashi Iwai]

On Mon, 31 Aug 2026 16:32:00 +0200,
Junjie Cao wrote:
> 
> dump_midi() drops a chunk with -ENOMEM as soon as it does not fit in the
> rawmidi output buffer, whatever context it runs in.  When a client
> write(2)s one large SysEx event, the event is dispatched directly with
> atomic == 0 and the payload is copied from user memory chunk by chunk
> (snd_seq_deliver_event() rejects USRPTR events with atomic set), so the
> caller can sleep, yet the transfer still dies as soon as it gets ahead
> of the device by output_buffer_size bytes:
> 
>   ALSA: seq_midi: MIDI output buffer overrun
> 
> The report is a sample upload to a Novation Circuit Tracks over Web
> MIDI, failing after about a second with the default 4096-byte buffer.
> Raising output_buffer_size only moves the limit.
> 
> When atomic is clear, wait for the device to drain instead, the way
> snd_rawmidi_write() does for a userspace writer: sleep on
> runtime->sleep with the same 30-second no-progress timeout and feed the
> buffer in pieces.  Atomic delivery keeps the fail-fast path.
> 
> A writer sleeping here still holds the source port's subscriber list
> read-side, so unsubscribing that connection waits for the transfer to
> finish or the timeout to fire.  midisynth_unuse() wakes a sleeping
> writer after clearing output_substream, so once teardown gets there it
> does not sit in snd_use_lock_sync() waiting out a stalled device.
> 
> Compile-tested only; no device with a large SysEx sink here.
> 
> Reported-by: Ruslan <ruslan.panasiuk@gmail.com>
> Closes: https://bugzilla.redhat.com/show_bug.cgi?id=2517136
> Signed-off-by: Junjie Cao <junjie.cao@intel.com>

A large SYSEX is a long-standing PITA, and I appreciate your attempt
to fix it.  But, the problem is that you can't take long time in
dump_midi() which is called from the sequencer event handler, per
design; the atomic=false there doesn't mean that you are allowed to
block for a too long time like 30 seconds.

For working around this problem, we need a basic design change in ALSA
sequencer core, I'm afraid.


thanks,

Takashi

Re: [PATCH] ALSA: seq: midi: wait for output buffer space on non-atomic delivery

[Junjie Cao]

On Tue, 01 Sep 2026 12:49:43 +0200, Takashi Iwai wrote:
> But, the problem is that you can't take long time in
> dump_midi() which is called from the sequencer event handler, per
> design; the atomic=false there doesn't mean that you are allowed to
> block for a too long time like 30 seconds.

Right, dropping this one.

> For working around this problem, we need a basic design change in ALSA
> sequencer core, I'm afraid.

Would you take an RFC along these lines, or do you have a different
shape in mind?

- event_input gets a way to say "port full": the core parks the event
  on a per-port FIFO instead of dropping it, copying direct/USRPTR
  payloads into the sender's pool cells (snd_seq_event_dup(),
  non-blocking).  While the FIFO is non-empty, further events for
  that port queue behind it.
- The kernel client signals readiness through a new snd_seq_kernel_*
  call and a work item re-offers the parked cells.  seq_midi would
  drive that from rawmidi's transmit ack, mirroring the input-side
  runtime->event hook.
- Backpressure stays at the sender's pool, the only place that sleeps
  today: a blocking writer with parked cells waits for pool space
  before its next direct dispatch, with ioctl_mutex dropped as
  snd_seq_cell_alloc() does; poll() already reports pool room.
  Nothing sleeps inside delivery.
- Client exit purges its parked cells from every port, as
  snd_seq_queue_client_leave() does for queues; port deletion frees
  the FIFO.

Open points: a SysEx bigger than the rawmidi buffer is consumed
partially, so the resume offset has to live in the parked entry or in
the driver; one event larger than the sender's pool cannot be parked
at all (the limit queued events already have); a broadcast is
duplicated per blocked destination.  Drivers that never report "full"
keep the current behaviour.

Re: [PATCH] ALSA: seq: midi: wait for output buffer space on non-atomic delivery

[Takashi Iwai]

On Tue, 01 Sep 2026 16:38:03 +0200,
Junjie Cao wrote:
> 
> On Tue, 01 Sep 2026 12:49:43 +0200, Takashi Iwai wrote:
> > But, the problem is that you can't take long time in
> > dump_midi() which is called from the sequencer event handler, per
> > design; the atomic=false there doesn't mean that you are allowed to
> > block for a too long time like 30 seconds.
> 
> Right, dropping this one.
> 
> > For working around this problem, we need a basic design change in ALSA
> > sequencer core, I'm afraid.
> 
> Would you take an RFC along these lines, or do you have a different
> shape in mind?
> 
> - event_input gets a way to say "port full": the core parks the event
>   on a per-port FIFO instead of dropping it, copying direct/USRPTR
>   payloads into the sender's pool cells (snd_seq_event_dup(),
>   non-blocking).  While the FIFO is non-empty, further events for
>   that port queue behind it.
> - The kernel client signals readiness through a new snd_seq_kernel_*
>   call and a work item re-offers the parked cells.  seq_midi would
>   drive that from rawmidi's transmit ack, mirroring the input-side
>   runtime->event hook.
> - Backpressure stays at the sender's pool, the only place that sleeps
>   today: a blocking writer with parked cells waits for pool space
>   before its next direct dispatch, with ioctl_mutex dropped as
>   snd_seq_cell_alloc() does; poll() already reports pool room.
>   Nothing sleeps inside delivery.
> - Client exit purges its parked cells from every port, as
>   snd_seq_queue_client_leave() does for queues; port deletion frees
>   the FIFO.
> 
> Open points: a SysEx bigger than the rawmidi buffer is consumed
> partially, so the resume offset has to live in the parked entry or in
> the driver; one event larger than the sender's pool cannot be parked
> at all (the limit queued events already have); a broadcast is
> duplicated per blocked destination.  Drivers that never report "full"
> keep the current behaviour.

As mentioned, this is no new problem but a known issue (rather from
the beginning) over decades.  So it's no hurry about the fix time
frame :)

In ALSA sequencer core, the event packet delivery itself must not be
blocked too long -- so waiting at callback (that happens at the
delivery) won't work well.  Otherwise this will block the delivery of
other events.  Although in the case of seq-midi it might work because
it's handled in a work, the same problem happens for other clients,
hence we'd have to tackle in ALSA sequencer core side more properly.

My idea has no solid shape yet, but its basic form would be something
to push-back and re-deliver of event packets.  For a large data like
SysEx, it should support a partial (re-)delivery, too.

Instead of blocking the delivery, if the buffer is full, it's pushed
back, and rescheduled.  Obviously, this reschedule would work only for
the queued event delivery from the pool.  When it's transferred as a
direct delivery, it should just fail like the current version.
And, if the destination client gives some partial success, we'd have
to record the succeeded size, so that the re-delivery begins from the
right point, too.

So far, so good.  The problem surfaced, however, when there are
multiple destination targets from a client/port; then one (or a few)
of them might block while others can process fully.  Handling for this
situation has to be implemented somehow.  I can imagine to extend the
struct snd_seq_subscribers and struct snd_seq_port_subs_info to track
the pending events per connection, but not sure whether this can fly
well...


thanks,

Takashi