[PATCH v3 00/17] ALSA: emu10k1 & emux: fixes related to wavetable playback

[PATCH v3 00/17] ALSA: emu10k1 & emux: fixes related to wavetable playback

[Oswald Buddenhagen]

---
v3:
- re-send with safer transfer encoding
v2:
- rebased on top of reverting problematic commit first

Oswald Buddenhagen (17):
  ALSA: emux: fix /proc teardown at module unload
  ALSA: emux: prune unused parameter from snd_soundfont_load_guspatch()
  ALSA: emux: fix validation of snd_emux.num_ports
  ALSA: emux: fix init of patch_info.truesize in load_data()
  ALSA: emu10k1: prune vestiges of
    SNDRV_SFNT_SAMPLE_{BIDIR,REVERSE}_LOOP support
  ALSA: emux: centralize & improve patch info validation
  ALSA: emux: improve patch ioctl data validation
  ALSA: emu10k1: move patch loader assertions into low-level functions
  ALSA: emu10k1: fix sample signedness issues in wavetable loader
  ALSA: emu10k1: fix playback of 8-bit wavetable samples
  ALSA: emu10k1: merge conditions in patch loader
  ALSA: emu10k1: fix wavetable offset recalculation
  ALSA: emu10k1: de-duplicate size calculations for 16-bit samples
  ALSA: emu10k1: improve cache behavior documentation
  ALSA: emu10k1: fix wavetable playback position and caching, take 2
  ALSA: emu10k1: shrink blank space in front of wavetable samples
  ALSA: emux: simplify snd_sf_list.callback handling

 include/sound/emu10k1.h              |  32 +++--
 include/sound/soundfont.h            |   2 +-
 sound/isa/sb/emu8000_patch.c         |  13 --
 sound/pci/emu10k1/emu10k1_callback.c |  13 +-
 sound/pci/emu10k1/emu10k1_patch.c    | 207 +++++++++++----------------
 sound/pci/emu10k1/memory.c           |  55 +++++--
 sound/synth/emux/emux.c              |   6 +-
 sound/synth/emux/emux_hwdep.c        |   3 +-
 sound/synth/emux/emux_oss.c          |   3 +-
 sound/synth/emux/emux_proc.c         |   1 +
 sound/synth/emux/emux_seq.c          |   6 +-
 sound/synth/emux/soundfont.c         |  73 +++++++---
 12 files changed, 219 insertions(+), 195 deletions(-)

-- 
2.44.0.701.g2cf7baacf3.dirty

[PATCH v3 01/17] ALSA: emux: fix /proc teardown at module unload

[Oswald Buddenhagen]

We forgot to remember the wavetable /proc entry, so we'd fail to free it
at module unload.

This matters only when only the synth module is unloaded, as unloading
the card driver would tear down the sub-entry anyway.

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
 sound/synth/emux/emux_proc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/sound/synth/emux/emux_proc.c b/sound/synth/emux/emux_proc.c
index 7993e6a01e54..820351f52551 100644
--- a/sound/synth/emux/emux_proc.c
+++ b/sound/synth/emux/emux_proc.c
@@ -102,6 +102,7 @@ void snd_emux_proc_init(struct snd_emux *emu, struct snd_card *card, int device)
 	entry->content = SNDRV_INFO_CONTENT_TEXT;
 	entry->private_data = emu;
 	entry->c.text.read = snd_emux_proc_info_read;
+	emu->proc = entry;
 }
 
 void snd_emux_proc_free(struct snd_emux *emu)
-- 
2.44.0.701.g2cf7baacf3.dirty

[PATCH v3 02/17] ALSA: emux: prune unused parameter from snd_soundfont_load_guspatch()

[Oswald Buddenhagen]

The `client` parameter was not used, so eliminate it from the call
chain.

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
 include/sound/soundfont.h     | 2 +-
 sound/synth/emux/emux_hwdep.c | 3 +--
 sound/synth/emux/emux_oss.c   | 3 +--
 sound/synth/emux/soundfont.c  | 7 +++----
 4 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/include/sound/soundfont.h b/include/sound/soundfont.h
