kernel snd seq bugs: SND_SEQ_EVENT_CLIENT_CHANGE & SND_SEQ_EVENT_PORT_CHANGE

kernel snd seq bugs: SND_SEQ_EVENT_CLIENT_CHANGE & SND_SEQ_EVENT_PORT_CHANGE

[Mark Lentczner]

I'm the author of amidiminder - a utility that keeps track of desired
connections between ALSA Seq (MIDI) ports.

I've found a bug in the kernel part of the ALSA Sequencer, and am not sure
where or how to report it.

Summary:
The events:
* SND_SEQ_EVENT_CLIENT_CHANGE is never sent
* SND_SEQ_EVENT_PORT_CHANGE is not sent on common port changes (name and
port info)

I have a lot of detail on these bugs, what other software causes them to be
a problem (PureData), and where the missing code in the kernel
sound/core/seq files is.

Before I dump all the details in this forum.... where is the right place
for me to report and/or discuss this?

- Mark

Re: kernel snd seq bugs: SND_SEQ_EVENT_CLIENT_CHANGE & SND_SEQ_EVENT_PORT_CHANGE

[Takashi Iwai]

On Sun, 24 Nov 2024 22:31:13 +0100,
Mark Lentczner wrote:
> 
> I'm the author of amidiminder - a utility that keeps track of desired
> connections between ALSA Seq (MIDI) ports.
> 
> I've found a bug in the kernel part of the ALSA Sequencer, and am not sure
> where or how to report it.
> 
> Summary:
> The events:
> * SND_SEQ_EVENT_CLIENT_CHANGE is never sent
> * SND_SEQ_EVENT_PORT_CHANGE is not sent on common port changes (name and
> port info)
> 
> I have a lot of detail on these bugs, what other software causes them to be
> a problem (PureData), and where the missing code in the kernel
> sound/core/seq files is.
> 
> Before I dump all the details in this forum.... where is the right place
> for me to report and/or discuss this?

Those events aren't created indeed.  I don't remember exactly, as the
implementation was really old (decades ago), but I guess it was
because it'd give too much broadcasting when the event is triggered at
each client / port change.

Actually it's trivial to enable those events.  It's just a patch like
below.  But, I have to consider the result by this change and try to
avoid the unnecessary event bombing before going ahead.


thanks,

Takashi

-- 8< --
--- a/sound/core/seq/seq_clientmgr.c
+++ b/sound/core/seq/seq_clientmgr.c
@@ -1290,6 +1290,10 @@ static int snd_seq_ioctl_set_client_info(struct snd_seq_client *client,
 		client->midi_version = client_info->midi_version;
 	memcpy(client->event_filter, client_info->event_filter, 32);
 	client->group_filter = client_info->group_filter;
+
+	/* notify the change */
+	snd_seq_system_client_ev_client_change(client->number);
+
 	return 0;
 }
 
@@ -1413,6 +1417,9 @@ static int snd_seq_ioctl_set_port_info(struct snd_seq_client *client, void *arg)
 	if (port) {
 		snd_seq_set_port_info(port, info);
 		snd_seq_port_unlock(port);
+		/* notify the change */
+		snd_seq_system_client_ev_port_change(info->addr.client,
+						     info->addr.port);
 	}
 	return 0;
 }

Re: kernel snd seq bugs: SND_SEQ_EVENT_CLIENT_CHANGE & SND_SEQ_EVENT_PORT_CHANGE

[Mark Lentczner]

Looking at the items that can be changed with client info, or port info, I
think there is little likelihood that applications are going to be changing
this information more than once in the lifetime of a client, or a port.

So far, my application hasn't detected any changes to port information
after it starts tracking a port (though it doesn't track everything about a
port.) This is certainly because the port info is given at the time of port
creation. Of the several DAWs that I tested, when you change their MIDI
configuration - rather than change the port info of their existing ports,
they simply delete them all, and recreate the ports as needed, which is
broadcasting PORT_EXIT and PORT_START events anyway.   In either case, this
isn't a broadcast concern as these events are relatively rare.

