| Message ID | 20241231115523.15796-1-tiwai@suse.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | ALSA: seq: oss: Send fragmented SysEx messages immediately | expand |
> 2024年12月31日 19:55，Takashi Iwai <tiwai@suse.de> 写道： > > 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@m.fudan.edu.cn> > Link: https://lore.kernel.org/2B7E93E4-B13A-4AE4-8E87-306A8EE9BBB7@m.fudan.edu.cn > Signed-off-by: Takashi Iwai <tiwai@suse.de> > --- > 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
>> > > > 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 > I see, you’re going to submit the patch to 6.14, but a temporary band-aid for 6.13. —— thanks Kun Hu
On Mon, 06 Jan 2025 04:10:01 +0100, Kun Hu wrote: > > >> > > > > > > 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 > > > > I see, you’re going to submit the patch to 6.14, but a temporary band-aid for 6.13. Yes. Although the second patch is cleaner, it is more intrusive and change the behavior, hence it needs more test coverage, which isn't suitable for 6.13 at this late stage. Takashi
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; } /*
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@m.fudan.edu.cn> Link: https://lore.kernel.org/2B7E93E4-B13A-4AE4-8E87-306A8EE9BBB7@m.fudan.edu.cn Signed-off-by: Takashi Iwai <tiwai@suse.de> --- 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(-)