index e445688a4f4f..98ed98d89d6d 100644
--- a/include/sound/soundfont.h
+++ b/include/sound/soundfont.h
@@ -89,7 +89,7 @@ struct snd_sf_list {
 int snd_soundfont_load(struct snd_sf_list *sflist, const void __user *data,
 		       long count, int client);
 int snd_soundfont_load_guspatch(struct snd_sf_list *sflist, const char __user *data,
-				long count, int client);
+				long count);
 int snd_soundfont_close_check(struct snd_sf_list *sflist, int client);
 
 struct snd_sf_list *snd_sf_new(struct snd_sf_callback *callback,
diff --git a/sound/synth/emux/emux_hwdep.c b/sound/synth/emux/emux_hwdep.c
index 81719bfb8ed7..fd8f978cde1c 100644
--- a/sound/synth/emux/emux_hwdep.c
+++ b/sound/synth/emux/emux_hwdep.c
@@ -27,8 +27,7 @@ snd_emux_hwdep_load_patch(struct snd_emux *emu, void __user *arg)
 
 	if (patch.key == GUS_PATCH)
 		return snd_soundfont_load_guspatch(emu->sflist, arg,
-						   patch.len + sizeof(patch),
-						   TMP_CLIENT_ID);
+						   patch.len + sizeof(patch));
 
 	if (patch.type >= SNDRV_SFNT_LOAD_INFO &&
 	    patch.type <= SNDRV_SFNT_PROBE_DATA) {
diff --git a/sound/synth/emux/emux_oss.c b/sound/synth/emux/emux_oss.c
index d8d32671f703..04df46b269d3 100644
--- a/sound/synth/emux/emux_oss.c
+++ b/sound/synth/emux/emux_oss.c
@@ -205,8 +205,7 @@ snd_emux_load_patch_seq_oss(struct snd_seq_oss_arg *arg, int format,
 		return -ENXIO;
 
 	if (format == GUS_PATCH)
-		rc = snd_soundfont_load_guspatch(emu->sflist, buf, count,
-						 SF_CLIENT_NO(p->chset.port));
+		rc = snd_soundfont_load_guspatch(emu->sflist, buf, count);
 	else if (format == SNDRV_OSS_SOUNDFONT_PATCH) {
 		struct soundfont_patch_info patch;
 		if (count < (int)sizeof(patch))
diff --git a/sound/synth/emux/soundfont.c b/sound/synth/emux/soundfont.c
index 16f00097cb95..e1e47518ac92 100644
--- a/sound/synth/emux/soundfont.c
+++ b/sound/synth/emux/soundfont.c
@@ -941,8 +941,7 @@ int snd_sf_vol_table[128] = {
 
 /* load GUS patch */
 static int
-load_guspatch(struct snd_sf_list *sflist, const char __user *data,
-	      long count, int client)
+load_guspatch(struct snd_sf_list *sflist, const char __user *data, long count)
 {
 	struct patch_info patch;
 	struct snd_soundfont *sf;
@@ -1122,11 +1121,11 @@ load_guspatch(struct snd_sf_list *sflist, const char __user *data,
 /* load GUS patch */
 int
 snd_soundfont_load_guspatch(struct snd_sf_list *sflist, const char __user *data,
-			    long count, int client)
+			    long count)
 {
 	int rc;
 	lock_preset(sflist);
-	rc = load_guspatch(sflist, data, count, client);
+	rc = load_guspatch(sflist, data, count);
 	unlock_preset(sflist);
 	return rc;
 }
-- 
2.44.0.701.g2cf7baacf3.dirty

[PATCH v3 03/17] ALSA: emux: fix validation of snd_emux.num_ports

[Oswald Buddenhagen]

Both bounds had off-by-one errors.

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
 sound/synth/emux/emux_seq.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/sound/synth/emux/emux_seq.c b/sound/synth/emux/emux_seq.c
index b227c7e0bc2a..1adaa75df2f6 100644
--- a/sound/synth/emux/emux_seq.c
+++ b/sound/synth/emux/emux_seq.c
@@ -65,11 +65,11 @@ snd_emux_init_seq(struct snd_emux *emu, struct snd_card *card, int index)
 		return -ENODEV;
 	}
 
-	if (emu->num_ports < 0) {
+	if (emu->num_ports <= 0) {
 		snd_printk(KERN_WARNING "seqports must be greater than zero\n");
 		emu->num_ports = 1;
-	} else if (emu->num_ports >= SNDRV_EMUX_MAX_PORTS) {
-		snd_printk(KERN_WARNING "too many ports."
+	} else if (emu->num_ports > SNDRV_EMUX_MAX_PORTS) {
+		snd_printk(KERN_WARNING "too many ports. "
 			   "limited max. ports %d\n", SNDRV_EMUX_MAX_PORTS);
 		emu->num_ports = SNDRV_EMUX_MAX_PORTS;
 	}
-- 
2.44.0.701.g2cf7baacf3.dirty

[PATCH v3 04/17] ALSA: emux: fix init of patch_info.truesize in load_data()

[Oswald Buddenhagen]

The field is explicitly documented to be initialized by the driver
(which it actually is). Also, using patch_info.size would be actually
wrong for 16-bit data, as one field counts samples, while the other
counts bytes.

load_guspatch() already did it right.

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
 sound/synth/emux/soundfont.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/synth/emux/soundfont.c b/sound/synth/emux/soundfont.c
index e1e47518ac92..ad0231d7a39d 100644
--- a/sound/synth/emux/soundfont.c
+++ b/sound/synth/emux/soundfont.c
@@ -735,7 +735,7 @@ load_data(struct snd_sf_list *sflist, const void __user *data, long count)
 	sp->v = sample_info;
 	sp->v.sf_id = sf->id;
 	sp->v.dummy = 0;
-	sp->v.truesize = sp->v.size;
+	sp->v.truesize = 0;
 
 	/*
 	 * If there is wave data then load it.
-- 
2.44.0.701.g2cf7baacf3.dirty

[PATCH v3 05/17] ALSA: emu10k1: prune vestiges of SNDRV_SFNT_SAMPLE_{BIDIR,REVERSE}_LOOP support

[Oswald Buddenhagen]

This is required only to implement WAVE_BIDIR_LOOP and WAVE_LOOP_BACK in
the GUS patch loader. It has not worked on emu10k1 since before ALSA hit
mainline, yet nobody appears to have complained. And as it isn't super
easy to implement, just admit defeat and clean up the code.

If somebody wanted to resurrect the feature, the emu8k driver could
serve as a template, but the code would be quite different. But
arguably, this should be done in user space in the first place, as this
doesn't represent a hardware feature (somewhat ironically, the actual
GUS driver has no synth support, and therefore no GUS patch loader).

Note that instead of properly rejecting affected samples, we continue to
just pretend that the feature wasn't requested. This is extremely
questionable behavior, but avoids that possibly unused instruments
suddenly prevent loading the entire file, which would break backwards
compatibility. But at least we log a warning now.

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
 sound/pci/emu10k1/emu10k1_patch.c | 73 ++++---------------------------
 1 file changed, 8 insertions(+), 65 deletions(-)

diff --git a/sound/pci/emu10k1/emu10k1_patch.c b/sound/pci/emu10k1/emu10k1_patch.c
index 89890f24509f..49214c226808 100644
--- a/sound/pci/emu10k1/emu10k1_patch.c
+++ b/sound/pci/emu10k1/emu10k1_patch.c
@@ -28,8 +28,6 @@ snd_emu10k1_sample_new(struct snd_emux *rec, struct snd_sf_sample *sp,
 {
 	int offset;
 	int truesize, size, blocksize;
-	__maybe_unused int loopsize;
-	int loopend, sampleend;
 	unsigned int start_addr;
 	struct snd_emu10k1 *emu;
 
@@ -43,32 +41,24 @@ snd_emu10k1_sample_new(struct snd_emux *rec, struct snd_sf_sample *sp,
 		return 0;
 	}
 
+	if (sp->v.mode_flags & (SNDRV_SFNT_SAMPLE_BIDIR_LOOP | SNDRV_SFNT_SAMPLE_REVERSE_LOOP)) {
+		/* should instead return -ENOTSUPP; but compatibility */
+		printk(KERN_WARNING "Emu10k1 wavetable patch %d with unsupported loop feature\n",
+		       sp->v.sample);
+	}
+
 	/* recalculate address offset */
 	sp->v.end -= sp->v.start;
 	sp->v.loopstart -= sp->v.start;
 	sp->v.loopend -= sp->v.start;
 	sp->v.start = 0;
 
-	/* some samples have invalid data.  the addresses are corrected in voice info */
-	sampleend = sp->v.end;
-	if (sampleend > sp->v.size)
-		sampleend = sp->v.size;
-	loopend = sp->v.loopend;
-	if (loopend > sampleend)
-		loopend = sampleend;
-
 	/* be sure loop points start < end */
 	if (sp->v.loopstart >= sp->v.loopend)
 		swap(sp->v.loopstart, sp->v.loopend);
 
 	/* compute true data size to be loaded */
 	truesize = sp->v.size + BLANK_HEAD_SIZE;
-	loopsize = 0;
-#if 0 /* not supported */
-	if (sp->v.mode_flags & (SNDRV_SFNT_SAMPLE_BIDIR_LOOP|SNDRV_SFNT_SAMPLE_REVERSE_LOOP))
-		loopsize = sp->v.loopend - sp->v.loopstart;
-	truesize += loopsize;
-#endif
 	if (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_NO_BLANK)
 		truesize += BLANK_LOOP_SIZE;
 
@@ -96,65 +86,18 @@ snd_emu10k1_sample_new(struct snd_emux *rec, struct snd_sf_sample *sp,
 	snd_emu10k1_synth_bzero(emu, sp->block, offset, size);
 	offset += size;
 
-	/* copy start->loopend */
-	size = loopend;
+	/* copy provided samples */
+	size = sp->v.size;
 	if (! (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_8BITS))
 		size *= 2;
 	if (offset + size > blocksize)
 		return -EINVAL;
 	if (snd_emu10k1_synth_copy_from_user(emu, sp->block, offset, data, size)) {
 		snd_emu10k1_synth_free(emu, sp->block);
 		sp->block = NULL;
 		return -EFAULT;
 	}
 	offset += size;
-	data += size;
-
-#if 0 /* not supported yet */
-	/* handle reverse (or bidirectional) loop */
-	if (sp->v.mode_flags & (SNDRV_SFNT_SAMPLE_BIDIR_LOOP|SNDRV_SFNT_SAMPLE_REVERSE_LOOP)) {
-		/* copy loop in reverse */
-		if (! (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_8BITS)) {
-			int woffset;
-			unsigned short *wblock = (unsigned short*)block;
-			woffset = offset / 2;
-			if (offset + loopsize * 2 > blocksize)
-				return -EINVAL;
-			for (i = 0; i < loopsize; i++)
-				wblock[woffset + i] = wblock[woffset - i -1];
-			offset += loopsize * 2;
-		} else {
-			if (offset + loopsize > blocksize)
-				return -EINVAL;
-			for (i = 0; i < loopsize; i++)
-				block[offset + i] = block[offset - i -1];
-			offset += loopsize;
-		}
-
-		/* modify loop pointers */
-		if (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_BIDIR_LOOP) {
-			sp->v.loopend += loopsize;
-		} else {
-			sp->v.loopstart += loopsize;
-			sp->v.loopend += loopsize;
-		}
-		/* add sample pointer */
-		sp->v.end += loopsize;
-	}
-#endif
-
-	/* loopend -> sample end */
-	size = sp->v.size - loopend;
-	if (size < 0)
-		return -EINVAL;
-	if (! (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_8BITS))
-		size *= 2;
-	if (snd_emu10k1_synth_copy_from_user(emu, sp->block, offset, data, size)) {
-		snd_emu10k1_synth_free(emu, sp->block);
-		sp->block = NULL;
-		return -EFAULT;
-	}
-	offset += size;
 
 	/* clear rest of samples (if any) */
 	if (offset < blocksize)
-- 
2.44.0.701.g2cf7baacf3.dirty

[PATCH v3 06/17] ALSA: emux: centralize & improve patch info validation

[Oswald Buddenhagen]

This does several closely related things:
- Move the code from the drivers into the SoundFont loader, which
  de-duplicates it.
- Sort of explain the weird "recalculate address offset" feature. Note
  that I don't think it actually makes any sense - the calling user
  space code should do that. The background is certainly that the source
  data (the SoundFont format) uses pointers into a single wave block
  (and the API allows doing the same for on-board ROM), but the API
  expects the wave data from user space to be pre-chopped into
  individual patches anyway.
- Make sure that the specified offsets actually lie within the supplied
  wave data. Note that we don't validate ROM offsets, so one can play
  back anything within the sound card's address space.
- In load_guspatch(), don't call the sample_new callback anymore when
  the patch size is zero, as was already the case in load_data(). The
  callbacks would instantly return in that case anyway; these checks are
  now removed.

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
 sound/isa/sb/emu8000_patch.c      | 13 -----------
 sound/pci/emu10k1/emu10k1_patch.c | 16 -------------
 sound/synth/emux/soundfont.c      | 37 ++++++++++++++++++++++++++++++-
 3 files changed, 36 insertions(+), 30 deletions(-)

diff --git a/sound/isa/sb/emu8000_patch.c b/sound/isa/sb/emu8000_patch.c
index 8c1e7f2bfc34..ab4f988f080d 100644
--- a/sound/isa/sb/emu8000_patch.c
+++ b/sound/isa/sb/emu8000_patch.c
@@ -148,13 +148,6 @@ snd_emu8000_sample_new(struct snd_emux *rec, struct snd_sf_sample *sp,
 	if (snd_BUG_ON(!sp))
 		return -EINVAL;
 
-	if (sp->v.size == 0)
-		return 0;
-
-	/* be sure loop points start < end */
-	if (sp->v.loopstart > sp->v.loopend)
-		swap(sp->v.loopstart, sp->v.loopend);
-
 	/* compute true data size to be loaded */
 	truesize = sp->v.size;
 	if (sp->v.mode_flags & (SNDRV_SFNT_SAMPLE_BIDIR_LOOP|SNDRV_SFNT_SAMPLE_REVERSE_LOOP))
@@ -177,12 +170,6 @@ snd_emu8000_sample_new(struct snd_emux *rec, struct snd_sf_sample *sp,
 			return -EFAULT;
 	}
 
-	/* recalculate address offset */
-	sp->v.end -= sp->v.start;
-	sp->v.loopstart -= sp->v.start;
-	sp->v.loopend -= sp->v.start;
-	sp->v.start = 0;
-
 	/* dram position (in word) -- mem_offset is byte */
 	dram_offset = EMU8000_DRAM_OFFSET + (sp->block->offset >> 1);
 	dram_start = dram_offset;
diff --git a/sound/pci/emu10k1/emu10k1_patch.c b/sound/pci/emu10k1/emu10k1_patch.c
index 49214c226808..47d69a0e44bc 100644
--- a/sound/pci/emu10k1/emu10k1_patch.c
+++ b/sound/pci/emu10k1/emu10k1_patch.c
@@ -35,28 +35,12 @@ snd_emu10k1_sample_new(struct snd_emux *rec, struct snd_sf_sample *sp,
 	if (snd_BUG_ON(!sp || !hdr))
 		return -EINVAL;
 
-	if (sp->v.size == 0) {
-		dev_dbg(emu->card->dev,
-			"emu: rom font for sample %d\n", sp->v.sample);
-		return 0;
-	}
-
 	if (sp->v.mode_flags & (SNDRV_SFNT_SAMPLE_BIDIR_LOOP | SNDRV_SFNT_SAMPLE_REVERSE_LOOP)) {
 		/* should instead return -ENOTSUPP; but compatibility */
 		printk(KERN_WARNING "Emu10k1 wavetable patch %d with unsupported loop feature\n",
 		       sp->v.sample);
 	}
 
-	/* recalculate address offset */
-	sp->v.end -= sp->v.start;
-	sp->v.loopstart -= sp->v.start;
-	sp->v.loopend -= sp->v.start;
-	sp->v.start = 0;
-
-	/* be sure loop points start < end */
-	if (sp->v.loopstart >= sp->v.loopend)
-		swap(sp->v.loopstart, sp->v.loopend);
-
 	/* compute true data size to be loaded */
 	truesize = sp->v.size + BLANK_HEAD_SIZE;
 	if (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_NO_BLANK)
diff --git a/sound/synth/emux/soundfont.c b/sound/synth/emux/soundfont.c
index ad0231d7a39d..6d6f0102ed5b 100644
--- a/sound/synth/emux/soundfont.c
+++ b/sound/synth/emux/soundfont.c
@@ -689,6 +689,21 @@ find_sample(struct snd_soundfont *sf, int sample_id)
 }
 
 
+static int
+validate_sample_info(struct soundfont_sample_info *si)
+{
+	if (si->end < 0 || si->end > si->size)
+		return -EINVAL;
+	if (si->loopstart < 0 || si->loopstart > si->end)
+		return -EINVAL;
+	if (si->loopend < 0 || si->loopend > si->end)
+		return -EINVAL;
+	/* be sure loop points start < end */
+	if (si->loopstart > si->loopend)
+		swap(si->loopstart, si->loopend);
+	return 0;
+}
+
 /*
  * Load sample information, this can include data to be loaded onto
  * the soundcard.  It can also just be a pointer into soundcard ROM.
@@ -727,6 +742,21 @@ load_data(struct snd_sf_list *sflist, const void __user *data, long count)
 		return -EINVAL;
 	}
 
+	if (sample_info.size > 0) {
+		if (sample_info.start < 0)
+			return -EINVAL;
+
+		// Here we "rebase out" the start address, because the
+		// real start is the start of the provided sample data.
+		sample_info.end -= sample_info.start;
+		sample_info.loopstart -= sample_info.start;
+		sample_info.loopend -= sample_info.start;
+		sample_info.start = 0;
+
+		if (validate_sample_info(&sample_info) < 0)
+			return -EINVAL;
+	}
+
 	/* Allocate a new sample structure */
 	sp = sf_sample_new(sflist, sf);
 	if (!sp)
@@ -974,6 +1004,11 @@ load_guspatch(struct snd_sf_list *sflist, const char __user *data, long count)
 	smp->v.loopend = patch.loop_end;
 	smp->v.size = patch.len;
 
+	if (validate_sample_info(&smp->v) < 0) {
+		sf_sample_delete(sflist, sf, smp);
+		return -EINVAL;
+	}
+
 	/* set up mode flags */
 	smp->v.mode_flags = 0;
 	if (!(patch.mode & WAVE_16_BITS))
@@ -1011,7 +1046,7 @@ load_guspatch(struct snd_sf_list *sflist, const char __user *data, long count)
 	/*
 	 * load wave data
 	 */
-	if (sflist->callback.sample_new) {
+	if (smp->v.size > 0 && sflist->callback.sample_new) {
 		rc = sflist->callback.sample_new
 			(sflist->callback.private_data, smp, sflist->memhdr,
 			 data, count);
-- 
2.44.0.701.g2cf7baacf3.dirty

[PATCH v3 07/17] ALSA: emux: improve patch ioctl data validation

[Oswald Buddenhagen]

In load_data(), make the validation of and skipping over the main info
block match that in load_guspatch().

In load_guspatch(), add checking that the specified patch length matches
the actually supplied data, like load_data() already did.

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
 sound/synth/emux/soundfont.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/sound/synth/emux/soundfont.c b/sound/synth/emux/soundfont.c
index 6d6f0102ed5b..4edc693da8e7 100644
--- a/sound/synth/emux/soundfont.c
+++ b/sound/synth/emux/soundfont.c
@@ -716,22 +716,25 @@ load_data(struct snd_sf_list *sflist, const void __user *data, long count)
 	struct snd_soundfont *sf;
 	struct soundfont_sample_info sample_info;
 	struct snd_sf_sample *sp;
-	long off;
 
 	/* patch must be opened */
 	sf = sflist->currsf;
 	if (!sf)
 		return -EINVAL;
 
 	if (is_special_type(sf->type))
 		return -EINVAL;
 
+	if (count < (long)sizeof(sample_info)) {
+		return -EINVAL;
+	}
 	if (copy_from_user(&sample_info, data, sizeof(sample_info)))
 		return -EFAULT;
+	data += sizeof(sample_info);
+	count -= sizeof(sample_info);
 
-	off = sizeof(sample_info);
-
-	if (sample_info.size != (count-off)/2)
+	// SoundFont uses S16LE samples.
+	if (sample_info.size * 2 != count)
 		return -EINVAL;
 
 	/* Check for dup */
@@ -774,7 +777,7 @@ load_data(struct snd_sf_list *sflist, const void __user *data, long count)
 		int  rc;
 		rc = sflist->callback.sample_new
 			(sflist->callback.private_data, sp, sflist->memhdr,
-			 data + off, count - off);
+			 data, count);
 		if (rc < 0) {
 			sf_sample_delete(sflist, sf, sp);
 			return rc;
@@ -986,10 +989,12 @@ load_guspatch(struct snd_sf_list *sflist, const char __user *data, long count)
 	}
 	if (copy_from_user(&patch, data, sizeof(patch)))
 		return -EFAULT;
-	
 	count -= sizeof(patch);
 	data += sizeof(patch);
 
+	if ((patch.len << (patch.mode & WAVE_16_BITS ? 1 : 0)) != count)
+		return -EINVAL;
+
 	sf = newsf(sflist, SNDRV_SFNT_PAT_TYPE_GUS|SNDRV_SFNT_PAT_SHARED, NULL);
 	if (sf == NULL)
 		return -ENOMEM;
-- 
2.44.0.701.g2cf7baacf3.dirty

[PATCH v3 08/17] ALSA: emu10k1: move patch loader assertions into low-level functions

[Oswald Buddenhagen]

Convert some checks in snd_emu10k1_sample_new() back into assertions (as
they were prior to da3cec35dd (ALSA: Kill snd_assert() in sound/pci/*,
2008-08-08)), and move them into the low-level memory access functions
they protect.

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>

---

Side note: this eliminates the memory leaks in the now gone error paths.
I don't think it was actually possible to trigger these even before the
foregoing cleanups. But if it were, it would allow a user with access to
the audio device a scope-limited DoS attack on it. This would be only a
very minor security hole, given that on modern systems it would merely
enable the current seat owner to be a nuisance to their successor, by
making a reboot necessary.
---
 sound/pci/emu10k1/emu10k1_patch.c | 4 ----
 sound/pci/emu10k1/memory.c        | 6 ++++++
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/sound/pci/emu10k1/emu10k1_patch.c b/sound/pci/emu10k1/emu10k1_patch.c
index 47d69a0e44bc..55bb60d31fe4 100644
--- a/sound/pci/emu10k1/emu10k1_patch.c
+++ b/sound/pci/emu10k1/emu10k1_patch.c
@@ -65,17 +65,13 @@ snd_emu10k1_sample_new(struct snd_emux *rec, struct snd_sf_sample *sp,
 	size = BLANK_HEAD_SIZE;
 	if (! (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_8BITS))
 		size *= 2;
-	if (offset + size > blocksize)
-		return -EINVAL;
 	snd_emu10k1_synth_bzero(emu, sp->block, offset, size);
 	offset += size;
 
 	/* copy provided samples */
 	size = sp->v.size;
 	if (! (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_8BITS))
 		size *= 2;
-	if (offset + size > blocksize)
-		return -EINVAL;
 	if (snd_emu10k1_synth_copy_from_user(emu, sp->block, offset, data, size)) {
 		snd_emu10k1_synth_free(emu, sp->block);
 		sp->block = NULL;
diff --git a/sound/pci/emu10k1/memory.c b/sound/pci/emu10k1/memory.c
index 20b07117574b..fc9444404151 100644
--- a/sound/pci/emu10k1/memory.c
+++ b/sound/pci/emu10k1/memory.c
@@ -574,6 +574,9 @@ int snd_emu10k1_synth_bzero(struct snd_emu10k1 *emu, struct snd_util_memblk *blk
 	void *ptr;
 	struct snd_emu10k1_memblk *p = (struct snd_emu10k1_memblk *)blk;
 
+	if (snd_BUG_ON(offset + size > p->mem.size))
+		return -EFAULT;
+
 	offset += blk->offset & (PAGE_SIZE - 1);
 	end_offset = offset + size;
 	page = get_aligned_page(offset);
@@ -604,6 +607,9 @@ int snd_emu10k1_synth_copy_from_user(struct snd_emu10k1 *emu, struct snd_util_me
 	void *ptr;
 	struct snd_emu10k1_memblk *p = (struct snd_emu10k1_memblk *)blk;
 
+	if (snd_BUG_ON(offset + size > p->mem.size))
+		return -EFAULT;
+
 	offset += blk->offset & (PAGE_SIZE - 1);
 	end_offset = offset + size;
 	page = get_aligned_page(offset);
-- 
2.44.0.701.g2cf7baacf3.dirty

[PATCH v3 09/17] ALSA: emu10k1: fix sample signedness issues in wavetable loader

[Oswald Buddenhagen]

The hardware supports S16LE and U8 samples, while U16LE and S8 (which
the driver implicitly claims to support) require sign flipping.

Note that this matters only for the GUS patch loader, as the implemented
SoundFont v2.01 spec is limited to S16LE.

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
 include/sound/emu10k1.h           |  4 +--
 sound/pci/emu10k1/emu10k1_patch.c | 30 ++++++++-----------
 sound/pci/emu10k1/memory.c        | 49 +++++++++++++++++++++++++------
 3 files changed, 55 insertions(+), 28 deletions(-)

diff --git a/include/sound/emu10k1.h b/include/sound/emu10k1.h
index 1af9e6819392..9e3bd4f81460 100644
--- a/include/sound/emu10k1.h
+++ b/include/sound/emu10k1.h
@@ -1882,8 +1882,8 @@ int snd_emu10k1_alloc_pages_maybe_wider(struct snd_emu10k1 *emu, size_t size,
 					struct snd_dma_buffer *dmab);
 struct snd_util_memblk *snd_emu10k1_synth_alloc(struct snd_emu10k1 *emu, unsigned int size);
 int snd_emu10k1_synth_free(struct snd_emu10k1 *emu, struct snd_util_memblk *blk);
-int snd_emu10k1_synth_bzero(struct snd_emu10k1 *emu, struct snd_util_memblk *blk, int offset, int size);
-int snd_emu10k1_synth_copy_from_user(struct snd_emu10k1 *emu, struct snd_util_memblk *blk, int offset, const char __user *data, int size);
+int snd_emu10k1_synth_memset(struct snd_emu10k1 *emu, struct snd_util_memblk *blk, int offset, int size, u8 value);
+int snd_emu10k1_synth_copy_from_user(struct snd_emu10k1 *emu, struct snd_util_memblk *blk, int offset, const char __user *data, int size, u32 xor);
 int snd_emu10k1_memblk_map(struct snd_emu10k1 *emu, struct snd_emu10k1_memblk *blk);
 
 /* voice allocation */
diff --git a/sound/pci/emu10k1/emu10k1_patch.c b/sound/pci/emu10k1/emu10k1_patch.c
index 55bb60d31fe4..eb3d1ef8a33a 100644
--- a/sound/pci/emu10k1/emu10k1_patch.c
+++ b/sound/pci/emu10k1/emu10k1_patch.c
@@ -26,6 +26,8 @@ snd_emu10k1_sample_new(struct snd_emux *rec, struct snd_sf_sample *sp,
 		       struct snd_util_memhdr *hdr,
 		       const void __user *data, long count)
 {
+	u8 fill;
+	u32 xor;
 	int offset;
 	int truesize, size, blocksize;
 	unsigned int start_addr;
@@ -41,6 +43,14 @@ snd_emu10k1_sample_new(struct snd_emux *rec, struct snd_sf_sample *sp,
 		       sp->v.sample);
 	}
 
+	if (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_8BITS) {
+		fill = 0x80;
+		xor = (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_UNSIGNED) ? 0 : 0x80808080;
+	} else {
+		fill = 0;
+		xor = (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_UNSIGNED) ? 0x80008000 : 0;
+	}
+
 	/* compute true data size to be loaded */
 	truesize = sp->v.size + BLANK_HEAD_SIZE;
 	if (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_NO_BLANK)
@@ -65,46 +75,32 @@ snd_emu10k1_sample_new(struct snd_emux *rec, struct snd_sf_sample *sp,
 	size = BLANK_HEAD_SIZE;
 	if (! (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_8BITS))
 		size *= 2;
-	snd_emu10k1_synth_bzero(emu, sp->block, offset, size);
+	snd_emu10k1_synth_memset(emu, sp->block, offset, size, fill);
 	offset += size;
 
 	/* copy provided samples */
 	size = sp->v.size;
 	if (! (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_8BITS))
 		size *= 2;
-	if (snd_emu10k1_synth_copy_from_user(emu, sp->block, offset, data, size)) {
+	if (snd_emu10k1_synth_copy_from_user(emu, sp->block, offset, data, size, xor)) {
 		snd_emu10k1_synth_free(emu, sp->block);
 		sp->block = NULL;
 		return -EFAULT;
 	}
 	offset += size;
 
 	/* clear rest of samples (if any) */
 	if (offset < blocksize)
-		snd_emu10k1_synth_bzero(emu, sp->block, offset, blocksize - offset);
+		snd_emu10k1_synth_memset(emu, sp->block, offset, blocksize - offset, fill);
 
 	if (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_NO_BLANK) {
 		/* if no blank loop is attached in the sample, add it */
 		if (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_SINGLESHOT) {
 			sp->v.loopstart = sp->v.end + BLANK_LOOP_START;
 			sp->v.loopend = sp->v.end + BLANK_LOOP_END;
 		}
 	}
 
-#if 0 /* not supported yet */
-	if (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_UNSIGNED) {
-		/* unsigned -> signed */
-		if (! (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_8BITS)) {
-			unsigned short *wblock = (unsigned short*)block;
-			for (i = 0; i < truesize; i++)
-				wblock[i] ^= 0x8000;
-		} else {
-			for (i = 0; i < truesize; i++)
-				block[i] ^= 0x80;
-		}
-	}
-#endif
-
 	/* recalculate offset */
 	start_addr = BLANK_HEAD_SIZE * 2;
 	if (! (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_8BITS))
diff --git a/sound/pci/emu10k1/memory.c b/sound/pci/emu10k1/memory.c
index fc9444404151..d29711777161 100644
--- a/sound/pci/emu10k1/memory.c
+++ b/sound/pci/emu10k1/memory.c
@@ -565,10 +565,10 @@ static inline void *offset_ptr(struct snd_emu10k1 *emu, int page, int offset)
 }
 
 /*
- * bzero(blk + offset, size)
+ * memset(blk + offset, value, size)
  */
-int snd_emu10k1_synth_bzero(struct snd_emu10k1 *emu, struct snd_util_memblk *blk,
-			    int offset, int size)
+int snd_emu10k1_synth_memset(struct snd_emu10k1 *emu, struct snd_util_memblk *blk,
+			     int offset, int size, u8 value)
 {
 	int page, nextofs, end_offset, temp, temp1;
 	void *ptr;
@@ -588,20 +588,47 @@ int snd_emu10k1_synth_bzero(struct snd_emu10k1 *emu, struct snd_util_memblk *blk
 			temp = temp1;
 		ptr = offset_ptr(emu, page + p->first_page, offset);
 		if (ptr)
-			memset(ptr, 0, temp);
+			memset(ptr, value, temp);
 		offset = nextofs;
 		page++;
 	} while (offset < end_offset);
 	return 0;
 }
 
-EXPORT_SYMBOL(snd_emu10k1_synth_bzero);
+EXPORT_SYMBOL(snd_emu10k1_synth_memset);
+
+// Note that the value is assumed to be suitably repetitive.
+static void xor_range(void *ptr, int size, u32 value)
+{
+	if ((long)ptr & 1) {
+		*(u8 *)ptr ^= (u8)value;
+		ptr++;
+		size--;
+	}
+	if (size > 1 && ((long)ptr & 2)) {
+		*(u16 *)ptr ^= (u16)value;
+		ptr += 2;
+		size -= 2;
+	}
+	while (size > 3) {
+		*(u32 *)ptr ^= value;
+		ptr += 4;
+		size -= 4;
+	}
+	if (size > 1) {
+		*(u16 *)ptr ^= (u16)value;
+		ptr += 2;
+		size -= 2;
+	}
+	if (size > 0)
+		*(u8 *)ptr ^= (u8)value;
+}
 
 /*
- * copy_from_user(blk + offset, data, size)
+ * copy_from_user(blk + offset, data, size) ^ xor
  */
 int snd_emu10k1_synth_copy_from_user(struct snd_emu10k1 *emu, struct snd_util_memblk *blk,
-				     int offset, const char __user *data, int size)
+				     int offset, const char __user *data, int size, u32 xor)
 {
 	int page, nextofs, end_offset, temp, temp1;
 	void *ptr;
@@ -620,8 +647,12 @@ int snd_emu10k1_synth_copy_from_user(struct snd_emu10k1 *emu, struct snd_util_me
 		if (temp1 < temp)
 			temp = temp1;
 		ptr = offset_ptr(emu, page + p->first_page, offset);
-		if (ptr && copy_from_user(ptr, data, temp))
-			return -EFAULT;
+		if (ptr) {
+			if (copy_from_user(ptr, data, temp))
+				return -EFAULT;
+			if (xor)
+				xor_range(ptr, temp, xor);
+		}
 		offset = nextofs;
 		data += temp;
 		page++;
-- 
2.44.0.701.g2cf7baacf3.dirty

[PATCH v3 10/17] ALSA: emu10k1: fix playback of 8-bit wavetable samples

[Oswald Buddenhagen]

Samples are byte-sized in this mode, and thus the offset calculation
needs no shifting.

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
 sound/pci/emu10k1/emu10k1_callback.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/sound/pci/emu10k1/emu10k1_callback.c b/sound/pci/emu10k1/emu10k1_callback.c
index 941bfbf812ed..5f6c47cbb809 100644
--- a/sound/pci/emu10k1/emu10k1_callback.c
+++ b/sound/pci/emu10k1/emu10k1_callback.c
@@ -310,27 +310,29 @@ start_voice(struct snd_emux_voice *vp)
 {
 	unsigned int temp;
 	int ch;
+	bool w_16;
 	u32 psst, dsl, map, ccca, vtarget;
 	unsigned int addr, mapped_offset;
 	struct snd_midi_channel *chan;
 	struct snd_emu10k1 *hw;
 	struct snd_emu10k1_memblk *emem;
 	
 	hw = vp->hw;
 	ch = vp->ch;
 	if (snd_BUG_ON(ch < 0))
 		return -EINVAL;
 	chan = vp->chan;
+	w_16 = !(vp->reg.sample_mode & SNDRV_SFNT_SAMPLE_8BITS);
 
 	emem = (struct snd_emu10k1_memblk *)vp->block;
 	if (emem == NULL)
 		return -EINVAL;
 	emem->map_locked++;
 	if (snd_emu10k1_memblk_map(hw, emem) < 0) {
 		/* dev_err(hw->card->devK, "emu: cannot map!\n"); */
 		return -ENOMEM;
 	}
-	mapped_offset = snd_emu10k1_memblk_offset(emem) >> 1;
+	mapped_offset = snd_emu10k1_memblk_offset(emem) >> w_16;
 	vp->reg.start += mapped_offset;
 	vp->reg.end += mapped_offset;
 	vp->reg.loopstart += mapped_offset;
@@ -371,7 +373,7 @@ start_voice(struct snd_emux_voice *vp)
 		unsigned int shift = (vp->apitch - 0xe000) >> 10;
 		ccca |= shift << 25;
 	}
-	if (vp->reg.sample_mode & SNDRV_SFNT_SAMPLE_8BITS)
+	if (!w_16)
 		ccca |= CCCA_8BITSELECT;
 
 	vtarget = (unsigned int)vp->vtarget << 16;
-- 
2.44.0.701.g2cf7baacf3.dirty

[PATCH v3 11/17] ALSA: emu10k1: merge conditions in patch loader

[Oswald Buddenhagen]

This de-duplicates the code slightly. But the real reason is that it
moves the code up, which the next patch will depend on.

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
 sound/pci/emu10k1/emu10k1_patch.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/sound/pci/emu10k1/emu10k1_patch.c b/sound/pci/emu10k1/emu10k1_patch.c
index eb3d1ef8a33a..281881f7d0a4 100644
--- a/sound/pci/emu10k1/emu10k1_patch.c
+++ b/sound/pci/emu10k1/emu10k1_patch.c
@@ -53,8 +53,14 @@ snd_emu10k1_sample_new(struct snd_emux *rec, struct snd_sf_sample *sp,
 
 	/* compute true data size to be loaded */
 	truesize = sp->v.size + BLANK_HEAD_SIZE;
-	if (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_NO_BLANK)
+	if (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_NO_BLANK) {
 		truesize += BLANK_LOOP_SIZE;
+		/* if no blank loop is attached in the sample, add it */
+		if (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_SINGLESHOT) {
+			sp->v.loopstart = sp->v.end + BLANK_LOOP_START;
+			sp->v.loopend = sp->v.end + BLANK_LOOP_END;
+		}
+	}
 
 	/* try to allocate a memory block */
 	blocksize = truesize;
@@ -93,14 +99,6 @@ snd_emu10k1_sample_new(struct snd_emux *rec, struct snd_sf_sample *sp,
 	if (offset < blocksize)
 		snd_emu10k1_synth_memset(emu, sp->block, offset, blocksize - offset, fill);
 
-	if (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_NO_BLANK) {
-		/* if no blank loop is attached in the sample, add it */
-		if (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_SINGLESHOT) {
-			sp->v.loopstart = sp->v.end + BLANK_LOOP_START;
-			sp->v.loopend = sp->v.end + BLANK_LOOP_END;
-		}
-	}
-
 	/* recalculate offset */
 	start_addr = BLANK_HEAD_SIZE * 2;
 	if (! (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_8BITS))
-- 
2.44.0.701.g2cf7baacf3.dirty

[PATCH v3 12/17] ALSA: emu10k1: fix wavetable offset recalculation

[Oswald Buddenhagen]

The offsets are counted in samples, not in bytes.

While the code block is being rewritten, also move it up a bit, to avoid
churn in a subsequent patch.

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
 sound/pci/emu10k1/emu10k1_patch.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/sound/pci/emu10k1/emu10k1_patch.c b/sound/pci/emu10k1/emu10k1_patch.c
index 281881f7d0a4..ad16de99b800 100644
--- a/sound/pci/emu10k1/emu10k1_patch.c
+++ b/sound/pci/emu10k1/emu10k1_patch.c
@@ -30,7 +30,6 @@ snd_emu10k1_sample_new(struct snd_emux *rec, struct snd_sf_sample *sp,
 	u32 xor;
 	int offset;
 	int truesize, size, blocksize;
-	unsigned int start_addr;
 	struct snd_emu10k1 *emu;
 
 	emu = rec->hw;
@@ -62,6 +61,12 @@ snd_emu10k1_sample_new(struct snd_emux *rec, struct snd_sf_sample *sp,
 		}
 	}
 
+	/* recalculate offset */
+	sp->v.start += BLANK_HEAD_SIZE;
+	sp->v.end += BLANK_HEAD_SIZE;
+	sp->v.loopstart += BLANK_HEAD_SIZE;
+	sp->v.loopend += BLANK_HEAD_SIZE;
+
 	/* try to allocate a memory block */
 	blocksize = truesize;
 	if (! (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_8BITS))
@@ -99,15 +104,6 @@ snd_emu10k1_sample_new(struct snd_emux *rec, struct snd_sf_sample *sp,
 	if (offset < blocksize)
 		snd_emu10k1_synth_memset(emu, sp->block, offset, blocksize - offset, fill);
 
-	/* recalculate offset */
-	start_addr = BLANK_HEAD_SIZE * 2;
-	if (! (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_8BITS))
-		start_addr >>= 1;
-	sp->v.start += start_addr;
-	sp->v.end += start_addr;
-	sp->v.loopstart += start_addr;
-	sp->v.loopend += start_addr;
-
 	return 0;
 }
 
-- 
2.44.0.701.g2cf7baacf3.dirty

[PATCH v3 13/17] ALSA: emu10k1: de-duplicate size calculations for 16-bit samples

[Oswald Buddenhagen]

Instead of repeatedly checking the sample width, assign a size shift
centrally.

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
 sound/pci/emu10k1/emu10k1_patch.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/sound/pci/emu10k1/emu10k1_patch.c b/sound/pci/emu10k1/emu10k1_patch.c
index ad16de99b800..481fe03fef4d 100644
--- a/sound/pci/emu10k1/emu10k1_patch.c
+++ b/sound/pci/emu10k1/emu10k1_patch.c
@@ -28,6 +28,7 @@ snd_emu10k1_sample_new(struct snd_emux *rec, struct snd_sf_sample *sp,
 {
 	u8 fill;
 	u32 xor;
+	int shift;
 	int offset;
 	int truesize, size, blocksize;
 	struct snd_emu10k1 *emu;
@@ -43,9 +44,11 @@ snd_emu10k1_sample_new(struct snd_emux *rec, struct snd_sf_sample *sp,
 	}
 
 	if (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_8BITS) {
+		shift = 0;
 		fill = 0x80;
 		xor = (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_UNSIGNED) ? 0 : 0x80808080;
 	} else {
+		shift = 1;
 		fill = 0;
 		xor = (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_UNSIGNED) ? 0x80008000 : 0;
 	}
@@ -68,9 +71,7 @@ snd_emu10k1_sample_new(struct snd_emux *rec, struct snd_sf_sample *sp,
 	sp->v.loopend += BLANK_HEAD_SIZE;
 
 	/* try to allocate a memory block */
-	blocksize = truesize;
-	if (! (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_8BITS))
-		blocksize *= 2;
+	blocksize = truesize << shift;
 	sp->block = snd_emu10k1_synth_alloc(emu, blocksize);
 	if (sp->block == NULL) {
 		dev_dbg(emu->card->dev,
@@ -83,16 +84,12 @@ snd_emu10k1_sample_new(struct snd_emux *rec, struct snd_sf_sample *sp,
 
 	/* write blank samples at head */
 	offset = 0;
-	size = BLANK_HEAD_SIZE;
-	if (! (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_8BITS))
-		size *= 2;
+	size = BLANK_HEAD_SIZE << shift;
 	snd_emu10k1_synth_memset(emu, sp->block, offset, size, fill);
 	offset += size;
 
 	/* copy provided samples */
-	size = sp->v.size;
-	if (! (sp->v.mode_flags & SNDRV_SFNT_SAMPLE_8BITS))
-		size *= 2;
+	size = sp->v.size << shift;
 	if (snd_emu10k1_synth_copy_from_user(emu, sp->block, offset, data, size, xor)) {
 		snd_emu10k1_synth_free(emu, sp->block);
 		sp->block = NULL;
-- 
2.44.0.701.g2cf7baacf3.dirty

[PATCH v3 15/17] ALSA: emu10k1: fix wavetable playback position and caching, take 2

[Oswald Buddenhagen]

Compensate for the cache lag of 64 frames, and actually populate the
cache. Without these, the playback would start with garbage (which
would be (mostly?) masqueraded by the note's attack phase).

Note that we set the starting address only 61 frames ahead, to
compensate for the interpolator's epsilon. Unlike for PCM playback, we
don't even need to manually silence-fill the first frames in the cache,
because we insert some silence in front of each sample anyway.

A challenge are extremely short samples with a loop end below the cache
size, because a) we'd have to wrap the current address to be within the
loop and b) automatic pre-filling of the cache with the right data does
not work in this case.

We could pre-fill the cache manually, but that's slow, requires
additional code for each sample width, and is made even more complex by
the driver's virtual address space having no contiguous mapping for the
CPU.

We could have the engine fill the cache piece-wise (which is really what
happens when playback is running), but that would also be complex, and
we'd need to wait for the engine to handle each piece, so it wouldn't be
that much faster than the manual fill.

For the case of requiring only one loop iteration prior to reaching the
cache size, we could leverage the engine's looping mechanism around
CCR_CACHELOOPFLAG, but this special case doesn't seem worth the
complexity.

So we just unroll the loop as far as necessary to be able to play back
the sample without any fiddling.

Pedantically, this would be incorrect for loop-until-release samples
with a low loop end which are released very quickly, but that would be
relatively harmless, is not a plausible use case in the first place, and
SoundFont sample mode 3 isn't actually implemented anyway (it's
conflated with mode 1, infinite looping).

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
 sound/pci/emu10k1/emu10k1_callback.c |  7 ++--
 sound/pci/emu10k1/emu10k1_patch.c    | 53 +++++++++++++++++++++++++---
 2 files changed, 53 insertions(+), 7 deletions(-)

diff --git a/sound/pci/emu10k1/emu10k1_callback.c b/sound/pci/emu10k1/emu10k1_callback.c
index 5f6c47cbb809..ef26e4d3e2a3 100644
--- a/sound/pci/emu10k1/emu10k1_callback.c
+++ b/sound/pci/emu10k1/emu10k1_callback.c
@@ -255,7 +255,7 @@ lookup_voices(struct snd_emux *emu, struct snd_emu10k1 *hw,
 		/* check if sample is finished playing (non-looping only) */
 		if (bp != best + V_OFF && bp != best + V_FREE &&
 		    (vp->reg.sample_mode & SNDRV_SFNT_SAMPLE_SINGLESHOT)) {
-			val = snd_emu10k1_ptr_read(hw, CCCA_CURRADDR, vp->ch);
+			val = snd_emu10k1_ptr_read(hw, CCCA_CURRADDR, vp->ch) - 64 + 3;
 			if (val >= vp->reg.loopstart)
 				bp = best + V_OFF;
 		}
@@ -364,7 +364,7 @@ start_voice(struct snd_emux_voice *vp)
 
 	map = (hw->silent_page.addr << hw->address_mode) | (hw->address_mode ? MAP_PTI_MASK1 : MAP_PTI_MASK0);
 
-	addr = vp->reg.start;
+	addr = vp->reg.start + 64 - 3;
 	temp = vp->reg.parm.filterQ;
 	ccca = (temp << 28) | addr;
 	if (vp->apitch < 0xe400)
@@ -432,6 +432,9 @@ start_voice(struct snd_emux_voice *vp)
 		/* Q & current address (Q 4bit value, MSB) */
 		CCCA, ccca,
 
+		/* cache */
+		CCR, REG_VAL_PUT(CCR_CACHEINVALIDSIZE, 64),
+
 		/* reset volume */
 		VTFT, vtarget | vp->ftarget,
 		CVCF, vtarget | CVCF_CURRENTFILTER_MASK,
diff --git a/sound/pci/emu10k1/emu10k1_patch.c b/sound/pci/emu10k1/emu10k1_patch.c
index 481fe03fef4d..2a13fb32c1d2 100644
--- a/sound/pci/emu10k1/emu10k1_patch.c
+++ b/sound/pci/emu10k1/emu10k1_patch.c
@@ -31,6 +31,7 @@ snd_emu10k1_sample_new(struct snd_emux *rec, struct snd_sf_sample *sp,
 	int shift;
 	int offset;
 	int truesize, size, blocksize;
+	int loop_start, loop_end, loop_size, data_end, unroll;
 	struct snd_emu10k1 *emu;
 
 	emu = rec->hw;
@@ -64,12 +65,35 @@ snd_emu10k1_sample_new(struct snd_emux *rec, struct snd_sf_sample *sp,
 		}
 	}
 
+	loop_start = sp->v.loopstart;
+	loop_end = sp->v.loopend;
+	loop_size = loop_end - loop_start;
+	if (!loop_size)
+		return -EINVAL;
+	data_end = sp->v.end;
+
 	/* recalculate offset */
 	sp->v.start += BLANK_HEAD_SIZE;
 	sp->v.end += BLANK_HEAD_SIZE;
 	sp->v.loopstart += BLANK_HEAD_SIZE;
 	sp->v.loopend += BLANK_HEAD_SIZE;
 
+	// Automatic pre-filling of the cache does not work in the presence
+	// of loops (*), and we don't want to fill it manually, as that is
+	// fiddly and slow. So we unroll the loop until the loop end is
+	// beyond the cache size.
+	// (*) Strictly speaking, a single iteration is supported (that's
+	// how it works when the playback engine runs), but handling this
+	// special case is not worth it.
+	unroll = 0;
+	while (sp->v.loopend < 64) {
+		truesize += loop_size;
+		sp->v.loopstart += loop_size;
+		sp->v.loopend += loop_size;
+		sp->v.end += loop_size;
+		unroll++;
+	}
+
 	/* try to allocate a memory block */
 	blocksize = truesize << shift;
 	sp->block = snd_emu10k1_synth_alloc(emu, blocksize);
@@ -89,19 +113,38 @@ snd_emu10k1_sample_new(struct snd_emux *rec, struct snd_sf_sample *sp,
 	offset += size;
 
 	/* copy provided samples */
-	size = sp->v.size << shift;
-	if (snd_emu10k1_synth_copy_from_user(emu, sp->block, offset, data, size, xor)) {
-		snd_emu10k1_synth_free(emu, sp->block);
-		sp->block = NULL;
-		return -EFAULT;
+	if (unroll && loop_end <= data_end) {
+		size = loop_end << shift;
+		if (snd_emu10k1_synth_copy_from_user(emu, sp->block, offset, data, size, xor))
+			goto faulty;
+		offset += size;
+
+		data += loop_start << shift;
+		while (--unroll > 0) {
+			size = loop_size << shift;
+			if (snd_emu10k1_synth_copy_from_user(emu, sp->block, offset, data, size, xor))
+				goto faulty;
+			offset += size;
+		}
+
+		size = (data_end - loop_start) << shift;
+	} else {
+		size = data_end << shift;
 	}
+	if (snd_emu10k1_synth_copy_from_user(emu, sp->block, offset, data, size, xor))
+		goto faulty;
 	offset += size;
 
 	/* clear rest of samples (if any) */
 	if (offset < blocksize)
 		snd_emu10k1_synth_memset(emu, sp->block, offset, blocksize - offset, fill);
 
 	return 0;
+
+faulty:
+	snd_emu10k1_synth_free(emu, sp->block);
+	sp->block = NULL;
+	return -EFAULT;
 }
 
 /*
-- 
2.44.0.701.g2cf7baacf3.dirty

[PATCH v3 16/17] ALSA: emu10k1: shrink blank space in front of wavetable samples

[Oswald Buddenhagen]

There is no need for it to be 32 samples - 3 will do just fine (which is
the interpolator's epsilon). The old size was presumably meant to
compensate for the cache's presence, but we're now handling that
properly.

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
 sound/pci/emu10k1/emu10k1_patch.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/pci/emu10k1/emu10k1_patch.c b/sound/pci/emu10k1/emu10k1_patch.c
index 2a13fb32c1d2..dbfa89435ac2 100644
--- a/sound/pci/emu10k1/emu10k1_patch.c
+++ b/sound/pci/emu10k1/emu10k1_patch.c
@@ -16,7 +16,7 @@
 #define BLANK_LOOP_START	4
 #define BLANK_LOOP_END		8
 #define BLANK_LOOP_SIZE		12
-#define BLANK_HEAD_SIZE		32
+#define BLANK_HEAD_SIZE		3
 
 /*
  * allocate a sample block and copy data from userspace
-- 
2.44.0.701.g2cf7baacf3.dirty

[PATCH v3 17/17] ALSA: emux: simplify snd_sf_list.callback handling

[Oswald Buddenhagen]

Both drivers provide both sample_new and sample_free, and it makes no
sense to pretend that they could not. In fact, load_data() would already
crash if sample_new was null. So remove the remaining null checks.

Contrary to that, the emu10k1 driver actually has a null sample_reset,
though I'm not convinced that this inconsistency is justified.

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
 sound/synth/emux/emux.c      |  6 ++----
 sound/synth/emux/soundfont.c | 12 +++++-------
 2 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/sound/synth/emux/emux.c b/sound/synth/emux/emux.c
index a82af9374852..01444fc960d0 100644
--- a/sound/synth/emux/emux.c
+++ b/sound/synth/emux/emux.c
@@ -94,10 +94,8 @@ int snd_emux_register(struct snd_emux *emu, struct snd_card *card, int index, ch
 	/* create soundfont list */
 	memset(&sf_cb, 0, sizeof(sf_cb));
 	sf_cb.private_data = emu;
-	if (emu->ops.sample_new)
-		sf_cb.sample_new = sf_sample_new;
-	if (emu->ops.sample_free)
-		sf_cb.sample_free = sf_sample_free;
+	sf_cb.sample_new = sf_sample_new;
+	sf_cb.sample_free = sf_sample_free;
 	if (emu->ops.sample_reset)
 		sf_cb.sample_reset = sf_sample_reset;
 	emu->sflist = snd_sf_new(&sf_cb, emu->memhdr);
diff --git a/sound/synth/emux/soundfont.c b/sound/synth/emux/soundfont.c
index 4edc693da8e7..2373ed580bf8 100644
--- a/sound/synth/emux/soundfont.c
+++ b/sound/synth/emux/soundfont.c
@@ -1051,7 +1051,7 @@ load_guspatch(struct snd_sf_list *sflist, const char __user *data, long count)
 	/*
 	 * load wave data
 	 */
-	if (smp->v.size > 0 && sflist->callback.sample_new) {
+	if (smp->v.size > 0) {
 		rc = sflist->callback.sample_new
 			(sflist->callback.private_data, smp, sflist->memhdr,
 			 data, count);
@@ -1416,9 +1416,8 @@ snd_sf_clear(struct snd_sf_list *sflist)
 		}
 		for (sp = sf->samples; sp; sp = nextsp) {
 			nextsp = sp->next;
-			if (sflist->callback.sample_free)
-				sflist->callback.sample_free(sflist->callback.private_data,
-							     sp, sflist->memhdr);
+			sflist->callback.sample_free(sflist->callback.private_data,
+						     sp, sflist->memhdr);
 			kfree(sp);
 		}
 		kfree(sf);
@@ -1520,9 +1519,8 @@ snd_soundfont_remove_unlocked(struct snd_sf_list *sflist)
 			nextsp = sp->next;
 			sf->samples = nextsp;
 			sflist->mem_used -= sp->v.truesize;
-			if (sflist->callback.sample_free)
-				sflist->callback.sample_free(sflist->callback.private_data,
-							     sp, sflist->memhdr);
+			sflist->callback.sample_free(sflist->callback.private_data,
+						     sp, sflist->memhdr);
 			kfree(sp);
 		}
 	}
-- 
2.44.0.701.g2cf7baacf3.dirty

Re: [PATCH v3 00/17] ALSA: emu10k1 & emux: fixes related to wavetable playback

[Takashi Iwai]

On Sat, 06 Apr 2024 08:48:13 +0200,
Oswald Buddenhagen wrote:
> 
> ---
> v3:
> - re-send with safer transfer encoding
> v2:
> - rebased on top of reverting problematic commit first
> 
> Oswald Buddenhagen (17):
>   ALSA: emux: fix /proc teardown at module unload
>   ALSA: emux: prune unused parameter from snd_soundfont_load_guspatch()
>   ALSA: emux: fix validation of snd_emux.num_ports
>   ALSA: emux: fix init of patch_info.truesize in load_data()
>   ALSA: emu10k1: prune vestiges of
>     SNDRV_SFNT_SAMPLE_{BIDIR,REVERSE}_LOOP support
>   ALSA: emux: centralize & improve patch info validation
>   ALSA: emux: improve patch ioctl data validation
>   ALSA: emu10k1: move patch loader assertions into low-level functions
>   ALSA: emu10k1: fix sample signedness issues in wavetable loader
>   ALSA: emu10k1: fix playback of 8-bit wavetable samples
>   ALSA: emu10k1: merge conditions in patch loader
>   ALSA: emu10k1: fix wavetable offset recalculation
>   ALSA: emu10k1: de-duplicate size calculations for 16-bit samples
>   ALSA: emu10k1: improve cache behavior documentation
>   ALSA: emu10k1: fix wavetable playback position and caching, take 2
>   ALSA: emu10k1: shrink blank space in front of wavetable samples
>   ALSA: emux: simplify snd_sf_list.callback handling

Applied to for-next branch now.

At the next time, please send to linux-sound@vger.kernel.org instead,
though, as listed in MAINTAINERS file.


thanks,

Takashi

Re: [PATCH v3 00/17] ALSA: emu10k1 & emux: fixes related to wavetable playback

[Arthur Marsh]

I have applied these patches and the playback of MIDI files appears to be fine.

Regards,

Arthur Marsh. 
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.