Re: [PATCH] ALSA: seq: oss: Send fragmented SysEx messages immediately

[Kun Hu <huk23@xxxxxxxxxxxxxx>]

> 2024年12月31日 19:55，Takashi Iwai <tiwai@xxxxxxx> 写道：
> 
> The recent bug report spotted on the old OSS sequencer code that tries
> to combine incoming SysEx messages to a single sequencer event.  This
> is good, per se, but it has more demerits:
> 
> - The sysex message delivery is delayed until the very last event
> - The use of internal buffer forced the serialization
> 
> The recent fix in commit 0179488ca992 ("ALSA: seq: oss: Fix races at
> processing SysEx messages") addressed the latter, but a better fix is
> to handle the sysex messages immediately, i.e. just send each incoming
> fragmented sysex message as is.  And this patch implements that.
> This resulted in a significant cleanup as well.
> 
> Note that the only caller of snd_seq_oss_synth_sysex() is
> snd_seq_oss_process_event(), and all its callers dispatch the event
> immediately, so we can just put the passed buffer pointer to the event
> record to be handled.
> 
> Reported-and-tested-by: Kun Hu <huk23@xxxxxxxxxxxxxx>
> Link: https://lore.kernel.org/2B7E93E4-B13A-4AE4-8E87-306A8EE9BBB7@xxxxxxxxxxxxxx
> Signed-off-by: Takashi Iwai <tiwai@xxxxxxx>
> ---
> sound/core/seq/oss/seq_oss_device.h |  1 -
> sound/core/seq/oss/seq_oss_synth.c  | 80 +++++------------------------
> 2 files changed, 14 insertions(+), 67 deletions(-)
> 
> diff --git a/sound/core/seq/oss/seq_oss_device.h b/sound/core/seq/oss/seq_oss_device.h
> index 98dd20b42976..c0b1cce127a3 100644
> --- a/sound/core/seq/oss/seq_oss_device.h
> +++ b/sound/core/seq/oss/seq_oss_device.h
> @@ -55,7 +55,6 @@ struct seq_oss_chinfo {
> struct seq_oss_synthinfo {
> struct snd_seq_oss_arg arg;
> struct seq_oss_chinfo *ch;
> - struct seq_oss_synth_sysex *sysex;
> int nr_voices;
> int opened;
> int is_midi;
> diff --git a/sound/core/seq/oss/seq_oss_synth.c b/sound/core/seq/oss/seq_oss_synth.c
> index 51ee4c00a843..02a90c960992 100644
> --- a/sound/core/seq/oss/seq_oss_synth.c
> +++ b/sound/core/seq/oss/seq_oss_synth.c
> @@ -26,13 +26,6 @@
>  * definition of synth info records
>  */
> 
> -/* sysex buffer */
> -struct seq_oss_synth_sysex {
> - int len;
> - int skip;
> - unsigned char buf[MAX_SYSEX_BUFLEN];
> -};
> -
> /* synth info */
> struct seq_oss_synth {
> int seq_device;
> @@ -66,7 +59,6 @@ static struct seq_oss_synth midi_synth_dev = {
> };
> 
> static DEFINE_SPINLOCK(register_lock);
> -static DEFINE_MUTEX(sysex_mutex);
> 
> /*
>  * prototypes
> @@ -319,8 +311,6 @@ snd_seq_oss_synth_cleanup(struct seq_oss_devinfo *dp)
> }
> snd_use_lock_free(&rec->use_lock);
> }
> - kfree(info->sysex);
> - info->sysex = NULL;
> kfree(info->ch);
> info->ch = NULL;
> }
> @@ -396,8 +386,6 @@ snd_seq_oss_synth_reset(struct seq_oss_devinfo *dp, int dev)
> info = get_synthinfo_nospec(dp, dev);
> if (!info || !info->opened)
> return;
> - if (info->sysex)
> - info->sysex->len = 0; /* reset sysex */
> reset_channels(info);
> if (info->is_midi) {
> if (midi_synth_dev.opened <= 0)
> @@ -409,8 +397,6 @@ snd_seq_oss_synth_reset(struct seq_oss_devinfo *dp, int dev)
>  dp->file_mode) < 0) {
> midi_synth_dev.opened--;
> info->opened = 0;
> - kfree(info->sysex);
> - info->sysex = NULL;
> kfree(info->ch);
> info->ch = NULL;
> }
> @@ -483,64 +469,26 @@ snd_seq_oss_synth_info(struct seq_oss_devinfo *dp, int dev)
> 
> /*
>  * receive OSS 6 byte sysex packet:
> - * the full sysex message will be sent if it reaches to the end of data
> - * (0xff).
> + * the event is filled and prepared for sending immediately
> + * (i.e. sysex messages are fragmented)
>  */
> int
> snd_seq_oss_synth_sysex(struct seq_oss_devinfo *dp, int dev, unsigned char *buf, struct snd_seq_event *ev)
> {
> - int i, send;
> - unsigned char *dest;
> - struct seq_oss_synth_sysex *sysex;
> - struct seq_oss_synthinfo *info;
> + unsigned char *p;
> + int len = 6;
> 
> - info = snd_seq_oss_synth_info(dp, dev);
> - if (!info)
> - return -ENXIO;
> + p = memchr(buf, 0xff, 6);
> + if (p)
> + len = p - buf + 1;
> 
> - guard(mutex)(&sysex_mutex);
> - sysex = info->sysex;
> - if (sysex == NULL) {
> - sysex = kzalloc(sizeof(*sysex), GFP_KERNEL);
> - if (sysex == NULL)
> - return -ENOMEM;
> - info->sysex = sysex;
> - }
> -
> - send = 0;
> - dest = sysex->buf + sysex->len;
> - /* copy 6 byte packet to the buffer */
> - for (i = 0; i < 6; i++) {
> - if (buf[i] == 0xff) {
> - send = 1;
> - break;
> - }
> - dest[i] = buf[i];
> - sysex->len++;
> - if (sysex->len >= MAX_SYSEX_BUFLEN) {
> - sysex->len = 0;
> - sysex->skip = 1;
> - break;
> - }
> - }
> -
> - if (sysex->len && send) {
> - if (sysex->skip) {
> - sysex->skip = 0;
> - sysex->len = 0;
> - return -EINVAL; /* skip */
> - }
> - /* copy the data to event record and send it */
> - ev->flags = SNDRV_SEQ_EVENT_LENGTH_VARIABLE;
> - if (snd_seq_oss_synth_addr(dp, dev, ev))
> - return -EINVAL;
> - ev->data.ext.len = sysex->len;
> - ev->data.ext.ptr = sysex->buf;
> - sysex->len = 0;
> - return 0;
> - }
> -
> - return -EINVAL; /* skip */
> + /* copy the data to event record and send it */
> + if (snd_seq_oss_synth_addr(dp, dev, ev))
> + return -EINVAL;
> + ev->flags = SNDRV_SEQ_EVENT_LENGTH_VARIABLE;
> + ev->data.ext.len = len;
> + ev->data.ext.ptr = buf;
> + return 0;
> }
> 
> /*
> -- 
> 2.43.0
> 
> 


Hi Takashi Iwai,
I noticed that in your previous modifications, you seemed to have made lots of changes to the related structures, along with many updates to the snd_seq_oss_synth_sysex function. However, in the latest kernel tree, it seems that only a mutex lock has been added to this function.
While this certainly resolves the issue, I’m personally curious to understand the reasoning behind this approach. Was there a specific consideration or constraint that led to this simplified solution?

——
Thankx
Kun hu