Client info is a bit different, as you must create your client first, then
set info. So the CLIENT_START event would be followed by one or more
CLIENT_CHANGE events. This is exacerbated because the helper functions for
setting client name, event filter, etc... which each get the client info,
set one field, and set the info back. So true, this change would cause a
broadcast of a few events each time a client is created... But again, in
the scheme of things, client creation is a relatively rare occurrence.

In the absence of these events, applications like mine, have to
periodically poll every client info and every port info to see if anything
that they care about (like client name or port name) has changed. For now,
since port info changes (at least of name or capabilities) is essentially
non-existent, my application doesn't bother to poll ports. And for clients,
since the changes only happen right after client start, my application
simply sleeps a short interval after getting CLIENT_START, before reading
the client info, giving the client process time to set the info up. Clearly
this isn't optimal or infalible - but in practice it works, and polling
info frequently seems too high a cost.

Lastly, there IS one place in the code where PORT_CHANGE is sent. In
seq_ump_client.c in the function update_port_infos(). In that case,
interestingly, a port_info change is only considered if and only if the
name or the capability fields are changed. That logic could be applied to
your patch - but given how infrequent I suspect port (or client) changes
are, it seems presumptive to be limiting what field changes an application
would be interested in.

Happy to dive into any more details you may need,
— Mark

Re: kernel snd seq bugs: SND_SEQ_EVENT_CLIENT_CHANGE & SND_SEQ_EVENT_PORT_CHANGE

[Takashi Iwai]

On Mon, 25 Nov 2024 19:55:00 +0100,
Mark Lentczner wrote:
> 
> 
> Looking at the items that can be changed with client info, or port info, I
> think there is little likelihood that applications are going to be changing
> this information more than once in the lifetime of a client, or a port.
> 
> So far, my application hasn't detected any changes to port information after
> it starts tracking a port (though it doesn't track everything about a port.)
> This is certainly because the port info is given at the time of port creation.
> Of the several DAWs that I tested, when you change their MIDI configuration -
> rather than change the port info of their existing ports, they simply delete
> them all, and recreate the ports as needed, which is broadcasting PORT_EXIT
> and PORT_START events anyway.   In either case, this isn't a broadcast concern
> as these events are relatively rare.
> 
> Client info is a bit different, as you must create your client first, then set
> info. So the CLIENT_START event would be followed by one or more CLIENT_CHANGE
> events. This is exacerbated because the helper functions for setting client
> name, event filter, etc... which each get the client info, set one field, and
> set the info back. So true, this change would cause a broadcast of a few
> events each time a client is created... But again, in the scheme of things,
> client creation is a relatively rare occurrence.
> 
> In the absence of these events, applications like mine, have to periodically
> poll every client info and every port info to see if anything that they care
> about (like client name or port name) has changed. For now, since port info
> changes (at least of name or capabilities) is essentially non-existent, my
> application doesn't bother to poll ports. And for clients, since the changes
> only happen right after client start, my application simply sleeps a short
> interval after getting CLIENT_START, before reading the client info, giving
> the client process time to set the info up. Clearly this isn't optimal or
> infalible - but in practice it works, and polling info frequently seems too
> high a cost.

Actually it's only the deliveries per subscription, hence the message
transmissions aren't too high.  We can optimize a bit in the case
where no subscription is done to the announcement port, but that's
another story.

> Lastly, there IS one place in the code where PORT_CHANGE is sent. In
> seq_ump_client.c in the function update_port_infos(). In that case,
> interestingly, a port_info change is only considered if and only if the name
> or the capability fields are changed. That logic could be applied to your
> patch - but given how infrequent I suspect port (or client) changes are, it
> seems presumptive to be limiting what field changes an application would be
> interested in.

Yes, that's possible, but I don't think it's worth for complication;
the call patterns of client and port info ioctls are usually for
really updating the attributes from applications.  OTOH, in the recent
UMP code, I implemented the check because it may driven by the device
firmware, hence it's not really predictable.

That being said, I'm fine to apply the change, but I hesitate to do so
for the current 6.13 merge window.  I'm going to submit (and likely
merge) the patch for 6.14 once after 6.13 merge window is closed in
the next week.


thanks,

Takashi